[PATCH v2 0/4] Progress display fixes
- From
SZEDER Gábor <szeder.dev@gmail.com>
- Date
- Apr 1, 2019, 11:52 UTC
- Message-ID
- <20190401115217.3423-1-szeder.dev@gmail.com>
- In-Reply-To
- <20190325103844.26749-1-szeder.dev@gmail.com>
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