[PATCH] rebase: fix garbled progress display with '-x'

[PATCH] rebase: fix garbled progress display with '-x'

[SZEDER Gábor]

When running a command with the 'exec' instruction during an
interactive rebase session, or for a range of commits using 'git
rebase -x', the output can be a bit garbled when the name of the
command is short enough:

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

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

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

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

Make sure that the previously displayed "Rebasing (N/M)" line is
completely covered up by printing a terminal width worth of space
characters.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---

This issue has already been present in the scripted rebase as well.

As far as I could tell, if any other rebase instruction prints a
message, then that tends to be so long (including abbreviated commit
OIDs and whatnot) that they practically always overwrite that
"Rebasing (N/M)" progress line (well, except, perhaps, when rebasing
billions of commits at a time?).

 sequencer.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/sequencer.c b/sequencer.c
index 546f281898..c2e4baa90e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3631,6 +3631,12 @@ static int pick_commits(struct repository *r,
 			int saved = *end_of_arg;
 			struct stat st;
 
+			if (!opts->verbose)
+				/*
+				 * Fully cover the previous "Rebasing (n/m)"
+				 * progress line.
+				 */
+				fprintf(stderr, "%*s\r", term_columns(), "");
 			*end_of_arg = '\0';
 			res = do_exec(r, arg);
 			*end_of_arg = saved;
-- 
2.21.0.1181.g24122a4251

Re: [PATCH] rebase: fix garbled progress display with '-x'

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 2798 bytes --]

Hi,

On Tue, 30 Apr 2019, SZEDER Gábor wrote:

> When running a command with the 'exec' instruction during an
> interactive rebase session, or for a range of commits using 'git
> rebase -x', the output can be a bit garbled when the name of the
> command is short enough:
>
>   $ git rebase -x true HEAD~5
>   Executing: true
>   Executing: true
>   Executing: true
>   Executing: true
>   Executing: true)
>   Successfully rebased and updated refs/heads/master.
>
> Note the ')' at the end of the last line.  It gets more garbled as the
> range of commits increases:
>
>   $ git rebase -x true HEAD~50
>   Executing: true)
>   [ repeated 3 more times ]
>   Executing: true0)
>   [ repeated 44 more times ]
>   Executing: true00)
>   Successfully rebased and updated refs/heads/master.
>
> Those extra numbers and ')' are remnants of the previously displayed
> "Rebasing (N/M)" progress lines that are usually completely
> overwritten by the "Executing: <cmd>" lines, unless 'cmd' is short and
> the "N/M" part is long.
>
> Make sure that the previously displayed "Rebasing (N/M)" line is
> completely covered up by printing a terminal width worth of space
> characters.
>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>

Makes sense.

> This issue has already been present in the scripted rebase as well.
>
> As far as I could tell, if any other rebase instruction prints a
> message, then that tends to be so long (including abbreviated commit
> OIDs and whatnot) that they practically always overwrite that
> "Rebasing (N/M)" progress line (well, except, perhaps, when rebasing
> billions of commits at a time?).
>
>  sequencer.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/sequencer.c b/sequencer.c
> index 546f281898..c2e4baa90e 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -3631,6 +3631,12 @@ static int pick_commits(struct repository *r,
>  			int saved = *end_of_arg;
>  			struct stat st;
>
> +			if (!opts->verbose)
> +				/*
> +				 * Fully cover the previous "Rebasing (n/m)"
> +				 * progress line.
> +				 */
> +				fprintf(stderr, "%*s\r", term_columns(), "");

IIRC there are terminals (`cmd.exe`?) that would advance to the next row
automatically when printing the exact number of columns in a row. So this
would not work.

But isn't there an ANSI sequence that we can use?

*clicketyclick*

Yes: https://github.com/git/git/blob/v2.21.0/editor.c#L101 (introduced in
https://github.com/git/git/commit/abfb04d0c7#diff-cdeec438beb851e450b94a11db9ab7edR89)

So maybe we should do the same here, i.e.

	fputs("\r\033[K", stderr);

Ciao,
Dscho

>  			*end_of_arg = '\0';
>  			res = do_exec(r, arg);
>  			*end_of_arg = saved;
> --
> 2.21.0.1181.g24122a4251
>
>

Re: [PATCH] rebase: fix garbled progress display with '-x'

[SZEDER Gábor]

On Tue, Apr 30, 2019 at 06:25:35PM -0400, Johannes Schindelin wrote:
> Hi,
> 
> On Tue, 30 Apr 2019, SZEDER Gábor wrote:
> 
> > When running a command with the 'exec' instruction during an
> > interactive rebase session, or for a range of commits using 'git
> > rebase -x', the output can be a bit garbled when the name of the
> > command is short enough:
> >
> >   $ git rebase -x true HEAD~5
> >   Executing: true
> >   Executing: true
> >   Executing: true
> >   Executing: true
> >   Executing: true)
> >   Successfully rebased and updated refs/heads/master.
> >
> > Note the ')' at the end of the last line.  It gets more garbled as the
> > range of commits increases:
> >
> >   $ git rebase -x true HEAD~50
> >   Executing: true)
> >   [ repeated 3 more times ]
> >   Executing: true0)
> >   [ repeated 44 more times ]
> >   Executing: true00)
> >   Successfully rebased and updated refs/heads/master.
> >
> > Those extra numbers and ')' are remnants of the previously displayed
> > "Rebasing (N/M)" progress lines that are usually completely
> > overwritten by the "Executing: <cmd>" lines, unless 'cmd' is short and
> > the "N/M" part is long.
> >
> > Make sure that the previously displayed "Rebasing (N/M)" line is
> > completely covered up by printing a terminal width worth of space
> > characters.
> >
> > Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> 
> Makes sense.
> 
> > This issue has already been present in the scripted rebase as well.
> >
> > As far as I could tell, if any other rebase instruction prints a
> > message, then that tends to be so long (including abbreviated commit
> > OIDs and whatnot) that they practically always overwrite that
> > "Rebasing (N/M)" progress line (well, except, perhaps, when rebasing
> > billions of commits at a time?).
> >
> >  sequencer.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/sequencer.c b/sequencer.c
> > index 546f281898..c2e4baa90e 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -3631,6 +3631,12 @@ static int pick_commits(struct repository *r,
> >  			int saved = *end_of_arg;
> >  			struct stat st;
> >
> > +			if (!opts->verbose)
> > +				/*
> > +				 * Fully cover the previous "Rebasing (n/m)"
> > +				 * progress line.
> > +				 */
> > +				fprintf(stderr, "%*s\r", term_columns(), "");
> 
> IIRC there are terminals (`cmd.exe`?) that would advance to the next row
> automatically when printing the exact number of columns in a row. So this
> would not work.

Hrm, I though about using 'term_columns()-1', or moving the '\r' from
the format string to the string to be printed, but in the end didn't
do either, because it seemed to work well as it is in the two
terminals that I tried (on Linux).

> But isn't there an ANSI sequence that we can use?
> 
> *clicketyclick*
> 
> Yes: https://github.com/git/git/blob/v2.21.0/editor.c#L101 (introduced in
> https://github.com/git/git/commit/abfb04d0c7#diff-cdeec438beb851e450b94a11db9ab7edR89)
> 
> So maybe we should do the same here, i.e.
> 
> 	fputs("\r\033[K", stderr);

Oh, that would be nice (and not only here, but it could have made the
changes in 'sg/overlong-progress-fix' a bit simpler as well).
Unfortunately, however, it only works on non-dumb terminals (note the
'!is_terminal_dumb()' call in the preceeding condition), while rebase
hasn't had such a limitation on the terminal yet.

> Ciao,
> Dscho
> 
> >  			*end_of_arg = '\0';
> >  			res = do_exec(r, arg);
> >  			*end_of_arg = saved;
> > --
> > 2.21.0.1181.g24122a4251
> >
> >

Re: [PATCH] rebase: fix garbled progress display with '-x'

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 2053 bytes --]

Hi,

On Thu, 2 May 2019, SZEDER Gábor wrote:

