{"thread":{"id":"51015","subject":"[PATCH] rebase: fix garbled progress display with '-x'","startedAt":"2019-04-30T14:26:05Z","lastAt":"2019-06-27T13:43:00Z","messageCount":41,"participants":["SZEDER Gábor","Johannes Schindelin","Junio C Hamano","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"374701","messageId":"20190430142556.20921-1-szeder.dev@gmail.com","threadId":"51015","inReplyTo":null,"subject":"[PATCH] rebase: fix garbled progress display with '-x'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-04-30T14:25:56Z","receivedAt":"2019-04-30T14:26:05Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"When running a command with the 'exec' instruction during an\ninteractive rebase session, or for a range of commits using 'git\nrebase -x', the output can be a bit garbled when the name of the\ncommand is short enough:\n\n $ git rebase -x true HEAD~5\n Executing: true\n Executing: true\n Executing: true\n Executing: true\n Executing: true)\n Successfully rebased and updated refs/heads/master.\n\nNote the ')' at the end of the last line. It gets more garbled as the\nrange of commits increases:\n\n $ git rebase -x true HEAD~50\n Executing: true)\n [ repeated 3 more times ]\n Executing: true0)\n [ repeated 44 more times ]\n Executing: true00)\n Successfully rebased and updated refs/heads/master.\n\nThose extra numbers and ')' are remnants of the previously displayed\n\"Rebasing (N/M)\" progress lines that are usually completely\noverwritten by the \"Executing: \" lines, unless 'cmd' is short and\nthe \"N/M\" part is long.\n\nMake sure that the previously displayed \"Rebasing (N/M)\" line is\ncompletely covered up by printing a terminal width worth of space\ncharacters.\n\nSigned-off-by: SZEDER Gábor \n---\n\nThis issue has already been present in the scripted rebase as well.\n\nAs far as I could tell, if any other rebase instruction prints a\nmessage, then that tends to be so long (including abbreviated commit\nOIDs and whatnot) that they practically always overwrite that\n\"Rebasing (N/M)\" progress line (well, except, perhaps, when rebasing\nbillions of commits at a time?).\n\n sequencer.c | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 546f281898..c2e4baa90e 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3631,6 +3631,12 @@ static int pick_commits(struct repository *r,\n \t\t\tint saved = *end_of_arg;\n \t\t\tstruct stat st;\n \n+\t\t\tif (!opts->verbose)\n+\t\t\t\t/*\n+\t\t\t\t * Fully cover the previous \"Rebasing (n/m)\"\n+\t\t\t\t * progress line.\n+\t\t\t\t */\n+\t\t\t\tfprintf(stderr, \"%*s\\r\", term_columns(), \"\");\n \t\t\t*end_of_arg = '\\0';\n \t\t\tres = do_exec(r, arg);\n \t\t\t*end_of_arg = saved;\n-- \n2.21.0.1181.g24122a4251\n\n"},{"id":"374726","messageId":"nycvar.QRO.7.76.6.1904301819540.45@tvgsbejvaqbjf.bet","threadId":"51015","inReplyTo":"20190430142556.20921-1-szeder.dev@gmail.com","subject":"Re: [PATCH] rebase: fix garbled progress display with '-x'","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-04-30T22:25:35Z","receivedAt":"2019-04-30T22:25:43Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 30 Apr 2019, SZEDER Gábor wrote:\n\n> When running a command with the 'exec' instruction during an\n> interactive rebase session, or for a range of commits using 'git\n> rebase -x', the output can be a bit garbled when the name of the\n> command is short enough:\n>\n> $ git rebase -x true HEAD~5\n> Executing: true\n> Executing: true\n> Executing: true\n> Executing: true\n> Executing: true)\n> Successfully rebased and updated refs/heads/master.\n>\n> Note the ')' at the end of the last line. It gets more garbled as the\n> range of commits increases:\n>\n> $ git rebase -x true HEAD~50\n> Executing: true)\n> [ repeated 3 more times ]\n> Executing: true0)\n> [ repeated 44 more times ]\n> Executing: true00)\n> Successfully rebased and updated refs/heads/master.\n>\n> Those extra numbers and ')' are remnants of the previously displayed\n> \"Rebasing (N/M)\" progress lines that are usually completely\n> overwritten by the \"Executing: \" lines, unless 'cmd' is short and\n> the \"N/M\" part is long.\n>\n> Make sure that the previously displayed \"Rebasing (N/M)\" line is\n> completely covered up by printing a terminal width worth of space\n> characters.\n>\n> Signed-off-by: SZEDER Gábor \n\nMakes sense.\n\n> This issue has already been present in the scripted rebase as well.\n>\n> As far as I could tell, if any other rebase instruction prints a\n> message, then that tends to be so long (including abbreviated commit\n> OIDs and whatnot) that they practically always overwrite that\n> \"Rebasing (N/M)\" progress line (well, except, perhaps, when rebasing\n> billions of commits at a time?).\n>\n> sequencer.c | 6 ++++++\n> 1 file changed, 6 insertions(+)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index 546f281898..c2e4baa90e 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -3631,6 +3631,12 @@ static int pick_commits(struct repository *r,\n> \t\t\tint saved = *end_of_arg;\n> \t\t\tstruct stat st;\n>\n> +\t\t\tif (!opts->verbose)\n> +\t\t\t\t/*\n> +\t\t\t\t * Fully cover the previous \"Rebasing (n/m)\"\n> +\t\t\t\t * progress line.\n> +\t\t\t\t */\n> +\t\t\t\tfprintf(stderr, \"%*s\\r\", term_columns(), \"\");\n\nIIRC there are terminals (`cmd.exe`?) that would advance to the next row\nautomatically when printing the exact number of columns in a row. So this\nwould not work.\n\nBut isn't there an ANSI sequence that we can use?\n\n*clicketyclick*\n\nYes: https://github.com/git/git/blob/v2.21.0/editor.c#L101 (introduced in\nhttps://github.com/git/git/commit/abfb04d0c7#diff-cdeec438beb851e450b94a11db9ab7edR89)\n\nSo maybe we should do the same here, i.e.\n\n\tfputs(\"\\r\\033[K\", stderr);\n\nCiao,\nDscho\n\n> \t\t\t*end_of_arg = '\\0';\n> \t\t\tres = do_exec(r, arg);\n> \t\t\t*end_of_arg = saved;\n> --\n> 2.21.0.1181.g24122a4251\n>\n>\n"},{"id":"374795","messageId":"20190501231640.GD14763@szeder.dev","threadId":"51015","inReplyTo":"nycvar.QRO.7.76.6.1904301819540.45@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] rebase: fix garbled progress display with '-x'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-05-01T23:16:40Z","receivedAt":"2019-05-01T23:16:47Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Apr 30, 2019 at 06:25:35PM -0400, Johannes Schindelin wrote:\n> Hi,\n> \n> On Tue, 30 Apr 2019, SZEDER Gábor wrote:\n> \n> > When running a command with the 'exec' instruction during an\n> > interactive rebase session, or for a range of commits using 'git\n> > rebase -x', the output can be a bit garbled when the name of the\n> > command is short enough:\n> >\n> > $ git rebase -x true HEAD~5\n> > Executing: true\n> > Executing: true\n> > Executing: true\n> > Executing: true\n> > Executing: true)\n> > Successfully rebased and updated refs/heads/master.\n> >\n> > Note the ')' at the end of the last line. It gets more garbled as the\n> > range of commits increases:\n> >\n> > $ git rebase -x true HEAD~50\n> > Executing: true)\n> > [ repeated 3 more times ]\n> > Executing: true0)\n> > [ repeated 44 more times ]\n> > Executing: true00)\n> > Successfully rebased and updated refs/heads/master.\n> >\n> > Those extra numbers and ')' are remnants of the previously displayed\n> > \"Rebasing (N/M)\" progress lines that are usually completely\n> > overwritten by the \"Executing: \" lines, unless 'cmd' is short and\n> > the \"N/M\" part is long.\n> >\n> > Make sure that the previously displayed \"Rebasing (N/M)\" line is\n> > completely covered up by printing a terminal width worth of space\n> > characters.\n> >\n> > Signed-off-by: SZEDER Gábor \n> \n> Makes sense.\n> \n> > This issue has already been present in the scripted rebase as well.\n> >\n> > As far as I could tell, if any other rebase instruction prints a\n> > message, then that tends to be so long (including abbreviated commit\n> > OIDs and whatnot) that they practically always overwrite that\n> > \"Rebasing (N/M)\" progress line (well, except, perhaps, when rebasing\n> > billions of commits at a time?).\n> >\n> > sequencer.c | 6 ++++++\n> > 1 file changed, 6 insertions(+)\n> >\n> > diff --git a/sequencer.c b/sequencer.c\n> > index 546f281898..c2e4baa90e 100644\n> > --- a/sequencer.c\n> > +++ b/sequencer.c\n> > @@ -3631,6 +3631,12 @@ static int pick_commits(struct repository *r,\n> > \t\t\tint saved = *end_of_arg;\n> > \t\t\tstruct stat st;\n> >\n> > +\t\t\tif (!opts->verbose)\n> > +\t\t\t\t/*\n> > +\t\t\t\t * Fully cover the previous \"Rebasing (n/m)\"\n> > +\t\t\t\t * progress line.\n> > +\t\t\t\t */\n> > +\t\t\t\tfprintf(stderr, \"%*s\\r\", term_columns(), \"\");\n> \n> IIRC there are terminals (`cmd.exe`?) that would advance to the next row\n> automatically when printing the exact number of columns in a row. So this\n> would not work.\n\nHrm, I though about using 'term_columns()-1', or moving the '\\r' from\nthe format string to the string to be printed, but in the end didn't\ndo either, because it seemed to work well as it is in the two\nterminals that I tried (on Linux).\n\n> But isn't there an ANSI sequence that we can use?\n> \n> *clicketyclick*\n> \n> Yes: https://github.com/git/git/blob/v2.21.0/editor.c#L101 (introduced in\n> https://github.com/git/git/commit/abfb04d0c7#diff-cdeec438beb851e450b94a11db9ab7edR89)\n> \n> So maybe we should do the same here, i.e.\n> \n> \tfputs(\"\\r\\033[K\", stderr);\n\nOh, that would be nice (and not only here, but it could have made the\nchanges in 'sg/overlong-progress-fix' a bit simpler as well).\nUnfortunately, however, it only works on non-dumb terminals (note the\n'!is_terminal_dumb()' call in the preceeding condition), while rebase\nhasn't had such a limitation on the terminal yet.\n\n> Ciao,\n> Dscho\n> \n> > \t\t\t*end_of_arg = '\\0';\n> > \t\t\tres = do_exec(r, arg);\n> > \t\t\t*end_of_arg = saved;\n> > --\n> > 2.21.0.1181.g24122a4251\n> >\n> >\n\n"},{"id":"374861","messageId":"nycvar.QRO.7.76.6.1905031034400.45@tvgsbejvaqbjf.bet","threadId":"51015","inReplyTo":"20190501231640.GD14763@szeder.dev","subject":"Re: [PATCH] rebase: fix garbled progress display with '-x'","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-05-03T08:41:01Z","receivedAt":"2019-05-03T08:41:11Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 2 May 2019, SZEDER Gábor wrote:\n\n> On Tue, Apr 30, 2019 at 06:25:35PM -0400, Johannes Schindelin wrote:\n> >\n> > On Tue, 30 Apr 2019, SZEDER Gábor wrote:\n> >\n> > > diff --git a/sequencer.c b/sequencer.c\n> > > index 546f281898..c2e4baa90e 100644\n> > > --- a/sequencer.c\n> > > +++ b/sequencer.c\n> > > @@ -3631,6 +3631,12 @@ static int pick_commits(struct repository *r,\n> > > \t\t\tint saved = *end_of_arg;\n> > > \t\t\tstruct stat st;\n> > >\n> > > +\t\t\tif (!opts->verbose)\n> > > +\t\t\t\t/*\n> > > +\t\t\t\t * Fully cover the previous \"Rebasing (n/m)\"\n> > > +\t\t\t\t * progress line.\n> > > +\t\t\t\t */\n> > > +\t\t\t\tfprintf(stderr, \"%*s\\r\", term_columns(), \"\");\n> >\n> > IIRC there are terminals (`cmd.exe`?) that would advance to the next row\n> > automatically when printing the exact number of columns in a row. So this\n> > would not work.\n>\n> Hrm, I though about using 'term_columns()-1', or moving the '\\r' from\n> the format string to the string to be printed, but in the end didn't\n> do either, because it seemed to work well as it is in the two\n> terminals that I tried (on Linux).\n>\n> > But isn't there an ANSI sequence that we can use?\n> >\n> > *clicketyclick*\n> >\n> > Yes: https://github.com/git/git/blob/v2.21.0/editor.c#L101 (introduced in\n> > https://github.com/git/git/commit/abfb04d0c7#diff-cdeec438beb851e450b94a11db9ab7edR89)\n> >\n> > So maybe we should do the same here, i.e.\n> >\n> > \tfputs(\"\\r\\033[K\", stderr);\n>\n> Oh, that would be nice (and not only here, but it could have made the\n> changes in 'sg/overlong-progress-fix' a bit simpler as well).\n> Unfortunately, however, it only works on non-dumb terminals (note the\n> '!is_terminal_dumb()' call in the preceeding condition), while rebase\n> hasn't had such a limitation on the terminal yet.\n\nI think it would still be nice to have it in the common case of a capable\nterminal.\n\nMaybe this could even become a helper function, e.g.\n`erase_terminal_line_remainder()`? This should also help other developers\ndiscover and use it.\n\nCiao,\nDscho\n"},{"id":"377004","messageId":"20190611123811.GF4012@szeder.dev","threadId":"51015","inReplyTo":"20190501231640.GD14763@szeder.dev","subject":"Re: [PATCH] rebase: fix garbled progress display with '-x'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-06-11T12:38:11Z","receivedAt":"2019-06-11T12:38:19Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, May 02, 2019 at 01:16:40AM +0200, SZEDER Gábor wrote:\n> On Tue, Apr 30, 2019 at 06:25:35PM -0400, Johannes Schindelin wrote:\n> > > Make sure that the previously displayed \"Rebasing (N/M)\" line is\n> > > completely covered up by printing a terminal width worth of space\n> > > characters.\n\n> > > +\t\t\tif (!opts->verbose)\n> > > +\t\t\t\t/*\n> > > +\t\t\t\t * Fully cover the previous \"Rebasing (n/m)\"\n> > > +\t\t\t\t * progress line.\n> > > +\t\t\t\t */\n> > > +\t\t\t\tfprintf(stderr, \"%*s\\r\", term_columns(), \"\");\n> > \n> > IIRC there are terminals (`cmd.exe`?) that would advance to the next row\n> > automatically when printing the exact number of columns in a row. So this\n> > would not work.\n> \n> Hrm, I though about using 'term_columns()-1', or moving the '\\r' from\n> the format string to the string to be printed, but in the end didn't\n> do either, because it seemed to work well as it is in the two\n> terminals that I tried (on Linux).\n\nAnd I would have been wrong doing so, because that fprintf'ed string\nends with '\\r', not '\\n', and therefore it is actually necessary to\nwrite as many space characters as the width of the terminal, or the\nlast character in the previously displayed line won't get overwritten.\n\n"},{"id":"377007","messageId":"20190611130320.18499-1-szeder.dev@gmail.com","threadId":"51015","inReplyTo":"20190430142556.20921-1-szeder.dev@gmail.com","subject":"[PATCH v2 0/4] rebase/progress: add and use term_clear_line()","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-06-11T13:03:16Z","receivedAt":"2019-06-11T13:03:39Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"This is a reworked/extended version of the single patch \"rebase: fix\ngarbled progress display with '-x'\" at:\n\n https://public-inbox.org/git/20190430142556.20921-1-szeder.dev@gmail.com/T/#u\n\nChanges:\n\n - Add a helper function to clear the whole line on the terminal,\n using an ANSI escape sequence on capable terminals and a\n terminal-width worth of spaces on dumb terminals.\n\n - Use this helper when fixing rebase's garbled progress display with\n '-x'.\n\n - Use this helper in progress.c:display() as well to get rid of\n quite some arithmetic that made a -rc-phase bugfix necessary.\n\nI don't really like the necessary adjustments to the tests in patches\n3 and 4, though.\n\n\nSZEDER Gábor (4):\n t3404-rebase-interactive: use the 'q_to_cr' helper\n pager: add a helper function to clear the last line in the terminal\n rebase: fix garbled progress display with '-x'\n progress: use term_clear_line()\n\n cache.h | 1 +\n editor.c | 6 +++---\n pager.c | 20 ++++++++++++++++++++\n progress.c | 28 +++++++++++-----------------\n sequencer.c | 17 ++++++++++++++---\n t/t3404-rebase-interactive.sh | 15 +++------------\n t/t3420-rebase-autostash.sh | 4 ++--\n t/t5541-http-push-smart.sh | 6 +++---\n 8 files changed, 57 insertions(+), 40 deletions(-)\n\n-- \n2.22.0.566.g58873a45ff\n\n"},{"id":"377008","messageId":"20190611130320.18499-3-szeder.dev@gmail.com","threadId":"51015","inReplyTo":"20190611130320.18499-1-szeder.dev@gmail.com","subject":"[PATCH v2 2/4] pager: add a helper function to clear the last line in the terminal","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-06-11T13:03:18Z","receivedAt":"2019-06-11T13:03:42Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"There are a couple of places where we want to clear the last line on\nthe terminal, e.g. when a progress bar line is overwritten by a\nshorter line, then the end of that progress line would remain visible,\nunless we cover it up.\n\nIn 'progress.c' we did this by always appending a fixed number of\nspace characters to the next line (even if it was not shorter than the\nprevious), but as it turned out that fixed number was not quite large\nenough, see the fix in 9f1fd84e15 (progress: clear previous progress\nupdate dynamically, 2019-04-12). From then on we've been keeping\ntrack of the length of the last displayed progress line and appending\nthe appropriate number of space characters to the next line, if\nnecessary, but, alas, this approach turned out to be error prone, see\nthe fix in 1aed1a5f25 (progress: avoid empty line when breaking the\nprogress line, 2019-05-19). The next patch in this series is about to\nfix a case where we don't clear the last line, and on occasion do end\nup with such garbage at the end of the line. It would be great if we\ncould do that without the need to deal with that without meticulously\ncomputing the necessary number of space characters.\n\nSo add a helper function to clear the last line on the terminal using\nan ANSI escape sequence, which has the advantage to clear the whole\nline no matter how wide it is, even after the terminal width changed.\nSuch an escape sequence is not available on dumb terminals, though, so\nin that case fall back to simply print a whole terminal width (as\nreported by term_columns()) worth of space characters.\n\nIn 'editor.c' launch_specified_editor() already used this ANSI escape\nsequence, so replace it with a call to this function.\n\nSigned-off-by: SZEDER Gábor \n---\n cache.h | 1 +\n editor.c | 6 +++---\n pager.c | 20 ++++++++++++++++++++\n 3 files changed, 24 insertions(+), 3 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex b4bb2e2c11..5b2cd32bad 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1759,6 +1759,7 @@ void setup_pager(void);\n int pager_in_use(void);\n extern int pager_use_color;\n int term_columns(void);\n+void term_clear_line(void);\n int decimal_width(uintmax_t);\n int check_pager_config(const char *cmd);\n void prepare_pager_args(struct child_process *, const char *pager);\ndiff --git a/editor.c b/editor.c\nindex 71547674ab..f079abbf11 100644\n--- a/editor.c\n+++ b/editor.c\n@@ -96,10 +96,10 @@ static int launch_specified_editor(const char *editor, const char *path,\n \n \t\tif (print_waiting_for_editor && !is_terminal_dumb())\n \t\t\t/*\n-\t\t\t * Go back to the beginning and erase the entire line to\n-\t\t\t * avoid wasting the vertical space.\n+\t\t\t * Erase the entire line to avoid wasting the\n+\t\t\t * vertical space.\n \t\t\t */\n-\t\t\tfputs(\"\\r\\033[K\", stderr);\n+\t\t\tterm_clear_line();\n \t}\n \n \tif (!buffer)\ndiff --git a/pager.c b/pager.c\nindex 4168460ae9..41446d4f05 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -177,6 +177,26 @@ int term_columns(void)\n \treturn term_columns_at_startup;\n }\n \n+/*\n+ * Clear the entire line, leave cursor in first column.\n+ */\n+void term_clear_line(void)\n+{\n+\tif (is_terminal_dumb())\n+\t\t/*\n+\t\t * Fall back to print a terminal width worth of space\n+\t\t * characters (hoping that the terminal is still as wide\n+\t\t * as it was upon the first call to term_columns()).\n+\t\t */\n+\t\tfprintf(stderr, \"\\r%*s\\r\", term_columns(), \"\");\n+\telse\n+\t\t/*\n+\t\t * On non-dumb terminals use an escape sequence to clear\n+\t\t * the whole line, no matter how wide the terminal.\n+\t\t */\n+\t\tfputs(\"\\r\\033[K\", stderr);\n+}\n+\n /*\n * How many columns do we need to show this number in decimal?\n */\n-- \n2.22.0.566.g58873a45ff\n\n"},{"id":"377009","messageId":"20190611130320.18499-2-szeder.dev@gmail.com","threadId":"51015","inReplyTo":"20190611130320.18499-1-szeder.dev@gmail.com","subject":"[PATCH v2 1/4] t3404-rebase-interactive: use the 'q_to_cr' helper","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-06-11T13:03:17Z","receivedAt":"2019-06-11T13:03:43Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The test 'rebase -i respects rebase.missingCommitsCheck = warn' checks\nthe output of 'git rebase', including progress lines: it prepares the\nexpected output from a here-doc using LFs at the end of progress\nlines, and converts the command's actual output by replacing the\nprogress lines' CRs with LFs with a one-shot local helper funcion.\nSimilar tests (in t3420) thoroughly checking 'git rebase's output do\nit the other way around: they leave the output of 'git rebase' as-is,\nand prepare the expected output with the necessary CRs in it using the\ncommon 'q_to_cr' test helper function from 'test-lib-functions.sh'.\n\nChange this test to use the 'q_to_cr' test helper to prepare the\nexpected output, too.\n\nThis way this test will be more consistent with other, similar tests,\nand it will make further updates to this test in a later patch of this\nseries just a tad bit simpler.\n\nSigned-off-by: SZEDER Gábor \n---\n t/t3404-rebase-interactive.sh | 15 +++------------\n 1 file changed, 3 insertions(+), 12 deletions(-)\n\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 1723e1a858..e66e95d453 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1304,7 +1304,7 @@ test_expect_success 'rebase -i respects rebase.missingCommitsCheck = ignore' '\n \t\tactual\n '\n \n-cat >expect <expect <actual.2 &&\n-\tcr_to_nl actual &&\n+\t\tgit rebase -i --root 2>actual &&\n \ttest_i18ncmp expect actual &&\n \ttest D = $(git cat-file commit HEAD | sed -ne \\$p)\n '\n-- \n2.22.0.566.g58873a45ff\n\n"},{"id":"377010","messageId":"20190611130320.18499-4-szeder.dev@gmail.com","threadId":"51015","inReplyTo":"20190611130320.18499-1-szeder.dev@gmail.com","subject":"[PATCH v2 3/4] rebase: fix garbled progress display with '-x'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-06-11T13:03:19Z","receivedAt":"2019-06-11T13:03:44Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"When running a command with the 'exec' instruction during an\ninteractive rebase session, or for a range of commits using 'git\nrebase -x', the output can be a bit garbled when the name of the\ncommand is short enough:\n\n $ git rebase -x true HEAD~5\n Executing: true\n Executing: true\n Executing: true\n Executing: true\n Executing: true)\n Successfully rebased and updated refs/heads/master.\n\nNote the ')' at the end of the last line. It gets more garbled as the\nrange of commits increases:\n\n $ git rebase -x true HEAD~50\n Executing: true)\n [ repeated 3 more times ]\n Executing: true0)\n [ repeated 44 more times ]\n Executing: true00)\n Successfully rebased and updated refs/heads/master.\n\nThose extra numbers and ')' are remnants of the previously displayed\n\"Rebasing (N/M)\" progress lines that are usually completely\noverwritten by the \"Executing: \" lines, unless 'cmd' is short and\nthe \"N/M\" part is long.\n\nMake sure that the previously displayed \"Rebasing (N/M)\" line is\ncleared by using the term_clear_line() helper function added in the\nprevious patch.\n\nA couple of other rebase commands print similar messages, e.g.\n\"Stopped at ... \" for the 'edit' or 'break'\ncommands, or the \"Successfully rebased and updated .\" at the\nvery end. These are so long that they practically always overwrite\nthat \"Rebasing (n/m)\" progress line, but let's be prudent, and clear\nthe last line before printing these, too.\n\nThree tests, one in 't3404-rebase-interactive.sh' and two in\n't3420-rebase-autostash.sh' check the full output of 'git rebase' and\nthus are affected by this change, so adjust their expectations to\naccount for the new line clearing.\n\nNote that this patch doesn't completely eliminate the possibility of\nsimilar garbled outputs, e.g. some error messages from rebase or the\n\"Auto-merging \" message from withing the depths of the merge\nmachinery might not be long enough to completely cover the last\n\"Rebasing (N/M)\" line. This patch doesn't do anything about them,\nbecause dealing with them individually would result in way too much\nchurn, while having a catch-all term_clear_line() call in the common\ncode path of pick_commits() would hide the \"Rebasing (N/M)\" line way\ntoo soon, and it would either flicker or be invisible.\n\nSigned-off-by: SZEDER Gábor \n---\n sequencer.c | 17 ++++++++++++++---\n t/t3404-rebase-interactive.sh | 2 +-\n t/t3420-rebase-autostash.sh | 4 ++--\n 3 files changed, 17 insertions(+), 6 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex f88a97fb10..63b09cfceb 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3731,8 +3731,11 @@ static int pick_commits(struct repository *r,\n \t\t\tunlink(git_path_merge_head(the_repository));\n \t\t\tdelete_ref(NULL, \"REBASE_HEAD\", NULL, REF_NO_DEREF);\n \n-\t\t\tif (item->command == TODO_BREAK)\n+\t\t\tif (item->command == TODO_BREAK) {\n+\t\t\t\tif (!opts->verbose)\n+\t\t\t\t\tterm_clear_line();\n \t\t\t\treturn stopped_at_head(r);\n+\t\t\t}\n \t\t}\n \t\tif (item->command <= TODO_SQUASH) {\n \t\t\tif (is_rebase_i(opts))\n@@ -3754,11 +3757,14 @@ static int pick_commits(struct repository *r,\n \t\t\t}\n \t\t\tif (item->command == TODO_EDIT) {\n \t\t\t\tstruct commit *commit = item->commit;\n-\t\t\t\tif (!res)\n+\t\t\t\tif (!res) {\n+\t\t\t\t\tif (!opts->verbose)\n+\t\t\t\t\t\tterm_clear_line();\n \t\t\t\t\tfprintf(stderr,\n \t\t\t\t\t\t_(\"Stopped at %s... %.*s\\n\"),\n \t\t\t\t\t\tshort_commit_name(commit),\n \t\t\t\t\t\titem->arg_len, arg);\n+\t\t\t\t}\n \t\t\t\treturn error_with_patch(r, commit,\n \t\t\t\t\targ, item->arg_len, opts, res, !res);\n \t\t\t}\n@@ -3796,6 +3802,8 @@ static int pick_commits(struct repository *r,\n \t\t\tint saved = *end_of_arg;\n \t\t\tstruct stat st;\n \n+\t\t\tif (!opts->verbose)\n+\t\t\t\tterm_clear_line();\n \t\t\t*end_of_arg = '\\0';\n \t\t\tres = do_exec(r, arg);\n \t\t\t*end_of_arg = saved;\n@@ -3954,10 +3962,13 @@ static int pick_commits(struct repository *r,\n \t\t}\n \t\tapply_autostash(opts);\n \n-\t\tif (!opts->quiet)\n+\t\tif (!opts->quiet) {\n+\t\t\tif (!opts->verbose)\n+\t\t\t\tterm_clear_line();\n \t\t\tfprintf(stderr,\n \t\t\t\t\"Successfully rebased and updated %s.\\n\",\n \t\t\t\thead_ref.buf);\n+\t\t}\n \n \t\tstrbuf_release(&buf);\n \t\tstrbuf_release(&head_ref);\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex e66e95d453..72ce9d393c 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1313,7 +1313,7 @@ To avoid this message, use \"drop\" to explicitly remove a commit.\n Use 'git config rebase.missingCommitsCheck' to change the level of warnings.\n The possible behaviours are: ignore, warn, error.\n \n-Rebasing (1/4)QRebasing (2/4)QRebasing (3/4)QRebasing (4/4)QSuccessfully rebased and updated refs/heads/missing-commit.\n+Rebasing (1/4)QRebasing (2/4)QRebasing (3/4)QRebasing (4/4)QQ QSuccessfully rebased and updated refs/heads/missing-commit.\n EOF\n \n test_expect_success 'rebase -i respects rebase.missingCommitsCheck = warn' '\ndiff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh\nindex 2d1094e483..9186e90127 100755\n--- a/t/t3420-rebase-autostash.sh\n+++ b/t/t3420-rebase-autostash.sh\n@@ -49,7 +49,7 @@ create_expected_success_interactive () {\n \t$(grep \"^Created autostash: [0-9a-f][0-9a-f]*\\$\" actual)\n \tHEAD is now at $(git rev-parse --short feature-branch) third commit\n \tRebasing (1/2)QRebasing (2/2)QApplied autostash.\n-\tSuccessfully rebased and updated refs/heads/rebased-feature-branch.\n+\tQ QSuccessfully rebased and updated refs/heads/rebased-feature-branch.\n \tEOF\n }\n \n@@ -73,7 +73,7 @@ create_expected_failure_interactive () {\n \tRebasing (1/2)QRebasing (2/2)QApplying autostash resulted in conflicts.\n \tYour changes are safe in the stash.\n \tYou can run \"git stash pop\" or \"git stash drop\" at any time.\n-\tSuccessfully rebased and updated refs/heads/rebased-feature-branch.\n+\tQ QSuccessfully rebased and updated refs/heads/rebased-feature-branch.\n \tEOF\n }\n \n-- \n2.22.0.566.g58873a45ff\n\n"},{"id":"377011","messageId":"20190611130320.18499-5-szeder.dev@gmail.com","threadId":"51015","inReplyTo":"20190611130320.18499-1-szeder.dev@gmail.com","subject":"[PATCH v2 4/4] progress: use term_clear_line()","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-06-11T13:03:20Z","receivedAt":"2019-06-11T13:03:46Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"To make sure that the previously displayed progress line is completely\ncovered up when the new line is shorter, commit 545dc345eb (progress:\nbreak too long progress bar lines, 2019-04-12) added a bunch of\ncalculations to figure out how many characters it needs to overwrite\nwith spaces.\n\nUse the just introduced term_clear_line() helper function to, well,\nclear the last line, making all these calculations unnecessary, and\nthus simplifying the code considerably.\n\nThree tests in 't5541-http-push-smart.sh' 'grep' for specific text\nshown in the progress lines at the beginning of the line, but now\nthose lines begin either with the ANSI escape sequence or with the\nterminal width worth of space characters clearing the line. Relax the\n'grep' patterns to match anywhere on the line. Note that only two of\nthese three tests fail without relaxing their 'grep' pattern, but the\nthird looks for the absence of the pattern, so it still succeeds, but\nwithout the adjustment would potentially hide future regressions.\n\nNote also that with this change we no longer need the length of the\npreviously displayed progress line, so the strbuf added to 'struct\nprogress' in d53ba841d4 (progress: assemble percentage and counters in\na strbuf before printing, 2019-04-05) is not strictly necessary\nanymore. We still keep it, though, as it avoids allocating and\nreleasing a strbuf each time the progress is updated.\n\nSigned-off-by: SZEDER Gábor \n---\n progress.c | 28 +++++++++++-----------------\n t/t5541-http-push-smart.sh | 6 +++---\n 2 files changed, 14 insertions(+), 20 deletions(-)\n\ndiff --git a/progress.c b/progress.c\nindex a2e8cf64a8..095dcd0ddf 100644\n--- a/progress.c\n+++ b/progress.c\n@@ -88,7 +88,6 @@ static void display(struct progress *progress, uint64_t n, const char *done)\n \tconst char *tp;\n \tstruct strbuf *counters_sb = &progress->counters_sb;\n \tint show_update = 0;\n-\tint last_count_len = counters_sb->len;\n \n \tif (progress->delay && (!progress_update || --progress->delay))\n \t\treturn;\n@@ -116,26 +115,21 @@ static void display(struct progress *progress, uint64_t n, const char *done)\n \tif (show_update) {\n \t\tif (is_foreground_fd(fileno(stderr)) || done) {\n \t\t\tconst char *eol = done ? done : \"\\r\";\n-\t\t\tsize_t clear_len = counters_sb->len < last_count_len ?\n-\t\t\t\t\tlast_count_len - counters_sb->len + 1 :\n-\t\t\t\t\t0;\n-\t\t\tsize_t progress_line_len = progress->title_len +\n-\t\t\t\t\t\tcounters_sb->len + 2;\n-\t\t\tint cols = term_columns();\n \n+\t\t\tterm_clear_line();\n \t\t\tif (progress->split) {\n-\t\t\t\tfprintf(stderr, \" %s%*s\", counters_sb->buf,\n-\t\t\t\t\t(int) clear_len, eol);\n-\t\t\t} else if (!done && cols < progress_line_len) {\n-\t\t\t\tclear_len = progress->title_len + 1 < cols ?\n-\t\t\t\t\t cols - progress->title_len - 1 : 0;\n-\t\t\t\tfprintf(stderr, \"%s:%*s\\n %s%s\",\n-\t\t\t\t\tprogress->title, (int) clear_len, \"\",\n-\t\t\t\t\tcounters_sb->buf, eol);\n+\t\t\t\tfprintf(stderr, \" %s%s\", counters_sb->buf,\n+\t\t\t\t\teol);\n+\t\t\t} else if (!done &&\n+\t\t\t\t /* The \"+ 2\" accounts for the \": \". */\n+\t\t\t\t term_columns() < progress->title_len +\n+\t\t\t\t\t\t counters_sb->len + 2) {\n+\t\t\t\tfprintf(stderr, \"%s:\\n %s%s\",\n+\t\t\t\t\tprogress->title, counters_sb->buf, eol);\n \t\t\t\tprogress->split = 1;\n \t\t\t} else {\n-\t\t\t\tfprintf(stderr, \"%s: %s%*s\", progress->title,\n-\t\t\t\t\tcounters_sb->buf, (int) clear_len, eol);\n+\t\t\t\tfprintf(stderr, \"%s: %s%s\", progress->title,\n+\t\t\t\t\tcounters_sb->buf, eol);\n \t\t\t}\n \t\t\tfflush(stderr);\n \t\t}\ndiff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh\nindex 8ef8763e06..2e4802e206 100755\n--- a/t/t5541-http-push-smart.sh\n+++ b/t/t5541-http-push-smart.sh\n@@ -213,7 +213,7 @@ test_expect_success TTY 'push shows progress when stderr is a tty' '\n \tcd \"$ROOT_PATH\"/test_repo_clone &&\n \ttest_commit noisy &&\n \ttest_terminal git push >output 2>&1 &&\n-\ttest_i18ngrep \"^Writing objects\" output\n+\ttest_i18ngrep \"Writing objects\" output\n '\n \n test_expect_success TTY 'push --quiet silences status and progress' '\n@@ -228,7 +228,7 @@ test_expect_success TTY 'push --no-progress silences progress but not status' '\n \ttest_commit no-progress &&\n \ttest_terminal git push --no-progress >output 2>&1 &&\n \ttest_i18ngrep \"^To http\" output &&\n-\ttest_i18ngrep ! \"^Writing objects\" output\n+\ttest_i18ngrep ! \"Writing objects\" output\n '\n \n test_expect_success 'push --progress shows progress to non-tty' '\n@@ -236,7 +236,7 @@ test_expect_success 'push --progress shows progress to non-tty' '\n \ttest_commit progress &&\n \tgit push --progress >output 2>&1 &&\n \ttest_i18ngrep \"^To http\" output &&\n-\ttest_i18ngrep \"^Writing objects\" output\n+\ttest_i18ngrep \"Writing objects\" output\n '\n \n test_expect_success 'http push gives sane defaults to reflog' '\n-- \n2.22.0.566.g58873a45ff\n\n"},{"id":"377027","messageId":"xmqq36kflv0f.fsf@gitster-ct.c.googlers.com","threadId":"51015","inReplyTo":"20190611130320.18499-4-szeder.dev@gmail.com","subject":"Re: [PATCH v2 3/4] rebase: fix garbled progress display with '-x'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-06-11T20:36:16Z","receivedAt":"2019-06-11T20:36:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> -Rebasing (1/4)QRebasing (2/4)QRebasing (3/4)QRebasing (4/4)QSuccessfully rebased and updated refs/heads/missing-commit.\n> +Rebasing (1/4)QRebasing (2/4)QRebasing (3/4)QRebasing (4/4)QQ QSuccessfully rebased and updated refs/heads/missing-commit.\n> EOF\n\nYuck, ... but I do not see how else/better this test can be written\nmyself, which makes it a double-yuck X-<\n\nAre we forcing out test to operate under dumb terminal mode and with\na known number of columns?\n"},{"id":"377029","messageId":"xmqqy327kfw1.fsf@gitster-ct.c.googlers.com","threadId":"51015","inReplyTo":"20190611130320.18499-4-szeder.dev@gmail.com","subject":"Re: [PATCH v2 3/4] rebase: fix garbled progress display with '-x'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-06-11T20:48:14Z","receivedAt":"2019-06-11T20:48:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> Make sure that the previously displayed \"Rebasing (N/M)\" line is\n> cleared by using the term_clear_line() helper function added in the\n> previous patch.\n>\n> A couple of other rebase commands print similar messages, e.g.\n> \"Stopped at ... \" for the 'edit' or 'break'\n> commands, or the \"Successfully rebased and updated .\" at the\n> very end. These are so long that they practically always overwrite\n> that \"Rebasing (n/m)\" progress line, but let's be prudent, and clear\n> the last line before printing these, too.\n> ...\n> Note that this patch doesn't completely eliminate the possibility of\n> similar garbled outputs, ...\n> too soon, and it would either flicker or be invisible.\n\nThe user of term_clear_line() in this patch always guard themselves\nbehind \"we do not do this if we are producing verbose output,\" but\nthe proposed log message does not explain why they need to do so.\n\nIs it because it is so obvious to potential readers?\n"},{"id":"377033","messageId":"20190611211151.GG4012@szeder.dev","threadId":"51015","inReplyTo":"xmqq36kflv0f.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 3/4] rebase: fix garbled progress display with '-x'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-06-11T21:11:51Z","receivedAt":"2019-06-11T21:11:57Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Jun 11, 2019 at 01:36:16PM -0700, Junio C Hamano wrote:\n> SZEDER Gábor writes:\n> \n> > -Rebasing (1/4)QRebasing (2/4)QRebasing (3/4)QRebasing (4/4)QSuccessfully rebased and updated refs/heads/missing-commit.\n> > +Rebasing (1/4)QRebasing (2/4)QRebasing (3/4)QRebasing (4/4)QQ QSuccessfully rebased and updated refs/heads/missing-commit.\n> > EOF\n> \n> Yuck, \n\nOh yeah...\n\n>... but I do not see how else/better this test can be written\n> myself, which makes it a double-yuck X-<\n\nPerhaps hiding those spaces behind a helper variable e.g.\n'dump_term_clear_line=Q<80-spaces>Q' and embedding that in the here\ndocs specifying the expected output in these three tests could make it\never so slightly less yuck...\n\n> Are we forcing out test to operate under dumb terminal mode and with\n> a known number of columns?\n\n'test-lib.sh' sets TERM=dumb relatively early on, and in these tests\nwe don't use 'test_terminal' to run 'git rebase', so... yeah. And\nterm_columns() defaults to 80.\n\nHowever, if the terminal were smart, then we would have to deal with\nANSI escape suddenly popping up...\n"},{"id":"377046","messageId":"20190611235052.GH4012@szeder.dev","threadId":"51015","inReplyTo":"xmqqy327kfw1.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 3/4] rebase: fix garbled progress display with '-x'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-06-11T23:50:52Z","receivedAt":"2019-06-11T23:50:58Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Jun 11, 2019 at 01:48:14PM -0700, Junio C Hamano wrote:\n> SZEDER Gábor writes:\n> \n> > Make sure that the previously displayed \"Rebasing (N/M)\" line is\n> > cleared by using the term_clear_line() helper function added in the\n> > previous patch.\n> >\n> > A couple of other rebase commands print similar messages, e.g.\n> > \"Stopped at ... \" for the 'edit' or 'break'\n> > commands, or the \"Successfully rebased and updated .\" at the\n> > very end. These are so long that they practically always overwrite\n> > that \"Rebasing (n/m)\" progress line, but let's be prudent, and clear\n> > the last line before printing these, too.\n> > ...\n> > Note that this patch doesn't completely eliminate the possibility of\n> > similar garbled outputs, ...\n> > too soon, and it would either flicker or be invisible.\n> \n> The user of term_clear_line() in this patch always guard themselves\n> behind \"we do not do this if we are producing verbose output,\" but\n> the proposed log message does not explain why they need to do so.\n\nIn verbose mode there is no progress output that is overwritten and\nmight need to be covered up.\n\n\n"},{"id":"377082","messageId":"xmqqimtakc4x.fsf@gitster-ct.c.googlers.com","threadId":"51015","inReplyTo":"xmqqy327kfw1.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 3/4] rebase: fix garbled progress display with '-x'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-06-12T16:21:34Z","receivedAt":"2019-06-12T16:21:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> SZEDER Gábor writes:\n>\n>> Make sure that the previously displayed \"Rebasing (N/M)\" line is\n>> cleared by using the term_clear_line() helper function added in the\n>> previous patch.\n>>\n>> A couple of other rebase commands print similar messages, e.g.\n>> \"Stopped at ... \" for the 'edit' or 'break'\n>> commands, or the \"Successfully rebased and updated .\" at the\n>> very end. These are so long that they practically always overwrite\n>> that \"Rebasing (n/m)\" progress line, but let's be prudent, and clear\n>> the last line before printing these, too.\n>> ...\n>> Note that this patch doesn't completely eliminate the possibility of\n>> similar garbled outputs, ...\n>> too soon, and it would either flicker or be invisible.\n>\n> The user of term_clear_line() in this patch always guard themselves\n> behind \"we do not do this if we are producing verbose output,\" but\n> the proposed log message does not explain why they need to do so.\n>\n> Is it because it is so obvious to potential readers?\n\nAnswering myself, it is due to this part in sequencer.c:\n\n 3721 if (!opts->quiet)\n 3722 fprintf(stderr, \"Rebasing (%d/%d)%s\",\n 3723 todo_list->done_nr,\n 3724 todo_list->total_nr,\n 3725 opts->verbose ? \"\\n\" : \"\\r\");\n\nThat is, under 'verbose' mode, we do not try to reuse a single line\nto show different steps of rebase in the progress output, but\nconsume one line per one step, so it would not be necessary to erase\nwhat is previously written on the line.\n\nI do not think it is so obvious, though.\n"},{"id":"377104","messageId":"nycvar.QRO.7.76.6.1906122056570.789@QRFXGBC-DHN364S.ybpnyqbznva","threadId":"51015","inReplyTo":"20190611211151.GG4012@szeder.dev","subject":"Re: [PATCH v2 3/4] rebase: fix garbled progress display with '-x'","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-06-12T19:14:40Z","receivedAt":"2019-06-12T19:14:53Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 11 Jun 2019, SZEDER Gábor wrote:\n\n> On Tue, Jun 11, 2019 at 01:36:16PM -0700, Junio C Hamano wrote:\n> > SZEDER Gábor writes:\n> >\n> > > -Rebasing (1/4)QRebasing (2/4)QRebasing (3/4)QRebasing (4/4)QSuccessfully rebased and updated refs/heads/missing-commit.\n> > > +Rebasing (1/4)QRebasing (2/4)QRebasing (3/4)QRebasing (4/4)QQ QSuccessfully rebased and updated refs/heads/missing-commit.\n> > > EOF\n> >\n> > Yuck,\n>\n> Oh yeah...\n>\n> >... but I do not see how else/better this test can be written\n> > myself, which makes it a double-yuck X-<\n>\n> Perhaps hiding those spaces behind a helper variable e.g.\n> 'dump_term_clear_line=Q<80-spaces>Q' and embedding that in the here\n> docs specifying the expected output in these three tests could make it\n> ever so slightly less yuck...\n>\n> > Are we forcing out test to operate under dumb terminal mode and with\n> > a known number of columns?\n>\n> 'test-lib.sh' sets TERM=dumb relatively early on, and in these tests\n> we don't use 'test_terminal' to run 'git rebase', so... yeah. And\n> term_columns() defaults to 80.\n>\n> However, if the terminal were smart, then we would have to deal with\n> ANSI escape suddenly popping up...\n\nAnd I fear that is *exactly* what makes\nhttps://dev.azure.com/gitgitgadget/git/_build/results?buildId=10539&view=ms.vss-test-web.build-test-results-tab\nfail...\n\nYou cannot easily see it in that output (probably because of incorrectly\nencoded Escape sequences in the `.xml` output), so I'll paste what I see\nhere, locally, when running `t3404-*.sh -i -V -x`:\n\n-- snip --\n[...]\nok 99 - rebase -i respects rebase.missingCommitsCheck = ignore\n\nexpecting success:\n test_config rebase.missingCommitsCheck warn &&\n rebase_setup_and_clean missing-commit &&\n set_fake_editor &&\n FAKE_LINES=\"1 2 3 4\" \\\n git rebase -i --root 2>actual &&\n test_i18ncmp expect actual &&\n test D = $(git cat-file commit HEAD | sed -ne \\$p)\n\n++ test_config rebase.missingCommitsCheck warn\n++ config_dir=\n++ test rebase.missingCommitsCheck = -C\n++ test_when_finished 'test_unconfig '\\''rebase.missingCommitsCheck'\\'''\n++ test 0 = 0\n++ test_cleanup='{ test_unconfig '\\''rebase.missingCommitsCheck'\\''\n } && (exit \"$eval_ret\"); eval_ret=$?; :'\n++ git config rebase.missingCommitsCheck warn\n++ rebase_setup_and_clean missing-commit\n++ test_when_finished '\n git checkout master &&\n test_might_fail git branch -D missing-commit &&\n test_might_fail git rebase --abort\n '\n++ test 0 = 0\n++ test_cleanup='{\n git checkout master &&\n test_might_fail git branch -D missing-commit &&\n test_might_fail git rebase --abort\n\n } && (exit \"$eval_ret\"); eval_ret=$?; { test_unconfig '\\''rebase.missingCommitsCheck'\\''\n } && (exit \"$eval_ret\"); eval_ret=$?; :'\n++ git checkout -b missing-commit master\nSwitched to a new branch 'missing-commit'\n++ set_fake_editor\n++ write_script fake-editor.sh\n++ echo '#!/bin/sh'\n++ cat\n++ chmod +x fake-editor.sh\n+++ pwd\n+++ builtin pwd -W\n++ 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'\n++ FAKE_EDITOR='C:/git-sdk-64/usr/src/git/.git/worktrees/wip/temp-rebase-123/t/trash directory.t3404-rebase-interactive/fake-editor.sh'\n++ export FAKE_EDITOR\n++ EDITOR='\"$FAKE_EDITOR\"'\n++ export EDITOR\n++ FAKE_LINES='1 2 3 4'\n++ git rebase -i --root\nrebase -i script before editing:\npick 6e62bf890e21 A\npick 313fe965c048 B\npick d0f65f2f81ee C\npick 0547e3f1350d D\npick 8f99a4f1fbbd E\n\nrebase -i script after editing:\npick 6e62bf890e21 A\npick 313fe965c048 B\npick d0f65f2f81ee C\npick 0547e3f1350d D\n++ test_i18ncmp expect actual\n++ test_have_prereq C_LOCALE_OUTPUT\n++ save_IFS='\n'\n++ IFS=,\n++ set -- C_LOCALE_OUTPUT\n++ IFS='\n'\n++ total_prereq=0\n++ ok_prereq=0\n++ missing_prereq=\n++ for prerequisite in \"$@\"\n++ case \"$prerequisite\" in\n++ negative_prereq=\n++ case \" $lazily_tested_prereq \" in\n++ case \" $lazily_testable_prereq \" in\n++ total_prereq=1\n++ case \"$satisfied_prereq\" in\n++ satisfied_this_prereq=t\n++ case \"$satisfied_this_prereq,$negative_prereq\" in\n++ ok_prereq=1\n++ test 1 = 1\n++ test_cmp expect actual\n++ GIT_ALLOC_LIMIT=0\n++ test-tool cmp expect actual\ndiff --git a/expect b/actual\nindex 05fcfcb..9555e34 100644\n--- a/expect\n+++ b/actual\n@@ -6,4 +6,4 @@ To avoid this message, use \"drop\" to explicitly remove a commit.\n Use 'git config rebase.missingCommitsCheck' to change the level of warnings.\n The possible behaviours are: ignore, warn, error.\n\n-Rebasing (1/4)^MRebasing (2/4)^MRebasing (3/4)^MRebasing (4/4)^M ^MSuccessfully rebased and updated refs/heads/missing-commit.\n+Rebasing (1/4)^MRebasing (2/4)^MRebasing (3/4)^MRebasing (4/4)^MESC[KSuccessfully rebased and updated refs/heads/missing-commit.\nerror: last command exited with $?=1\nnot ok 100 - rebase -i respects rebase.missingCommitsCheck = warn\n#\n# test_config rebase.missingCommitsCheck warn &&\n# rebase_setup_and_clean missing-commit &&\n# set_fake_editor &&\n# FAKE_LINES=\"1 2 3 4\" \\\n# git rebase -i --root 2>actual &&\n# test_i18ncmp expect actual &&\n# test D = $(git cat-file commit HEAD | sed -ne \\$p)\n#\n-- snap --\n\n(I copy-pasted this from the output of `less` so that the control sequences can be seen.)\n\nTo be utterly honest, I really fail to see a reason why a test case that\npurports to verify that `git rebase -i` respects\n`rebase.missingCommitsCheck=warn` should fail when the progress is shown in an\nunexpected format.\n\nIt strikes me as yet another poorly written test case that fails to catch the\nintended regressions, instead it catches a bug *in the test case itself* when\nlegitimate changes are made to the progress code.\n\nNothing in a test suite is worse than a test that fails (or succeeds) for the\nwrong reasons.\n\nTo make things even worse, the code that generates that `expect` file is\noutside the test case.\n\nHere it is, in its full \"glory\":\n\n-- snip --\nq_to_cr >expect <actual-skip-progress &&\n\ttest_i18ncmp expect actual-skip-progress\n\nThis should obviously be made as a separate, introductory patch (probably with\na less scathing commit message than my comments above would suggest).\n\nAnd that would also remove the double-yuck.\n\nCiao,\nDscho\n"},{"id":"377109","messageId":"20190612194106.GJ4012@szeder.dev","threadId":"51015","inReplyTo":"nycvar.QRO.7.76.6.1906122056570.789@QRFXGBC-DHN364S.ybpnyqbznva","subject":"Re: [PATCH v2 3/4] rebase: fix garbled progress display with '-x'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-06-12T19:41:06Z","receivedAt":"2019-06-12T19:41:13Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Wed, Jun 12, 2019 at 09:14:40PM +0200, Johannes Schindelin wrote:\n> Hi,\n> \n> On Tue, 11 Jun 2019, SZEDER Gábor wrote:\n> \n> > On Tue, Jun 11, 2019 at 01:36:16PM -0700, Junio C Hamano wrote:\n> > > SZEDER Gábor writes:\n> > >\n> > > > -Rebasing (1/4)QRebasing (2/4)QRebasing (3/4)QRebasing (4/4)QSuccessfully rebased and updated refs/heads/missing-commit.\n> > > > +Rebasing (1/4)QRebasing (2/4)QRebasing (3/4)QRebasing (4/4)QQ QSuccessfully rebased and updated refs/heads/missing-commit.\n> > > > EOF\n> > >\n> > > Yuck,\n> >\n> > Oh yeah...\n> >\n> > >... but I do not see how else/better this test can be written\n> > > myself, which makes it a double-yuck X-<\n> >\n> > Perhaps hiding those spaces behind a helper variable e.g.\n> > 'dump_term_clear_line=Q<80-spaces>Q' and embedding that in the here\n> > docs specifying the expected output in these three tests could make it\n> > ever so slightly less yuck...\n> >\n> > > Are we forcing out test to operate under dumb terminal mode and with\n> > > a known number of columns?\n> >\n> > 'test-lib.sh' sets TERM=dumb relatively early on, and in these tests\n> > we don't use 'test_terminal' to run 'git rebase', so... yeah. And\n> > term_columns() defaults to 80.\n> >\n> > However, if the terminal were smart, then we would have to deal with\n> > ANSI escape suddenly popping up...\n> \n> And I fear that is *exactly* what makes\n> https://dev.azure.com/gitgitgadget/git/_build/results?buildId=10539&view=ms.vss-test-web.build-test-results-tab\n> fail...\n\nIsn't it a sign of a problem in that Windows test environment that\nit mistakenly believes that the terminal is smart, even though it has\nbeen explicitly set to dumb?\n\n"},{"id":"377111","messageId":"nycvar.QRO.7.76.6.1906122158500.789@QRFXGBC-DHN364S.ybpnyqbznva","threadId":"51015","inReplyTo":"20190611130320.18499-5-szeder.dev@gmail.com","subject":"Re: [PATCH v2 4/4] progress: use term_clear_line()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-06-12T20:00:40Z","receivedAt":"2019-06-12T20:00:49Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 11 Jun 2019, SZEDER Gábor wrote:\n\n> diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh\n> index 8ef8763e06..2e4802e206 100755\n> --- a/t/t5541-http-push-smart.sh\n> +++ b/t/t5541-http-push-smart.sh\n> @@ -213,7 +213,7 @@ test_expect_success TTY 'push shows progress when stderr is a tty' '\n> \tcd \"$ROOT_PATH\"/test_repo_clone &&\n> \ttest_commit noisy &&\n> \ttest_terminal git push >output 2>&1 &&\n> -\ttest_i18ngrep \"^Writing objects\" output\n> +\ttest_i18ngrep \"Writing objects\" output\n\nI guess the problem is that now the \"Writing objects\" is preceded by a CR\ninstead of a NL? If so, maybe the `cr_to_nl` function could be introduced\ninto `test-lib-functions.sh` and be used here and in the subsequent test\ncases?\n\n(Just an idea, as you seemed unhappy about these adjustments according to\nthe cover letter.)\n\nCiao,\nDscho\n\n> '\n>\n> test_expect_success TTY 'push --quiet silences status and progress' '\n> @@ -228,7 +228,7 @@ test_expect_success TTY 'push --no-progress silences progress but not status' '\n> \ttest_commit no-progress &&\n> \ttest_terminal git push --no-progress >output 2>&1 &&\n> \ttest_i18ngrep \"^To http\" output &&\n> -\ttest_i18ngrep ! \"^Writing objects\" output\n> +\ttest_i18ngrep ! \"Writing objects\" output\n> '\n>\n> test_expect_success 'push --progress shows progress to non-tty' '\n> @@ -236,7 +236,7 @@ test_expect_success 'push --progress shows progress to non-tty' '\n> \ttest_commit progress &&\n> \tgit push --progress >output 2>&1 &&\n> \ttest_i18ngrep \"^To http\" output &&\n> -\ttest_i18ngrep \"^Writing objects\" output\n> +\ttest_i18ngrep \"Writing objects\" output\n> '\n>\n> test_expect_success 'http push gives sane defaults to reflog' '\n> --\n> 2.22.0.566.g58873a45ff\n>\n>\n"},{"id":"377133","messageId":"nycvar.QRO.7.76.6.1906130952360.42@tvgsbejvaqbjf.bet","threadId":"51015","inReplyTo":"20190612194106.GJ4012@szeder.dev","subject":"Re: [PATCH v2 3/4] rebase: fix garbled progress display with '-x'","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-06-13T07:54:12Z","receivedAt":"2019-06-13T16:34:12Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 12 Jun 2019, SZEDER Gábor wrote:\n\n> On Wed, Jun 12, 2019 at 09:14:40PM +0200, Johannes Schindelin wrote:\n> >\n> > On Tue, 11 Jun 2019, SZEDER Gábor wrote:\n> >\n> > > On Tue, Jun 11, 2019 at 01:36:16PM -0700, Junio C Hamano wrote:\n> > > > SZEDER Gábor writes:\n> > > >\n> > > > > -Rebasing (1/4)QRebasing (2/4)QRebasing (3/4)QRebasing (4/4)QSuccessfully rebased and updated refs/heads/missing-commit.\n> > > > > +Rebasing (1/4)QRebasing (2/4)QRebasing (3/4)QRebasing (4/4)QQ QSuccessfully rebased and updated refs/heads/missing-commit.\n> > > > > EOF\n> > > >\n> > > > Yuck,\n> > >\n> > > Oh yeah...\n> > >\n> > > >... but I do not see how else/better this test can be written\n> > > > myself, which makes it a double-yuck X-<\n> > >\n> > > Perhaps hiding those spaces behind a helper variable e.g.\n> > > 'dump_term_clear_line=Q<80-spaces>Q' and embedding that in the here\n> > > docs specifying the expected output in these three tests could make it\n> > > ever so slightly less yuck...\n> > >\n> > > > Are we forcing out test to operate under dumb terminal mode and with\n> > > > a known number of columns?\n> > >\n> > > 'test-lib.sh' sets TERM=dumb relatively early on, and in these tests\n> > > we don't use 'test_terminal' to run 'git rebase', so... yeah. And\n> > > term_columns() defaults to 80.\n> > >\n> > > However, if the terminal were smart, then we would have to deal with\n> > > ANSI escape suddenly popping up...\n> >\n> > And I fear that is *exactly* what makes\n> > https://dev.azure.com/gitgitgadget/git/_build/results?buildId=10539&view=ms.vss-test-web.build-test-results-tab\n> > fail...\n>\n> Isn't it a sign of a problem in that Windows test environment that\n> it mistakenly believes that the terminal is smart, even though it has\n> been explicitly set to dumb?\n\nYes, it looks like some odd decision there.\n\nBut the more important take-away from my mail is that we do not even want\nthose test cases to depend on the vagaries of the current progress\nmachinery.\n\nCiao,\nDscho\n"},{"id":"377250","messageId":"nycvar.QRO.7.76.6.1906141356140.44@tvgsbejvaqbjf.bet","threadId":"51015","inReplyTo":"20190612194106.GJ4012@szeder.dev","subject":"Re: [PATCH v2 3/4] rebase: fix garbled progress display with '-x'","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-06-14T18:42:29Z","receivedAt":"2019-06-14T18:42:29Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 12 Jun 2019, SZEDER Gábor wrote:\n\n> On Wed, Jun 12, 2019 at 09:14:40PM +0200, Johannes Schindelin wrote:\n>\n> > On Tue, 11 Jun 2019, SZEDER Gábor wrote:\n> >\n> > > On Tue, Jun 11, 2019 at 01:36:16PM -0700, Junio C Hamano wrote:\n> > > > SZEDER Gábor writes:\n> > > >\n> > > > > -Rebasing (1/4)QRebasing (2/4)QRebasing (3/4)QRebasing (4/4)QSuccessfully rebased and updated refs/heads/missing-commit.\n> > > > > +Rebasing (1/4)QRebasing (2/4)QRebasing (3/4)QRebasing (4/4)QQ QSuccessfully rebased and updated refs/heads/missing-commit.\n> > > > > EOF\n> > > >\n> > > > Yuck,\n> > >\n> > > Oh yeah...\n> > >\n> > > >... but I do not see how else/better this test can be written\n> > > > myself, which makes it a double-yuck X-<\n> > >\n> > > Perhaps hiding those spaces behind a helper variable e.g.\n> > > 'dump_term_clear_line=Q<80-spaces>Q' and embedding that in the here\n> > > docs specifying the expected output in these three tests could make it\n> > > ever so slightly less yuck...\n> > >\n> > > > Are we forcing out test to operate under dumb terminal mode and with\n> > > > a known number of columns?\n> > >\n> > > 'test-lib.sh' sets TERM=dumb relatively early on, and in these tests\n> > > we don't use 'test_terminal' to run 'git rebase', so... yeah. And\n> > > term_columns() defaults to 80.\n> > >\n> > > However, if the terminal were smart, then we would have to deal with\n> > > ANSI escape suddenly popping up...\n> >\n> > And I fear that is *exactly* what makes\n> > https://dev.azure.com/gitgitgadget/git/_build/results?buildId=10539&view=ms.vss-test-web.build-test-results-tab\n> > fail...\n>\n> Isn't it a sign of a problem in that Windows test environment that\n> it mistakenly believes that the terminal is smart, even though it has\n> been explicitly set to dumb?\n\nI investigated this today.\n\nMind you, I still think that it is totally inappropriate for a test case\nwith the title 'rebase -i respects rebase.missingCommitsCheck = warn' to\nvalidate the expected progress output, in particular since it verifies the\nprogress on non-sophisticated terminals, i.e. the totally least\ninteresting and least common scenario.\n\nIn short: I stand by my suggestion to fix these tests (i.e. ignore the\nprogress altogether) in a preparatory patch in your patch series.\n\nThe investigation why the test fails on Windows (due to the progress being\ndisplayed for TERM=cygwin instead of TERM=dumb) took quite a bit longer\nthan I had originally anticipated, essentially because I did not expect to\nuncover a bug that I introduced into Git for Windows v2.x apparently from\nday one of the v2.x era.\n\nIn case you are interested in the details, please read on, otherwise just\nmark this mail as read and move on.\n\nStill with me? Well, here you go, enjoy the ride.\n\nThere are quite a few interesting bits about this bug, and I have to start\nby stating that in DOS, there was no difference between empty values of\nenvironment variables and unset environment variables. In other words,\nthere was no way to distinguish between the equivalent of `export x=` and\n`unset x`. Back in the days, this was obviously perceived as reasonable,\nand I kind of agree given my own difficulty to describe the problem\nclearly.\n\nNow, in the Win32 API there is a relatively easy way to distinguish\nbetween those values: if the return value of `GetEnvironmentVariableW()`\n(which indicates the length of the value) is 0 *and* `GetLastError()`\nreturns `ERROR_ENVVAR_NOT_FOUND`, then the environment variable is unset,\nif it instead returns `ERROR_SUCCESS`, then it is set, and the value is\nthe empty string.\n\nSide note: if you want to rely on this behavior, you will most likely want\nto call `SetLastError(ERROR_SUCCESS)` before querying the environment, as\nthere seem to be conditions where the last error is not re-set to that\nvalue even if the call succeeded.\n\nSince Cygwin started really, really early in the history of Windows (even\nsupporting Windows 95 at some stage), it emulates the DOS behavior, not\nthe Win32 API behavior, and simply skips environment variables with empty\nvalues when spawning non-Cygwin programs. In other words, it pretends that\nthey are unset instead.\n\nGit for Windows' Bash (which runs the test suite) is an MSYS2 program, and\nsince MSYS2 is based on Cygwin, inherits this behavior, and since\n`git.exe` is a non-MSYS2 program, there would be no way for the test suite\nto set environment variables to the empty value and have Git respect that.\n\nThis broke t/t3301-notes.sh (because it sets GIT_NOTES_REF= and\nGIT_NOTES_DISPLAY_REF= to override the configured settings), and therefore\nI came up with this fix in February 2015:\n\nhttps://github.com/git-for-windows/msys2-runtime/commit/c19199cc14ee\n\nIt tells the MSYS2 runtime to *keep* environment variables with empty\nvalues.\n\nNote: this fix was really made in order to let Git for Windows' test suite\npass, for no other reason. And it was not accepted by the MSYS2 project,\nso this really only affects Git for Windows.\n\nThat fix seemed to work at the time (and maybe it really, really did), and\nit seemed to work, still, until my investigation that took the better part\nof today revealed that my fix was buggy. Under certain circumstances\n(which I believe have to do with the environment variable referring to a\nUnix-y path at some point, which is the case for `SHELL`), the subsequent\n`getwinenv()` call mishandles empty values. It tries to convert them from\na Unix-y path (that looks like an absolute Unix path, but it really is\nrooted in MSYS2's top-level directory, identified as the second-level\nparent directory of `msys-2.0.dll`) to a Windows path, and failing that,\nit replaces the `SHELL=` by a NUL character.\n\nThe `SHELL` thing here is important, as t/t3404-rebase-interactive.sh sets\nthis to the empty value explicitly:\n\nhttps://github.com/git/git/blob/v2.22.0/t/t3404-rebase-interactive.sh#L63-L68\n\nSo instead of a `SHELL=\\0` in the middle of the environment block, Git for\nWindows' MSYS2 runtime inserts a `\\0\\0`. That, however, is the marker for\nthe end of the environment block, and as the environment has been sorted\nbefore being converted in order to launch a non-MSYS2 program (in this\ncase, `git.exe`), the `TERM=dumb` setting is lost.\n\nEven worse, for unrelated reasons, `git.exe` defaults to setting\n`TERM=cygwin` if `TERM` is unset.\n\nI hope you, dear reader, can appreciate the number of circumstances that\nhad to come together to trigger this bug.\n\nThe fix with which I came up can be adored here:\n\nhttps://github.com/git-for-windows/msys2-runtime/commit/c10b4185a35f\n\nI tested this locally and will re-test as soon as a new MSYS2 runtime has\nbeen deployed into Git for Windows' SDK.\n\nCiao,\nDscho\n"},{"id":"377365","messageId":"20190617184047.GK31952@szeder.dev","threadId":"51015","inReplyTo":"nycvar.QRO.7.76.6.1906141356140.44@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 3/4] rebase: fix garbled progress display with '-x'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-06-17T18:40:47Z","receivedAt":"2019-06-17T18:40:53Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Jun 14, 2019 at 08:42:29PM +0200, Johannes Schindelin wrote:\n\n> > > > However, if the terminal were smart, then we would have to deal with\n> > > > ANSI escape suddenly popping up...\n> > >\n> > > And I fear that is *exactly* what makes\n> > > https://dev.azure.com/gitgitgadget/git/_build/results?buildId=10539&view=ms.vss-test-web.build-test-results-tab\n> > > fail...\n> >\n> > Isn't it a sign of a problem in that Windows test environment that\n> > it mistakenly believes that the terminal is smart, even though it has\n> > been explicitly set to dumb?\n> \n> I investigated this today.\n> \n> Mind you, I still think that it is totally inappropriate for a test case\n> with the title 'rebase -i respects rebase.missingCommitsCheck = warn' to\n> validate the expected progress output, in particular since it verifies the\n> progress on non-sophisticated terminals, i.e. the totally least\n> interesting and least common scenario.\n> \n> In short: I stand by my suggestion to fix these tests (i.e. ignore the\n> progress altogether) in a preparatory patch in your patch series.\n\nI agree and will try to get around to it in the coming days.\n\n> In case you are interested in the details, please read on, otherwise just\n> mark this mail as read and move on.\n> \n> Still with me? Well, here you go, enjoy the ride.\n\nSure!\n\n[...]\n\n> I hope you, dear reader, can appreciate the number of circumstances that\n> had to come together to trigger this bug.\n\nThank you, it was both educational and entertaining :)\n\n"},{"id":"377368","messageId":"20190617191328.GL31952@szeder.dev","threadId":"51015","inReplyTo":"nycvar.QRO.7.76.6.1906141356140.44@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 3/4] rebase: fix garbled progress display with '-x'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-06-17T19:13:28Z","receivedAt":"2019-06-17T19:13:35Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Jun 14, 2019 at 08:42:29PM +0200, Johannes Schindelin wrote:\n\n> The `SHELL` thing here is important, as t/t3404-rebase-interactive.sh sets\n> this to the empty value explicitly:\n> \n> https://github.com/git/git/blob/v2.22.0/t/t3404-rebase-interactive.sh#L63-L68\n\nHmm, hang on for a sec. Why does it set SHELL to empty?\n\nSo t3404 sets SHELL to the empty value since cd035b1cef\n(rebase -i: add exec command to launch a shell command, 2010-08-10),\nand the in-test comment highlighted on the above link says:\n\n # \"exec\" commands are run with the user shell by default, but this may\n # be non-POSIX. For example, if SHELL=zsh then \">file\" doesn't work\n # to create a file. Unsetting SHELL avoids such non-portable behavior\n # in tests. It must be exported for it to take effect where needed.\n SHELL=\n\nFurthermore, the corresponding documentation added in cd035b1cef\nsays the following:\n\n The \"exec\" command launches the command in a shell (the one specified\n in `$SHELL`, or the default shell if `$SHELL` is not set)\n\nIOW if SHELL is not set/empty, then it will run the default '/bin/sh',\nbut who knows what it might be, there's no guarantee that it's a sane\nPOSIX shell. That's why we have \"Define SHELL_PATH to a POSIX shell\nif your /bin/sh is broken.\" very near to the top of our Makefile.\n\nSo while setting SHELL to the empty value might indeed avoid\nnon-portable behavior when the user in general prefers a non-POSIX\nshell, it wouldn't help if '/bin/sh' is broken. For that it should be\nset to $SHELL_PATH (or perhaps $TEST_SHELL_PATH nowadays...).\n\nThough cd035b1cef is now close to 9 years old, plenty of time for\nsomebody to run into this issue in the wild and speak up...\n\n"},{"id":"377369","messageId":"20190617192522.GM31952@szeder.dev","threadId":"51015","inReplyTo":"20190617191328.GL31952@szeder.dev","subject":"Re: [PATCH v2 3/4] rebase: fix garbled progress display with '-x'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-06-17T19:25:22Z","receivedAt":"2019-06-17T19:25:27Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"[resend to Matthieu's current email address]\n\nOn Mon, Jun 17, 2019 at 09:13:28PM +0200, SZEDER Gábor wrote:\n> On Fri, Jun 14, 2019 at 08:42:29PM +0200, Johannes Schindelin wrote:\n> \n> > The `SHELL` thing here is important, as t/t3404-rebase-interactive.sh sets\n> > this to the empty value explicitly:\n> > \n> > https://github.com/git/git/blob/v2.22.0/t/t3404-rebase-interactive.sh#L63-L68\n> \n> Hmm, hang on for a sec. Why does it set SHELL to empty?\n> \n> So t3404 sets SHELL to the empty value since cd035b1cef\n> (rebase -i: add exec command to launch a shell command, 2010-08-10),\n> and the in-test comment highlighted on the above link says:\n> \n> # \"exec\" commands are run with the user shell by default, but this may\n> # be non-POSIX. For example, if SHELL=zsh then \">file\" doesn't work\n> # to create a file. Unsetting SHELL avoids such non-portable behavior\n> # in tests. It must be exported for it to take effect where needed.\n> SHELL=\n> \n> Furthermore, the corresponding documentation added in cd035b1cef\n> says the following:\n> \n> The \"exec\" command launches the command in a shell (the one specified\n> in `$SHELL`, or the default shell if `$SHELL` is not set)\n> \n> IOW if SHELL is not set/empty, then it will run the default '/bin/sh',\n> but who knows what it might be, there's no guarantee that it's a sane\n> POSIX shell. That's why we have \"Define SHELL_PATH to a POSIX shell\n> if your /bin/sh is broken.\" very near to the top of our Makefile.\n> \n> So while setting SHELL to the empty value might indeed avoid\n> non-portable behavior when the user in general prefers a non-POSIX\n> shell, it wouldn't help if '/bin/sh' is broken. For that it should be\n> set to $SHELL_PATH (or perhaps $TEST_SHELL_PATH nowadays...).\n> \n> Though cd035b1cef is now close to 9 years old, plenty of time for\n> somebody to run into this issue in the wild and speak up...\n> \n"},{"id":"377910","messageId":"20190624181318.17388-1-szeder.dev@gmail.com","threadId":"51015","inReplyTo":"20190611130320.18499-1-szeder.dev@gmail.com","subject":"[PATCH v3 0/5] rebase/progress: add and use term_clear_line()","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-06-24T18:13:13Z","receivedAt":"2019-06-24T18:13:29Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"This series fixes some garbled progress display in 'git rebase' and\nsimplifies clearing up the progress display in general.\n\nOnly slight updates since the previous version to remove one of the\n\"Yuck\"s by making the offending test in t3404 more focused, making\nthe first patch of the previous version pointless. There is also a \na preparatory cleanup patch 't3404: modernize here doc style'.\n\n\nv2: https://public-inbox.org/git/20190611130320.18499-1-szeder.dev@gmail.com/T/#u\n\n\nSZEDER Gábor (5):\n t3404: modernize here doc style\n t3404: make the 'rebase.missingCommitsCheck=ignore' test more focused\n pager: add a helper function to clear the last line in the terminal\n rebase: fix garbled progress display with '-x'\n progress: use term_clear_line()\n\n cache.h | 1 +\n editor.c | 6 +-\n pager.c | 20 ++++++\n progress.c | 28 ++++----\n sequencer.c | 17 ++++-\n t/t3404-rebase-interactive.sh | 120 ++++++++++++++--------------------\n t/t3420-rebase-autostash.sh | 4 +-\n t/t5541-http-push-smart.sh | 6 +-\n 8 files changed, 104 insertions(+), 98 deletions(-)\n\nRange-diff:\n1: b392128604 < -: ---------- t3404-rebase-interactive: use the 'q_to_cr' helper\n-: ---------- > 1: 4372a3cde4 t3404: modernize here doc style\n-: ---------- > 2: 5444f547c5 t3404: make the 'rebase.missingCommitsCheck=ignore' test more focused\n2: b48f49c114 = 3: 5ebf218cb9 pager: add a helper function to clear the last line in the terminal\n3: b25363c7c3 ! 4: 51cd5ccd46 rebase: fix garbled progress display with '-x'\n @@ -39,13 +39,13 @@\n \"Stopped at ... \" for the 'edit' or 'break'\n commands, or the \"Successfully rebased and updated .\" at the\n very end. These are so long that they practically always overwrite\n - that \"Rebasing (n/m)\" progress line, but let's be prudent, and clear\n + that \"Rebasing (N/M)\" progress line, but let's be prudent, and clear\n the last line before printing these, too.\n \n - Three tests, one in 't3404-rebase-interactive.sh' and two in\n - 't3420-rebase-autostash.sh' check the full output of 'git rebase' and\n - thus are affected by this change, so adjust their expectations to\n - account for the new line clearing.\n + In 't3420-rebase-autostash.sh' two helper functions prepare the\n + expected output of four tests that check the full output of 'git\n + rebase' and thus are affected by this change, so adjust their\n + expectations to account for the new line clearing.\n \n Note that this patch doesn't completely eliminate the possibility of\n similar garbled outputs, e.g. some error messages from rebase or the\n @@ -116,19 +116,6 @@\n \t\tstrbuf_release(&buf);\n \t\tstrbuf_release(&head_ref);\n \n - diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n - --- a/t/t3404-rebase-interactive.sh\n - +++ b/t/t3404-rebase-interactive.sh\n -@@\n - Use 'git config rebase.missingCommitsCheck' to change the level of warnings.\n - The possible behaviours are: ignore, warn, error.\n - \n --Rebasing (1/4)QRebasing (2/4)QRebasing (3/4)QRebasing (4/4)QSuccessfully rebased and updated refs/heads/missing-commit.\n -+Rebasing (1/4)QRebasing (2/4)QRebasing (3/4)QRebasing (4/4)QQ QSuccessfully rebased and updated refs/heads/missing-commit.\n - EOF\n - \n - test_expect_success 'rebase -i respects rebase.missingCommitsCheck = warn' '\n -\n diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh\n --- a/t/t3420-rebase-autostash.sh\n +++ b/t/t3420-rebase-autostash.sh\n4: ec85b386fd = 5: 09642a458e progress: use term_clear_line()\n-- \n2.22.0.589.g5bd7971b91\n\n"},{"id":"377911","messageId":"20190624181318.17388-3-szeder.dev@gmail.com","threadId":"51015","inReplyTo":"20190624181318.17388-1-szeder.dev@gmail.com","subject":"[PATCH v3 2/5] t3404: make the 'rebase.missingCommitsCheck=ignore' test more focused","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-06-24T18:13:15Z","receivedAt":"2019-06-24T18:13:30Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The test 'rebase -i respects rebase.missingCommitsCheck = warn' is\nmainly interested in the warning about the dropped commits, but it\nchecks the whole output of 'git rebase', including progress lines and\nwhat not that are not at all relevant to 'rebase.missingCommitsCheck',\nbut make it necessary to update this test whenever e.g. the way we\nshow progress is updated (as it will happen in one of the later\npatches of this series).\n\nModify the test to verify only the first four lines of 'git rebase's\noutput that contain all the important lines, notably the line\ncontaining the \"Warning:\" itself and the oneline log of the dropped\ncommit.\n\nSigned-off-by: SZEDER Gábor \n---\n t/t3404-rebase-interactive.sh | 15 +--------------\n 1 file changed, 1 insertion(+), 14 deletions(-)\n\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 9146f9d47b..0b8267c97c 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1299,32 +1299,19 @@ test_expect_success 'rebase -i respects rebase.missingCommitsCheck = ignore' '\n \t\tactual\n '\n \n-cr_to_nl () {\n-\ttr '\\015' '\\012'\n-}\n-\n test_expect_success 'rebase -i respects rebase.missingCommitsCheck = warn' '\n \tcat >expect <<-EOF &&\n \tWarning: some commits may have been dropped accidentally.\n \tDropped commits (newer to older):\n \t - $(git rev-list --pretty=oneline --abbrev-commit -1 master)\n \tTo avoid this message, use \"drop\" to explicitly remove a commit.\n-\n-\tUse '\\''git config rebase.missingCommitsCheck'\\'' to change the level of warnings.\n-\tThe possible behaviours are: ignore, warn, error.\n-\n-\tRebasing (1/4)\n-\tRebasing (2/4)\n-\tRebasing (3/4)\n-\tRebasing (4/4)\n-\tSuccessfully rebased and updated refs/heads/missing-commit.\n \tEOF\n \ttest_config rebase.missingCommitsCheck warn &&\n \trebase_setup_and_clean missing-commit &&\n \tset_fake_editor &&\n \tFAKE_LINES=\"1 2 3 4\" \\\n \t\tgit rebase -i --root 2>actual.2 &&\n-\tcr_to_nl actual &&\n+\thead -n4 actual.2 >actual &&\n \ttest_i18ncmp expect actual &&\n \ttest D = $(git cat-file commit HEAD | sed -ne \\$p)\n '\n-- \n2.22.0.589.g5bd7971b91\n\n"},{"id":"377912","messageId":"20190624181318.17388-2-szeder.dev@gmail.com","threadId":"51015","inReplyTo":"20190624181318.17388-1-szeder.dev@gmail.com","subject":"[PATCH v3 1/5] t3404: modernize here doc style","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-06-24T18:13:14Z","receivedAt":"2019-06-24T18:13:31Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"In 't3404-rebase-interactive.sh' the expected output of several tests\nis prepared from here documents, which are outside of\n'test_expect_success' blocks and have spaces around redirection\noperators.\n\nMove these here documents into the corresponding 'test_expect_success'\nblock and avoid spaces between filename and redition operators.\nFurthermore, quote the here docs' delimiter word to prevent parameter\nexpansions and what not, where applicable.\n\nSigned-off-by: SZEDER Gábor \n---\n t/t3404-rebase-interactive.sh | 123 ++++++++++++++++------------------\n 1 file changed, 58 insertions(+), 65 deletions(-)\n\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 1723e1a858..9146f9d47b 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -75,11 +75,10 @@ test_expect_success 'rebase --keep-empty' '\n \ttest_line_count = 6 actual\n '\n \n-cat > expect <expect <<-\\EOF &&\n+\terror: nothing to do\n+\tEOF\n \tset_fake_editor &&\n \ttest_must_fail env FAKE_LINES=\"1 exec_true\" git rebase -i HEAD^ >actual 2>&1 &&\n \ttest_i18ncmp expect actual\n@@ -237,25 +236,23 @@ test_expect_success 'exchange two commits' '\n \ttest G = $(git cat-file commit HEAD | sed -ne \\$p)\n '\n \n-cat > expect << EOF\n-diff --git a/file1 b/file1\n-index f70f10e..fd79235 100644\n---- a/file1\n-+++ b/file1\n-@@ -1 +1 @@\n--A\n-+G\n-EOF\n-\n-cat > expect2 << EOF\n-<<<<<<< HEAD\n-D\n-=======\n-G\n->>>>>>> 5d18e54... G\n-EOF\n-\n test_expect_success 'stop on conflicting pick' '\n+\tcat >expect <<-\\EOF &&\n+\tdiff --git a/file1 b/file1\n+\tindex f70f10e..fd79235 100644\n+\t--- a/file1\n+\t+++ b/file1\n+\t@@ -1 +1 @@\n+\t-A\n+\t+G\n+\tEOF\n+\tcat >expect2 <<-\\EOF &&\n+\t<<<<<<< HEAD\n+\tD\n+\t=======\n+\tG\n+\t>>>>>>> 5d18e54... G\n+\tEOF\n \tgit tag new-branch1 &&\n \tset_fake_editor &&\n \ttest_must_fail git rebase -i master &&\n@@ -495,15 +492,14 @@ test_expect_success 'commit message retained after conflict' '\n \tgit branch -D conflict-squash\n '\n \n-cat > expect-squash-fixup << EOF\n-B\n-\n-D\n+test_expect_success C_LOCALE_OUTPUT 'squash and fixup generate correct log messages' '\n+\tcat >expect-squash-fixup <<-\\EOF &&\n+\tB\n \n-ONCE\n-EOF\n+\tD\n \n-test_expect_success C_LOCALE_OUTPUT 'squash and fixup generate correct log messages' '\n+\tONCE\n+\tEOF\n \tgit checkout -b squash-fixup E &&\n \tbase=$(git rev-parse HEAD~4) &&\n \tset_fake_editor &&\n@@ -799,13 +795,12 @@ test_expect_success 'rebase -i can copy notes' '\n \ttest \"a note\" = \"$(git notes show HEAD)\"\n '\n \n-cat >expect <expect <<-\\EOF &&\n+\tan earlier note\n+\n+\ta note\n+\tEOF\n \tgit reset --hard n3 &&\n \tgit notes add -m\"an earlier note\" n2 &&\n \tset_fake_editor &&\n@@ -1304,27 +1299,26 @@ test_expect_success 'rebase -i respects rebase.missingCommitsCheck = ignore' '\n \t\tactual\n '\n \n-cat >expect <expect <<-EOF &&\n+\tWarning: some commits may have been dropped accidentally.\n+\tDropped commits (newer to older):\n+\t - $(git rev-list --pretty=oneline --abbrev-commit -1 master)\n+\tTo avoid this message, use \"drop\" to explicitly remove a commit.\n+\n+\tUse '\\''git config rebase.missingCommitsCheck'\\'' to change the level of warnings.\n+\tThe possible behaviours are: ignore, warn, error.\n+\n+\tRebasing (1/4)\n+\tRebasing (2/4)\n+\tRebasing (3/4)\n+\tRebasing (4/4)\n+\tSuccessfully rebased and updated refs/heads/missing-commit.\n+\tEOF\n \ttest_config rebase.missingCommitsCheck warn &&\n \trebase_setup_and_clean missing-commit &&\n \tset_fake_editor &&\n@@ -1335,21 +1329,20 @@ test_expect_success 'rebase -i respects rebase.missingCommitsCheck = warn' '\n \ttest D = $(git cat-file commit HEAD | sed -ne \\$p)\n '\n \n-cat >expect <expect <<-EOF &&\n+\tWarning: some commits may have been dropped accidentally.\n+\tDropped commits (newer to older):\n+\t - $(git rev-list --pretty=oneline --abbrev-commit -1 master)\n+\t - $(git rev-list --pretty=oneline --abbrev-commit -1 master~2)\n+\tTo avoid this message, use \"drop\" to explicitly remove a commit.\n+\n+\tUse '\\''git config rebase.missingCommitsCheck'\\'' to change the level of warnings.\n+\tThe possible behaviours are: ignore, warn, error.\n+\n+\tYou can fix this with '\\''git rebase --edit-todo'\\'' and then run '\\''git rebase --continue'\\''.\n+\tOr you can abort the rebase with '\\''git rebase --abort'\\''.\n+\tEOF\n \ttest_config rebase.missingCommitsCheck error &&\n \trebase_setup_and_clean missing-commit &&\n \tset_fake_editor &&\n-- \n2.22.0.589.g5bd7971b91\n\n"},{"id":"377913","messageId":"20190624181318.17388-4-szeder.dev@gmail.com","threadId":"51015","inReplyTo":"20190624181318.17388-1-szeder.dev@gmail.com","subject":"[PATCH v3 3/5] pager: add a helper function to clear the last line in the terminal","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-06-24T18:13:16Z","receivedAt":"2019-06-24T18:13:32Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"There are a couple of places where we want to clear the last line on\nthe terminal, e.g. when a progress bar line is overwritten by a\nshorter line, then the end of that progress line would remain visible,\nunless we cover it up.\n\nIn 'progress.c' we did this by always appending a fixed number of\nspace characters to the next line (even if it was not shorter than the\nprevious), but as it turned out that fixed number was not quite large\nenough, see the fix in 9f1fd84e15 (progress: clear previous progress\nupdate dynamically, 2019-04-12). From then on we've been keeping\ntrack of the length of the last displayed progress line and appending\nthe appropriate number of space characters to the next line, if\nnecessary, but, alas, this approach turned out to be error prone, see\nthe fix in 1aed1a5f25 (progress: avoid empty line when breaking the\nprogress line, 2019-05-19). The next patch in this series is about to\nfix a case where we don't clear the last line, and on occasion do end\nup with such garbage at the end of the line. It would be great if we\ncould do that without the need to deal with that without meticulously\ncomputing the necessary number of space characters.\n\nSo add a helper function to clear the last line on the terminal using\nan ANSI escape sequence, which has the advantage to clear the whole\nline no matter how wide it is, even after the terminal width changed.\nSuch an escape sequence is not available on dumb terminals, though, so\nin that case fall back to simply print a whole terminal width (as\nreported by term_columns()) worth of space characters.\n\nIn 'editor.c' launch_specified_editor() already used this ANSI escape\nsequence, so replace it with a call to this function.\n\nSigned-off-by: SZEDER Gábor \n---\n cache.h | 1 +\n editor.c | 6 +++---\n pager.c | 20 ++++++++++++++++++++\n 3 files changed, 24 insertions(+), 3 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex b4bb2e2c11..5b2cd32bad 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1759,6 +1759,7 @@ void setup_pager(void);\n int pager_in_use(void);\n extern int pager_use_color;\n int term_columns(void);\n+void term_clear_line(void);\n int decimal_width(uintmax_t);\n int check_pager_config(const char *cmd);\n void prepare_pager_args(struct child_process *, const char *pager);\ndiff --git a/editor.c b/editor.c\nindex 71547674ab..f079abbf11 100644\n--- a/editor.c\n+++ b/editor.c\n@@ -96,10 +96,10 @@ static int launch_specified_editor(const char *editor, const char *path,\n \n \t\tif (print_waiting_for_editor && !is_terminal_dumb())\n \t\t\t/*\n-\t\t\t * Go back to the beginning and erase the entire line to\n-\t\t\t * avoid wasting the vertical space.\n+\t\t\t * Erase the entire line to avoid wasting the\n+\t\t\t * vertical space.\n \t\t\t */\n-\t\t\tfputs(\"\\r\\033[K\", stderr);\n+\t\t\tterm_clear_line();\n \t}\n \n \tif (!buffer)\ndiff --git a/pager.c b/pager.c\nindex 4168460ae9..41446d4f05 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -177,6 +177,26 @@ int term_columns(void)\n \treturn term_columns_at_startup;\n }\n \n+/*\n+ * Clear the entire line, leave cursor in first column.\n+ */\n+void term_clear_line(void)\n+{\n+\tif (is_terminal_dumb())\n+\t\t/*\n+\t\t * Fall back to print a terminal width worth of space\n+\t\t * characters (hoping that the terminal is still as wide\n+\t\t * as it was upon the first call to term_columns()).\n+\t\t */\n+\t\tfprintf(stderr, \"\\r%*s\\r\", term_columns(), \"\");\n+\telse\n+\t\t/*\n+\t\t * On non-dumb terminals use an escape sequence to clear\n+\t\t * the whole line, no matter how wide the terminal.\n+\t\t */\n+\t\tfputs(\"\\r\\033[K\", stderr);\n+}\n+\n /*\n * How many columns do we need to show this number in decimal?\n */\n-- \n2.22.0.589.g5bd7971b91\n\n"},{"id":"377914","messageId":"20190624181318.17388-5-szeder.dev@gmail.com","threadId":"51015","inReplyTo":"20190624181318.17388-1-szeder.dev@gmail.com","subject":"[PATCH v3 4/5] rebase: fix garbled progress display with '-x'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-06-24T18:13:17Z","receivedAt":"2019-06-24T18:13:34Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"When running a command with the 'exec' instruction during an\ninteractive rebase session, or for a range of commits using 'git\nrebase -x', the output can be a bit garbled when the name of the\ncommand is short enough:\n\n $ git rebase -x true HEAD~5\n Executing: true\n Executing: true\n Executing: true\n Executing: true\n Executing: true)\n Successfully rebased and updated refs/heads/master.\n\nNote the ')' at the end of the last line. It gets more garbled as the\nrange of commits increases:\n\n $ git rebase -x true HEAD~50\n Executing: true)\n [ repeated 3 more times ]\n Executing: true0)\n [ repeated 44 more times ]\n Executing: true00)\n Successfully rebased and updated refs/heads/master.\n\nThose extra numbers and ')' are remnants of the previously displayed\n\"Rebasing (N/M)\" progress lines that are usually completely\noverwritten by the \"Executing: \" lines, unless 'cmd' is short and\nthe \"N/M\" part is long.\n\nMake sure that the previously displayed \"Rebasing (N/M)\" line is\ncleared by using the term_clear_line() helper function added in the\nprevious patch.\n\nA couple of other rebase commands print similar messages, e.g.\n\"Stopped at ... \" for the 'edit' or 'break'\ncommands, or the \"Successfully rebased and updated .\" at the\nvery end. These are so long that they practically always overwrite\nthat \"Rebasing (N/M)\" progress line, but let's be prudent, and clear\nthe last line before printing these, too.\n\nIn 't3420-rebase-autostash.sh' two helper functions prepare the\nexpected output of four tests that check the full output of 'git\nrebase' and thus are affected by this change, so adjust their\nexpectations to account for the new line clearing.\n\nNote that this patch doesn't completely eliminate the possibility of\nsimilar garbled outputs, e.g. some error messages from rebase or the\n\"Auto-merging \" message from withing the depths of the merge\nmachinery might not be long enough to completely cover the last\n\"Rebasing (N/M)\" line. This patch doesn't do anything about them,\nbecause dealing with them individually would result in way too much\nchurn, while having a catch-all term_clear_line() call in the common\ncode path of pick_commits() would hide the \"Rebasing (N/M)\" line way\ntoo soon, and it would either flicker or be invisible.\n\nSigned-off-by: SZEDER Gábor \n---\n sequencer.c | 17 ++++++++++++++---\n t/t3420-rebase-autostash.sh | 4 ++--\n 2 files changed, 16 insertions(+), 5 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex f88a97fb10..63b09cfceb 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3731,8 +3731,11 @@ static int pick_commits(struct repository *r,\n \t\t\tunlink(git_path_merge_head(the_repository));\n \t\t\tdelete_ref(NULL, \"REBASE_HEAD\", NULL, REF_NO_DEREF);\n \n-\t\t\tif (item->command == TODO_BREAK)\n+\t\t\tif (item->command == TODO_BREAK) {\n+\t\t\t\tif (!opts->verbose)\n+\t\t\t\t\tterm_clear_line();\n \t\t\t\treturn stopped_at_head(r);\n+\t\t\t}\n \t\t}\n \t\tif (item->command <= TODO_SQUASH) {\n \t\t\tif (is_rebase_i(opts))\n@@ -3754,11 +3757,14 @@ static int pick_commits(struct repository *r,\n \t\t\t}\n \t\t\tif (item->command == TODO_EDIT) {\n \t\t\t\tstruct commit *commit = item->commit;\n-\t\t\t\tif (!res)\n+\t\t\t\tif (!res) {\n+\t\t\t\t\tif (!opts->verbose)\n+\t\t\t\t\t\tterm_clear_line();\n \t\t\t\t\tfprintf(stderr,\n \t\t\t\t\t\t_(\"Stopped at %s... %.*s\\n\"),\n \t\t\t\t\t\tshort_commit_name(commit),\n \t\t\t\t\t\titem->arg_len, arg);\n+\t\t\t\t}\n \t\t\t\treturn error_with_patch(r, commit,\n \t\t\t\t\targ, item->arg_len, opts, res, !res);\n \t\t\t}\n@@ -3796,6 +3802,8 @@ static int pick_commits(struct repository *r,\n \t\t\tint saved = *end_of_arg;\n \t\t\tstruct stat st;\n \n+\t\t\tif (!opts->verbose)\n+\t\t\t\tterm_clear_line();\n \t\t\t*end_of_arg = '\\0';\n \t\t\tres = do_exec(r, arg);\n \t\t\t*end_of_arg = saved;\n@@ -3954,10 +3962,13 @@ static int pick_commits(struct repository *r,\n \t\t}\n \t\tapply_autostash(opts);\n \n-\t\tif (!opts->quiet)\n+\t\tif (!opts->quiet) {\n+\t\t\tif (!opts->verbose)\n+\t\t\t\tterm_clear_line();\n \t\t\tfprintf(stderr,\n \t\t\t\t\"Successfully rebased and updated %s.\\n\",\n \t\t\t\thead_ref.buf);\n+\t\t}\n \n \t\tstrbuf_release(&buf);\n \t\tstrbuf_release(&head_ref);\ndiff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh\nindex 2d1094e483..9186e90127 100755\n--- a/t/t3420-rebase-autostash.sh\n+++ b/t/t3420-rebase-autostash.sh\n@@ -49,7 +49,7 @@ create_expected_success_interactive () {\n \t$(grep \"^Created autostash: [0-9a-f][0-9a-f]*\\$\" actual)\n \tHEAD is now at $(git rev-parse --short feature-branch) third commit\n \tRebasing (1/2)QRebasing (2/2)QApplied autostash.\n-\tSuccessfully rebased and updated refs/heads/rebased-feature-branch.\n+\tQ QSuccessfully rebased and updated refs/heads/rebased-feature-branch.\n \tEOF\n }\n \n@@ -73,7 +73,7 @@ create_expected_failure_interactive () {\n \tRebasing (1/2)QRebasing (2/2)QApplying autostash resulted in conflicts.\n \tYour changes are safe in the stash.\n \tYou can run \"git stash pop\" or \"git stash drop\" at any time.\n-\tSuccessfully rebased and updated refs/heads/rebased-feature-branch.\n+\tQ QSuccessfully rebased and updated refs/heads/rebased-feature-branch.\n \tEOF\n }\n \n-- \n2.22.0.589.g5bd7971b91\n\n"},{"id":"377915","messageId":"20190624181318.17388-6-szeder.dev@gmail.com","threadId":"51015","inReplyTo":"20190624181318.17388-1-szeder.dev@gmail.com","subject":"[PATCH v3 5/5] progress: use term_clear_line()","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-06-24T18:13:18Z","receivedAt":"2019-06-24T18:13:36Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"To make sure that the previously displayed progress line is completely\ncovered up when the new line is shorter, commit 545dc345eb (progress:\nbreak too long progress bar lines, 2019-04-12) added a bunch of\ncalculations to figure out how many characters it needs to overwrite\nwith spaces.\n\nUse the just introduced term_clear_line() helper function to, well,\nclear the last line, making all these calculations unnecessary, and\nthus simplifying the code considerably.\n\nThree tests in 't5541-http-push-smart.sh' 'grep' for specific text\nshown in the progress lines at the beginning of the line, but now\nthose lines begin either with the ANSI escape sequence or with the\nterminal width worth of space characters clearing the line. Relax the\n'grep' patterns to match anywhere on the line. Note that only two of\nthese three tests fail without relaxing their 'grep' pattern, but the\nthird looks for the absence of the pattern, so it still succeeds, but\nwithout the adjustment would potentially hide future regressions.\n\nNote also that with this change we no longer need the length of the\npreviously displayed progress line, so the strbuf added to 'struct\nprogress' in d53ba841d4 (progress: assemble percentage and counters in\na strbuf before printing, 2019-04-05) is not strictly necessary\nanymore. We still keep it, though, as it avoids allocating and\nreleasing a strbuf each time the progress is updated.\n\nSigned-off-by: SZEDER Gábor \n---\n progress.c | 28 +++++++++++-----------------\n t/t5541-http-push-smart.sh | 6 +++---\n 2 files changed, 14 insertions(+), 20 deletions(-)\n\ndiff --git a/progress.c b/progress.c\nindex a2e8cf64a8..095dcd0ddf 100644\n--- a/progress.c\n+++ b/progress.c\n@@ -88,7 +88,6 @@ static void display(struct progress *progress, uint64_t n, const char *done)\n \tconst char *tp;\n \tstruct strbuf *counters_sb = &progress->counters_sb;\n \tint show_update = 0;\n-\tint last_count_len = counters_sb->len;\n \n \tif (progress->delay && (!progress_update || --progress->delay))\n \t\treturn;\n@@ -116,26 +115,21 @@ static void display(struct progress *progress, uint64_t n, const char *done)\n \tif (show_update) {\n \t\tif (is_foreground_fd(fileno(stderr)) || done) {\n \t\t\tconst char *eol = done ? done : \"\\r\";\n-\t\t\tsize_t clear_len = counters_sb->len < last_count_len ?\n-\t\t\t\t\tlast_count_len - counters_sb->len + 1 :\n-\t\t\t\t\t0;\n-\t\t\tsize_t progress_line_len = progress->title_len +\n-\t\t\t\t\t\tcounters_sb->len + 2;\n-\t\t\tint cols = term_columns();\n \n+\t\t\tterm_clear_line();\n \t\t\tif (progress->split) {\n-\t\t\t\tfprintf(stderr, \" %s%*s\", counters_sb->buf,\n-\t\t\t\t\t(int) clear_len, eol);\n-\t\t\t} else if (!done && cols < progress_line_len) {\n-\t\t\t\tclear_len = progress->title_len + 1 < cols ?\n-\t\t\t\t\t cols - progress->title_len - 1 : 0;\n-\t\t\t\tfprintf(stderr, \"%s:%*s\\n %s%s\",\n-\t\t\t\t\tprogress->title, (int) clear_len, \"\",\n-\t\t\t\t\tcounters_sb->buf, eol);\n+\t\t\t\tfprintf(stderr, \" %s%s\", counters_sb->buf,\n+\t\t\t\t\teol);\n+\t\t\t} else if (!done &&\n+\t\t\t\t /* The \"+ 2\" accounts for the \": \". */\n+\t\t\t\t term_columns() < progress->title_len +\n+\t\t\t\t\t\t counters_sb->len + 2) {\n+\t\t\t\tfprintf(stderr, \"%s:\\n %s%s\",\n+\t\t\t\t\tprogress->title, counters_sb->buf, eol);\n \t\t\t\tprogress->split = 1;\n \t\t\t} else {\n-\t\t\t\tfprintf(stderr, \"%s: %s%*s\", progress->title,\n-\t\t\t\t\tcounters_sb->buf, (int) clear_len, eol);\n+\t\t\t\tfprintf(stderr, \"%s: %s%s\", progress->title,\n+\t\t\t\t\tcounters_sb->buf, eol);\n \t\t\t}\n \t\t\tfflush(stderr);\n \t\t}\ndiff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh\nindex 8ef8763e06..2e4802e206 100755\n--- a/t/t5541-http-push-smart.sh\n+++ b/t/t5541-http-push-smart.sh\n@@ -213,7 +213,7 @@ test_expect_success TTY 'push shows progress when stderr is a tty' '\n \tcd \"$ROOT_PATH\"/test_repo_clone &&\n \ttest_commit noisy &&\n \ttest_terminal git push >output 2>&1 &&\n-\ttest_i18ngrep \"^Writing objects\" output\n+\ttest_i18ngrep \"Writing objects\" output\n '\n \n test_expect_success TTY 'push --quiet silences status and progress' '\n@@ -228,7 +228,7 @@ test_expect_success TTY 'push --no-progress silences progress but not status' '\n \ttest_commit no-progress &&\n \ttest_terminal git push --no-progress >output 2>&1 &&\n \ttest_i18ngrep \"^To http\" output &&\n-\ttest_i18ngrep ! \"^Writing objects\" output\n+\ttest_i18ngrep ! \"Writing objects\" output\n '\n \n test_expect_success 'push --progress shows progress to non-tty' '\n@@ -236,7 +236,7 @@ test_expect_success 'push --progress shows progress to non-tty' '\n \ttest_commit progress &&\n \tgit push --progress >output 2>&1 &&\n \ttest_i18ngrep \"^To http\" output &&\n-\ttest_i18ngrep \"^Writing objects\" output\n+\ttest_i18ngrep \"Writing objects\" output\n '\n \n test_expect_success 'http push gives sane defaults to reflog' '\n-- \n2.22.0.589.g5bd7971b91\n\n"},{"id":"377918","messageId":"20190624183927.GA10853@szeder.dev","threadId":"51015","inReplyTo":"nycvar.QRO.7.76.6.1906122056570.789@QRFXGBC-DHN364S.ybpnyqbznva","subject":"Re: [PATCH v2 3/4] rebase: fix garbled progress display with '-x'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-06-24T18:39:27Z","receivedAt":"2019-06-24T18:39:37Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Wed, Jun 12, 2019 at 09:14:40PM +0200, Johannes Schindelin wrote:\n> Hi,\n> \n> On Tue, 11 Jun 2019, SZEDER Gábor wrote:\n> \n> > On Tue, Jun 11, 2019 at 01:36:16PM -0700, Junio C Hamano wrote:\n> > > SZEDER Gábor writes:\n> > >\n> > > > -Rebasing (1/4)QRebasing (2/4)QRebasing (3/4)QRebasing (4/4)QSuccessfully rebased and updated refs/heads/missing-commit.\n> > > > +Rebasing (1/4)QRebasing (2/4)QRebasing (3/4)QRebasing (4/4)QQ QSuccessfully rebased and updated refs/heads/missing-commit.\n> > > > EOF\n> > >\n> > > Yuck,\n> >\n> > Oh yeah...\n> >\n> > >... but I do not see how else/better this test can be written\n> > > myself, which makes it a double-yuck X-<\n> >\n> > Perhaps hiding those spaces behind a helper variable e.g.\n> > 'dump_term_clear_line=Q<80-spaces>Q' and embedding that in the here\n> > docs specifying the expected output in these three tests could make it\n> > ever so slightly less yuck...\n> >\n> > > Are we forcing out test to operate under dumb terminal mode and with\n> > > a known number of columns?\n> >\n> > 'test-lib.sh' sets TERM=dumb relatively early on, and in these tests\n> > we don't use 'test_terminal' to run 'git rebase', so... yeah. And\n> > term_columns() defaults to 80.\n> >\n> > However, if the terminal were smart, then we would have to deal with\n> > ANSI escape suddenly popping up...\n> \n> And I fear that is *exactly* what makes\n> https://dev.azure.com/gitgitgadget/git/_build/results?buildId=10539&view=ms.vss-test-web.build-test-results-tab\n> fail...\n> \n> You cannot easily see it in that output (probably because of incorrectly\n> encoded Escape sequences in the `.xml` output), so I'll paste what I see\n> here, locally, when running `t3404-*.sh -i -V -x`:\n> \n> -- snip --\n> [...]\n> ok 99 - rebase -i respects rebase.missingCommitsCheck = ignore\n> \n> expecting success:\n> test_config rebase.missingCommitsCheck warn &&\n> rebase_setup_and_clean missing-commit &&\n> set_fake_editor &&\n> FAKE_LINES=\"1 2 3 4\" \\\n> git rebase -i --root 2>actual &&\n> test_i18ncmp expect actual &&\n> test D = $(git cat-file commit HEAD | sed -ne \\$p)\n\n[...]\n\n> ++ test_cmp expect actual\n> ++ GIT_ALLOC_LIMIT=0\n> ++ test-tool cmp expect actual\n> diff --git a/expect b/actual\n> index 05fcfcb..9555e34 100644\n> --- a/expect\n> +++ b/actual\n> @@ -6,4 +6,4 @@ To avoid this message, use \"drop\" to explicitly remove a commit.\n> Use 'git config rebase.missingCommitsCheck' to change the level of warnings.\n> The possible behaviours are: ignore, warn, error.\n> \n> -Rebasing (1/4)^MRebasing (2/4)^MRebasing (3/4)^MRebasing (4/4)^M ^MSuccessfully rebased and updated refs/heads/missing-commit.\n> +Rebasing (1/4)^MRebasing (2/4)^MRebasing (3/4)^MRebasing (4/4)^MESC[KSuccessfully rebased and updated refs/heads/missing-commit.\n> error: last command exited with $?=1\n> not ok 100 - rebase -i respects rebase.missingCommitsCheck = warn\n> #\n> # test_config rebase.missingCommitsCheck warn &&\n> # rebase_setup_and_clean missing-commit &&\n> # set_fake_editor &&\n> # FAKE_LINES=\"1 2 3 4\" \\\n> # git rebase -i --root 2>actual &&\n> # test_i18ncmp expect actual &&\n> # test D = $(git cat-file commit HEAD | sed -ne \\$p)\n> #\n> -- snap --\n> \n> (I copy-pasted this from the output of `less` so that the control sequences can be seen.)\n> \n> To be utterly honest, I really fail to see a reason why a test case that\n> purports to verify that `git rebase -i` respects\n> `rebase.missingCommitsCheck=warn` should fail when the progress is shown in an\n> unexpected format.\n> \n> It strikes me as yet another poorly written test case that fails to catch the\n> intended regressions, instead it catches a bug *in the test case itself* when\n> legitimate changes are made to the progress code.\n> \n> Nothing in a test suite is worse than a test that fails (or succeeds) for the\n> wrong reasons.\n> \n> To make things even worse, the code that generates that `expect` file is\n> outside the test case.\n> \n> Here it is, in its full \"glory\":\n> \n> -- snip --\n> q_to_cr >expect < Warning: some commits may have been dropped accidentally.\n> Dropped commits (newer to older):\n> - $(git rev-list --pretty=oneline --abbrev-commit -1 master)\n> To avoid this message, use \"drop\" to explicitly remove a commit.\n> \n> Use 'git config rebase.missingCommitsCheck' to change the level of warnings.\n> The possible behaviours are: ignore, warn, error.\n> \n> Rebasing (1/4)QRebasing (2/4)QRebasing (3/4)QRebasing (4/4)QQ QSuccessfully rebased and updated refs/heads/missing-commit.\n> EOF\n> -- snap --\n> \n> May I please *strongly* suggest to fix this first? It should\n> \n> - completely lose that last line,\n> - be inserted into the test case itself so that e.g. disk full problems are\n> caught and logged properly, and\n> - the `test_i18ncmp expect actual` call in the test case should be replaced\n> by something like:\n> \n> \tsed \"\\$d\" actual-skip-progress &&\n> \ttest_i18ncmp expect actual-skip-progress\n> \n> This should obviously be made as a separate, introductory patch (probably with\n> a less scathing commit message than my comments above would suggest).\n> \n> And that would also remove the double-yuck.\n\nUnfortunately, this addresses only one of the \"Yuck\"s; see v3 of this\npatch series [1].\n\nThe other yucks affect the following four tests in\n't3420-rebase-autostash.sh':\n\n 16 - rebase --merge --autostash: check output\n 23 - rebase --merge: check output with conflicting stash\n 26 - rebase --interactive --autostash: check output\n 33 - rebase --interactive: check output with conflicting stash\n\nThese tests come from commits b76aeae553 (rebase: add regression tests\nfor console output, 2017-06-19) and 7d70e6b902 (rebase: add more\nregression tests for console output, 2017-06-19), and are specifically\nabout checking the (whole) console output of 'git rebase', so I left\nthe updates to them as they were.\n\nIn any case, Cc-ing Phillip to discuss whether something could be done\nabout them (now perhaps preferably (for me :) as a follow-up, and not\nanother preparatory patches).\n\n\n[1] https://public-inbox.org/git/20190624181318.17388-3-szeder.dev@gmail.com/T/#u\n\n"},{"id":"377945","messageId":"nycvar.QRO.7.76.6.1906251104300.44@tvgsbejvaqbjf.bet","threadId":"51015","inReplyTo":"20190624181318.17388-2-szeder.dev@gmail.com","subject":"Re: [PATCH v3 1/5] t3404: modernize here doc style","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-06-25T09:06:01Z","receivedAt":"2019-06-25T09:05:50Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Gábor,\n\nOn Mon, 24 Jun 2019, SZEDER Gábor wrote:\n\n> In 't3404-rebase-interactive.sh' the expected output of several tests\n> is prepared from here documents, which are outside of\n> 'test_expect_success' blocks and have spaces around redirection\n> operators.\n>\n> Move these here documents into the corresponding 'test_expect_success'\n> block and avoid spaces between filename and redition operators.\n> Furthermore, quote the here docs' delimiter word to prevent parameter\n> expansions and what not, where applicable.\n\nMakes sense. I did not really look at it in detail (it's a bit tough to\nread, and `--patience` would have made it only _slightly_ easier),\ntrusting that you used `git diff -w --color-moved` to verify the moves.\n\nThanks,\nDscho\n\n>\n> Signed-off-by: SZEDER Gábor \n> ---\n> t/t3404-rebase-interactive.sh | 123 ++++++++++++++++------------------\n> 1 file changed, 58 insertions(+), 65 deletions(-)\n>\n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index 1723e1a858..9146f9d47b 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -75,11 +75,10 @@ test_expect_success 'rebase --keep-empty' '\n> \ttest_line_count = 6 actual\n> '\n>\n> -cat > expect < -error: nothing to do\n> -EOF\n> -\n> test_expect_success 'rebase -i with empty HEAD' '\n> +\tcat >expect <<-\\EOF &&\n> +\terror: nothing to do\n> +\tEOF\n> \tset_fake_editor &&\n> \ttest_must_fail env FAKE_LINES=\"1 exec_true\" git rebase -i HEAD^ >actual 2>&1 &&\n> \ttest_i18ncmp expect actual\n> @@ -237,25 +236,23 @@ test_expect_success 'exchange two commits' '\n> \ttest G = $(git cat-file commit HEAD | sed -ne \\$p)\n> '\n>\n> -cat > expect << EOF\n> -diff --git a/file1 b/file1\n> -index f70f10e..fd79235 100644\n> ---- a/file1\n> -+++ b/file1\n> -@@ -1 +1 @@\n> --A\n> -+G\n> -EOF\n> -\n> -cat > expect2 << EOF\n> -<<<<<<< HEAD\n> -D\n> -=======\n> -G\n> ->>>>>>> 5d18e54... G\n> -EOF\n> -\n> test_expect_success 'stop on conflicting pick' '\n> +\tcat >expect <<-\\EOF &&\n> +\tdiff --git a/file1 b/file1\n> +\tindex f70f10e..fd79235 100644\n> +\t--- a/file1\n> +\t+++ b/file1\n> +\t@@ -1 +1 @@\n> +\t-A\n> +\t+G\n> +\tEOF\n> +\tcat >expect2 <<-\\EOF &&\n> +\t<<<<<<< HEAD\n> +\tD\n> +\t=======\n> +\tG\n> +\t>>>>>>> 5d18e54... G\n> +\tEOF\n> \tgit tag new-branch1 &&\n> \tset_fake_editor &&\n> \ttest_must_fail git rebase -i master &&\n> @@ -495,15 +492,14 @@ test_expect_success 'commit message retained after conflict' '\n> \tgit branch -D conflict-squash\n> '\n>\n> -cat > expect-squash-fixup << EOF\n> -B\n> -\n> -D\n> +test_expect_success C_LOCALE_OUTPUT 'squash and fixup generate correct log messages' '\n> +\tcat >expect-squash-fixup <<-\\EOF &&\n> +\tB\n>\n> -ONCE\n> -EOF\n> +\tD\n>\n> -test_expect_success C_LOCALE_OUTPUT 'squash and fixup generate correct log messages' '\n> +\tONCE\n> +\tEOF\n> \tgit checkout -b squash-fixup E &&\n> \tbase=$(git rev-parse HEAD~4) &&\n> \tset_fake_editor &&\n> @@ -799,13 +795,12 @@ test_expect_success 'rebase -i can copy notes' '\n> \ttest \"a note\" = \"$(git notes show HEAD)\"\n> '\n>\n> -cat >expect < -an earlier note\n> -\n> -a note\n> -EOF\n> -\n> test_expect_success 'rebase -i can copy notes over a fixup' '\n> +\tcat >expect <<-\\EOF &&\n> +\tan earlier note\n> +\n> +\ta note\n> +\tEOF\n> \tgit reset --hard n3 &&\n> \tgit notes add -m\"an earlier note\" n2 &&\n> \tset_fake_editor &&\n> @@ -1304,27 +1299,26 @@ test_expect_success 'rebase -i respects rebase.missingCommitsCheck = ignore' '\n> \t\tactual\n> '\n>\n> -cat >expect < -Warning: some commits may have been dropped accidentally.\n> -Dropped commits (newer to older):\n> - - $(git rev-list --pretty=oneline --abbrev-commit -1 master)\n> -To avoid this message, use \"drop\" to explicitly remove a commit.\n> -\n> -Use 'git config rebase.missingCommitsCheck' to change the level of warnings.\n> -The possible behaviours are: ignore, warn, error.\n> -\n> -Rebasing (1/4)\n> -Rebasing (2/4)\n> -Rebasing (3/4)\n> -Rebasing (4/4)\n> -Successfully rebased and updated refs/heads/missing-commit.\n> -EOF\n> -\n> cr_to_nl () {\n> \ttr '\\015' '\\012'\n> }\n>\n> test_expect_success 'rebase -i respects rebase.missingCommitsCheck = warn' '\n> +\tcat >expect <<-EOF &&\n> +\tWarning: some commits may have been dropped accidentally.\n> +\tDropped commits (newer to older):\n> +\t - $(git rev-list --pretty=oneline --abbrev-commit -1 master)\n> +\tTo avoid this message, use \"drop\" to explicitly remove a commit.\n> +\n> +\tUse '\\''git config rebase.missingCommitsCheck'\\'' to change the level of warnings.\n> +\tThe possible behaviours are: ignore, warn, error.\n> +\n> +\tRebasing (1/4)\n> +\tRebasing (2/4)\n> +\tRebasing (3/4)\n> +\tRebasing (4/4)\n> +\tSuccessfully rebased and updated refs/heads/missing-commit.\n> +\tEOF\n> \ttest_config rebase.missingCommitsCheck warn &&\n> \trebase_setup_and_clean missing-commit &&\n> \tset_fake_editor &&\n> @@ -1335,21 +1329,20 @@ test_expect_success 'rebase -i respects rebase.missingCommitsCheck = warn' '\n> \ttest D = $(git cat-file commit HEAD | sed -ne \\$p)\n> '\n>\n> -cat >expect < -Warning: some commits may have been dropped accidentally.\n> -Dropped commits (newer to older):\n> - - $(git rev-list --pretty=oneline --abbrev-commit -1 master)\n> - - $(git rev-list --pretty=oneline --abbrev-commit -1 master~2)\n> -To avoid this message, use \"drop\" to explicitly remove a commit.\n> -\n> -Use 'git config rebase.missingCommitsCheck' to change the level of warnings.\n> -The possible behaviours are: ignore, warn, error.\n> -\n> -You can fix this with 'git rebase --edit-todo' and then run 'git rebase --continue'.\n> -Or you can abort the rebase with 'git rebase --abort'.\n> -EOF\n> -\n> test_expect_success 'rebase -i respects rebase.missingCommitsCheck = error' '\n> +\tcat >expect <<-EOF &&\n> +\tWarning: some commits may have been dropped accidentally.\n> +\tDropped commits (newer to older):\n> +\t - $(git rev-list --pretty=oneline --abbrev-commit -1 master)\n> +\t - $(git rev-list --pretty=oneline --abbrev-commit -1 master~2)\n> +\tTo avoid this message, use \"drop\" to explicitly remove a commit.\n> +\n> +\tUse '\\''git config rebase.missingCommitsCheck'\\'' to change the level of warnings.\n> +\tThe possible behaviours are: ignore, warn, error.\n> +\n> +\tYou can fix this with '\\''git rebase --edit-todo'\\'' and then run '\\''git rebase --continue'\\''.\n> +\tOr you can abort the rebase with '\\''git rebase --abort'\\''.\n> +\tEOF\n> \ttest_config rebase.missingCommitsCheck error &&\n> \trebase_setup_and_clean missing-commit &&\n> \tset_fake_editor &&\n> --\n> 2.22.0.589.g5bd7971b91\n>\n>\n"},{"id":"377948","messageId":"nycvar.QRO.7.76.6.1906251106480.44@tvgsbejvaqbjf.bet","threadId":"51015","inReplyTo":"20190624181318.17388-3-szeder.dev@gmail.com","subject":"Re: [PATCH v3 2/5] t3404: make the 'rebase.missingCommitsCheck=ignore' test more focused","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-06-25T09:07:10Z","receivedAt":"2019-06-25T09:06:56Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Gábor,\n\nOn Mon, 24 Jun 2019, SZEDER Gábor wrote:\n\n> The test 'rebase -i respects rebase.missingCommitsCheck = warn' is\n> mainly interested in the warning about the dropped commits, but it\n> checks the whole output of 'git rebase', including progress lines and\n> what not that are not at all relevant to 'rebase.missingCommitsCheck',\n> but make it necessary to update this test whenever e.g. the way we\n> show progress is updated (as it will happen in one of the later\n> patches of this series).\n>\n> Modify the test to verify only the first four lines of 'git rebase's\n> output that contain all the important lines, notably the line\n> containing the \"Warning:\" itself and the oneline log of the dropped\n> commit.\n\nThank you. Looks very good to me,\nDscho\n\n>\n> Signed-off-by: SZEDER Gábor \n> ---\n> t/t3404-rebase-interactive.sh | 15 +--------------\n> 1 file changed, 1 insertion(+), 14 deletions(-)\n>\n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index 9146f9d47b..0b8267c97c 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -1299,32 +1299,19 @@ test_expect_success 'rebase -i respects rebase.missingCommitsCheck = ignore' '\n> \t\tactual\n> '\n>\n> -cr_to_nl () {\n> -\ttr '\\015' '\\012'\n> -}\n> -\n> test_expect_success 'rebase -i respects rebase.missingCommitsCheck = warn' '\n> \tcat >expect <<-EOF &&\n> \tWarning: some commits may have been dropped accidentally.\n> \tDropped commits (newer to older):\n> \t - $(git rev-list --pretty=oneline --abbrev-commit -1 master)\n> \tTo avoid this message, use \"drop\" to explicitly remove a commit.\n> -\n> -\tUse '\\''git config rebase.missingCommitsCheck'\\'' to change the level of warnings.\n> -\tThe possible behaviours are: ignore, warn, error.\n> -\n> -\tRebasing (1/4)\n> -\tRebasing (2/4)\n> -\tRebasing (3/4)\n> -\tRebasing (4/4)\n> -\tSuccessfully rebased and updated refs/heads/missing-commit.\n> \tEOF\n> \ttest_config rebase.missingCommitsCheck warn &&\n> \trebase_setup_and_clean missing-commit &&\n> \tset_fake_editor &&\n> \tFAKE_LINES=\"1 2 3 4\" \\\n> \t\tgit rebase -i --root 2>actual.2 &&\n> -\tcr_to_nl actual &&\n> +\thead -n4 actual.2 >actual &&\n> \ttest_i18ncmp expect actual &&\n> \ttest D = $(git cat-file commit HEAD | sed -ne \\$p)\n> '\n> --\n> 2.22.0.589.g5bd7971b91\n>\n>\n"},{"id":"377949","messageId":"nycvar.QRO.7.76.6.1906251110160.44@tvgsbejvaqbjf.bet","threadId":"51015","inReplyTo":"20190624181318.17388-6-szeder.dev@gmail.com","subject":"Re: [PATCH v3 5/5] progress: use term_clear_line()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-06-25T09:10:57Z","receivedAt":"2019-06-25T09:10:44Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Gábor,\n\nOn Mon, 24 Jun 2019, SZEDER Gábor wrote:\n\n> To make sure that the previously displayed progress line is completely\n> covered up when the new line is shorter, commit 545dc345eb (progress:\n> break too long progress bar lines, 2019-04-12) added a bunch of\n> calculations to figure out how many characters it needs to overwrite\n> with spaces.\n>\n> Use the just introduced term_clear_line() helper function to, well,\n> clear the last line, making all these calculations unnecessary, and\n> thus simplifying the code considerably.\n>\n> Three tests in 't5541-http-push-smart.sh' 'grep' for specific text\n> shown in the progress lines at the beginning of the line, but now\n> those lines begin either with the ANSI escape sequence or with the\n> terminal width worth of space characters clearing the line. Relax the\n> 'grep' patterns to match anywhere on the line. Note that only two of\n> these three tests fail without relaxing their 'grep' pattern, but the\n> third looks for the absence of the pattern, so it still succeeds, but\n> without the adjustment would potentially hide future regressions.\n>\n> Note also that with this change we no longer need the length of the\n> previously displayed progress line, so the strbuf added to 'struct\n> progress' in d53ba841d4 (progress: assemble percentage and counters in\n> a strbuf before printing, 2019-04-05) is not strictly necessary\n> anymore. We still keep it, though, as it avoids allocating and\n> releasing a strbuf each time the progress is updated.\n\nVery nice change, indeed, I totally love how much simpler the post-image\nlooks. Well done, for the entire patch series, and thank you so much!\nDscho\n\n>\n> Signed-off-by: SZEDER Gábor \n> ---\n> progress.c | 28 +++++++++++-----------------\n> t/t5541-http-push-smart.sh | 6 +++---\n> 2 files changed, 14 insertions(+), 20 deletions(-)\n>\n> diff --git a/progress.c b/progress.c\n> index a2e8cf64a8..095dcd0ddf 100644\n> --- a/progress.c\n> +++ b/progress.c\n> @@ -88,7 +88,6 @@ static void display(struct progress *progress, uint64_t n, const char *done)\n> \tconst char *tp;\n> \tstruct strbuf *counters_sb = &progress->counters_sb;\n> \tint show_update = 0;\n> -\tint last_count_len = counters_sb->len;\n>\n> \tif (progress->delay && (!progress_update || --progress->delay))\n> \t\treturn;\n> @@ -116,26 +115,21 @@ static void display(struct progress *progress, uint64_t n, const char *done)\n> \tif (show_update) {\n> \t\tif (is_foreground_fd(fileno(stderr)) || done) {\n> \t\t\tconst char *eol = done ? done : \"\\r\";\n> -\t\t\tsize_t clear_len = counters_sb->len < last_count_len ?\n> -\t\t\t\t\tlast_count_len - counters_sb->len + 1 :\n> -\t\t\t\t\t0;\n> -\t\t\tsize_t progress_line_len = progress->title_len +\n> -\t\t\t\t\t\tcounters_sb->len + 2;\n> -\t\t\tint cols = term_columns();\n>\n> +\t\t\tterm_clear_line();\n> \t\t\tif (progress->split) {\n> -\t\t\t\tfprintf(stderr, \" %s%*s\", counters_sb->buf,\n> -\t\t\t\t\t(int) clear_len, eol);\n> -\t\t\t} else if (!done && cols < progress_line_len) {\n> -\t\t\t\tclear_len = progress->title_len + 1 < cols ?\n> -\t\t\t\t\t cols - progress->title_len - 1 : 0;\n> -\t\t\t\tfprintf(stderr, \"%s:%*s\\n %s%s\",\n> -\t\t\t\t\tprogress->title, (int) clear_len, \"\",\n> -\t\t\t\t\tcounters_sb->buf, eol);\n> +\t\t\t\tfprintf(stderr, \" %s%s\", counters_sb->buf,\n> +\t\t\t\t\teol);\n> +\t\t\t} else if (!done &&\n> +\t\t\t\t /* The \"+ 2\" accounts for the \": \". */\n> +\t\t\t\t term_columns() < progress->title_len +\n> +\t\t\t\t\t\t counters_sb->len + 2) {\n> +\t\t\t\tfprintf(stderr, \"%s:\\n %s%s\",\n> +\t\t\t\t\tprogress->title, counters_sb->buf, eol);\n> \t\t\t\tprogress->split = 1;\n> \t\t\t} else {\n> -\t\t\t\tfprintf(stderr, \"%s: %s%*s\", progress->title,\n> -\t\t\t\t\tcounters_sb->buf, (int) clear_len, eol);\n> +\t\t\t\tfprintf(stderr, \"%s: %s%s\", progress->title,\n> +\t\t\t\t\tcounters_sb->buf, eol);\n> \t\t\t}\n> \t\t\tfflush(stderr);\n> \t\t}\n> diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh\n> index 8ef8763e06..2e4802e206 100755\n> --- a/t/t5541-http-push-smart.sh\n> +++ b/t/t5541-http-push-smart.sh\n> @@ -213,7 +213,7 @@ test_expect_success TTY 'push shows progress when stderr is a tty' '\n> \tcd \"$ROOT_PATH\"/test_repo_clone &&\n> \ttest_commit noisy &&\n> \ttest_terminal git push >output 2>&1 &&\n> -\ttest_i18ngrep \"^Writing objects\" output\n> +\ttest_i18ngrep \"Writing objects\" output\n> '\n>\n> test_expect_success TTY 'push --quiet silences status and progress' '\n> @@ -228,7 +228,7 @@ test_expect_success TTY 'push --no-progress silences progress but not status' '\n> \ttest_commit no-progress &&\n> \ttest_terminal git push --no-progress >output 2>&1 &&\n> \ttest_i18ngrep \"^To http\" output &&\n> -\ttest_i18ngrep ! \"^Writing objects\" output\n> +\ttest_i18ngrep ! \"Writing objects\" output\n> '\n>\n> test_expect_success 'push --progress shows progress to non-tty' '\n> @@ -236,7 +236,7 @@ test_expect_success 'push --progress shows progress to non-tty' '\n> \ttest_commit progress &&\n> \tgit push --progress >output 2>&1 &&\n> \ttest_i18ngrep \"^To http\" output &&\n> -\ttest_i18ngrep \"^Writing objects\" output\n> +\ttest_i18ngrep \"Writing objects\" output\n> '\n>\n> test_expect_success 'http push gives sane defaults to reflog' '\n> --\n> 2.22.0.589.g5bd7971b91\n>\n>\n"},{"id":"377957","messageId":"efc4a141-071a-6549-f25d-21cc6256832a@gmail.com","threadId":"51015","inReplyTo":"20190624183927.GA10853@szeder.dev","subject":"Re: [PATCH v2 3/4] rebase: fix garbled progress display with '-x'","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-06-25T10:08:04Z","receivedAt":"2019-06-25T10:08:10Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi dscho and Gábor\n\nOn 24/06/2019 19:39, SZEDER Gábor wrote:\n> On Wed, Jun 12, 2019 at 09:14:40PM +0200, Johannes Schindelin wrote:\n>> Hi,\n>>\n>> On Tue, 11 Jun 2019, SZEDER Gábor wrote:\n>>\n>>> On Tue, Jun 11, 2019 at 01:36:16PM -0700, Junio C Hamano wrote:\n>>>> SZEDER Gábor writes:\n>>>>\n>>>>> -Rebasing (1/4)QRebasing (2/4)QRebasing (3/4)QRebasing (4/4)QSuccessfully rebased and updated refs/heads/missing-commit.\n>>>>> +Rebasing (1/4)QRebasing (2/4)QRebasing (3/4)QRebasing (4/4)QQ QSuccessfully rebased and updated refs/heads/missing-commit.\n>>>>> EOF\n>>>>\n>>>> Yuck,\n>>>\n>>> Oh yeah...\n>>>\n>>>> ... but I do not see how else/better this test can be written\n>>>> myself, which makes it a double-yuck X-<\n>>>\n>>> Perhaps hiding those spaces behind a helper variable e.g.\n>>> 'dump_term_clear_line=Q<80-spaces>Q' and embedding that in the here\n>>> docs specifying the expected output in these three tests could make it\n>>> ever so slightly less yuck...\n>>>\n>>>> Are we forcing out test to operate under dumb terminal mode and with\n>>>> a known number of columns?\n>>>\n>>> 'test-lib.sh' sets TERM=dumb relatively early on, and in these tests\n>>> we don't use 'test_terminal' to run 'git rebase', so... yeah. And\n>>> term_columns() defaults to 80.\n>>>\n>>> However, if the terminal were smart, then we would have to deal with\n>>> ANSI escape suddenly popping up...\n>>\n>> And I fear that is *exactly* what makes\n>> https://dev.azure.com/gitgitgadget/git/_build/results?buildId=10539&view=ms.vss-test-web.build-test-results-tab\n>> fail...\n>>\n>> You cannot easily see it in that output (probably because of incorrectly\n>> encoded Escape sequences in the `.xml` output), so I'll paste what I see\n>> here, locally, when running `t3404-*.sh -i -V -x`:\n>>\n>> -- snip --\n>> [...]\n>> ok 99 - rebase -i respects rebase.missingCommitsCheck = ignore\n>>\n>> expecting success:\n>> test_config rebase.missingCommitsCheck warn &&\n>> rebase_setup_and_clean missing-commit &&\n>> set_fake_editor &&\n>> FAKE_LINES=\"1 2 3 4\" \\\n>> git rebase -i --root 2>actual &&\n>> test_i18ncmp expect actual &&\n>> test D = $(git cat-file commit HEAD | sed -ne \\$p)\n> \n> [...]\n> \n>> ++ test_cmp expect actual\n>> ++ GIT_ALLOC_LIMIT=0\n>> ++ test-tool cmp expect actual\n>> diff --git a/expect b/actual\n>> index 05fcfcb..9555e34 100644\n>> --- a/expect\n>> +++ b/actual\n>> @@ -6,4 +6,4 @@ To avoid this message, use \"drop\" to explicitly remove a commit.\n>> Use 'git config rebase.missingCommitsCheck' to change the level of warnings.\n>> The possible behaviours are: ignore, warn, error.\n>>\n>> -Rebasing (1/4)^MRebasing (2/4)^MRebasing (3/4)^MRebasing (4/4)^M ^MSuccessfully rebased and updated refs/heads/missing-commit.\n>> +Rebasing (1/4)^MRebasing (2/4)^MRebasing (3/4)^MRebasing (4/4)^MESC[KSuccessfully rebased and updated refs/heads/missing-commit.\n>> error: last command exited with $?=1\n>> not ok 100 - rebase -i respects rebase.missingCommitsCheck = warn\n>> #\n>> # test_config rebase.missingCommitsCheck warn &&\n>> # rebase_setup_and_clean missing-commit &&\n>> # set_fake_editor &&\n>> # FAKE_LINES=\"1 2 3 4\" \\\n>> # git rebase -i --root 2>actual &&\n>> # test_i18ncmp expect actual &&\n>> # test D = $(git cat-file commit HEAD | sed -ne \\$p)\n>> #\n>> -- snap --\n>>\n>> (I copy-pasted this from the output of `less` so that the control sequences can be seen.)\n>>\n>> To be utterly honest, I really fail to see a reason why a test case that\n>> purports to verify that `git rebase -i` respects\n>> `rebase.missingCommitsCheck=warn` should fail when the progress is shown in an\n>> unexpected format.\n>>\n>> It strikes me as yet another poorly written test case that fails to catch the\n>> intended regressions, instead it catches a bug *in the test case itself* when\n>> legitimate changes are made to the progress code.\n>>\n>> Nothing in a test suite is worse than a test that fails (or succeeds) for the\n>> wrong reasons.\n>>\n>> To make things even worse, the code that generates that `expect` file is\n>> outside the test case.\n>>\n>> Here it is, in its full \"glory\":\n>>\n>> -- snip --\n>> q_to_cr >expect <> Warning: some commits may have been dropped accidentally.\n>> Dropped commits (newer to older):\n>> - $(git rev-list --pretty=oneline --abbrev-commit -1 master)\n>> To avoid this message, use \"drop\" to explicitly remove a commit.\n>>\n>> Use 'git config rebase.missingCommitsCheck' to change the level of warnings.\n>> The possible behaviours are: ignore, warn, error.\n>>\n>> Rebasing (1/4)QRebasing (2/4)QRebasing (3/4)QRebasing (4/4)QQ QSuccessfully rebased and updated refs/heads/missing-commit.\n>> EOF\n>> -- snap --\n>>\n>> May I please *strongly* suggest to fix this first? It should\n>>\n>> - completely lose that last line,\n>> - be inserted into the test case itself so that e.g. disk full problems are\n>> caught and logged properly, and\n>> - the `test_i18ncmp expect actual` call in the test case should be replaced\n>> by something like:\n>>\n>> \tsed \"\\$d\" actual-skip-progress &&\n>> \ttest_i18ncmp expect actual-skip-progress\n>>\n>> This should obviously be made as a separate, introductory patch (probably with\n>> a less scathing commit message than my comments above would suggest).\n>>\n>> And that would also remove the double-yuck.\n> \n> Unfortunately, this addresses only one of the \"Yuck\"s; see v3 of this\n> patch series [1].\n> \n> The other yucks affect the following four tests in\n> 't3420-rebase-autostash.sh':\n> \n> 16 - rebase --merge --autostash: check output\n> 23 - rebase --merge: check output with conflicting stash\n> 26 - rebase --interactive --autostash: check output\n> 33 - rebase --interactive: check output with conflicting stash\n> \n> These tests come from commits b76aeae553 (rebase: add regression tests\n> for console output, 2017-06-19) and 7d70e6b902 (rebase: add more\n> regression tests for console output, 2017-06-19), and are specifically\n> about checking the (whole) console output of 'git rebase', so I left\n> the updates to them as they were.\n> \n> In any case, Cc-ing Phillip to discuss whether something could be done\n> about them (now perhaps preferably (for me :) as a follow-up, and not\n> another preparatory patches).\n\nThose tests were added to check that `git stash` was being silenced (see\n79a6226981 (\"rebase -i: silence stash apply\", 2017-05-18)). I can have a\nthink about a better way to do that, but is it still a problem? I just\ntried to take a look at your CI output and\nhttps://dev.azure.com/gitgitgadget/git/_build/results?buildId=11406\nseems to be all green - have I missed something or has Gábor fixed the\nissue?\n\nBest Wishes\n\nPhillip\n\n> \n> \n> [1] https://public-inbox.org/git/20190624181318.17388-3-szeder.dev@gmail.com/T/#u\n> \n\n"},{"id":"377970","messageId":"20190625113103.GB10853@szeder.dev","threadId":"51015","inReplyTo":"efc4a141-071a-6549-f25d-21cc6256832a@gmail.com","subject":"Re: [PATCH v2 3/4] rebase: fix garbled progress display with '-x'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-06-25T11:31:03Z","receivedAt":"2019-06-25T11:31:09Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Jun 25, 2019 at 11:08:04AM +0100, Phillip Wood wrote:\n> >> Rebasing (1/4)QRebasing (2/4)QRebasing (3/4)QRebasing (4/4)QQ QSuccessfully rebased and updated refs/heads/missing-commit.\n\n\n> >> May I please *strongly* suggest to fix this first? It should\n> >>\n> >> - completely lose that last line,\n> >> - be inserted into the test case itself so that e.g. disk full problems are\n> >> caught and logged properly, and\n> >> - the `test_i18ncmp expect actual` call in the test case should be replaced\n> >> by something like:\n> >>\n> >> \tsed \"\\$d\" actual-skip-progress &&\n> >> \ttest_i18ncmp expect actual-skip-progress\n> >>\n> >> This should obviously be made as a separate, introductory patch (probably with\n> >> a less scathing commit message than my comments above would suggest).\n> >>\n> >> And that would also remove the double-yuck.\n> > \n> > Unfortunately, this addresses only one of the \"Yuck\"s; see v3 of this\n> > patch series [1].\n> > \n> > The other yucks affect the following four tests in\n> > 't3420-rebase-autostash.sh':\n> > \n> > 16 - rebase --merge --autostash: check output\n> > 23 - rebase --merge: check output with conflicting stash\n> > 26 - rebase --interactive --autostash: check output\n> > 33 - rebase --interactive: check output with conflicting stash\n> > \n> > These tests come from commits b76aeae553 (rebase: add regression tests\n> > for console output, 2017-06-19) and 7d70e6b902 (rebase: add more\n> > regression tests for console output, 2017-06-19), and are specifically\n> > about checking the (whole) console output of 'git rebase', so I left\n> > the updates to them as they were.\n> > \n> > In any case, Cc-ing Phillip to discuss whether something could be done\n> > about them (now perhaps preferably (for me :) as a follow-up, and not\n> > another preparatory patches).\n> \n> Those tests were added to check that `git stash` was being silenced (see\n> 79a6226981 (\"rebase -i: silence stash apply\", 2017-05-18)).\n\nHmm, so would it then be possible/sensible to do something like this\ninstead:\n\n git rebase --options... >out &&\n test_i18ngrep ! \"\" out\n\n> I can have a\n> think about a better way to do that, but is it still a problem? I just\n> tried to take a look at your CI output and\n> https://dev.azure.com/gitgitgadget/git/_build/results?buildId=11406\n> seems to be all green - have I missed something or has Gábor fixed the\n> issue?\n\nNo, it was Dscho who fixed the Azure CI issue, see\n\n https://public-inbox.org/git/nycvar.QRO.7.76.6.1906141356140.44@tvgsbejvaqbjf.bet/\n\nelsewhere in this thread (it's a long one, but well worth the read\nIMO).\n\nHowever, the point here is not that Azure CI failure, but rather the\nfact that tests had to be updated to include the new line clearing\nsequence in the expected output, and that \"Q<...80 spaces...>Q\" looks\nyuck indeed.\n\n"},{"id":"377972","messageId":"nycvar.QRO.7.76.6.1906251336150.44@tvgsbejvaqbjf.bet","threadId":"51015","inReplyTo":"efc4a141-071a-6549-f25d-21cc6256832a@gmail.com","subject":"Re: [PATCH v2 3/4] rebase: fix garbled progress display with '-x'","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-06-25T11:38:14Z","receivedAt":"2019-06-25T11:38:03Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Phillip,\n\nOn Tue, 25 Jun 2019, Phillip Wood wrote:\n\n> On 24/06/2019 19:39, SZEDER Gábor wrote:\n> > On Wed, Jun 12, 2019 at 09:14:40PM +0200, Johannes Schindelin wrote:\n>\n> > The other yucks affect the following four tests in\n> > 't3420-rebase-autostash.sh':\n> >\n> > 16 - rebase --merge --autostash: check output\n> > 23 - rebase --merge: check output with conflicting stash\n> > 26 - rebase --interactive --autostash: check output\n> > 33 - rebase --interactive: check output with conflicting stash\n> >\n> > These tests come from commits b76aeae553 (rebase: add regression tests\n> > for console output, 2017-06-19) and 7d70e6b902 (rebase: add more\n> > regression tests for console output, 2017-06-19), and are specifically\n> > about checking the (whole) console output of 'git rebase', so I left\n> > the updates to them as they were.\n> >\n> > In any case, Cc-ing Phillip to discuss whether something could be done\n> > about them (now perhaps preferably (for me :) as a follow-up, and not\n> > another preparatory patches).\n>\n> Those tests were added to check that `git stash` was being silenced (see\n> 79a6226981 (\"rebase -i: silence stash apply\", 2017-05-18)). I can have a\n> think about a better way to do that, but is it still a problem? I just\n> tried to take a look at your CI output and\n> https://dev.azure.com/gitgitgadget/git/_build/results?buildId=11406\n> seems to be all green - have I missed something or has Gábor fixed the\n> issue?\n\nAFAIU the test cases were patched to accommodate for the new output.\nIdeally, they would be changed to not need any changes in the future,\ncertainly not to accommodate changes in unrelated areas (such as the\nprogress).\n\nSo yes, the build is green, and the patches are probably good to go, but\nthere is room for add-on patches to clean up the test cases to test\nsuccinctly what they are supposed to test.\n\nCiao,\nDscho\n"},{"id":"377980","messageId":"a6aef2a0-bacd-9af2-0e96-9048da6c9a93@gmail.com","threadId":"51015","inReplyTo":"20190625113103.GB10853@szeder.dev","subject":"Re: [PATCH v2 3/4] rebase: fix garbled progress display with '-x'","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-06-25T13:33:28Z","receivedAt":"2019-06-25T13:33:39Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 25/06/2019 12:31, SZEDER Gábor wrote:\n> On Tue, Jun 25, 2019 at 11:08:04AM +0100, Phillip Wood wrote:\n>>>> Rebasing (1/4)QRebasing (2/4)QRebasing (3/4)QRebasing (4/4)QQ QSuccessfully rebased and updated refs/heads/missing-commit.\n> \n> \n>>>> May I please *strongly* suggest to fix this first? It should\n>>>>\n>>>> - completely lose that last line,\n>>>> - be inserted into the test case itself so that e.g. disk full problems are\n>>>> caught and logged properly, and\n>>>> - the `test_i18ncmp expect actual` call in the test case should be replaced\n>>>> by something like:\n>>>>\n>>>> \tsed \"\\$d\" actual-skip-progress &&\n>>>> \ttest_i18ncmp expect actual-skip-progress\n>>>>\n>>>> This should obviously be made as a separate, introductory patch (probably with\n>>>> a less scathing commit message than my comments above would suggest).\n>>>>\n>>>> And that would also remove the double-yuck.\n>>>\n>>> Unfortunately, this addresses only one of the \"Yuck\"s; see v3 of this\n>>> patch series [1].\n>>>\n>>> The other yucks affect the following four tests in\n>>> 't3420-rebase-autostash.sh':\n>>>\n>>> 16 - rebase --merge --autostash: check output\n>>> 23 - rebase --merge: check output with conflicting stash\n>>> 26 - rebase --interactive --autostash: check output\n>>> 33 - rebase --interactive: check output with conflicting stash\n>>>\n>>> These tests come from commits b76aeae553 (rebase: add regression tests\n>>> for console output, 2017-06-19) and 7d70e6b902 (rebase: add more\n>>> regression tests for console output, 2017-06-19), and are specifically\n>>> about checking the (whole) console output of 'git rebase', so I left\n>>> the updates to them as they were.\n>>>\n>>> In any case, Cc-ing Phillip to discuss whether something could be done\n>>> about them (now perhaps preferably (for me :) as a follow-up, and not\n>>> another preparatory patches).\n>>\n>> Those tests were added to check that `git stash` was being silenced (see\n>> 79a6226981 (\"rebase -i: silence stash apply\", 2017-05-18)).\n> \n> Hmm, so would it then be possible/sensible to do something like this\n> instead:\n> \n> git rebase --options... >out &&\n> test_i18ngrep ! \"\" out\n\nYes we could do something like that, but I think there is merit in \ntesting the whole output as it catches any changes that accidentally \nstart polluting what the user sees. It also means that any changes to \nthe output are deliberate. It may be a pain to have to update a test \ncase when the output changes but at least it ensures the change was \ndeliberate. I'm not sure what we can do to get around the problems that \nin creates for unrelated changes such as this series.\n\n>> I can have a\n>> think about a better way to do that, but is it still a problem? I just\n>> tried to take a look at your CI output and\n>> https://dev.azure.com/gitgitgadget/git/_build/results?buildId=11406\n>> seems to be all green - have I missed something or has Gábor fixed the\n>> issue?\n> \n> No, it was Dscho who fixed the Azure CI issue, see\n> \n> https://public-inbox.org/git/nycvar.QRO.7.76.6.1906141356140.44@tvgsbejvaqbjf.bet/\n> \n> elsewhere in this thread (it's a long one, but well worth the read\n> IMO).\n\n\nThanks for the pointer, I'll have a read\n\n> However, the point here is not that Azure CI failure, but rather the\n> fact that tests had to be updated to include the new line clearing\n> sequence in the expected output, and that \"Q<...80 spaces...>Q\" looks\n> yuck indeed.\n\nAh, thanks for explaining\n\nBest Wishes\n\nPhillip\n\n"},{"id":"377981","messageId":"ad93318f-7304-2b99-2fe6-7c03d91989e2@gmail.com","threadId":"51015","inReplyTo":"nycvar.QRO.7.76.6.1906251336150.44@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 3/4] rebase: fix garbled progress display with '-x'","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-06-25T13:35:50Z","receivedAt":"2019-06-25T13:35:54Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Dscho\n\nOn 25/06/2019 12:38, Johannes Schindelin wrote:\n> Hi Phillip,\n> \n> On Tue, 25 Jun 2019, Phillip Wood wrote:\n> \n>> On 24/06/2019 19:39, SZEDER Gábor wrote:\n>>> On Wed, Jun 12, 2019 at 09:14:40PM +0200, Johannes Schindelin wrote:\n>>\n>>> The other yucks affect the following four tests in\n>>> 't3420-rebase-autostash.sh':\n>>>\n>>> 16 - rebase --merge --autostash: check output\n>>> 23 - rebase --merge: check output with conflicting stash\n>>> 26 - rebase --interactive --autostash: check output\n>>> 33 - rebase --interactive: check output with conflicting stash\n>>>\n>>> These tests come from commits b76aeae553 (rebase: add regression tests\n>>> for console output, 2017-06-19) and 7d70e6b902 (rebase: add more\n>>> regression tests for console output, 2017-06-19), and are specifically\n>>> about checking the (whole) console output of 'git rebase', so I left\n>>> the updates to them as they were.\n>>>\n>>> In any case, Cc-ing Phillip to discuss whether something could be done\n>>> about them (now perhaps preferably (for me :) as a follow-up, and not\n>>> another preparatory patches).\n>>\n>> Those tests were added to check that `git stash` was being silenced (see\n>> 79a6226981 (\"rebase -i: silence stash apply\", 2017-05-18)). I can have a\n>> think about a better way to do that, but is it still a problem? I just\n>> tried to take a look at your CI output and\n>> https://dev.azure.com/gitgitgadget/git/_build/results?buildId=11406\n>> seems to be all green - have I missed something or has Gábor fixed the\n>> issue?\n> \n> AFAIU the test cases were patched to accommodate for the new output.\n> Ideally, they would be changed to not need any changes in the future,\n> certainly not to accommodate changes in unrelated areas (such as the\n> progress).\n> \n> So yes, the build is green, and the patches are probably good to go, but\n> there is room for add-on patches to clean up the test cases to test\n> succinctly what they are supposed to test.\n\nThanks, I'd missed the point entirely! As I said in my reply to Gábor, I \nthink testing the whole output does have some use, but I can see that it \nwould be very good to get rid of the whole Q thing\n\nBest Wishes\n\nPhillip\n\n> \n> Ciao,\n> Dscho\n> \n"},{"id":"378033","messageId":"573b9280-939b-879e-6e5e-b63779836740@gmail.com","threadId":"51015","inReplyTo":"20190625113103.GB10853@szeder.dev","subject":"Re: [PATCH v2 3/4] rebase: fix garbled progress display with '-x'","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-06-25T18:00:42Z","receivedAt":"2019-06-25T18:00:49Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Gábor and Dscho\nOn 25/06/2019 12:31, SZEDER Gábor wrote:\n> On Tue, Jun 25, 2019 at 11:08:04AM +0100, Phillip Wood wrote:\n>>>> Rebasing (1/4)QRebasing (2/4)QRebasing (3/4)QRebasing (4/4)QQ QSuccessfully rebased and updated refs/heads/missing-commit.\n> \n> \n>>>> May I please *strongly* suggest to fix this first? It should\n>>>>\n>>>> - completely lose that last line,\n>>>> - be inserted into the test case itself so that e.g. disk full problems are\n>>>> caught and logged properly, and\n>>>> - the `test_i18ncmp expect actual` call in the test case should be replaced\n>>>> by something like:\n>>>>\n>>>> \tsed \"\\$d\" actual-skip-progress &&\n>>>> \ttest_i18ncmp expect actual-skip-progress\n>>>>\n>>>> This should obviously be made as a separate, introductory patch (probably with\n>>>> a less scathing commit message than my comments above would suggest).\n>>>>\n>>>> And that would also remove the double-yuck.\n>>>\n>>> Unfortunately, this addresses only one of the \"Yuck\"s; see v3 of this\n>>> patch series [1].\n>>>\n>>> The other yucks affect the following four tests in\n>>> 't3420-rebase-autostash.sh':\n>>>\n>>> 16 - rebase --merge --autostash: check output\n>>> 23 - rebase --merge: check output with conflicting stash\n>>> 26 - rebase --interactive --autostash: check output\n>>> 33 - rebase --interactive: check output with conflicting stash\n>>>\n>>> These tests come from commits b76aeae553 (rebase: add regression tests\n>>> for console output, 2017-06-19) and 7d70e6b902 (rebase: add more\n>>> regression tests for console output, 2017-06-19), and are specifically\n>>> about checking the (whole) console output of 'git rebase', so I left\n>>> the updates to them as they were.\n>>>\n>>> In any case, Cc-ing Phillip to discuss whether something could be done\n>>> about them (now perhaps preferably (for me :) as a follow-up, and not\n>>> another preparatory patches).\n>>\n>> Those tests were added to check that `git stash` was being silenced (see\n>> 79a6226981 (\"rebase -i: silence stash apply\", 2017-05-18)).\n> \n> Hmm, so would it then be possible/sensible to do something like this\n> instead:\n> \n> git rebase --options... >out &&\n> test_i18ngrep ! \"\" out\n> \n>> I can have a\n>> think about a better way to do that, but is it still a problem? I just\n>> tried to take a look at your CI output and\n>> https://dev.azure.com/gitgitgadget/git/_build/results?buildId=11406\n>> seems to be all green - have I missed something or has Gábor fixed the\n>> issue?\n> \n> No, it was Dscho who fixed the Azure CI issue, see\n> \n> https://public-inbox.org/git/nycvar.QRO.7.76.6.1906141356140.44@tvgsbejvaqbjf.bet/\n> \n> elsewhere in this thread (it's a long one, but well worth the read\n> IMO).\n> \n> However, the point here is not that Azure CI failure, but rather the\n> fact that tests had to be updated to include the new line clearing\n> sequence in the expected output, and that \"Q<...80 spaces...>Q\" looks\n> yuck indeed.\n\nI've come up with a sed based solution to remove the progress\nnotifications from the output - see\nhttps://github.com/gitgitgadget/git/pull/276 If you're both happy with\nthe basic idea I'll clean it up and submit it (I've just noticed I left\nsome debugging bits in one of the tests). I was worried using \"\\r\" in a\nsed expression wouldn't be portable but the tests pass on gitgitgadget.\nIf it proves to be a problem on other platforms we could use always tr\nto convert \"\\r\" to some unique string and use that string in the sed\nexpression.\n\nBest Wishes\n\nPhillip\n\n"},{"id":"378038","messageId":"xmqq5zotfo1k.fsf@gitster-ct.c.googlers.com","threadId":"51015","inReplyTo":"nycvar.QRO.7.76.6.1906251110160.44@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v3 5/5] progress: use term_clear_line()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-06-25T19:44:07Z","receivedAt":"2019-06-25T19:44:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> To make sure that the previously displayed progress line is completely\n>> covered up when the new line is shorter, commit 545dc345eb (progress:\n>> break too long progress bar lines, 2019-04-12) added a bunch of\n>> calculations to figure out how many characters it needs to overwrite\n>> with spaces.\n>> ...\n> Very nice change, indeed, I totally love how much simpler the post-image\n> looks. Well done, for the entire patch series, and thank you so much!\n> Dscho\n\nThanks, both. Let's merge down sg/rebase-progress soonish.\n\n"},{"id":"378160","messageId":"20190627134248.28469-1-szeder.dev@gmail.com","threadId":"51015","inReplyTo":"20190624181318.17388-5-szeder.dev@gmail.com","subject":"[PATCH v3.1 4/5] rebase: fix garbled progress display with '-x'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-06-27T13:42:48Z","receivedAt":"2019-06-27T13:43:00Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"When running a command with the 'exec' instruction during an\ninteractive rebase session, or for a range of commits using 'git\nrebase -x', the output can be a bit garbled when the name of the\ncommand is short enough:\n\n $ git rebase -x true HEAD~5\n Executing: true\n Executing: true\n Executing: true\n Executing: true\n Executing: true)\n Successfully rebased and updated refs/heads/master.\n\nNote the ')' at the end of the last line. It gets more garbled as the\nrange of commits increases:\n\n $ git rebase -x true HEAD~50\n Executing: true)\n [ repeated 3 more times ]\n Executing: true0)\n [ repeated 44 more times ]\n Executing: true00)\n Successfully rebased and updated refs/heads/master.\n\nThose extra numbers and ')' are remnants of the previously displayed\n\"Rebasing (N/M)\" progress lines that are usually completely\noverwritten by the \"Executing: \" lines, unless 'cmd' is short and\nthe \"N/M\" part is long.\n\nMake sure that the previously displayed \"Rebasing (N/M)\" line is\ncleared by using the term_clear_line() helper function added in the\nprevious patch. Do so only when not being '--verbose', because in\nthat case these \"Rebasing (N/M)\" lines are not printed as progress\n(i.e. as lines with '\\r' at the end), but as \"regular\" output (with\n'\\n' at the end).\n\nA couple of other rebase commands print similar messages, e.g.\n\"Stopped at ... \" for the 'edit' or 'break'\ncommands, or the \"Successfully rebased and updated .\" at the\nvery end. These are so long that they practically always overwrite\nthat \"Rebasing (N/M)\" progress line, but let's be prudent, and clear\nthe last line before printing these, too.\n\nIn 't3420-rebase-autostash.sh' two helper functions prepare the\nexpected output of four tests that check the full output of 'git\nrebase' and thus are affected by this change, so adjust their\nexpectations to account for the new line clearing.\n\nNote that this patch doesn't completely eliminate the possibility of\nsimilar garbled outputs, e.g. some error messages from rebase or the\n\"Auto-merging \" message from within the depths of the merge\nmachinery might not be long enough to completely cover the last\n\"Rebasing (N/M)\" line. This patch doesn't do anything about them,\nbecause dealing with them individually would result in way too much\nchurn, while having a catch-all term_clear_line() call in the common\ncode path of pick_commits() would hide the \"Rebasing (N/M)\" line way\ntoo soon, and it would either flicker or be invisible.\n\nSigned-off-by: SZEDER Gábor \n---\n\nThe exact same patch with a slightly updated commit message to address\n\n https://public-inbox.org/git/xmqqy327kfw1.fsf@gitster-ct.c.googlers.com/\n\nand to fix a typo.\n\nRange-diff:\n1: 4372a3cde4 = 1: 4372a3cde4 t3404: modernize here doc style\n2: 5444f547c5 = 2: 5444f547c5 t3404: make the 'rebase.missingCommitsCheck=ignore' test more focused\n3: 5ebf218cb9 = 3: 5ebf218cb9 pager: add a helper function to clear the last line in the terminal\n4: 51cd5ccd46 ! 4: 652b69d0dd rebase: fix garbled progress display with '-x'\n @@ -33,7 +33,10 @@\n \n Make sure that the previously displayed \"Rebasing (N/M)\" line is\n cleared by using the term_clear_line() helper function added in the\n - previous patch.\n + previous patch. Do so only when not being '--verbose', because in\n + that case these \"Rebasing (N/M)\" lines are not printed as progress\n + (i.e. as lines with '\\r' at the end), but as \"regular\" output (with\n + '\\n' at the end).\n \n A couple of other rebase commands print similar messages, e.g.\n \"Stopped at ... \" for the 'edit' or 'break'\n @@ -49,7 +52,7 @@\n \n Note that this patch doesn't completely eliminate the possibility of\n similar garbled outputs, e.g. some error messages from rebase or the\n - \"Auto-merging \" message from withing the depths of the merge\n + \"Auto-merging \" message from within the depths of the merge\n machinery might not be long enough to completely cover the last\n \"Rebasing (N/M)\" line. This patch doesn't do anything about them,\n because dealing with them individually would result in way too much\n5: 09642a458e = 5: 313ce83d0a progress: use term_clear_line()\n\n\n sequencer.c | 17 ++++++++++++++---\n t/t3420-rebase-autostash.sh | 4 ++--\n 2 files changed, 16 insertions(+), 5 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex f88a97fb10..63b09cfceb 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3731,8 +3731,11 @@ static int pick_commits(struct repository *r,\n \t\t\tunlink(git_path_merge_head(the_repository));\n \t\t\tdelete_ref(NULL, \"REBASE_HEAD\", NULL, REF_NO_DEREF);\n \n-\t\t\tif (item->command == TODO_BREAK)\n+\t\t\tif (item->command == TODO_BREAK) {\n+\t\t\t\tif (!opts->verbose)\n+\t\t\t\t\tterm_clear_line();\n \t\t\t\treturn stopped_at_head(r);\n+\t\t\t}\n \t\t}\n \t\tif (item->command <= TODO_SQUASH) {\n \t\t\tif (is_rebase_i(opts))\n@@ -3754,11 +3757,14 @@ static int pick_commits(struct repository *r,\n \t\t\t}\n \t\t\tif (item->command == TODO_EDIT) {\n \t\t\t\tstruct commit *commit = item->commit;\n-\t\t\t\tif (!res)\n+\t\t\t\tif (!res) {\n+\t\t\t\t\tif (!opts->verbose)\n+\t\t\t\t\t\tterm_clear_line();\n \t\t\t\t\tfprintf(stderr,\n \t\t\t\t\t\t_(\"Stopped at %s... %.*s\\n\"),\n \t\t\t\t\t\tshort_commit_name(commit),\n \t\t\t\t\t\titem->arg_len, arg);\n+\t\t\t\t}\n \t\t\t\treturn error_with_patch(r, commit,\n \t\t\t\t\targ, item->arg_len, opts, res, !res);\n \t\t\t}\n@@ -3796,6 +3802,8 @@ static int pick_commits(struct repository *r,\n \t\t\tint saved = *end_of_arg;\n \t\t\tstruct stat st;\n \n+\t\t\tif (!opts->verbose)\n+\t\t\t\tterm_clear_line();\n \t\t\t*end_of_arg = '\\0';\n \t\t\tres = do_exec(r, arg);\n \t\t\t*end_of_arg = saved;\n@@ -3954,10 +3962,13 @@ static int pick_commits(struct repository *r,\n \t\t}\n \t\tapply_autostash(opts);\n \n-\t\tif (!opts->quiet)\n+\t\tif (!opts->quiet) {\n+\t\t\tif (!opts->verbose)\n+\t\t\t\tterm_clear_line();\n \t\t\tfprintf(stderr,\n \t\t\t\t\"Successfully rebased and updated %s.\\n\",\n \t\t\t\thead_ref.buf);\n+\t\t}\n \n \t\tstrbuf_release(&buf);\n \t\tstrbuf_release(&head_ref);\ndiff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh\nindex 2d1094e483..9186e90127 100755\n--- a/t/t3420-rebase-autostash.sh\n+++ b/t/t3420-rebase-autostash.sh\n@@ -49,7 +49,7 @@ create_expected_success_interactive () {\n \t$(grep \"^Created autostash: [0-9a-f][0-9a-f]*\\$\" actual)\n \tHEAD is now at $(git rev-parse --short feature-branch) third commit\n \tRebasing (1/2)QRebasing (2/2)QApplied autostash.\n-\tSuccessfully rebased and updated refs/heads/rebased-feature-branch.\n+\tQ QSuccessfully rebased and updated refs/heads/rebased-feature-branch.\n \tEOF\n }\n \n@@ -73,7 +73,7 @@ create_expected_failure_interactive () {\n \tRebasing (1/2)QRebasing (2/2)QApplying autostash resulted in conflicts.\n \tYour changes are safe in the stash.\n \tYou can run \"git stash pop\" or \"git stash drop\" at any time.\n-\tSuccessfully rebased and updated refs/heads/rebased-feature-branch.\n+\tQ QSuccessfully rebased and updated refs/heads/rebased-feature-branch.\n \tEOF\n }\n \n-- \n2.22.0.589.g5bd7971b91\n\n"}]}