[v2,3/4] rebase: fix garbled progress display with '-x'
diff mbox series

Message ID 20190611130320.18499-4-szeder.dev@gmail.com
State New
Headers show
Series
  • rebase/progress: add and use term_clear_line()
Related show

Commit Message

SZEDER Gábor June 11, 2019, 1:03 p.m. UTC
When running a command with the 'exec' instruction during an
interactive rebase session, or for a range of commits using 'git
rebase -x', the output can be a bit garbled when the name of the
command is short enough:

  $ git rebase -x true HEAD~5
  Executing: true
  Executing: true
  Executing: true
  Executing: true
  Executing: true)
  Successfully rebased and updated refs/heads/master.

Note the ')' at the end of the last line.  It gets more garbled as the
range of commits increases:

  $ git rebase -x true HEAD~50
  Executing: true)
  [ repeated 3 more times ]
  Executing: true0)
  [ repeated 44 more times ]
  Executing: true00)
  Successfully rebased and updated refs/heads/master.

Those extra numbers and ')' are remnants of the previously displayed
"Rebasing (N/M)" progress lines that are usually completely
overwritten by the "Executing: <cmd>" lines, unless 'cmd' is short and
the "N/M" part is long.

Make sure that the previously displayed "Rebasing (N/M)" line is
cleared by using the term_clear_line() helper function added in the
previous patch.

A couple of other rebase commands print similar messages, e.g.
"Stopped at <abbrev-oid>... <subject>" for the 'edit' or 'break'
commands, or the "Successfully rebased and updated <full-ref>." at the
very end.  These are so long that they practically always overwrite
that "Rebasing (n/m)" progress line, but let's be prudent, and clear
the last line before printing these, too.

Three tests, one in 't3404-rebase-interactive.sh' and two in
't3420-rebase-autostash.sh' check the full output of 'git rebase' and
thus are affected by this change, so adjust their expectations to
account for the new line clearing.

Note that this patch doesn't completely eliminate the possibility of
similar garbled outputs, e.g. some error messages from rebase or the
"Auto-merging <file>" message from withing the depths of the merge
machinery might not be long enough to completely cover the last
"Rebasing (N/M)" line.  This patch doesn't do anything about them,
because dealing with them individually would result in way too much
churn, while having a catch-all term_clear_line() call in the common
code path of pick_commits() would hide the "Rebasing (N/M)" line way
too soon, and it would either flicker or be invisible.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 sequencer.c                   | 17 ++++++++++++++---
 t/t3404-rebase-interactive.sh |  2 +-
 t/t3420-rebase-autostash.sh   |  4 ++--
 3 files changed, 17 insertions(+), 6 deletions(-)

Comments

Junio C Hamano June 11, 2019, 8:36 p.m. UTC | #1
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, ... but I do not see how else/better this test can be written
myself, which makes it a double-yuck X-<

Are we forcing out test to operate under dumb terminal mode and with
a known number of columns?
Junio C Hamano June 11, 2019, 8:48 p.m. UTC | #2
SZEDER Gábor <szeder.dev@gmail.com> writes:

> Make sure that the previously displayed "Rebasing (N/M)" line is
> cleared by using the term_clear_line() helper function added in the
> previous patch.
>
> A couple of other rebase commands print similar messages, e.g.
> "Stopped at <abbrev-oid>... <subject>" for the 'edit' or 'break'
> commands, or the "Successfully rebased and updated <full-ref>." at the
> very end.  These are so long that they practically always overwrite
> that "Rebasing (n/m)" progress line, but let's be prudent, and clear
> the last line before printing these, too.
> ...
> Note that this patch doesn't completely eliminate the possibility of
> similar garbled outputs, ...
> too soon, and it would either flicker or be invisible.

The user of term_clear_line() in this patch always guard themselves
behind "we do not do this if we are producing verbose output," but
the proposed log message does not explain why they need to do so.

Is it because it is so obvious to potential readers?
SZEDER Gábor June 11, 2019, 9:11 p.m. UTC | #3
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...
SZEDER Gábor June 11, 2019, 11:50 p.m. UTC | #4
On Tue, Jun 11, 2019 at 01:48:14PM -0700, Junio C Hamano wrote:
> SZEDER Gábor <szeder.dev@gmail.com> writes:
> 
> > Make sure that the previously displayed "Rebasing (N/M)" line is
> > cleared by using the term_clear_line() helper function added in the
> > previous patch.
> >
> > A couple of other rebase commands print similar messages, e.g.
> > "Stopped at <abbrev-oid>... <subject>" for the 'edit' or 'break'
> > commands, or the "Successfully rebased and updated <full-ref>." at the
> > very end.  These are so long that they practically always overwrite
> > that "Rebasing (n/m)" progress line, but let's be prudent, and clear
> > the last line before printing these, too.
> > ...
> > Note that this patch doesn't completely eliminate the possibility of
> > similar garbled outputs, ...
> > too soon, and it would either flicker or be invisible.
> 
> The user of term_clear_line() in this patch always guard themselves
> behind "we do not do this if we are producing verbose output," but
> the proposed log message does not explain why they need to do so.

In verbose mode there is no progress output that is overwritten and
might need to be covered up.
Junio C Hamano June 12, 2019, 4:21 p.m. UTC | #5
Junio C Hamano <gitster@pobox.com> writes:

> SZEDER Gábor <szeder.dev@gmail.com> writes:
>
>> Make sure that the previously displayed "Rebasing (N/M)" line is
>> cleared by using the term_clear_line() helper function added in the
>> previous patch.
>>
>> A couple of other rebase commands print similar messages, e.g.
>> "Stopped at <abbrev-oid>... <subject>" for the 'edit' or 'break'
>> commands, or the "Successfully rebased and updated <full-ref>." at the
>> very end.  These are so long that they practically always overwrite
>> that "Rebasing (n/m)" progress line, but let's be prudent, and clear
>> the last line before printing these, too.
>> ...
>> Note that this patch doesn't completely eliminate the possibility of
>> similar garbled outputs, ...
>> too soon, and it would either flicker or be invisible.
>
> The user of term_clear_line() in this patch always guard themselves
> behind "we do not do this if we are producing verbose output," but
> the proposed log message does not explain why they need to do so.
>
> Is it because it is so obvious to potential readers?

