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

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

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