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

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

[-- Attachment #1: Type: text/plain, Size: 6904 bytes --]

Hi,

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

> 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