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

[Junio C Hamano <gitster@pobox.com>]

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