[PATCH 0/5] Progress display fixes

[PATCH 0/5] Progress display fixes

[SZEDER Gábor]

This patch series fixes two progress display issues:

  - When showing throughput, and the both the total and the throughput
    change units in the same update, than the previously shown
    progress bar is not cleaned up properly:

      Receiving objects:  25% (2901/11603), 772.01 KiB | 733.00 KiB/s
      Receiving objects:  27% (3133/11603), 1.43 MiB | 1.16 MiB/s   s

  - When the progress bar is longer than the width of the terminal,
    then we end up with a bunch of truncated progress bar lines
    scrolling past:

      $ LANG=es_ES.UTF-8 git commit-graph write
      Encontrando commits para commit graph entre los objetos empaquetados:   2% (1599
      Encontrando commits para commit graph entre los objetos empaquetados:   3% (1975
      Encontrando commits para commit graph entre los objetos empaquetados:   4% (2633
      Encontrando commits para commit graph entre los objetos empaquetados:   5% (3292
      [...]

Patches 4 and 5 fix these two issues, while the first three are
minor preparatory cleanups and refactorings.

SZEDER Gábor (5):
  progress: make display_progress() return void
  progress: return early when in the background
  progress: assemble percentage and counters in a strbuf before printing
  progress: clear previous progress update dynamically
  progress: break too long progress bar lines

 progress.c | 78 +++++++++++++++++++++++++++++++++++++++---------------
 progress.h |  2 +-
 2 files changed, 57 insertions(+), 23 deletions(-)

-- 
2.21.0.539.g07239c3a71.dirty

[PATCH 1/5] progress: make display_progress() return void

[SZEDER Gábor]

Ever since the progress infrastructure was introduced in 96a02f8f6d
(common progress display support, 2007-04-18), display_progress() has
returned an int, telling callers whether it updated the progress bar
or not.  However, this is:

  - useless, because over the last dozen years there has never been a
    single caller that cared about that return value.

  - not quite true, because it doesn't print a progress bar when
    running in the background, yet it returns 1; see 85cb8906f0
    (progress: no progress in background, 2015-04-13).

The related display_throughput() function returned void already upon
its introduction in cf84d51c43 (add throughput to progress display,
2007-10-30).

Let's make display_progress() return void, too.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 progress.c | 15 ++++++++-------
 progress.h |  2 +-
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/progress.c b/progress.c
index 5a99c9fbf0..02a20e7d58 100644
--- a/progress.c
+++ b/progress.c
@@ -78,12 +78,12 @@ static int is_foreground_fd(int fd)
 	return tpgrp < 0 || tpgrp == getpgid(0);
 }
 
-static int display(struct progress *progress, uint64_t n, const char *done)
+static void display(struct progress *progress, uint64_t n, const char *done)
 {
 	const char *eol, *tp;
 
 	if (progress->delay && (!progress_update || --progress->delay))
-		return 0;
+		return;
 
 	progress->last_value = n;
 	tp = (progress->throughput) ? progress->throughput->display.buf : "";
@@ -100,7 +100,7 @@ static int display(struct progress *progress, uint64_t n, const char *done)
 				fflush(stderr);
 			}
 			progress_update = 0;
-			return 1;
+			return;
 		}
 	} else if (progress_update) {
 		if (is_foreground_fd(fileno(stderr)) || done) {
@@ -109,10 +109,10 @@ static int display(struct progress *progress, uint64_t n, const char *done)
 			fflush(stderr);
 		}
 		progress_update = 0;
-		return 1;
+		return;
 	}
 
-	return 0;
+	return;
 }
 
 static void throughput_string(struct strbuf *buf, uint64_t total,
@@ -188,9 +188,10 @@ void display_throughput(struct progress *progress, uint64_t total)
 		display(progress, progress->last_value, NULL);
 }
 
-int display_progress(struct progress *progress, uint64_t n)
+void display_progress(struct progress *progress, uint64_t n)
 {
-	return progress ? display(progress, n, NULL) : 0;
+	if (progress)
+		display(progress, n, NULL);
 }
 
 static struct progress *start_progress_delay(const char *title, uint64_t total,
diff --git a/progress.h b/progress.h
index 70a4d4a0d6..59e40cc4fd 100644
--- a/progress.h
+++ b/progress.h
@@ -4,7 +4,7 @@
 struct progress;
 
 void display_throughput(struct progress *progress, uint64_t total);
-int display_progress(struct progress *progress, uint64_t n);
+void display_progress(struct progress *progress, uint64_t n);
 struct progress *start_progress(const char *title, uint64_t total);
 struct progress *start_delayed_progress(const char *title, uint64_t total);
 void stop_progress(struct progress **progress);
-- 
2.21.0.539.g07239c3a71.dirty

[PATCH 2/5] progress: return early when in the background

[SZEDER Gábor]

When a git process runs in the background, it doesn't display
progress, only the final "done" line [1].  The condition to check that
are a bit too deep in the display() function, and thus it calculates
the progress percentage even when no progress will be displayed
anyway.

Restructure the display() function to return early when we are in the
background, which prevents the unnecessary progress percentae
calculation, and make the function look a bit better by losing one
level of indentation.

[1] 85cb8906f0 (progress: no progress in background, 2015-04-13)

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 progress.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/progress.c b/progress.c
index 02a20e7d58..b57c0dae16 100644
--- a/progress.c
+++ b/progress.c
@@ -86,28 +86,30 @@ static void display(struct progress *progress, uint64_t n, const char *done)
 		return;
 
 	progress->last_value = n;
+
+	if (!is_foreground_fd(fileno(stderr)) && !done) {
+		progress_update = 0;
+		return;
+	}
+
 	tp = (progress->throughput) ? progress->throughput->display.buf : "";
 	eol = done ? done : "   \r";
 	if (progress->total) {
 		unsigned percent = n * 100 / progress->total;
 		if (percent != progress->last_percent || progress_update) {
 			progress->last_percent = percent;
-			if (is_foreground_fd(fileno(stderr)) || done) {
-				fprintf(stderr, "%s: %3u%% (%"PRIuMAX"/%"PRIuMAX")%s%s",
-					progress->title, percent,
-					(uintmax_t)n, (uintmax_t)progress->total,
-					tp, eol);
-				fflush(stderr);
-			}
+			fprintf(stderr, "%s: %3u%% (%"PRIuMAX"/%"PRIuMAX")%s%s",
+				progress->title, percent,
+				(uintmax_t)n, (uintmax_t)progress->total,
+				tp, eol);
+			fflush(stderr);
 			progress_update = 0;
 			return;
 		}
 	} else if (progress_update) {
-		if (is_foreground_fd(fileno(stderr)) || done) {
-			fprintf(stderr, "%s: %"PRIuMAX"%s%s",
-				progress->title, (uintmax_t)n, tp, eol);
-			fflush(stderr);
-		}
+		fprintf(stderr, "%s: %"PRIuMAX"%s%s",
+			progress->title, (uintmax_t)n, tp, eol);
+		fflush(stderr);
 		progress_update = 0;
 		return;
 	}
-- 
2.21.0.539.g07239c3a71.dirty

[PATCH 3/5] progress: assemble percentage and counters in a strbuf before printing

[SZEDER Gábor]

The following patches in this series want to handle the progress bar's
title and changing parts (i.e. the counter and the optional percentage
and throughput combined) differently.

To prepare for those changes assemble the changing parts in a separate
strbuf before printing.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 progress.c | 35 +++++++++++++++++++++++------------
 1 file changed, 23 insertions(+), 12 deletions(-)

diff --git a/progress.c b/progress.c
index b57c0dae16..390d2487ca 100644
--- a/progress.c
+++ b/progress.c
@@ -36,6 +36,7 @@ struct progress {
 	unsigned delay;
 	struct throughput *throughput;
 	uint64_t start_ns;
+	struct strbuf counters_sb;
 };
 
 static volatile sig_atomic_t progress_update;
@@ -80,7 +81,9 @@ static int is_foreground_fd(int fd)
 
 static void display(struct progress *progress, uint64_t n, const char *done)
 {
-	const char *eol, *tp;
+	const char *tp;
+	struct strbuf *counters_sb = &progress->counters_sb;
+	int update = 0;
 
 	if (progress->delay && (!progress_update || --progress->delay))
 		return;
@@ -93,25 +96,31 @@ static void display(struct progress *progress, uint64_t n, const char *done)
 	}
 
 	tp = (progress->throughput) ? progress->throughput->display.buf : "";
-	eol = done ? done : "   \r";
 	if (progress->total) {
 		unsigned percent = n * 100 / progress->total;
 		if (percent != progress->last_percent || progress_update) {
 			progress->last_percent = percent;
-			fprintf(stderr, "%s: %3u%% (%"PRIuMAX"/%"PRIuMAX")%s%s",
-				progress->title, percent,
-				(uintmax_t)n, (uintmax_t)progress->total,
-				tp, eol);
-			fflush(stderr);
-			progress_update = 0;
-			return;
+
+			strbuf_reset(counters_sb);
+			strbuf_addf(counters_sb,
+				    "%3u%% (%"PRIuMAX"/%"PRIuMAX")%s", percent,
+				    (uintmax_t)n, (uintmax_t)progress->total,
+				    tp);
+			update = 1;
 		}
 	} else if (progress_update) {
-		fprintf(stderr, "%s: %"PRIuMAX"%s%s",
-			progress->title, (uintmax_t)n, tp, eol);
+		strbuf_reset(counters_sb);
+		strbuf_addf(counters_sb, "%"PRIuMAX"%s", (uintmax_t)n, tp);
+		update = 1;
+	}
+
+	if (update) {
+		const char *eol = done ? done : "   \r";
+
+		fprintf(stderr, "%s: %s%s", progress->title, counters_sb->buf,
+			eol);
 		fflush(stderr);
 		progress_update = 0;
-		return;
 	}
 
 	return;
@@ -213,6 +222,7 @@ static struct progress *start_progress_delay(const char *title, uint64_t total,
 	progress->delay = delay;
 	progress->throughput = NULL;
 	progress->start_ns = getnanotime();
+	strbuf_init(&progress->counters_sb, 0);
 	set_progress_signal();
 	return progress;
 }
@@ -256,6 +266,7 @@ void stop_progress_msg(struct progress **p_progress, const char *msg)
 		free(buf);
 	}
 	clear_progress_signal();
+	strbuf_release(&progress->counters_sb);
 	if (progress->throughput)
 		strbuf_release(&progress->throughput->display);
 	free(progress->throughput);
-- 
2.21.0.539.g07239c3a71.dirty

[PATCH 4/5] progress: clear previous progress update dynamically

[SZEDER Gábor]

When the progress bar includes throughput, its length can shorten as
the unit of display changes from KiB to MiB.  To cover up remnants of
the previous progress bar when such a change of units happens we
always print three spaces at the end of the progress bar.

Alas, covering only three characters is not quite enough: when both
the total and the throughput happen to change units from KiB to MiB in
the same update, then the progress bar's length is shortened by four
characters:

  Receiving objects:  25% (2901/11603), 772.01 KiB | 733.00 KiB/s
  Receiving objects:  27% (3133/11603), 1.43 MiB | 1.16 MiB/s   s

and a stray 's' is left behind.

So instead of hard-coding the three characters to cover, let's compare
the length of the current progress bar with the previous one, and
cover up as many characters as needed.

Sure, it would be much simpler to just print four spaces instead of
three at the end of the progress bar, but this approach is more
future-proof, and it won't print extra spaces when none are needed,
notably when the progress bar doesn't show throughput and thus never
shrinks.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 progress.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/progress.c b/progress.c
index 390d2487ca..b37a5398c5 100644
--- a/progress.c
+++ b/progress.c
@@ -84,6 +84,7 @@ static void display(struct progress *progress, uint64_t n, const char *done)
 	const char *tp;
 	struct strbuf *counters_sb = &progress->counters_sb;
 	int update = 0;
+	int last_count_len = counters_sb->len;
 
 	if (progress->delay && (!progress_update || --progress->delay))
 		return;
@@ -115,10 +116,11 @@ static void display(struct progress *progress, uint64_t n, const char *done)
 	}
 
 	if (update) {
-		const char *eol = done ? done : "   \r";
-
-		fprintf(stderr, "%s: %s%s", progress->title, counters_sb->buf,
-			eol);
+		const char *eol = done ? done : "\r";
+		int clear_len = counters_sb->len < last_count_len ?
+				last_count_len - counters_sb->len : 0;
+		fprintf(stderr, "%s: %s%-*s", progress->title,
+			counters_sb->buf, clear_len, eol);
 		fflush(stderr);
 		progress_update = 0;
 	}
-- 
2.21.0.539.g07239c3a71.dirty

[PATCH 5/5] progress: break too long progress bar lines

[SZEDER Gábor]

Some of the recently added progress indicators have quite long titles,
which might be even longer when translated to some languages, and when
they are shown while operating on bigger repositories, then the
progress bar grows longer than the default 80 column terminal width.

