On Tue, 5 Jun 2018 16:08:21 -0700 Jonathan Nieder <jrnieder@xxxxxxxxx> wrote: > Hi, > > Jonathan Tan wrote: > > > If tag following is required when using a transport that does not > > support tag following, fetch_pack() will be invoked twice in the same > > process, necessitating a clearing of the object flags used by > > fetch_pack() sometime during the second invocation. This is currently > > done in find_common(), which means that the work done by > > everything_local() in marking complete remote refs as COMMON_REF is > > wasted. > > > > To avoid this wastage, move this clearing from find_common() to its > > parent function do_fetch_pack(), right before it calls > > everything_local(). > > I had to read this a few times and didn't end up understanding it. > > Is the idea that this will speed something up? Can you provide e.g. > "perf stat" output (or even a new perf test in t/perf) demonstrating > the improvement? Or is it a cleanup? Firstly, I don't know of a practical way to demonstrate this, because we don't have an implementation of a transport that does not support tag following. If we could demonstrate this, I think we can demonstrating it by constructing a negotiation scenario in which COMMON_REF would have been helpful, e.g. the following (untested) scenario: T C (T=tag, C=commit) |/ O We run "git fetch C" and on the second round (when we fetch T), if we wiped the flags *after* everything_local() (as is currently done), negotiation would send "have C" and "have O". But if we wiped the flags *before* everything_local(), then C would have the COMMON_REF flag and we will see that we only send "have C". So we have more efficient negotiation. > As an experiment, I tried applying the '-' part of the change without > the '+' part to get confidence that tests cover this well. To my > chagrin, all tests still passed. :/ Yes, because we don't have tests against a transport which doesn't support tag following. > In the preimage, we call clear_marks in find_common. This is right > before we start setting up the revision walk, e.g. by inserting > revisions for each ref. In the postimage, we call clear_marks in > do_fetch_pack. This is right before we call everything_local. > > I end up feeling that I don't understand the code well, neither before > nor after the patch. Ideas for making it clearer? One idea is to first separate everything_local() into its side effect parts and the "true" check that everything is local. I'll do that and send it as part of v2 of this patch series.
