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

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

[Royce Remer]

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);
-- 
2.34.1

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

[Junio C Hamano]

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.

>  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);

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

[Junio C Hamano]

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.

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

[Royce Gerard Remer]

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

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

[Junio C Hamano]

Royce Gerard Remer <royceremer@gmail.com> 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.

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

[Royce Remer]

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(-)

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

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

[Junio C Hamano]

Royce Remer <royceremer@gmail.com> 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 <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