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

[Junio C Hamano]

Jonathan Tan  writes:

> +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_1 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?

> + 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 &&

> + 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 &&

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

[Brandon Williams]

On 06/06, Jonathan Tan wrote:
> In negotiation using protocol v2, fetch-pack sometimes does not make
> full use of the information obtained in the ref advertisement:
> specifically, that if the server advertises a commit that the client
> also has, the client never needs to inform the server that it has the
> commit's parents, since it can just tell the server that it has the
> advertised commit and it knows that the server can and will infer the
> rest.
> 
> This is because, in do_fetch_pack_v2(), rev_list_insert_ref_oid() is
> invoked before everything_local(). This means that if we have a commit
> that is both our ref and their ref, it would be enqueued by
> rev_list_insert_ref_oid() as SEEN, and since it is thus already SEEN,
> everything_local() would not enqueue it.

Thanks for fixing this slight issue with v2.  Though maybe we need to
update the commit message here because a previous patch in this version
of the series broke up everything_local() into various parts so that it
is no longer responsible for enqueueing commits?

> 
> If everything_local() were invoked first, as it is in do_fetch_pack()
> for protocol v0, then everything_local() would enqueue it with
> COMMON_REF | SEEN. The addition of COMMON_REF ensures that its parents
> are not sent as "have" lines.
> 
> Change the order in do_fetch_pack_v2() to be consistent with
> do_fetch_pack(), and to avoid sending unnecessary "have" lines.
> 
> Signed-off-by: Jonathan Tan 
> ---

-- 
Brandon Williams