git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
* [PATCH] rebase: fix garbled progress display with '-x'
@ 2019-04-30 14:25 SZEDER Gábor
  2019-04-30 22:25 ` Johannes Schindelin
  2019-06-11 13:03 ` [PATCH v2 0/4] rebase/progress: add and use term_clear_line() SZEDER Gábor
  0 siblings, 2 replies; 20+ messages in thread
From: SZEDER Gábor @ 2019-04-30 14:25 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, 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


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] rebase: fix garbled progress display with '-x'
  2019-04-30 14:25 [PATCH] rebase: fix garbled progress display with '-x' SZEDER Gábor
@ 2019-04-30 22:25 ` Johannes Schindelin
  2019-05-01 23:16   ` SZEDER Gábor
  2019-06-11 13:03 ` [PATCH v2 0/4] rebase/progress: add and use term_clear_line() SZEDER Gábor
  1 sibling, 1 reply; 20+ messages in thread
From: Johannes Schindelin @ 2019-04-30 22:25 UTC (permalink / raw)
  To: SZEDER Gábor; +Cc: Junio C Hamano, git

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] rebase: fix garbled progress display with '-x'
  2019-04-30 22:25 ` Johannes Schindelin
@ 2019-05-01 23:16   ` SZEDER Gábor
  2019-05-03  8:41     ` Johannes Schindelin
  2019-06-11 12:38     ` SZEDER Gábor
  0 siblings, 2 replies; 20+ messages in thread
From: SZEDER Gábor @ 2019-05-01 23:16 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Junio C Hamano, git

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


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] rebase: fix garbled progress display with '-x'
  2019-05-01 23:16   ` SZEDER Gábor
@ 2019-05-03  8:41     ` Johannes Schindelin
  2019-06-11 12:38     ` SZEDER Gábor
  1 sibling, 0 replies; 20+ messages in thread
From: Johannes Schindelin @ 2019-05-03  8:41 UTC (permalink / raw)
  To: SZEDER Gábor; +Cc: Junio C Hamano, git

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] rebase: fix garbled progress display with '-x'
  2019-05-01 23:16   ` SZEDER Gábor
  2019-05-03  8:41     ` Johannes Schindelin
@ 2019-06-11 12:38     ` SZEDER Gábor
  1 sibling, 0 replies; 20+ messages in thread
From: SZEDER Gábor @ 2019-06-11 12:38 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Junio C Hamano, git

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.


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH v2 0/4] rebase/progress: add and use term_clear_line()
  2019-04-30 14:25 [PATCH] rebase: fix garbled progress display with '-x' SZEDER Gábor
  2019-04-30 22:25 ` Johannes Schindelin
@ 2019-06-11 13:03 ` SZEDER Gábor
  2019-06-11 13:03   ` [PATCH v2 1/4] t3404-rebase-interactive: use the 'q_to_cr' helper SZEDER Gábor
                     ` (3 more replies)
  1 sibling, 4 replies; 20+ messages in thread
From: SZEDER Gábor @ 2019-06-11 13:03 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Johannes Schindelin, git, 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


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH v2 1/4] t3404-rebase-interactive: use the 'q_to_cr' helper
  2019-06-11 13:03 ` [PATCH v2 0/4] rebase/progress: add and use term_clear_line() SZEDER Gábor
@ 2019-06-11 13:03   ` SZEDER Gábor
  2019-06-11 13:03   ` [PATCH v2 2/4] pager: add a helper function to clear the last line in the terminal SZEDER Gábor
                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 20+ messages in thread
From: SZEDER Gábor @ 2019-06-11 13:03 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Johannes Schindelin, git, 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


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH v2 2/4] pager: add a helper function to clear the last line in the terminal
  2019-06-11 13:03 ` [PATCH v2 0/4] rebase/progress: add and use term_clear_line() SZEDER Gábor
  2019-06-11 13:03   ` [PATCH v2 1/4] t3404-rebase-interactive: use the 'q_to_cr' helper SZEDER Gábor
@ 2019-06-11 13:03   ` SZEDER Gábor
  2019-06-11 13:03   ` [PATCH v2 3/4] rebase: fix garbled progress display with '-x' SZEDER Gábor
  2019-06-11 13:03   ` [PATCH v2 4/4] progress: use term_clear_line() SZEDER Gábor
  3 siblings, 0 replies; 20+ messages in thread
From: SZEDER Gábor @ 2019-06-11 13:03 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Johannes Schindelin, git, 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


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH v2 3/4] rebase: fix garbled progress display with '-x'
  2019-06-11 13:03 ` [PATCH v2 0/4] rebase/progress: add and use term_clear_line() SZEDER Gábor
  2019-06-11 13:03   ` [PATCH v2 1/4] t3404-rebase-interactive: use the 'q_to_cr' helper SZEDER Gábor
  2019-06-11 13:03   ` [PATCH v2 2/4] pager: add a helper function to clear the last line in the terminal SZEDER Gábor
@ 2019-06-11 13:03   ` SZEDER Gábor
  2019-06-11 20:36     ` Junio C Hamano
  2019-06-11 20:48     ` Junio C Hamano
  2019-06-11 13:03   ` [PATCH v2 4/4] progress: use term_clear_line() SZEDER Gábor
  3 siblings, 2 replies; 20+ messages in thread
From: SZEDER Gábor @ 2019-06-11 13:03 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Johannes Schindelin, git, 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


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH v2 4/4] progress: use term_clear_line()
  2019-06-11 13:03 ` [PATCH v2 0/4] rebase/progress: add and use term_clear_line() SZEDER Gábor
                     ` (2 preceding siblings ...)
  2019-06-11 13:03   ` [PATCH v2 3/4] rebase: fix garbled progress display with '-x' SZEDER Gábor
@ 2019-06-11 13:03   ` SZEDER Gábor
  2019-06-12 20:00     ` Johannes Schindelin
  3 siblings, 1 reply; 20+ messages in thread
From: SZEDER Gábor @ 2019-06-11 13:03 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Johannes Schindelin, git, 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


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 3/4] rebase: fix garbled progress display with '-x'
  2019-06-11 13:03   ` [PATCH v2 3/4] rebase: fix garbled progress display with '-x' SZEDER Gábor
@ 2019-06-11 20:36     ` Junio C Hamano
  2019-06-11 21:11       ` SZEDER Gábor
  2019-06-11 20:48     ` Junio C Hamano
  1 sibling, 1 reply; 20+ messages in thread
From: Junio C Hamano @ 2019-06-11 20:36 UTC (permalink / raw)
  To: SZEDER Gábor; +Cc: Johannes Schindelin, git

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?

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 3/4] rebase: fix garbled progress display with '-x'
  2019-06-11 13:03   ` [PATCH v2 3/4] rebase: fix garbled progress display with '-x' SZEDER Gábor
  2019-06-11 20:36     ` Junio C Hamano
@ 2019-06-11 20:48     ` Junio C Hamano
  2019-06-11 23:50       ` SZEDER Gábor
  2019-06-12 16:21       ` Junio C Hamano
  1 sibling, 2 replies; 20+ messages in thread
From: Junio C Hamano @ 2019-06-11 20:48 UTC (permalink / raw)
  To: SZEDER Gábor; +Cc: Johannes Schindelin, git

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?

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 3/4] rebase: fix garbled progress display with '-x'
  2019-06-11 20:36     ` Junio C Hamano
@ 2019-06-11 21:11       ` SZEDER Gábor
  2019-06-12 19:14         ` Johannes Schindelin
  0 siblings, 1 reply; 20+ messages in thread
From: SZEDER Gábor @ 2019-06-11 21:11 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Johannes Schindelin, git

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 3/4] rebase: fix garbled progress display with '-x'
  2019-06-11 20:48     ` Junio C Hamano
@ 2019-06-11 23:50       ` SZEDER Gábor
  2019-06-12 16:21       ` Junio C Hamano
  1 sibling, 0 replies; 20+ messages in thread
From: SZEDER Gábor @ 2019-06-11 23:50 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Johannes Schindelin, git

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.



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 3/4] rebase: fix garbled progress display with '-x'
  2019-06-11 20:48     ` Junio C Hamano
  2019-06-11 23:50       ` SZEDER Gábor