Answering myself, it is due to this part in sequencer.c:

   3721                                 if (!opts->quiet)
   3722                                         fprintf(stderr, "Rebasing (%d/%d)%s",
   3723                                                 todo_list->done_nr,
   3724                                                 todo_list->total_nr,
   3725                                                 opts->verbose ? "\n" : "\r");

That is, under 'verbose' mode, we do not try to reuse a single line
to show different steps of rebase in the progress output, but
consume one line per one step, so it would not be necessary to erase
what is previously written on the line.

I do not think it is so obvious, though.
Johannes Schindelin June 12, 2019, 7:14 p.m. UTC | #6
Hi,

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

You cannot easily see it in that output (probably because of incorrectly
encoded Escape sequences in the `.xml` output), so I'll paste what I see
here, locally, when running `t3404-*.sh -i -V -x`:

-- snip --
[...]
ok 99 - rebase -i respects rebase.missingCommitsCheck = ignore

expecting success:
        test_config rebase.missingCommitsCheck warn &&
        rebase_setup_and_clean missing-commit &&
        set_fake_editor &&
        FAKE_LINES="1 2 3 4" \
                git rebase -i --root 2>actual &&
        test_i18ncmp expect actual &&
        test D = $(git cat-file commit HEAD | sed -ne \$p)

++ test_config rebase.missingCommitsCheck warn
++ config_dir=
++ test rebase.missingCommitsCheck = -C
++ test_when_finished 'test_unconfig  '\''rebase.missingCommitsCheck'\'''
++ test 0 = 0
++ test_cleanup='{ test_unconfig  '\''rebase.missingCommitsCheck'\''
                } && (exit "$eval_ret"); eval_ret=$?; :'
++ git config rebase.missingCommitsCheck warn
++ rebase_setup_and_clean missing-commit
++ test_when_finished '
                git checkout master &&
                test_might_fail git branch -D missing-commit &&
                test_might_fail git rebase --abort
        '
++ test 0 = 0
++ test_cleanup='{
                git checkout master &&
                test_might_fail git branch -D missing-commit &&
                test_might_fail git rebase --abort

                } && (exit "$eval_ret"); eval_ret=$?; { test_unconfig  '\''rebase.missingCommitsCheck'\''
                } && (exit "$eval_ret"); eval_ret=$?; :'
++ git checkout -b missing-commit master
Switched to a new branch 'missing-commit'
++ set_fake_editor
++ write_script fake-editor.sh
++ echo '#!/bin/sh'
++ cat
++ chmod +x fake-editor.sh
+++ pwd
+++ builtin pwd -W
++ test_set_editor 'C:/git-sdk-64/usr/src/git/.git/worktrees/wip/temp-rebase-123/t/trash directory.t3404-rebase-interactive/fake-editor.sh'
++ FAKE_EDITOR='C:/git-sdk-64/usr/src/git/.git/worktrees/wip/temp-rebase-123/t/trash directory.t3404-rebase-interactive/fake-editor.sh'
++ export FAKE_EDITOR
++ EDITOR='"$FAKE_EDITOR"'
++ export EDITOR
++ FAKE_LINES='1 2 3 4'
++ git rebase -i --root
rebase -i script before editing:
pick 6e62bf890e21 A
pick 313fe965c048 B
pick d0f65f2f81ee C
pick 0547e3f1350d D
pick 8f99a4f1fbbd E

rebase -i script after editing:
pick 6e62bf890e21 A
pick 313fe965c048 B
pick d0f65f2f81ee C
pick 0547e3f1350d D
++ test_i18ncmp expect actual
++ test_have_prereq C_LOCALE_OUTPUT
++ save_IFS='
'
++ IFS=,
++ set -- C_LOCALE_OUTPUT
++ IFS='
'
++ total_prereq=0
++ ok_prereq=0
++ missing_prereq=
++ for prerequisite in "$@"
++ case "$prerequisite" in
++ negative_prereq=
++ case " $lazily_tested_prereq " in
++ case " $lazily_testable_prereq " in
++ total_prereq=1
++ case "$satisfied_prereq" in
++ satisfied_this_prereq=t
++ case "$satisfied_this_prereq,$negative_prereq" in
++ ok_prereq=1
++ test 1 = 1
++ test_cmp expect actual
++ GIT_ALLOC_LIMIT=0
++ test-tool cmp expect actual
diff --git a/expect b/actual
index 05fcfcb..9555e34 100644
--- a/expect
+++ b/actual
@@ -6,4 +6,4 @@ To avoid this message, use "drop" to explicitly remove a commit.
 Use 'git config rebase.missingCommitsCheck' to change the level of warnings.
 The possible behaviours are: ignore, warn, error.

