patchFix upload_pack_v2 response ordering for shallow fetch when server has uploadpack.allowRefInWant=true

7 messages between Sep 15, 2026 and Sep 17, 2026, from Royce Remer, Junio C Hamano, Royce Gerard Remer.

Plain Markdown or JSON for tools and agents. Diffs are folded; open one to read it.

Royce RemerSep 15, 2026, 19:30 UTC on lore

Signed-off-by: Royce Remer <royceremer@gmail.com>
---
 t/t5703-upload-pack-ref-in-want.sh | 18 ++++++++++++++++++
 upload-pack.c                      |  2 +-
 2 files changed, 19 insertions(+), 1 deletion(-)

Show changes to 2 files +19 −1

t/t5703-upload-pack-ref-in-want.sh, upload-pack.c

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 HamanoSep 15, 2026, 19:58 UTC in reply to Royce Remer on lore

Re: [PATCH] [PATCH] Fix upload_pack_v2 response ordering for shallow fetch when server has uploadpack.allowRefInWant=true

Royce Remer <royceremer@gmail.com> writes:

> Signed-off-by: Royce Remer <royceremer@gmail.com>
> ---

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.

Show 47 quoted lines

>  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 HamanoSep 15, 2026, 20:19 UTC in reply to Junio C Hamano on lore

Re: [PATCH] [PATCH] Fix upload_pack_v2 response ordering for shallow fetch when server has uploadpack.allowRefInWant=true

Junio C Hamano <gitster@pobox.com> writes:

Show 14 quoted lines

>> 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 RemerSep 15, 2026, 21:03 UTC in reply to Junio C Hamano on lore

Re: [PATCH] [PATCH] Fix upload_pack_v2 response ordering for shallow fetch when server has uploadpack.allowRefInWant=true

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 <gitster@pobox.com> wrote:

Show 56 quoted lines

>
> Junio C Hamano <gitster@pobox.com> 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 HamanoSep 16, 2026, 16:43 UTC in reply to Royce Gerard Remer on lore

Re: [PATCH] [PATCH] Fix upload_pack_v2 response ordering for shallow fetch when server has uploadpack.allowRefInWant=true

Royce Gerard Remer <royceremer@gmail.com> writes:

Show 37 quoted lines

> 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 RemerSep 16, 2026, 20:32 UTC in reply to Royce Remer on lore

[PATCH v2] upload-pack: swap wanted-ref/shallow-info responses

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 <royceremer@gmail.com>
---
 t/t5703-upload-pack-ref-in-want.sh | 18 ++++++++++++++++++
 upload-pack.c                      |  2 +-
 2 files changed, 19 insertions(+), 1 deletion(-)

Show changes to 2 files +19 −1

t/t5703-upload-pack-ref-in-want.sh, upload-pack.c

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 HamanoSep 17, 2026, 06:20 UTC in reply to Royce Remer on lore

Re: [PATCH v2] upload-pack: swap wanted-ref/shallow-info responses

Royce Remer <royceremer@gmail.com> writes:

Show 14 quoted lines

> 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.

Show 52 quoted lines

>
> Signed-off-by: Royce Remer <royceremer@gmail.com>
> ---
>  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