| Message ID | 20190611130320.18499-4-szeder.dev@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rebase/progress: add and use term_clear_line() | expand |
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?
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?
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...
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 <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.
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
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?
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
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
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 :)
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...
[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... >
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
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 >
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.
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
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
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 >
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
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 }
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(-)