rebase: fix garbled progress display with '-x'
diff mbox series

Message ID 20190430142556.20921-1-szeder.dev@gmail.com
State New
Headers show
Series
  • rebase: fix garbled progress display with '-x'
Related show

Commit Message

SZEDER Gábor April 30, 2019, 2:25 p.m. UTC
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(+)

Comments

Johannes Schindelin April 30, 2019, 10:25 p.m. UTC | #1
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
>
>
SZEDER Gábor May 1, 2019, 11:16 p.m. UTC | #2
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
> >
> >
Johannes Schindelin May 3, 2019, 8:41 a.m. UTC | #3
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
SZEDER Gábor June 11, 2019, 12:38 p.m. UTC | #4
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
diff mbox series

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;