-Rebasing (1/4)^MRebasing (2/4)^MRebasing (3/4)^MRebasing (4/4)^M                                                                                ^MSuccessfully rebased and updated refs/heads/missing-commit.
+Rebasing (1/4)^MRebasing (2/4)^MRebasing (3/4)^MRebasing (4/4)^MESC[KSuccessfully rebased and updated refs/heads/missing-commit.
error: last command exited with $?=1
not ok 100 - rebase -i respects rebase.missingCommitsCheck = warn
#
#               test_config rebase.missingCommitsCheck warn &&
#               rebase_setup_and_clean missing-commit &&
#               set_fake_editor &&
#               FAKE_LINES="1 2 3 4" \
#                       git rebase -i --root 2>actual &&
#               test_i18ncmp expect actual &&
#               test D = $(git cat-file commit HEAD | sed -ne \$p)
#
-- snap --

(I copy-pasted this from the output of `less` so that the control sequences can be seen.)

To be utterly honest, I really fail to see a reason why a test case that
purports to verify that `git rebase -i` respects
`rebase.missingCommitsCheck=warn` should fail when the progress is shown in an
unexpected format.

It strikes me as yet another poorly written test case that fails to catch the
intended regressions, instead it catches a bug *in the test case itself* when
legitimate changes are made to the progress code.

Nothing in a test suite is worse than a test that fails (or succeeds) for the
wrong reasons.

To make things even worse, the code that generates that `expect` file is
outside the test case.

Here it is, in its full "glory":

-- snip --
q_to_cr >expect <<EOF
Warning: some commits may have been dropped accidentally.
Dropped commits (newer to older):
 - $(git rev-list --pretty=oneline --abbrev-commit -1 master)
To avoid this message, use "drop" to explicitly remove a commit.

Use 'git config rebase.missingCommitsCheck' to change the level of warnings.
The possible behaviours are: ignore, warn, error.

Rebasing (1/4)QRebasing (2/4)QRebasing (3/4)QRebasing (4/4)QQ                                                                                QSuccessfully rebased and updated refs/heads/missing-commit.
EOF
-- snap --

May I please *strongly* suggest to fix this first? It should

- completely lose that last line,
- be inserted into the test case itself so that e.g. disk full problems are
  caught and logged properly, and
- the `test_i18ncmp expect actual` call in the test case should be replaced
  by something like:

	sed "\$d" <actual >actual-skip-progress &&
	test_i18ncmp expect actual-skip-progress

This should obviously be made as a separate, introductory patch (probably with
a less scathing commit message than my comments above would suggest).

And that would also remove the double-yuck.

Ciao,
Dscho
SZEDER Gábor June 12, 2019, 7:41 p.m. UTC | #7
On Wed, Jun 12, 2019 at 09:14:40PM +0200, Johannes Schindelin wrote:
> Hi,
> 
> 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?
Johannes Schindelin June 13, 2019, 7:54 a.m. UTC | #8
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?

Yes, it looks like some odd decision there.

But the more important take-away from my mail is that we do not even want
those test cases to depend on the vagaries of the current progress
machinery.

Ciao,
Dscho
Johannes Schindelin June 14, 2019, 6:42 p.m. UTC | #9
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
SZEDER Gábor June 17, 2019, 6:40 p.m. UTC | #10
On Fri, Jun 14, 2019 at 08:42:29PM +0200, Johannes Schindelin wrote:

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

I agree and will try to get around to it in the coming days.

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

Sure!

[...]

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

Thank you, it was both educational and entertaining :)
SZEDER Gábor June 17, 2019, 7:13 p.m. UTC | #11
On Fri, Jun 14, 2019 at 08:42:29PM +0200, Johannes Schindelin wrote:

> 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

Hmm, hang on for a sec.  Why does it set SHELL to empty?

So t3404 sets SHELL to the empty value since cd035b1cef
(rebase -i: add exec command to launch a shell command, 2010-08-10),
and the in-test comment highlighted on the above link says:

  # "exec" commands are run with the user shell by default, but this may
  # be non-POSIX. For example, if SHELL=zsh then ">file" doesn't work
  # to create a file. Unsetting SHELL avoids such non-portable behavior
  # in tests. It must be exported for it to take effect where needed.
  SHELL=

Furthermore, the corresponding documentation added in cd035b1cef
says the following:

  The "exec" command launches the command in a shell (the one specified
  in `$SHELL`, or the default shell if `$SHELL` is not set)

IOW if SHELL is not set/empty, then it will run the default '/bin/sh',
but who knows what it might be, there's no guarantee that it's a sane
POSIX shell.  That's why we have "Define SHELL_PATH to a POSIX shell
if your /bin/sh is broken." very near to the top of our Makefile.

So while setting SHELL to the empty value might indeed avoid
non-portable behavior when the user in general prefers a non-POSIX
shell, it wouldn't help if '/bin/sh' is broken.  For that it should be
set to $SHELL_PATH (or perhaps $TEST_SHELL_PATH nowadays...).