> On Tue, Apr 30, 2019 at 06:25:35PM -0400, Johannes Schindelin wrote:
> >
> > On Tue, 30 Apr 2019, SZEDER Gábor wrote:
> >
> > > diff --git a/sequencer.c b/sequencer.c
> > > index 546f281898..c2e4baa90e 100644
> > > --- a/sequencer.c
> > > +++ b/sequencer.c
> > > @@ -3631,6 +3631,12 @@ static int pick_commits(struct repository *r,
> > >  			int saved = *end_of_arg;
> > >  			struct stat st;
> > >
> > > +			if (!opts->verbose)
> > > +				/*
> > > +				 * Fully cover the previous "Rebasing (n/m)"
> > > +				 * progress line.
> > > +				 */
> > > +				fprintf(stderr, "%*s\r", term_columns(), "");
> >
> > IIRC there are terminals (`cmd.exe`?) that would advance to the next row
> > automatically when printing the exact number of columns in a row. So this
> > would not work.
>
> Hrm, I though about using 'term_columns()-1', or moving the '\r' from
> the format string to the string to be printed, but in the end didn't
> do either, because it seemed to work well as it is in the two
> terminals that I tried (on Linux).
>
> > But isn't there an ANSI sequence that we can use?
> >
> > *clicketyclick*
> >
> > Yes: https://github.com/git/git/blob/v2.21.0/editor.c#L101 (introduced in
> > https://github.com/git/git/commit/abfb04d0c7#diff-cdeec438beb851e450b94a11db9ab7edR89)
> >
> > So maybe we should do the same here, i.e.
> >
> > 	fputs("\r\033[K", stderr);
>
> Oh, that would be nice (and not only here, but it could have made the
> changes in 'sg/overlong-progress-fix' a bit simpler as well).
> Unfortunately, however, it only works on non-dumb terminals (note the
> '!is_terminal_dumb()' call in the preceeding condition), while rebase
> hasn't had such a limitation on the terminal yet.

I think it would still be nice to have it in the common case of a capable
terminal.

Maybe this could even become a helper function, e.g.
`erase_terminal_line_remainder()`? This should also help other developers
discover and use it.

Ciao,
Dscho

Re: [PATCH] rebase: fix garbled progress display with '-x'

[SZEDER Gábor]

On Thu, May 02, 2019 at 01:16:40AM +0200, SZEDER Gábor wrote:
> On Tue, Apr 30, 2019 at 06:25:35PM -0400, Johannes Schindelin wrote:
> > > Make sure that the previously displayed "Rebasing (N/M)" line is
> > > completely covered up by printing a terminal width worth of space
> > > characters.

> > > +			if (!opts->verbose)
> > > +				/*
> > > +				 * Fully cover the previous "Rebasing (n/m)"
> > > +				 * progress line.
> > > +				 */
> > > +				fprintf(stderr, "%*s\r", term_columns(), "");
> > 
> > IIRC there are terminals (`cmd.exe`?) that would advance to the next row
> > automatically when printing the exact number of columns in a row. So this
> > would not work.
> 
> Hrm, I though about using 'term_columns()-1', or moving the '\r' from
> the format string to the string to be printed, but in the end didn't
> do either, because it seemed to work well as it is in the two
> terminals that I tried (on Linux).

And I would have been wrong doing so, because that fprintf'ed string
ends with '\r', not '\n', and therefore it is actually necessary to
write as many space characters as the width of the terminal, or the
last character in the previously displayed line won't get overwritten.

[PATCH v2 0/4] rebase/progress: add and use term_clear_line()

[SZEDER Gábor]

This is a reworked/extended version of the single patch "rebase: fix
garbled progress display with '-x'" at:

  https://public-inbox.org/git/20190430142556.20921-1-szeder.dev@gmail.com/T/#u

Changes:

  - Add a helper function to clear the whole line on the terminal,
    using an ANSI escape sequence on capable terminals and a
    terminal-width worth of spaces on dumb terminals.

  - Use this helper when fixing rebase's garbled progress display with
    '-x'.

  - Use this helper in progress.c:display() as well to get rid of
    quite some arithmetic that made a -rc-phase bugfix necessary.

I don't really like the necessary adjustments to the tests in patches
3 and 4, though.


SZEDER Gábor (4):
  t3404-rebase-interactive: use the 'q_to_cr' helper
  pager: add a helper function to clear the last line in the terminal
  rebase: fix garbled progress display with '-x'
  progress: use term_clear_line()

 cache.h                       |  1 +
 editor.c                      |  6 +++---
 pager.c                       | 20 ++++++++++++++++++++
 progress.c                    | 28 +++++++++++-----------------
 sequencer.c                   | 17 ++++++++++++++---
 t/t3404-rebase-interactive.sh | 15 +++------------
 t/t3420-rebase-autostash.sh   |  4 ++--
 t/t5541-http-push-smart.sh    |  6 +++---
 8 files changed, 57 insertions(+), 40 deletions(-)

-- 
2.22.0.566.g58873a45ff

[PATCH v2 1/4] t3404-rebase-interactive: use the 'q_to_cr' helper

[SZEDER Gábor]

The test 'rebase -i respects rebase.missingCommitsCheck = warn' checks
the output of 'git rebase', including progress lines: it prepares the
expected output from a here-doc using LFs at the end of progress
lines, and converts the command's actual output by replacing the
progress lines' CRs with LFs with a one-shot local helper funcion.
Similar tests (in t3420) thoroughly checking 'git rebase's output do
it the other way around: they leave the output of 'git rebase' as-is,
and prepare the expected output with the necessary CRs in it using the
common 'q_to_cr' test helper function from 'test-lib-functions.sh'.

Change this test to use the 'q_to_cr' test helper to prepare the
expected output, too.

This way this test will be more consistent with other, similar tests,
and it will make further updates to this test in a later patch of this
series just a tad bit simpler.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t3404-rebase-interactive.sh | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 1723e1a858..e66e95d453 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1304,7 +1304,7 @@ test_expect_success 'rebase -i respects rebase.missingCommitsCheck = ignore' '
 		actual
 '
 
-cat >expect <<EOF
+q_to_cr >expect <<EOF
 Warning: some commits may have been dropped accidentally.
 Dropped commits (newer to older):
  - $(git rev-list --pretty=oneline --abbrev-commit -1 master)
@@ -1313,24 +1313,15 @@ To avoid this message, use "drop" to explicitly remove a commit.
 Use 'git config rebase.missingCommitsCheck' to change the level of warnings.
 The possible behaviours are: ignore, warn, error.
 
-Rebasing (1/4)
-Rebasing (2/4)
-Rebasing (3/4)
-Rebasing (4/4)
-Successfully rebased and updated refs/heads/missing-commit.
+Rebasing (1/4)QRebasing (2/4)QRebasing (3/4)QRebasing (4/4)QSuccessfully rebased and updated refs/heads/missing-commit.
 EOF
 
-cr_to_nl () {
-	tr '\015' '\012'
-}
-
 test_expect_success 'rebase -i respects rebase.missingCommitsCheck = warn' '
 	test_config rebase.missingCommitsCheck warn &&
 	rebase_setup_and_clean missing-commit &&
 	set_fake_editor &&
 	FAKE_LINES="1 2 3 4" \
-		git rebase -i --root 2>actual.2 &&
-	cr_to_nl <actual.2 >actual &&
+		git rebase -i --root 2>actual &&
 	test_i18ncmp expect actual &&
 	test D = $(git cat-file commit HEAD | sed -ne \$p)
 '
-- 
2.22.0.566.g58873a45ff

[PATCH v2 2/4] pager: add a helper function to clear the last line in the terminal

[SZEDER Gábor]

There are a couple of places where we want to clear the last line on
the terminal, e.g. when a progress bar line is overwritten by a
shorter line, then the end of that progress line would remain visible,
unless we cover it up.

In 'progress.c' we did this by always appending a fixed number of
space characters to the next line (even if it was not shorter than the
previous), but as it turned out that fixed number was not quite large
enough, see the fix in 9f1fd84e15 (progress: clear previous progress
update dynamically, 2019-04-12).  From then on we've been keeping
track of the length of the last displayed progress line and appending
the appropriate number of space characters to the next line, if
necessary, but, alas, this approach turned out to be error prone, see
the fix in 1aed1a5f25 (progress: avoid empty line when breaking the
progress line, 2019-05-19).  The next patch in this series is about to
fix a case where we don't clear the last line, and on occasion do end
up with such garbage at the end of the line.  It would be great if we
could do that without the need to deal with that without meticulously
computing the necessary number of space characters.

So add a helper function to clear the last line on the terminal using
an ANSI escape sequence, which has the advantage to clear the whole
line no matter how wide it is, even after the terminal width changed.
Such an escape sequence is not available on dumb terminals, though, so
in that case fall back to simply print a whole terminal width (as
reported by term_columns()) worth of space characters.

In 'editor.c' launch_specified_editor() already used this ANSI escape
sequence, so replace it with a call to this function.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 cache.h  |  1 +
 editor.c |  6 +++---
 pager.c  | 20 ++++++++++++++++++++
 3 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/cache.h b/cache.h
index b4bb2e2c11..5b2cd32bad 100644
--- a/cache.h
+++ b/cache.h
@@ -1759,6 +1759,7 @@ void setup_pager(void);
 int pager_in_use(void);
 extern int pager_use_color;
 int term_columns(void);
+void term_clear_line(void);
 int decimal_width(uintmax_t);
 int check_pager_config(const char *cmd);
 void prepare_pager_args(struct child_process *, const char *pager);
diff --git a/editor.c b/editor.c
index 71547674ab..f079abbf11 100644
--- a/editor.c
+++ b/editor.c
@@ -96,10 +96,10 @@ static int launch_specified_editor(const char *editor, const char *path,
 
 		if (print_waiting_for_editor && !is_terminal_dumb())
 			/*
-			 * Go back to the beginning and erase the entire line to
-			 * avoid wasting the vertical space.
+			 * Erase the entire line to avoid wasting the
+			 * vertical space.
 			 */
-			fputs("\r\033[K", stderr);
+			term_clear_line();
 	}
 
 	if (!buffer)
diff --git a/pager.c b/pager.c
index 4168460ae9..41446d4f05 100644
--- a/pager.c
+++ b/pager.c
@@ -177,6 +177,26 @@ int term_columns(void)
 	return term_columns_at_startup;
 }
 
+/*
+ * Clear the entire line, leave cursor in first column.
+ */
+void term_clear_line(void)
+{
+	if (is_terminal_dumb())
+		/*
+		 * Fall back to print a terminal width worth of space
+		 * characters (hoping that the terminal is still as wide
+		 * as it was upon the first call to term_columns()).
+		 */
+		fprintf(stderr, "\r%*s\r", term_columns(), "");
+	else
+		/*
+		 * On non-dumb terminals use an escape sequence to clear
+		 * the whole line, no matter how wide the terminal.
+		 */
+		fputs("\r\033[K", stderr);
+}
+
 /*
  * How many columns do we need to show this number in decimal?
  */
-- 
2.22.0.566.g58873a45ff

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

[SZEDER Gábor]

When running a command with the 'exec' instruction during an
interactive rebase session, or for a range of commits using 'git
rebase -x', the output can be a bit garbled when the name of the
command is short enough:

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

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

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

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

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

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

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

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

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

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

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

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

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

Yuck, ... but I do not see how else/better this test can be written
myself, which makes it a double-yuck X-<

Are we forcing out test to operate under dumb terminal mode and with
a known number of columns?

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

[SZEDER Gábor]

On Tue, Jun 11, 2019 at 01:36:16PM -0700, Junio C Hamano wrote:
> SZEDER Gábor <szeder.dev@gmail.com> writes:
> 
> > -Rebasing (1/4)QRebasing (2/4)QRebasing (3/4)QRebasing (4/4)QSuccessfully rebased and updated refs/heads/missing-commit.
> > +Rebasing (1/4)QRebasing (2/4)QRebasing (3/4)QRebasing (4/4)QQ                                                                                QSuccessfully rebased and updated refs/heads/missing-commit.
> >  EOF
> 
> Yuck, 

Oh yeah...

>... but I do not see how else/better this test can be written
> myself, which makes it a double-yuck X-<

Perhaps hiding those spaces behind a helper variable e.g.
'dump_term_clear_line=Q<80-spaces>Q' and embedding that in the here
docs specifying the expected output in these three tests could make it
ever so slightly less yuck...

> Are we forcing out test to operate under dumb terminal mode and with
> a known number of columns?

'test-lib.sh' sets TERM=dumb relatively early on, and in these tests
we don't use 'test_terminal' to run 'git rebase', so...  yeah.  And
term_columns() defaults to 80.

However, if the terminal were smart, then we would have to deal with
ANSI escape suddenly popping up...

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 7672 bytes --]

Hi,

On Tue, 11 Jun 2019, SZEDER Gábor wrote:

> On Tue, Jun 11, 2019 at 01:36:16PM -0700, Junio C Hamano wrote:
> > SZEDER Gábor <szeder.dev@gmail.com> writes:
> >
> > > -Rebasing (1/4)QRebasing (2/4)QRebasing (3/4)QRebasing (4/4)QSuccessfully rebased and updated refs/heads/missing-commit.
> > > +Rebasing (1/4)QRebasing (2/4)QRebasing (3/4)QRebasing (4/4)QQ                                                                                QSuccessfully rebased and updated refs/heads/missing-commit.
> > >  EOF
> >
> > Yuck,
>
> Oh yeah...
>
> >... but I do not see how else/better this test can be written
> > myself, which makes it a double-yuck X-<
>
> Perhaps hiding those spaces behind a helper variable e.g.
> 'dump_term_clear_line=Q<80-spaces>Q' and embedding that in the here
> docs specifying the expected output in these three tests could make it
> ever so slightly less yuck...
>
> > Are we forcing out test to operate under dumb terminal mode and with
> > a known number of columns?
>
> 'test-lib.sh' sets TERM=dumb relatively early on, and in these tests
> we don't use 'test_terminal' to run 'git rebase', so...  yeah.  And
> term_columns() defaults to 80.
>
> However, if the terminal were smart, then we would have to deal with
> ANSI escape suddenly popping up...

And I fear that is *exactly* what makes
https://dev.azure.com/gitgitgadget/git/_build/results?buildId=10539&view=ms.vss-test-web.build-test-results-tab
fail...

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

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

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

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

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

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

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

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

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

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

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

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

Here it is, in its full "glory":

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

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

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

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

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

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

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

And that would also remove the double-yuck.

Ciao,
Dscho

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

[SZEDER Gábor]

On Wed, Jun 12, 2019 at 09:14:40PM +0200, Johannes Schindelin wrote:
> Hi,
> 
> On Tue, 11 Jun 2019, SZEDER Gábor wrote:
> 
> > On Tue, Jun 11, 2019 at 01:36:16PM -0700, Junio C Hamano wrote:
> > > SZEDER Gábor <szeder.dev@gmail.com> writes:
> > >
> > > > -Rebasing (1/4)QRebasing (2/4)QRebasing (3/4)QRebasing (4/4)QSuccessfully rebased and updated refs/heads/missing-commit.
> > > > +Rebasing (1/4)QRebasing (2/4)QRebasing (3/4)QRebasing (4/4)QQ                                                                                QSuccessfully rebased and updated refs/heads/missing-commit.
> > > >  EOF
> > >
> > > Yuck,
> >
> > Oh yeah...
> >
> > >... but I do not see how else/better this test can be written
> > > myself, which makes it a double-yuck X-<
> >
> > Perhaps hiding those spaces behind a helper variable e.g.
> > 'dump_term_clear_line=Q<80-spaces>Q' and embedding that in the here
> > docs specifying the expected output in these three tests could make it
> > ever so slightly less yuck...
> >
> > > Are we forcing out test to operate under dumb terminal mode and with
> > > a known number of columns?
> >
> > 'test-lib.sh' sets TERM=dumb relatively early on, and in these tests
> > we don't use 'test_terminal' to run 'git rebase', so...  yeah.  And
> > term_columns() defaults to 80.
> >
> > However, if the terminal were smart, then we would have to deal with
> > ANSI escape suddenly popping up...
> 
> And I fear that is *exactly* what makes
> https://dev.azure.com/gitgitgadget/git/_build/results?buildId=10539&view=ms.vss-test-web.build-test-results-tab
> fail...

