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

Hi Jonathan,

On Fri, 27 Jul 2018, Johannes Schindelin wrote:

On Thu, 26 Jul 2018, Jonathan Tan wrote:

quoted

quoted

On Mon, 16 Jul 2018, Jonathan Tan wrote:

quoted

 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.

quoted

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.

Hopefully you have a chance to do so. I got the impression that it is
actually more of a flakey test than a consistent test failure:

https://git-for-windows.visualstudio.com/git/_build/results?buildId=15015&view=logs

Ciao,
Dscho