@ 2019-06-12 16:21       ` Junio C Hamano
  1 sibling, 0 replies; 20+ messages in thread
From: Junio C Hamano @ 2019-06-12 16:21 UTC (permalink / raw)
  To: SZEDER Gábor; +Cc: Johannes Schindelin, git

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.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 3/4] rebase: fix garbled progress display with '-x'
  2019-06-11 21:11       ` SZEDER Gábor
@ 2019-06-12 19:14         ` Johannes Schindelin
  2019-06-12 19:41           ` SZEDER Gábor
  0 siblings, 1 reply; 20+ messages in thread
From: Johannes Schindelin @ 2019-06-12 19:14 UTC (permalink / raw)
  To: SZEDER Gábor; +Cc: Junio C Hamano, git

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 3/4] rebase: fix garbled progress display with '-x'
  2019-06-12 19:14         ` Johannes Schindelin
@ 2019-06-12 19:41           ` SZEDER Gábor
  2019-06-13  7:54             ` Johannes Schindelin
  2019-06-14 18:42             ` Johannes Schindelin
  0 siblings, 2 replies; 20+ messages in thread
From: SZEDER Gábor @ 2019-06-12 19:41 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Junio C Hamano, git

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?


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 4/4] progress: use term_clear_line()
  2019-06-11 13:03   ` [PATCH v2 4/4] progress: use term_clear_line() SZEDER Gábor
@ 2019-06-12 20:00     ` Johannes Schindelin
  0 siblings, 0 replies; 20+ messages in thread
From: Johannes Schindelin @ 2019-06-12 20:00 UTC (permalink / raw)
  To: SZEDER Gábor; +Cc: Junio C Hamano, git

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 3/4] rebase: fix garbled progress display with '-x'
  2019-06-12 19:41           ` SZEDER Gábor
@ 2019-06-13  7:54             ` Johannes Schindelin
  2019-06-14 18:42             ` Johannes Schindelin
  1 sibling, 0 replies; 20+ messages in thread
From: Johannes Schindelin @ 2019-06-13  7:54 UTC (permalink / raw)
  To: SZEDER Gábor; +Cc: Junio C Hamano, git

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 3/4] rebase: fix garbled progress display with '-x'
  2019-06-12 19:41           ` SZEDER Gábor
  2019-06-13  7:54             ` Johannes Schindelin
@ 2019-06-14 18:42             ` Johannes Schindelin
  1 sibling, 0 replies; 20+ messages in thread
From: Johannes Schindelin @ 2019-06-14 18:42 UTC (permalink / raw)
  To: SZEDER Gábor; +Cc: Junio C Hamano, git

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, back to index

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-30 14:25 [PATCH] rebase: fix garbled progress display with '-x' SZEDER Gábor
2019-04-30 22:25 ` Johannes Schindelin
2019-05-01 23:16   ` SZEDER Gábor
2019-05-03  8:41     ` Johannes Schindelin
2019-06-11 12:38     ` SZEDER Gábor
2019-06-11 13:03 ` [PATCH v2 0/4] rebase/progress: add and use term_clear_line() SZEDER Gábor
2019-06-11 13:03   ` [PATCH v2 1/4] t3404-rebase-interactive: use the 'q_to_cr' helper SZEDER Gábor
2019-06-11 13:03   ` [PATCH v2 2/4] pager: add a helper function to clear the last line in the terminal SZEDER Gábor
2019-06-11 13:03   ` [PATCH v2 3/4] rebase: fix garbled progress display with '-x' SZEDER Gábor
2019-06-11 20:36     ` Junio C Hamano
2019-06-11 21:11       ` SZEDER Gábor
2019-06-12 19:14         ` Johannes Schindelin
2019-06-12 19:41           ` SZEDER Gábor
2019-06-13  7:54             ` Johannes Schindelin
2019-06-14 18:42             ` Johannes Schindelin
2019-06-11 20:48     ` Junio C Hamano
2019-06-11 23:50       ` SZEDER Gábor
2019-06-12 16:21       ` Junio C Hamano
2019-06-11 13:03   ` [PATCH v2 4/4] progress: use term_clear_line() SZEDER Gábor
2019-06-12 20:00     ` Johannes Schindelin

git@vger.kernel.org list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox