Re: [PATCH 3/6] fetch-pack: in protocol v2, enqueue commons first
- Date: Tue, 5 Jun 2018 19:10:13 -0700
- From: Jonathan Tan <jonathantanmy@xxxxxxxxxx>
- Subject: Re: [PATCH 3/6] fetch-pack: in protocol v2, enqueue commons first
On Tue, Jun 5, 2018 at 4:30 PM, Jonathan Nieder <jrnieder@xxxxxxxxx> wrote:
> I get lost in the above description. I suspect it's doing a good job
> of describing the code, instead of answering the question I really
> have about what is broken and what behavior we want instead.
> E.g. are there some commands that I can run to trigger the unnecessary
> "have" lines? That would make it easier for me to understand the rest
> and whether this is a good approach for suppressing them.
> It's possible I should be skipping to the test, but a summary in the
> commit message would make life easier for lazy people like me. :)
OK, I'll start the commit message with explaining a situation in which
these redundant "have" lines will appear instead. (The situation will
be the same as the one in the test.)
> This is subtle. My instinct would be to assume that the purpose of
> everything_local is just to determine whether we need to send a fetch
> request, but it appears we also want to rely on a side effect from it.
> Could everything_local get a function comment to describe what side
> effects we will be counting on from it?
You're right that there's a side effect in everything_local. In v2,
I'll have a preparatory patch to separate it into a few functions so
that we can see what happens more clearly.
> nit: this adds the new test as last in the script. Is there some
> logical earlier place in the file it can go instead? That way, the
> file stays organized and concurrent patches that modify the same test
> script are less likely to conflict.
Good point. I'll find a place.
>> + rm -rf server client &&
>> + git init server &&
>> + test_commit -C server aref_both_1 &&
>> + git -C server tag -d aref_both_1 &&
>> + test_commit -C server aref_both_2 &&
> What does aref stand for?
"A ref", "a" as in "one". I'll find a better name (probably just
"both_1" and "both_2").
>> + # The ref name that only the server has must be a prefix of all the
>> + # others, to ensure that the client has the same information regardless
>> + # of whether protocol v0 (which does not have ref prefix filtering) or
>> + # protocol v2 (which does) is used.
> must or else what? Maybe:
> # In this test, the ref name that only the server has is a prefix of
> # all other refs. This ensures that the client has the same information
> # regardless of [etc]
Thanks - I'll use your suggestion.
>> + git clone server client &&
>> + test_commit -C server aref &&
>> + test_commit -C client aref_client &&
>> + # In both protocol v0 and v2, ensure that the parent of aref_both_2 is
>> + # not sent as a "have" line.
> Why shouldn't it be sent as a "have" line? E.g. does another "have"
> line make it redundant?
The server's ref advertisement makes it redundant. I'll explain this
more clearly in v2.
>> + rm -f trace &&
>> + cp -r client clientv0 &&
>> + GIT_TRACE_PACKET="$(pwd)/trace" git -C clientv0 \
>> + fetch origin aref &&
>> + grep "have $(git -C client rev-parse aref_client)" trace &&
>> + grep "have $(git -C client rev-parse aref_both_2)" trace &&
> nit: can make this more robust by doing
> aref_client=$(git -C client rev-parse aref_client) &&
> aref_both_2=$(git -C client rev-parse aref_both_2) &&
> since this way if the git command fails, the test fails.
Will do. Thanks for your comments.