Re: [PATCH] upload-pack: clear flags before each v2 request

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 18, 2018, 05:16 UTC
Message-ID: <xmqq5zxzvnq1.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20181016215850.47821-1-jonathantanmy@google.com>

Jonathan Tan <jonathantanmy@google.com> writes:

Show 11 quoted lines

> Suppose a server has the following commit graph:
>
>  A   B
>   \ /
>    O
>
> We create a client by cloning A from the server with depth 1, and add
> many commits to it (so that future fetches span multiple requests due to
> lengthy negotiation). If it then fetches B using protocol v2, the fetch
> spanning multiple requests, the resulting packfile does not contain O
> even though the client did report that A is shallow.

Hmph, what if commit O had a long history behind it?

Should fetching of B result in fetching the whole history? Would we notice that now all of A's parents are available locally and declare that the repository is no longer shallow?

I am trying to figure out if "does not contain O when we fetch B, even though we earlier fetched A shallowly, excluding its parents" is unconditionally a bad thing.

The change to the code itself sort-of makes sense (I say sort-of because I didn't carefully look at the callers to see if they mind getting all these flags cleared, but the ones that are cleared are the ones that are involved mostly in the negotiation and shold be OK).

Show 20 quoted lines

> diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
> index 88a886975d..70b88385ba 100755
> --- a/t/t5702-protocol-v2.sh
> +++ b/t/t5702-protocol-v2.sh
> @@ -429,6 +429,31 @@ test_expect_success 'fetch supports include-tag and tag following' '
>  	git -C client cat-file -e $(git -C client rev-parse annotated_tag)
>  '
>  
> +test_expect_success 'upload-pack respects client shallows' '
> +	rm -rf server client trace &&
> +
> +	git init server &&
> +	test_commit -C server base &&
> +	test_commit -C server client_has &&
> +
> +	git clone --depth=1 "file://$(pwd)/server" client &&
> +
> +	# Add extra commits to the client so that the whole fetch takes more
> +	# than 1 request (due to negotiation)
> +	for i in $(seq 1 32)

Use test_seq instead, or you'll get hit by test-lint?

Applied on 'master' or 'maint', this new test does not pass even with s/seq/test_&/, so there may be something else wrong with it, though.