# [PATCH] [PATCH] Fix upload_pack_v2 response ordering for shallow fetch when server has uploadpack.allowRefInWant=true 7 messages from 2026-09-15 to 2026-09-17. Participants: Royce Remer, Junio C Hamano, Royce Gerard Remer. Thread: https://gitlist.dev/t/66334 ## Royce Remer, 2026-09-15 19:30 Subject: [PATCH] [PATCH] Fix upload_pack_v2 response ordering for shallow fetch when server has uploadpack.allowRefInWant=true Message-ID: <20260915193009.222678-1-royceremer@gmail.com> ``` Signed-off-by: Royce Remer --- t/t5703-upload-pack-ref-in-want.sh | 18 ++++++++++++++++++ upload-pack.c | 2 +- 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh index 249137b467..9e2a090c9e 100755 --- a/t/t5703-upload-pack-ref-in-want.sh +++ b/t/t5703-upload-pack-ref-in-want.sh @@ -295,6 +295,24 @@ test_expect_success 'fetching with wildcard that matches multiple refs' ' grep "want-ref refs/heads/o/bar" log ' +test_expect_success 'shallow clone with ref-in-want' ' + rm -rf local && + GIT_TEST_PROTOCOL_VERSION=2 git clone --depth=1 "file://$REPO" local && + git -C "$REPO" rev-parse main >expected && + git -C local rev-parse refs/remotes/origin/main >actual && + test_cmp expected actual && + git -C local log --oneline refs/remotes/origin/main >log && + test_line_count = 1 log +' + +test_expect_success 'incremental shallow fetch with ref-in-want' ' + rm -rf local && + GIT_TEST_PROTOCOL_VERSION=2 git clone --depth=1 "file://$REPO" local && + GIT_TEST_PROTOCOL_VERSION=2 git -C local fetch --depth=2 origin main && + git -C local log --oneline refs/remotes/origin/main >log && + test_line_count = 2 log +' + REPO="$(pwd)/repo-ns" test_expect_success 'setup namespaced repo' ' diff --git a/upload-pack.c b/upload-pack.c index a52856d869..a70d237ad3 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -1812,8 +1812,8 @@ int upload_pack_v2(struct repository *r, struct packet_reader *request) state = UPLOAD_DONE; break; case UPLOAD_SEND_PACK: - send_wanted_ref_info(&data); send_shallow_info(&data); + send_wanted_ref_info(&data); if (data.uri_protocols.nr) { create_pack_file(&data, &data.uri_protocols); -- 2.34.1 ``` ## Junio C Hamano, 2026-09-15 19:58 Subject: Re: [PATCH] [PATCH] Fix upload_pack_v2 response ordering for shallow fetch when server has uploadpack.allowRefInWant=true Message-ID: In-Reply-To: <20260915193009.222678-1-royceremer@gmail.com> ``` Royce Remer writes: > Signed-off-by: Royce Remer > --- The usual way to compose a log message of this project is to - Give an observation on how the current system works in the present tense (so no need to say "Currently X is Y", or "Previously X was Y" to describe the state before your change; just "X is Y" is enough), and discuss what you perceive as a problem in it. - Propose a solution (optional---often, problem description trivially leads to an obvious solution in reader's minds). - Give commands to somebody editing the codebase to "make it so", instead of saying "This commit does X". in this order. Also see [[describe-changes]] and especially [[summary-section]] in Documentation/SubmittingPatches. What is especially troubling in this partuclar patch is that its title claims that the change is a fix, but it does not explain why the updated behaviour is more correct than the current code. The implementations of "git fetch" and "git clone" that come with currently deployed versions of Git must be happily accepting what the current implementation of "git upload-pack" gives them (the missing proposed log message does not say it is broken in any way). If a new version of "git upload-pack" suddenly swapped the order of them, would it break existing "git fetch" and "git clone"? If not, how? There may be other questions that naturally come to reviewers' minds, and a change given in this patch must come with enough explanation to answer questions like the above. Thanks. > t/t5703-upload-pack-ref-in-want.sh | 18 ++++++++++++++++++ > upload-pack.c | 2 +- > 2 files changed, 19 insertions(+), 1 deletion(-) > > diff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh > index 249137b467..9e2a090c9e 100755 > --- a/t/t5703-upload-pack-ref-in-want.sh > +++ b/t/t5703-upload-pack-ref-in-want.sh > @@ -295,6 +295,24 @@ test_expect_success 'fetching with wildcard that matches multiple refs' ' > grep "want-ref refs/heads/o/bar" log > ' > > +test_expect_success 'shallow clone with ref-in-want' ' > + rm -rf local && > + GIT_TEST_PROTOCOL_VERSION=2 git clone --depth=1 "file://$REPO" local && > + git -C "$REPO" rev-parse main >expected && > + git -C local rev-parse refs/remotes/origin/main >actual && > + test_cmp expected actual && > + git -C local log --oneline refs/remotes/origin/main >log && > + test_line_count = 1 log > +' > + > +test_expect_success 'incremental shallow fetch with ref-in-want' ' > + rm -rf local && > + GIT_TEST_PROTOCOL_VERSION=2 git clone --depth=1 "file://$REPO" local && > + GIT_TEST_PROTOCOL_VERSION=2 git -C local fetch --depth=2 origin main && > + git -C local log --oneline refs/remotes/origin/main >log && > + test_line_count = 2 log > +' > + > REPO="$(pwd)/repo-ns" > > test_expect_success 'setup namespaced repo' ' > diff --git a/upload-pack.c b/upload-pack.c > index a52856d869..a70d237ad3 100644 > --- a/upload-pack.c > +++ b/upload-pack.c > @@ -1812,8 +1812,8 @@ int upload_pack_v2(struct repository *r, struct packet_reader *request) > state = UPLOAD_DONE; > break; > case UPLOAD_SEND_PACK: > - send_wanted_ref_info(&data); > send_shallow_info(&data); > + send_wanted_ref_info(&data); > > if (data.uri_protocols.nr) { > create_pack_file(&data, &data.uri_protocols); ``` ## Junio C Hamano, 2026-09-15 20:19 Subject: Re: [PATCH] [PATCH] Fix upload_pack_v2 response ordering for shallow fetch when server has uploadpack.allowRefInWant=true Message-ID: In-Reply-To: ``` Junio C Hamano writes: >> diff --git a/upload-pack.c b/upload-pack.c >> index a52856d869..a70d237ad3 100644 >> --- a/upload-pack.c >> +++ b/upload-pack.c >> @@ -1812,8 +1812,8 @@ int upload_pack_v2(struct repository *r, struct packet_reader *request) >> state = UPLOAD_DONE; >> break; >> case UPLOAD_SEND_PACK: >> - send_wanted_ref_info(&data); >> send_shallow_info(&data); >> + send_wanted_ref_info(&data); >> >> if (data.uri_protocols.nr) { >> create_pack_file(&data, &data.uri_protocols); I am merely guessing what your reasoning is, but is this meant to match this part of the code on the other side of the connection? case FETCH_GET_PACK: trace2_region_leave("fetch-pack", "negotiation_v2", the_repository); trace2_data_intmax("negotiation_v2", the_repository, "total_rounds", negotiation_round); /* Check for shallow-info section */ if (process_section_header(&reader, "shallow-info", 1)) receive_shallow_info(args, &reader, shallows, si); if (process_section_header(&reader, "wanted-refs", 1)) receive_wanted_refs(&reader, sought, nr_sought); These process_section_header() calls are made with the peek bit set, signaling that it is OK if the packet we are about to receive is not the one that is being checked, so what may happen is - upload-pack gives wanted-ref info and then shallow-info. - fetch-pack sees wanted-refs, notices that it is not shallow-info, ignores it, and the next process_section_header() call does notice it is wanted-refs and processes it. But then, who consumes the shallow-info? Does fetch-pack notices shallow-info that it did not expect to see and crashes? If so, that is a very noteworthy thing to say in the proposed log message. If it does not crash and goes on but without utilizing what was carried in the shallow-info packet, the resulting behaviour of fetch-pack would be different from what we would expect, and if that is the case, that difference is a noteworthy thing to decribe in the proposed log message. Thanks. ``` ## Royce Gerard Remer, 2026-09-15 21:03 Subject: Re: [PATCH] [PATCH] Fix upload_pack_v2 response ordering for shallow fetch when server has uploadpack.allowRefInWant=true Message-ID: In-Reply-To: ``` Apologies, clearly struggling with using git send-email for the first time (and thank you for the reply). Here's the missing context from my cover: On my fleet of git severs (running Gitea, although it just shells out to the git cli and uses this client verbatim), I enabled this in the upload-pack config: uploadpack.allowRefInWant=true Clients performing fetches and clones all worked as normal unless they attempted a clone with the --depth parameter, where the client would get this error: fatal: expected 'packfile', received 'shallow-info' Looking at Documentation/gitprotocol-v2.adoc, it seems like when this feature was added, the ordering was just incorrect server-side. You wouldn't notice unless: 1) the server enabled the config above (I suspect it's not a heavily used feature in the wild) 2) the client performed a fetch operation with --depth That's what the new test cases in t/t5703-upload-pack-ref-in-want.sh are, those were written to prove the failure before the fix. I've been running this in my dev fleet of servers for a day now, trying various combinations of clone, with/without --depth, and fetches with --unshallow-since . This is purely server-side to honor the existing documented contract when these two features are in use together. > If a new version of "git upload-pack" suddenly swapped the order of them, would it break existing "git fetch" and "git clone"? I think this is a question about backwards-compatibility? This combination of features appears to have never worked, so clients which were receiving failures would no longer. If servers were previously configured to advertise allowRefInWant, clients could not have shallow cloned. If they did not have this feature configured, shallow clones would work the same way (the ordering of packets is unchanged). On Tue, Sep 15, 2026 at 1:19 PM Junio C Hamano wrote: > > Junio C Hamano writes: > > >> diff --git a/upload-pack.c b/upload-pack.c > >> index a52856d869..a70d237ad3 100644 > >> --- a/upload-pack.c > >> +++ b/upload-pack.c > >> @@ -1812,8 +1812,8 @@ int upload_pack_v2(struct repository *r, struct packet_reader *request) > >> state = UPLOAD_DONE; > >> break; > >> case UPLOAD_SEND_PACK: > >> - send_wanted_ref_info(&data); > >> send_shallow_info(&data); > >> + send_wanted_ref_info(&data); > >> > >> if (data.uri_protocols.nr) { > >> create_pack_file(&data, &data.uri_protocols); > > I am merely guessing what your reasoning is, but is this meant to > match this part of the code on the other side of the connection? > > case FETCH_GET_PACK: > trace2_region_leave("fetch-pack", > "negotiation_v2", > the_repository); > trace2_data_intmax("negotiation_v2", the_repository, > "total_rounds", negotiation_round); > /* Check for shallow-info section */ > if (process_section_header(&reader, "shallow-info", 1)) > receive_shallow_info(args, &reader, shallows, si); > > if (process_section_header(&reader, "wanted-refs", 1)) > receive_wanted_refs(&reader, sought, nr_sought); > > > These process_section_header() calls are made with the peek bit set, > signaling that it is OK if the packet we are about to receive is not > the one that is being checked, so what may happen is > > - upload-pack gives wanted-ref info and then shallow-info. > > - fetch-pack sees wanted-refs, notices that it is not shallow-info, > ignores it, and the next process_section_header() call does > notice it is wanted-refs and processes it. > > But then, who consumes the shallow-info? Does fetch-pack notices > shallow-info that it did not expect to see and crashes? If so, that > is a very noteworthy thing to say in the proposed log message. If > it does not crash and goes on but without utilizing what was carried > in the shallow-info packet, the resulting behaviour of fetch-pack > would be different from what we would expect, and if that is the > case, that difference is a noteworthy thing to decribe in the > proposed log message. > > Thanks. > ``` ## Junio C Hamano, 2026-09-16 16:43 Subject: Re: [PATCH] [PATCH] Fix upload_pack_v2 response ordering for shallow fetch when server has uploadpack.allowRefInWant=true Message-ID: In-Reply-To: ``` Royce Gerard Remer writes: > Apologies, clearly struggling with using git send-email for the first > time (and thank you for the reply). Here's the missing context from my > cover: > > On my fleet of git severs (running Gitea, although it just shells out > to the git cli and uses this client verbatim), I enabled this in the > upload-pack config: > uploadpack.allowRefInWant=true > > Clients performing fetches and clones all worked as normal unless they > attempted a clone with the --depth parameter, where the client would > get this error: > fatal: expected 'packfile', received 'shallow-info' > > Looking at Documentation/gitprotocol-v2.adoc, it seems like when this > feature was added, the ordering was just incorrect server-side. You > wouldn't notice unless: > 1) the server enabled the config above (I suspect it's not a heavily > used feature in the wild) > 2) the client performed a fetch operation with --depth > > That's what the new test cases in t/t5703-upload-pack-ref-in-want.sh > are, those were written to prove the failure before the fix. I've been > running this in my dev fleet of servers for a day now, trying various > combinations of clone, with/without --depth, and fetches with > --unshallow-since . This is purely server-side to honor the existing > documented contract when these two features are in use together. > >> If a new version of "git upload-pack" suddenly swapped the order of > them, would it break existing "git fetch" and "git clone"? > > I think this is a question about backwards-compatibility? This > combination of features appears to have never worked, so clients which > were receiving failures would no longer. If servers were previously > configured to advertise allowRefInWant, clients could not have shallow > cloned. If they did not have this feature configured, shallow clones > would work the same way (the ordering of packets is unchanged). Yes, all of the above are good material to be distilled into an excellent commit log message. The way how the problematic packet sequence is produced, how the server and the client would behave and cause reliable breakage on the client, how recent the features involved in the bug are, and that apparently the combination are rarely used, which would all explain why this breakage hasn't been reported and diagnosed so far. ``` ## Royce Remer, 2026-09-16 20:32 Subject: [PATCH v2] upload-pack: swap wanted-ref/shallow-info responses Message-ID: <20260916203221.5265-1-royceremer@gmail.com> In-Reply-To: <20260915193009.222678-1-royceremer@gmail.com> ``` When a server enables uploadpack.allowRefInWant, upload_pack_v2() sends wanted-ref info before shallow-info. The fetch-pack client expects shallow-info first; receiving them out of order causes it to exit: fatal: expected 'packfile', received 'shallow-info' This error condition only applies to protocol v2 clients performs a shallow fetch (--depth) against servers with allowRefInWant configured. Swap the send order so that upload_pack_v2() sends shallow-info before wanted-ref info. This is a server-side-only change and is compatible with all existing client versions. Signed-off-by: Royce Remer --- t/t5703-upload-pack-ref-in-want.sh | 18 ++++++++++++++++++ upload-pack.c | 2 +- 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh index 249137b467..9e2a090c9e 100755 --- a/t/t5703-upload-pack-ref-in-want.sh +++ b/t/t5703-upload-pack-ref-in-want.sh @@ -295,6 +295,24 @@ test_expect_success 'fetching with wildcard that matches multiple refs' ' grep "want-ref refs/heads/o/bar" log ' +test_expect_success 'shallow clone with ref-in-want' ' + rm -rf local && + GIT_TEST_PROTOCOL_VERSION=2 git clone --depth=1 "file://$REPO" local && + git -C "$REPO" rev-parse main >expected && + git -C local rev-parse refs/remotes/origin/main >actual && + test_cmp expected actual && + git -C local log --oneline refs/remotes/origin/main >log && + test_line_count = 1 log +' + +test_expect_success 'incremental shallow fetch with ref-in-want' ' + rm -rf local && + GIT_TEST_PROTOCOL_VERSION=2 git clone --depth=1 "file://$REPO" local && + GIT_TEST_PROTOCOL_VERSION=2 git -C local fetch --depth=2 origin main && + git -C local log --oneline refs/remotes/origin/main >log && + test_line_count = 2 log +' + REPO="$(pwd)/repo-ns" test_expect_success 'setup namespaced repo' ' diff --git a/upload-pack.c b/upload-pack.c index a52856d869..a70d237ad3 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -1812,8 +1812,8 @@ int upload_pack_v2(struct repository *r, struct packet_reader *request) state = UPLOAD_DONE; break; case UPLOAD_SEND_PACK: - send_wanted_ref_info(&data); send_shallow_info(&data); + send_wanted_ref_info(&data); if (data.uri_protocols.nr) { create_pack_file(&data, &data.uri_protocols); base-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc -- 2.55.0.1.ga30d533ec0 ``` ## Junio C Hamano, 2026-09-17 06:20 Subject: Re: [PATCH v2] upload-pack: swap wanted-ref/shallow-info responses Message-ID: In-Reply-To: <20260916203221.5265-1-royceremer@gmail.com> ``` Royce Remer writes: > When a server enables uploadpack.allowRefInWant, upload_pack_v2() > sends wanted-ref info before shallow-info. The fetch-pack client > expects shallow-info first; receiving them out of order causes it > to exit: > > fatal: expected 'packfile', received 'shallow-info' > > This error condition only applies to protocol v2 clients performs > a shallow fetch (--depth) against servers with allowRefInWant > configured. > > Swap the send order so that upload_pack_v2() sends shallow-info > before wanted-ref info. This is a server-side-only change and is > compatible with all existing client versions. It seems that this bug existed in the very first set of patches that introduced the ref-in-want feature, namely, 733020517a (fetch-pack: implement ref-in-want, 2018-06-27) and 516e2b76bd (upload-pack: implement ref-in-want, 2018-06-27). There were a few changes on the code around that area, but on-the-wire protocol never changed, so it never worked correctly, but the ref-in-want feature is a rather exotic thing to want in the first place, so it is not all that unexpected. > > Signed-off-by: Royce Remer > --- > t/t5703-upload-pack-ref-in-want.sh | 18 ++++++++++++++++++ > upload-pack.c | 2 +- > 2 files changed, 19 insertions(+), 1 deletion(-) > > diff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh > index 249137b467..9e2a090c9e 100755 > --- a/t/t5703-upload-pack-ref-in-want.sh > +++ b/t/t5703-upload-pack-ref-in-want.sh > @@ -295,6 +295,24 @@ test_expect_success 'fetching with wildcard that matches multiple refs' ' > grep "want-ref refs/heads/o/bar" log > ' > > +test_expect_success 'shallow clone with ref-in-want' ' > + rm -rf local && > + GIT_TEST_PROTOCOL_VERSION=2 git clone --depth=1 "file://$REPO" local && > + git -C "$REPO" rev-parse main >expected && > + git -C local rev-parse refs/remotes/origin/main >actual && > + test_cmp expected actual && > + git -C local log --oneline refs/remotes/origin/main >log && > + test_line_count = 1 log > +' > + > +test_expect_success 'incremental shallow fetch with ref-in-want' ' > + rm -rf local && > + GIT_TEST_PROTOCOL_VERSION=2 git clone --depth=1 "file://$REPO" local && > + GIT_TEST_PROTOCOL_VERSION=2 git -C local fetch --depth=2 origin main && > + git -C local log --oneline refs/remotes/origin/main >log && > + test_line_count = 2 log > +' > + > REPO="$(pwd)/repo-ns" > > test_expect_success 'setup namespaced repo' ' > diff --git a/upload-pack.c b/upload-pack.c > index a52856d869..a70d237ad3 100644 > --- a/upload-pack.c > +++ b/upload-pack.c > @@ -1812,8 +1812,8 @@ int upload_pack_v2(struct repository *r, struct packet_reader *request) > state = UPLOAD_DONE; > break; > case UPLOAD_SEND_PACK: > - send_wanted_ref_info(&data); > send_shallow_info(&data); > + send_wanted_ref_info(&data); > > if (data.uri_protocols.nr) { > create_pack_file(&data, &data.uri_protocols); > > base-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc ```