Re: [PATCH v2 3/4] rebase: fix garbled progress display with '-x'

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jun 14, 2019, 18:42 UTC
Message-ID: <nycvar.QRO.7.76.6.1906141356140.44@tvgsbejvaqbjf.bet>
In-Reply-To: <20190612194106.GJ4012@szeder.dev>

Hi,

On Wed, 12 Jun 2019, SZEDER Gábor wrote:

Show 40 quoted lines

> On Wed, Jun 12, 2019 at 09:14:40PM +0200, Johannes Schindelin wrote:
>
> > On Tue, 11 Jun 2019, SZEDER Gábor wrote:
> >
> > > On Tue, Jun 11, 2019 at 01:36:16PM -0700, Junio C Hamano wrote:
> > > > SZEDER Gábor <szeder.dev@gmail.com> writes:
> > > >
> > > > > -Rebasing (1/4)QRebasing (2/4)QRebasing (3/4)QRebasing (4/4)QSuccessfully rebased and updated refs/heads/missing-commit.
> > > > > +Rebasing (1/4)QRebasing (2/4)QRebasing (3/4)QRebasing (4/4)QQ                                                                                QSuccessfully rebased and updated refs/heads/missing-commit.
> > > > >  EOF
> > > >
> > > > Yuck,
> > >
> > > Oh yeah...
> > >
> > > >... but I do not see how else/better this test can be written
> > > > myself, which makes it a double-yuck X-<
> > >
> > > Perhaps hiding those spaces behind a helper variable e.g.
> > > 'dump_term_clear_line=Q<80-spaces>Q' and embedding that in the here
> > > docs specifying the expected output in these three tests could make it
> > > ever so slightly less yuck...
> > >
> > > > Are we forcing out test to operate under dumb terminal mode and with
> > > > a known number of columns?
> > >
> > > 'test-lib.sh' sets TERM=dumb relatively early on, and in these tests
> > > we don't use 'test_terminal' to run 'git rebase', so...  yeah.  And
> > > term_columns() defaults to 80.
> > >
> > > However, if the terminal were smart, then we would have to deal with
> > > ANSI escape suddenly popping up...
> >
> > And I fear that is *exactly* what makes
> > https://dev.azure.com/gitgitgadget/git/_build/results?buildId=10539&view=ms.vss-test-web.build-test-results-tab
> > fail...
>
> Isn't it a sign of a problem in that Windows test environment that
> it mistakenly believes that the terminal is smart, even though it has
> been explicitly set to dumb?

I investigated this today.

Mind you, I still think that it is totally inappropriate for a test case with the title 'rebase -i respects rebase.missingCommitsCheck = warn' to validate the expected progress output, in particular since it verifies the progress on non-sophisticated terminals, i.e. the totally least interesting and least common scenario.

In short: I stand by my suggestion to fix these tests (i.e. ignore the progress altogether) in a preparatory patch in your patch series.

The investigation why the test fails on Windows (due to the progress being displayed for TERM=cygwin instead of TERM=dumb) took quite a bit longer than I had originally anticipated, essentially because I did not expect to uncover a bug that I introduced into Git for Windows v2.x apparently from day one of the v2.x era.

In case you are interested in the details, please read on, otherwise just mark this mail as read and move on.

Still with me? Well, here you go, enjoy the ride.

There are quite a few interesting bits about this bug, and I have to start by stating that in DOS, there was no difference between empty values of environment variables and unset environment variables. In other words, there was no way to distinguish between the equivalent of `export x=` and `unset x`. Back in the days, this was obviously perceived as reasonable, and I kind of agree given my own difficulty to describe the problem clearly.

Now, in the Win32 API there is a relatively easy way to distinguish between those values: if the return value of `GetEnvironmentVariableW()` (which indicates the length of the value) is 0 *and* `GetLastError()` returns `ERROR_ENVVAR_NOT_FOUND`, then the environment variable is unset, if it instead returns `ERROR_SUCCESS`, then it is set, and the value is the empty string.

Side note: if you want to rely on this behavior, you will most likely want to call `SetLastError(ERROR_SUCCESS)` before querying the environment, as there seem to be conditions where the last error is not re-set to that value even if the call succeeded.

Since Cygwin started really, really early in the history of Windows (even supporting Windows 95 at some stage), it emulates the DOS behavior, not the Win32 API behavior, and simply skips environment variables with empty values when spawning non-Cygwin programs. In other words, it pretends that they are unset instead.

Git for Windows' Bash (which runs the test suite) is an MSYS2 program, and since MSYS2 is based on Cygwin, inherits this behavior, and since `git.exe` is a non-MSYS2 program, there would be no way for the test suite to set environment variables to the empty value and have Git respect that.

This broke t/t3301-notes.sh (because it sets GIT_NOTES_REF= and GIT_NOTES_DISPLAY_REF= to override the configured settings), and therefore I came up with this fix in February 2015:

https://github.com/git-for-windows/msys2-runtime/commit/c19199cc14ee

It tells the MSYS2 runtime to *keep* environment variables with empty values.

Note: this fix was really made in order to let Git for Windows' test suite
pass, for no other reason. And it was not accepted by the MSYS2 project,
so this really only affects Git for Windows.

That fix seemed to work at the time (and maybe it really, really did), and it seemed to work, still, until my investigation that took the better part of today revealed that my fix was buggy. Under certain circumstances (which I believe have to do with the environment variable referring to a Unix-y path at some point, which is the case for `SHELL`), the subsequent `getwinenv()` call mishandles empty values. It tries to convert them from a Unix-y path (that looks like an absolute Unix path, but it really is rooted in MSYS2's top-level directory, identified as the second-level parent directory of `msys-2.0.dll`) to a Windows path, and failing that, it replaces the `SHELL=` by a NUL character.

The `SHELL` thing here is important, as t/t3404-rebase-interactive.sh sets this to the empty value explicitly:

https://github.com/git/git/blob/v2.22.0/t/t3404-rebase-interactive.sh#L63-L68

So instead of a `SHELL=\0` in the middle of the environment block, Git for Windows' MSYS2 runtime inserts a `\0\0`. That, however, is the marker for the end of the environment block, and as the environment has been sorted before being converted in order to launch a non-MSYS2 program (in this case, `git.exe`), the `TERM=dumb` setting is lost.

Even worse, for unrelated reasons, `git.exe` defaults to setting `TERM=cygwin` if `TERM` is unset.

I hope you, dear reader, can appreciate the number of circumstances that had to come together to trigger this bug.

The fix with which I came up can be adored here:

https://github.com/git-for-windows/msys2-runtime/commit/c10b4185a35f

I tested this locally and will re-test as soon as a new MSYS2 runtime has been deployed into Git for Windows' SDK.

Ciao, Dscho