Though cd035b1cef is now close to 9 years old, plenty of time for
somebody to run into this issue in the wild and speak up...
SZEDER Gábor June 17, 2019, 7:25 p.m. UTC | #12
[resend to Matthieu's current email address]

On Mon, Jun 17, 2019 at 09:13:28PM +0200, SZEDER Gábor wrote:
> On Fri, Jun 14, 2019 at 08:42:29PM +0200, Johannes Schindelin wrote:
> 
> > 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
> 
> Hmm, hang on for a sec.  Why does it set SHELL to empty?
> 
> So t3404 sets SHELL to the empty value since cd035b1cef
> (rebase -i: add exec command to launch a shell command, 2010-08-10),
> and the in-test comment highlighted on the above link says:
> 
>   # "exec" commands are run with the user shell by default, but this may
>   # be non-POSIX. For example, if SHELL=zsh then ">file" doesn't work
>   # to create a file. Unsetting SHELL avoids such non-portable behavior
>   # in tests. It must be exported for it to take effect where needed.
>   SHELL=
> 
> Furthermore, the corresponding documentation added in cd035b1cef
> says the following:
> 
>   The "exec" command launches the command in a shell (the one specified
>   in `$SHELL`, or the default shell if `$SHELL` is not set)
> 
> IOW if SHELL is not set/empty, then it will run the default '/bin/sh',
> but who knows what it might be, there's no guarantee that it's a sane
> POSIX shell.  That's why we have "Define SHELL_PATH to a POSIX shell
> if your /bin/sh is broken." very near to the top of our Makefile.
> 
> So while setting SHELL to the empty value might indeed avoid
> non-portable behavior when the user in general prefers a non-POSIX
> shell, it wouldn't help if '/bin/sh' is broken.  For that it should be
> set to $SHELL_PATH (or perhaps $TEST_SHELL_PATH nowadays...).
> 
> Though cd035b1cef is now close to 9 years old, plenty of time for
> somebody to run into this issue in the wild and speak up...
>
SZEDER Gábor June 24, 2019, 6:39 p.m. UTC | #13
On Wed, Jun 12, 2019 at 09:14:40PM +0200, Johannes Schindelin wrote:
> Hi,
> 
> 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...
> 
> You cannot easily see it in that output (probably because of incorrectly
> encoded Escape sequences in the `.xml` output), so I'll paste what I see
> here, locally, when running `t3404-*.sh -i -V -x`:
> 
> -- snip --
> [...]
> ok 99 - rebase -i respects rebase.missingCommitsCheck = ignore
> 
> expecting success:
>         test_config rebase.missingCommitsCheck warn &&
>         rebase_setup_and_clean missing-commit &&
>         set_fake_editor &&
>         FAKE_LINES="1 2 3 4" \
>                 git rebase -i --root 2>actual &&
>         test_i18ncmp expect actual &&
>         test D = $(git cat-file commit HEAD | sed -ne \$p)

[...]

> ++ test_cmp expect actual
> ++ GIT_ALLOC_LIMIT=0
> ++ test-tool cmp expect actual
> diff --git a/expect b/actual
> index 05fcfcb..9555e34 100644
> --- a/expect
> +++ b/actual
> @@ -6,4 +6,4 @@ To avoid this message, use "drop" to explicitly remove a commit.
>  Use 'git config rebase.missingCommitsCheck' to change the level of warnings.
>  The possible behaviours are: ignore, warn, error.
> 
> -Rebasing (1/4)^MRebasing (2/4)^MRebasing (3/4)^MRebasing (4/4)^M                                                                                ^MSuccessfully rebased and updated refs/heads/missing-commit.
> +Rebasing (1/4)^MRebasing (2/4)^MRebasing (3/4)^MRebasing (4/4)^MESC[KSuccessfully rebased and updated refs/heads/missing-commit.
> error: last command exited with $?=1
> not ok 100 - rebase -i respects rebase.missingCommitsCheck = warn
> #
> #               test_config rebase.missingCommitsCheck warn &&
> #               rebase_setup_and_clean missing-commit &&
> #               set_fake_editor &&
> #               FAKE_LINES="1 2 3 4" \
> #                       git rebase -i --root 2>actual &&
> #               test_i18ncmp expect actual &&
> #               test D = $(git cat-file commit HEAD | sed -ne \$p)
> #
> -- snap --
> 
> (I copy-pasted this from the output of `less` so that the control sequences can be seen.)
> 
> To be utterly honest, I really fail to see a reason why a test case that
> purports to verify that `git rebase -i` respects
> `rebase.missingCommitsCheck=warn` should fail when the progress is shown in an
> unexpected format.
> 
> It strikes me as yet another poorly written test case that fails to catch the
> intended regressions, instead it catches a bug *in the test case itself* when
> legitimate changes are made to the progress code.
> 
> Nothing in a test suite is worse than a test that fails (or succeeds) for the
> wrong reasons.
> 
> To make things even worse, the code that generates that `expect` file is
> outside the test case.
> 
> Here it is, in its full "glory":
> 
> -- snip --
> q_to_cr >expect <<EOF
> Warning: some commits may have been dropped accidentally.
> Dropped commits (newer to older):
>  - $(git rev-list --pretty=oneline --abbrev-commit -1 master)
> To avoid this message, use "drop" to explicitly remove a commit.
> 
> Use 'git config rebase.missingCommitsCheck' to change the level of warnings.
> The possible behaviours are: ignore, warn, error.
> 
> Rebasing (1/4)QRebasing (2/4)QRebasing (3/4)QRebasing (4/4)QQ                                                                                QSuccessfully rebased and updated refs/heads/missing-commit.
> EOF
> -- snap --
> 
> May I please *strongly* suggest to fix this first? It should
> 
> - completely lose that last line,
> - be inserted into the test case itself so that e.g. disk full problems are
>   caught and logged properly, and
> - the `test_i18ncmp expect actual` call in the test case should be replaced
>   by something like:
> 
> 	sed "\$d" <actual >actual-skip-progress &&
> 	test_i18ncmp expect actual-skip-progress
> 
> This should obviously be made as a separate, introductory patch (probably with
> a less scathing commit message than my comments above would suggest).
> 
> And that would also remove the double-yuck.

Unfortunately, this addresses only one of the "Yuck"s; see v3 of this
patch series [1].

The other yucks affect the following four tests in
't3420-rebase-autostash.sh':

  16 - rebase --merge --autostash: check output
  23 - rebase --merge: check output with conflicting stash
  26 - rebase --interactive --autostash: check output
  33 - rebase --interactive: check output with conflicting stash

These tests come from commits b76aeae553 (rebase: add regression tests
for console output, 2017-06-19) and 7d70e6b902 (rebase: add more
regression tests for console output, 2017-06-19), and are specifically
about checking the (whole) console output of 'git rebase', so I left
the updates to them as they were.

In any case, Cc-ing Phillip to discuss whether something could be done
about them (now perhaps preferably (for me :) as a follow-up, and not
another preparatory patches).


[1] https://public-inbox.org/git/20190624181318.17388-3-szeder.dev@gmail.com/T/#u
Phillip Wood June 25, 2019, 10:08 a.m. UTC | #14
Hi dscho and Gábor

On 24/06/2019 19:39, SZEDER Gábor wrote:
> On Wed, Jun 12, 2019 at 09:14:40PM +0200, Johannes Schindelin wrote:
>> Hi,
>>
>> 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...
>>
>> You cannot easily see it in that output (probably because of incorrectly
>> encoded Escape sequences in the `.xml` output), so I'll paste what I see
>> here, locally, when running `t3404-*.sh -i -V -x`:
>>
>> -- snip --
>> [...]
>> ok 99 - rebase -i respects rebase.missingCommitsCheck = ignore
>>
>> expecting success:
>>         test_config rebase.missingCommitsCheck warn &&
>>         rebase_setup_and_clean missing-commit &&
>>         set_fake_editor &&
>>         FAKE_LINES="1 2 3 4" \
>>                 git rebase -i --root 2>actual &&
>>         test_i18ncmp expect actual &&
>>         test D = $(git cat-file commit HEAD | sed -ne \$p)
> 
> [...]
> 
>> ++ test_cmp expect actual
>> ++ GIT_ALLOC_LIMIT=0
>> ++ test-tool cmp expect actual
>> diff --git a/expect b/actual
>> index 05fcfcb..9555e34 100644
>> --- a/expect
>> +++ b/actual
>> @@ -6,4 +6,4 @@ To avoid this message, use "drop" to explicitly remove a commit.
>>  Use 'git config rebase.missingCommitsCheck' to change the level of warnings.
>>  The possible behaviours are: ignore, warn, error.
>>
>> -Rebasing (1/4)^MRebasing (2/4)^MRebasing (3/4)^MRebasing (4/4)^M                                                                                ^MSuccessfully rebased and updated refs/heads/missing-commit.
>> +Rebasing (1/4)^MRebasing (2/4)^MRebasing (3/4)^MRebasing (4/4)^MESC[KSuccessfully rebased and updated refs/heads/missing-commit.
>> error: last command exited with $?=1
>> not ok 100 - rebase -i respects rebase.missingCommitsCheck = warn
>> #
>> #               test_config rebase.missingCommitsCheck warn &&
>> #               rebase_setup_and_clean missing-commit &&
>> #               set_fake_editor &&
>> #               FAKE_LINES="1 2 3 4" \
>> #                       git rebase -i --root 2>actual &&
>> #               test_i18ncmp expect actual &&
>> #               test D = $(git cat-file commit HEAD | sed -ne \$p)
>> #
>> -- snap --
>>
>> (I copy-pasted this from the output of `less` so that the control sequences can be seen.)
>>
>> To be utterly honest, I really fail to see a reason why a test case that
>> purports to verify that `git rebase -i` respects
>> `rebase.missingCommitsCheck=warn` should fail when the progress is shown in an
>> unexpected format.
>>
>> It strikes me as yet another poorly written test case that fails to catch the
>> intended regressions, instead it catches a bug *in the test case itself* when
>> legitimate changes are made to the progress code.
>>
>> Nothing in a test suite is worse than a test that fails (or succeeds) for the
>> wrong reasons.
>>
>> To make things even worse, the code that generates that `expect` file is
>> outside the test case.
>>
>> Here it is, in its full "glory":
>>
>> -- snip --
>> q_to_cr >expect <<EOF
>> Warning: some commits may have been dropped accidentally.
>> Dropped commits (newer to older):
>>  - $(git rev-list --pretty=oneline --abbrev-commit -1 master)
>> To avoid this message, use "drop" to explicitly remove a commit.
>>
>> Use 'git config rebase.missingCommitsCheck' to change the level of warnings.
>> The possible behaviours are: ignore, warn, error.
>>
>> Rebasing (1/4)QRebasing (2/4)QRebasing (3/4)QRebasing (4/4)QQ                                                                                QSuccessfully rebased and updated refs/heads/missing-commit.
>> EOF
>> -- snap --
>>
>> May I please *strongly* suggest to fix this first? It should
>>
>> - completely lose that last line,
>> - be inserted into the test case itself so that e.g. disk full problems are
>>   caught and logged properly, and
>> - the `test_i18ncmp expect actual` call in the test case should be replaced
>>   by something like:
>>
>> 	sed "\$d" <actual >actual-skip-progress &&
>> 	test_i18ncmp expect actual-skip-progress
>>
>> This should obviously be made as a separate, introductory patch (probably with
>> a less scathing commit message than my comments above would suggest).
>>
>> And that would also remove the double-yuck.
> 
> Unfortunately, this addresses only one of the "Yuck"s; see v3 of this
> patch series [1].
> 
> The other yucks affect the following four tests in
> 't3420-rebase-autostash.sh':
> 
>   16 - rebase --merge --autostash: check output
>   23 - rebase --merge: check output with conflicting stash
>   26 - rebase --interactive --autostash: check output
>   33 - rebase --interactive: check output with conflicting stash
> 
> These tests come from commits b76aeae553 (rebase: add regression tests
> for console output, 2017-06-19) and 7d70e6b902 (rebase: add more
> regression tests for console output, 2017-06-19), and are specifically
> about checking the (whole) console output of 'git rebase', so I left
> the updates to them as they were.
> 
> In any case, Cc-ing Phillip to discuss whether something could be done
> about them (now perhaps preferably (for me :) as a follow-up, and not
> another preparatory patches).

Those tests were added to check that `git stash` was being silenced (see
79a6226981 ("rebase -i: silence stash apply", 2017-05-18)). I can have a
think about a better way to do that, but is it still a problem? I just
tried to take a look at your CI output and
https://dev.azure.com/gitgitgadget/git/_build/results?buildId=11406
seems to be all green - have I missed something or has Gábor fixed the
issue?

Best Wishes

Phillip

> 
> 
> [1] https://public-inbox.org/git/20190624181318.17388-3-szeder.dev@gmail.com/T/#u
>
SZEDER Gábor June 25, 2019, 11:31 a.m. UTC | #15
On Tue, Jun 25, 2019 at 11:08:04AM +0100, Phillip Wood wrote:
> >> Rebasing (1/4)QRebasing (2/4)QRebasing (3/4)QRebasing (4/4)QQ                                                                                QSuccessfully rebased and updated refs/heads/missing-commit.


> >> May I please *strongly* suggest to fix this first? It should
> >>
> >> - completely lose that last line,
> >> - be inserted into the test case itself so that e.g. disk full problems are
> >>   caught and logged properly, and
> >> - the `test_i18ncmp expect actual` call in the test case should be replaced
> >>   by something like:
> >>
> >> 	sed "\$d" <actual >actual-skip-progress &&
> >> 	test_i18ncmp expect actual-skip-progress
> >>
> >> This should obviously be made as a separate, introductory patch (probably with
> >> a less scathing commit message than my comments above would suggest).
> >>
> >> And that would also remove the double-yuck.
> > 
> > Unfortunately, this addresses only one of the "Yuck"s; see v3 of this
> > patch series [1].
> > 
> > The other yucks affect the following four tests in
> > 't3420-rebase-autostash.sh':
> > 
> >   16 - rebase --merge --autostash: check output
> >   23 - rebase --merge: check output with conflicting stash
> >   26 - rebase --interactive --autostash: check output
> >   33 - rebase --interactive: check output with conflicting stash
> > 
> > These tests come from commits b76aeae553 (rebase: add regression tests
> > for console output, 2017-06-19) and 7d70e6b902 (rebase: add more
> > regression tests for console output, 2017-06-19), and are specifically
> > about checking the (whole) console output of 'git rebase', so I left
> > the updates to them as they were.
> > 
> > In any case, Cc-ing Phillip to discuss whether something could be done
> > about them (now perhaps preferably (for me :) as a follow-up, and not
> > another preparatory patches).
> 
> Those tests were added to check that `git stash` was being silenced (see
> 79a6226981 ("rebase -i: silence stash apply", 2017-05-18)).

Hmm, so would it then be possible/sensible to do something like this
instead:

  git rebase --options... >out &&
  test_i18ngrep ! "<something specific that only 'git stash' would print if it wasn't silenced>" out

> I can have a
> think about a better way to do that, but is it still a problem? I just
> tried to take a look at your CI output and
> https://dev.azure.com/gitgitgadget/git/_build/results?buildId=11406
> seems to be all green - have I missed something or has Gábor fixed the
> issue?

No, it was Dscho who fixed the Azure CI issue, see

  https://public-inbox.org/git/nycvar.QRO.7.76.6.1906141356140.44@tvgsbejvaqbjf.bet/

elsewhere in this thread (it's a long one, but well worth the read
IMO).

However, the point here is not that Azure CI failure, but rather the
fact that tests had to be updated to include the new line clearing
sequence in the expected output, and that "Q<...80 spaces...>Q" looks
yuck indeed.
Johannes Schindelin June 25, 2019, 11:38 a.m. UTC | #16
Hi Phillip,

On Tue, 25 Jun 2019, Phillip Wood wrote:

> On 24/06/2019 19:39, SZEDER Gábor wrote:
> > On Wed, Jun 12, 2019 at 09:14:40PM +0200, Johannes Schindelin wrote:
>
> > The other yucks affect the following four tests in
> > 't3420-rebase-autostash.sh':
> >
> >   16 - rebase --merge --autostash: check output
> >   23 - rebase --merge: check output with conflicting stash
> >   26 - rebase --interactive --autostash: check output
> >   33 - rebase --interactive: check output with conflicting stash
> >
> > These tests come from commits b76aeae553 (rebase: add regression tests
> > for console output, 2017-06-19) and 7d70e6b902 (rebase: add more
> > regression tests for console output, 2017-06-19), and are specifically
> > about checking the (whole) console output of 'git rebase', so I left
> > the updates to them as they were.
> >
> > In any case, Cc-ing Phillip to discuss whether something could be done
> > about them (now perhaps preferably (for me :) as a follow-up, and not
> > another preparatory patches).
>
> Those tests were added to check that `git stash` was being silenced (see
> 79a6226981 ("rebase -i: silence stash apply", 2017-05-18)). I can have a
> think about a better way to do that, but is it still a problem? I just
> tried to take a look at your CI output and
> https://dev.azure.com/gitgitgadget/git/_build/results?buildId=11406
> seems to be all green - have I missed something or has Gábor fixed the
> issue?

AFAIU the test cases were patched to accommodate for the new output.
Ideally, they would be changed to not need any changes in the future,
certainly not to accommodate changes in unrelated areas (such as the
progress).

So yes, the build is green, and the patches are probably good to go, but
there is room for add-on patches to clean up the test cases to test
succinctly what they are supposed to test.

Ciao,
Dscho
Phillip Wood June 25, 2019, 1:33 p.m. UTC | #17
On 25/06/2019 12:31, SZEDER Gábor wrote:
> On Tue, Jun 25, 2019 at 11:08:04AM +0100, Phillip Wood wrote:
>>>> Rebasing (1/4)QRebasing (2/4)QRebasing (3/4)QRebasing (4/4)QQ                                                                                QSuccessfully rebased and updated refs/heads/missing-commit.
> 
> 
>>>> May I please *strongly* suggest to fix this first? It should
>>>>
>>>> - completely lose that last line,
>>>> - be inserted into the test case itself so that e.g. disk full problems are
>>>>    caught and logged properly, and
>>>> - the `test_i18ncmp expect actual` call in the test case should be replaced
>>>>    by something like:
>>>>
>>>> 	sed "\$d" <actual >actual-skip-progress &&
>>>> 	test_i18ncmp expect actual-skip-progress
>>>>
>>>> This should obviously be made as a separate, introductory patch (probably with
>>>> a less scathing commit message than my comments above would suggest).
>>>>
>>>> And that would also remove the double-yuck.
>>>
>>> Unfortunately, this addresses only one of the "Yuck"s; see v3 of this
>>> patch series [1].
>>>
>>> The other yucks affect the following four tests in
>>> 't3420-rebase-autostash.sh':
>>>
>>>    16 - rebase --merge --autostash: check output
>>>    23 - rebase --merge: check output with conflicting stash
>>>    26 - rebase --interactive --autostash: check output
>>>    33 - rebase --interactive: check output with conflicting stash
>>>
>>> These tests come from commits b76aeae553 (rebase: add regression tests
>>> for console output, 2017-06-19) and 7d70e6b902 (rebase: add more
>>> regression tests for console output, 2017-06-19), and are specifically
>>> about checking the (whole) console output of 'git rebase', so I left
>>> the updates to them as they were.
>>>
>>> In any case, Cc-ing Phillip to discuss whether something could be done
>>> about them (now perhaps preferably (for me :) as a follow-up, and not
>>> another preparatory patches).
>>
>> Those tests were added to check that `git stash` was being silenced (see
>> 79a6226981 ("rebase -i: silence stash apply", 2017-05-18)).
> 
> Hmm, so would it then be possible/sensible to do something like this
> instead:
> 
>    git rebase --options... >out &&
>    test_i18ngrep ! "<something specific that only 'git stash' would print if it wasn't silenced>" out

Yes we could do something like that, but I think there is merit in 
testing the whole output as it catches any changes that accidentally 
start polluting what the user sees. It also means that any changes to 
the output are deliberate. It may be a pain to have to update a test 
case when the output changes but at least it ensures the change was 
deliberate. I'm not sure what we can do to get around the problems that 
in creates for unrelated changes such as this series.

>> I can have a
>> think about a better way to do that, but is it still a problem? I just
>> tried to take a look at your CI output and
>> https://dev.azure.com/gitgitgadget/git/_build/results?buildId=11406
>> seems to be all green - have I missed something or has Gábor fixed the
>> issue?
> 
> No, it was Dscho who fixed the Azure CI issue, see
> 
>    https://public-inbox.org/git/nycvar.QRO.7.76.6.1906141356140.44@tvgsbejvaqbjf.bet/
> 
> elsewhere in this thread (it's a long one, but well worth the read
> IMO).


Thanks for the pointer, I'll have a read

> However, the point here is not that Azure CI failure, but rather the
> fact that tests had to be updated to include the new line clearing
> sequence in the expected output, and that "Q<...80 spaces...>Q" looks
> yuck indeed.

Ah, thanks for explaining

Best Wishes

Phillip
Phillip Wood June 25, 2019, 1:35 p.m. UTC | #18
Hi Dscho

On 25/06/2019 12:38, Johannes Schindelin wrote:
> Hi Phillip,
> 
> On Tue, 25 Jun 2019, Phillip Wood wrote:
> 
>> On 24/06/2019 19:39, SZEDER Gábor wrote:
>>> On Wed, Jun 12, 2019 at 09:14:40PM +0200, Johannes Schindelin wrote:
>>
>>> The other yucks affect the following four tests in
>>> 't3420-rebase-autostash.sh':
>>>
>>>    16 - rebase --merge --autostash: check output
>>>    23 - rebase --merge: check output with conflicting stash
>>>    26 - rebase --interactive --autostash: check output
>>>    33 - rebase --interactive: check output with conflicting stash
>>>
>>> These tests come from commits b76aeae553 (rebase: add regression tests
>>> for console output, 2017-06-19) and 7d70e6b902 (rebase: add more
>>> regression tests for console output, 2017-06-19), and are specifically
>>> about checking the (whole) console output of 'git rebase', so I left
>>> the updates to them as they were.
>>>
>>> In any case, Cc-ing Phillip to discuss whether something could be done
>>> about them (now perhaps preferably (for me :) as a follow-up, and not
>>> another preparatory patches).
>>
>> Those tests were added to check that `git stash` was being silenced (see
>> 79a6226981 ("rebase -i: silence stash apply", 2017-05-18)). I can have a
>> think about a better way to do that, but is it still a problem? I just
>> tried to take a look at your CI output and
>> https://dev.azure.com/gitgitgadget/git/_build/results?buildId=11406
>> seems to be all green - have I missed something or has Gábor fixed the
>> issue?
> 
> AFAIU the test cases were patched to accommodate for the new output.
> Ideally, they would be changed to not need any changes in the future,
> certainly not to accommodate changes in unrelated areas (such as the
> progress).
> 
> So yes, the build is green, and the patches are probably good to go, but
> there is room for add-on patches to clean up the test cases to test
> succinctly what they are supposed to test.

Thanks, I'd missed the point entirely! As I said in my reply to Gábor, I 
think testing the whole output does have some use, but I can see that it 
would be very good to get rid of the whole Q thing

Best Wishes

Phillip

> 
> Ciao,
> Dscho
>
Phillip Wood June 25, 2019, 6 p.m. UTC | #19
Hi Gábor and Dscho
On 25/06/2019 12:31, SZEDER Gábor wrote:
> On Tue, Jun 25, 2019 at 11:08:04AM +0100, Phillip Wood wrote:
>>>> Rebasing (1/4)QRebasing (2/4)QRebasing (3/4)QRebasing (4/4)QQ                                                                                QSuccessfully rebased and updated refs/heads/missing-commit.
> 
> 
>>>> May I please *strongly* suggest to fix this first? It should
>>>>
>>>> - completely lose that last line,
>>>> - be inserted into the test case itself so that e.g. disk full problems are
>>>>   caught and logged properly, and
>>>> - the `test_i18ncmp expect actual` call in the test case should be replaced
>>>>   by something like:
>>>>
>>>> 	sed "\$d" <actual >actual-skip-progress &&
>>>> 	test_i18ncmp expect actual-skip-progress
>>>>
>>>> This should obviously be made as a separate, introductory patch (probably with
>>>> a less scathing commit message than my comments above would suggest).
>>>>
>>>> And that would also remove the double-yuck.
>>>
>>> Unfortunately, this addresses only one of the "Yuck"s; see v3 of this
>>> patch series [1].
>>>
>>> The other yucks affect the following four tests in
>>> 't3420-rebase-autostash.sh':
>>>
>>>   16 - rebase --merge --autostash: check output
>>>   23 - rebase --merge: check output with conflicting stash
>>>   26 - rebase --interactive --autostash: check output
>>>   33 - rebase --interactive: check output with conflicting stash
>>>
>>> These tests come from commits b76aeae553 (rebase: add regression tests
>>> for console output, 2017-06-19) and 7d70e6b902 (rebase: add more
>>> regression tests for console output, 2017-06-19), and are specifically
>>> about checking the (whole) console output of 'git rebase', so I left
>>> the updates to them as they were.
>>>
>>> In any case, Cc-ing Phillip to discuss whether something could be done
>>> about them (now perhaps preferably (for me :) as a follow-up, and not
>>> another preparatory patches).
>>
>> Those tests were added to check that `git stash` was being silenced (see
>> 79a6226981 ("rebase -i: silence stash apply", 2017-05-18)).
> 
> Hmm, so would it then be possible/sensible to do something like this
> instead:
> 
>   git rebase --options... >out &&
>   test_i18ngrep ! "<something specific that only 'git stash' would print if it wasn't silenced>" out
> 
>> I can have a
>> think about a better way to do that, but is it still a problem? I just
>> tried to take a look at your CI output and
>> https://dev.azure.com/gitgitgadget/git/_build/results?buildId=11406
>> seems to be all green - have I missed something or has Gábor fixed the
>> issue?
> 
> No, it was Dscho who fixed the Azure CI issue, see
> 
>   https://public-inbox.org/git/nycvar.QRO.7.76.6.1906141356140.44@tvgsbejvaqbjf.bet/
> 
> elsewhere in this thread (it's a long one, but well worth the read
> IMO).
> 
> However, the point here is not that Azure CI failure, but rather the
> fact that tests had to be updated to include the new line clearing
> sequence in the expected output, and that "Q<...80 spaces...>Q" looks
> yuck indeed.

I've come up with a sed based solution to remove the progress
notifications from the output - see
https://github.com/gitgitgadget/git/pull/276 If you're both happy with
the basic idea I'll clean it up and submit it (I've just noticed I left
some debugging bits in one of the tests). I was worried using "\r" in a
sed expression wouldn't be portable but the tests pass on gitgitgadget.
If it proves to be a problem on other platforms we could use always tr
to convert "\r" to some unique string and use that string in the sed
expression.

Best Wishes

Phillip

Patch
diff mbox series

diff --git a/sequencer.c b/sequencer.c
index f88a97fb10..63b09cfceb 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3731,8 +3731,11 @@  static int pick_commits(struct repository *r,
 			unlink(git_path_merge_head(the_repository));
 			delete_ref(NULL, "REBASE_HEAD", NULL, REF_NO_DEREF);
 
-			if (item->command == TODO_BREAK)
+			if (item->command == TODO_BREAK) {
+				if (!opts->verbose)
+					term_clear_line();
 				return stopped_at_head(r);
+			}
 		}
 		if (item->command <= TODO_SQUASH) {
 			if (is_rebase_i(opts))
@@ -3754,11 +3757,14 @@  static int pick_commits(struct repository *r,
 			}
 			if (item->command == TODO_EDIT) {
 				struct commit *commit = item->commit;
-				if (!res)
+				if (!res) {
+					if (!opts->verbose)
+						term_clear_line();
 					fprintf(stderr,
 						_("Stopped at %s...  %.*s\n"),
 						short_commit_name(commit),
 						item->arg_len, arg);
+				}
 				return error_with_patch(r, commit,
 					arg, item->arg_len, opts, res, !res);
 			}
@@ -3796,6 +3802,8 @@  static int pick_commits(struct repository *r,
 			int saved = *end_of_arg;
 			struct stat st;
 
+			if (!opts->verbose)
+				term_clear_line();
 			*end_of_arg = '\0';
 			res = do_exec(r, arg);
 			*end_of_arg = saved;
@@ -3954,10 +3962,13 @@  static int pick_commits(struct repository *r,
 		}
 		apply_autostash(opts);
 
-		if (!opts->quiet)
+		if (!opts->quiet) {
+			if (!opts->verbose)
+				term_clear_line();
 			fprintf(stderr,
 				"Successfully rebased and updated %s.\n",
 				head_ref.buf);
+		}
 
 		strbuf_release(&buf);
 		strbuf_release(&head_ref);
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index e66e95d453..72ce9d393c 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1313,7 +1313,7 @@  To avoid this message, use "drop" to explicitly remove a commit.
 Use 'git config rebase.missingCommitsCheck' to change the level of warnings.
 The possible behaviours are: ignore, warn, error.
 
-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
 
 test_expect_success 'rebase -i respects rebase.missingCommitsCheck = warn' '
diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
index 2d1094e483..9186e90127 100755
--- a/t/t3420-rebase-autostash.sh
+++ b/t/t3420-rebase-autostash.sh
@@ -49,7 +49,7 @@  create_expected_success_interactive () {
 	$(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
 	HEAD is now at $(git rev-parse --short feature-branch) third commit
 	Rebasing (1/2)QRebasing (2/2)QApplied autostash.
-	Successfully rebased and updated refs/heads/rebased-feature-branch.
+	Q                                                                                QSuccessfully rebased and updated refs/heads/rebased-feature-branch.
 	EOF
 }
 
@@ -73,7 +73,7 @@  create_expected_failure_interactive () {
 	Rebasing (1/2)QRebasing (2/2)QApplying autostash resulted in conflicts.
 	Your changes are safe in the stash.
 	You can run "git stash pop" or "git stash drop" at any time.
-	Successfully rebased and updated refs/heads/rebased-feature-branch.
+	Q                                                                                QSuccessfully rebased and updated refs/heads/rebased-feature-branch.
 	EOF
 }