git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Johannes Schindelin <johannes.schindelin@gmx.de>
Date
Jun 12, 2019, 19:14 UTC
Message-ID
<nycvar.QRO.7.76.6.1906122056570.789@QRFXGBC-DHN364S.ybpnyqbznva>
In-Reply-To
<20190611211151.GG4012@szeder.dev>
Hi,
On Tue, 11 Jun 2019, SZEDER Gábor wrote:
Show 28 quoted lines
> 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
Previous: SZEDER GáborNext: SZEDER Gábor
Message 12 of 41 in “rebase: fix garbled progress display with '-x'”
  1. rebase: fix garbled progress display with '-x'SZEDER Gábor, Apr 30, 2019
  2. Johannes SchindelinApr 30, 2019
  3. SZEDER GáborMay 1, 2019
  4. Johannes SchindelinMay 3, 2019
  5. SZEDER GáborJun 11, 2019
  6. 0/4 rebase/progress: add and use term_clear_line()SZEDER Gábor, Jun 11, 2019
  7. 2/4 pager: add a helper function to clear the last line in the terminalSZEDER Gábor, Jun 11, 2019
  8. 1/4 t3404-rebase-interactive: use the 'q_to_cr' helperSZEDER Gábor, Jun 11, 2019
  9. 3/4 rebase: fix garbled progress display with '-x'SZEDER Gábor, Jun 11, 2019
  10. Junio C HamanoJun 11, 2019
  11. SZEDER GáborJun 11, 2019
  12. Johannes SchindelinJun 12, 2019
  13. SZEDER GáborJun 12, 2019
  14. Johannes SchindelinJun 13, 2019
  15. Johannes SchindelinJun 14, 2019
  16. SZEDER GáborJun 17, 2019
  17. SZEDER GáborJun 17, 2019
  18. SZEDER GáborJun 17, 2019
  19. SZEDER GáborJun 24, 2019
  20. Phillip WoodJun 25, 2019
  21. SZEDER GáborJun 25, 2019
  22. Phillip WoodJun 25, 2019
  23. Phillip WoodJun 25, 2019
  24. Johannes SchindelinJun 25, 2019
  25. Phillip WoodJun 25, 2019
  26. Junio C HamanoJun 11, 2019
  27. SZEDER GáborJun 11, 2019
  28. Junio C HamanoJun 12, 2019
  29. 4/4 progress: use term_clear_line()SZEDER Gábor, Jun 11, 2019
  30. Johannes SchindelinJun 12, 2019
  31. 0/5 rebase/progress: add and use term_clear_line()SZEDER Gábor, Jun 24, 2019
  32. 2/5 t3404: make the 'rebase.missingCommitsCheck=ignore' test more focusedSZEDER Gábor, Jun 24, 2019
  33. Johannes SchindelinJun 25, 2019
  34. 1/5 t3404: modernize here doc styleSZEDER Gábor, Jun 24, 2019
  35. Johannes SchindelinJun 25, 2019
  36. 3/5 pager: add a helper function to clear the last line in the terminalSZEDER Gábor, Jun 24, 2019
  37. 4/5 rebase: fix garbled progress display with '-x'SZEDER Gábor, Jun 24, 2019
  38. 4/5 rebase: fix garbled progress display with '-x'SZEDER Gábor, Jun 27, 2019
  39. 5/5 progress: use term_clear_line()SZEDER Gábor, Jun 24, 2019
  40. Johannes SchindelinJun 25, 2019
  41. Junio C HamanoJun 25, 2019

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.