When the progress bar exceeds the width of the terminal it gets
line-wrapped, and after that the CR at the end doesn't return to the
beginning of the progress bar, but to the first column of its last
line.  Consequently, the first line of the previously shown progress
bar is not overwritten by the next, and we end up with a bunch of
truncated progress bar lines scrolling past:

  $ LANG=es_ES.UTF-8 git commit-graph write
  Encontrando commits para commit graph entre los objetos empaquetados:   2% (1599
  Encontrando commits para commit graph entre los objetos empaquetados:   3% (1975
  Encontrando commits para commit graph entre los objetos empaquetados:   4% (2633
  Encontrando commits para commit graph entre los objetos empaquetados:   5% (3292
  [...]

Prevent this by breaking progress bars after the title once they
exceed the width of the terminal, so the counter and optional
percentage and throughput, i.e. all changing parts, are on the last
line.  Subsequent updates will from then on only refresh the changing
parts, but not the title, and it will look like this:

  $ LANG=es_ES.UTF-8 ~/src/git/git commit-graph write
  Encontrando commits para commit graph entre los objetos empaquetados:
    100% (6584502/6584502), listo.
  Calculando números de generación de commit graph: 100% (824705/824705), listo.
  Escribiendo commit graph en 4 pasos: 100% (3298820/3298820), listo.

Note that the number of columns in the terminal is cached by
term_columns(), so this might not kick in when it should when a
terminal window is resized while the operation is running.
Furthermore, this change won't help if the terminal is so narrow that
the counters don't fit on one line, but I would put this in the "If it
hurts, don't do it" box.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 progress.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/progress.c b/progress.c
index b37a5398c5..36aaeea340 100644
--- a/progress.c
+++ b/progress.c
@@ -8,7 +8,7 @@
  * published by the Free Software Foundation.
  */
 
-#include "git-compat-util.h"
+#include "cache.h"
 #include "gettext.h"
 #include "progress.h"
 #include "strbuf.h"
@@ -37,6 +37,8 @@ struct progress {
 	struct throughput *throughput;
 	uint64_t start_ns;
 	struct strbuf counters_sb;
+	int title_len;
+	int split;
 };
 
 static volatile sig_atomic_t progress_update;
@@ -119,8 +121,22 @@ static void display(struct progress *progress, uint64_t n, const char *done)
 		const char *eol = done ? done : "\r";
 		int clear_len = counters_sb->len < last_count_len ?
 				last_count_len - counters_sb->len : 0;
-		fprintf(stderr, "%s: %s%-*s", progress->title,
-			counters_sb->buf, clear_len, eol);
+		int cols = term_columns();
+
+		if (progress->split) {
+			fprintf(stderr, "  %s%-*s", counters_sb->buf, clear_len,
+				eol);
+		} else if (!done &&
+			   cols < progress->title_len + counters_sb->len + 2) {
+			clear_len = progress->title_len + 1 < cols ?
+				    cols - progress->title_len - 1 : 0;
+			fprintf(stderr, "%s:%*s\n  %s%s", progress->title,
+					clear_len, "", counters_sb->buf, eol);
+			progress->split = 1;
+		} else {
+			fprintf(stderr, "%s: %s%-*s", progress->title,
+				counters_sb->buf, clear_len, eol);
+		}
 		fflush(stderr);
 		progress_update = 0;
 	}
@@ -225,6 +241,8 @@ static struct progress *start_progress_delay(const char *title, uint64_t total,
 	progress->throughput = NULL;
 	progress->start_ns = getnanotime();
 	strbuf_init(&progress->counters_sb, 0);
+	progress->title_len = strlen(title);
+	progress->split = 0;
 	set_progress_signal();
 	return progress;
 }
-- 
2.21.0.539.g07239c3a71.dirty

Re: [PATCH 5/5] progress: break too long progress bar lines

[Duy Nguyen]

On Mon, Mar 25, 2019 at 5:41 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:
>
> Some of the recently added progress indicators have quite long titles,
> which might be even longer when translated to some languages, and when
> they are shown while operating on bigger repositories, then the
> progress bar grows longer than the default 80 column terminal width.
>
> When the progress bar exceeds the width of the terminal it gets
> line-wrapped, and after that the CR at the end doesn't return to the
> beginning of the progress bar, but to the first column of its last
> line.  Consequently, the first line of the previously shown progress
> bar is not overwritten by the next, and we end up with a bunch of
> truncated progress bar lines scrolling past:
>
>   $ LANG=es_ES.UTF-8 git commit-graph write
>   Encontrando commits para commit graph entre los objetos empaquetados:   2% (1599
>   Encontrando commits para commit graph entre los objetos empaquetados:   3% (1975
>   Encontrando commits para commit graph entre los objetos empaquetados:   4% (2633
>   Encontrando commits para commit graph entre los objetos empaquetados:   5% (3292
>   [...]
>
> Prevent this by breaking progress bars after the title once they
> exceed the width of the terminal, so the counter and optional
> percentage and throughput, i.e. all changing parts, are on the last
> line.  Subsequent updates will from then on only refresh the changing
> parts, but not the title, and it will look like this:
>
>   $ LANG=es_ES.UTF-8 ~/src/git/git commit-graph write
>   Encontrando commits para commit graph entre los objetos empaquetados:
>     100% (6584502/6584502), listo.
>   Calculando números de generación de commit graph: 100% (824705/824705), listo.
>   Escribiendo commit graph en 4 pasos: 100% (3298820/3298820), listo.
>
> Note that the number of columns in the terminal is cached by
> term_columns(), so this might not kick in when it should when a
> terminal window is resized while the operation is running.
> Furthermore, this change won't help if the terminal is so narrow that
> the counters don't fit on one line, but I would put this in the "If it
> hurts, don't do it" box.
>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
>  progress.c | 24 +++++++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/progress.c b/progress.c
> index b37a5398c5..36aaeea340 100644
> --- a/progress.c
> +++ b/progress.c
> @@ -8,7 +8,7 @@
>   * published by the Free Software Foundation.
>   */
>
> -#include "git-compat-util.h"
> +#include "cache.h"
>  #include "gettext.h"
>  #include "progress.h"
>  #include "strbuf.h"
> @@ -37,6 +37,8 @@ struct progress {
>         struct throughput *throughput;
>         uint64_t start_ns;
>         struct strbuf counters_sb;
> +       int title_len;
> +       int split;
>  };
>
>  static volatile sig_atomic_t progress_update;
> @@ -119,8 +121,22 @@ static void display(struct progress *progress, uint64_t n, const char *done)
>                 const char *eol = done ? done : "\r";
>                 int clear_len = counters_sb->len < last_count_len ?
>                                 last_count_len - counters_sb->len : 0;
> -               fprintf(stderr, "%s: %s%-*s", progress->title,
> -                       counters_sb->buf, clear_len, eol);
> +               int cols = term_columns();
> +
> +               if (progress->split) {
> +                       fprintf(stderr, "  %s%-*s", counters_sb->buf, clear_len,
> +                               eol);
> +               } else if (!done &&
> +                          cols < progress->title_len + counters_sb->len + 2) {
> +                       clear_len = progress->title_len + 1 < cols ?
> +                                   cols - progress->title_len - 1 : 0;
> +                       fprintf(stderr, "%s:%*s\n  %s%s", progress->title,
> +                                       clear_len, "", counters_sb->buf, eol);
> +                       progress->split = 1;
> +               } else {
> +                       fprintf(stderr, "%s: %s%-*s", progress->title,
> +                               counters_sb->buf, clear_len, eol);
> +               }
>                 fflush(stderr);
>                 progress_update = 0;
>         }
> @@ -225,6 +241,8 @@ static struct progress *start_progress_delay(const char *title, uint64_t total,
>         progress->throughput = NULL;
>         progress->start_ns = getnanotime();
>         strbuf_init(&progress->counters_sb, 0);
> +       progress->title_len = strlen(title);

I think you need utf8_strwidth() so that the "cols < title_len"
comparison above works for multibyte encoding too.

> +       progress->split = 0;
>         set_progress_signal();
>         return progress;
>  }
> --
> 2.21.0.539.g07239c3a71.dirty
>


-- 
Duy

Re: [PATCH 2/5] progress: return early when in the background

[Ævar Arnfjörð Bjarmason]

On Mon, Mar 25 2019, SZEDER Gábor wrote:

> When a git process runs in the background, it doesn't display
> progress, only the final "done" line [1].  The condition to check that
> are a bit too deep in the display() function, and thus it calculates
> the progress percentage even when no progress will be displayed
> anyway.
>
> Restructure the display() function to return early when we are in the
> background, which prevents the unnecessary progress percentae
> calculation, and make the function look a bit better by losing one
> level of indentation.
>
> [1] 85cb8906f0 (progress: no progress in background, 2015-04-13)

CC-ing the author of that patch.

> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
>  progress.c | 26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)
>
> diff --git a/progress.c b/progress.c
> index 02a20e7d58..b57c0dae16 100644
> --- a/progress.c
> +++ b/progress.c
> @@ -86,28 +86,30 @@ static void display(struct progress *progress, uint64_t n, const char *done)
>  		return;
>
>  	progress->last_value = n;
> +
> +	if (!is_foreground_fd(fileno(stderr)) && !done) {
> +		progress_update = 0;
> +		return;
> +	}
> +
>  	tp = (progress->throughput) ? progress->throughput->display.buf : "";
>  	eol = done ? done : "   \r";
>  	if (progress->total) {
>  		unsigned percent = n * 100 / progress->total;
>  		if (percent != progress->last_percent || progress_update) {
>  			progress->last_percent = percent;
> -			if (is_foreground_fd(fileno(stderr)) || done) {
> -				fprintf(stderr, "%s: %3u%% (%"PRIuMAX"/%"PRIuMAX")%s%s",
> -					progress->title, percent,
> -					(uintmax_t)n, (uintmax_t)progress->total,
> -					tp, eol);
> -				fflush(stderr);
> -			}
> +			fprintf(stderr, "%s: %3u%% (%"PRIuMAX"/%"PRIuMAX")%s%s",
> +				progress->title, percent,
> +				(uintmax_t)n, (uintmax_t)progress->total,
> +				tp, eol);
> +			fflush(stderr);
>  			progress_update = 0;
>  			return;
>  		}
>  	} else if (progress_update) {
> -		if (is_foreground_fd(fileno(stderr)) || done) {
> -			fprintf(stderr, "%s: %"PRIuMAX"%s%s",
> -				progress->title, (uintmax_t)n, tp, eol);
> -			fflush(stderr);
> -		}
> +		fprintf(stderr, "%s: %"PRIuMAX"%s%s",
> +			progress->title, (uintmax_t)n, tp, eol);
> +		fflush(stderr);
>  		progress_update = 0;
>  		return;
>  	}

This patch looks good, just notes for potential follow-up:

 * Is the "is_foreground_fd(fileno(stderr))" case worth moving into
   start_progress_delay() & setting a variable? It's a few C lib calls &
   potential syscall (getpid(...)).

 * Is that "|| done" part in the "progress_update" case something that
   needs to happen? I.e. can we entirely skip the "setup signal handler"
   part in start_progress_delay() if we detect that we're not in the
   foreground, and then rely on the stop_progress() call to print the
   "done"?

   Although we set "progress_update = 1" in stop_progress_msg(), so it's
   not *just* the signal handler but also us "faking" it, and we'd still
   need to stash away "progress->last_value = n" in display() in that
   backgrounding case.

   So maybe it's as simple as it's going to get.

Re: [PATCH 5/5] progress: break too long progress bar lines

[SZEDER Gábor]

On Mon, Mar 25, 2019 at 06:02:13PM +0700, Duy Nguyen wrote:
> > @@ -225,6 +241,8 @@ static struct progress *start_progress_delay(const char *title, uint64_t total,
> >         progress->throughput = NULL;
> >         progress->start_ns = getnanotime();
> >         strbuf_init(&progress->counters_sb, 0);
> > +       progress->title_len = strlen(title);
> 
> I think you need utf8_strwidth() so that the "cols < title_len"
> comparison above works for multibyte encoding too.

Oh, indeed.  I remember seeing the Chinese translation and wondering
about it, but then completely forgot by the time I got around to it.

> > +       progress->split = 0;
> >         set_progress_signal();
> >         return progress;
> >  }
> > --
> > 2.21.0.539.g07239c3a71.dirty
> >
> 
> 
> -- 
> Duy

Re: [PATCH 2/5] progress: return early when in the background

[SZEDER Gábor]

On Mon, Mar 25, 2019 at 12:08:47PM +0100, Ævar Arnfjörð Bjarmason wrote:
> 
> On Mon, Mar 25 2019, SZEDER Gábor wrote:
> 
> > When a git process runs in the background, it doesn't display
> > progress, only the final "done" line [1].  The condition to check that
> > are a bit too deep in the display() function, and thus it calculates
> > the progress percentage even when no progress will be displayed
> > anyway.
> >
> > Restructure the display() function to return early when we are in the
> > background, which prevents the unnecessary progress percentae
> > calculation, and make the function look a bit better by losing one
> > level of indentation.
> >
> > [1] 85cb8906f0 (progress: no progress in background, 2015-04-13)
> 
> CC-ing the author of that patch.
> 
> > Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> > ---
> >  progress.c | 26 ++++++++++++++------------
> >  1 file changed, 14 insertions(+), 12 deletions(-)
> >
> > diff --git a/progress.c b/progress.c
> > index 02a20e7d58..b57c0dae16 100644
> > --- a/progress.c
> > +++ b/progress.c
> > @@ -86,28 +86,30 @@ static void display(struct progress *progress, uint64_t n, const char *done)
> >  		return;
> >
> >  	progress->last_value = n;
> > +
> > +	if (!is_foreground_fd(fileno(stderr)) && !done) {
> > +		progress_update = 0;
> > +		return;
> > +	}
> > +
> >  	tp = (progress->throughput) ? progress->throughput->display.buf : "";
> >  	eol = done ? done : "   \r";
> >  	if (progress->total) {
> >  		unsigned percent = n * 100 / progress->total;
> >  		if (percent != progress->last_percent || progress_update) {
> >  			progress->last_percent = percent;
> > -			if (is_foreground_fd(fileno(stderr)) || done) {
> > -				fprintf(stderr, "%s: %3u%% (%"PRIuMAX"/%"PRIuMAX")%s%s",
> > -					progress->title, percent,
> > -					(uintmax_t)n, (uintmax_t)progress->total,
> > -					tp, eol);
> > -				fflush(stderr);
> > -			}
> > +			fprintf(stderr, "%s: %3u%% (%"PRIuMAX"/%"PRIuMAX")%s%s",
> > +				progress->title, percent,
> > +				(uintmax_t)n, (uintmax_t)progress->total,
> > +				tp, eol);
> > +			fflush(stderr);
> >  			progress_update = 0;
> >  			return;
> >  		}
> >  	} else if (progress_update) {
> > -		if (is_foreground_fd(fileno(stderr)) || done) {
> > -			fprintf(stderr, "%s: %"PRIuMAX"%s%s",
> > -				progress->title, (uintmax_t)n, tp, eol);
> > -			fflush(stderr);
> > -		}
> > +		fprintf(stderr, "%s: %"PRIuMAX"%s%s",
> > +			progress->title, (uintmax_t)n, tp, eol);
> > +		fflush(stderr);
> >  		progress_update = 0;
> >  		return;
> >  	}
> 
> This patch looks good, just notes for potential follow-up:
> 
>  * Is the "is_foreground_fd(fileno(stderr))" case worth moving into
>    start_progress_delay() & setting a variable? It's a few C lib calls &
>    potential syscall (getpid(...)).

It depends on whether you consider the following case worth caring
about:

  $ git long-cmd
  <shows progress>
  Ctrl-Z!
  $ bg
  <silent>
  $ fg
  <shows progress>

Or:

  $ git long-cmd &
  <silent>
  $ fg
  <shows progress>

By moving the is_foreground_fd() check to start_progress_delay() and
caching its result, in the first case we would print progress even
after the process is sent to the background, while in the second we
wouldn't print progress even after the initially backgrounded process
is brought to the foreground.

I think the current behavior makes sense (though I'm not quite sure
about printing the final "done" line, as I think I would be annoyed if
it were printed from the background process while I was typing a
longer command... but I don't run git commands in the background in
the first place)

>  * Is that "|| done" part in the "progress_update" case something that
>    needs to happen? I.e. can we entirely skip the "setup signal handler"
>    part in start_progress_delay() if we detect that we're not in the
>    foreground, and then rely on the stop_progress() call to print the
>    "done"?

This, too, depends on how (or whether at all) we want to handle the
user sending the process to the background and bringing it back.

>    Although we set "progress_update = 1" in stop_progress_msg(), so it's
>    not *just* the signal handler but also us "faking" it, and we'd still
>    need to stash away "progress->last_value = n" in display() in that
>    backgrounding case.
> 
>    So maybe it's as simple as it's going to get.

Re: [PATCH 2/5] progress: return early when in the background

[Jeff King]

On Mon, Mar 25, 2019 at 11:38:41AM +0100, SZEDER Gábor wrote:

> diff --git a/progress.c b/progress.c
> index 02a20e7d58..b57c0dae16 100644
> --- a/progress.c
> +++ b/progress.c
> @@ -86,28 +86,30 @@ static void display(struct progress *progress, uint64_t n, const char *done)
>  		return;
>  
>  	progress->last_value = n;
> +
> +	if (!is_foreground_fd(fileno(stderr)) && !done) {
> +		progress_update = 0;
> +		return;
> +	}
> +

Moving it here causes a measurable slowdown for:

  git rev-list --progress=foo --objects --all

This function gets called for every single increment of the progress
counter. Whereas in its old location:

>  	tp = (progress->throughput) ? progress->throughput->display.buf : "";
>  	eol = done ? done : "   \r";
>  	if (progress->total) {
>  		unsigned percent = n * 100 / progress->total;
>  		if (percent != progress->last_percent || progress_update) {
>  			progress->last_percent = percent;
> -			if (is_foreground_fd(fileno(stderr)) || done) {
> -				fprintf(stderr, "%s: %3u%% (%"PRIuMAX"/%"PRIuMAX")%s%s",
> -					progress->title, percent,
> -					(uintmax_t)n, (uintmax_t)progress->total,
> -					tp, eol);
> -				fflush(stderr);
> -			}

It was only triggered when we accumulated enough increments to print. So
we save a few instructions in the backgrounded case, but it costs us a
lot of extra syscalls in every other case.

According to "strace -c", the number of ioctls for that rev-list on
git.git went from 6 to 373,461. But more importantly, my best-of-five
timings went from 3.340s from 3.407s. That's only 2%, but it would be
nice not to pay it if we don't need to.

-Peff

Re: [PATCH 3/5] progress: assemble percentage and counters in a strbuf before printing

[Jeff King]

On Mon, Mar 25, 2019 at 11:38:42AM +0100, SZEDER Gábor wrote:

> The following patches in this series want to handle the progress bar's
> title and changing parts (i.e. the counter and the optional percentage
> and throughput combined) differently.
> 
> To prepare for those changes assemble the changing parts in a separate
> strbuf before printing.

Makes sense. Unlike the previous patch, we're already in the "slow path"
of the function here, so we don't need to worry about adding an extra
buffer (though do still think it's worth reusing the same buffer each
time, as you do here).

-Peff

Re: [PATCH 0/5] Progress display fixes

[Jeff King]

On Mon, Mar 25, 2019 at 11:38:39AM +0100, SZEDER Gábor wrote:

> This patch series fixes two progress display issues:
> 
>   - When showing throughput, and the both the total and the throughput
>     change units in the same update, than the previously shown
>     progress bar is not cleaned up properly:
> 
>       Receiving objects:  25% (2901/11603), 772.01 KiB | 733.00 KiB/s
>       Receiving objects:  27% (3133/11603), 1.43 MiB | 1.16 MiB/s   s
> 
>   - When the progress bar is longer than the width of the terminal,
>     then we end up with a bunch of truncated progress bar lines
>     scrolling past:
> 
>       $ LANG=es_ES.UTF-8 git commit-graph write
>       Encontrando commits para commit graph entre los objetos empaquetados:   2% (1599
>       Encontrando commits para commit graph entre los objetos empaquetados:   3% (1975
>       Encontrando commits para commit graph entre los objetos empaquetados:   4% (2633
>       Encontrando commits para commit graph entre los objetos empaquetados:   5% (3292
>       [...]
> 
> Patches 4 and 5 fix these two issues, while the first three are
> minor preparatory cleanups and refactorings.

With the exception of the points raised in the thread, these look good
to me. Seeing your later patches, I think it would not be too bad to
just stick the is_foreground_fd() check inside the "if (update)" block
you introduce later.

-Peff

Re: [PATCH 2/5] progress: return early when in the background

[Luke Mewburn]

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

On 19-03-25 12:39, SZEDER Gábor wrote:
  | On Mon, Mar 25, 2019 at 12:08:47PM +0100, Ævar Arnfjörð Bjarmason wrote:
  | > 
  | > On Mon, Mar 25 2019, SZEDER Gábor wrote:
  | > 
  | > > When a git process runs in the background, it doesn't display
  | > > progress, only the final "done" line [1].  The condition to check that
  | > > are a bit too deep in the display() function, and thus it calculates
  | > > the progress percentage even when no progress will be displayed
  | > > anyway.
  | > >
  | > > Restructure the display() function to return early when we are in the
  | > > background, which prevents the unnecessary progress percentae
  | > > calculation, and make the function look a bit better by losing one
  | > > level of indentation.
  | > >
  | > > [1] 85cb8906f0 (progress: no progress in background, 2015-04-13)
  | > 
  | > CC-ing the author of that patch.
  | > 
  | > > Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
  | > > ---
  | > >  progress.c | 26 ++++++++++++++------------
  | > >  1 file changed, 14 insertions(+), 12 deletions(-)
  | > >
  | > > diff --git a/progress.c b/progress.c
  | > > index 02a20e7d58..b57c0dae16 100644
  | > > --- a/progress.c
  | > > +++ b/progress.c
  | > > @@ -86,28 +86,30 @@ static void display(struct progress *progress, uint64_t n, const char *done)
  | > >  		return;
  | > >
  | > >  	progress->last_value = n;
  | > > +
  | > > +	if (!is_foreground_fd(fileno(stderr)) && !done) {
  | > > +		progress_update = 0;
  | > > +		return;
  | > > +	}
  | > > +
  | > >  	tp = (progress->throughput) ? progress->throughput->display.buf : "";
  | > >  	eol = done ? done : "   \r";
  | > >  	if (progress->total) {
  | > >  		unsigned percent = n * 100 / progress->total;
  | > >  		if (percent != progress->last_percent || progress_update) {
  | > >  			progress->last_percent = percent;
  | > > -			if (is_foreground_fd(fileno(stderr)) || done) {
  | > > -				fprintf(stderr, "%s: %3u%% (%"PRIuMAX"/%"PRIuMAX")%s%s",
  | > > -					progress->title, percent,
  | > > -					(uintmax_t)n, (uintmax_t)progress->total,
  | > > -					tp, eol);
  | > > -				fflush(stderr);
  | > > -			}
  | > > +			fprintf(stderr, "%s: %3u%% (%"PRIuMAX"/%"PRIuMAX")%s%s",
  | > > +				progress->title, percent,
  | > > +				(uintmax_t)n, (uintmax_t)progress->total,
  | > > +				tp, eol);
  | > > +			fflush(stderr);
  | > >  			progress_update = 0;
  | > >  			return;
  | > >  		}
  | > >  	} else if (progress_update) {
  | > > -		if (is_foreground_fd(fileno(stderr)) || done) {
  | > > -			fprintf(stderr, "%s: %"PRIuMAX"%s%s",
  | > > -				progress->title, (uintmax_t)n, tp, eol);
  | > > -			fflush(stderr);
  | > > -		}
  | > > +		fprintf(stderr, "%s: %"PRIuMAX"%s%s",
  | > > +			progress->title, (uintmax_t)n, tp, eol);
  | > > +		fflush(stderr);
  | > >  		progress_update = 0;
  | > >  		return;
  | > >  	}
  | > 
  | > This patch looks good, just notes for potential follow-up:
  | > 
  | >  * Is the "is_foreground_fd(fileno(stderr))" case worth moving into
  | >    start_progress_delay() & setting a variable? It's a few C lib calls &
  | >    potential syscall (getpid(...)).
  | 
  | It depends on whether you consider the following case worth caring
  | about:
  | 
  |   $ git long-cmd
  |   <shows progress>
  |   Ctrl-Z!
  |   $ bg
  |   <silent>
  |   $ fg
  |   <shows progress>
  | 
  | Or:
  | 
  |   $ git long-cmd &
  |   <silent>
  |   $ fg
  |   <shows progress>
  | 
  | By moving the is_foreground_fd() check to start_progress_delay() and
  | caching its result, in the first case we would print progress even
  | after the process is sent to the background, while in the second we
  | wouldn't print progress even after the initially backgrounded process
  | is brought to the foreground.
  | 
  | I think the current behavior makes sense (though I'm not quite sure
  | about printing the final "done" line, as I think I would be annoyed if
  | it were printed from the background process while I was typing a
  | longer command... but I don't run git commands in the background in
  | the first place)

You've described the current behaviour as I intended it
in the original patch. I.e,:
- Display progress if foreground.
- Suppress output if background.
- Check the foreground/background state each update in case it changed.

I based that on other tools that also dynamically change their
output/progress behaviour whether in the foreground or background.

Regarding the final "done" line; I think that's a matter of
personal preference. I'm not too fussed if that was changed
so that "done" isn't printed if in the background.


  | 
  | >  * Is that "|| done" part in the "progress_update" case something that
  | >    needs to happen? I.e. can we entirely skip the "setup signal handler"
  | >    part in start_progress_delay() if we detect that we're not in the
  | >    foreground, and then rely on the stop_progress() call to print the
  | >    "done"?
  | 
  | This, too, depends on how (or whether at all) we want to handle the
  | user sending the process to the background and bringing it back.
  | 
  | >    Although we set "progress_update = 1" in stop_progress_msg(), so it's
  | >    not *just* the signal handler but also us "faking" it, and we'd still
  | >    need to stash away "progress->last_value = n" in display() in that
  | >    backgrounding case.
  | > 
  | >    So maybe it's as simple as it's going to get.
  | 



regards,
Luke.

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 3/5] progress: assemble percentage and counters in a strbuf before printing

[SZEDER Gábor]

On Tue, Mar 26, 2019 at 01:45:41AM -0400, Jeff King wrote:
> On Mon, Mar 25, 2019 at 11:38:42AM +0100, SZEDER Gábor wrote:
> 
> > The following patches in this series want to handle the progress bar's
> > title and changing parts (i.e. the counter and the optional percentage
> > and throughput combined) differently.
> > 
> > To prepare for those changes assemble the changing parts in a separate
> > strbuf before printing.
> 
> Makes sense. Unlike the previous patch, we're already in the "slow path"
> of the function here, so we don't need to worry about adding an extra
> buffer (though do still think it's worth reusing the same buffer each
> time, as you do here).

The commit message doesn't mention this, but the next patch needs the
length of the previously displayed progress bar to properly clean up
its remnants.  Or the length of its changing parts anyway.  So I could
either add a 'prev_len' field to 'struct progress', or the whole
strbuf.  The strbuf containing the throughput is already stored in
there and reused, so I just followed suit.

Re: [PATCH 3/5] progress: assemble percentage and counters in a strbuf before printing

[Jeff King]

On Wed, Mar 27, 2019 at 11:24:51AM +0100, SZEDER Gábor wrote:

> On Tue, Mar 26, 2019 at 01:45:41AM -0400, Jeff King wrote:
> > On Mon, Mar 25, 2019 at 11:38:42AM +0100, SZEDER Gábor wrote:
> > 
> > > The following patches in this series want to handle the progress bar's
> > > title and changing parts (i.e. the counter and the optional percentage
> > > and throughput combined) differently.
> > > 
> > > To prepare for those changes assemble the changing parts in a separate
> > > strbuf before printing.
> > 
> > Makes sense. Unlike the previous patch, we're already in the "slow path"
> > of the function here, so we don't need to worry about adding an extra
> > buffer (though do still think it's worth reusing the same buffer each
> > time, as you do here).
> 
> The commit message doesn't mention this, but the next patch needs the
> length of the previously displayed progress bar to properly clean up
> its remnants.  Or the length of its changing parts anyway.  So I could
> either add a 'prev_len' field to 'struct progress', or the whole
> strbuf.  The strbuf containing the throughput is already stored in
> there and reused, so I just followed suit.

Ah, right, that makes perfect sense. It is doubly a good idea, then. :)

-Peff

[PATCH v2 0/4] Progress display fixes

[SZEDER Gábor]

This patch series fixes two progress display issues:

  - When showing throughput, and the both the total and the throughput
    change units in the same update, than the previously shown
    progress bar is not cleaned up properly:

      Receiving objects:  25% (2901/11603), 772.01 KiB | 733.00 KiB/s
      Receiving objects:  27% (3133/11603), 1.43 MiB | 1.16 MiB/s   s

  - When the progress bar is longer than the width of the terminal,
    then we end up with a bunch of truncated progress bar lines
    scrolling past:

      $ LANG=es_ES.UTF-8 git commit-graph write
      Encontrando commits para commit graph entre los objetos empaquetados:   2% (1599
      Encontrando commits para commit graph entre los objetos empaquetados:   3% (1975
      Encontrando commits para commit graph entre los objetos empaquetados:   4% (2633
      Encontrando commits para commit graph entre los objetos empaquetados:   5% (3292
      [...]

Patches 3 and 4 fix these two issues, while the first three are
minor preparatory cleanups and refactorings.


Changes since v1:

  - Use utf8_strwidth().

  - Dropped patch 2/5 (progress: return early when in the background).

    Consequently, the new patch 2/4 (progress: assemble percentage and
    counters in a strbuf before printing; was patch 3/5 in v1) moves
    the is_foreground_fd() check around a bit, resulting in enough
    changes that range-diff can't match the new patch 3/4 to the old
    4/5.  With increased '--creation-factor' it's able to line up
    those two patches, and shows the updates to the commit message,
    but the resulting diff-of-a-diff looks utterly unreadable to me.
    I've included an interdiff as well, as I find it much more
    telling.

  - Commit message updates.


SZEDER Gábor (4):
  progress: make display_progress() return void
  progress: assemble percentage and counters in a strbuf before printing
  progress: clear previous progress update dynamically
  progress: break too long progress bar lines

 progress.c | 73 +++++++++++++++++++++++++++++++++++++++---------------
 progress.h |  2 +-
 2 files changed, 54 insertions(+), 21 deletions(-)

Interdiff:
diff --git a/progress.c b/progress.c
index 97bc4b04e8..e28ccdafd2 100644
--- a/progress.c
+++ b/progress.c
@@ -86,19 +86,13 @@ static void display(struct progress *progress, uint64_t n, const char *done)
 {
 	const char *tp;
 	struct strbuf *counters_sb = &progress->counters_sb;
-	int update = 0;
+	int show_update = 0;
 	int last_count_len = counters_sb->len;
 
 	if (progress->delay && (!progress_update || --progress->delay))
 		return;
 
 	progress->last_value = n;
-
-	if (!is_foreground_fd(fileno(stderr)) && !done) {
-		progress_update = 0;
-		return;
-	}
-
 	tp = (progress->throughput) ? progress->throughput->display.buf : "";
 	if (progress->total) {
 		unsigned percent = n * 100 / progress->total;
@@ -110,35 +104,39 @@ static void display(struct progress *progress, uint64_t n, const char *done)
 				    "%3u%% (%"PRIuMAX"/%"PRIuMAX")%s", percent,
 				    (uintmax_t)n, (uintmax_t)progress->total,
 				    tp);
-			update = 1;
+			show_update = 1;
 		}
 	} else if (progress_update) {
 		strbuf_reset(counters_sb);
 		strbuf_addf(counters_sb, "%"PRIuMAX"%s", (uintmax_t)n, tp);
-		update = 1;
+		show_update = 1;
 	}
 
-	if (update) {
-		const char *eol = done ? done : "\r";
-		int clear_len = counters_sb->len < last_count_len ?
-				last_count_len - counters_sb->len : 0;
-		int cols = term_columns();
-
-		if (progress->split) {
-			fprintf(stderr, "  %s%-*s", counters_sb->buf, clear_len,
-				eol);
-		} else if (!done &&
-			   cols < progress->title_len + counters_sb->len + 2) {
-			clear_len = progress->title_len + 1 < cols ?
-				    cols - progress->title_len - 1 : 0;
-			fprintf(stderr, "%s:%*s\n  %s%s", progress->title,
-					clear_len, "", counters_sb->buf, eol);
-			progress->split = 1;
-		} else {
-			fprintf(stderr, "%s: %s%-*s", progress->title,
-				counters_sb->buf, clear_len, eol);
+	if (show_update) {
+		if (is_foreground_fd(fileno(stderr)) || done) {
+			const char *eol = done ? done : "\r";
+			int clear_len = counters_sb->len < last_count_len ?
+					last_count_len - counters_sb->len : 0;
+			int progress_line_len = progress->title_len +
+						counters_sb->len + 2;
+			int cols = term_columns();
+
+			if (progress->split) {
+				fprintf(stderr, "  %s%-*s", counters_sb->buf,
+					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, clear_len, "",
+					counters_sb->buf, eol);
+				progress->split = 1;
+			} else {
+				fprintf(stderr, "%s: %s%-*s", progress->title,
+					counters_sb->buf, clear_len, eol);
+			}
+			fflush(stderr);
 		}
-		fflush(stderr);
 		progress_update = 0;
 	}
 
@@ -242,7 +240,7 @@ static struct progress *start_progress_delay(const char *title, uint64_t total,
 	progress->throughput = NULL;
 	progress->start_ns = getnanotime();
 	strbuf_init(&progress->counters_sb, 0);
-	progress->title_len = strlen(title);
+	progress->title_len = utf8_strwidth(title);
 	progress->split = 0;
 	set_progress_signal();
 	return progress;
Range-diff:
1:  dea36bd2a7 = 1:  dea36bd2a7 progress: make display_progress() return void
2:  159a0b9561 < -:  ---------- progress: return early when in the background
3:  ecd6b0fd68 ! 2:  97de2a98a0 progress: assemble percentage and counters in a strbuf before printing
    @@ -4,10 +4,11 @@
     
         The following patches in this series want to handle the progress bar's
         title and changing parts (i.e. the counter and the optional percentage
    -    and throughput combined) differently.
    +    and throughput combined) differently, and need to know the length
    +    of the changing parts of the previously displayed progress bar.
     
         To prepare for those changes assemble the changing parts in a separate
    -    strbuf before printing.
    +    strbuf kept in 'struct progress' before printing.
     
         Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
     
    @@ -29,24 +30,25 @@
     -	const char *eol, *tp;
     +	const char *tp;
     +	struct strbuf *counters_sb = &progress->counters_sb;
    -+	int update = 0;
    ++	int show_update = 0;
      
      	if (progress->delay && (!progress_update || --progress->delay))
      		return;
    -@@
    - 	}
      
    + 	progress->last_value = n;
      	tp = (progress->throughput) ? progress->throughput->display.buf : "";
     -	eol = done ? done : "   \r";
      	if (progress->total) {
      		unsigned percent = n * 100 / progress->total;
      		if (percent != progress->last_percent || progress_update) {
      			progress->last_percent = percent;
    --			fprintf(stderr, "%s: %3u%% (%"PRIuMAX"/%"PRIuMAX")%s%s",
    --				progress->title, percent,
    --				(uintmax_t)n, (uintmax_t)progress->total,
    --				tp, eol);
    --			fflush(stderr);
    +-			if (is_foreground_fd(fileno(stderr)) || done) {
    +-				fprintf(stderr, "%s: %3u%% (%"PRIuMAX"/%"PRIuMAX")%s%s",
    +-					progress->title, percent,
    +-					(uintmax_t)n, (uintmax_t)progress->total,
    +-					tp, eol);
    +-				fflush(stderr);
    +-			}
     -			progress_update = 0;
     -			return;
     +
    @@ -55,22 +57,24 @@
     +				    "%3u%% (%"PRIuMAX"/%"PRIuMAX")%s", percent,
     +				    (uintmax_t)n, (uintmax_t)progress->total,
     +				    tp);
    -+			update = 1;
    ++			show_update = 1;
      		}
      	} else if (progress_update) {
    --		fprintf(stderr, "%s: %"PRIuMAX"%s%s",
    --			progress->title, (uintmax_t)n, tp, eol);
     +		strbuf_reset(counters_sb);
     +		strbuf_addf(counters_sb, "%"PRIuMAX"%s", (uintmax_t)n, tp);
    -+		update = 1;
    ++		show_update = 1;
     +	}
     +
    -+	if (update) {
    -+		const char *eol = done ? done : "   \r";
    ++	if (show_update) {
    + 		if (is_foreground_fd(fileno(stderr)) || done) {
    +-			fprintf(stderr, "%s: %"PRIuMAX"%s%s",
    +-				progress->title, (uintmax_t)n, tp, eol);
    ++			const char *eol = done ? done : "   \r";
     +
    -+		fprintf(stderr, "%s: %s%s", progress->title, counters_sb->buf,
    -+			eol);
    - 		fflush(stderr);
    ++			fprintf(stderr, "%s: %s%s", progress->title,
    ++				counters_sb->buf, eol);
    + 			fflush(stderr);
    + 		}
      		progress_update = 0;
     -		return;
      	}
4:  e360365f18 ! 3:  edfe0157a7 progress: clear previous progress update dynamically
    @@ -10,7 +10,7 @@
         Alas, covering only three characters is not quite enough: when both
         the total and the throughput happen to change units from KiB to MiB in
         the same update, then the progress bar's length is shortened by four
    -    characters:
    +    characters (or maybe even more!):
     
           Receiving objects:  25% (2901/11603), 772.01 KiB | 733.00 KiB/s
           Receiving objects:  27% (3133/11603), 1.43 MiB | 1.16 MiB/s   s
    @@ -21,11 +21,10 @@
         the length of the current progress bar with the previous one, and
         cover up as many characters as needed.
     
    -    Sure, it would be much simpler to just print four spaces instead of
    -    three at the end of the progress bar, but this approach is more
    -    future-proof, and it won't print extra spaces when none are needed,
    -    notably when the progress bar doesn't show throughput and thus never
    -    shrinks.
    +    Sure, it would be much simpler to just print more spaces at the end of
    +    the progress bar, but this approach is more future-proof, and it won't
    +    print extra spaces when none are needed, notably when the progress bar
    +    doesn't show throughput and thus never shrinks.
     
         Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
     
    @@ -35,24 +34,24 @@
     @@
      	const char *tp;
      	struct strbuf *counters_sb = &progress->counters_sb;
    - 	int update = 0;
    + 	int show_update = 0;
     +	int last_count_len = counters_sb->len;
      
      	if (progress->delay && (!progress_update || --progress->delay))
      		return;
     @@
    - 	}
      
    - 	if (update) {
    --		const char *eol = done ? done : "   \r";
    + 	if (show_update) {
    + 		if (is_foreground_fd(fileno(stderr)) || done) {
    +-			const char *eol = done ? done : "   \r";
     -
    --		fprintf(stderr, "%s: %s%s", progress->title, counters_sb->buf,
    --			eol);
    -+		const char *eol = done ? done : "\r";
    -+		int clear_len = counters_sb->len < last_count_len ?
    -+				last_count_len - counters_sb->len : 0;
    -+		fprintf(stderr, "%s: %s%-*s", progress->title,
    -+			counters_sb->buf, clear_len, eol);
    - 		fflush(stderr);
    +-			fprintf(stderr, "%s: %s%s", progress->title,
    +-				counters_sb->buf, eol);
    ++			const char *eol = done ? done : "\r";
    ++			int clear_len = counters_sb->len < last_count_len ?
    ++					last_count_len - counters_sb->len : 0;
    ++			fprintf(stderr, "%s: %s%-*s", progress->title,
    ++				counters_sb->buf, clear_len, eol);
    + 			fflush(stderr);
    + 		}
      		progress_update = 0;
    - 	}
5:  cbd3be1c6d ! 4:  d53de231ee progress: break too long progress bar lines
    @@ -69,35 +69,37 @@
      
      static volatile sig_atomic_t progress_update;
     @@
    - 		const char *eol = done ? done : "\r";
    - 		int clear_len = counters_sb->len < last_count_len ?
    - 				last_count_len - counters_sb->len : 0;
    --		fprintf(stderr, "%s: %s%-*s", progress->title,
    --			counters_sb->buf, clear_len, eol);
    -+		int cols = term_columns();
    + 			const char *eol = done ? done : "\r";
    + 			int clear_len = counters_sb->len < last_count_len ?
    + 					last_count_len - counters_sb->len : 0;
    +-			fprintf(stderr, "%s: %s%-*s", progress->title,
    +-				counters_sb->buf, clear_len, eol);
    ++			int progress_line_len = progress->title_len +
    ++						counters_sb->len + 2;
    ++			int cols = term_columns();
     +
    -+		if (progress->split) {
    -+			fprintf(stderr, "  %s%-*s", counters_sb->buf, clear_len,
    -+				eol);
    -+		} else if (!done &&
    -+			   cols < progress->title_len + counters_sb->len + 2) {
    -+			clear_len = progress->title_len + 1 < cols ?
    -+				    cols - progress->title_len - 1 : 0;
    -+			fprintf(stderr, "%s:%*s\n  %s%s", progress->title,
    -+					clear_len, "", counters_sb->buf, eol);
    -+			progress->split = 1;
    -+		} else {
    -+			fprintf(stderr, "%s: %s%-*s", progress->title,
    -+				counters_sb->buf, clear_len, eol);
    -+		}
    - 		fflush(stderr);
    ++			if (progress->split) {
    ++				fprintf(stderr, "  %s%-*s", counters_sb->buf,
    ++					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, clear_len, "",
    ++					counters_sb->buf, eol);
    ++				progress->split = 1;
    ++			} else {
    ++				fprintf(stderr, "%s: %s%-*s", progress->title,
    ++					counters_sb->buf, clear_len, eol);
    ++			}
    + 			fflush(stderr);
    + 		}
      		progress_update = 0;
    - 	}
     @@
      	progress->throughput = NULL;
      	progress->start_ns = getnanotime();
      	strbuf_init(&progress->counters_sb, 0);
    -+	progress->title_len = strlen(title);
    ++	progress->title_len = utf8_strwidth(title);
     +	progress->split = 0;
      	set_progress_signal();
      	return progress;
-- 
2.21.0.539.g07239c3a71.dirty

[PATCH v2 1/4] progress: make display_progress() return void

[SZEDER Gábor]

Ever since the progress infrastructure was introduced in 96a02f8f6d
(common progress display support, 2007-04-18), display_progress() has
returned an int, telling callers whether it updated the progress bar
or not.  However, this is:

  - useless, because over the last dozen years there has never been a
    single caller that cared about that return value.

  - not quite true, because it doesn't print a progress bar when
    running in the background, yet it returns 1; see 85cb8906f0
    (progress: no progress in background, 2015-04-13).

The related display_throughput() function returned void already upon
its introduction in cf84d51c43 (add throughput to progress display,
2007-10-30).

Let's make display_progress() return void, too.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 progress.c | 15 ++++++++-------
 progress.h |  2 +-
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/progress.c b/progress.c
index 5a99c9fbf0..02a20e7d58 100644
--- a/progress.c
+++ b/progress.c
@@ -78,12 +78,12 @@ static int is_foreground_fd(int fd)
 	return tpgrp < 0 || tpgrp == getpgid(0);
 }
 
-static int display(struct progress *progress, uint64_t n, const char *done)
+static void display(struct progress *progress, uint64_t n, const char *done)
 {
 	const char *eol, *tp;
 
 	if (progress->delay && (!progress_update || --progress->delay))
-		return 0;
+		return;
 
 	progress->last_value = n;
 	tp = (progress->throughput) ? progress->throughput->display.buf : "";
@@ -100,7 +100,7 @@ static int display(struct progress *progress, uint64_t n, const char *done)
 				fflush(stderr);
 			}
 			progress_update = 0;
-			return 1;
+			return;
 		}
 	} else if (progress_update) {
 		if (is_foreground_fd(fileno(stderr)) || done) {
@@ -109,10 +109,10 @@ static int display(struct progress *progress, uint64_t n, const char *done)
 			fflush(stderr);
 		}
 		progress_update = 0;
-		return 1;
+		return;
 	}
 
-	return 0;
+	return;
 }
 
 static void throughput_string(struct strbuf *buf, uint64_t total,
@@ -188,9 +188,10 @@ void display_throughput(struct progress *progress, uint64_t total)
 		display(progress, progress->last_value, NULL);
 }
 
-int display_progress(struct progress *progress, uint64_t n)
+void display_progress(struct progress *progress, uint64_t n)
 {
-	return progress ? display(progress, n, NULL) : 0;
+	if (progress)
+		display(progress, n, NULL);
 }
 
 static struct progress *start_progress_delay(const char *title, uint64_t total,
diff --git a/progress.h b/progress.h
index 70a4d4a0d6..59e40cc4fd 100644
--- a/progress.h
+++ b/progress.h
@@ -4,7 +4,7 @@
 struct progress;
 
 void display_throughput(struct progress *progress, uint64_t total);
-int display_progress(struct progress *progress, uint64_t n);
+void display_progress(struct progress *progress, uint64_t n);
 struct progress *start_progress(const char *title, uint64_t total);
 struct progress *start_delayed_progress(const char *title, uint64_t total);
 void stop_progress(struct progress **progress);
-- 
2.21.0.539.g07239c3a71.dirty

[PATCH v2 2/4] progress: assemble percentage and counters in a strbuf before printing

[SZEDER Gábor]

The following patches in this series want to handle the progress bar's
title and changing parts (i.e. the counter and the optional percentage
and throughput combined) differently, and need to know the length
of the changing parts of the previously displayed progress bar.

To prepare for those changes assemble the changing parts in a separate
strbuf kept in 'struct progress' before printing.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 progress.c | 37 +++++++++++++++++++++++--------------
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/progress.c b/progress.c
index 02a20e7d58..842db14b72 100644
--- a/progress.c
+++ b/progress.c
@@ -36,6 +36,7 @@ struct progress {
 	unsigned delay;
 	struct throughput *throughput;
 	uint64_t start_ns;
+	struct strbuf counters_sb;
 };
 
 static volatile sig_atomic_t progress_update;
@@ -80,36 +81,42 @@ static int is_foreground_fd(int fd)
 
 static void display(struct progress *progress, uint64_t n, const char *done)
 {
-	const char *eol, *tp;
+	const char *tp;
+	struct strbuf *counters_sb = &progress->counters_sb;
+	int show_update = 0;
 
 	if (progress->delay && (!progress_update || --progress->delay))
 		return;
 
 	progress->last_value = n;
 	tp = (progress->throughput) ? progress->throughput->display.buf : "";
-	eol = done ? done : "   \r";
 	if (progress->total) {
 		unsigned percent = n * 100 / progress->total;
 		if (percent != progress->last_percent || progress_update) {
 			progress->last_percent = percent;
-			if (is_foreground_fd(fileno(stderr)) || done) {
-				fprintf(stderr, "%s: %3u%% (%"PRIuMAX"/%"PRIuMAX")%s%s",
-					progress->title, percent,
-					(uintmax_t)n, (uintmax_t)progress->total,
-					tp, eol);
-				fflush(stderr);
-			}
-			progress_update = 0;
-			return;
+
+			strbuf_reset(counters_sb);
+			strbuf_addf(counters_sb,
+				    "%3u%% (%"PRIuMAX"/%"PRIuMAX")%s", percent,
+				    (uintmax_t)n, (uintmax_t)progress->total,
+				    tp);
+			show_update = 1;
 		}
 	} else if (progress_update) {
+		strbuf_reset(counters_sb);
+		strbuf_addf(counters_sb, "%"PRIuMAX"%s", (uintmax_t)n, tp);
+		show_update = 1;
+	}
+
+	if (show_update) {
 		if (is_foreground_fd(fileno(stderr)) || done) {
-			fprintf(stderr, "%s: %"PRIuMAX"%s%s",
-				progress->title, (uintmax_t)n, tp, eol);
+			const char *eol = done ? done : "   \r";
+
+			fprintf(stderr, "%s: %s%s", progress->title,
+				counters_sb->buf, eol);
 			fflush(stderr);
 		}
 		progress_update = 0;
-		return;
 	}
 
 	return;
@@ -211,6 +218,7 @@ static struct progress *start_progress_delay(const char *title, uint64_t total,
 	progress->delay = delay;
 	progress->throughput = NULL;
 	progress->start_ns = getnanotime();
+	strbuf_init(&progress->counters_sb, 0);
 	set_progress_signal();
 	return progress;
 }
@@ -254,6 +262,7 @@ void stop_progress_msg(struct progress **p_progress, const char *msg)
 		free(buf);
 	}
 	clear_progress_signal();
+	strbuf_release(&progress->counters_sb);
 	if (progress->throughput)
 		strbuf_release(&progress->throughput->display);
 	free(progress->throughput);
-- 
2.21.0.539.g07239c3a71.dirty

[PATCH v2 3/4] progress: clear previous progress update dynamically

[SZEDER Gábor]

When the progress bar includes throughput, its length can shorten as
the unit of display changes from KiB to MiB.  To cover up remnants of
the previous progress bar when such a change of units happens we
always print three spaces at the end of the progress bar.

Alas, covering only three characters is not quite enough: when both
the total and the throughput happen to change units from KiB to MiB in
the same update, then the progress bar's length is shortened by four
characters (or maybe even more!):

  Receiving objects:  25% (2901/11603), 772.01 KiB | 733.00 KiB/s
  Receiving objects:  27% (3133/11603), 1.43 MiB | 1.16 MiB/s   s

and a stray 's' is left behind.

So instead of hard-coding the three characters to cover, let's compare
the length of the current progress bar with the previous one, and
cover up as many characters as needed.

Sure, it would be much simpler to just print more spaces at the end of
the progress bar, but this approach is more future-proof, and it won't
print extra spaces when none are needed, notably when the progress bar
doesn't show throughput and thus never shrinks.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 progress.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/progress.c b/progress.c
index 842db14b72..3149ecd460 100644
--- a/progress.c
+++ b/progress.c
@@ -84,6 +84,7 @@ 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;
@@ -110,10 +111,11 @@ 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";
-
-			fprintf(stderr, "%s: %s%s", progress->title,
-				counters_sb->buf, eol);
+			const char *eol = done ? done : "\r";
+			int clear_len = counters_sb->len < last_count_len ?
+					last_count_len - counters_sb->len : 0;
+			fprintf(stderr, "%s: %s%-*s", progress->title,
+				counters_sb->buf, clear_len, eol);
 			fflush(stderr);
 		}
 		progress_update = 0;
-- 
2.21.0.539.g07239c3a71.dirty

[PATCH v2 4/4] progress: break too long progress bar lines

[SZEDER Gábor]

Some of the recently added progress indicators have quite long titles,
which might be even longer when translated to some languages, and when
they are shown while operating on bigger repositories, then the
progress bar grows longer than the default 80 column terminal width.

When the progress bar exceeds the width of the terminal it gets
line-wrapped, and after that the CR at the end doesn't return to the
beginning of the progress bar, but to the first column of its last
line.  Consequently, the first line of the previously shown progress
bar is not overwritten by the next, and we end up with a bunch of
truncated progress bar lines scrolling past:

  $ LANG=es_ES.UTF-8 git commit-graph write
  Encontrando commits para commit graph entre los objetos empaquetados:   2% (1599
  Encontrando commits para commit graph entre los objetos empaquetados:   3% (1975
  Encontrando commits para commit graph entre los objetos empaquetados:   4% (2633
  Encontrando commits para commit graph entre los objetos empaquetados:   5% (3292
  [...]

Prevent this by breaking progress bars after the title once they
exceed the width of the terminal, so the counter and optional
percentage and throughput, i.e. all changing parts, are on the last
line.  Subsequent updates will from then on only refresh the changing
parts, but not the title, and it will look like this:

  $ LANG=es_ES.UTF-8 ~/src/git/git commit-graph write
  Encontrando commits para commit graph entre los objetos empaquetados:
    100% (6584502/6584502), listo.
  Calculando números de generación de commit graph: 100% (824705/824705), listo.
  Escribiendo commit graph en 4 pasos: 100% (3298820/3298820), listo.

Note that the number of columns in the terminal is cached by
term_columns(), so this might not kick in when it should when a
terminal window is resized while the operation is running.
Furthermore, this change won't help if the terminal is so narrow that
the counters don't fit on one line, but I would put this in the "If it
hurts, don't do it" box.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 progress.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/progress.c b/progress.c
index 3149ecd460..e28ccdafd2 100644
--- a/progress.c
+++ b/progress.c
@@ -8,11 +8,12 @@
  * published by the Free Software Foundation.
  */
 
-#include "git-compat-util.h"
+#include "cache.h"
 #include "gettext.h"
 #include "progress.h"
 #include "strbuf.h"
 #include "trace.h"
+#include "utf8.h"
 
 #define TP_IDX_MAX      8
 
@@ -37,6 +38,8 @@ struct progress {
 	struct throughput *throughput;
 	uint64_t start_ns;
 	struct strbuf counters_sb;
+	int title_len;
+	int split;
 };
 
 static volatile sig_atomic_t progress_update;
@@ -114,8 +117,24 @@ static void display(struct progress *progress, uint64_t n, const char *done)
 			const char *eol = done ? done : "\r";
 			int clear_len = counters_sb->len < last_count_len ?
 					last_count_len - counters_sb->len : 0;
-			fprintf(stderr, "%s: %s%-*s", progress->title,
-				counters_sb->buf, clear_len, eol);
+			int progress_line_len = progress->title_len +
+						counters_sb->len + 2;
+			int cols = term_columns();
+
+			if (progress->split) {
+				fprintf(stderr, "  %s%-*s", counters_sb->buf,
+					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, clear_len, "",
+					counters_sb->buf, eol);
+				progress->split = 1;
+			} else {
+				fprintf(stderr, "%s: %s%-*s", progress->title,
+					counters_sb->buf, clear_len, eol);
+			}
 			fflush(stderr);
 		}
 		progress_update = 0;
@@ -221,6 +240,8 @@ static struct progress *start_progress_delay(const char *title, uint64_t total,
 	progress->throughput = NULL;
 	progress->start_ns = getnanotime();
 	strbuf_init(&progress->counters_sb, 0);
+	progress->title_len = utf8_strwidth(title);
+	progress->split = 0;
 	set_progress_signal();
 	return progress;
 }
-- 
2.21.0.539.g07239c3a71.dirty

Re: [PATCH v2 3/4] progress: clear previous progress update dynamically

[Jeff King]

On Mon, Apr 01, 2019 at 01:52:16PM +0200, SZEDER Gábor wrote:

> diff --git a/progress.c b/progress.c
> index 842db14b72..3149ecd460 100644
> --- a/progress.c
> +++ b/progress.c
> @@ -84,6 +84,7 @@ 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;

I don't think it could matter here, as these are meant to be smallish
strings, but I think we should get into the habit of using size_t
consistently to hold string lengths.

It makes auditing for integer overflow problems much simpler (this is on
my mind as I happen to be tracing some bugs around this the past few
days).

(There are a few instances in the next patch, too. Other than this nit,
though, your series looks good to me).

-Peff

Re: [PATCH v2 3/4] progress: clear previous progress update dynamically

[SZEDER Gábor]

On Mon, Apr 01, 2019 at 09:30:00AM -0400, Jeff King wrote:
> On Mon, Apr 01, 2019 at 01:52:16PM +0200, SZEDER Gábor wrote:
> 
> > diff --git a/progress.c b/progress.c
> > index 842db14b72..3149ecd460 100644
> > --- a/progress.c
> > +++ b/progress.c
> > @@ -84,6 +84,7 @@ 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;
> 
> I don't think it could matter here, as these are meant to be smallish
> strings, but I think we should get into the habit of using size_t
> consistently to hold string lengths.
> 
> It makes auditing for integer overflow problems much simpler (this is on
> my mind as I happen to be tracing some bugs around this the past few
> days).
> 
> (There are a few instances in the next patch, too. Other than this nit,
> though, your series looks good to me).

I started with using size_t, but then switched to int, because:

  - After a bit of arithmetic I had to compare to term_columns()'s int
    return value anyway (in the next patch).

  - The second hunk in this patch adds these lines:

        int clear_len = counters_sb->len < last_count_len ?
                        last_count_len - counters_sb->len : 0;
        fprintf(stderr, "%s: %s%-*s", progress->title,
                counters_sb->buf, clear_len, eol);

    Here 'clear_len' has to be int, because the printf() format "%-*s"
    expects an int, and otherwise -Werror=format= errors ensue.  OK,
    it could be size_t, but then it must be casted to an int upon
    passing it to fprintf(), and after the next patch there will be
    three such calls.

I could resend using size_t.  Should I resend using size_t? :)

Re: [PATCH v2 1/4] progress: make display_progress() return void

[Eric Sunshine]

On Mon, Apr 1, 2019 at 7:52 AM SZEDER Gábor <szeder.dev@gmail.com> wrote:
> [...]
> Let's make display_progress() return void, too.
>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
> diff --git a/progress.c b/progress.c
> @@ -78,12 +78,12 @@ static int is_foreground_fd(int fd)
> -static int display(struct progress *progress, uint64_t n, const char *done)
> +static void display(struct progress *progress, uint64_t n, const char *done)
>  {
>         if (progress->delay && (!progress_update || --progress->delay))
> -               return 0;
> +               return;

This 'return' needs to stay, but...

> @@ -100,7 +100,7 @@ static int display(struct progress *progress, uint64_t n, const char *done)
>                         progress_update = 0;
> -                       return 1;
> +                       return;
>                 }
>         } else if (progress_update) {
> @@ -109,10 +109,10 @@ static int display(struct progress *progress, uint64_t n, const char *done)
>                 progress_update = 0;
> -               return 1;
> +               return;
>         }
>
> -       return 0;
> +       return;
>  }

... these three 'returns' can all go away. (In fact, the first two
mysteriously disappear in patch 2/4.)

Re: [PATCH v2 2/4] progress: assemble percentage and counters in a strbuf before printing

[Eric Sunshine]

On Mon, Apr 1, 2019 at 7:52 AM SZEDER Gábor <szeder.dev@gmail.com> wrote:
> [...]
> To prepare for those changes assemble the changing parts in a separate
> strbuf kept in 'struct progress' before printing.
>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
> diff --git a/progress.c b/progress.c
> @@ -80,36 +81,42 @@ static int is_foreground_fd(int fd)
>  static void display(struct progress *progress, uint64_t n, const char *done)
>  {
>         if (progress->total) {
>                 if (percent != progress->last_percent || progress_update) {
>                         [...]
> -                       progress_update = 0;
> -                       return;
>                 }
> +       if (show_update) {
>                 [...]
>                 progress_update = 0;
> -               return;
>         }

Removal of these two 'returns' is unrelated to the change made by this
patch and should have been done by 1/4.

>         return;

Likewise, this final 'return' doesn't need to be here and should go away in 1/4.

Re: [PATCH v2 2/4] progress: assemble percentage and counters in a strbuf before printing

[Eric Sunshine]

On Tue, Apr 2, 2019 at 1:45 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Mon, Apr 1, 2019 at 7:52 AM SZEDER Gábor <szeder.dev@gmail.com> wrote:
> > +       if (show_update) {
> >                 [...]
> >                 progress_update = 0;
> > -               return;
> >         }
>
> Removal of these two 'returns' is unrelated to the change made by this
> patch and should have been done by 1/4.
>
> >         return;
>
> Likewise, this final 'return' doesn't need to be here and should go away in 1/4.

I forgot to mention that these are micro-nits, not necessarily worth a re-roll.

Re: [PATCH v2 3/4] progress: clear previous progress update dynamically

[Jeff King]

On Mon, Apr 01, 2019 at 04:15:42PM +0200, SZEDER Gábor wrote:

> > I don't think it could matter here, as these are meant to be smallish
> > strings, but I think we should get into the habit of using size_t
> > consistently to hold string lengths.
> > 
> > It makes auditing for integer overflow problems much simpler (this is on
> > my mind as I happen to be tracing some bugs around this the past few
> > days).
> > 
> > (There are a few instances in the next patch, too. Other than this nit,
> > though, your series looks good to me).
> 
> I started with using size_t, but then switched to int, because:
> 
>   - After a bit of arithmetic I had to compare to term_columns()'s int
>     return value anyway (in the next patch).
> 
>   - The second hunk in this patch adds these lines:
> 
>         int clear_len = counters_sb->len < last_count_len ?
>                         last_count_len - counters_sb->len : 0;
>         fprintf(stderr, "%s: %s%-*s", progress->title,
>                 counters_sb->buf, clear_len, eol);
> 
>     Here 'clear_len' has to be int, because the printf() format "%-*s"
>     expects an int, and otherwise -Werror=format= errors ensue.  OK,
>     it could be size_t, but then it must be casted to an int upon
>     passing it to fprintf(), and after the next patch there will be
>     three such calls.
> 
> I could resend using size_t.  Should I resend using size_t? :)

IMHO it's better to keep it as a size_t for as long as possible, and
then cast when we pass to printf, for a few reasons:

  1. The cast is made explicitly, so it calls attention to it.

  2. We know that a cast to int there can at worst produce truncated
     output, and not lead to any kind of memory error. (And if we really
     care about that, it's easy to convert it to an fwrite() at that
     point, though I would not bother in this case).

I think the comparison to "cols" is OK, because we are just checking
whether cols is smaller than us. If we assume that size_t is at least as
big as an int (which I think is a reasonable assumption to make, and
certainly holds true for all platforms I know) then there's no
possibility of logic errors.

I wouldn't even bother with a cast there. Probably -Wsign-compare would
complain, but we are pretty far from enabling that. And I think the
right solution is for term_columns() to return an unsigned, anyway. ;)

I admit all of this is academic enough that I can live with it either
way (there are definitely places where it is _not_ academic, so I am
mostly just trying to encourage a general style).

-Peff

[PATCH v3 0/4] Progress display fixes

[SZEDER Gábor]

This patch series fixes two progress display issues by breaking
progress bars longer than the width of the terminal and by properly
cleaning up the previously shown progress bar.

Changes since v2, following Eric's and Peff's suggestions:

  - Remove return statements that just became unnecessary in patch
    1/4.

  - Use size_t helper variables to store intermediate results of
    calculations based on length of strings.

SZEDER Gábor (4):
  progress: make display_progress() return void
  progress: assemble percentage and counters in a strbuf before printing
  progress: clear previous progress update dynamically
  progress: break too long progress bar lines

 progress.c | 73 ++++++++++++++++++++++++++++++++++++++----------------
 progress.h |  2 +-
 2 files changed, 53 insertions(+), 22 deletions(-)

Interdiff:
diff --git a/progress.c b/progress.c
index e28ccdafd2..97e18671e5 100644
--- a/progress.c
+++ b/progress.c
@@ -115,32 +115,30 @@ 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";
-			int clear_len = counters_sb->len < last_count_len ?
+			size_t clear_len = counters_sb->len < last_count_len ?
 					last_count_len - counters_sb->len : 0;
-			int progress_line_len = progress->title_len +
+			size_t progress_line_len = progress->title_len +
 						counters_sb->len + 2;
 			int cols = term_columns();
 
 			if (progress->split) {
 				fprintf(stderr, "  %s%-*s", counters_sb->buf,
-					clear_len, eol);
+					(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, clear_len, "",
+					progress->title, (int) clear_len, "",
 					counters_sb->buf, eol);
 				progress->split = 1;
 			} else {
 				fprintf(stderr, "%s: %s%-*s", progress->title,
-					counters_sb->buf, clear_len, eol);
+					counters_sb->buf, (int) clear_len, eol);
 			}
 			fflush(stderr);
 		}
 		progress_update = 0;
 	}
-
-	return;
 }
 
 static void throughput_string(struct strbuf *buf, uint64_t total,
Range-diff:
1:  dea36bd2a7 ! 1:  cb68e5b0ec progress: make display_progress() return void
    @@ -18,7 +18,9 @@
         its introduction in cf84d51c43 (add throughput to progress display,
         2007-10-30).
     
    -    Let's make display_progress() return void, too.
    +    Let's make display_progress() return void, too.  While doing so
    +    several return statements in display() become unnecessary, remove
    +    them.
     
         Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
     
    @@ -45,7 +47,6 @@
      			}
      			progress_update = 0;
     -			return 1;
    -+			return;
      		}
      	} else if (progress_update) {
      		if (is_foreground_fd(fileno(stderr)) || done) {
    @@ -54,11 +55,9 @@
      		}
      		progress_update = 0;
     -		return 1;
    -+		return;
      	}
    - 
    +-
     -	return 0;
    -+	return;
      }
      
      static void throughput_string(struct strbuf *buf, uint64_t total,
2:  97de2a98a0 ! 2:  017d095142 progress: assemble percentage and counters in a strbuf before printing
    @@ -50,7 +50,6 @@
     -				fflush(stderr);
     -			}
     -			progress_update = 0;
    --			return;
     +
     +			strbuf_reset(counters_sb);
     +			strbuf_addf(counters_sb,
    @@ -76,10 +75,6 @@
      			fflush(stderr);
      		}
      		progress_update = 0;
    --		return;
    - 	}
    - 
    - 	return;
     @@
      	progress->delay = delay;
      	progress->throughput = NULL;
