Re: [PATCH 3/6] fetch-pack: in protocol v2, enqueue commons first
- From
- Jonathan Tan <jonathantanmy@google.com>
- Date
- Jun 6, 2018, 02:10 UTC
- Message-ID
- <CAGf8dgLJy5yKx3yD3-ADdMTBW8_8N984wMN8FfN4g2_xjAON4A@mail.gmail.com>
- In-Reply-To
- <20180605233026.GF9266@aiede.svl.corp.google.com>
On Tue, Jun 5, 2018 at 4:30 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 10 quoted lines
> 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.)
Show 6 quoted lines
> 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.
Show 7 quoted lines
>> + 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").
Show 11 quoted lines
>> + >> + # 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.
Show 9 quoted lines
>> + 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.
Show 14 quoted lines
>> + >> + 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.