Re: [PATCH v2 4/8] fetch-pack: use ref adv. to prune "have" sent

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 14, 2018, 19:52 UTC
Message-ID: <xmqqsh5p2lja.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <f12342fb2760eb0449c86c66bf44d39f5871be57.1528317619.git.jonathantanmy@google.com>

Jonathan Tan <jonathantanmy@google.com> writes:

Show 13 quoted lines

> +test_expect_success 'use ref advertisement to prune "have" lines sent' '
> +	rm -rf server client &&
> +	git init server &&
> +	test_commit -C server both_have_1 &&
> +	git -C server tag -d both_have_1 &&
> +	test_commit -C server both_have_2 &&
> +
> +	# In this test, the ref name that only the server has is a prefix of all
> +	# other refs. This is because in protocol v2, the client sends
> +	# "ref-prefix" to limit the ref advertisement. Naming the ref "bo" means
> +	# that "ref-prefix refs/tags/bo*" is sent, resulting in the client also
> +	# knowing about refs/tags/both_have_2, just as it would when it uses
> +	# protocol v0.

I have a mixed feeling about this test.

The client wants to fetch "bo" and nothing else in this example. And refs "both_have_*", which have *nothing* to do with the ref the client actually wants, is advertised merely because "both_have_*" begins with "bo" with v2. But that is an implementation detail that we do not necessarily want to cast in stone, isn't it? After all, in future iterations of the protocol, we may find it too excessive that we have to advertise both_have_1..both_have_10000 when the client asks for bo and change either the pattern matching rule of what "ref-prefix refs/tags/bo" matches, or the ref-prefix sent by the client, and at that point, the expectation that both_have_2 is sent as "have" but both_have_2^1 is not may have to change, no?

Show 13 quoted lines

> +	git clone server client &&
> +	test_commit -C server bo &&
> +	test_commit -C client client_has &&
> +
> +	# In both protocol v0 and v2, ensure that the parent of both_have_2 is
> +	# not sent as a "have" line. The client should know that the server has
> +	# both_have_2, so it only needs to inform the server that it has
> +	# both_have_2, and the server can infer the rest.
> +
> +	rm -f trace &&
> +	cp -r client clientv0 &&
> +	GIT_TRACE_PACKET="$(pwd)/trace" git -C clientv0 \
> +		fetch origin bo &&

Show 16 quoted lines

> +	grep "have $(git -C client rev-parse client_has)" trace &&
> +	grep "have $(git -C client rev-parse both_have_2)" trace &&
> +	! grep "have $(git -C client rev-parse both_have_2^)" trace &&
> +
> +	rm -f trace &&
> +	cp -r client clientv2 &&
> +	GIT_TRACE_PACKET="$(pwd)/trace" git -C clientv2 -c protocol.version=2 \
> +		fetch origin bo &&
> +	grep "have $(git -C client rev-parse client_has)" trace &&
> +	grep "have $(git -C client rev-parse both_have_2)" trace &&
> +	! grep "have $(git -C client rev-parse both_have_2^)" trace
> +'
> +
>  test_expect_success 'filtering by size' '
>  	rm -rf server client &&
>  	test_create_repo server &&