3:  edfe0157a7 ! 3:  c5a4def5ac progress: clear previous progress update dynamically
    @@ -48,10 +48,10 @@
     -			fprintf(stderr, "%s: %s%s", progress->title,
     -				counters_sb->buf, eol);
     +			const char *eol = done ? done : "\r";
    -+			int clear_len = counters_sb->len < last_count_len ?
    ++			size_t clear_len = counters_sb->len < last_count_len ?
     +					last_count_len - counters_sb->len : 0;
     +			fprintf(stderr, "%s: %s%-*s", progress->title,
    -+				counters_sb->buf, clear_len, eol);
    ++				counters_sb->buf, (int) clear_len, eol);
      			fflush(stderr);
      		}
      		progress_update = 0;
4:  d53de231ee ! 4:  2f44dff84e progress: break too long progress bar lines
    @@ -70,27 +70,27 @@
      static volatile sig_atomic_t progress_update;
     @@
      			const char *eol = done ? done : "\r";
    - 			int clear_len = counters_sb->len < last_count_len ?
    + 			size_t clear_len = counters_sb->len < last_count_len ?
      					last_count_len - counters_sb->len : 0;
     -			fprintf(stderr, "%s: %s%-*s", progress->title,
    --				counters_sb->buf, clear_len, eol);
    -+			int progress_line_len = progress->title_len +
    +-				counters_sb->buf, (int) clear_len, eol);
    ++			size_t progress_line_len = progress->title_len +
     +						counters_sb->len + 2;
     +			int cols = term_columns();
     +
     +			if (progress->split) {
     +				fprintf(stderr, "  %s%-*s", counters_sb->buf,
    -+					clear_len, eol);
    ++					(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, clear_len, "",
    ++					progress->title, (int) clear_len, "",
     +					counters_sb->buf, eol);
     +				progress->split = 1;
     +			} else {
     +				fprintf(stderr, "%s: %s%-*s", progress->title,
    -+					counters_sb->buf, clear_len, eol);
    ++					counters_sb->buf, (int) clear_len, eol);
     +			}
      			fflush(stderr);
      		}