Isn't it a sign of a problem in that Windows test environment that
it mistakenly believes that the terminal is smart, even though it has
been explicitly set to dumb?

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 2135 bytes --]

Hi,

On Wed, 12 Jun 2019, SZEDER Gábor wrote:

> On Wed, Jun 12, 2019 at 09:14:40PM +0200, Johannes Schindelin wrote:
> >
> > On Tue, 11 Jun 2019, SZEDER Gábor wrote:
> >
> > > On Tue, Jun 11, 2019 at 01:36:16PM -0700, Junio C Hamano wrote:
> > > > SZEDER Gábor <szeder.dev@gmail.com> writes:
> > > >
> > > > > -Rebasing (1/4)QRebasing (2/4)QRebasing (3/4)QRebasing (4/4)QSuccessfully rebased and updated refs/heads/missing-commit.
> > > > > +Rebasing (1/4)QRebasing (2/4)QRebasing (3/4)QRebasing (4/4)QQ                                                                                QSuccessfully rebased and updated refs/heads/missing-commit.
> > > > >  EOF
> > > >
> > > > Yuck,
> > >
> > > Oh yeah...
> > >
> > > >... but I do not see how else/better this test can be written
> > > > myself, which makes it a double-yuck X-<
> > >
> > > Perhaps hiding those spaces behind a helper variable e.g.
> > > 'dump_term_clear_line=Q<80-spaces>Q' and embedding that in the here
> > > docs specifying the expected output in these three tests could make it
> > > ever so slightly less yuck...
> > >
> > > > Are we forcing out test to operate under dumb terminal mode and with
> > > > a known number of columns?
> > >
> > > 'test-lib.sh' sets TERM=dumb relatively early on, and in these tests
> > > we don't use 'test_terminal' to run 'git rebase', so...  yeah.  And
> > > term_columns() defaults to 80.
> > >
> > > However, if the terminal were smart, then we would have to deal with
> > > ANSI escape suddenly popping up...
> >
> > And I fear that is *exactly* what makes
> > https://dev.azure.com/gitgitgadget/git/_build/results?buildId=10539&view=ms.vss-test-web.build-test-results-tab
> > fail...
>
> Isn't it a sign of a problem in that Windows test environment that
> it mistakenly believes that the terminal is smart, even though it has
> been explicitly set to dumb?

Yes, it looks like some odd decision there.

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

Ciao,
Dscho

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 6904 bytes --]

Hi,

On Wed, 12 Jun 2019, SZEDER Gábor wrote:

> On Wed, Jun 12, 2019 at 09:14:40PM +0200, Johannes Schindelin wrote:
>
> > On Tue, 11 Jun 2019, SZEDER Gábor wrote:
> >
> > > On Tue, Jun 11, 2019 at 01:36:16PM -0700, Junio C Hamano wrote:
> > > > SZEDER Gábor <szeder.dev@gmail.com> writes:
> > > >
> > > > > -Rebasing (1/4)QRebasing (2/4)QRebasing (3/4)QRebasing (4/4)QSuccessfully rebased and updated refs/heads/missing-commit.
> > > > > +Rebasing (1/4)QRebasing (2/4)QRebasing (3/4)QRebasing (4/4)QQ                                                                                QSuccessfully rebased and updated refs/heads/missing-commit.
> > > > >  EOF
> > > >
> > > > Yuck,
> > >
> > > Oh yeah...
> > >
> > > >... but I do not see how else/better this test can be written
> > > > myself, which makes it a double-yuck X-<
> > >
> > > Perhaps hiding those spaces behind a helper variable e.g.
> > > 'dump_term_clear_line=Q<80-spaces>Q' and embedding that in the here
> > > docs specifying the expected output in these three tests could make it
> > > ever so slightly less yuck...
> > >
> > > > Are we forcing out test to operate under dumb terminal mode and with
> > > > a known number of columns?
> > >
> > > 'test-lib.sh' sets TERM=dumb relatively early on, and in these tests
> > > we don't use 'test_terminal' to run 'git rebase', so...  yeah.  And
> > > term_columns() defaults to 80.
> > >
> > > However, if the terminal were smart, then we would have to deal with
> > > ANSI escape suddenly popping up...
> >
> > And I fear that is *exactly* what makes
> > https://dev.azure.com/gitgitgadget/git/_build/results?buildId=10539&view=ms.vss-test-web.build-test-results-tab
> > fail...
>
> Isn't it a sign of a problem in that Windows test environment that
> it mistakenly believes that the terminal is smart, even though it has
> been explicitly set to dumb?

I investigated this today.

Mind you, I still think that it is totally inappropriate for a test case
with the title 'rebase -i respects rebase.missingCommitsCheck = warn' to
validate the expected progress output, in particular since it verifies the
progress on non-sophisticated terminals, i.e. the totally least
interesting and least common scenario.

In short: I stand by my suggestion to fix these tests (i.e. ignore the
progress altogether) in a preparatory patch in your patch series.

The investigation why the test fails on Windows (due to the progress being
displayed for TERM=cygwin instead of TERM=dumb) took quite a bit longer
than I had originally anticipated, essentially because I did not expect to
uncover a bug that I introduced into Git for Windows v2.x apparently from
day one of the v2.x era.

In case you are interested in the details, please read on, otherwise just
mark this mail as read and move on.

Still with me? Well, here you go, enjoy the ride.

There are quite a few interesting bits about this bug, and I have to start
by stating that in DOS, there was no difference between empty values of
environment variables and unset environment variables. In other words,
there was no way to distinguish between the equivalent of `export x=` and
`unset x`. Back in the days, this was obviously perceived as reasonable,
and I kind of agree given my own difficulty to describe the problem
clearly.

Now, in the Win32 API there is a relatively easy way to distinguish
between those values: if the return value of `GetEnvironmentVariableW()`
(which indicates the length of the value) is 0 *and* `GetLastError()`
returns `ERROR_ENVVAR_NOT_FOUND`, then the environment variable is unset,
if it instead returns `ERROR_SUCCESS`, then it is set, and the value is
the empty string.

Side note: if you want to rely on this behavior, you will most likely want
to call `SetLastError(ERROR_SUCCESS)` before querying the environment, as
there seem to be conditions where the last error is not re-set to that
value even if the call succeeded.

Since Cygwin started really, really early in the history of Windows (even
supporting Windows 95 at some stage), it emulates the DOS behavior, not
the Win32 API behavior, and simply skips environment variables with empty
values when spawning non-Cygwin programs. In other words, it pretends that
they are unset instead.

Git for Windows' Bash (which runs the test suite) is an MSYS2 program, and
since MSYS2 is based on Cygwin, inherits this behavior, and since
`git.exe` is a non-MSYS2 program, there would be no way for the test suite
to set environment variables to the empty value and have Git respect that.

This broke t/t3301-notes.sh (because it sets GIT_NOTES_REF= and
GIT_NOTES_DISPLAY_REF= to override the configured settings), and therefore
I came up with this fix in February 2015:

https://github.com/git-for-windows/msys2-runtime/commit/c19199cc14ee

It tells the MSYS2 runtime to *keep* environment variables with empty
values.

Note: this fix was really made in order to let Git for Windows' test suite
pass, for no other reason. And it was not accepted by the MSYS2 project,
so this really only affects Git for Windows.

That fix seemed to work at the time (and maybe it really, really did), and
it seemed to work, still, until my investigation that took the better part
of today revealed that my fix was buggy. Under certain circumstances
(which I believe have to do with the environment variable referring to a
Unix-y path at some point, which is the case for `SHELL`), the subsequent
`getwinenv()` call mishandles empty values. It tries to convert them from
a Unix-y path (that looks like an absolute Unix path, but it really is
rooted in MSYS2's top-level directory, identified as the second-level
parent directory of `msys-2.0.dll`) to a Windows path, and failing that,
it replaces the `SHELL=` by a NUL character.

The `SHELL` thing here is important, as t/t3404-rebase-interactive.sh sets
this to the empty value explicitly:

https://github.com/git/git/blob/v2.22.0/t/t3404-rebase-interactive.sh#L63-L68

So instead of a `SHELL=\0` in the middle of the environment block, Git for
Windows' MSYS2 runtime inserts a `\0\0`. That, however, is the marker for
the end of the environment block, and as the environment has been sorted
before being converted in order to launch a non-MSYS2 program (in this
case, `git.exe`), the `TERM=dumb` setting is lost.

Even worse, for unrelated reasons, `git.exe` defaults to setting
`TERM=cygwin` if `TERM` is unset.

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

The fix with which I came up can be adored here:

https://github.com/git-for-windows/msys2-runtime/commit/c10b4185a35f

I tested this locally and will re-test as soon as a new MSYS2 runtime has
been deployed into Git for Windows' SDK.

Ciao,
Dscho

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

[SZEDER Gábor]

On Fri, Jun 14, 2019 at 08:42:29PM +0200, Johannes Schindelin wrote:

> > > > However, if the terminal were smart, then we would have to deal with
> > > > ANSI escape suddenly popping up...
> > >
> > > And I fear that is *exactly* what makes
> > > https://dev.azure.com/gitgitgadget/git/_build/results?buildId=10539&view=ms.vss-test-web.build-test-results-tab
> > > fail...
> >
> > Isn't it a sign of a problem in that Windows test environment that
> > it mistakenly believes that the terminal is smart, even though it has
> > been explicitly set to dumb?
> 
> I investigated this today.
> 
> Mind you, I still think that it is totally inappropriate for a test case
> with the title 'rebase -i respects rebase.missingCommitsCheck = warn' to
> validate the expected progress output, in particular since it verifies the
> progress on non-sophisticated terminals, i.e. the totally least
> interesting and least common scenario.
> 
> In short: I stand by my suggestion to fix these tests (i.e. ignore the
> progress altogether) in a preparatory patch in your patch series.

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

> In case you are interested in the details, please read on, otherwise just
> mark this mail as read and move on.
> 
> Still with me? Well, here you go, enjoy the ride.

Sure!

[...]

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

Thank you, it was both educational and entertaining :)

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

[SZEDER Gábor]

On Fri, Jun 14, 2019 at 08:42:29PM +0200, Johannes Schindelin wrote:

> The `SHELL` thing here is important, as t/t3404-rebase-interactive.sh sets
> this to the empty value explicitly:
> 
> https://github.com/git/git/blob/v2.22.0/t/t3404-rebase-interactive.sh#L63-L68

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

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

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

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

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

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

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

Though cd035b1cef is now close to 9 years old, plenty of time for
somebody to run into this issue in the wild and speak up...

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

[SZEDER Gábor]

[resend to Matthieu's current email address]

On Mon, Jun 17, 2019 at 09:13:28PM +0200, SZEDER Gábor wrote:
> On Fri, Jun 14, 2019 at 08:42:29PM +0200, Johannes Schindelin wrote:
> 
> > The `SHELL` thing here is important, as t/t3404-rebase-interactive.sh sets
> > this to the empty value explicitly:
> > 
> > https://github.com/git/git/blob/v2.22.0/t/t3404-rebase-interactive.sh#L63-L68
> 
> Hmm, hang on for a sec.  Why does it set SHELL to empty?
> 
> So t3404 sets SHELL to the empty value since cd035b1cef
> (rebase -i: add exec command to launch a shell command, 2010-08-10),
> and the in-test comment highlighted on the above link says:
> 
>   # "exec" commands are run with the user shell by default, but this may
>   # be non-POSIX. For example, if SHELL=zsh then ">file" doesn't work
>   # to create a file. Unsetting SHELL avoids such non-portable behavior
>   # in tests. It must be exported for it to take effect where needed.
>   SHELL=
> 
> Furthermore, the corresponding documentation added in cd035b1cef
> says the following:
> 
>   The "exec" command launches the command in a shell (the one specified
>   in `$SHELL`, or the default shell if `$SHELL` is not set)
> 
> IOW if SHELL is not set/empty, then it will run the default '/bin/sh',
> but who knows what it might be, there's no guarantee that it's a sane
> POSIX shell.  That's why we have "Define SHELL_PATH to a POSIX shell
> if your /bin/sh is broken." very near to the top of our Makefile.
> 
> So while setting SHELL to the empty value might indeed avoid
> non-portable behavior when the user in general prefers a non-POSIX
> shell, it wouldn't help if '/bin/sh' is broken.  For that it should be
> set to $SHELL_PATH (or perhaps $TEST_SHELL_PATH nowadays...).
> 
> Though cd035b1cef is now close to 9 years old, plenty of time for
> somebody to run into this issue in the wild and speak up...
> 

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

[SZEDER Gábor]

On Wed, Jun 12, 2019 at 09:14:40PM +0200, Johannes Schindelin wrote:
> Hi,
> 
> On Tue, 11 Jun 2019, SZEDER Gábor wrote:
> 
> > On Tue, Jun 11, 2019 at 01:36:16PM -0700, Junio C Hamano wrote:
> > > SZEDER Gábor <szeder.dev@gmail.com> writes:
> > >
> > > > -Rebasing (1/4)QRebasing (2/4)QRebasing (3/4)QRebasing (4/4)QSuccessfully rebased and updated refs/heads/missing-commit.
> > > > +Rebasing (1/4)QRebasing (2/4)QRebasing (3/4)QRebasing (4/4)QQ                                                                                QSuccessfully rebased and updated refs/heads/missing-commit.
> > > >  EOF
> > >
> > > Yuck,
> >
> > Oh yeah...
> >
> > >... but I do not see how else/better this test can be written
> > > myself, which makes it a double-yuck X-<
> >
> > Perhaps hiding those spaces behind a helper variable e.g.
> > 'dump_term_clear_line=Q<80-spaces>Q' and embedding that in the here
> > docs specifying the expected output in these three tests could make it
> > ever so slightly less yuck...
> >
> > > Are we forcing out test to operate under dumb terminal mode and with
> > > a known number of columns?
> >
> > 'test-lib.sh' sets TERM=dumb relatively early on, and in these tests
> > we don't use 'test_terminal' to run 'git rebase', so...  yeah.  And
> > term_columns() defaults to 80.
> >
> > However, if the terminal were smart, then we would have to deal with
> > ANSI escape suddenly popping up...
> 
> And I fear that is *exactly* what makes
> https://dev.azure.com/gitgitgadget/git/_build/results?buildId=10539&view=ms.vss-test-web.build-test-results-tab
> fail...
> 
> You cannot easily see it in that output (probably because of incorrectly
> encoded Escape sequences in the `.xml` output), so I'll paste what I see
> here, locally, when running `t3404-*.sh -i -V -x`:
> 
> -- snip --
> [...]
> ok 99 - rebase -i respects rebase.missingCommitsCheck = ignore
> 
> expecting success:
>         test_config rebase.missingCommitsCheck warn &&
>         rebase_setup_and_clean missing-commit &&
>         set_fake_editor &&
>         FAKE_LINES="1 2 3 4" \
>                 git rebase -i --root 2>actual &&
>         test_i18ncmp expect actual &&
>         test D = $(git cat-file commit HEAD | sed -ne \$p)

[...]

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

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

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

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

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

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


[1] https://public-inbox.org/git/20190624181318.17388-3-szeder.dev@gmail.com/T/#u

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

[Phillip Wood]

Hi dscho and Gábor

On 24/06/2019 19:39, SZEDER Gábor wrote:
> On Wed, Jun 12, 2019 at 09:14:40PM +0200, Johannes Schindelin wrote:
>> Hi,
>>
>> On Tue, 11 Jun 2019, SZEDER Gábor wrote:
>>
>>> On Tue, Jun 11, 2019 at 01:36:16PM -0700, Junio C Hamano wrote:
>>>> SZEDER Gábor <szeder.dev@gmail.com> writes:
>>>>
>>>>> -Rebasing (1/4)QRebasing (2/4)QRebasing (3/4)QRebasing (4/4)QSuccessfully rebased and updated refs/heads/missing-commit.
>>>>> +Rebasing (1/4)QRebasing (2/4)QRebasing (3/4)QRebasing (4/4)QQ                                                                                QSuccessfully rebased and updated refs/heads/missing-commit.
>>>>>  EOF
>>>>
>>>> Yuck,
>>>
>>> Oh yeah...
>>>
>>>> ... but I do not see how else/better this test can be written
>>>> myself, which makes it a double-yuck X-<
>>>
>>> Perhaps hiding those spaces behind a helper variable e.g.
>>> 'dump_term_clear_line=Q<80-spaces>Q' and embedding that in the here
>>> docs specifying the expected output in these three tests could make it
>>> ever so slightly less yuck...
>>>
>>>> Are we forcing out test to operate under dumb terminal mode and with
>>>> a known number of columns?
>>>
>>> 'test-lib.sh' sets TERM=dumb relatively early on, and in these tests
>>> we don't use 'test_terminal' to run 'git rebase', so...  yeah.  And
>>> term_columns() defaults to 80.
>>>
>>> However, if the terminal were smart, then we would have to deal with
>>> ANSI escape suddenly popping up...
>>
>> And I fear that is *exactly* what makes
>> https://dev.azure.com/gitgitgadget/git/_build/results?buildId=10539&view=ms.vss-test-web.build-test-results-tab
>> fail...
>>
>> You cannot easily see it in that output (probably because of incorrectly
>> encoded Escape sequences in the `.xml` output), so I'll paste what I see
>> here, locally, when running `t3404-*.sh -i -V -x`:
>>
>> -- snip --
>> [...]
>> ok 99 - rebase -i respects rebase.missingCommitsCheck = ignore
>>
>> expecting success:
>>         test_config rebase.missingCommitsCheck warn &&
>>         rebase_setup_and_clean missing-commit &&
>>         set_fake_editor &&
>>         FAKE_LINES="1 2 3 4" \
>>                 git rebase -i --root 2>actual &&
>>         test_i18ncmp expect actual &&
>>         test D = $(git cat-file commit HEAD | sed -ne \$p)
> 
> [...]
> 
>> ++ test_cmp expect actual
>> ++ GIT_ALLOC_LIMIT=0
>> ++ test-tool cmp expect actual
>> diff --git a/expect b/actual
>> index 05fcfcb..9555e34 100644
>> --- a/expect
>> +++ b/actual
>> @@ -6,4 +6,4 @@ To avoid this message, use "drop" to explicitly remove a commit.
>>  Use 'git config rebase.missingCommitsCheck' to change the level of warnings.
>>  The possible behaviours are: ignore, warn, error.
>>
>> -Rebasing (1/4)^MRebasing (2/4)^MRebasing (3/4)^MRebasing (4/4)^M                                                                                ^MSuccessfully rebased and updated refs/heads/missing-commit.
>> +Rebasing (1/4)^MRebasing (2/4)^MRebasing (3/4)^MRebasing (4/4)^MESC[KSuccessfully rebased and updated refs/heads/missing-commit.
>> error: last command exited with $?=1
>> not ok 100 - rebase -i respects rebase.missingCommitsCheck = warn
>> #
>> #               test_config rebase.missingCommitsCheck warn &&
>> #               rebase_setup_and_clean missing-commit &&
>> #               set_fake_editor &&
>> #               FAKE_LINES="1 2 3 4" \
>> #                       git rebase -i --root 2>actual &&
>> #               test_i18ncmp expect actual &&
>> #               test D = $(git cat-file commit HEAD | sed -ne \$p)
>> #
>> -- snap --
>>
>> (I copy-pasted this from the output of `less` so that the control sequences can be seen.)
>>
>> To be utterly honest, I really fail to see a reason why a test case that
>> purports to verify that `git rebase -i` respects
>> `rebase.missingCommitsCheck=warn` should fail when the progress is shown in an
>> unexpected format.
>>
>> It strikes me as yet another poorly written test case that fails to catch the
>> intended regressions, instead it catches a bug *in the test case itself* when
>> legitimate changes are made to the progress code.
>>
>> Nothing in a test suite is worse than a test that fails (or succeeds) for the
>> wrong reasons.
>>
>> To make things even worse, the code that generates that `expect` file is
>> outside the test case.
>>
>> Here it is, in its full "glory":
>>
>> -- snip --
>> q_to_cr >expect <<EOF
>> Warning: some commits may have been dropped accidentally.
>> Dropped commits (newer to older):
>>  - $(git rev-list --pretty=oneline --abbrev-commit -1 master)
>> To avoid this message, use "drop" to explicitly remove a commit.
>>
>> Use 'git config rebase.missingCommitsCheck' to change the level of warnings.
>> The possible behaviours are: ignore, warn, error.
>>
>> Rebasing (1/4)QRebasing (2/4)QRebasing (3/4)QRebasing (4/4)QQ                                                                                QSuccessfully rebased and updated refs/heads/missing-commit.
>> EOF
>> -- snap --
>>
>> May I please *strongly* suggest to fix this first? It should
>>
>> - completely lose that last line,
>> - be inserted into the test case itself so that e.g. disk full problems are
>>   caught and logged properly, and
>> - the `test_i18ncmp expect actual` call in the test case should be replaced
>>   by something like:
>>
>> 	sed "\$d" <actual >actual-skip-progress &&
>> 	test_i18ncmp expect actual-skip-progress
>>
>> This should obviously be made as a separate, introductory patch (probably with
>> a less scathing commit message than my comments above would suggest).
>>
>> And that would also remove the double-yuck.
> 
> Unfortunately, this addresses only one of the "Yuck"s; see v3 of this
> patch series [1].
> 
> The other yucks affect the following four tests in
> 't3420-rebase-autostash.sh':
> 
>   16 - rebase --merge --autostash: check output
>   23 - rebase --merge: check output with conflicting stash
>   26 - rebase --interactive --autostash: check output
>   33 - rebase --interactive: check output with conflicting stash
> 
> These tests come from commits b76aeae553 (rebase: add regression tests
> for console output, 2017-06-19) and 7d70e6b902 (rebase: add more
> regression tests for console output, 2017-06-19), and are specifically
> about checking the (whole) console output of 'git rebase', so I left
> the updates to them as they were.
> 
> In any case, Cc-ing Phillip to discuss whether something could be done
> about them (now perhaps preferably (for me :) as a follow-up, and not
> another preparatory patches).

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

Best Wishes

Phillip

> 
> 
> [1] https://public-inbox.org/git/20190624181318.17388-3-szeder.dev@gmail.com/T/#u
> 

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

[SZEDER Gábor]

On Tue, Jun 25, 2019 at 11:08:04AM +0100, Phillip Wood wrote:
> >> Rebasing (1/4)QRebasing (2/4)QRebasing (3/4)QRebasing (4/4)QQ                                                                                QSuccessfully rebased and updated refs/heads/missing-commit.


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

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

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

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

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

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

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

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

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

[Phillip Wood]

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

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

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


Thanks for the pointer, I'll have a read

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

Ah, thanks for explaining

Best Wishes

Phillip

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

[Phillip Wood]

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

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

Best Wishes

Phillip

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1914 bytes --]

Hi Phillip,

On Tue, 25 Jun 2019, Phillip Wood wrote:

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

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

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

Ciao,
Dscho

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

[Phillip Wood]

Hi Dscho

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

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

Best Wishes

Phillip

> 
> Ciao,
> Dscho
> 

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

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

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

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

Is it because it is so obvious to potential readers?

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

[SZEDER Gábor]

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

In verbose mode there is no progress output that is overwritten and
might need to be covered up.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

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

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

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

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

I do not think it is so obvious, though.

[PATCH v2 4/4] progress: use term_clear_line()

[SZEDER Gábor]

To make sure that the previously displayed progress line is completely
covered up when the new line is shorter, commit 545dc345eb (progress:
break too long progress bar lines, 2019-04-12) added a bunch of
calculations to figure out how many characters it needs to overwrite
with spaces.

Use the just introduced term_clear_line() helper function to, well,
clear the last line, making all these calculations unnecessary, and
thus simplifying the code considerably.

Three tests in 't5541-http-push-smart.sh' 'grep' for specific text
shown in the progress lines at the beginning of the line, but now
those lines begin either with the ANSI escape sequence or with the
terminal width worth of space characters clearing the line.  Relax the
'grep' patterns to match anywhere on the line.  Note that only two of
these three tests fail without relaxing their 'grep' pattern, but the
third looks for the absence of the pattern, so it still succeeds, but
without the adjustment would potentially hide future regressions.

Note also that with this change we no longer need the length of the
previously displayed progress line, so the strbuf added to 'struct
progress' in d53ba841d4 (progress: assemble percentage and counters in
a strbuf before printing, 2019-04-05) is not strictly necessary
anymore.  We still keep it, though, as it avoids allocating and
releasing a strbuf each time the progress is updated.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 progress.c                 | 28 +++++++++++-----------------
 t/t5541-http-push-smart.sh |  6 +++---
 2 files changed, 14 insertions(+), 20 deletions(-)

diff --git a/progress.c b/progress.c
index a2e8cf64a8..095dcd0ddf 100644
--- a/progress.c
+++ b/progress.c
@@ -88,7 +88,6 @@ static void display(struct progress *progress, uint64_t n, const char *done)
 	const char *tp;
 	struct strbuf *counters_sb = &progress->counters_sb;
 	int show_update = 0;
-	int last_count_len = counters_sb->len;
 
 	if (progress->delay && (!progress_update || --progress->delay))
 		return;
@@ -116,26 +115,21 @@ static void display(struct progress *progress, uint64_t n, const char *done)
 	if (show_update) {
 		if (is_foreground_fd(fileno(stderr)) || done) {
 			const char *eol = done ? done : "\r";
-			size_t clear_len = counters_sb->len < last_count_len ?
-					last_count_len - counters_sb->len + 1 :
-					0;
-			size_t progress_line_len = progress->title_len +
-						counters_sb->len + 2;
-			int cols = term_columns();
 
+			term_clear_line();
 			if (progress->split) {
-				fprintf(stderr, "  %s%*s", counters_sb->buf,
-					(int) clear_len, eol);
-			} else if (!done && cols < progress_line_len) {
-				clear_len = progress->title_len + 1 < cols ?
-					    cols - progress->title_len - 1 : 0;
-				fprintf(stderr, "%s:%*s\n  %s%s",
-					progress->title, (int) clear_len, "",
-					counters_sb->buf, eol);
+				fprintf(stderr, "  %s%s", counters_sb->buf,
+					eol);
+			} else if (!done &&
+				   /* The "+ 2" accounts for the ": ". */
+				   term_columns() < progress->title_len +
+						    counters_sb->len + 2) {
+				fprintf(stderr, "%s:\n  %s%s",
+					progress->title, counters_sb->buf, eol);
 				progress->split = 1;
 			} else {
-				fprintf(stderr, "%s: %s%*s", progress->title,
-					counters_sb->buf, (int) clear_len, eol);
+				fprintf(stderr, "%s: %s%s", progress->title,
+					counters_sb->buf, eol);
 			}
 			fflush(stderr);
 		}
diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh
index 8ef8763e06..2e4802e206 100755
--- a/t/t5541-http-push-smart.sh
+++ b/t/t5541-http-push-smart.sh
@@ -213,7 +213,7 @@ test_expect_success TTY 'push shows progress when stderr is a tty' '
 	cd "$ROOT_PATH"/test_repo_clone &&
 	test_commit noisy &&
 	test_terminal git push >output 2>&1 &&
-	test_i18ngrep "^Writing objects" output
+	test_i18ngrep "Writing objects" output
 '
 
 test_expect_success TTY 'push --quiet silences status and progress' '
@@ -228,7 +228,7 @@ test_expect_success TTY 'push --no-progress silences progress but not status' '
 	test_commit no-progress &&
 	test_terminal git push --no-progress >output 2>&1 &&
 	test_i18ngrep "^To http" output &&
-	test_i18ngrep ! "^Writing objects" output
+	test_i18ngrep ! "Writing objects" output
 '
 
 test_expect_success 'push --progress shows progress to non-tty' '
@@ -236,7 +236,7 @@ test_expect_success 'push --progress shows progress to non-tty' '
 	test_commit progress &&
 	git push --progress >output 2>&1 &&
 	test_i18ngrep "^To http" output &&
-	test_i18ngrep "^Writing objects" output
+	test_i18ngrep "Writing objects" output
 '
 
 test_expect_success 'http push gives sane defaults to reflog' '
-- 
2.22.0.566.g58873a45ff

Re: [PATCH v2 4/4] progress: use term_clear_line()

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1766 bytes --]

Hi,

On Tue, 11 Jun 2019, SZEDER Gábor wrote:

> diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh
> index 8ef8763e06..2e4802e206 100755
> --- a/t/t5541-http-push-smart.sh
> +++ b/t/t5541-http-push-smart.sh
> @@ -213,7 +213,7 @@ test_expect_success TTY 'push shows progress when stderr is a tty' '
>  	cd "$ROOT_PATH"/test_repo_clone &&
>  	test_commit noisy &&
>  	test_terminal git push >output 2>&1 &&
> -	test_i18ngrep "^Writing objects" output
> +	test_i18ngrep "Writing objects" output

I guess the problem is that now the "Writing objects" is preceded by a CR
instead of a NL? If so, maybe the `cr_to_nl` function could be introduced
into `test-lib-functions.sh` and be used here and in the subsequent test
cases?

(Just an idea, as you seemed unhappy about these adjustments according to
the cover letter.)

Ciao,
Dscho

>  '
>
>  test_expect_success TTY 'push --quiet silences status and progress' '
> @@ -228,7 +228,7 @@ test_expect_success TTY 'push --no-progress silences progress but not status' '
>  	test_commit no-progress &&
>  	test_terminal git push --no-progress >output 2>&1 &&
>  	test_i18ngrep "^To http" output &&
> -	test_i18ngrep ! "^Writing objects" output
> +	test_i18ngrep ! "Writing objects" output
>  '
>
>  test_expect_success 'push --progress shows progress to non-tty' '
> @@ -236,7 +236,7 @@ test_expect_success 'push --progress shows progress to non-tty' '
>  	test_commit progress &&
>  	git push --progress >output 2>&1 &&
>  	test_i18ngrep "^To http" output &&
> -	test_i18ngrep "^Writing objects" output
> +	test_i18ngrep "Writing objects" output
>  '
>
>  test_expect_success 'http push gives sane defaults to reflog' '
> --
> 2.22.0.566.g58873a45ff
>
>

[PATCH v3 0/5] rebase/progress: add and use term_clear_line()

[SZEDER Gábor]

This series fixes some garbled progress display in 'git rebase' and
simplifies clearing up the progress display in general.

Only slight updates since the previous version to remove one of the
"Yuck"s by making the offending test in t3404 more focused, making
the first patch of the previous version pointless.  There is also a 
a preparatory cleanup patch 't3404: modernize here doc style'.


v2: https://public-inbox.org/git/20190611130320.18499-1-szeder.dev@gmail.com/T/#u


SZEDER Gábor (5):
  t3404: modernize here doc style
  t3404: make the 'rebase.missingCommitsCheck=ignore' test more focused
  pager: add a helper function to clear the last line in the terminal
  rebase: fix garbled progress display with '-x'
  progress: use term_clear_line()

 cache.h                       |   1 +
 editor.c                      |   6 +-
 pager.c                       |  20 ++++++
 progress.c                    |  28 ++++----
 sequencer.c                   |  17 ++++-
 t/t3404-rebase-interactive.sh | 120 ++++++++++++++--------------------
 t/t3420-rebase-autostash.sh   |   4 +-
 t/t5541-http-push-smart.sh    |   6 +-
 8 files changed, 104 insertions(+), 98 deletions(-)

Range-diff:
1:  b392128604 < -:  ---------- t3404-rebase-interactive: use the 'q_to_cr' helper
-:  ---------- > 1:  4372a3cde4 t3404: modernize here doc style
-:  ---------- > 2:  5444f547c5 t3404: make the 'rebase.missingCommitsCheck=ignore' test more focused
2:  b48f49c114 = 3:  5ebf218cb9 pager: add a helper function to clear the last line in the terminal
3:  b25363c7c3 ! 4:  51cd5ccd46 rebase: fix garbled progress display with '-x'
    @@ -39,13 +39,13 @@
         "Stopped at <abbrev-oid>... <subject>" for the 'edit' or 'break'
         commands, or the "Successfully rebased and updated <full-ref>." at the
         very end.  These are so long that they practically always overwrite
    -    that "Rebasing (n/m)" progress line, but let's be prudent, and clear
    +    that "Rebasing (N/M)" progress line, but let's be prudent, and clear
         the last line before printing these, too.
     
    -    Three tests, one in 't3404-rebase-interactive.sh' and two in
    -    't3420-rebase-autostash.sh' check the full output of 'git rebase' and
    -    thus are affected by this change, so adjust their expectations to
    -    account for the new line clearing.
    +    In 't3420-rebase-autostash.sh' two helper functions prepare the
    +    expected output of four tests that check the full output of 'git
    +    rebase' and thus are affected by this change, so adjust their
    +    expectations to account for the new line clearing.
     
         Note that this patch doesn't completely eliminate the possibility of
         similar garbled outputs, e.g. some error messages from rebase or the
    @@ -116,19 +116,6 @@
      		strbuf_release(&buf);
      		strbuf_release(&head_ref);
     
    - diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
    - --- a/t/t3404-rebase-interactive.sh
    - +++ b/t/t3404-rebase-interactive.sh
    -@@
    - Use 'git config rebase.missingCommitsCheck' to change the level of warnings.
    - The possible behaviours are: ignore, warn, error.
    - 
    --Rebasing (1/4)QRebasing (2/4)QRebasing (3/4)QRebasing (4/4)QSuccessfully rebased and updated refs/heads/missing-commit.
    -+Rebasing (1/4)QRebasing (2/4)QRebasing (3/4)QRebasing (4/4)QQ                                                                                QSuccessfully rebased and updated refs/heads/missing-commit.
    - EOF
    - 
    - test_expect_success 'rebase -i respects rebase.missingCommitsCheck = warn' '
    -
      diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
      --- a/t/t3420-rebase-autostash.sh
      +++ b/t/t3420-rebase-autostash.sh
4:  ec85b386fd = 5:  09642a458e progress: use term_clear_line()
-- 
2.22.0.589.g5bd7971b91

[PATCH v3 1/5] t3404: modernize here doc style

[SZEDER Gábor]

In 't3404-rebase-interactive.sh' the expected output of several tests
is prepared from here documents, which are outside of
'test_expect_success' blocks and have spaces around redirection
operators.

Move these here documents into the corresponding 'test_expect_success'
block and avoid spaces between filename and redition operators.
Furthermore, quote the here docs' delimiter word to prevent parameter
expansions and what not, where applicable.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t3404-rebase-interactive.sh | 123 ++++++++++++++++------------------
 1 file changed, 58 insertions(+), 65 deletions(-)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 1723e1a858..9146f9d47b 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -75,11 +75,10 @@ test_expect_success 'rebase --keep-empty' '
 	test_line_count = 6 actual
 '
 
-cat > expect <<EOF
-error: nothing to do
-EOF
-
 test_expect_success 'rebase -i with empty HEAD' '
+	cat >expect <<-\EOF &&
+	error: nothing to do
+	EOF
 	set_fake_editor &&
 	test_must_fail env FAKE_LINES="1 exec_true" git rebase -i HEAD^ >actual 2>&1 &&
 	test_i18ncmp expect actual
@@ -237,25 +236,23 @@ test_expect_success 'exchange two commits' '
 	test G = $(git cat-file commit HEAD | sed -ne \$p)
 '
 
-cat > expect << EOF
-diff --git a/file1 b/file1
-index f70f10e..fd79235 100644
---- a/file1
-+++ b/file1
-@@ -1 +1 @@
--A
-+G
-EOF
-
-cat > expect2 << EOF
-<<<<<<< HEAD
-D
-=======
-G
->>>>>>> 5d18e54... G
-EOF
-
 test_expect_success 'stop on conflicting pick' '
+	cat >expect <<-\EOF &&
+	diff --git a/file1 b/file1
+	index f70f10e..fd79235 100644
+	--- a/file1
+	+++ b/file1
+	@@ -1 +1 @@
+	-A
+	+G
+	EOF
+	cat >expect2 <<-\EOF &&
+	<<<<<<< HEAD
+	D
+	=======
+	G
+	>>>>>>> 5d18e54... G
+	EOF
 	git tag new-branch1 &&
 	set_fake_editor &&
 	test_must_fail git rebase -i master &&
@@ -495,15 +492,14 @@ test_expect_success 'commit message retained after conflict' '
 	git branch -D conflict-squash
 '
 
-cat > expect-squash-fixup << EOF
-B
-
-D
+test_expect_success C_LOCALE_OUTPUT 'squash and fixup generate correct log messages' '
+	cat >expect-squash-fixup <<-\EOF &&
+	B
 
-ONCE
-EOF
+	D
 
-test_expect_success C_LOCALE_OUTPUT 'squash and fixup generate correct log messages' '
+	ONCE
+	EOF
 	git checkout -b squash-fixup E &&
 	base=$(git rev-parse HEAD~4) &&
 	set_fake_editor &&
@@ -799,13 +795,12 @@ test_expect_success 'rebase -i can copy notes' '
 	test "a note" = "$(git notes show HEAD)"
 '
 
-cat >expect <<EOF
-an earlier note
-
-a note
-EOF
-
 test_expect_success 'rebase -i can copy notes over a fixup' '
+	cat >expect <<-\EOF &&
+	an earlier note
+
+	a note
+	EOF
 	git reset --hard n3 &&
 	git notes add -m"an earlier note" n2 &&
 	set_fake_editor &&
@@ -1304,27 +1299,26 @@ test_expect_success 'rebase -i respects rebase.missingCommitsCheck = ignore' '
 		actual
 '
 
-cat >expect <<EOF
-Warning: some commits may have been dropped accidentally.
-Dropped commits (newer to older):
- - $(git rev-list --pretty=oneline --abbrev-commit -1 master)
-To avoid this message, use "drop" to explicitly remove a commit.
-
-Use 'git config rebase.missingCommitsCheck' to change the level of warnings.
-The possible behaviours are: ignore, warn, error.
-
-Rebasing (1/4)
-Rebasing (2/4)
-Rebasing (3/4)
-Rebasing (4/4)
-Successfully rebased and updated refs/heads/missing-commit.
-EOF
-
 cr_to_nl () {
 	tr '\015' '\012'
 }
 
 test_expect_success 'rebase -i respects rebase.missingCommitsCheck = warn' '
+	cat >expect <<-EOF &&
+	Warning: some commits may have been dropped accidentally.
+	Dropped commits (newer to older):
+	 - $(git rev-list --pretty=oneline --abbrev-commit -1 master)
+	To avoid this message, use "drop" to explicitly remove a commit.
+
+	Use '\''git config rebase.missingCommitsCheck'\'' to change the level of warnings.
+	The possible behaviours are: ignore, warn, error.
+
+	Rebasing (1/4)
+	Rebasing (2/4)
+	Rebasing (3/4)
+	Rebasing (4/4)
+	Successfully rebased and updated refs/heads/missing-commit.
+	EOF
 	test_config rebase.missingCommitsCheck warn &&
 	rebase_setup_and_clean missing-commit &&
 	set_fake_editor &&
@@ -1335,21 +1329,20 @@ test_expect_success 'rebase -i respects rebase.missingCommitsCheck = warn' '
 	test D = $(git cat-file commit HEAD | sed -ne \$p)
 '
 
-cat >expect <<EOF
-Warning: some commits may have been dropped accidentally.
-Dropped commits (newer to older):
- - $(git rev-list --pretty=oneline --abbrev-commit -1 master)
- - $(git rev-list --pretty=oneline --abbrev-commit -1 master~2)
-To avoid this message, use "drop" to explicitly remove a commit.
-
-Use 'git config rebase.missingCommitsCheck' to change the level of warnings.
-The possible behaviours are: ignore, warn, error.
-
-You can fix this with 'git rebase --edit-todo' and then run 'git rebase --continue'.
-Or you can abort the rebase with 'git rebase --abort'.
-EOF
-
 test_expect_success 'rebase -i respects rebase.missingCommitsCheck = error' '
+	cat >expect <<-EOF &&
+	Warning: some commits may have been dropped accidentally.
+	Dropped commits (newer to older):
+	 - $(git rev-list --pretty=oneline --abbrev-commit -1 master)
+	 - $(git rev-list --pretty=oneline --abbrev-commit -1 master~2)
+	To avoid this message, use "drop" to explicitly remove a commit.
+
+	Use '\''git config rebase.missingCommitsCheck'\'' to change the level of warnings.
+	The possible behaviours are: ignore, warn, error.
+
+	You can fix this with '\''git rebase --edit-todo'\'' and then run '\''git rebase --continue'\''.
+	Or you can abort the rebase with '\''git rebase --abort'\''.
+	EOF
 	test_config rebase.missingCommitsCheck error &&
 	rebase_setup_and_clean missing-commit &&
 	set_fake_editor &&
-- 
2.22.0.589.g5bd7971b91

Re: [PATCH v3 1/5] t3404: modernize here doc style

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 6639 bytes --]

Hi Gábor,

On Mon, 24 Jun 2019, SZEDER Gábor wrote:

> In 't3404-rebase-interactive.sh' the expected output of several tests
> is prepared from here documents, which are outside of
> 'test_expect_success' blocks and have spaces around redirection
> operators.
>
> Move these here documents into the corresponding 'test_expect_success'
> block and avoid spaces between filename and redition operators.
> Furthermore, quote the here docs' delimiter word to prevent parameter
> expansions and what not, where applicable.

Makes sense. I did not really look at it in detail (it's a bit tough to
read, and `--patience` would have made it only _slightly_ easier),
trusting that you used `git diff -w --color-moved` to verify the moves.

Thanks,
Dscho

>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
>  t/t3404-rebase-interactive.sh | 123 ++++++++++++++++------------------
>  1 file changed, 58 insertions(+), 65 deletions(-)
>
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 1723e1a858..9146f9d47b 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -75,11 +75,10 @@ test_expect_success 'rebase --keep-empty' '
>  	test_line_count = 6 actual
>  '
>
> -cat > expect <<EOF
> -error: nothing to do
> -EOF
> -
>  test_expect_success 'rebase -i with empty HEAD' '
> +	cat >expect <<-\EOF &&
> +	error: nothing to do
> +	EOF
>  	set_fake_editor &&
>  	test_must_fail env FAKE_LINES="1 exec_true" git rebase -i HEAD^ >actual 2>&1 &&
>  	test_i18ncmp expect actual
> @@ -237,25 +236,23 @@ test_expect_success 'exchange two commits' '
>  	test G = $(git cat-file commit HEAD | sed -ne \$p)
>  '
>
> -cat > expect << EOF
> -diff --git a/file1 b/file1
> -index f70f10e..fd79235 100644
> ---- a/file1
> -+++ b/file1
> -@@ -1 +1 @@
> --A
> -+G
> -EOF
> -
> -cat > expect2 << EOF
> -<<<<<<< HEAD
> -D
> -=======
> -G
> ->>>>>>> 5d18e54... G
> -EOF
> -
>  test_expect_success 'stop on conflicting pick' '
> +	cat >expect <<-\EOF &&
> +	diff --git a/file1 b/file1
> +	index f70f10e..fd79235 100644
> +	--- a/file1
> +	+++ b/file1
> +	@@ -1 +1 @@
> +	-A
> +	+G
> +	EOF
> +	cat >expect2 <<-\EOF &&
> +	<<<<<<< HEAD
> +	D
> +	=======
> +	G
> +	>>>>>>> 5d18e54... G
> +	EOF
>  	git tag new-branch1 &&
>  	set_fake_editor &&
>  	test_must_fail git rebase -i master &&
> @@ -495,15 +492,14 @@ test_expect_success 'commit message retained after conflict' '
>  	git branch -D conflict-squash
>  '
>
> -cat > expect-squash-fixup << EOF
> -B
> -
> -D
> +test_expect_success C_LOCALE_OUTPUT 'squash and fixup generate correct log messages' '
> +	cat >expect-squash-fixup <<-\EOF &&
> +	B
>
> -ONCE
> -EOF
> +	D
>
> -test_expect_success C_LOCALE_OUTPUT 'squash and fixup generate correct log messages' '
> +	ONCE
> +	EOF
>  	git checkout -b squash-fixup E &&
>  	base=$(git rev-parse HEAD~4) &&
>  	set_fake_editor &&
> @@ -799,13 +795,12 @@ test_expect_success 'rebase -i can copy notes' '
>  	test "a note" = "$(git notes show HEAD)"
>  '
>
> -cat >expect <<EOF
> -an earlier note
> -
> -a note
> -EOF
> -
>  test_expect_success 'rebase -i can copy notes over a fixup' '
> +	cat >expect <<-\EOF &&
> +	an earlier note
> +
> +	a note
> +	EOF
>  	git reset --hard n3 &&
>  	git notes add -m"an earlier note" n2 &&
>  	set_fake_editor &&
> @@ -1304,27 +1299,26 @@ test_expect_success 'rebase -i respects rebase.missingCommitsCheck = ignore' '
>  		actual
>  '
>
> -cat >expect <<EOF
> -Warning: some commits may have been dropped accidentally.
> -Dropped commits (newer to older):
> - - $(git rev-list --pretty=oneline --abbrev-commit -1 master)
> -To avoid this message, use "drop" to explicitly remove a commit.
> -
> -Use 'git config rebase.missingCommitsCheck' to change the level of warnings.
> -The possible behaviours are: ignore, warn, error.
> -
> -Rebasing (1/4)
> -Rebasing (2/4)
> -Rebasing (3/4)
> -Rebasing (4/4)
> -Successfully rebased and updated refs/heads/missing-commit.
> -EOF
> -
>  cr_to_nl () {
>  	tr '\015' '\012'
>  }
>
>  test_expect_success 'rebase -i respects rebase.missingCommitsCheck = warn' '
> +	cat >expect <<-EOF &&
> +	Warning: some commits may have been dropped accidentally.
> +	Dropped commits (newer to older):
> +	 - $(git rev-list --pretty=oneline --abbrev-commit -1 master)
> +	To avoid this message, use "drop" to explicitly remove a commit.
> +
> +	Use '\''git config rebase.missingCommitsCheck'\'' to change the level of warnings.
> +	The possible behaviours are: ignore, warn, error.
> +
> +	Rebasing (1/4)
> +	Rebasing (2/4)
> +	Rebasing (3/4)
> +	Rebasing (4/4)
> +	Successfully rebased and updated refs/heads/missing-commit.
> +	EOF
>  	test_config rebase.missingCommitsCheck warn &&
>  	rebase_setup_and_clean missing-commit &&
>  	set_fake_editor &&
> @@ -1335,21 +1329,20 @@ test_expect_success 'rebase -i respects rebase.missingCommitsCheck = warn' '
>  	test D = $(git cat-file commit HEAD | sed -ne \$p)
>  '
>
> -cat >expect <<EOF
> -Warning: some commits may have been dropped accidentally.
> -Dropped commits (newer to older):
> - - $(git rev-list --pretty=oneline --abbrev-commit -1 master)
> - - $(git rev-list --pretty=oneline --abbrev-commit -1 master~2)
> -To avoid this message, use "drop" to explicitly remove a commit.
> -
> -Use 'git config rebase.missingCommitsCheck' to change the level of warnings.
> -The possible behaviours are: ignore, warn, error.
> -
> -You can fix this with 'git rebase --edit-todo' and then run 'git rebase --continue'.
> -Or you can abort the rebase with 'git rebase --abort'.
> -EOF
> -
>  test_expect_success 'rebase -i respects rebase.missingCommitsCheck = error' '
> +	cat >expect <<-EOF &&
> +	Warning: some commits may have been dropped accidentally.
> +	Dropped commits (newer to older):
> +	 - $(git rev-list --pretty=oneline --abbrev-commit -1 master)
> +	 - $(git rev-list --pretty=oneline --abbrev-commit -1 master~2)
> +	To avoid this message, use "drop" to explicitly remove a commit.
> +
> +	Use '\''git config rebase.missingCommitsCheck'\'' to change the level of warnings.
> +	The possible behaviours are: ignore, warn, error.
> +
> +	You can fix this with '\''git rebase --edit-todo'\'' and then run '\''git rebase --continue'\''.
> +	Or you can abort the rebase with '\''git rebase --abort'\''.
> +	EOF
>  	test_config rebase.missingCommitsCheck error &&
>  	rebase_setup_and_clean missing-commit &&
>  	set_fake_editor &&
> --
> 2.22.0.589.g5bd7971b91
>
>

