From bd0b42aed3084bf66557485fd7d87e975a4f6d4e Mon Sep 17 00:00:00 2001 From: Jonathan Tan Date: Thu, 10 Jan 2019 11:36:45 -0800 Subject: [PATCH] fetch-pack: do not take shallow lock unnecessarily When fetching using protocol v2, the remote may send a "shallow-info" section if the client is shallow. If so, Git as the client currently takes the shallow file lock, even if the "shallow-info" section is empty. This is not a problem except that Git does not support taking the shallow file lock after modifying the shallow file, because is_repository_shallow() stores information that is never cleared. And this take-after-modify occurs when Git does a tag-following fetch from a shallow repository on a transport that does not support tag following (since in this case, 2 fetches are performed). To solve this issue, take the shallow file lock (and perform all other shallow processing) only if the "shallow-info" section is non-empty; otherwise, behave as if it were empty. A full solution (probably, ensuring that any action of committing shallow file locks also includes clearing the information stored by is_repository_shallow()) would solve the issue without need for this patch, but this patch is independently useful (as an optimization to prevent writing a file in an unnecessary case), hence why I wrote it. I have included a NEEDSWORK outlining the full solution. Signed-off-by: Jonathan Tan Signed-off-by: Junio C Hamano --- fetch-pack.c | 11 +++++++++-- shallow.c | 7 +++++++ t/t5702-protocol-v2.sh | 18 ++++++++++++++++++ 3 files changed, 34 insertions(+), 2 deletions(-) diff --git a/fetch-pack.c b/fetch-pack.c index dd6700bda9240f..5885623eceb0de 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -1232,6 +1232,8 @@ static int process_acks(struct fetch_negotiator *negotiator, static void receive_shallow_info(struct fetch_pack_args *args, struct packet_reader *reader) { + int line_received = 0; + process_section_header(reader, "shallow-info", 0); while (packet_reader_read(reader) == PACKET_READ_NORMAL) { const char *arg; @@ -1241,6 +1243,7 @@ static void receive_shallow_info(struct fetch_pack_args *args, if (get_oid_hex(arg, &oid)) die(_("invalid shallow line: %s"), reader->line); register_shallow(the_repository, &oid); + line_received = 1; continue; } if (skip_prefix(reader->line, "unshallow ", &arg)) { @@ -1253,6 +1256,7 @@ static void receive_shallow_info(struct fetch_pack_args *args, die(_("error in object: %s"), reader->line); if (unregister_shallow(&oid)) die(_("no shallow found: %s"), reader->line); + line_received = 1; continue; } die(_("expected shallow/unshallow, got %s"), reader->line); @@ -1262,8 +1266,11 @@ static void receive_shallow_info(struct fetch_pack_args *args, reader->status != PACKET_READ_DELIM) die(_("error processing shallow info: %d"), reader->status); - setup_alternate_shallow(&shallow_lock, &alternate_shallow_file, NULL); - args->deepen = 1; + if (line_received) { + setup_alternate_shallow(&shallow_lock, &alternate_shallow_file, + NULL); + args->deepen = 1; + } } static void receive_wanted_refs(struct packet_reader *reader, diff --git a/shallow.c b/shallow.c index 02fdbfc554c462..ce45297940d417 100644 --- a/shallow.c +++ b/shallow.c @@ -43,6 +43,13 @@ int register_shallow(struct repository *r, const struct object_id *oid) int is_repository_shallow(struct repository *r) { + /* + * NEEDSWORK: This function updates + * r->parsed_objects->{is_shallow,shallow_stat} as a side effect but + * there is no corresponding function to clear them when the shallow + * file is updated. + */ + FILE *fp; char buf[1024]; const char *path = r->parsed_objects->alternate_shallow_file; diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh index 0f2b09ebb8d662..fd164d414dad9c 100755 --- a/t/t5702-protocol-v2.sh +++ b/t/t5702-protocol-v2.sh @@ -471,6 +471,24 @@ test_expect_success 'upload-pack respects client shallows' ' grep "fetch< version 2" trace ' +test_expect_success 'ensure that multiple fetches in same process from a shallow repo works' ' + rm -rf server client trace && + + test_create_repo server && + test_commit -C server one && + test_commit -C server two && + test_commit -C server three && + git clone --shallow-exclude two "file://$(pwd)/server" client && + + git -C server tag -a -m "an annotated tag" twotag two && + + # Triggers tag following (thus, 2 fetches in one process) + GIT_TRACE_PACKET="$(pwd)/trace" git -C client -c protocol.version=2 \ + fetch --shallow-exclude one origin && + # Ensure that protocol v2 is used + grep "fetch< version 2" trace +' + # Test protocol v2 with 'http://' transport # . "$TEST_DIRECTORY"/lib-httpd.sh