git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: SZEDER Gábor <szeder.dev@gmail.com>
Cc: Junio C Hamano <gitster@pobox.com>, git@vger.kernel.org
Subject: Re: [PATCH v2 3/4] rebase: fix garbled progress display with '-x'
Date: Fri, 14 Jun 2019 20:42:29 +0200 (CEST)
Message-ID: <nycvar.QRO.7.76.6.1906141356140.44@tvgsbejvaqbjf.bet> (raw)
In-Reply-To: <20190612194106.GJ4012@szeder.dev>

[-- 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

  parent reply index

Thread overview: 41+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-04-30 14:25 [PATCH] " SZEDER Gábor
2019-04-30 22:25 ` Johannes Schindelin
2019-05-01 23:16   ` SZEDER Gábor
2019-05-03  8:41     ` Johannes Schindelin
2019-06-11 12:38     ` SZEDER Gábor
2019-06-11 13:03 ` [PATCH v2 0/4] rebase/progress: add and use term_clear_line() SZEDER Gábor
2019-06-11 13:03   ` [PATCH v2 1/4] t3404-rebase-interactive: use the 'q_to_cr' helper SZEDER Gábor
2019-06-11 13:03   ` [PATCH v2 2/4] pager: add a helper function to clear the last line in the terminal SZEDER Gábor
2019-06-11 13:03   ` [PATCH v2 3/4] rebase: fix garbled progress display with '-x' SZEDER Gábor
2019-06-11 20:36     ` Junio C Hamano
2019-06-11 21:11       ` SZEDER Gábor
2019-06-12 19:14         ` Johannes Schindelin
2019-06-12 19:41           ` SZEDER Gábor
2019-06-13  7:54             ` Johannes Schindelin
2019-06-14 18:42             ` Johannes Schindelin [this message]
2019-06-17 18:40               ` SZEDER Gábor
2019-06-17 19:13               ` SZEDER Gábor
2019-06-17 19:25                 ` SZEDER Gábor
2019-06-24 18:39           ` SZEDER Gábor
2019-06-25 10:08             ` Phillip Wood
2019-06-25 11:31               ` SZEDER Gábor
2019-06-25 13:33                 ` Phillip Wood
2019-06-25 18:00                 ` Phillip Wood
2019-06-25 11:38               ` Johannes Schindelin
2019-06-25 13:35                 ` Phillip Wood
2019-06-11 20:48     ` Junio C Hamano
2019-06-11 23:50       ` SZEDER Gábor
2019-06-12 16:21       ` Junio C Hamano
2019-06-11 13:03   ` [PATCH v2 4/4] progress: use term_clear_line() SZEDER Gábor
2019-06-12 20:00     ` Johannes Schindelin
2019-06-24 18:13   ` [PATCH v3 0/5] rebase/progress: add and " SZEDER Gábor
2019-06-24 18:13     ` [PATCH v3 1/5] t3404: modernize here doc style SZEDER Gábor
2019-06-25  9:06       ` Johannes Schindelin
2019-06-24 18:13     ` [PATCH v3 2/5] t3404: make the 'rebase.missingCommitsCheck=ignore' test more focused SZEDER Gábor
2019-06-25  9:07       ` Johannes Schindelin
2019-06-24 18:13     ` [PATCH v3 3/5] pager: add a helper function to clear the last line in the terminal SZEDER Gábor
2019-06-24 18:13     ` [PATCH v3 4/5] rebase: fix garbled progress display with '-x' SZEDER Gábor
2019-06-27 13:42       ` [PATCH v3.1 " SZEDER Gábor
2019-06-24 18:13     ` [PATCH v3 5/5] progress: use term_clear_line() SZEDER Gábor
2019-06-25  9:10       ` Johannes Schindelin
2019-06-25 19:44         ` Junio C Hamano

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=nycvar.QRO.7.76.6.1906141356140.44@tvgsbejvaqbjf.bet \
    --to=johannes.schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=szeder.dev@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox