While investigating t5537's failure revealed by Aevar's GIT_TEST_PROTOCOL_VERSION patches [1], I found an answer to my confusion that I described in [2]. Quoting the relevant part: > I'm also not sure why this issue only occurs with protocol v2. It's true > that, when using protocol v0, the server can communicate shallow > information both before and after "want"s are received, and if shallow > communication is only communicated before, the client will invoke > setup_temporary_shallow() instead of setup_alternate_shallow(). (In > protocol v2, shallow information is always communicated after "want"s, > thus demonstrating the issue.) But even in protocol v0, writes to the > shallow file go through setup_alternate_shallow() anyway (in > update_shallow()), so I would think that the issue would occur, but it > doesn't. It turns out that update_shallow() doesn't write pre-"want" shallow information unless --update-shallow is set. But post-"want" shallow information is always written regardless of --update-shallow. (So maybe --update-shallow is not the best name for it, but that is another issue.) Which raises the question: in protocol v2, all shallow information is post-"want", so how should we handle this? My current inclination is to treat them like v0 pre-"want" information if no depth-like argument is given by the user (that is, args->deepen is 0), and like v0 post-"want" information otherwise. (Right now, it is treated always like the latter.) This patch does it by delaying initialization of the struct shallow_info until we fully know what's going on, and not only fixes the bug revealed by t5537, but the bug that [2] fixes as well (eliminating the need for [2]). This does mean that there will be some difference in functionality between v0 and v2: if fetching from a shallow repository with our own depth arguments, v0 would write shallows resulting from our own depth arguments but not those that are there because the remote is shallow; v2 would write all shallows. But this is the best solution I can think of so far. Any thoughts? The code can be written more clearly but I wanted to discuss the design first. [1] https://public-inbox.org/git/20181213155817.27666-1-avarab@xxxxxxxxx/ [2] https://public-inbox.org/git/20181218010811.143608-1-jonathantanmy@xxxxxxxxxx/ Signed-off-by: Jonathan Tan <jonathantanmy@xxxxxxxxxx> --- fetch-pack.c | 34 ++++++++++++++++++++++++++-------- t/t5702-protocol-v2.sh | 16 ++++++++++++++++ 2 files changed, 42 insertions(+), 8 deletions(-) diff --git a/fetch-pack.c b/fetch-pack.c index 9691046e64..0a89a210b0 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -1265,8 +1265,11 @@ static int process_acks(struct fetch_negotiator *negotiator, } static void receive_shallow_info(struct fetch_pack_args *args, - struct packet_reader *reader) + struct packet_reader *reader, + struct shallow_info *si) { + struct oid_array shallows = OID_ARRAY_INIT; + process_section_header(reader, "shallow-info", 0); while (packet_reader_read(reader) == PACKET_READ_NORMAL) { const char *arg; @@ -1275,7 +1278,9 @@ static void receive_shallow_info(struct fetch_pack_args *args, if (skip_prefix(reader->line, "shallow ", &arg)) { if (get_oid_hex(arg, &oid)) die(_("invalid shallow line: %s"), reader->line); - register_shallow(the_repository, &oid); + if (args->deepen) + register_shallow(the_repository, &oid); + oid_array_append(&shallows, &oid); continue; } if (skip_prefix(reader->line, "unshallow ", &arg)) { @@ -1297,8 +1302,17 @@ 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 (args->deepen) { + prepare_shallow_info(si, NULL); + setup_alternate_shallow(&shallow_lock, &alternate_shallow_file, + NULL); + } else { + prepare_shallow_info(si, &shallows); + if (si->nr_ours || si->nr_theirs) + alternate_shallow_file = setup_temporary_shallow(si->shallow); + else + alternate_shallow_file = NULL; + } } static void receive_wanted_refs(struct packet_reader *reader, @@ -1340,6 +1354,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args, int fd[2], const struct ref *orig_ref, struct ref **sought, int nr_sought, + struct shallow_info *si, char **pack_lockfile) { struct ref *ref = copy_ref_list(orig_ref); @@ -1407,7 +1422,9 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args, case FETCH_GET_PACK: /* Check for shallow-info section */ if (process_section_header(&reader, "shallow-info", 1)) - receive_shallow_info(args, &reader); + receive_shallow_info(args, &reader, si); + else + prepare_shallow_info(si, NULL); if (process_section_header(&reader, "wanted-refs", 1)) receive_wanted_refs(&reader, sought, nr_sought); @@ -1641,13 +1658,14 @@ struct ref *fetch_pack(struct fetch_pack_args *args, packet_flush(fd[1]); die(_("no matching remote head")); } - prepare_shallow_info(&si, shallow); if (version == protocol_v2) ref_cpy = do_fetch_pack_v2(args, fd, ref, sought, nr_sought, - pack_lockfile); - else + &si, pack_lockfile); + else { + prepare_shallow_info(&si, shallow); ref_cpy = do_fetch_pack(args, fd, ref, sought, nr_sought, &si, pack_lockfile); + } reprepare_packed_git(the_repository); if (!args->cloning && args->deepen) { diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh index 0f2b09ebb8..390a71399d 100755 --- a/t/t5702-protocol-v2.sh +++ b/t/t5702-protocol-v2.sh @@ -471,6 +471,22 @@ test_expect_success 'upload-pack respects client shallows' ' grep "fetch< version 2" trace ' +test_expect_success '2 fetches in one process updating shallow' ' + 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 && + + # 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 -- 2.19.0.271.gfe8321ec05.dirty
