Re: [PATCH] negotiator/skipping: skip commits during fetch

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Jonathan,

On Thu, 26 Jul 2018, Jonathan Tan wrote:

> > On Mon, 16 Jul 2018, Jonathan Tan wrote:
> > 
> > >  t/t5552-skipping-fetch-negotiator.sh | 179 +++++++++++++++++++
> > 
> > This test seems to be failing consistently in the recent `pu` builds:
> > 
> > https://git-for-windows.visualstudio.com/git/_build/results?buildId=14337&view=logs
> > 
> > Could you have a look, please?
> 
> Hmm...on my Linux computer, this test passes on both pu (as of the time
> of writing) and 838143aa5c ("Merge branch 'ab/newhash-is-sha256' into
> pu", 2018-07-25) (pu at the time of that build, according to the website
> you linked above). If you could rerun that test with additional code,
> could you add a "cat trace" and show me what the client sends?

I can give you something even better: a playground. Just open a PR at
https://github.com/gitgitgadget/git (all of the branches on gitster/git ar
mirrored, including yours, I am sure, so you can target that branch
specifically).

Once you open a Pull Request, it will automatically build and run the test
suite on Windows, macOS and Linux. You will see it in the "checks" section
on the bottom. Example for my range-diff series:

https://git-for-windows.visualstudio.com/git/_build/results?buildId=14279

For a quicker turnaround, you could add a commit that forces the `all`
target in `t/Makefile` to run only your test.

> When I do that, the relevant parts are:
> 
>   packet:        fetch> have 9ab46928dc282aa09f4dbf96893a252e058e7e8e
>   packet:        fetch> have dc824fafb05f3229aedf1f320bbe572e35364dfe
>   packet:        fetch> have caef059de69917b9119176a11b88afcef769331d
>   packet:        fetch> have 41bd8dc092ee110ba80e350a346ec507ab2e42a0
>   packet:        fetch> have e9a2c092a8e911567a377c881a7f6031e7f892ea
>   packet:        fetch> done
> 
> which is exactly as I (and the test) expect.
> 
> Two possible reasons for the discrepancy that I can think of offhand are
> that (1) my computer generates different commits from your test system,
> and (2) the priority queue pops commits in a different order. For (1),
> that's not possible because the SHA-1s are the same (as can be seen by
> comparing your link and the "have" lines I quoted above), and for (2),
> the code seems OK:
> 
>   static int compare(const void *a_, const void *b_, void *unused)
>   {
>   	const struct entry *a = a_;
>   	const struct entry *b = b_;
>   	return compare_commits_by_commit_date(a->commit, b->commit, NULL);
>   }
> 
> Let me know if you can observe the output of "cat trace" or if you have
> any other ideas.

Like I said, you can use those "CI" builds, I think that would be more
effective than if you waited for me to react, I am quite overwhelmed these
days.

Ciao,
Dscho