-- 
2.21.0.539.g07239c3a71.dirty

[PATCH v3 1/4] progress: make display_progress() return void

[SZEDER Gábor]

Ever since the progress infrastructure was introduced in 96a02f8f6d
(common progress display support, 2007-04-18), display_progress() has
returned an int, telling callers whether it updated the progress bar
or not.  However, this is:

  - useless, because over the last dozen years there has never been a
    single caller that cared about that return value.

  - not quite true, because it doesn't print a progress bar when
    running in the background, yet it returns 1; see 85cb8906f0
    (progress: no progress in background, 2015-04-13).

The related display_throughput() function returned void already upon
its introduction in cf84d51c43 (add throughput to progress display,
2007-10-30).

Let's make display_progress() return void, too.  While doing so
several return statements in display() become unnecessary, remove
them.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 progress.c | 13 +++++--------
 progress.h |  2 +-
 2 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/progress.c b/progress.c
index 5a99c9fbf0..9010032446 100644
--- a/progress.c
+++ b/progress.c
@@ -78,12 +78,12 @@ static int is_foreground_fd(int fd)
 	return tpgrp < 0 || tpgrp == getpgid(0);
 }
 
-static int display(struct progress *progress, uint64_t n, const char *done)
+static void display(struct progress *progress, uint64_t n, const char *done)
 {
 	const char *eol, *tp;
 
 	if (progress->delay && (!progress_update || --progress->delay))
-		return 0;
+		return;
 
 	progress->last_value = n;
 	tp = (progress->throughput) ? progress->throughput->display.buf : "";
@@ -100,7 +100,6 @@ static int display(struct progress *progress, uint64_t n, const char *done)
 				fflush(stderr);
 			}
 			progress_update = 0;