[PATCH v3 2/5] t3404: make the 'rebase.missingCommitsCheck=ignore' test more focused

[SZEDER Gábor]

The test 'rebase -i respects rebase.missingCommitsCheck = warn' is
mainly interested in the warning about the dropped commits, but it
checks the whole output of 'git rebase', including progress lines and
what not that are not at all relevant to 'rebase.missingCommitsCheck',
but make it necessary to update this test whenever e.g. the way we
show progress is updated (as it will happen in one of the later
patches of this series).

Modify the test to verify only the first four lines of 'git rebase's
output that contain all the important lines, notably the line
containing the "Warning:" itself and the oneline log of the dropped
commit.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t3404-rebase-interactive.sh | 15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 9146f9d47b..0b8267c97c 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1299,32 +1299,19 @@ test_expect_success 'rebase -i respects rebase.missingCommitsCheck = ignore' '
 		actual
 '
 
-cr_to_nl () {
-	tr '\015' '\012'
-}
-
 test_expect_success 'rebase -i respects rebase.missingCommitsCheck = warn' '
 	cat >expect <<-EOF &&
 	Warning: some commits may have been dropped accidentally.
 	Dropped commits (newer to older):
 	 - $(git rev-list --pretty=oneline --abbrev-commit -1 master)
 	To avoid this message, use "drop" to explicitly remove a commit.
-
-	Use '\''git config rebase.missingCommitsCheck'\'' to change the level of warnings.
-	The possible behaviours are: ignore, warn, error.
-
-	Rebasing (1/4)
-	Rebasing (2/4)
-	Rebasing (3/4)
-	Rebasing (4/4)
-	Successfully rebased and updated refs/heads/missing-commit.
 	EOF
 	test_config rebase.missingCommitsCheck warn &&
 	rebase_setup_and_clean missing-commit &&
 	set_fake_editor &&
 	FAKE_LINES="1 2 3 4" \
 		git rebase -i --root 2>actual.2 &&
-	cr_to_nl <actual.2 >actual &&
+	head -n4 actual.2 >actual &&
 	test_i18ncmp expect actual &&
 	test D = $(git cat-file commit HEAD | sed -ne \$p)
 '
-- 
2.22.0.589.g5bd7971b91

Re: [PATCH v3 2/5] t3404: make the 'rebase.missingCommitsCheck=ignore' test more focused

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 2377 bytes --]

Hi Gábor,

On Mon, 24 Jun 2019, SZEDER Gábor wrote:

> The test 'rebase -i respects rebase.missingCommitsCheck = warn' is
> mainly interested in the warning about the dropped commits, but it
> checks the whole output of 'git rebase', including progress lines and
> what not that are not at all relevant to 'rebase.missingCommitsCheck',
> but make it necessary to update this test whenever e.g. the way we
> show progress is updated (as it will happen in one of the later
> patches of this series).
>
> Modify the test to verify only the first four lines of 'git rebase's
> output that contain all the important lines, notably the line
> containing the "Warning:" itself and the oneline log of the dropped
> commit.

Thank you. Looks very good to me,
Dscho

>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
>  t/t3404-rebase-interactive.sh | 15 +--------------
>  1 file changed, 1 insertion(+), 14 deletions(-)
>
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 9146f9d47b..0b8267c97c 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -1299,32 +1299,19 @@ test_expect_success 'rebase -i respects rebase.missingCommitsCheck = ignore' '
>  		actual
>  '
>
> -cr_to_nl () {
> -	tr '\015' '\012'
> -}
> -
>  test_expect_success 'rebase -i respects rebase.missingCommitsCheck = warn' '
>  	cat >expect <<-EOF &&
>  	Warning: some commits may have been dropped accidentally.
>  	Dropped commits (newer to older):
>  	 - $(git rev-list --pretty=oneline --abbrev-commit -1 master)
>  	To avoid this message, use "drop" to explicitly remove a commit.
> -
> -	Use '\''git config rebase.missingCommitsCheck'\'' to change the level of warnings.
> -	The possible behaviours are: ignore, warn, error.
> -
> -	Rebasing (1/4)
> -	Rebasing (2/4)
> -	Rebasing (3/4)
> -	Rebasing (4/4)
> -	Successfully rebased and updated refs/heads/missing-commit.
>  	EOF
>  	test_config rebase.missingCommitsCheck warn &&
>  	rebase_setup_and_clean missing-commit &&
>  	set_fake_editor &&
>  	FAKE_LINES="1 2 3 4" \
>  		git rebase -i --root 2>actual.2 &&
> -	cr_to_nl <actual.2 >actual &&
> +	head -n4 actual.2 >actual &&
>  	test_i18ncmp expect actual &&
>  	test D = $(git cat-file commit HEAD | sed -ne \$p)
>  '
> --
> 2.22.0.589.g5bd7971b91
>
>

[PATCH v3 3/5] pager: add a helper function to clear the last line in the terminal

[SZEDER Gábor]

There are a couple of places where we want to clear the last line on
the terminal, e.g. when a progress bar line is overwritten by a
shorter line, then the end of that progress line would remain visible,
unless we cover it up.

In 'progress.c' we did this by always appending a fixed number of
space characters to the next line (even if it was not shorter than the
previous), but as it turned out that fixed number was not quite large
enough, see the fix in 9f1fd84e15 (progress: clear previous progress
update dynamically, 2019-04-12).  From then on we've been keeping
track of the length of the last displayed progress line and appending
the appropriate number of space characters to the next line, if
necessary, but, alas, this approach turned out to be error prone, see
the fix in 1aed1a5f25 (progress: avoid empty line when breaking the
progress line, 2019-05-19).  The next patch in this series is about to
fix a case where we don't clear the last line, and on occasion do end
up with such garbage at the end of the line.  It would be great if we
could do that without the need to deal with that without meticulously
computing the necessary number of space characters.

So add a helper function to clear the last line on the terminal using
an ANSI escape sequence, which has the advantage to clear the whole
line no matter how wide it is, even after the terminal width changed.
Such an escape sequence is not available on dumb terminals, though, so
in that case fall back to simply print a whole terminal width (as
reported by term_columns()) worth of space characters.

In 'editor.c' launch_specified_editor() already used this ANSI escape
sequence, so replace it with a call to this function.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 cache.h  |  1 +
 editor.c |  6 +++---
 pager.c  | 20 ++++++++++++++++++++
 3 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/cache.h b/cache.h
index b4bb2e2c11..5b2cd32bad 100644
--- a/cache.h
+++ b/cache.h
@@ -1759,6 +1759,7 @@ void setup_pager(void);
 int pager_in_use(void);
 extern int pager_use_color;
 int term_columns(void);
+void term_clear_line(void);
 int decimal_width(uintmax_t);
 int check_pager_config(const char *cmd);
 void prepare_pager_args(struct child_process *, const char *pager);
diff --git a/editor.c b/editor.c
index 71547674ab..f079abbf11 100644
--- a/editor.c
+++ b/editor.c
@@ -96,10 +96,10 @@ static int launch_specified_editor(const char *editor, const char *path,
 
 		if (print_waiting_for_editor && !is_terminal_dumb())
 			/*
-			 * Go back to the beginning and erase the entire line to
-			 * avoid wasting the vertical space.
+			 * Erase the entire line to avoid wasting the
+			 * vertical space.
 			 */
-			fputs("\r\033[K", stderr);
+			term_clear_line();
 	}
 
 	if (!buffer)
diff --git a/pager.c b/pager.c
index 4168460ae9..41446d4f05 100644
--- a/pager.c
+++ b/pager.c
@@ -177,6 +177,26 @@ int term_columns(void)
 	return term_columns_at_startup;
 }
 
+/*
+ * Clear the entire line, leave cursor in first column.
+ */
+void term_clear_line(void)
+{
+	if (is_terminal_dumb())
+		/*
+		 * Fall back to print a terminal width worth of space
+		 * characters (hoping that the terminal is still as wide
+		 * as it was upon the first call to term_columns()).
+		 */
+		fprintf(stderr, "\r%*s\r", term_columns(), "");
+	else
+		/*
+		 * On non-dumb terminals use an escape sequence to clear
+		 * the whole line, no matter how wide the terminal.
+		 */
+		fputs("\r\033[K", stderr);
+}
+
 /*
  * How many columns do we need to show this number in decimal?
  */
-- 
2.22.0.589.g5bd7971b91

[PATCH v3 4/5] rebase: fix garbled progress display with '-x'

[SZEDER Gábor]

When running a command with the 'exec' instruction during an
interactive rebase session, or for a range of commits using 'git
rebase -x', the output can be a bit garbled when the name of the
command is short enough:

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

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

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

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

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

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

In 't3420-rebase-autostash.sh' two helper functions prepare the
expected output of four tests that check the full output of 'git
rebase' and thus are affected by this change, so adjust their
expectations to account for the new line clearing.

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

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

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

[PATCH v3.1 4/5] rebase: fix garbled progress display with '-x'

[SZEDER Gábor]

When running a command with the 'exec' instruction during an
interactive rebase session, or for a range of commits using 'git
rebase -x', the output can be a bit garbled when the name of the
command is short enough:

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

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

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

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

Make sure that the previously displayed "Rebasing (N/M)" line is
cleared by using the term_clear_line() helper function added in the
previous patch.  Do so only when not being '--verbose', because in
that case these "Rebasing (N/M)" lines are not printed as progress
(i.e. as lines with '\r' at the end), but as "regular" output (with
'\n' at the end).

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

In 't3420-rebase-autostash.sh' two helper functions prepare the
expected output of four tests that check the full output of 'git
rebase' and thus are affected by this change, so adjust their
expectations to account for the new line clearing.

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

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---

The exact same patch with a slightly updated commit message to address

  https://public-inbox.org/git/xmqqy327kfw1.fsf@gitster-ct.c.googlers.com/

and to fix a typo.

Range-diff:
1:  4372a3cde4 = 1:  4372a3cde4 t3404: modernize here doc style
2:  5444f547c5 = 2:  5444f547c5 t3404: make the 'rebase.missingCommitsCheck=ignore' test more focused
3:  5ebf218cb9 = 3:  5ebf218cb9 pager: add a helper function to clear the last line in the terminal
4:  51cd5ccd46 ! 4:  652b69d0dd rebase: fix garbled progress display with '-x'
    @@ -33,7 +33,10 @@
     
         Make sure that the previously displayed "Rebasing (N/M)" line is
         cleared by using the term_clear_line() helper function added in the
    -    previous patch.
    +    previous patch.  Do so only when not being '--verbose', because in
    +    that case these "Rebasing (N/M)" lines are not printed as progress
    +    (i.e. as lines with '\r' at the end), but as "regular" output (with
    +    '\n' at the end).
     
         A couple of other rebase commands print similar messages, e.g.
         "Stopped at <abbrev-oid>... <subject>" for the 'edit' or 'break'
    @@ -49,7 +52,7 @@
     
         Note that this patch doesn't completely eliminate the possibility of
         similar garbled outputs, e.g. some error messages from rebase or the
    -    "Auto-merging <file>" message from withing the depths of the merge
    +    "Auto-merging <file>" message from within the depths of the merge
         machinery might not be long enough to completely cover the last
         "Rebasing (N/M)" line.  This patch doesn't do anything about them,
         because dealing with them individually would result in way too much
5:  09642a458e = 5:  313ce83d0a progress: use term_clear_line()


 sequencer.c                 | 17 ++++++++++++++---
 t/t3420-rebase-autostash.sh |  4 ++--
 2 files changed, 16 insertions(+), 5 deletions(-)

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

[PATCH v3 5/5] progress: use term_clear_line()

[SZEDER Gábor]

To make sure that the previously displayed progress line is completely
covered up when the new line is shorter, commit 545dc345eb (progress:
break too long progress bar lines, 2019-04-12) added a bunch of
calculations to figure out how many characters it needs to overwrite
with spaces.

Use the just introduced term_clear_line() helper function to, well,
clear the last line, making all these calculations unnecessary, and
thus simplifying the code considerably.

Three tests in 't5541-http-push-smart.sh' 'grep' for specific text
shown in the progress lines at the beginning of the line, but now
those lines begin either with the ANSI escape sequence or with the
terminal width worth of space characters clearing the line.  Relax the
'grep' patterns to match anywhere on the line.  Note that only two of
these three tests fail without relaxing their 'grep' pattern, but the
third looks for the absence of the pattern, so it still succeeds, but
without the adjustment would potentially hide future regressions.

Note also that with this change we no longer need the length of the
previously displayed progress line, so the strbuf added to 'struct
progress' in d53ba841d4 (progress: assemble percentage and counters in
a strbuf before printing, 2019-04-05) is not strictly necessary
anymore.  We still keep it, though, as it avoids allocating and
releasing a strbuf each time the progress is updated.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 progress.c                 | 28 +++++++++++-----------------
 t/t5541-http-push-smart.sh |  6 +++---
 2 files changed, 14 insertions(+), 20 deletions(-)

diff --git a/progress.c b/progress.c
index a2e8cf64a8..095dcd0ddf 100644
--- a/progress.c
+++ b/progress.c
@@ -88,7 +88,6 @@ static void display(struct progress *progress, uint64_t n, const char *done)
 	const char *tp;
 	struct strbuf *counters_sb = &progress->counters_sb;
 	int show_update = 0;
-	int last_count_len = counters_sb->len;
 
 	if (progress->delay && (!progress_update || --progress->delay))
 		return;
@@ -116,26 +115,21 @@ static void display(struct progress *progress, uint64_t n, const char *done)
 	if (show_update) {
 		if (is_foreground_fd(fileno(stderr)) || done) {
 			const char *eol = done ? done : "\r";
-			size_t clear_len = counters_sb->len < last_count_len ?
-					last_count_len - counters_sb->len + 1 :
-					0;
-			size_t progress_line_len = progress->title_len +
-						counters_sb->len + 2;
-			int cols = term_columns();
 
+			term_clear_line();
 			if (progress->split) {
-				fprintf(stderr, "  %s%*s", counters_sb->buf,
-					(int) clear_len, eol);
-			} else if (!done && cols < progress_line_len) {
-				clear_len = progress->title_len + 1 < cols ?
-					    cols - progress->title_len - 1 : 0;
-				fprintf(stderr, "%s:%*s\n  %s%s",
-					progress->title, (int) clear_len, "",
-					counters_sb->buf, eol);
+				fprintf(stderr, "  %s%s", counters_sb->buf,
+					eol);
+			} else if (!done &&
+				   /* The "+ 2" accounts for the ": ". */
+				   term_columns() < progress->title_len +
+						    counters_sb->len + 2) {
+				fprintf(stderr, "%s:\n  %s%s",
+					progress->title, counters_sb->buf, eol);
 				progress->split = 1;
 			} else {
-				fprintf(stderr, "%s: %s%*s", progress->title,
-					counters_sb->buf, (int) clear_len, eol);
+				fprintf(stderr, "%s: %s%s", progress->title,
+					counters_sb->buf, eol);
 			}
 			fflush(stderr);
 		}
diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh
index 8ef8763e06..2e4802e206 100755
--- a/t/t5541-http-push-smart.sh
+++ b/t/t5541-http-push-smart.sh
@@ -213,7 +213,7 @@ test_expect_success TTY 'push shows progress when stderr is a tty' '
 	cd "$ROOT_PATH"/test_repo_clone &&
 	test_commit noisy &&
 	test_terminal git push >output 2>&1 &&
-	test_i18ngrep "^Writing objects" output
+	test_i18ngrep "Writing objects" output
 '
 
 test_expect_success TTY 'push --quiet silences status and progress' '
@@ -228,7 +228,7 @@ test_expect_success TTY 'push --no-progress silences progress but not status' '
 	test_commit no-progress &&
 	test_terminal git push --no-progress >output 2>&1 &&
 	test_i18ngrep "^To http" output &&
-	test_i18ngrep ! "^Writing objects" output
+	test_i18ngrep ! "Writing objects" output
 '
 
 test_expect_success 'push --progress shows progress to non-tty' '
@@ -236,7 +236,7 @@ test_expect_success 'push --progress shows progress to non-tty' '
 	test_commit progress &&
 	git push --progress >output 2>&1 &&
 	test_i18ngrep "^To http" output &&
-	test_i18ngrep "^Writing objects" output
+	test_i18ngrep "Writing objects" output
 '
 
 test_expect_success 'http push gives sane defaults to reflog' '
-- 
2.22.0.589.g5bd7971b91

Re: [PATCH v3 5/5] progress: use term_clear_line()

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 5225 bytes --]

Hi Gábor,

On Mon, 24 Jun 2019, SZEDER Gábor wrote:

> To make sure that the previously displayed progress line is completely
> covered up when the new line is shorter, commit 545dc345eb (progress:
> break too long progress bar lines, 2019-04-12) added a bunch of
> calculations to figure out how many characters it needs to overwrite
> with spaces.
>
> Use the just introduced term_clear_line() helper function to, well,
> clear the last line, making all these calculations unnecessary, and
> thus simplifying the code considerably.
>
> Three tests in 't5541-http-push-smart.sh' 'grep' for specific text
> shown in the progress lines at the beginning of the line, but now
> those lines begin either with the ANSI escape sequence or with the
> terminal width worth of space characters clearing the line.  Relax the
> 'grep' patterns to match anywhere on the line.  Note that only two of
> these three tests fail without relaxing their 'grep' pattern, but the
> third looks for the absence of the pattern, so it still succeeds, but
> without the adjustment would potentially hide future regressions.
>
> Note also that with this change we no longer need the length of the
> previously displayed progress line, so the strbuf added to 'struct
> progress' in d53ba841d4 (progress: assemble percentage and counters in
> a strbuf before printing, 2019-04-05) is not strictly necessary
> anymore.  We still keep it, though, as it avoids allocating and
> releasing a strbuf each time the progress is updated.

Very nice change, indeed, I totally love how much simpler the post-image
looks. Well done, for the entire patch series, and thank you so much!
Dscho

>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
>  progress.c                 | 28 +++++++++++-----------------
>  t/t5541-http-push-smart.sh |  6 +++---
>  2 files changed, 14 insertions(+), 20 deletions(-)
>
> diff --git a/progress.c b/progress.c
> index a2e8cf64a8..095dcd0ddf 100644
> --- a/progress.c
> +++ b/progress.c
> @@ -88,7 +88,6 @@ static void display(struct progress *progress, uint64_t n, const char *done)
>  	const char *tp;
>  	struct strbuf *counters_sb = &progress->counters_sb;
>  	int show_update = 0;
> -	int last_count_len = counters_sb->len;
>
>  	if (progress->delay && (!progress_update || --progress->delay))
>  		return;
> @@ -116,26 +115,21 @@ static void display(struct progress *progress, uint64_t n, const char *done)
>  	if (show_update) {
>  		if (is_foreground_fd(fileno(stderr)) || done) {
>  			const char *eol = done ? done : "\r";
> -			size_t clear_len = counters_sb->len < last_count_len ?
> -					last_count_len - counters_sb->len + 1 :
> -					0;
> -			size_t progress_line_len = progress->title_len +
> -						counters_sb->len + 2;
> -			int cols = term_columns();
>
> +			term_clear_line();
>  			if (progress->split) {
> -				fprintf(stderr, "  %s%*s", counters_sb->buf,
> -					(int) clear_len, eol);
> -			} else if (!done && cols < progress_line_len) {
> -				clear_len = progress->title_len + 1 < cols ?
> -					    cols - progress->title_len - 1 : 0;
> -				fprintf(stderr, "%s:%*s\n  %s%s",
> -					progress->title, (int) clear_len, "",
> -					counters_sb->buf, eol);
> +				fprintf(stderr, "  %s%s", counters_sb->buf,
> +					eol);
> +			} else if (!done &&
> +				   /* The "+ 2" accounts for the ": ". */
> +				   term_columns() < progress->title_len +
> +						    counters_sb->len + 2) {
> +				fprintf(stderr, "%s:\n  %s%s",
> +					progress->title, counters_sb->buf, eol);
>  				progress->split = 1;
>  			} else {
> -				fprintf(stderr, "%s: %s%*s", progress->title,
> -					counters_sb->buf, (int) clear_len, eol);
> +				fprintf(stderr, "%s: %s%s", progress->title,
> +					counters_sb->buf, eol);
>  			}
>  			fflush(stderr);
>  		}
> diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh
> index 8ef8763e06..2e4802e206 100755
> --- a/t/t5541-http-push-smart.sh
> +++ b/t/t5541-http-push-smart.sh
> @@ -213,7 +213,7 @@ test_expect_success TTY 'push shows progress when stderr is a tty' '
>  	cd "$ROOT_PATH"/test_repo_clone &&
>  	test_commit noisy &&
>  	test_terminal git push >output 2>&1 &&
> -	test_i18ngrep "^Writing objects" output
> +	test_i18ngrep "Writing objects" output
>  '
>
>  test_expect_success TTY 'push --quiet silences status and progress' '
> @@ -228,7 +228,7 @@ test_expect_success TTY 'push --no-progress silences progress but not status' '
>  	test_commit no-progress &&
>  	test_terminal git push --no-progress >output 2>&1 &&
>  	test_i18ngrep "^To http" output &&
> -	test_i18ngrep ! "^Writing objects" output
> +	test_i18ngrep ! "Writing objects" output
>  '
>
>  test_expect_success 'push --progress shows progress to non-tty' '
> @@ -236,7 +236,7 @@ test_expect_success 'push --progress shows progress to non-tty' '
>  	test_commit progress &&
>  	git push --progress >output 2>&1 &&
>  	test_i18ngrep "^To http" output &&
> -	test_i18ngrep "^Writing objects" output
> +	test_i18ngrep "Writing objects" output
>  '
>
>  test_expect_success 'http push gives sane defaults to reflog' '
> --
> 2.22.0.589.g5bd7971b91
>
>

Re: [PATCH v3 5/5] progress: use term_clear_line()

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> To make sure that the previously displayed progress line is completely
>> covered up when the new line is shorter, commit 545dc345eb (progress:
>> break too long progress bar lines, 2019-04-12) added a bunch of
>> calculations to figure out how many characters it needs to overwrite
>> with spaces.
>> ...
> Very nice change, indeed, I totally love how much simpler the post-image
> looks. Well done, for the entire patch series, and thank you so much!
> Dscho

Thanks, both.  Let's merge down sg/rebase-progress soonish.