Web lists-archives.com

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.