-			return 1;
 		}
 	} else if (progress_update) {
 		if (is_foreground_fd(fileno(stderr)) || done) {
@@ -109,10 +108,7 @@ static int display(struct progress *progress, uint64_t n, const char *done)
 			fflush(stderr);
 		}
 		progress_update = 0;
-		return 1;
 	}
-
-	return 0;
 }
 
 static void throughput_string(struct strbuf *buf, uint64_t total,
@@ -188,9 +184,10 @@ void display_throughput(struct progress *progress, uint64_t total)
 		display(progress, progress->last_value, NULL);
 }
 
-int display_progress(struct progress *progress, uint64_t n)
+void display_progress(struct progress *progress, uint64_t n)
 {
-	return progress ? display(progress, n, NULL) : 0;
+	if (progress)
+		display(progress, n, NULL);
 }
 
 static struct progress *start_progress_delay(const char *title, uint64_t total,
diff --git a/progress.h b/progress.h
index 70a4d4a0d6..59e40cc4fd 100644
--- a/progress.h
+++ b/progress.h
@@ -4,7 +4,7 @@
 struct progress;
 
 void display_throughput(struct progress *progress, uint64_t total);
-int display_progress(struct progress *progress, uint64_t n);
+void display_progress(struct progress *progress, uint64_t n);
 struct progress *start_progress(const char *title, uint64_t total);
 struct progress *start_delayed_progress(const char *title, uint64_t total);
 void stop_progress(struct progress **progress);
-- 
2.21.0.539.g07239c3a71.dirty

[PATCH v3 2/4] progress: assemble percentage and counters in a strbuf before printing

[SZEDER Gábor]

The following patches in this series want to handle the progress bar's
title and changing parts (i.e. the counter and the optional percentage
and throughput combined) differently, and need to know the length
of the changing parts of the previously displayed progress bar.

To prepare for those changes assemble the changing parts in a separate
strbuf kept in 'struct progress' before printing.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 progress.c | 35 +++++++++++++++++++++++------------
 1 file changed, 23 insertions(+), 12 deletions(-)

diff --git a/progress.c b/progress.c
index 9010032446..564845a36b 100644
--- a/progress.c
+++ b/progress.c
@@ -36,6 +36,7 @@ struct progress {
 	unsigned delay;
 	struct throughput *throughput;
 	uint64_t start_ns;
+	struct strbuf counters_sb;
 };
 
 static volatile sig_atomic_t progress_update;
@@ -80,31 +81,39 @@ static int is_foreground_fd(int fd)
 
 static void display(struct progress *progress, uint64_t n, const char *done)
 {
-	const char *eol, *tp;
+	const char *tp;
+	struct strbuf *counters_sb = &progress->counters_sb;
+	int show_update = 0;
 
 	if (progress->delay && (!progress_update || --progress->delay))
 		return;
 
 	progress->last_value = n;
 	tp = (progress->throughput) ? progress->throughput->display.buf : "";
-	eol = done ? done : "   \r";
 	if (progress->total) {
 		unsigned percent = n * 100 / progress->total;
 		if (percent != progress->last_percent || progress_update) {
 			progress->last_percent = percent;
-			if (is_foreground_fd(fileno(stderr)) || done) {
-				fprintf(stderr, "%s: %3u%% (%"PRIuMAX"/%"PRIuMAX")%s%s",
-					progress->title, percent,
-					(uintmax_t)n, (uintmax_t)progress->total,
-					tp, eol);
-				fflush(stderr);
-			}
-			progress_update = 0;
+
+			strbuf_reset(counters_sb);
+			strbuf_addf(counters_sb,
+				    "%3u%% (%"PRIuMAX"/%"PRIuMAX")%s", percent,
+				    (uintmax_t)n, (uintmax_t)progress->total,
+				    tp);
+			show_update = 1;
 		}
 	} else if (progress_update) {
+		strbuf_reset(counters_sb);
+		strbuf_addf(counters_sb, "%"PRIuMAX"%s", (uintmax_t)n, tp);
+		show_update = 1;
+	}
+
+	if (show_update) {
 		if (is_foreground_fd(fileno(stderr)) || done) {
-			fprintf(stderr, "%s: %"PRIuMAX"%s%s",
-				progress->title, (uintmax_t)n, tp, eol);
+			const char *eol = done ? done : "   \r";
+
+			fprintf(stderr, "%s: %s%s", progress->title,
+				counters_sb->buf, eol);
 			fflush(stderr);
 		}
 		progress_update = 0;
@@ -207,6 +216,7 @@ static struct progress *start_progress_delay(const char *title, uint64_t total,
 	progress->delay = delay;
 	progress->throughput = NULL;
 	progress->start_ns = getnanotime();
+	strbuf_init(&progress->counters_sb, 0);
 	set_progress_signal();
 	return progress;
 }
@@ -250,6 +260,7 @@ void stop_progress_msg(struct progress **p_progress, const char *msg)
 		free(buf);
 	}
 	clear_progress_signal();
+	strbuf_release(&progress->counters_sb);
 	if (progress->throughput)
 		strbuf_release(&progress->throughput->display);
 	free(progress->throughput);
-- 
2.21.0.539.g07239c3a71.dirty

[PATCH v3 3/4] progress: clear previous progress update dynamically

[SZEDER Gábor]

When the progress bar includes throughput, its length can shorten as
the unit of display changes from KiB to MiB.  To cover up remnants of
the previous progress bar when such a change of units happens we
always print three spaces at the end of the progress bar.

Alas, covering only three characters is not quite enough: when both
the total and the throughput happen to change units from KiB to MiB in
the same update, then the progress bar's length is shortened by four
characters (or maybe even more!):

  Receiving objects:  25% (2901/11603), 772.01 KiB | 733.00 KiB/s
  Receiving objects:  27% (3133/11603), 1.43 MiB | 1.16 MiB/s   s

and a stray 's' is left behind.

So instead of hard-coding the three characters to cover, let's compare
the length of the current progress bar with the previous one, and
cover up as many characters as needed.

Sure, it would be much simpler to just print more spaces at the end of
the progress bar, but this approach is more future-proof, and it won't
print extra spaces when none are needed, notably when the progress bar
doesn't show throughput and thus never shrinks.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 progress.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/progress.c b/progress.c
index 564845a36b..ca0945d2be 100644
--- a/progress.c
+++ b/progress.c
@@ -84,6 +84,7 @@ 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;
@@ -110,10 +111,11 @@ 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";
-
-			fprintf(stderr, "%s: %s%s", progress->title,
-				counters_sb->buf, eol);
+			const char *eol = done ? done : "\r";
+			size_t clear_len = counters_sb->len < last_count_len ?
+					last_count_len - counters_sb->len : 0;
+			fprintf(stderr, "%s: %s%-*s", progress->title,
+				counters_sb->buf, (int) clear_len, eol);
 			fflush(stderr);
 		}
 		progress_update = 0;
-- 
2.21.0.539.g07239c3a71.dirty

[PATCH v3 4/4] progress: break too long progress bar lines

[SZEDER Gábor]

Some of the recently added progress indicators have quite long titles,
which might be even longer when translated to some languages, and when
they are shown while operating on bigger repositories, then the
progress bar grows longer than the default 80 column terminal width.

When the progress bar exceeds the width of the terminal it gets
line-wrapped, and after that the CR at the end doesn't return to the
beginning of the progress bar, but to the first column of its last
line.  Consequently, the first line of the previously shown progress
bar is not overwritten by the next, and we end up with a bunch of
truncated progress bar lines scrolling past:

  $ LANG=es_ES.UTF-8 git commit-graph write
  Encontrando commits para commit graph entre los objetos empaquetados:   2% (1599
  Encontrando commits para commit graph entre los objetos empaquetados:   3% (1975
  Encontrando commits para commit graph entre los objetos empaquetados:   4% (2633
  Encontrando commits para commit graph entre los objetos empaquetados:   5% (3292
  [...]

Prevent this by breaking progress bars after the title once they
exceed the width of the terminal, so the counter and optional
percentage and throughput, i.e. all changing parts, are on the last
line.  Subsequent updates will from then on only refresh the changing
parts, but not the title, and it will look like this:

  $ LANG=es_ES.UTF-8 ~/src/git/git commit-graph write
  Encontrando commits para commit graph entre los objetos empaquetados:
    100% (6584502/6584502), listo.
  Calculando números de generación de commit graph: 100% (824705/824705), listo.
  Escribiendo commit graph en 4 pasos: 100% (3298820/3298820), listo.

Note that the number of columns in the terminal is cached by
term_columns(), so this might not kick in when it should when a
terminal window is resized while the operation is running.
Furthermore, this change won't help if the terminal is so narrow that
the counters don't fit on one line, but I would put this in the "If it
hurts, don't do it" box.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 progress.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/progress.c b/progress.c
index ca0945d2be..97e18671e5 100644
--- a/progress.c
+++ b/progress.c
@@ -8,11 +8,12 @@
  * published by the Free Software Foundation.
  */
 
-#include "git-compat-util.h"
+#include "cache.h"
 #include "gettext.h"
 #include "progress.h"
 #include "strbuf.h"
 #include "trace.h"
+#include "utf8.h"
 
 #define TP_IDX_MAX      8
 
@@ -37,6 +38,8 @@ struct progress {
 	struct throughput *throughput;
 	uint64_t start_ns;
 	struct strbuf counters_sb;
+	int title_len;
+	int split;
 };
 
 static volatile sig_atomic_t progress_update;
@@ -114,8 +117,24 @@ static void display(struct progress *progress, uint64_t n, const char *done)
 			const char *eol = done ? done : "\r";
 			size_t clear_len = counters_sb->len < last_count_len ?
 					last_count_len - counters_sb->len : 0;
-			fprintf(stderr, "%s: %s%-*s", progress->title,
-				counters_sb->buf, (int) clear_len, eol);
+			size_t progress_line_len = progress->title_len +
+						counters_sb->len + 2;
+			int cols = term_columns();
+
+			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);
+				progress->split = 1;
+			} else {
+				fprintf(stderr, "%s: %s%-*s", progress->title,
+					counters_sb->buf, (int) clear_len, eol);
+			}
 			fflush(stderr);
 		}
 		progress_update = 0;
@@ -219,6 +238,8 @@ static struct progress *start_progress_delay(const char *title, uint64_t total,
 	progress->throughput = NULL;
 	progress->start_ns = getnanotime();
 	strbuf_init(&progress->counters_sb, 0);
+	progress->title_len = utf8_strwidth(title);
+	progress->split = 0;
 	set_progress_signal();
 	return progress;
 }
-- 
2.21.0.539.g07239c3a71.dirty

Re: [PATCH v3 0/4] Progress display fixes

[Jeff King]

On Fri, Apr 05, 2019 at 02:45:35AM +0200, SZEDER Gábor wrote:

> This patch series fixes two progress display issues by breaking
> progress bars longer than the width of the terminal and by properly
> cleaning up the previously shown progress bar.
> 
> Changes since v2, following Eric's and Peff's suggestions:
> 
>   - Remove return statements that just became unnecessary in patch
>     1/4.
> 
>   - Use size_t helper variables to store intermediate results of
>     calculations based on length of strings.

Thanks, I have no more nits to pick.

-Peff

[PATCH v4 0/4] Progress display fixes

[SZEDER Gábor]

This patch series fixes two progress display issues by breaking
progress bars longer than the width of the terminal and by properly
cleaning up the previously shown progress bar.  Hopefully for good
this time...

This "properly cleaning up" part, i.e. mainly patch 3, was buggy in
previous versions, because:

  - It used the wrong format flag and the '\r' was padded on the
    right, but should have been padded to the left.

  - The padding was one space shorter than necessary, because I didn't
    account for the '\r' included in the field width as well.


SZEDER Gábor (4):
  progress: make display_progress() return void
  progress: assemble percentage and counters in a strbuf before printing
  progress: clear previous progress update dynamically
  progress: break too long progress bar lines

 progress.c | 74 ++++++++++++++++++++++++++++++++++++++----------------
 progress.h |  2 +-
 2 files changed, 54 insertions(+), 22 deletions(-)

Interdiff:
diff --git a/progress.c b/progress.c
index 97e18671e5..2d8022a622 100644
--- a/progress.c
+++ b/progress.c
@@ -116,23 +116,24 @@ static void display(struct progress *progress, uint64_t n, const char *done)
 		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 : 0;
+					last_count_len - counters_sb->len + 1 :
+					0;
 			size_t progress_line_len = progress->title_len +
 						counters_sb->len + 2;
 			int cols = term_columns();
 
 			if (progress->split) {
-				fprintf(stderr, "  %s%-*s", counters_sb->buf,
+				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;
+					    cols - progress->title_len : 0;
 				fprintf(stderr, "%s:%*s\n  %s%s",
 					progress->title, (int) clear_len, "",
 					counters_sb->buf, eol);
 				progress->split = 1;
 			} else {
-				fprintf(stderr, "%s: %s%-*s", progress->title,
+				fprintf(stderr, "%s: %s%*s", progress->title,
 					counters_sb->buf, (int) clear_len, eol);
 			}
 			fflush(stderr);
Range-diff:
1:  cb68e5b0ec = 1:  cb68e5b0ec progress: make display_progress() return void
2:  017d095142 = 2:  017d095142 progress: assemble percentage and counters in a strbuf before printing
3:  c5a4def5ac ! 3:  ec9c96d102 progress: clear previous progress update dynamically
    @@ -49,8 +49,9 @@
     -				counters_sb->buf, eol);
     +			const char *eol = done ? done : "\r";
     +			size_t clear_len = counters_sb->len < last_count_len ?
    -+					last_count_len - counters_sb->len : 0;
    -+			fprintf(stderr, "%s: %s%-*s", progress->title,
    ++					last_count_len - counters_sb->len + 1 :
    ++					0;
    ++			fprintf(stderr, "%s: %s%*s", progress->title,
     +				counters_sb->buf, (int) clear_len, eol);
      			fflush(stderr);
      		}
4:  2f44dff84e ! 4:  8fc8e3cf94 progress: break too long progress bar lines
    @@ -69,27 +69,27 @@
      
      static volatile sig_atomic_t progress_update;
     @@
    - 			const char *eol = done ? done : "\r";
      			size_t clear_len = counters_sb->len < last_count_len ?
    - 					last_count_len - counters_sb->len : 0;
    --			fprintf(stderr, "%s: %s%-*s", progress->title,
    + 					last_count_len - counters_sb->len + 1 :
    + 					0;
    +-			fprintf(stderr, "%s: %s%*s", progress->title,
     -				counters_sb->buf, (int) clear_len, eol);
     +			size_t progress_line_len = progress->title_len +
     +						counters_sb->len + 2;
     +			int cols = term_columns();
     +
     +			if (progress->split) {
    -+				fprintf(stderr, "  %s%-*s", counters_sb->buf,
    ++				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;
    ++					    cols - progress->title_len : 0;
     +				fprintf(stderr, "%s:%*s\n  %s%s",
     +					progress->title, (int) clear_len, "",
     +					counters_sb->buf, eol);
     +				progress->split = 1;
     +			} else {
    -+				fprintf(stderr, "%s: %s%-*s", progress->title,
    ++				fprintf(stderr, "%s: %s%*s", progress->title,
     +					counters_sb->buf, (int) clear_len, eol);
     +			}
      			fflush(stderr);
-- 
2.21.0.746.gd74f1657d3

[PATCH v4 1/4] progress: make display_progress() return void

[SZEDER Gábor]

Ever since the progress infrastructure was introduced in 96a02f8f6d
(common progress display support, 2007-04-18), display_progress() has
returned an int, telling callers whether it updated the progress bar
or not.  However, this is:

  - useless, because over the last dozen years there has never been a
    single caller that cared about that return value.

  - not quite true, because it doesn't print a progress bar when
    running in the background, yet it returns 1; see 85cb8906f0
    (progress: no progress in background, 2015-04-13).

The related display_throughput() function returned void already upon
its introduction in cf84d51c43 (add throughput to progress display,
2007-10-30).

Let's make display_progress() return void, too.  While doing so
several return statements in display() become unnecessary, remove
them.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 progress.c | 13 +++++--------
 progress.h |  2 +-
 2 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/progress.c b/progress.c
index 5a99c9fbf0..9010032446 100644
--- a/progress.c
+++ b/progress.c
@@ -78,12 +78,12 @@ static int is_foreground_fd(int fd)
 	return tpgrp < 0 || tpgrp == getpgid(0);
 }
 
-static int display(struct progress *progress, uint64_t n, const char *done)
+static void display(struct progress *progress, uint64_t n, const char *done)
 {
 	const char *eol, *tp;
 
 	if (progress->delay && (!progress_update || --progress->delay))
-		return 0;
+		return;
 
 	progress->last_value = n;
 	tp = (progress->throughput) ? progress->throughput->display.buf : "";
@@ -100,7 +100,6 @@ static int display(struct progress *progress, uint64_t n, const char *done)
 				fflush(stderr);
 			}
 			progress_update = 0;
-			return 1;
 		}
 	} else if (progress_update) {
 		if (is_foreground_fd(fileno(stderr)) || done) {
@@ -109,10 +108,7 @@ static int display(struct progress *progress, uint64_t n, const char *done)
 			fflush(stderr);
 		}
 		progress_update = 0;
-		return 1;
 	}
-
-	return 0;
 }
 
 static void throughput_string(struct strbuf *buf, uint64_t total,
@@ -188,9 +184,10 @@ void display_throughput(struct progress *progress, uint64_t total)
 		display(progress, progress->last_value, NULL);
 }
 
-int display_progress(struct progress *progress, uint64_t n)
+void display_progress(struct progress *progress, uint64_t n)
 {
-	return progress ? display(progress, n, NULL) : 0;
+	if (progress)
+		display(progress, n, NULL);
 }
 
 static struct progress *start_progress_delay(const char *title, uint64_t total,
diff --git a/progress.h b/progress.h
index 70a4d4a0d6..59e40cc4fd 100644
--- a/progress.h
+++ b/progress.h
@@ -4,7 +4,7 @@
 struct progress;
 
 void display_throughput(struct progress *progress, uint64_t total);
-int display_progress(struct progress *progress, uint64_t n);
+void display_progress(struct progress *progress, uint64_t n);
 struct progress *start_progress(const char *title, uint64_t total);
 struct progress *start_delayed_progress(const char *title, uint64_t total);
 void stop_progress(struct progress **progress);
-- 
2.21.0.746.gd74f1657d3

[PATCH v4 2/4] progress: assemble percentage and counters in a strbuf before printing

[SZEDER Gábor]

The following patches in this series want to handle the progress bar's
title and changing parts (i.e. the counter and the optional percentage
and throughput combined) differently, and need to know the length
of the changing parts of the previously displayed progress bar.

To prepare for those changes assemble the changing parts in a separate
strbuf kept in 'struct progress' before printing.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 progress.c | 35 +++++++++++++++++++++++------------
 1 file changed, 23 insertions(+), 12 deletions(-)

diff --git a/progress.c b/progress.c
index 9010032446..564845a36b 100644
--- a/progress.c
+++ b/progress.c
@@ -36,6 +36,7 @@ struct progress {
 	unsigned delay;
 	struct throughput *throughput;
 	uint64_t start_ns;
+	struct strbuf counters_sb;
 };
 
 static volatile sig_atomic_t progress_update;
@@ -80,31 +81,39 @@ static int is_foreground_fd(int fd)
 
 static void display(struct progress *progress, uint64_t n, const char *done)
 {
-	const char *eol, *tp;
+	const char *tp;
+	struct strbuf *counters_sb = &progress->counters_sb;
+	int show_update = 0;
 
 	if (progress->delay && (!progress_update || --progress->delay))
 		return;
 
 	progress->last_value = n;
 	tp = (progress->throughput) ? progress->throughput->display.buf : "";
-	eol = done ? done : "   \r";
 	if (progress->total) {
 		unsigned percent = n * 100 / progress->total;
 		if (percent != progress->last_percent || progress_update) {
 			progress->last_percent = percent;
-			if (is_foreground_fd(fileno(stderr)) || done) {
-				fprintf(stderr, "%s: %3u%% (%"PRIuMAX"/%"PRIuMAX")%s%s",
-					progress->title, percent,
-					(uintmax_t)n, (uintmax_t)progress->total,
-					tp, eol);
-				fflush(stderr);
-			}
-			progress_update = 0;
+
+			strbuf_reset(counters_sb);
+			strbuf_addf(counters_sb,
+				    "%3u%% (%"PRIuMAX"/%"PRIuMAX")%s", percent,
+				    (uintmax_t)n, (uintmax_t)progress->total,
+				    tp);
+			show_update = 1;
 		}
 	} else if (progress_update) {
+		strbuf_reset(counters_sb);
+		strbuf_addf(counters_sb, "%"PRIuMAX"%s", (uintmax_t)n, tp);
+		show_update = 1;
+	}
+
+	if (show_update) {
 		if (is_foreground_fd(fileno(stderr)) || done) {
-			fprintf(stderr, "%s: %"PRIuMAX"%s%s",
-				progress->title, (uintmax_t)n, tp, eol);
+			const char *eol = done ? done : "   \r";
+
+			fprintf(stderr, "%s: %s%s", progress->title,
+				counters_sb->buf, eol);
 			fflush(stderr);
 		}
 		progress_update = 0;
@@ -207,6 +216,7 @@ static struct progress *start_progress_delay(const char *title, uint64_t total,
 	progress->delay = delay;
 	progress->throughput = NULL;
 	progress->start_ns = getnanotime();
+	strbuf_init(&progress->counters_sb, 0);
 	set_progress_signal();
 	return progress;
 }
@@ -250,6 +260,7 @@ void stop_progress_msg(struct progress **p_progress, const char *msg)
 		free(buf);
 	}
 	clear_progress_signal();
+	strbuf_release(&progress->counters_sb);
 	if (progress->throughput)
 		strbuf_release(&progress->throughput->display);
 	free(progress->throughput);
-- 
2.21.0.746.gd74f1657d3

[PATCH v4 3/4] progress: clear previous progress update dynamically

[SZEDER Gábor]

When the progress bar includes throughput, its length can shorten as
the unit of display changes from KiB to MiB.  To cover up remnants of
the previous progress bar when such a change of units happens we
always print three spaces at the end of the progress bar.

Alas, covering only three characters is not quite enough: when both
the total and the throughput happen to change units from KiB to MiB in
the same update, then the progress bar's length is shortened by four
characters (or maybe even more!):

  Receiving objects:  25% (2901/11603), 772.01 KiB | 733.00 KiB/s
  Receiving objects:  27% (3133/11603), 1.43 MiB | 1.16 MiB/s   s

and a stray 's' is left behind.

So instead of hard-coding the three characters to cover, let's compare
the length of the current progress bar with the previous one, and
cover up as many characters as needed.

Sure, it would be much simpler to just print more spaces at the end of
the progress bar, but this approach is more future-proof, and it won't
print extra spaces when none are needed, notably when the progress bar
doesn't show throughput and thus never shrinks.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 progress.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/progress.c b/progress.c
index 564845a36b..949a2a576d 100644
--- a/progress.c
+++ b/progress.c
@@ -84,6 +84,7 @@ 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;
@@ -110,10 +111,12 @@ 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";
-
-			fprintf(stderr, "%s: %s%s", progress->title,
-				counters_sb->buf, eol);
+			const char *eol = done ? done : "\r";
+			size_t clear_len = counters_sb->len < last_count_len ?
+					last_count_len - counters_sb->len + 1 :
+					0;
+			fprintf(stderr, "%s: %s%*s", progress->title,
+				counters_sb->buf, (int) clear_len, eol);
 			fflush(stderr);
 		}
 		progress_update = 0;
-- 
2.21.0.746.gd74f1657d3

[PATCH v4 4/4] progress: break too long progress bar lines

[SZEDER Gábor]

Some of the recently added progress indicators have quite long titles,
which might be even longer when translated to some languages, and when
they are shown while operating on bigger repositories, then the
progress bar grows longer than the default 80 column terminal width.

When the progress bar exceeds the width of the terminal it gets
line-wrapped, and after that the CR at the end doesn't return to the
beginning of the progress bar, but to the first column of its last
line.  Consequently, the first line of the previously shown progress
bar is not overwritten by the next, and we end up with a bunch of
truncated progress bar lines scrolling past:

  $ LANG=es_ES.UTF-8 git commit-graph write
  Encontrando commits para commit graph entre los objetos empaquetados:   2% (1599
  Encontrando commits para commit graph entre los objetos empaquetados:   3% (1975
  Encontrando commits para commit graph entre los objetos empaquetados:   4% (2633
  Encontrando commits para commit graph entre los objetos empaquetados:   5% (3292
  [...]

Prevent this by breaking progress bars after the title once they
exceed the width of the terminal, so the counter and optional
percentage and throughput, i.e. all changing parts, are on the last
line.  Subsequent updates will from then on only refresh the changing
parts, but not the title, and it will look like this:

  $ LANG=es_ES.UTF-8 ~/src/git/git commit-graph write
  Encontrando commits para commit graph entre los objetos empaquetados:
    100% (6584502/6584502), listo.
  Calculando números de generación de commit graph: 100% (824705/824705), listo.
  Escribiendo commit graph en 4 pasos: 100% (3298820/3298820), listo.

Note that the number of columns in the terminal is cached by
term_columns(), so this might not kick in when it should when a
terminal window is resized while the operation is running.
Furthermore, this change won't help if the terminal is so narrow that
the counters don't fit on one line, but I would put this in the "If it
hurts, don't do it" box.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 progress.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/progress.c b/progress.c
index 949a2a576d..2d8022a622 100644
--- a/progress.c
+++ b/progress.c
@@ -8,11 +8,12 @@
  * published by the Free Software Foundation.
  */
 
-#include "git-compat-util.h"
+#include "cache.h"
 #include "gettext.h"
 #include "progress.h"
 #include "strbuf.h"
 #include "trace.h"
+#include "utf8.h"
 
 #define TP_IDX_MAX      8
 
@@ -37,6 +38,8 @@ struct progress {
 	struct throughput *throughput;
 	uint64_t start_ns;
 	struct strbuf counters_sb;
+	int title_len;
+	int split;
 };
 
 static volatile sig_atomic_t progress_update;
@@ -115,8 +118,24 @@ static void display(struct progress *progress, uint64_t n, const char *done)
 			size_t clear_len = counters_sb->len < last_count_len ?
 					last_count_len - counters_sb->len + 1 :
 					0;
-			fprintf(stderr, "%s: %s%*s", progress->title,
-				counters_sb->buf, (int) clear_len, eol);
+			size_t progress_line_len = progress->title_len +
+						counters_sb->len + 2;
+			int cols = term_columns();
+
+			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 : 0;
+				fprintf(stderr, "%s:%*s\n  %s%s",
+					progress->title, (int) clear_len, "",
+					counters_sb->buf, eol);
+				progress->split = 1;
+			} else {
+				fprintf(stderr, "%s: %s%*s", progress->title,
+					counters_sb->buf, (int) clear_len, eol);
+			}
 			fflush(stderr);
 		}
 		progress_update = 0;
@@ -220,6 +239,8 @@ static struct progress *start_progress_delay(const char *title, uint64_t total,
 	progress->throughput = NULL;
 	progress->start_ns = getnanotime();
 	strbuf_init(&progress->counters_sb, 0);
+	progress->title_len = utf8_strwidth(title);
+	progress->split = 0;
 	set_progress_signal();
 	return progress;
 }
-- 
2.21.0.746.gd74f1657d3