Re: nd/merge-quit, was Re: What's cooking in git.git (May 2019, #01; Thu, 9)

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: May 13, 2019, 14:02 UTC
Message-ID: <nycvar.QRO.7.76.6.1905131531000.44@tvgsbejvaqbjf.bet>
In-Reply-To: <CACsJy8DTErpeoNHUkCieDafnjc3iwg6QKAp2VP-8tUNYh33tJA@mail.gmail.com>

Hi Duy,

On Fri, 10 May 2019, Duy Nguyen wrote:

Show 43 quoted lines

> On Fri, May 10, 2019 at 3:54 AM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > Hi Junio & Duy,
> >
> > On Thu, 9 May 2019, Junio C Hamano wrote:
> >
> > > * nd/merge-quit (2019-05-07) 2 commits
> > >  - merge: add --quit
> > >  - merge: remove drop_save() in favor of remove_merge_branch_state()
> > >
> > >  "git merge" learned "--quit" option that cleans up the in-progress
> > >  merge while leaving the working tree and the index still in a mess.
> > >
> > >  Hmph, why is this a good idea?
> >
> > It also seems to work *only* on Linux. At least the tests break on macOS
> > and on Windows:
> >
> > https://dev.azure.com/gitgitgadget/git/_build/results?buildId=8313&view=ms.vss-test-web.build-test-results-tab
>
> Sorry I have no idea what the problem is. That's basically the same as
> the 'merge detects mod-256 conflicts (recursive)' test earlier but
> with rerere enabled. It does not even look like some leftover rerere
> records accidentally fix the conflict.
>
> I tried with a case-insensitive filesytem (on linux) and with
> --valgrind, no problem found. Travis on pu seemed ok with t7600 on
> mac.
>
> One difference I notice is the the failed test looks like it found the
> wrong merge base
>
> found 1 common ancestor:
> c4c4222 commit 1
>
> while my tests have "commit 0" as the base. "git log --graph
> --oneline" indicates "commit 1" is the wrong base.
>
> Something is wrong with the merge code (this has not even reached the
> new --quit code). I could change the setup steps to be more stable,
> using a simpler commit history, but this looks like something we
> should find and fix.

Yeah... someone should look at this... Someone. But who?

:-)

Well, since you seemed quite reluctant to figure out why your patches fail the test suite, and since we're about to enter the -rc0 phase (where we all spend all of our time to hammer out the next version, right? Right?), I figured out I better look into it before nobody does.

Turns out that the culprit is not even hard to figure out. All I had to do is to compare, carefully, the logs from the Azure Pipelines and from a local run in a local Ubuntu.

It has nothing to do with our merge code. There might be bugs, but this breakage is safely in this here patch series: the test case you introduced relies on side effects.

Namely, when test cases 51 and 52 are skipped because of a missing GPG prerequisite [*1*], and those two are obviously required to run for the `git merge to fail in your test case, as you can very easily verify by downloading the artifact containing the `trash directory.t7600-merge` directory and re-running the last steps on Linux (where the `git -c rerere.enabled=true merge master` *succeeds*).

In fact, you can very, very easily emulate the whole situation on your box by running:

	sh t7600-merge.sh -i -v -x --run=1-50,53-59

And then you can fix your test case so that it does not need to rely on test cases that may, or may not, have run previously.

Ciao, Johannes

Footnote *1*: GNU Privacy Guard is not actually missing from Git for Windows' SDK, quite to the contrary. But it fails to start a gpg-agent due to the fact that we pass a `--homedir` that contains a colon, something that is totally expected on Windows, and at the same something that GNU Privacy Guard totally cannot handle.