{"thread":{"id":"50817","subject":"[PATCH 0/5] Progress display fixes","startedAt":"2019-03-25T10:38:59Z","lastAt":"2019-04-12T19:45:32Z","messageCount":38,"participants":["SZEDER Gábor","Duy Nguyen","Ævar Arnfjörð Bjarmason","Jeff King","Luke Mewburn","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"372405","messageId":"20190325103844.26749-1-szeder.dev@gmail.com","threadId":"50817","inReplyTo":null,"subject":"[PATCH 0/5] Progress display fixes","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-03-25T10:38:39Z","receivedAt":"2019-03-25T10:38:59Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"This patch series fixes two progress display issues:\n\n - When showing throughput, and the both the total and the throughput\n change units in the same update, than the previously shown\n progress bar is not cleaned up properly:\n\n Receiving objects: 25% (2901/11603), 772.01 KiB | 733.00 KiB/s\n Receiving objects: 27% (3133/11603), 1.43 MiB | 1.16 MiB/s s\n\n - When the progress bar is longer than the width of the terminal,\n then we end up with a bunch of truncated progress bar lines\n scrolling past:\n\n $ LANG=es_ES.UTF-8 git commit-graph write\n Encontrando commits para commit graph entre los objetos empaquetados: 2% (1599\n Encontrando commits para commit graph entre los objetos empaquetados: 3% (1975\n Encontrando commits para commit graph entre los objetos empaquetados: 4% (2633\n Encontrando commits para commit graph entre los objetos empaquetados: 5% (3292\n [...]\n\nPatches 4 and 5 fix these two issues, while the first three are\nminor preparatory cleanups and refactorings.\n\nSZEDER Gábor (5):\n progress: make display_progress() return void\n progress: return early when in the background\n progress: assemble percentage and counters in a strbuf before printing\n progress: clear previous progress update dynamically\n progress: break too long progress bar lines\n\n progress.c | 78 +++++++++++++++++++++++++++++++++++++++---------------\n progress.h | 2 +-\n 2 files changed, 57 insertions(+), 23 deletions(-)\n\n-- \n2.21.0.539.g07239c3a71.dirty\n\n"},{"id":"372406","messageId":"20190325103844.26749-2-szeder.dev@gmail.com","threadId":"50817","inReplyTo":"20190325103844.26749-1-szeder.dev@gmail.com","subject":"[PATCH 1/5] progress: make display_progress() return void","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-03-25T10:38:40Z","receivedAt":"2019-03-25T10:39:01Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Ever since the progress infrastructure was introduced in 96a02f8f6d\n(common progress display support, 2007-04-18), display_progress() has\nreturned an int, telling callers whether it updated the progress bar\nor not. However, this is:\n\n - useless, because over the last dozen years there has never been a\n single caller that cared about that return value.\n\n - not quite true, because it doesn't print a progress bar when\n running in the background, yet it returns 1; see 85cb8906f0\n (progress: no progress in background, 2015-04-13).\n\nThe related display_throughput() function returned void already upon\nits introduction in cf84d51c43 (add throughput to progress display,\n2007-10-30).\n\nLet's make display_progress() return void, too.\n\nSigned-off-by: SZEDER Gábor \n---\n progress.c | 15 ++++++++-------\n progress.h | 2 +-\n 2 files changed, 9 insertions(+), 8 deletions(-)\n\ndiff --git a/progress.c b/progress.c\nindex 5a99c9fbf0..02a20e7d58 100644\n--- a/progress.c\n+++ b/progress.c\n@@ -78,12 +78,12 @@ static int is_foreground_fd(int fd)\n \treturn tpgrp < 0 || tpgrp == getpgid(0);\n }\n \n-static int display(struct progress *progress, uint64_t n, const char *done)\n+static void display(struct progress *progress, uint64_t n, const char *done)\n {\n \tconst char *eol, *tp;\n \n \tif (progress->delay && (!progress_update || --progress->delay))\n-\t\treturn 0;\n+\t\treturn;\n \n \tprogress->last_value = n;\n \ttp = (progress->throughput) ? progress->throughput->display.buf : \"\";\n@@ -100,7 +100,7 @@ static int display(struct progress *progress, uint64_t n, const char *done)\n \t\t\t\tfflush(stderr);\n \t\t\t}\n \t\t\tprogress_update = 0;\n-\t\t\treturn 1;\n+\t\t\treturn;\n \t\t}\n \t} else if (progress_update) {\n \t\tif (is_foreground_fd(fileno(stderr)) || done) {\n@@ -109,10 +109,10 @@ static int display(struct progress *progress, uint64_t n, const char *done)\n \t\t\tfflush(stderr);\n \t\t}\n \t\tprogress_update = 0;\n-\t\treturn 1;\n+\t\treturn;\n \t}\n \n-\treturn 0;\n+\treturn;\n }\n \n static void throughput_string(struct strbuf *buf, uint64_t total,\n@@ -188,9 +188,10 @@ void display_throughput(struct progress *progress, uint64_t total)\n \t\tdisplay(progress, progress->last_value, NULL);\n }\n \n-int display_progress(struct progress *progress, uint64_t n)\n+void display_progress(struct progress *progress, uint64_t n)\n {\n-\treturn progress ? display(progress, n, NULL) : 0;\n+\tif (progress)\n+\t\tdisplay(progress, n, NULL);\n }\n \n static struct progress *start_progress_delay(const char *title, uint64_t total,\ndiff --git a/progress.h b/progress.h\nindex 70a4d4a0d6..59e40cc4fd 100644\n--- a/progress.h\n+++ b/progress.h\n@@ -4,7 +4,7 @@\n struct progress;\n \n void display_throughput(struct progress *progress, uint64_t total);\n-int display_progress(struct progress *progress, uint64_t n);\n+void display_progress(struct progress *progress, uint64_t n);\n struct progress *start_progress(const char *title, uint64_t total);\n struct progress *start_delayed_progress(const char *title, uint64_t total);\n void stop_progress(struct progress **progress);\n-- \n2.21.0.539.g07239c3a71.dirty\n\n"},{"id":"372407","messageId":"20190325103844.26749-4-szeder.dev@gmail.com","threadId":"50817","inReplyTo":"20190325103844.26749-1-szeder.dev@gmail.com","subject":"[PATCH 3/5] progress: assemble percentage and counters in a strbuf before printing","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-03-25T10:38:42Z","receivedAt":"2019-03-25T10:39:04Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The following patches in this series want to handle the progress bar's\ntitle and changing parts (i.e. the counter and the optional percentage\nand throughput combined) differently.\n\nTo prepare for those changes assemble the changing parts in a separate\nstrbuf before printing.\n\nSigned-off-by: SZEDER Gábor \n---\n progress.c | 35 +++++++++++++++++++++++------------\n 1 file changed, 23 insertions(+), 12 deletions(-)\n\ndiff --git a/progress.c b/progress.c\nindex b57c0dae16..390d2487ca 100644\n--- a/progress.c\n+++ b/progress.c\n@@ -36,6 +36,7 @@ struct progress {\n \tunsigned delay;\n \tstruct throughput *throughput;\n \tuint64_t start_ns;\n+\tstruct strbuf counters_sb;\n };\n \n static volatile sig_atomic_t progress_update;\n@@ -80,7 +81,9 @@ static int is_foreground_fd(int fd)\n \n static void display(struct progress *progress, uint64_t n, const char *done)\n {\n-\tconst char *eol, *tp;\n+\tconst char *tp;\n+\tstruct strbuf *counters_sb = &progress->counters_sb;\n+\tint update = 0;\n \n \tif (progress->delay && (!progress_update || --progress->delay))\n \t\treturn;\n@@ -93,25 +96,31 @@ static void display(struct progress *progress, uint64_t n, const char *done)\n \t}\n \n \ttp = (progress->throughput) ? progress->throughput->display.buf : \"\";\n-\teol = done ? done : \" \\r\";\n \tif (progress->total) {\n \t\tunsigned percent = n * 100 / progress->total;\n \t\tif (percent != progress->last_percent || progress_update) {\n \t\t\tprogress->last_percent = percent;\n-\t\t\tfprintf(stderr, \"%s: %3u%% (%\"PRIuMAX\"/%\"PRIuMAX\")%s%s\",\n-\t\t\t\tprogress->title, percent,\n-\t\t\t\t(uintmax_t)n, (uintmax_t)progress->total,\n-\t\t\t\ttp, eol);\n-\t\t\tfflush(stderr);\n-\t\t\tprogress_update = 0;\n-\t\t\treturn;\n+\n+\t\t\tstrbuf_reset(counters_sb);\n+\t\t\tstrbuf_addf(counters_sb,\n+\t\t\t\t \"%3u%% (%\"PRIuMAX\"/%\"PRIuMAX\")%s\", percent,\n+\t\t\t\t (uintmax_t)n, (uintmax_t)progress->total,\n+\t\t\t\t tp);\n+\t\t\tupdate = 1;\n \t\t}\n \t} else if (progress_update) {\n-\t\tfprintf(stderr, \"%s: %\"PRIuMAX\"%s%s\",\n-\t\t\tprogress->title, (uintmax_t)n, tp, eol);\n+\t\tstrbuf_reset(counters_sb);\n+\t\tstrbuf_addf(counters_sb, \"%\"PRIuMAX\"%s\", (uintmax_t)n, tp);\n+\t\tupdate = 1;\n+\t}\n+\n+\tif (update) {\n+\t\tconst char *eol = done ? done : \" \\r\";\n+\n+\t\tfprintf(stderr, \"%s: %s%s\", progress->title, counters_sb->buf,\n+\t\t\teol);\n \t\tfflush(stderr);\n \t\tprogress_update = 0;\n-\t\treturn;\n \t}\n \n \treturn;\n@@ -213,6 +222,7 @@ static struct progress *start_progress_delay(const char *title, uint64_t total,\n \tprogress->delay = delay;\n \tprogress->throughput = NULL;\n \tprogress->start_ns = getnanotime();\n+\tstrbuf_init(&progress->counters_sb, 0);\n \tset_progress_signal();\n \treturn progress;\n }\n@@ -256,6 +266,7 @@ void stop_progress_msg(struct progress **p_progress, const char *msg)\n \t\tfree(buf);\n \t}\n \tclear_progress_signal();\n+\tstrbuf_release(&progress->counters_sb);\n \tif (progress->throughput)\n \t\tstrbuf_release(&progress->throughput->display);\n \tfree(progress->throughput);\n-- \n2.21.0.539.g07239c3a71.dirty\n\n"},{"id":"372408","messageId":"20190325103844.26749-5-szeder.dev@gmail.com","threadId":"50817","inReplyTo":"20190325103844.26749-1-szeder.dev@gmail.com","subject":"[PATCH 4/5] progress: clear previous progress update dynamically","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-03-25T10:38:43Z","receivedAt":"2019-03-25T10:39:05Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"When the progress bar includes throughput, its length can shorten as\nthe unit of display changes from KiB to MiB. To cover up remnants of\nthe previous progress bar when such a change of units happens we\nalways print three spaces at the end of the progress bar.\n\nAlas, covering only three characters is not quite enough: when both\nthe total and the throughput happen to change units from KiB to MiB in\nthe same update, then the progress bar's length is shortened by four\ncharacters:\n\n Receiving objects: 25% (2901/11603), 772.01 KiB | 733.00 KiB/s\n Receiving objects: 27% (3133/11603), 1.43 MiB | 1.16 MiB/s s\n\nand a stray 's' is left behind.\n\nSo instead of hard-coding the three characters to cover, let's compare\nthe length of the current progress bar with the previous one, and\ncover up as many characters as needed.\n\nSure, it would be much simpler to just print four spaces instead of\nthree at the end of the progress bar, but this approach is more\nfuture-proof, and it won't print extra spaces when none are needed,\nnotably when the progress bar doesn't show throughput and thus never\nshrinks.\n\nSigned-off-by: SZEDER Gábor \n---\n progress.c | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/progress.c b/progress.c\nindex 390d2487ca..b37a5398c5 100644\n--- a/progress.c\n+++ b/progress.c\n@@ -84,6 +84,7 @@ static void display(struct progress *progress, uint64_t n, const char *done)\n \tconst char *tp;\n \tstruct strbuf *counters_sb = &progress->counters_sb;\n \tint update = 0;\n+\tint last_count_len = counters_sb->len;\n \n \tif (progress->delay && (!progress_update || --progress->delay))\n \t\treturn;\n@@ -115,10 +116,11 @@ static void display(struct progress *progress, uint64_t n, const char *done)\n \t}\n \n \tif (update) {\n-\t\tconst char *eol = done ? done : \" \\r\";\n-\n-\t\tfprintf(stderr, \"%s: %s%s\", progress->title, counters_sb->buf,\n-\t\t\teol);\n+\t\tconst char *eol = done ? done : \"\\r\";\n+\t\tint clear_len = counters_sb->len < last_count_len ?\n+\t\t\t\tlast_count_len - counters_sb->len : 0;\n+\t\tfprintf(stderr, \"%s: %s%-*s\", progress->title,\n+\t\t\tcounters_sb->buf, clear_len, eol);\n \t\tfflush(stderr);\n \t\tprogress_update = 0;\n \t}\n-- \n2.21.0.539.g07239c3a71.dirty\n\n"},{"id":"372409","messageId":"20190325103844.26749-3-szeder.dev@gmail.com","threadId":"50817","inReplyTo":"20190325103844.26749-1-szeder.dev@gmail.com","subject":"[PATCH 2/5] progress: return early when in the background","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-03-25T10:38:41Z","receivedAt":"2019-03-25T10:39:06Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"When a git process runs in the background, it doesn't display\nprogress, only the final \"done\" line [1]. The condition to check that\nare a bit too deep in the display() function, and thus it calculates\nthe progress percentage even when no progress will be displayed\nanyway.\n\nRestructure the display() function to return early when we are in the\nbackground, which prevents the unnecessary progress percentae\ncalculation, and make the function look a bit better by losing one\nlevel of indentation.\n\n[1] 85cb8906f0 (progress: no progress in background, 2015-04-13)\n\nSigned-off-by: SZEDER Gábor \n---\n progress.c | 26 ++++++++++++++------------\n 1 file changed, 14 insertions(+), 12 deletions(-)\n\ndiff --git a/progress.c b/progress.c\nindex 02a20e7d58..b57c0dae16 100644\n--- a/progress.c\n+++ b/progress.c\n@@ -86,28 +86,30 @@ static void display(struct progress *progress, uint64_t n, const char *done)\n \t\treturn;\n \n \tprogress->last_value = n;\n+\n+\tif (!is_foreground_fd(fileno(stderr)) && !done) {\n+\t\tprogress_update = 0;\n+\t\treturn;\n+\t}\n+\n \ttp = (progress->throughput) ? progress->throughput->display.buf : \"\";\n \teol = done ? done : \" \\r\";\n \tif (progress->total) {\n \t\tunsigned percent = n * 100 / progress->total;\n \t\tif (percent != progress->last_percent || progress_update) {\n \t\t\tprogress->last_percent = percent;\n-\t\t\tif (is_foreground_fd(fileno(stderr)) || done) {\n-\t\t\t\tfprintf(stderr, \"%s: %3u%% (%\"PRIuMAX\"/%\"PRIuMAX\")%s%s\",\n-\t\t\t\t\tprogress->title, percent,\n-\t\t\t\t\t(uintmax_t)n, (uintmax_t)progress->total,\n-\t\t\t\t\ttp, eol);\n-\t\t\t\tfflush(stderr);\n-\t\t\t}\n+\t\t\tfprintf(stderr, \"%s: %3u%% (%\"PRIuMAX\"/%\"PRIuMAX\")%s%s\",\n+\t\t\t\tprogress->title, percent,\n+\t\t\t\t(uintmax_t)n, (uintmax_t)progress->total,\n+\t\t\t\ttp, eol);\n+\t\t\tfflush(stderr);\n \t\t\tprogress_update = 0;\n \t\t\treturn;\n \t\t}\n \t} else if (progress_update) {\n-\t\tif (is_foreground_fd(fileno(stderr)) || done) {\n-\t\t\tfprintf(stderr, \"%s: %\"PRIuMAX\"%s%s\",\n-\t\t\t\tprogress->title, (uintmax_t)n, tp, eol);\n-\t\t\tfflush(stderr);\n-\t\t}\n+\t\tfprintf(stderr, \"%s: %\"PRIuMAX\"%s%s\",\n+\t\t\tprogress->title, (uintmax_t)n, tp, eol);\n+\t\tfflush(stderr);\n \t\tprogress_update = 0;\n \t\treturn;\n \t}\n-- \n2.21.0.539.g07239c3a71.dirty\n\n"},{"id":"372410","messageId":"20190325103844.26749-6-szeder.dev@gmail.com","threadId":"50817","inReplyTo":"20190325103844.26749-1-szeder.dev@gmail.com","subject":"[PATCH 5/5] progress: break too long progress bar lines","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-03-25T10:38:44Z","receivedAt":"2019-03-25T10:39:09Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Some of the recently added progress indicators have quite long titles,\nwhich might be even longer when translated to some languages, and when\nthey are shown while operating on bigger repositories, then the\nprogress bar grows longer than the default 80 column terminal width.\n\nWhen the progress bar exceeds the width of the terminal it gets\nline-wrapped, and after that the CR at the end doesn't return to the\nbeginning of the progress bar, but to the first column of its last\nline. Consequently, the first line of the previously shown progress\nbar is not overwritten by the next, and we end up with a bunch of\ntruncated progress bar lines scrolling past:\n\n $ LANG=es_ES.UTF-8 git commit-graph write\n Encontrando commits para commit graph entre los objetos empaquetados: 2% (1599\n Encontrando commits para commit graph entre los objetos empaquetados: 3% (1975\n Encontrando commits para commit graph entre los objetos empaquetados: 4% (2633\n Encontrando commits para commit graph entre los objetos empaquetados: 5% (3292\n [...]\n\nPrevent this by breaking progress bars after the title once they\nexceed the width of the terminal, so the counter and optional\npercentage and throughput, i.e. all changing parts, are on the last\nline. Subsequent updates will from then on only refresh the changing\nparts, but not the title, and it will look like this:\n\n $ LANG=es_ES.UTF-8 ~/src/git/git commit-graph write\n Encontrando commits para commit graph entre los objetos empaquetados:\n 100% (6584502/6584502), listo.\n Calculando números de generación de commit graph: 100% (824705/824705), listo.\n Escribiendo commit graph en 4 pasos: 100% (3298820/3298820), listo.\n\nNote that the number of columns in the terminal is cached by\nterm_columns(), so this might not kick in when it should when a\nterminal window is resized while the operation is running.\nFurthermore, this change won't help if the terminal is so narrow that\nthe counters don't fit on one line, but I would put this in the \"If it\nhurts, don't do it\" box.\n\nSigned-off-by: SZEDER Gábor \n---\n progress.c | 24 +++++++++++++++++++++---\n 1 file changed, 21 insertions(+), 3 deletions(-)\n\ndiff --git a/progress.c b/progress.c\nindex b37a5398c5..36aaeea340 100644\n--- a/progress.c\n+++ b/progress.c\n@@ -8,7 +8,7 @@\n * published by the Free Software Foundation.\n */\n \n-#include \"git-compat-util.h\"\n+#include \"cache.h\"\n #include \"gettext.h\"\n #include \"progress.h\"\n #include \"strbuf.h\"\n@@ -37,6 +37,8 @@ struct progress {\n \tstruct throughput *throughput;\n \tuint64_t start_ns;\n \tstruct strbuf counters_sb;\n+\tint title_len;\n+\tint split;\n };\n \n static volatile sig_atomic_t progress_update;\n@@ -119,8 +121,22 @@ static void display(struct progress *progress, uint64_t n, const char *done)\n \t\tconst char *eol = done ? done : \"\\r\";\n \t\tint clear_len = counters_sb->len < last_count_len ?\n \t\t\t\tlast_count_len - counters_sb->len : 0;\n-\t\tfprintf(stderr, \"%s: %s%-*s\", progress->title,\n-\t\t\tcounters_sb->buf, clear_len, eol);\n+\t\tint cols = term_columns();\n+\n+\t\tif (progress->split) {\n+\t\t\tfprintf(stderr, \" %s%-*s\", counters_sb->buf, clear_len,\n+\t\t\t\teol);\n+\t\t} else if (!done &&\n+\t\t\t cols < progress->title_len + counters_sb->len + 2) {\n+\t\t\tclear_len = progress->title_len + 1 < cols ?\n+\t\t\t\t cols - progress->title_len - 1 : 0;\n+\t\t\tfprintf(stderr, \"%s:%*s\\n %s%s\", progress->title,\n+\t\t\t\t\tclear_len, \"\", counters_sb->buf, eol);\n+\t\t\tprogress->split = 1;\n+\t\t} else {\n+\t\t\tfprintf(stderr, \"%s: %s%-*s\", progress->title,\n+\t\t\t\tcounters_sb->buf, clear_len, eol);\n+\t\t}\n \t\tfflush(stderr);\n \t\tprogress_update = 0;\n \t}\n@@ -225,6 +241,8 @@ static struct progress *start_progress_delay(const char *title, uint64_t total,\n \tprogress->throughput = NULL;\n \tprogress->start_ns = getnanotime();\n \tstrbuf_init(&progress->counters_sb, 0);\n+\tprogress->title_len = strlen(title);\n+\tprogress->split = 0;\n \tset_progress_signal();\n \treturn progress;\n }\n-- \n2.21.0.539.g07239c3a71.dirty\n\n"},{"id":"372411","messageId":"CACsJy8AYC24tbDhq6LJ5FgZEcSsOYTuw-G88eXdKzbT6iP3NNw@mail.gmail.com","threadId":"50817","inReplyTo":"20190325103844.26749-6-szeder.dev@gmail.com","subject":"Re: [PATCH 5/5] progress: break too long progress bar lines","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-03-25T11:02:13Z","receivedAt":"2019-03-25T11:02:42Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Mar 25, 2019 at 5:41 PM SZEDER Gábor wrote:\n>\n> Some of the recently added progress indicators have quite long titles,\n> which might be even longer when translated to some languages, and when\n> they are shown while operating on bigger repositories, then the\n> progress bar grows longer than the default 80 column terminal width.\n>\n> When the progress bar exceeds the width of the terminal it gets\n> line-wrapped, and after that the CR at the end doesn't return to the\n> beginning of the progress bar, but to the first column of its last\n> line. Consequently, the first line of the previously shown progress\n> bar is not overwritten by the next, and we end up with a bunch of\n> truncated progress bar lines scrolling past:\n>\n> $ LANG=es_ES.UTF-8 git commit-graph write\n> Encontrando commits para commit graph entre los objetos empaquetados: 2% (1599\n> Encontrando commits para commit graph entre los objetos empaquetados: 3% (1975\n> Encontrando commits para commit graph entre los objetos empaquetados: 4% (2633\n> Encontrando commits para commit graph entre los objetos empaquetados: 5% (3292\n> [...]\n>\n> Prevent this by breaking progress bars after the title once they\n> exceed the width of the terminal, so the counter and optional\n> percentage and throughput, i.e. all changing parts, are on the last\n> line. Subsequent updates will from then on only refresh the changing\n> parts, but not the title, and it will look like this:\n>\n> $ LANG=es_ES.UTF-8 ~/src/git/git commit-graph write\n> Encontrando commits para commit graph entre los objetos empaquetados:\n> 100% (6584502/6584502), listo.\n> Calculando números de generación de commit graph: 100% (824705/824705), listo.\n> Escribiendo commit graph en 4 pasos: 100% (3298820/3298820), listo.\n>\n> Note that the number of columns in the terminal is cached by\n> term_columns(), so this might not kick in when it should when a\n> terminal window is resized while the operation is running.\n> Furthermore, this change won't help if the terminal is so narrow that\n> the counters don't fit on one line, but I would put this in the \"If it\n> hurts, don't do it\" box.\n>\n> Signed-off-by: SZEDER Gábor \n> ---\n> progress.c | 24 +++++++++++++++++++++---\n> 1 file changed, 21 insertions(+), 3 deletions(-)\n>\n> diff --git a/progress.c b/progress.c\n> index b37a5398c5..36aaeea340 100644\n> --- a/progress.c\n> +++ b/progress.c\n> @@ -8,7 +8,7 @@\n> * published by the Free Software Foundation.\n> */\n>\n> -#include \"git-compat-util.h\"\n> +#include \"cache.h\"\n> #include \"gettext.h\"\n> #include \"progress.h\"\n> #include \"strbuf.h\"\n> @@ -37,6 +37,8 @@ struct progress {\n> struct throughput *throughput;\n> uint64_t start_ns;\n> struct strbuf counters_sb;\n> + int title_len;\n> + int split;\n> };\n>\n> static volatile sig_atomic_t progress_update;\n> @@ -119,8 +121,22 @@ static void display(struct progress *progress, uint64_t n, const char *done)\n> const char *eol = done ? done : \"\\r\";\n> int clear_len = counters_sb->len < last_count_len ?\n> last_count_len - counters_sb->len : 0;\n> - fprintf(stderr, \"%s: %s%-*s\", progress->title,\n> - counters_sb->buf, clear_len, eol);\n> + int cols = term_columns();\n> +\n> + if (progress->split) {\n> + fprintf(stderr, \" %s%-*s\", counters_sb->buf, clear_len,\n> + eol);\n> + } else if (!done &&\n> + cols < progress->title_len + counters_sb->len + 2) {\n> + clear_len = progress->title_len + 1 < cols ?\n> + cols - progress->title_len - 1 : 0;\n> + fprintf(stderr, \"%s:%*s\\n %s%s\", progress->title,\n> + clear_len, \"\", counters_sb->buf, eol);\n> + progress->split = 1;\n> + } else {\n> + fprintf(stderr, \"%s: %s%-*s\", progress->title,\n> + counters_sb->buf, clear_len, eol);\n> + }\n> fflush(stderr);\n> progress_update = 0;\n> }\n> @@ -225,6 +241,8 @@ static struct progress *start_progress_delay(const char *title, uint64_t total,\n> progress->throughput = NULL;\n> progress->start_ns = getnanotime();\n> strbuf_init(&progress->counters_sb, 0);\n> + progress->title_len = strlen(title);\n\nI think you need utf8_strwidth() so that the \"cols < title_len\"\ncomparison above works for multibyte encoding too.\n\n> + progress->split = 0;\n> set_progress_signal();\n> return progress;\n> }\n> --\n> 2.21.0.539.g07239c3a71.dirty\n>\n\n\n-- \nDuy\n"},{"id":"372412","messageId":"8736nbcitc.fsf@evledraar.gmail.com","threadId":"50817","inReplyTo":"20190325103844.26749-3-szeder.dev@gmail.com","subject":"Re: [PATCH 2/5] progress: return early when in the background","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-25T11:08:47Z","receivedAt":"2019-03-25T11:08:56Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Mar 25 2019, SZEDER Gábor wrote:\n\n> When a git process runs in the background, it doesn't display\n> progress, only the final \"done\" line [1]. The condition to check that\n> are a bit too deep in the display() function, and thus it calculates\n> the progress percentage even when no progress will be displayed\n> anyway.\n>\n> Restructure the display() function to return early when we are in the\n> background, which prevents the unnecessary progress percentae\n> calculation, and make the function look a bit better by losing one\n> level of indentation.\n>\n> [1] 85cb8906f0 (progress: no progress in background, 2015-04-13)\n\nCC-ing the author of that patch.\n\n> Signed-off-by: SZEDER Gábor \n> ---\n> progress.c | 26 ++++++++++++++------------\n> 1 file changed, 14 insertions(+), 12 deletions(-)\n>\n> diff --git a/progress.c b/progress.c\n> index 02a20e7d58..b57c0dae16 100644\n> --- a/progress.c\n> +++ b/progress.c\n> @@ -86,28 +86,30 @@ static void display(struct progress *progress, uint64_t n, const char *done)\n> \t\treturn;\n>\n> \tprogress->last_value = n;\n> +\n> +\tif (!is_foreground_fd(fileno(stderr)) && !done) {\n> +\t\tprogress_update = 0;\n> +\t\treturn;\n> +\t}\n> +\n> \ttp = (progress->throughput) ? progress->throughput->display.buf : \"\";\n> \teol = done ? done : \" \\r\";\n> \tif (progress->total) {\n> \t\tunsigned percent = n * 100 / progress->total;\n> \t\tif (percent != progress->last_percent || progress_update) {\n> \t\t\tprogress->last_percent = percent;\n> -\t\t\tif (is_foreground_fd(fileno(stderr)) || done) {\n> -\t\t\t\tfprintf(stderr, \"%s: %3u%% (%\"PRIuMAX\"/%\"PRIuMAX\")%s%s\",\n> -\t\t\t\t\tprogress->title, percent,\n> -\t\t\t\t\t(uintmax_t)n, (uintmax_t)progress->total,\n> -\t\t\t\t\ttp, eol);\n> -\t\t\t\tfflush(stderr);\n> -\t\t\t}\n> +\t\t\tfprintf(stderr, \"%s: %3u%% (%\"PRIuMAX\"/%\"PRIuMAX\")%s%s\",\n> +\t\t\t\tprogress->title, percent,\n> +\t\t\t\t(uintmax_t)n, (uintmax_t)progress->total,\n> +\t\t\t\ttp, eol);\n> +\t\t\tfflush(stderr);\n> \t\t\tprogress_update = 0;\n> \t\t\treturn;\n> \t\t}\n> \t} else if (progress_update) {\n> -\t\tif (is_foreground_fd(fileno(stderr)) || done) {\n> -\t\t\tfprintf(stderr, \"%s: %\"PRIuMAX\"%s%s\",\n> -\t\t\t\tprogress->title, (uintmax_t)n, tp, eol);\n> -\t\t\tfflush(stderr);\n> -\t\t}\n> +\t\tfprintf(stderr, \"%s: %\"PRIuMAX\"%s%s\",\n> +\t\t\tprogress->title, (uintmax_t)n, tp, eol);\n> +\t\tfflush(stderr);\n> \t\tprogress_update = 0;\n> \t\treturn;\n> \t}\n\nThis patch looks good, just notes for potential follow-up:\n\n * Is the \"is_foreground_fd(fileno(stderr))\" case worth moving into\n start_progress_delay() & setting a variable? It's a few C lib calls &\n potential syscall (getpid(...)).\n\n * Is that \"|| done\" part in the \"progress_update\" case something that\n needs to happen? I.e. can we entirely skip the \"setup signal handler\"\n part in start_progress_delay() if we detect that we're not in the\n foreground, and then rely on the stop_progress() call to print the\n \"done\"?\n\n Although we set \"progress_update = 1\" in stop_progress_msg(), so it's\n not *just* the signal handler but also us \"faking\" it, and we'd still\n need to stash away \"progress->last_value = n\" in display() in that\n backgrounding case.\n\n So maybe it's as simple as it's going to get.\n"},{"id":"372413","messageId":"20190325111255.GK22459@szeder.dev","threadId":"50817","inReplyTo":"CACsJy8AYC24tbDhq6LJ5FgZEcSsOYTuw-G88eXdKzbT6iP3NNw@mail.gmail.com","subject":"Re: [PATCH 5/5] progress: break too long progress bar lines","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-03-25T11:12:55Z","receivedAt":"2019-03-25T11:13:07Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Mar 25, 2019 at 06:02:13PM +0700, Duy Nguyen wrote:\n> > @@ -225,6 +241,8 @@ static struct progress *start_progress_delay(const char *title, uint64_t total,\n> > progress->throughput = NULL;\n> > progress->start_ns = getnanotime();\n> > strbuf_init(&progress->counters_sb, 0);\n> > + progress->title_len = strlen(title);\n> \n> I think you need utf8_strwidth() so that the \"cols < title_len\"\n> comparison above works for multibyte encoding too.\n\nOh, indeed. I remember seeing the Chinese translation and wondering\nabout it, but then completely forgot by the time I got around to it.\n\n> > + progress->split = 0;\n> > set_progress_signal();\n> > return progress;\n> > }\n> > --\n> > 2.21.0.539.g07239c3a71.dirty\n> >\n> \n> \n> -- \n> Duy\n"},{"id":"372414","messageId":"20190325113913.GL22459@szeder.dev","threadId":"50817","inReplyTo":"8736nbcitc.fsf@evledraar.gmail.com","subject":"Re: [PATCH 2/5] progress: return early when in the background","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-03-25T11:39:13Z","receivedAt":"2019-03-25T11:39:20Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Mar 25, 2019 at 12:08:47PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Mon, Mar 25 2019, SZEDER Gábor wrote:\n> \n> > When a git process runs in the background, it doesn't display\n> > progress, only the final \"done\" line [1]. The condition to check that\n> > are a bit too deep in the display() function, and thus it calculates\n> > the progress percentage even when no progress will be displayed\n> > anyway.\n> >\n> > Restructure the display() function to return early when we are in the\n> > background, which prevents the unnecessary progress percentae\n> > calculation, and make the function look a bit better by losing one\n> > level of indentation.\n> >\n> > [1] 85cb8906f0 (progress: no progress in background, 2015-04-13)\n> \n> CC-ing the author of that patch.\n> \n> > Signed-off-by: SZEDER Gábor \n> > ---\n> > progress.c | 26 ++++++++++++++------------\n> > 1 file changed, 14 insertions(+), 12 deletions(-)\n> >\n> > diff --git a/progress.c b/progress.c\n> > index 02a20e7d58..b57c0dae16 100644\n> > --- a/progress.c\n> > +++ b/progress.c\n> > @@ -86,28 +86,30 @@ static void display(struct progress *progress, uint64_t n, const char *done)\n> > \t\treturn;\n> >\n> > \tprogress->last_value = n;\n> > +\n> > +\tif (!is_foreground_fd(fileno(stderr)) && !done) {\n> > +\t\tprogress_update = 0;\n> > +\t\treturn;\n> > +\t}\n> > +\n> > \ttp = (progress->throughput) ? progress->throughput->display.buf : \"\";\n> > \teol = done ? done : \" \\r\";\n> > \tif (progress->total) {\n> > \t\tunsigned percent = n * 100 / progress->total;\n> > \t\tif (percent != progress->last_percent || progress_update) {\n> > \t\t\tprogress->last_percent = percent;\n> > -\t\t\tif (is_foreground_fd(fileno(stderr)) || done) {\n> > -\t\t\t\tfprintf(stderr, \"%s: %3u%% (%\"PRIuMAX\"/%\"PRIuMAX\")%s%s\",\n> > -\t\t\t\t\tprogress->title, percent,\n> > -\t\t\t\t\t(uintmax_t)n, (uintmax_t)progress->total,\n> > -\t\t\t\t\ttp, eol);\n> > -\t\t\t\tfflush(stderr);\n> > -\t\t\t}\n> > +\t\t\tfprintf(stderr, \"%s: %3u%% (%\"PRIuMAX\"/%\"PRIuMAX\")%s%s\",\n> > +\t\t\t\tprogress->title, percent,\n> > +\t\t\t\t(uintmax_t)n, (uintmax_t)progress->total,\n> > +\t\t\t\ttp, eol);\n> > +\t\t\tfflush(stderr);\n> > \t\t\tprogress_update = 0;\n> > \t\t\treturn;\n> > \t\t}\n> > \t} else if (progress_update) {\n> > -\t\tif (is_foreground_fd(fileno(stderr)) || done) {\n> > -\t\t\tfprintf(stderr, \"%s: %\"PRIuMAX\"%s%s\",\n> > -\t\t\t\tprogress->title, (uintmax_t)n, tp, eol);\n> > -\t\t\tfflush(stderr);\n> > -\t\t}\n> > +\t\tfprintf(stderr, \"%s: %\"PRIuMAX\"%s%s\",\n> > +\t\t\tprogress->title, (uintmax_t)n, tp, eol);\n> > +\t\tfflush(stderr);\n> > \t\tprogress_update = 0;\n> > \t\treturn;\n> > \t}\n> \n> This patch looks good, just notes for potential follow-up:\n> \n> * Is the \"is_foreground_fd(fileno(stderr))\" case worth moving into\n> start_progress_delay() & setting a variable? It's a few C lib calls &\n> potential syscall (getpid(...)).\n\nIt depends on whether you consider the following case worth caring\nabout:\n\n $ git long-cmd\n \n Ctrl-Z!\n $ bg\n \n $ fg\n \n\nOr:\n\n $ git long-cmd &\n \n $ fg\n \n\nBy moving the is_foreground_fd() check to start_progress_delay() and\ncaching its result, in the first case we would print progress even\nafter the process is sent to the background, while in the second we\nwouldn't print progress even after the initially backgrounded process\nis brought to the foreground.\n\nI think the current behavior makes sense (though I'm not quite sure\nabout printing the final \"done\" line, as I think I would be annoyed if\nit were printed from the background process while I was typing a\nlonger command... but I don't run git commands in the background in\nthe first place)\n\n> * Is that \"|| done\" part in the \"progress_update\" case something that\n> needs to happen? I.e. can we entirely skip the \"setup signal handler\"\n> part in start_progress_delay() if we detect that we're not in the\n> foreground, and then rely on the stop_progress() call to print the\n> \"done\"?\n\nThis, too, depends on how (or whether at all) we want to handle the\nuser sending the process to the background and bringing it back.\n\n> Although we set \"progress_update = 1\" in stop_progress_msg(), so it's\n> not *just* the signal handler but also us \"faking\" it, and we'd still\n> need to stash away \"progress->last_value = n\" in display() in that\n> backgrounding case.\n> \n> So maybe it's as simple as it's going to get.\n\n"},{"id":"372494","messageId":"20190326053839.GC1933@sigill.intra.peff.net","threadId":"50817","inReplyTo":"20190325103844.26749-3-szeder.dev@gmail.com","subject":"Re: [PATCH 2/5] progress: return early when in the background","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-26T05:38:39Z","receivedAt":"2019-03-26T05:38:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 25, 2019 at 11:38:41AM +0100, SZEDER Gábor wrote:\n\n> diff --git a/progress.c b/progress.c\n> index 02a20e7d58..b57c0dae16 100644\n> --- a/progress.c\n> +++ b/progress.c\n> @@ -86,28 +86,30 @@ static void display(struct progress *progress, uint64_t n, const char *done)\n> \t\treturn;\n> \n> \tprogress->last_value = n;\n> +\n> +\tif (!is_foreground_fd(fileno(stderr)) && !done) {\n> +\t\tprogress_update = 0;\n> +\t\treturn;\n> +\t}\n> +\n\nMoving it here causes a measurable slowdown for:\n\n git rev-list --progress=foo --objects --all\n\nThis function gets called for every single increment of the progress\ncounter. Whereas in its old location:\n\n> \ttp = (progress->throughput) ? progress->throughput->display.buf : \"\";\n> \teol = done ? done : \" \\r\";\n> \tif (progress->total) {\n> \t\tunsigned percent = n * 100 / progress->total;\n> \t\tif (percent != progress->last_percent || progress_update) {\n> \t\t\tprogress->last_percent = percent;\n> -\t\t\tif (is_foreground_fd(fileno(stderr)) || done) {\n> -\t\t\t\tfprintf(stderr, \"%s: %3u%% (%\"PRIuMAX\"/%\"PRIuMAX\")%s%s\",\n> -\t\t\t\t\tprogress->title, percent,\n> -\t\t\t\t\t(uintmax_t)n, (uintmax_t)progress->total,\n> -\t\t\t\t\ttp, eol);\n> -\t\t\t\tfflush(stderr);\n> -\t\t\t}\n\nIt was only triggered when we accumulated enough increments to print. So\nwe save a few instructions in the backgrounded case, but it costs us a\nlot of extra syscalls in every other case.\n\nAccording to \"strace -c\", the number of ioctls for that rev-list on\ngit.git went from 6 to 373,461. But more importantly, my best-of-five\ntimings went from 3.340s from 3.407s. That's only 2%, but it would be\nnice not to pay it if we don't need to.\n\n-Peff\n"},{"id":"372495","messageId":"20190326054541.GD1933@sigill.intra.peff.net","threadId":"50817","inReplyTo":"20190325103844.26749-4-szeder.dev@gmail.com","subject":"Re: [PATCH 3/5] progress: assemble percentage and counters in a strbuf before printing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-26T05:45:41Z","receivedAt":"2019-03-26T05:45:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 25, 2019 at 11:38:42AM +0100, SZEDER Gábor wrote:\n\n> The following patches in this series want to handle the progress bar's\n> title and changing parts (i.e. the counter and the optional percentage\n> and throughput combined) differently.\n> \n> To prepare for those changes assemble the changing parts in a separate\n> strbuf before printing.\n\nMakes sense. Unlike the previous patch, we're already in the \"slow path\"\nof the function here, so we don't need to worry about adding an extra\nbuffer (though do still think it's worth reusing the same buffer each\ntime, as you do here).\n\n-Peff\n"},{"id":"372496","messageId":"20190326055218.GE1933@sigill.intra.peff.net","threadId":"50817","inReplyTo":"20190325103844.26749-1-szeder.dev@gmail.com","subject":"Re: [PATCH 0/5] Progress display fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-26T05:52:19Z","receivedAt":"2019-03-26T05:52:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 25, 2019 at 11:38:39AM +0100, SZEDER Gábor wrote:\n\n> This patch series fixes two progress display issues:\n> \n> - When showing throughput, and the both the total and the throughput\n> change units in the same update, than the previously shown\n> progress bar is not cleaned up properly:\n> \n> Receiving objects: 25% (2901/11603), 772.01 KiB | 733.00 KiB/s\n> Receiving objects: 27% (3133/11603), 1.43 MiB | 1.16 MiB/s s\n> \n> - When the progress bar is longer than the width of the terminal,\n> then we end up with a bunch of truncated progress bar lines\n> scrolling past:\n> \n> $ LANG=es_ES.UTF-8 git commit-graph write\n> Encontrando commits para commit graph entre los objetos empaquetados: 2% (1599\n> Encontrando commits para commit graph entre los objetos empaquetados: 3% (1975\n> Encontrando commits para commit graph entre los objetos empaquetados: 4% (2633\n> Encontrando commits para commit graph entre los objetos empaquetados: 5% (3292\n> [...]\n> \n> Patches 4 and 5 fix these two issues, while the first three are\n> minor preparatory cleanups and refactorings.\n\nWith the exception of the points raised in the thread, these look good\nto me. Seeing your later patches, I think it would not be too bad to\njust stick the is_foreground_fd() check inside the \"if (update)\" block\nyou introduce later.\n\n-Peff\n"},{"id":"372498","messageId":"20190326062858.GE4753@mewburn.net","threadId":"50817","inReplyTo":"20190325113913.GL22459@szeder.dev","subject":"Re: [PATCH 2/5] progress: return early when in the background","fromName":"Luke Mewburn","fromEmail":"luke@mewburn.net","sentAt":"2019-03-26T06:28:58Z","receivedAt":"2019-03-26T06:43:17Z","isPatch":true,"sender":{"key":"luke@mewburn.net","avatar":"https://avatars.githubusercontent.com/u/629143?v=4"},"body":"On 19-03-25 12:39, SZEDER Gábor wrote:\n | On Mon, Mar 25, 2019 at 12:08:47PM +0100, Ævar Arnfjörð Bjarmason wrote:\n | > \n | > On Mon, Mar 25 2019, SZEDER Gábor wrote:\n | > \n | > > When a git process runs in the background, it doesn't display\n | > > progress, only the final \"done\" line [1]. The condition to check that\n | > > are a bit too deep in the display() function, and thus it calculates\n | > > the progress percentage even when no progress will be displayed\n | > > anyway.\n | > >\n | > > Restructure the display() function to return early when we are in the\n | > > background, which prevents the unnecessary progress percentae\n | > > calculation, and make the function look a bit better by losing one\n | > > level of indentation.\n | > >\n | > > [1] 85cb8906f0 (progress: no progress in background, 2015-04-13)\n | > \n | > CC-ing the author of that patch.\n | > \n | > > Signed-off-by: SZEDER Gábor \n | > > ---\n | > > progress.c | 26 ++++++++++++++------------\n | > > 1 file changed, 14 insertions(+), 12 deletions(-)\n | > >\n | > > diff --git a/progress.c b/progress.c\n | > > index 02a20e7d58..b57c0dae16 100644\n | > > --- a/progress.c\n | > > +++ b/progress.c\n | > > @@ -86,28 +86,30 @@ static void display(struct progress *progress, uint64_t n, const char *done)\n | > > \t\treturn;\n | > >\n | > > \tprogress->last_value = n;\n | > > +\n | > > +\tif (!is_foreground_fd(fileno(stderr)) && !done) {\n | > > +\t\tprogress_update = 0;\n | > > +\t\treturn;\n | > > +\t}\n | > > +\n | > > \ttp = (progress->throughput) ? progress->throughput->display.buf : \"\";\n | > > \teol = done ? done : \" \\r\";\n | > > \tif (progress->total) {\n | > > \t\tunsigned percent = n * 100 / progress->total;\n | > > \t\tif (percent != progress->last_percent || progress_update) {\n | > > \t\t\tprogress->last_percent = percent;\n | > > -\t\t\tif (is_foreground_fd(fileno(stderr)) || done) {\n | > > -\t\t\t\tfprintf(stderr, \"%s: %3u%% (%\"PRIuMAX\"/%\"PRIuMAX\")%s%s\",\n | > > -\t\t\t\t\tprogress->title, percent,\n | > > -\t\t\t\t\t(uintmax_t)n, (uintmax_t)progress->total,\n | > > -\t\t\t\t\ttp, eol);\n | > > -\t\t\t\tfflush(stderr);\n | > > -\t\t\t}\n | > > +\t\t\tfprintf(stderr, \"%s: %3u%% (%\"PRIuMAX\"/%\"PRIuMAX\")%s%s\",\n | > > +\t\t\t\tprogress->title, percent,\n | > > +\t\t\t\t(uintmax_t)n, (uintmax_t)progress->total,\n | > > +\t\t\t\ttp, eol);\n | > > +\t\t\tfflush(stderr);\n | > > \t\t\tprogress_update = 0;\n | > > \t\t\treturn;\n | > > \t\t}\n | > > \t} else if (progress_update) {\n | > > -\t\tif (is_foreground_fd(fileno(stderr)) || done) {\n | > > -\t\t\tfprintf(stderr, \"%s: %\"PRIuMAX\"%s%s\",\n | > > -\t\t\t\tprogress->title, (uintmax_t)n, tp, eol);\n | > > -\t\t\tfflush(stderr);\n | > > -\t\t}\n | > > +\t\tfprintf(stderr, \"%s: %\"PRIuMAX\"%s%s\",\n | > > +\t\t\tprogress->title, (uintmax_t)n, tp, eol);\n | > > +\t\tfflush(stderr);\n | > > \t\tprogress_update = 0;\n | > > \t\treturn;\n | > > \t}\n | > \n | > This patch looks good, just notes for potential follow-up:\n | > \n | > * Is the \"is_foreground_fd(fileno(stderr))\" case worth moving into\n | > start_progress_delay() & setting a variable? It's a few C lib calls &\n | > potential syscall (getpid(...)).\n | \n | It depends on whether you consider the following case worth caring\n | about:\n | \n | $ git long-cmd\n | \n | Ctrl-Z!\n | $ bg\n | \n | $ fg\n | \n | \n | Or:\n | \n | $ git long-cmd &\n | \n | $ fg\n | \n | \n | By moving the is_foreground_fd() check to start_progress_delay() and\n | caching its result, in the first case we would print progress even\n | after the process is sent to the background, while in the second we\n | wouldn't print progress even after the initially backgrounded process\n | is brought to the foreground.\n | \n | I think the current behavior makes sense (though I'm not quite sure\n | about printing the final \"done\" line, as I think I would be annoyed if\n | it were printed from the background process while I was typing a\n | longer command... but I don't run git commands in the background in\n | the first place)\n\nYou've described the current behaviour as I intended it\nin the original patch. I.e,:\n- Display progress if foreground.\n- Suppress output if background.\n- Check the foreground/background state each update in case it changed.\n\nI based that on other tools that also dynamically change their\noutput/progress behaviour whether in the foreground or background.\n\nRegarding the final \"done\" line; I think that's a matter of\npersonal preference. I'm not too fussed if that was changed\nso that \"done\" isn't printed if in the background.\n\n\n | \n | > * Is that \"|| done\" part in the \"progress_update\" case something that\n | > needs to happen? I.e. can we entirely skip the \"setup signal handler\"\n | > part in start_progress_delay() if we detect that we're not in the\n | > foreground, and then rely on the stop_progress() call to print the\n | > \"done\"?\n | \n | This, too, depends on how (or whether at all) we want to handle the\n | user sending the process to the background and bringing it back.\n | \n | > Although we set \"progress_update = 1\" in stop_progress_msg(), so it's\n | > not *just* the signal handler but also us \"faking\" it, and we'd still\n | > need to stash away \"progress->last_value = n\" in display() in that\n | > backgrounding case.\n | > \n | > So maybe it's as simple as it's going to get.\n | \n\n\n\nregards,\nLuke.\n"},{"id":"372566","messageId":"20190327102451.GC32732@szeder.dev","threadId":"50817","inReplyTo":"20190326054541.GD1933@sigill.intra.peff.net","subject":"Re: [PATCH 3/5] progress: assemble percentage and counters in a strbuf before printing","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-03-27T10:24:51Z","receivedAt":"2019-03-27T10:24:56Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Mar 26, 2019 at 01:45:41AM -0400, Jeff King wrote:\n> On Mon, Mar 25, 2019 at 11:38:42AM +0100, SZEDER Gábor wrote:\n> \n> > The following patches in this series want to handle the progress bar's\n> > title and changing parts (i.e. the counter and the optional percentage\n> > and throughput combined) differently.\n> > \n> > To prepare for those changes assemble the changing parts in a separate\n> > strbuf before printing.\n> \n> Makes sense. Unlike the previous patch, we're already in the \"slow path\"\n> of the function here, so we don't need to worry about adding an extra\n> buffer (though do still think it's worth reusing the same buffer each\n> time, as you do here).\n\nThe commit message doesn't mention this, but the next patch needs the\nlength of the previously displayed progress bar to properly clean up\nits remnants. Or the length of its changing parts anyway. So I could\neither add a 'prev_len' field to 'struct progress', or the whole\nstrbuf. The strbuf containing the throughput is already stored in\nthere and reused, so I just followed suit.\n\n"},{"id":"372601","messageId":"20190328021217.GC7887@sigill.intra.peff.net","threadId":"50817","inReplyTo":"20190327102451.GC32732@szeder.dev","subject":"Re: [PATCH 3/5] progress: assemble percentage and counters in a strbuf before printing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-28T02:12:17Z","receivedAt":"2019-03-28T02:12:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 27, 2019 at 11:24:51AM +0100, SZEDER Gábor wrote:\n\n> On Tue, Mar 26, 2019 at 01:45:41AM -0400, Jeff King wrote:\n> > On Mon, Mar 25, 2019 at 11:38:42AM +0100, SZEDER Gábor wrote:\n> > \n> > > The following patches in this series want to handle the progress bar's\n> > > title and changing parts (i.e. the counter and the optional percentage\n> > > and throughput combined) differently.\n> > > \n> > > To prepare for those changes assemble the changing parts in a separate\n> > > strbuf before printing.\n> > \n> > Makes sense. Unlike the previous patch, we're already in the \"slow path\"\n> > of the function here, so we don't need to worry about adding an extra\n> > buffer (though do still think it's worth reusing the same buffer each\n> > time, as you do here).\n> \n> The commit message doesn't mention this, but the next patch needs the\n> length of the previously displayed progress bar to properly clean up\n> its remnants. Or the length of its changing parts anyway. So I could\n> either add a 'prev_len' field to 'struct progress', or the whole\n> strbuf. The strbuf containing the throughput is already stored in\n> there and reused, so I just followed suit.\n\nAh, right, that makes perfect sense. It is doubly a good idea, then. :)\n\n-Peff\n"},{"id":"372877","messageId":"20190401115217.3423-2-szeder.dev@gmail.com","threadId":"50817","inReplyTo":"20190401115217.3423-1-szeder.dev@gmail.com","subject":"[PATCH v2 1/4] progress: make display_progress() return void","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-04-01T11:52:14Z","receivedAt":"2019-04-01T11:52:26Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Ever since the progress infrastructure was introduced in 96a02f8f6d\n(common progress display support, 2007-04-18), display_progress() has\nreturned an int, telling callers whether it updated the progress bar\nor not. However, this is:\n\n - useless, because over the last dozen years there has never been a\n single caller that cared about that return value.\n\n - not quite true, because it doesn't print a progress bar when\n running in the background, yet it returns 1; see 85cb8906f0\n (progress: no progress in background, 2015-04-13).\n\nThe related display_throughput() function returned void already upon\nits introduction in cf84d51c43 (add throughput to progress display,\n2007-10-30).\n\nLet's make display_progress() return void, too.\n\nSigned-off-by: SZEDER Gábor \n---\n progress.c | 15 ++++++++-------\n progress.h | 2 +-\n 2 files changed, 9 insertions(+), 8 deletions(-)\n\ndiff --git a/progress.c b/progress.c\nindex 5a99c9fbf0..02a20e7d58 100644\n--- a/progress.c\n+++ b/progress.c\n@@ -78,12 +78,12 @@ static int is_foreground_fd(int fd)\n \treturn tpgrp < 0 || tpgrp == getpgid(0);\n }\n \n-static int display(struct progress *progress, uint64_t n, const char *done)\n+static void display(struct progress *progress, uint64_t n, const char *done)\n {\n \tconst char *eol, *tp;\n \n \tif (progress->delay && (!progress_update || --progress->delay))\n-\t\treturn 0;\n+\t\treturn;\n \n \tprogress->last_value = n;\n \ttp = (progress->throughput) ? progress->throughput->display.buf : \"\";\n@@ -100,7 +100,7 @@ static int display(struct progress *progress, uint64_t n, const char *done)\n \t\t\t\tfflush(stderr);\n \t\t\t}\n \t\t\tprogress_update = 0;\n-\t\t\treturn 1;\n+\t\t\treturn;\n \t\t}\n \t} else if (progress_update) {\n \t\tif (is_foreground_fd(fileno(stderr)) || done) {\n@@ -109,10 +109,10 @@ static int display(struct progress *progress, uint64_t n, const char *done)\n \t\t\tfflush(stderr);\n \t\t}\n \t\tprogress_update = 0;\n-\t\treturn 1;\n+\t\treturn;\n \t}\n \n-\treturn 0;\n+\treturn;\n }\n \n static void throughput_string(struct strbuf *buf, uint64_t total,\n@@ -188,9 +188,10 @@ void display_throughput(struct progress *progress, uint64_t total)\n \t\tdisplay(progress, progress->last_value, NULL);\n }\n \n-int display_progress(struct progress *progress, uint64_t n)\n+void display_progress(struct progress *progress, uint64_t n)\n {\n-\treturn progress ? display(progress, n, NULL) : 0;\n+\tif (progress)\n+\t\tdisplay(progress, n, NULL);\n }\n \n static struct progress *start_progress_delay(const char *title, uint64_t total,\ndiff --git a/progress.h b/progress.h\nindex 70a4d4a0d6..59e40cc4fd 100644\n--- a/progress.h\n+++ b/progress.h\n@@ -4,7 +4,7 @@\n struct progress;\n \n void display_throughput(struct progress *progress, uint64_t total);\n-int display_progress(struct progress *progress, uint64_t n);\n+void display_progress(struct progress *progress, uint64_t n);\n struct progress *start_progress(const char *title, uint64_t total);\n struct progress *start_delayed_progress(const char *title, uint64_t total);\n void stop_progress(struct progress **progress);\n-- \n2.21.0.539.g07239c3a71.dirty\n\n"},{"id":"372878","messageId":"20190401115217.3423-1-szeder.dev@gmail.com","threadId":"50817","inReplyTo":"20190325103844.26749-1-szeder.dev@gmail.com","subject":"[PATCH v2 0/4] Progress display fixes","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-04-01T11:52:13Z","receivedAt":"2019-04-01T11:52:27Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"This patch series fixes two progress display issues:\n\n - When showing throughput, and the both the total and the throughput\n change units in the same update, than the previously shown\n progress bar is not cleaned up properly:\n\n Receiving objects: 25% (2901/11603), 772.01 KiB | 733.00 KiB/s\n Receiving objects: 27% (3133/11603), 1.43 MiB | 1.16 MiB/s s\n\n - When the progress bar is longer than the width of the terminal,\n then we end up with a bunch of truncated progress bar lines\n scrolling past:\n\n $ LANG=es_ES.UTF-8 git commit-graph write\n Encontrando commits para commit graph entre los objetos empaquetados: 2% (1599\n Encontrando commits para commit graph entre los objetos empaquetados: 3% (1975\n Encontrando commits para commit graph entre los objetos empaquetados: 4% (2633\n Encontrando commits para commit graph entre los objetos empaquetados: 5% (3292\n [...]\n\nPatches 3 and 4 fix these two issues, while the first three are\nminor preparatory cleanups and refactorings.\n\n\nChanges since v1:\n\n - Use utf8_strwidth().\n\n - Dropped patch 2/5 (progress: return early when in the background).\n\n Consequently, the new patch 2/4 (progress: assemble percentage and\n counters in a strbuf before printing; was patch 3/5 in v1) moves\n the is_foreground_fd() check around a bit, resulting in enough\n changes that range-diff can't match the new patch 3/4 to the old\n 4/5. With increased '--creation-factor' it's able to line up\n those two patches, and shows the updates to the commit message,\n but the resulting diff-of-a-diff looks utterly unreadable to me.\n I've included an interdiff as well, as I find it much more\n telling.\n\n - Commit message updates.\n\n\nSZEDER Gábor (4):\n progress: make display_progress() return void\n progress: assemble percentage and counters in a strbuf before printing\n progress: clear previous progress update dynamically\n progress: break too long progress bar lines\n\n progress.c | 73 +++++++++++++++++++++++++++++++++++++++---------------\n progress.h | 2 +-\n 2 files changed, 54 insertions(+), 21 deletions(-)\n\nInterdiff:\ndiff --git a/progress.c b/progress.c\nindex 97bc4b04e8..e28ccdafd2 100644\n--- a/progress.c\n+++ b/progress.c\n@@ -86,19 +86,13 @@ static void display(struct progress *progress, uint64_t n, const char *done)\n {\n \tconst char *tp;\n \tstruct strbuf *counters_sb = &progress->counters_sb;\n-\tint update = 0;\n+\tint show_update = 0;\n \tint last_count_len = counters_sb->len;\n \n \tif (progress->delay && (!progress_update || --progress->delay))\n \t\treturn;\n \n \tprogress->last_value = n;\n-\n-\tif (!is_foreground_fd(fileno(stderr)) && !done) {\n-\t\tprogress_update = 0;\n-\t\treturn;\n-\t}\n-\n \ttp = (progress->throughput) ? progress->throughput->display.buf : \"\";\n \tif (progress->total) {\n \t\tunsigned percent = n * 100 / progress->total;\n@@ -110,35 +104,39 @@ static void display(struct progress *progress, uint64_t n, const char *done)\n \t\t\t\t \"%3u%% (%\"PRIuMAX\"/%\"PRIuMAX\")%s\", percent,\n \t\t\t\t (uintmax_t)n, (uintmax_t)progress->total,\n \t\t\t\t tp);\n-\t\t\tupdate = 1;\n+\t\t\tshow_update = 1;\n \t\t}\n \t} else if (progress_update) {\n \t\tstrbuf_reset(counters_sb);\n \t\tstrbuf_addf(counters_sb, \"%\"PRIuMAX\"%s\", (uintmax_t)n, tp);\n-\t\tupdate = 1;\n+\t\tshow_update = 1;\n \t}\n \n-\tif (update) {\n-\t\tconst char *eol = done ? done : \"\\r\";\n-\t\tint clear_len = counters_sb->len < last_count_len ?\n-\t\t\t\tlast_count_len - counters_sb->len : 0;\n-\t\tint cols = term_columns();\n-\n-\t\tif (progress->split) {\n-\t\t\tfprintf(stderr, \" %s%-*s\", counters_sb->buf, clear_len,\n-\t\t\t\teol);\n-\t\t} else if (!done &&\n-\t\t\t cols < progress->title_len + counters_sb->len + 2) {\n-\t\t\tclear_len = progress->title_len + 1 < cols ?\n-\t\t\t\t cols - progress->title_len - 1 : 0;\n-\t\t\tfprintf(stderr, \"%s:%*s\\n %s%s\", progress->title,\n-\t\t\t\t\tclear_len, \"\", counters_sb->buf, eol);\n-\t\t\tprogress->split = 1;\n-\t\t} else {\n-\t\t\tfprintf(stderr, \"%s: %s%-*s\", progress->title,\n-\t\t\t\tcounters_sb->buf, clear_len, eol);\n+\tif (show_update) {\n+\t\tif (is_foreground_fd(fileno(stderr)) || done) {\n+\t\t\tconst char *eol = done ? done : \"\\r\";\n+\t\t\tint clear_len = counters_sb->len < last_count_len ?\n+\t\t\t\t\tlast_count_len - counters_sb->len : 0;\n+\t\t\tint progress_line_len = progress->title_len +\n+\t\t\t\t\t\tcounters_sb->len + 2;\n+\t\t\tint cols = term_columns();\n+\n+\t\t\tif (progress->split) {\n+\t\t\t\tfprintf(stderr, \" %s%-*s\", counters_sb->buf,\n+\t\t\t\t\tclear_len, eol);\n+\t\t\t} else if (!done && cols < progress_line_len) {\n+\t\t\t\tclear_len = progress->title_len + 1 < cols ?\n+\t\t\t\t\t cols - progress->title_len - 1 : 0;\n+\t\t\t\tfprintf(stderr, \"%s:%*s\\n %s%s\",\n+\t\t\t\t\tprogress->title, clear_len, \"\",\n+\t\t\t\t\tcounters_sb->buf, eol);\n+\t\t\t\tprogress->split = 1;\n+\t\t\t} else {\n+\t\t\t\tfprintf(stderr, \"%s: %s%-*s\", progress->title,\n+\t\t\t\t\tcounters_sb->buf, clear_len, eol);\n+\t\t\t}\n+\t\t\tfflush(stderr);\n \t\t}\n-\t\tfflush(stderr);\n \t\tprogress_update = 0;\n \t}\n \n@@ -242,7 +240,7 @@ static struct progress *start_progress_delay(const char *title, uint64_t total,\n \tprogress->throughput = NULL;\n \tprogress->start_ns = getnanotime();\n \tstrbuf_init(&progress->counters_sb, 0);\n-\tprogress->title_len = strlen(title);\n+\tprogress->title_len = utf8_strwidth(title);\n \tprogress->split = 0;\n \tset_progress_signal();\n \treturn progress;\nRange-diff:\n1: dea36bd2a7 = 1: dea36bd2a7 progress: make display_progress() return void\n2: 159a0b9561 < -: ---------- progress: return early when in the background\n3: ecd6b0fd68 ! 2: 97de2a98a0 progress: assemble percentage and counters in a strbuf before printing\n @@ -4,10 +4,11 @@\n \n The following patches in this series want to handle the progress bar's\n title and changing parts (i.e. the counter and the optional percentage\n - and throughput combined) differently.\n + and throughput combined) differently, and need to know the length\n + of the changing parts of the previously displayed progress bar.\n \n To prepare for those changes assemble the changing parts in a separate\n - strbuf before printing.\n + strbuf kept in 'struct progress' before printing.\n \n Signed-off-by: SZEDER Gábor \n \n @@ -29,24 +30,25 @@\n -\tconst char *eol, *tp;\n +\tconst char *tp;\n +\tstruct strbuf *counters_sb = &progress->counters_sb;\n -+\tint update = 0;\n ++\tint show_update = 0;\n \n \tif (progress->delay && (!progress_update || --progress->delay))\n \t\treturn;\n -@@\n - \t}\n \n + \tprogress->last_value = n;\n \ttp = (progress->throughput) ? progress->throughput->display.buf : \"\";\n -\teol = done ? done : \" \\r\";\n \tif (progress->total) {\n \t\tunsigned percent = n * 100 / progress->total;\n \t\tif (percent != progress->last_percent || progress_update) {\n \t\t\tprogress->last_percent = percent;\n --\t\t\tfprintf(stderr, \"%s: %3u%% (%\"PRIuMAX\"/%\"PRIuMAX\")%s%s\",\n --\t\t\t\tprogress->title, percent,\n --\t\t\t\t(uintmax_t)n, (uintmax_t)progress->total,\n --\t\t\t\ttp, eol);\n --\t\t\tfflush(stderr);\n +-\t\t\tif (is_foreground_fd(fileno(stderr)) || done) {\n +-\t\t\t\tfprintf(stderr, \"%s: %3u%% (%\"PRIuMAX\"/%\"PRIuMAX\")%s%s\",\n +-\t\t\t\t\tprogress->title, percent,\n +-\t\t\t\t\t(uintmax_t)n, (uintmax_t)progress->total,\n +-\t\t\t\t\ttp, eol);\n +-\t\t\t\tfflush(stderr);\n +-\t\t\t}\n -\t\t\tprogress_update = 0;\n -\t\t\treturn;\n +\n @@ -55,22 +57,24 @@\n +\t\t\t\t \"%3u%% (%\"PRIuMAX\"/%\"PRIuMAX\")%s\", percent,\n +\t\t\t\t (uintmax_t)n, (uintmax_t)progress->total,\n +\t\t\t\t tp);\n -+\t\t\tupdate = 1;\n ++\t\t\tshow_update = 1;\n \t\t}\n \t} else if (progress_update) {\n --\t\tfprintf(stderr, \"%s: %\"PRIuMAX\"%s%s\",\n --\t\t\tprogress->title, (uintmax_t)n, tp, eol);\n +\t\tstrbuf_reset(counters_sb);\n +\t\tstrbuf_addf(counters_sb, \"%\"PRIuMAX\"%s\", (uintmax_t)n, tp);\n -+\t\tupdate = 1;\n ++\t\tshow_update = 1;\n +\t}\n +\n -+\tif (update) {\n -+\t\tconst char *eol = done ? done : \" \\r\";\n ++\tif (show_update) {\n + \t\tif (is_foreground_fd(fileno(stderr)) || done) {\n +-\t\t\tfprintf(stderr, \"%s: %\"PRIuMAX\"%s%s\",\n +-\t\t\t\tprogress->title, (uintmax_t)n, tp, eol);\n ++\t\t\tconst char *eol = done ? done : \" \\r\";\n +\n -+\t\tfprintf(stderr, \"%s: %s%s\", progress->title, counters_sb->buf,\n -+\t\t\teol);\n - \t\tfflush(stderr);\n ++\t\t\tfprintf(stderr, \"%s: %s%s\", progress->title,\n ++\t\t\t\tcounters_sb->buf, eol);\n + \t\t\tfflush(stderr);\n + \t\t}\n \t\tprogress_update = 0;\n -\t\treturn;\n \t}\n4: e360365f18 ! 3: edfe0157a7 progress: clear previous progress update dynamically\n @@ -10,7 +10,7 @@\n Alas, covering only three characters is not quite enough: when both\n the total and the throughput happen to change units from KiB to MiB in\n the same update, then the progress bar's length is shortened by four\n - characters:\n + characters (or maybe even more!):\n \n Receiving objects: 25% (2901/11603), 772.01 KiB | 733.00 KiB/s\n Receiving objects: 27% (3133/11603), 1.43 MiB | 1.16 MiB/s s\n @@ -21,11 +21,10 @@\n the length of the current progress bar with the previous one, and\n cover up as many characters as needed.\n \n - Sure, it would be much simpler to just print four spaces instead of\n - three at the end of the progress bar, but this approach is more\n - future-proof, and it won't print extra spaces when none are needed,\n - notably when the progress bar doesn't show throughput and thus never\n - shrinks.\n + Sure, it would be much simpler to just print more spaces at the end of\n + the progress bar, but this approach is more future-proof, and it won't\n + print extra spaces when none are needed, notably when the progress bar\n + doesn't show throughput and thus never shrinks.\n \n Signed-off-by: SZEDER Gábor \n \n @@ -35,24 +34,24 @@\n @@\n \tconst char *tp;\n \tstruct strbuf *counters_sb = &progress->counters_sb;\n - \tint update = 0;\n + \tint show_update = 0;\n +\tint last_count_len = counters_sb->len;\n \n \tif (progress->delay && (!progress_update || --progress->delay))\n \t\treturn;\n @@\n - \t}\n \n - \tif (update) {\n --\t\tconst char *eol = done ? done : \" \\r\";\n + \tif (show_update) {\n + \t\tif (is_foreground_fd(fileno(stderr)) || done) {\n +-\t\t\tconst char *eol = done ? done : \" \\r\";\n -\n --\t\tfprintf(stderr, \"%s: %s%s\", progress->title, counters_sb->buf,\n --\t\t\teol);\n -+\t\tconst char *eol = done ? done : \"\\r\";\n -+\t\tint clear_len = counters_sb->len < last_count_len ?\n -+\t\t\t\tlast_count_len - counters_sb->len : 0;\n -+\t\tfprintf(stderr, \"%s: %s%-*s\", progress->title,\n -+\t\t\tcounters_sb->buf, clear_len, eol);\n - \t\tfflush(stderr);\n +-\t\t\tfprintf(stderr, \"%s: %s%s\", progress->title,\n +-\t\t\t\tcounters_sb->buf, eol);\n ++\t\t\tconst char *eol = done ? done : \"\\r\";\n ++\t\t\tint clear_len = counters_sb->len < last_count_len ?\n ++\t\t\t\t\tlast_count_len - counters_sb->len : 0;\n ++\t\t\tfprintf(stderr, \"%s: %s%-*s\", progress->title,\n ++\t\t\t\tcounters_sb->buf, clear_len, eol);\n + \t\t\tfflush(stderr);\n + \t\t}\n \t\tprogress_update = 0;\n - \t}\n5: cbd3be1c6d ! 4: d53de231ee progress: break too long progress bar lines\n @@ -69,35 +69,37 @@\n \n static volatile sig_atomic_t progress_update;\n @@\n - \t\tconst char *eol = done ? done : \"\\r\";\n - \t\tint clear_len = counters_sb->len < last_count_len ?\n - \t\t\t\tlast_count_len - counters_sb->len : 0;\n --\t\tfprintf(stderr, \"%s: %s%-*s\", progress->title,\n --\t\t\tcounters_sb->buf, clear_len, eol);\n -+\t\tint cols = term_columns();\n + \t\t\tconst char *eol = done ? done : \"\\r\";\n + \t\t\tint clear_len = counters_sb->len < last_count_len ?\n + \t\t\t\t\tlast_count_len - counters_sb->len : 0;\n +-\t\t\tfprintf(stderr, \"%s: %s%-*s\", progress->title,\n +-\t\t\t\tcounters_sb->buf, clear_len, eol);\n ++\t\t\tint progress_line_len = progress->title_len +\n ++\t\t\t\t\t\tcounters_sb->len + 2;\n ++\t\t\tint cols = term_columns();\n +\n -+\t\tif (progress->split) {\n -+\t\t\tfprintf(stderr, \" %s%-*s\", counters_sb->buf, clear_len,\n -+\t\t\t\teol);\n -+\t\t} else if (!done &&\n -+\t\t\t cols < progress->title_len + counters_sb->len + 2) {\n -+\t\t\tclear_len = progress->title_len + 1 < cols ?\n -+\t\t\t\t cols - progress->title_len - 1 : 0;\n -+\t\t\tfprintf(stderr, \"%s:%*s\\n %s%s\", progress->title,\n -+\t\t\t\t\tclear_len, \"\", counters_sb->buf, eol);\n -+\t\t\tprogress->split = 1;\n -+\t\t} else {\n -+\t\t\tfprintf(stderr, \"%s: %s%-*s\", progress->title,\n -+\t\t\t\tcounters_sb->buf, clear_len, eol);\n -+\t\t}\n - \t\tfflush(stderr);\n ++\t\t\tif (progress->split) {\n ++\t\t\t\tfprintf(stderr, \" %s%-*s\", counters_sb->buf,\n ++\t\t\t\t\tclear_len, eol);\n ++\t\t\t} else if (!done && cols < progress_line_len) {\n ++\t\t\t\tclear_len = progress->title_len + 1 < cols ?\n ++\t\t\t\t\t cols - progress->title_len - 1 : 0;\n ++\t\t\t\tfprintf(stderr, \"%s:%*s\\n %s%s\",\n ++\t\t\t\t\tprogress->title, clear_len, \"\",\n ++\t\t\t\t\tcounters_sb->buf, eol);\n ++\t\t\t\tprogress->split = 1;\n ++\t\t\t} else {\n ++\t\t\t\tfprintf(stderr, \"%s: %s%-*s\", progress->title,\n ++\t\t\t\t\tcounters_sb->buf, clear_len, eol);\n ++\t\t\t}\n + \t\t\tfflush(stderr);\n + \t\t}\n \t\tprogress_update = 0;\n - \t}\n @@\n \tprogress->throughput = NULL;\n \tprogress->start_ns = getnanotime();\n \tstrbuf_init(&progress->counters_sb, 0);\n -+\tprogress->title_len = strlen(title);\n ++\tprogress->title_len = utf8_strwidth(title);\n +\tprogress->split = 0;\n \tset_progress_signal();\n \treturn progress;\n-- \n2.21.0.539.g07239c3a71.dirty\n\n"},{"id":"372879","messageId":"20190401115217.3423-3-szeder.dev@gmail.com","threadId":"50817","inReplyTo":"20190401115217.3423-1-szeder.dev@gmail.com","subject":"[PATCH v2 2/4] progress: assemble percentage and counters in a strbuf before printing","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-04-01T11:52:15Z","receivedAt":"2019-04-01T11:52:30Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The following patches in this series want to handle the progress bar's\ntitle and changing parts (i.e. the counter and the optional percentage\nand throughput combined) differently, and need to know the length\nof the changing parts of the previously displayed progress bar.\n\nTo prepare for those changes assemble the changing parts in a separate\nstrbuf kept in 'struct progress' before printing.\n\nSigned-off-by: SZEDER Gábor \n---\n progress.c | 37 +++++++++++++++++++++++--------------\n 1 file changed, 23 insertions(+), 14 deletions(-)\n\ndiff --git a/progress.c b/progress.c\nindex 02a20e7d58..842db14b72 100644\n--- a/progress.c\n+++ b/progress.c\n@@ -36,6 +36,7 @@ struct progress {\n \tunsigned delay;\n \tstruct throughput *throughput;\n \tuint64_t start_ns;\n+\tstruct strbuf counters_sb;\n };\n \n static volatile sig_atomic_t progress_update;\n@@ -80,36 +81,42 @@ static int is_foreground_fd(int fd)\n \n static void display(struct progress *progress, uint64_t n, const char *done)\n {\n-\tconst char *eol, *tp;\n+\tconst char *tp;\n+\tstruct strbuf *counters_sb = &progress->counters_sb;\n+\tint show_update = 0;\n \n \tif (progress->delay && (!progress_update || --progress->delay))\n \t\treturn;\n \n \tprogress->last_value = n;\n \ttp = (progress->throughput) ? progress->throughput->display.buf : \"\";\n-\teol = done ? done : \" \\r\";\n \tif (progress->total) {\n \t\tunsigned percent = n * 100 / progress->total;\n \t\tif (percent != progress->last_percent || progress_update) {\n \t\t\tprogress->last_percent = percent;\n-\t\t\tif (is_foreground_fd(fileno(stderr)) || done) {\n-\t\t\t\tfprintf(stderr, \"%s: %3u%% (%\"PRIuMAX\"/%\"PRIuMAX\")%s%s\",\n-\t\t\t\t\tprogress->title, percent,\n-\t\t\t\t\t(uintmax_t)n, (uintmax_t)progress->total,\n-\t\t\t\t\ttp, eol);\n-\t\t\t\tfflush(stderr);\n-\t\t\t}\n-\t\t\tprogress_update = 0;\n-\t\t\treturn;\n+\n+\t\t\tstrbuf_reset(counters_sb);\n+\t\t\tstrbuf_addf(counters_sb,\n+\t\t\t\t \"%3u%% (%\"PRIuMAX\"/%\"PRIuMAX\")%s\", percent,\n+\t\t\t\t (uintmax_t)n, (uintmax_t)progress->total,\n+\t\t\t\t tp);\n+\t\t\tshow_update = 1;\n \t\t}\n \t} else if (progress_update) {\n+\t\tstrbuf_reset(counters_sb);\n+\t\tstrbuf_addf(counters_sb, \"%\"PRIuMAX\"%s\", (uintmax_t)n, tp);\n+\t\tshow_update = 1;\n+\t}\n+\n+\tif (show_update) {\n \t\tif (is_foreground_fd(fileno(stderr)) || done) {\n-\t\t\tfprintf(stderr, \"%s: %\"PRIuMAX\"%s%s\",\n-\t\t\t\tprogress->title, (uintmax_t)n, tp, eol);\n+\t\t\tconst char *eol = done ? done : \" \\r\";\n+\n+\t\t\tfprintf(stderr, \"%s: %s%s\", progress->title,\n+\t\t\t\tcounters_sb->buf, eol);\n \t\t\tfflush(stderr);\n \t\t}\n \t\tprogress_update = 0;\n-\t\treturn;\n \t}\n \n \treturn;\n@@ -211,6 +218,7 @@ static struct progress *start_progress_delay(const char *title, uint64_t total,\n \tprogress->delay = delay;\n \tprogress->throughput = NULL;\n \tprogress->start_ns = getnanotime();\n+\tstrbuf_init(&progress->counters_sb, 0);\n \tset_progress_signal();\n \treturn progress;\n }\n@@ -254,6 +262,7 @@ void stop_progress_msg(struct progress **p_progress, const char *msg)\n \t\tfree(buf);\n \t}\n \tclear_progress_signal();\n+\tstrbuf_release(&progress->counters_sb);\n \tif (progress->throughput)\n \t\tstrbuf_release(&progress->throughput->display);\n \tfree(progress->throughput);\n-- \n2.21.0.539.g07239c3a71.dirty\n\n"},{"id":"372880","messageId":"20190401115217.3423-5-szeder.dev@gmail.com","threadId":"50817","inReplyTo":"20190401115217.3423-1-szeder.dev@gmail.com","subject":"[PATCH v2 4/4] progress: break too long progress bar lines","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-04-01T11:52:17Z","receivedAt":"2019-04-01T11:52:31Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Some of the recently added progress indicators have quite long titles,\nwhich might be even longer when translated to some languages, and when\nthey are shown while operating on bigger repositories, then the\nprogress bar grows longer than the default 80 column terminal width.\n\nWhen the progress bar exceeds the width of the terminal it gets\nline-wrapped, and after that the CR at the end doesn't return to the\nbeginning of the progress bar, but to the first column of its last\nline. Consequently, the first line of the previously shown progress\nbar is not overwritten by the next, and we end up with a bunch of\ntruncated progress bar lines scrolling past:\n\n $ LANG=es_ES.UTF-8 git commit-graph write\n Encontrando commits para commit graph entre los objetos empaquetados: 2% (1599\n Encontrando commits para commit graph entre los objetos empaquetados: 3% (1975\n Encontrando commits para commit graph entre los objetos empaquetados: 4% (2633\n Encontrando commits para commit graph entre los objetos empaquetados: 5% (3292\n [...]\n\nPrevent this by breaking progress bars after the title once they\nexceed the width of the terminal, so the counter and optional\npercentage and throughput, i.e. all changing parts, are on the last\nline. Subsequent updates will from then on only refresh the changing\nparts, but not the title, and it will look like this:\n\n $ LANG=es_ES.UTF-8 ~/src/git/git commit-graph write\n Encontrando commits para commit graph entre los objetos empaquetados:\n 100% (6584502/6584502), listo.\n Calculando números de generación de commit graph: 100% (824705/824705), listo.\n Escribiendo commit graph en 4 pasos: 100% (3298820/3298820), listo.\n\nNote that the number of columns in the terminal is cached by\nterm_columns(), so this might not kick in when it should when a\nterminal window is resized while the operation is running.\nFurthermore, this change won't help if the terminal is so narrow that\nthe counters don't fit on one line, but I would put this in the \"If it\nhurts, don't do it\" box.\n\nSigned-off-by: SZEDER Gábor \n---\n progress.c | 27 ++++++++++++++++++++++++---\n 1 file changed, 24 insertions(+), 3 deletions(-)\n\ndiff --git a/progress.c b/progress.c\nindex 3149ecd460..e28ccdafd2 100644\n--- a/progress.c\n+++ b/progress.c\n@@ -8,11 +8,12 @@\n * published by the Free Software Foundation.\n */\n \n-#include \"git-compat-util.h\"\n+#include \"cache.h\"\n #include \"gettext.h\"\n #include \"progress.h\"\n #include \"strbuf.h\"\n #include \"trace.h\"\n+#include \"utf8.h\"\n \n #define TP_IDX_MAX 8\n \n@@ -37,6 +38,8 @@ struct progress {\n \tstruct throughput *throughput;\n \tuint64_t start_ns;\n \tstruct strbuf counters_sb;\n+\tint title_len;\n+\tint split;\n };\n \n static volatile sig_atomic_t progress_update;\n@@ -114,8 +117,24 @@ static void display(struct progress *progress, uint64_t n, const char *done)\n \t\t\tconst char *eol = done ? done : \"\\r\";\n \t\t\tint clear_len = counters_sb->len < last_count_len ?\n \t\t\t\t\tlast_count_len - counters_sb->len : 0;\n-\t\t\tfprintf(stderr, \"%s: %s%-*s\", progress->title,\n-\t\t\t\tcounters_sb->buf, clear_len, eol);\n+\t\t\tint progress_line_len = progress->title_len +\n+\t\t\t\t\t\tcounters_sb->len + 2;\n+\t\t\tint cols = term_columns();\n+\n+\t\t\tif (progress->split) {\n+\t\t\t\tfprintf(stderr, \" %s%-*s\", counters_sb->buf,\n+\t\t\t\t\tclear_len, eol);\n+\t\t\t} else if (!done && cols < progress_line_len) {\n+\t\t\t\tclear_len = progress->title_len + 1 < cols ?\n+\t\t\t\t\t cols - progress->title_len - 1 : 0;\n+\t\t\t\tfprintf(stderr, \"%s:%*s\\n %s%s\",\n+\t\t\t\t\tprogress->title, clear_len, \"\",\n+\t\t\t\t\tcounters_sb->buf, eol);\n+\t\t\t\tprogress->split = 1;\n+\t\t\t} else {\n+\t\t\t\tfprintf(stderr, \"%s: %s%-*s\", progress->title,\n+\t\t\t\t\tcounters_sb->buf, clear_len, eol);\n+\t\t\t}\n \t\t\tfflush(stderr);\n \t\t}\n \t\tprogress_update = 0;\n@@ -221,6 +240,8 @@ static struct progress *start_progress_delay(const char *title, uint64_t total,\n \tprogress->throughput = NULL;\n \tprogress->start_ns = getnanotime();\n \tstrbuf_init(&progress->counters_sb, 0);\n+\tprogress->title_len = utf8_strwidth(title);\n+\tprogress->split = 0;\n \tset_progress_signal();\n \treturn progress;\n }\n-- \n2.21.0.539.g07239c3a71.dirty\n\n"},{"id":"372881","messageId":"20190401115217.3423-4-szeder.dev@gmail.com","threadId":"50817","inReplyTo":"20190401115217.3423-1-szeder.dev@gmail.com","subject":"[PATCH v2 3/4] progress: clear previous progress update dynamically","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-04-01T11:52:16Z","receivedAt":"2019-04-01T11:52:33Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"When the progress bar includes throughput, its length can shorten as\nthe unit of display changes from KiB to MiB. To cover up remnants of\nthe previous progress bar when such a change of units happens we\nalways print three spaces at the end of the progress bar.\n\nAlas, covering only three characters is not quite enough: when both\nthe total and the throughput happen to change units from KiB to MiB in\nthe same update, then the progress bar's length is shortened by four\ncharacters (or maybe even more!):\n\n Receiving objects: 25% (2901/11603), 772.01 KiB | 733.00 KiB/s\n Receiving objects: 27% (3133/11603), 1.43 MiB | 1.16 MiB/s s\n\nand a stray 's' is left behind.\n\nSo instead of hard-coding the three characters to cover, let's compare\nthe length of the current progress bar with the previous one, and\ncover up as many characters as needed.\n\nSure, it would be much simpler to just print more spaces at the end of\nthe progress bar, but this approach is more future-proof, and it won't\nprint extra spaces when none are needed, notably when the progress bar\ndoesn't show throughput and thus never shrinks.\n\nSigned-off-by: SZEDER Gábor \n---\n progress.c | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/progress.c b/progress.c\nindex 842db14b72..3149ecd460 100644\n--- a/progress.c\n+++ b/progress.c\n@@ -84,6 +84,7 @@ static void display(struct progress *progress, uint64_t n, const char *done)\n \tconst char *tp;\n \tstruct strbuf *counters_sb = &progress->counters_sb;\n \tint show_update = 0;\n+\tint last_count_len = counters_sb->len;\n \n \tif (progress->delay && (!progress_update || --progress->delay))\n \t\treturn;\n@@ -110,10 +111,11 @@ static void display(struct progress *progress, uint64_t n, const char *done)\n \n \tif (show_update) {\n \t\tif (is_foreground_fd(fileno(stderr)) || done) {\n-\t\t\tconst char *eol = done ? done : \" \\r\";\n-\n-\t\t\tfprintf(stderr, \"%s: %s%s\", progress->title,\n-\t\t\t\tcounters_sb->buf, eol);\n+\t\t\tconst char *eol = done ? done : \"\\r\";\n+\t\t\tint clear_len = counters_sb->len < last_count_len ?\n+\t\t\t\t\tlast_count_len - counters_sb->len : 0;\n+\t\t\tfprintf(stderr, \"%s: %s%-*s\", progress->title,\n+\t\t\t\tcounters_sb->buf, clear_len, eol);\n \t\t\tfflush(stderr);\n \t\t}\n \t\tprogress_update = 0;\n-- \n2.21.0.539.g07239c3a71.dirty\n\n"},{"id":"372888","messageId":"20190401132959.GB11664@sigill.intra.peff.net","threadId":"50817","inReplyTo":"20190401115217.3423-4-szeder.dev@gmail.com","subject":"Re: [PATCH v2 3/4] progress: clear previous progress update dynamically","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-01T13:30:00Z","receivedAt":"2019-04-01T13:30:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 01, 2019 at 01:52:16PM +0200, SZEDER Gábor wrote:\n\n> diff --git a/progress.c b/progress.c\n> index 842db14b72..3149ecd460 100644\n> --- a/progress.c\n> +++ b/progress.c\n> @@ -84,6 +84,7 @@ static void display(struct progress *progress, uint64_t n, const char *done)\n> \tconst char *tp;\n> \tstruct strbuf *counters_sb = &progress->counters_sb;\n> \tint show_update = 0;\n> +\tint last_count_len = counters_sb->len;\n\nI don't think it could matter here, as these are meant to be smallish\nstrings, but I think we should get into the habit of using size_t\nconsistently to hold string lengths.\n\nIt makes auditing for integer overflow problems much simpler (this is on\nmy mind as I happen to be tracing some bugs around this the past few\ndays).\n\n(There are a few instances in the next patch, too. Other than this nit,\nthough, your series looks good to me).\n\n-Peff\n"},{"id":"372892","messageId":"20190401141542.GJ32732@szeder.dev","threadId":"50817","inReplyTo":"20190401132959.GB11664@sigill.intra.peff.net","subject":"Re: [PATCH v2 3/4] progress: clear previous progress update dynamically","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-04-01T14:15:42Z","receivedAt":"2019-04-01T14:15:48Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Apr 01, 2019 at 09:30:00AM -0400, Jeff King wrote:\n> On Mon, Apr 01, 2019 at 01:52:16PM +0200, SZEDER Gábor wrote:\n> \n> > diff --git a/progress.c b/progress.c\n> > index 842db14b72..3149ecd460 100644\n> > --- a/progress.c\n> > +++ b/progress.c\n> > @@ -84,6 +84,7 @@ static void display(struct progress *progress, uint64_t n, const char *done)\n> > \tconst char *tp;\n> > \tstruct strbuf *counters_sb = &progress->counters_sb;\n> > \tint show_update = 0;\n> > +\tint last_count_len = counters_sb->len;\n> \n> I don't think it could matter here, as these are meant to be smallish\n> strings, but I think we should get into the habit of using size_t\n> consistently to hold string lengths.\n> \n> It makes auditing for integer overflow problems much simpler (this is on\n> my mind as I happen to be tracing some bugs around this the past few\n> days).\n> \n> (There are a few instances in the next patch, too. Other than this nit,\n> though, your series looks good to me).\n\nI started with using size_t, but then switched to int, because:\n\n - After a bit of arithmetic I had to compare to term_columns()'s int\n return value anyway (in the next patch).\n\n - The second hunk in this patch adds these lines:\n\n int clear_len = counters_sb->len < last_count_len ?\n last_count_len - counters_sb->len : 0;\n fprintf(stderr, \"%s: %s%-*s\", progress->title,\n counters_sb->buf, clear_len, eol);\n\n Here 'clear_len' has to be int, because the printf() format \"%-*s\"\n expects an int, and otherwise -Werror=format= errors ensue. OK,\n it could be size_t, but then it must be casted to an int upon\n passing it to fprintf(), and after the next patch there will be\n three such calls.\n\nI could resend using size_t. Should I resend using size_t? :)\n\n\n"},{"id":"372969","messageId":"CAPig+cSF8A0-rSM3q1AvpE7xjkFcrMEBdsGTe+qkd2rU_uxcEg@mail.gmail.com","threadId":"50817","inReplyTo":"20190401115217.3423-2-szeder.dev@gmail.com","subject":"Re: [PATCH v2 1/4] progress: make display_progress() return void","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-04-02T05:42:12Z","receivedAt":"2019-04-02T05:42:28Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Apr 1, 2019 at 7:52 AM SZEDER Gábor wrote:\n> [...]\n> Let's make display_progress() return void, too.\n>\n> Signed-off-by: SZEDER Gábor \n> ---\n> diff --git a/progress.c b/progress.c\n> @@ -78,12 +78,12 @@ static int is_foreground_fd(int fd)\n> -static int display(struct progress *progress, uint64_t n, const char *done)\n> +static void display(struct progress *progress, uint64_t n, const char *done)\n> {\n> if (progress->delay && (!progress_update || --progress->delay))\n> - return 0;\n> + return;\n\nThis 'return' needs to stay, but...\n\n> @@ -100,7 +100,7 @@ static int display(struct progress *progress, uint64_t n, const char *done)\n> progress_update = 0;\n> - return 1;\n> + return;\n> }\n> } else if (progress_update) {\n> @@ -109,10 +109,10 @@ static int display(struct progress *progress, uint64_t n, const char *done)\n> progress_update = 0;\n> - return 1;\n> + return;\n> }\n>\n> - return 0;\n> + return;\n> }\n\n... these three 'returns' can all go away. (In fact, the first two\nmysteriously disappear in patch 2/4.)\n"},{"id":"372970","messageId":"CAPig+cRMBmfG1YDeP9t9eLZdqMqthfxp6nviQYGr7qNEA2U54Q@mail.gmail.com","threadId":"50817","inReplyTo":"20190401115217.3423-3-szeder.dev@gmail.com","subject":"Re: [PATCH v2 2/4] progress: assemble percentage and counters in a strbuf before printing","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-04-02T05:45:15Z","receivedAt":"2019-04-02T05:45:30Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Apr 1, 2019 at 7:52 AM SZEDER Gábor wrote:\n> [...]\n> To prepare for those changes assemble the changing parts in a separate\n> strbuf kept in 'struct progress' before printing.\n>\n> Signed-off-by: SZEDER Gábor \n> ---\n> diff --git a/progress.c b/progress.c\n> @@ -80,36 +81,42 @@ static int is_foreground_fd(int fd)\n> static void display(struct progress *progress, uint64_t n, const char *done)\n> {\n> if (progress->total) {\n> if (percent != progress->last_percent || progress_update) {\n> [...]\n> - progress_update = 0;\n> - return;\n> }\n> + if (show_update) {\n> [...]\n> progress_update = 0;\n> - return;\n> }\n\nRemoval of these two 'returns' is unrelated to the change made by this\npatch and should have been done by 1/4.\n\n> return;\n\nLikewise, this final 'return' doesn't need to be here and should go away in 1/4.\n"},{"id":"372971","messageId":"CAPig+cTOOthnjweyRjJuLy6o2TffPwTPJmQ_5tJoP1jhdKyD6g@mail.gmail.com","threadId":"50817","inReplyTo":"CAPig+cRMBmfG1YDeP9t9eLZdqMqthfxp6nviQYGr7qNEA2U54Q@mail.gmail.com","subject":"Re: [PATCH v2 2/4] progress: assemble percentage and counters in a strbuf before printing","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-04-02T05:50:43Z","receivedAt":"2019-04-02T05:50:58Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Apr 2, 2019 at 1:45 AM Eric Sunshine wrote:\n> On Mon, Apr 1, 2019 at 7:52 AM SZEDER Gábor wrote:\n> > + if (show_update) {\n> > [...]\n> > progress_update = 0;\n> > - return;\n> > }\n>\n> Removal of these two 'returns' is unrelated to the change made by this\n> patch and should have been done by 1/4.\n>\n> > return;\n>\n> Likewise, this final 'return' doesn't need to be here and should go away in 1/4.\n\nI forgot to mention that these are micro-nits, not necessarily worth a re-roll.\n"},{"id":"372980","messageId":"20190402142700.GA10564@sigill.intra.peff.net","threadId":"50817","inReplyTo":"20190401141542.GJ32732@szeder.dev","subject":"Re: [PATCH v2 3/4] progress: clear previous progress update dynamically","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-02T14:27:00Z","receivedAt":"2019-04-02T14:27:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 01, 2019 at 04:15:42PM +0200, SZEDER Gábor wrote:\n\n> > I don't think it could matter here, as these are meant to be smallish\n> > strings, but I think we should get into the habit of using size_t\n> > consistently to hold string lengths.\n> > \n> > It makes auditing for integer overflow problems much simpler (this is on\n> > my mind as I happen to be tracing some bugs around this the past few\n> > days).\n> > \n> > (There are a few instances in the next patch, too. Other than this nit,\n> > though, your series looks good to me).\n> \n> I started with using size_t, but then switched to int, because:\n> \n> - After a bit of arithmetic I had to compare to term_columns()'s int\n> return value anyway (in the next patch).\n> \n> - The second hunk in this patch adds these lines:\n> \n> int clear_len = counters_sb->len < last_count_len ?\n> last_count_len - counters_sb->len : 0;\n> fprintf(stderr, \"%s: %s%-*s\", progress->title,\n> counters_sb->buf, clear_len, eol);\n> \n> Here 'clear_len' has to be int, because the printf() format \"%-*s\"\n> expects an int, and otherwise -Werror=format= errors ensue. OK,\n> it could be size_t, but then it must be casted to an int upon\n> passing it to fprintf(), and after the next patch there will be\n> three such calls.\n> \n> I could resend using size_t. Should I resend using size_t? :)\n\nIMHO it's better to keep it as a size_t for as long as possible, and\nthen cast when we pass to printf, for a few reasons:\n\n 1. The cast is made explicitly, so it calls attention to it.\n\n 2. We know that a cast to int there can at worst produce truncated\n output, and not lead to any kind of memory error. (And if we really\n care about that, it's easy to convert it to an fwrite() at that\n point, though I would not bother in this case).\n\nI think the comparison to \"cols\" is OK, because we are just checking\nwhether cols is smaller than us. If we assume that size_t is at least as\nbig as an int (which I think is a reasonable assumption to make, and\ncertainly holds true for all platforms I know) then there's no\npossibility of logic errors.\n\nI wouldn't even bother with a cast there. Probably -Wsign-compare would\ncomplain, but we are pretty far from enabling that. And I think the\nright solution is for term_columns() to return an unsigned, anyway. ;)\n\nI admit all of this is academic enough that I can live with it either\nway (there are definitely places where it is _not_ academic, so I am\nmostly just trying to encourage a general style).\n\n-Peff\n"},{"id":"373157","messageId":"20190405004539.31467-1-szeder.dev@gmail.com","threadId":"50817","inReplyTo":"20190401115217.3423-1-szeder.dev@gmail.com","subject":"[PATCH v3 0/4] Progress display fixes","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-04-05T00:45:35Z","receivedAt":"2019-04-05T00:46:34Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"This patch series fixes two progress display issues by breaking\nprogress bars longer than the width of the terminal and by properly\ncleaning up the previously shown progress bar.\n\nChanges since v2, following Eric's and Peff's suggestions:\n\n - Remove return statements that just became unnecessary in patch\n 1/4.\n\n - Use size_t helper variables to store intermediate results of\n calculations based on length of strings.\n\nSZEDER Gábor (4):\n progress: make display_progress() return void\n progress: assemble percentage and counters in a strbuf before printing\n progress: clear previous progress update dynamically\n progress: break too long progress bar lines\n\n progress.c | 73 ++++++++++++++++++++++++++++++++++++++----------------\n progress.h | 2 +-\n 2 files changed, 53 insertions(+), 22 deletions(-)\n\nInterdiff:\ndiff --git a/progress.c b/progress.c\nindex e28ccdafd2..97e18671e5 100644\n--- a/progress.c\n+++ b/progress.c\n@@ -115,32 +115,30 @@ static void display(struct progress *progress, uint64_t n, const char *done)\n \tif (show_update) {\n \t\tif (is_foreground_fd(fileno(stderr)) || done) {\n \t\t\tconst char *eol = done ? done : \"\\r\";\n-\t\t\tint clear_len = counters_sb->len < last_count_len ?\n+\t\t\tsize_t clear_len = counters_sb->len < last_count_len ?\n \t\t\t\t\tlast_count_len - counters_sb->len : 0;\n-\t\t\tint progress_line_len = progress->title_len +\n+\t\t\tsize_t progress_line_len = progress->title_len +\n \t\t\t\t\t\tcounters_sb->len + 2;\n \t\t\tint cols = term_columns();\n \n \t\t\tif (progress->split) {\n \t\t\t\tfprintf(stderr, \" %s%-*s\", counters_sb->buf,\n-\t\t\t\t\tclear_len, eol);\n+\t\t\t\t\t(int) clear_len, eol);\n \t\t\t} else if (!done && cols < progress_line_len) {\n \t\t\t\tclear_len = progress->title_len + 1 < cols ?\n \t\t\t\t\t cols - progress->title_len - 1 : 0;\n \t\t\t\tfprintf(stderr, \"%s:%*s\\n %s%s\",\n-\t\t\t\t\tprogress->title, clear_len, \"\",\n+\t\t\t\t\tprogress->title, (int) clear_len, \"\",\n \t\t\t\t\tcounters_sb->buf, eol);\n \t\t\t\tprogress->split = 1;\n \t\t\t} else {\n \t\t\t\tfprintf(stderr, \"%s: %s%-*s\", progress->title,\n-\t\t\t\t\tcounters_sb->buf, clear_len, eol);\n+\t\t\t\t\tcounters_sb->buf, (int) clear_len, eol);\n \t\t\t}\n \t\t\tfflush(stderr);\n \t\t}\n \t\tprogress_update = 0;\n \t}\n-\n-\treturn;\n }\n \n static void throughput_string(struct strbuf *buf, uint64_t total,\nRange-diff:\n1: dea36bd2a7 ! 1: cb68e5b0ec progress: make display_progress() return void\n @@ -18,7 +18,9 @@\n its introduction in cf84d51c43 (add throughput to progress display,\n 2007-10-30).\n \n - Let's make display_progress() return void, too.\n + Let's make display_progress() return void, too. While doing so\n + several return statements in display() become unnecessary, remove\n + them.\n \n Signed-off-by: SZEDER Gábor \n \n @@ -45,7 +47,6 @@\n \t\t\t}\n \t\t\tprogress_update = 0;\n -\t\t\treturn 1;\n -+\t\t\treturn;\n \t\t}\n \t} else if (progress_update) {\n \t\tif (is_foreground_fd(fileno(stderr)) || done) {\n @@ -54,11 +55,9 @@\n \t\t}\n \t\tprogress_update = 0;\n -\t\treturn 1;\n -+\t\treturn;\n \t}\n - \n +-\n -\treturn 0;\n -+\treturn;\n }\n \n static void throughput_string(struct strbuf *buf, uint64_t total,\n2: 97de2a98a0 ! 2: 017d095142 progress: assemble percentage and counters in a strbuf before printing\n @@ -50,7 +50,6 @@\n -\t\t\t\tfflush(stderr);\n -\t\t\t}\n -\t\t\tprogress_update = 0;\n --\t\t\treturn;\n +\n +\t\t\tstrbuf_reset(counters_sb);\n +\t\t\tstrbuf_addf(counters_sb,\n @@ -76,10 +75,6 @@\n \t\t\tfflush(stderr);\n \t\t}\n \t\tprogress_update = 0;\n --\t\treturn;\n - \t}\n - \n - \treturn;\n @@\n \tprogress->delay = delay;\n \tprogress->throughput = NULL;\n3: edfe0157a7 ! 3: c5a4def5ac progress: clear previous progress update dynamically\n @@ -48,10 +48,10 @@\n -\t\t\tfprintf(stderr, \"%s: %s%s\", progress->title,\n -\t\t\t\tcounters_sb->buf, eol);\n +\t\t\tconst char *eol = done ? done : \"\\r\";\n -+\t\t\tint clear_len = counters_sb->len < last_count_len ?\n ++\t\t\tsize_t clear_len = counters_sb->len < last_count_len ?\n +\t\t\t\t\tlast_count_len - counters_sb->len : 0;\n +\t\t\tfprintf(stderr, \"%s: %s%-*s\", progress->title,\n -+\t\t\t\tcounters_sb->buf, clear_len, eol);\n ++\t\t\t\tcounters_sb->buf, (int) clear_len, eol);\n \t\t\tfflush(stderr);\n \t\t}\n \t\tprogress_update = 0;\n4: d53de231ee ! 4: 2f44dff84e progress: break too long progress bar lines\n @@ -70,27 +70,27 @@\n static volatile sig_atomic_t progress_update;\n @@\n \t\t\tconst char *eol = done ? done : \"\\r\";\n - \t\t\tint clear_len = counters_sb->len < last_count_len ?\n + \t\t\tsize_t clear_len = counters_sb->len < last_count_len ?\n \t\t\t\t\tlast_count_len - counters_sb->len : 0;\n -\t\t\tfprintf(stderr, \"%s: %s%-*s\", progress->title,\n --\t\t\t\tcounters_sb->buf, clear_len, eol);\n -+\t\t\tint progress_line_len = progress->title_len +\n +-\t\t\t\tcounters_sb->buf, (int) clear_len, eol);\n ++\t\t\tsize_t progress_line_len = progress->title_len +\n +\t\t\t\t\t\tcounters_sb->len + 2;\n +\t\t\tint cols = term_columns();\n +\n +\t\t\tif (progress->split) {\n +\t\t\t\tfprintf(stderr, \" %s%-*s\", counters_sb->buf,\n -+\t\t\t\t\tclear_len, eol);\n ++\t\t\t\t\t(int) clear_len, eol);\n +\t\t\t} else if (!done && cols < progress_line_len) {\n +\t\t\t\tclear_len = progress->title_len + 1 < cols ?\n +\t\t\t\t\t cols - progress->title_len - 1 : 0;\n +\t\t\t\tfprintf(stderr, \"%s:%*s\\n %s%s\",\n -+\t\t\t\t\tprogress->title, clear_len, \"\",\n ++\t\t\t\t\tprogress->title, (int) clear_len, \"\",\n +\t\t\t\t\tcounters_sb->buf, eol);\n +\t\t\t\tprogress->split = 1;\n +\t\t\t} else {\n +\t\t\t\tfprintf(stderr, \"%s: %s%-*s\", progress->title,\n -+\t\t\t\t\tcounters_sb->buf, clear_len, eol);\n ++\t\t\t\t\tcounters_sb->buf, (int) clear_len, eol);\n +\t\t\t}\n \t\t\tfflush(stderr);\n \t\t}\n-- \n2.21.0.539.g07239c3a71.dirty\n\n"},{"id":"373158","messageId":"20190405004539.31467-2-szeder.dev@gmail.com","threadId":"50817","inReplyTo":"20190405004539.31467-1-szeder.dev@gmail.com","subject":"[PATCH v3 1/4] progress: make display_progress() return void","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-04-05T00:45:36Z","receivedAt":"2019-04-05T00:46:35Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Ever since the progress infrastructure was introduced in 96a02f8f6d\n(common progress display support, 2007-04-18), display_progress() has\nreturned an int, telling callers whether it updated the progress bar\nor not. However, this is:\n\n - useless, because over the last dozen years there has never been a\n single caller that cared about that return value.\n\n - not quite true, because it doesn't print a progress bar when\n running in the background, yet it returns 1; see 85cb8906f0\n (progress: no progress in background, 2015-04-13).\n\nThe related display_throughput() function returned void already upon\nits introduction in cf84d51c43 (add throughput to progress display,\n2007-10-30).\n\nLet's make display_progress() return void, too. While doing so\nseveral return statements in display() become unnecessary, remove\nthem.\n\nSigned-off-by: SZEDER Gábor \n---\n progress.c | 13 +++++--------\n progress.h | 2 +-\n 2 files changed, 6 insertions(+), 9 deletions(-)\n\ndiff --git a/progress.c b/progress.c\nindex 5a99c9fbf0..9010032446 100644\n--- a/progress.c\n+++ b/progress.c\n@@ -78,12 +78,12 @@ static int is_foreground_fd(int fd)\n \treturn tpgrp < 0 || tpgrp == getpgid(0);\n }\n \n-static int display(struct progress *progress, uint64_t n, const char *done)\n+static void display(struct progress *progress, uint64_t n, const char *done)\n {\n \tconst char *eol, *tp;\n \n \tif (progress->delay && (!progress_update || --progress->delay))\n-\t\treturn 0;\n+\t\treturn;\n \n \tprogress->last_value = n;\n \ttp = (progress->throughput) ? progress->throughput->display.buf : \"\";\n@@ -100,7 +100,6 @@ static int display(struct progress *progress, uint64_t n, const char *done)\n \t\t\t\tfflush(stderr);\n \t\t\t}\n \t\t\tprogress_update = 0;\n-\t\t\treturn 1;\n \t\t}\n \t} else if (progress_update) {\n \t\tif (is_foreground_fd(fileno(stderr)) || done) {\n@@ -109,10 +108,7 @@ static int display(struct progress *progress, uint64_t n, const char *done)\n \t\t\tfflush(stderr);\n \t\t}\n \t\tprogress_update = 0;\n-\t\treturn 1;\n \t}\n-\n-\treturn 0;\n }\n \n static void throughput_string(struct strbuf *buf, uint64_t total,\n@@ -188,9 +184,10 @@ void display_throughput(struct progress *progress, uint64_t total)\n \t\tdisplay(progress, progress->last_value, NULL);\n }\n \n-int display_progress(struct progress *progress, uint64_t n)\n+void display_progress(struct progress *progress, uint64_t n)\n {\n-\treturn progress ? display(progress, n, NULL) : 0;\n+\tif (progress)\n+\t\tdisplay(progress, n, NULL);\n }\n \n static struct progress *start_progress_delay(const char *title, uint64_t total,\ndiff --git a/progress.h b/progress.h\nindex 70a4d4a0d6..59e40cc4fd 100644\n--- a/progress.h\n+++ b/progress.h\n@@ -4,7 +4,7 @@\n struct progress;\n \n void display_throughput(struct progress *progress, uint64_t total);\n-int display_progress(struct progress *progress, uint64_t n);\n+void display_progress(struct progress *progress, uint64_t n);\n struct progress *start_progress(const char *title, uint64_t total);\n struct progress *start_delayed_progress(const char *title, uint64_t total);\n void stop_progress(struct progress **progress);\n-- \n2.21.0.539.g07239c3a71.dirty\n\n"},{"id":"373159","messageId":"20190405004539.31467-3-szeder.dev@gmail.com","threadId":"50817","inReplyTo":"20190405004539.31467-1-szeder.dev@gmail.com","subject":"[PATCH v3 2/4] progress: assemble percentage and counters in a strbuf before printing","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-04-05T00:45:37Z","receivedAt":"2019-04-05T00:46:37Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The following patches in this series want to handle the progress bar's\ntitle and changing parts (i.e. the counter and the optional percentage\nand throughput combined) differently, and need to know the length\nof the changing parts of the previously displayed progress bar.\n\nTo prepare for those changes assemble the changing parts in a separate\nstrbuf kept in 'struct progress' before printing.\n\nSigned-off-by: SZEDER Gábor \n---\n progress.c | 35 +++++++++++++++++++++++------------\n 1 file changed, 23 insertions(+), 12 deletions(-)\n\ndiff --git a/progress.c b/progress.c\nindex 9010032446..564845a36b 100644\n--- a/progress.c\n+++ b/progress.c\n@@ -36,6 +36,7 @@ struct progress {\n \tunsigned delay;\n \tstruct throughput *throughput;\n \tuint64_t start_ns;\n+\tstruct strbuf counters_sb;\n };\n \n static volatile sig_atomic_t progress_update;\n@@ -80,31 +81,39 @@ static int is_foreground_fd(int fd)\n \n static void display(struct progress *progress, uint64_t n, const char *done)\n {\n-\tconst char *eol, *tp;\n+\tconst char *tp;\n+\tstruct strbuf *counters_sb = &progress->counters_sb;\n+\tint show_update = 0;\n \n \tif (progress->delay && (!progress_update || --progress->delay))\n \t\treturn;\n \n \tprogress->last_value = n;\n \ttp = (progress->throughput) ? progress->throughput->display.buf : \"\";\n-\teol = done ? done : \" \\r\";\n \tif (progress->total) {\n \t\tunsigned percent = n * 100 / progress->total;\n \t\tif (percent != progress->last_percent || progress_update) {\n \t\t\tprogress->last_percent = percent;\n-\t\t\tif (is_foreground_fd(fileno(stderr)) || done) {\n-\t\t\t\tfprintf(stderr, \"%s: %3u%% (%\"PRIuMAX\"/%\"PRIuMAX\")%s%s\",\n-\t\t\t\t\tprogress->title, percent,\n-\t\t\t\t\t(uintmax_t)n, (uintmax_t)progress->total,\n-\t\t\t\t\ttp, eol);\n-\t\t\t\tfflush(stderr);\n-\t\t\t}\n-\t\t\tprogress_update = 0;\n+\n+\t\t\tstrbuf_reset(counters_sb);\n+\t\t\tstrbuf_addf(counters_sb,\n+\t\t\t\t \"%3u%% (%\"PRIuMAX\"/%\"PRIuMAX\")%s\", percent,\n+\t\t\t\t (uintmax_t)n, (uintmax_t)progress->total,\n+\t\t\t\t tp);\n+\t\t\tshow_update = 1;\n \t\t}\n \t} else if (progress_update) {\n+\t\tstrbuf_reset(counters_sb);\n+\t\tstrbuf_addf(counters_sb, \"%\"PRIuMAX\"%s\", (uintmax_t)n, tp);\n+\t\tshow_update = 1;\n+\t}\n+\n+\tif (show_update) {\n \t\tif (is_foreground_fd(fileno(stderr)) || done) {\n-\t\t\tfprintf(stderr, \"%s: %\"PRIuMAX\"%s%s\",\n-\t\t\t\tprogress->title, (uintmax_t)n, tp, eol);\n+\t\t\tconst char *eol = done ? done : \" \\r\";\n+\n+\t\t\tfprintf(stderr, \"%s: %s%s\", progress->title,\n+\t\t\t\tcounters_sb->buf, eol);\n \t\t\tfflush(stderr);\n \t\t}\n \t\tprogress_update = 0;\n@@ -207,6 +216,7 @@ static struct progress *start_progress_delay(const char *title, uint64_t total,\n \tprogress->delay = delay;\n \tprogress->throughput = NULL;\n \tprogress->start_ns = getnanotime();\n+\tstrbuf_init(&progress->counters_sb, 0);\n \tset_progress_signal();\n \treturn progress;\n }\n@@ -250,6 +260,7 @@ void stop_progress_msg(struct progress **p_progress, const char *msg)\n \t\tfree(buf);\n \t}\n \tclear_progress_signal();\n+\tstrbuf_release(&progress->counters_sb);\n \tif (progress->throughput)\n \t\tstrbuf_release(&progress->throughput->display);\n \tfree(progress->throughput);\n-- \n2.21.0.539.g07239c3a71.dirty\n\n"},{"id":"373160","messageId":"20190405004539.31467-4-szeder.dev@gmail.com","threadId":"50817","inReplyTo":"20190405004539.31467-1-szeder.dev@gmail.com","subject":"[PATCH v3 3/4] progress: clear previous progress update dynamically","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-04-05T00:45:38Z","receivedAt":"2019-04-05T00:46:38Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"When the progress bar includes throughput, its length can shorten as\nthe unit of display changes from KiB to MiB. To cover up remnants of\nthe previous progress bar when such a change of units happens we\nalways print three spaces at the end of the progress bar.\n\nAlas, covering only three characters is not quite enough: when both\nthe total and the throughput happen to change units from KiB to MiB in\nthe same update, then the progress bar's length is shortened by four\ncharacters (or maybe even more!):\n\n Receiving objects: 25% (2901/11603), 772.01 KiB | 733.00 KiB/s\n Receiving objects: 27% (3133/11603), 1.43 MiB | 1.16 MiB/s s\n\nand a stray 's' is left behind.\n\nSo instead of hard-coding the three characters to cover, let's compare\nthe length of the current progress bar with the previous one, and\ncover up as many characters as needed.\n\nSure, it would be much simpler to just print more spaces at the end of\nthe progress bar, but this approach is more future-proof, and it won't\nprint extra spaces when none are needed, notably when the progress bar\ndoesn't show throughput and thus never shrinks.\n\nSigned-off-by: SZEDER Gábor \n---\n progress.c | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/progress.c b/progress.c\nindex 564845a36b..ca0945d2be 100644\n--- a/progress.c\n+++ b/progress.c\n@@ -84,6 +84,7 @@ static void display(struct progress *progress, uint64_t n, const char *done)\n \tconst char *tp;\n \tstruct strbuf *counters_sb = &progress->counters_sb;\n \tint show_update = 0;\n+\tint last_count_len = counters_sb->len;\n \n \tif (progress->delay && (!progress_update || --progress->delay))\n \t\treturn;\n@@ -110,10 +111,11 @@ static void display(struct progress *progress, uint64_t n, const char *done)\n \n \tif (show_update) {\n \t\tif (is_foreground_fd(fileno(stderr)) || done) {\n-\t\t\tconst char *eol = done ? done : \" \\r\";\n-\n-\t\t\tfprintf(stderr, \"%s: %s%s\", progress->title,\n-\t\t\t\tcounters_sb->buf, eol);\n+\t\t\tconst char *eol = done ? done : \"\\r\";\n+\t\t\tsize_t clear_len = counters_sb->len < last_count_len ?\n+\t\t\t\t\tlast_count_len - counters_sb->len : 0;\n+\t\t\tfprintf(stderr, \"%s: %s%-*s\", progress->title,\n+\t\t\t\tcounters_sb->buf, (int) clear_len, eol);\n \t\t\tfflush(stderr);\n \t\t}\n \t\tprogress_update = 0;\n-- \n2.21.0.539.g07239c3a71.dirty\n\n"},{"id":"373161","messageId":"20190405004539.31467-5-szeder.dev@gmail.com","threadId":"50817","inReplyTo":"20190405004539.31467-1-szeder.dev@gmail.com","subject":"[PATCH v3 4/4] progress: break too long progress bar lines","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-04-05T00:45:39Z","receivedAt":"2019-04-05T00:46:39Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Some of the recently added progress indicators have quite long titles,\nwhich might be even longer when translated to some languages, and when\nthey are shown while operating on bigger repositories, then the\nprogress bar grows longer than the default 80 column terminal width.\n\nWhen the progress bar exceeds the width of the terminal it gets\nline-wrapped, and after that the CR at the end doesn't return to the\nbeginning of the progress bar, but to the first column of its last\nline. Consequently, the first line of the previously shown progress\nbar is not overwritten by the next, and we end up with a bunch of\ntruncated progress bar lines scrolling past:\n\n $ LANG=es_ES.UTF-8 git commit-graph write\n Encontrando commits para commit graph entre los objetos empaquetados: 2% (1599\n Encontrando commits para commit graph entre los objetos empaquetados: 3% (1975\n Encontrando commits para commit graph entre los objetos empaquetados: 4% (2633\n Encontrando commits para commit graph entre los objetos empaquetados: 5% (3292\n [...]\n\nPrevent this by breaking progress bars after the title once they\nexceed the width of the terminal, so the counter and optional\npercentage and throughput, i.e. all changing parts, are on the last\nline. Subsequent updates will from then on only refresh the changing\nparts, but not the title, and it will look like this:\n\n $ LANG=es_ES.UTF-8 ~/src/git/git commit-graph write\n Encontrando commits para commit graph entre los objetos empaquetados:\n 100% (6584502/6584502), listo.\n Calculando números de generación de commit graph: 100% (824705/824705), listo.\n Escribiendo commit graph en 4 pasos: 100% (3298820/3298820), listo.\n\nNote that the number of columns in the terminal is cached by\nterm_columns(), so this might not kick in when it should when a\nterminal window is resized while the operation is running.\nFurthermore, this change won't help if the terminal is so narrow that\nthe counters don't fit on one line, but I would put this in the \"If it\nhurts, don't do it\" box.\n\nSigned-off-by: SZEDER Gábor \n---\n progress.c | 27 ++++++++++++++++++++++++---\n 1 file changed, 24 insertions(+), 3 deletions(-)\n\ndiff --git a/progress.c b/progress.c\nindex ca0945d2be..97e18671e5 100644\n--- a/progress.c\n+++ b/progress.c\n@@ -8,11 +8,12 @@\n * published by the Free Software Foundation.\n */\n \n-#include \"git-compat-util.h\"\n+#include \"cache.h\"\n #include \"gettext.h\"\n #include \"progress.h\"\n #include \"strbuf.h\"\n #include \"trace.h\"\n+#include \"utf8.h\"\n \n #define TP_IDX_MAX 8\n \n@@ -37,6 +38,8 @@ struct progress {\n \tstruct throughput *throughput;\n \tuint64_t start_ns;\n \tstruct strbuf counters_sb;\n+\tint title_len;\n+\tint split;\n };\n \n static volatile sig_atomic_t progress_update;\n@@ -114,8 +117,24 @@ static void display(struct progress *progress, uint64_t n, const char *done)\n \t\t\tconst char *eol = done ? done : \"\\r\";\n \t\t\tsize_t clear_len = counters_sb->len < last_count_len ?\n \t\t\t\t\tlast_count_len - counters_sb->len : 0;\n-\t\t\tfprintf(stderr, \"%s: %s%-*s\", progress->title,\n-\t\t\t\tcounters_sb->buf, (int) clear_len, eol);\n+\t\t\tsize_t progress_line_len = progress->title_len +\n+\t\t\t\t\t\tcounters_sb->len + 2;\n+\t\t\tint cols = term_columns();\n+\n+\t\t\tif (progress->split) {\n+\t\t\t\tfprintf(stderr, \" %s%-*s\", counters_sb->buf,\n+\t\t\t\t\t(int) clear_len, eol);\n+\t\t\t} else if (!done && cols < progress_line_len) {\n+\t\t\t\tclear_len = progress->title_len + 1 < cols ?\n+\t\t\t\t\t cols - progress->title_len - 1 : 0;\n+\t\t\t\tfprintf(stderr, \"%s:%*s\\n %s%s\",\n+\t\t\t\t\tprogress->title, (int) clear_len, \"\",\n+\t\t\t\t\tcounters_sb->buf, eol);\n+\t\t\t\tprogress->split = 1;\n+\t\t\t} else {\n+\t\t\t\tfprintf(stderr, \"%s: %s%-*s\", progress->title,\n+\t\t\t\t\tcounters_sb->buf, (int) clear_len, eol);\n+\t\t\t}\n \t\t\tfflush(stderr);\n \t\t}\n \t\tprogress_update = 0;\n@@ -219,6 +238,8 @@ static struct progress *start_progress_delay(const char *title, uint64_t total,\n \tprogress->throughput = NULL;\n \tprogress->start_ns = getnanotime();\n \tstrbuf_init(&progress->counters_sb, 0);\n+\tprogress->title_len = utf8_strwidth(title);\n+\tprogress->split = 0;\n \tset_progress_signal();\n \treturn progress;\n }\n-- \n2.21.0.539.g07239c3a71.dirty\n\n"},{"id":"373271","messageId":"20190405222154.GB10787@sigill.intra.peff.net","threadId":"50817","inReplyTo":"20190405004539.31467-1-szeder.dev@gmail.com","subject":"Re: [PATCH v3 0/4] Progress display fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-05T22:21:55Z","receivedAt":"2019-04-05T22:21:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 05, 2019 at 02:45:35AM +0200, SZEDER Gábor wrote:\n\n> This patch series fixes two progress display issues by breaking\n> progress bars longer than the width of the terminal and by properly\n> cleaning up the previously shown progress bar.\n> \n> Changes since v2, following Eric's and Peff's suggestions:\n> \n> - Remove return statements that just became unnecessary in patch\n> 1/4.\n> \n> - Use size_t helper variables to store intermediate results of\n> calculations based on length of strings.\n\nThanks, I have no more nits to pick.\n\n-Peff\n"},{"id":"373754","messageId":"20190412194515.6244-1-szeder.dev@gmail.com","threadId":"50817","inReplyTo":"20190405004539.31467-1-szeder.dev@gmail.com","subject":"[PATCH v4 0/4] Progress display fixes","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-04-12T19:45:11Z","receivedAt":"2019-04-12T19:45:26Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"This patch series fixes two progress display issues by breaking\nprogress bars longer than the width of the terminal and by properly\ncleaning up the previously shown progress bar. Hopefully for good\nthis time...\n\nThis \"properly cleaning up\" part, i.e. mainly patch 3, was buggy in\nprevious versions, because:\n\n - It used the wrong format flag and the '\\r' was padded on the\n right, but should have been padded to the left.\n\n - The padding was one space shorter than necessary, because I didn't\n account for the '\\r' included in the field width as well.\n\n\nSZEDER Gábor (4):\n progress: make display_progress() return void\n progress: assemble percentage and counters in a strbuf before printing\n progress: clear previous progress update dynamically\n progress: break too long progress bar lines\n\n progress.c | 74 ++++++++++++++++++++++++++++++++++++++----------------\n progress.h | 2 +-\n 2 files changed, 54 insertions(+), 22 deletions(-)\n\nInterdiff:\ndiff --git a/progress.c b/progress.c\nindex 97e18671e5..2d8022a622 100644\n--- a/progress.c\n+++ b/progress.c\n@@ -116,23 +116,24 @@ static void display(struct progress *progress, uint64_t n, const char *done)\n \t\tif (is_foreground_fd(fileno(stderr)) || done) {\n \t\t\tconst char *eol = done ? done : \"\\r\";\n \t\t\tsize_t clear_len = counters_sb->len < last_count_len ?\n-\t\t\t\t\tlast_count_len - counters_sb->len : 0;\n+\t\t\t\t\tlast_count_len - counters_sb->len + 1 :\n+\t\t\t\t\t0;\n \t\t\tsize_t progress_line_len = progress->title_len +\n \t\t\t\t\t\tcounters_sb->len + 2;\n \t\t\tint cols = term_columns();\n \n \t\t\tif (progress->split) {\n-\t\t\t\tfprintf(stderr, \" %s%-*s\", counters_sb->buf,\n+\t\t\t\tfprintf(stderr, \" %s%*s\", counters_sb->buf,\n \t\t\t\t\t(int) clear_len, eol);\n \t\t\t} else if (!done && cols < progress_line_len) {\n \t\t\t\tclear_len = progress->title_len + 1 < cols ?\n-\t\t\t\t\t cols - progress->title_len - 1 : 0;\n+\t\t\t\t\t cols - progress->title_len : 0;\n \t\t\t\tfprintf(stderr, \"%s:%*s\\n %s%s\",\n \t\t\t\t\tprogress->title, (int) clear_len, \"\",\n \t\t\t\t\tcounters_sb->buf, eol);\n \t\t\t\tprogress->split = 1;\n \t\t\t} else {\n-\t\t\t\tfprintf(stderr, \"%s: %s%-*s\", progress->title,\n+\t\t\t\tfprintf(stderr, \"%s: %s%*s\", progress->title,\n \t\t\t\t\tcounters_sb->buf, (int) clear_len, eol);\n \t\t\t}\n \t\t\tfflush(stderr);\nRange-diff:\n1: cb68e5b0ec = 1: cb68e5b0ec progress: make display_progress() return void\n2: 017d095142 = 2: 017d095142 progress: assemble percentage and counters in a strbuf before printing\n3: c5a4def5ac ! 3: ec9c96d102 progress: clear previous progress update dynamically\n @@ -49,8 +49,9 @@\n -\t\t\t\tcounters_sb->buf, eol);\n +\t\t\tconst char *eol = done ? done : \"\\r\";\n +\t\t\tsize_t clear_len = counters_sb->len < last_count_len ?\n -+\t\t\t\t\tlast_count_len - counters_sb->len : 0;\n -+\t\t\tfprintf(stderr, \"%s: %s%-*s\", progress->title,\n ++\t\t\t\t\tlast_count_len - counters_sb->len + 1 :\n ++\t\t\t\t\t0;\n ++\t\t\tfprintf(stderr, \"%s: %s%*s\", progress->title,\n +\t\t\t\tcounters_sb->buf, (int) clear_len, eol);\n \t\t\tfflush(stderr);\n \t\t}\n4: 2f44dff84e ! 4: 8fc8e3cf94 progress: break too long progress bar lines\n @@ -69,27 +69,27 @@\n \n static volatile sig_atomic_t progress_update;\n @@\n - \t\t\tconst char *eol = done ? done : \"\\r\";\n \t\t\tsize_t clear_len = counters_sb->len < last_count_len ?\n - \t\t\t\t\tlast_count_len - counters_sb->len : 0;\n --\t\t\tfprintf(stderr, \"%s: %s%-*s\", progress->title,\n + \t\t\t\t\tlast_count_len - counters_sb->len + 1 :\n + \t\t\t\t\t0;\n +-\t\t\tfprintf(stderr, \"%s: %s%*s\", progress->title,\n -\t\t\t\tcounters_sb->buf, (int) clear_len, eol);\n +\t\t\tsize_t progress_line_len = progress->title_len +\n +\t\t\t\t\t\tcounters_sb->len + 2;\n +\t\t\tint cols = term_columns();\n +\n +\t\t\tif (progress->split) {\n -+\t\t\t\tfprintf(stderr, \" %s%-*s\", counters_sb->buf,\n ++\t\t\t\tfprintf(stderr, \" %s%*s\", counters_sb->buf,\n +\t\t\t\t\t(int) clear_len, eol);\n +\t\t\t} else if (!done && cols < progress_line_len) {\n +\t\t\t\tclear_len = progress->title_len + 1 < cols ?\n -+\t\t\t\t\t cols - progress->title_len - 1 : 0;\n ++\t\t\t\t\t cols - progress->title_len : 0;\n +\t\t\t\tfprintf(stderr, \"%s:%*s\\n %s%s\",\n +\t\t\t\t\tprogress->title, (int) clear_len, \"\",\n +\t\t\t\t\tcounters_sb->buf, eol);\n +\t\t\t\tprogress->split = 1;\n +\t\t\t} else {\n -+\t\t\t\tfprintf(stderr, \"%s: %s%-*s\", progress->title,\n ++\t\t\t\tfprintf(stderr, \"%s: %s%*s\", progress->title,\n +\t\t\t\t\tcounters_sb->buf, (int) clear_len, eol);\n +\t\t\t}\n \t\t\tfflush(stderr);\n-- \n2.21.0.746.gd74f1657d3\n\n"},{"id":"373755","messageId":"20190412194515.6244-2-szeder.dev@gmail.com","threadId":"50817","inReplyTo":"20190412194515.6244-1-szeder.dev@gmail.com","subject":"[PATCH v4 1/4] progress: make display_progress() return void","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-04-12T19:45:12Z","receivedAt":"2019-04-12T19:45:26Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Ever since the progress infrastructure was introduced in 96a02f8f6d\n(common progress display support, 2007-04-18), display_progress() has\nreturned an int, telling callers whether it updated the progress bar\nor not. However, this is:\n\n - useless, because over the last dozen years there has never been a\n single caller that cared about that return value.\n\n - not quite true, because it doesn't print a progress bar when\n running in the background, yet it returns 1; see 85cb8906f0\n (progress: no progress in background, 2015-04-13).\n\nThe related display_throughput() function returned void already upon\nits introduction in cf84d51c43 (add throughput to progress display,\n2007-10-30).\n\nLet's make display_progress() return void, too. While doing so\nseveral return statements in display() become unnecessary, remove\nthem.\n\nSigned-off-by: SZEDER Gábor \n---\n progress.c | 13 +++++--------\n progress.h | 2 +-\n 2 files changed, 6 insertions(+), 9 deletions(-)\n\ndiff --git a/progress.c b/progress.c\nindex 5a99c9fbf0..9010032446 100644\n--- a/progress.c\n+++ b/progress.c\n@@ -78,12 +78,12 @@ static int is_foreground_fd(int fd)\n \treturn tpgrp < 0 || tpgrp == getpgid(0);\n }\n \n-static int display(struct progress *progress, uint64_t n, const char *done)\n+static void display(struct progress *progress, uint64_t n, const char *done)\n {\n \tconst char *eol, *tp;\n \n \tif (progress->delay && (!progress_update || --progress->delay))\n-\t\treturn 0;\n+\t\treturn;\n \n \tprogress->last_value = n;\n \ttp = (progress->throughput) ? progress->throughput->display.buf : \"\";\n@@ -100,7 +100,6 @@ static int display(struct progress *progress, uint64_t n, const char *done)\n \t\t\t\tfflush(stderr);\n \t\t\t}\n \t\t\tprogress_update = 0;\n-\t\t\treturn 1;\n \t\t}\n \t} else if (progress_update) {\n \t\tif (is_foreground_fd(fileno(stderr)) || done) {\n@@ -109,10 +108,7 @@ static int display(struct progress *progress, uint64_t n, const char *done)\n \t\t\tfflush(stderr);\n \t\t}\n \t\tprogress_update = 0;\n-\t\treturn 1;\n \t}\n-\n-\treturn 0;\n }\n \n static void throughput_string(struct strbuf *buf, uint64_t total,\n@@ -188,9 +184,10 @@ void display_throughput(struct progress *progress, uint64_t total)\n \t\tdisplay(progress, progress->last_value, NULL);\n }\n \n-int display_progress(struct progress *progress, uint64_t n)\n+void display_progress(struct progress *progress, uint64_t n)\n {\n-\treturn progress ? display(progress, n, NULL) : 0;\n+\tif (progress)\n+\t\tdisplay(progress, n, NULL);\n }\n \n static struct progress *start_progress_delay(const char *title, uint64_t total,\ndiff --git a/progress.h b/progress.h\nindex 70a4d4a0d6..59e40cc4fd 100644\n--- a/progress.h\n+++ b/progress.h\n@@ -4,7 +4,7 @@\n struct progress;\n \n void display_throughput(struct progress *progress, uint64_t total);\n-int display_progress(struct progress *progress, uint64_t n);\n+void display_progress(struct progress *progress, uint64_t n);\n struct progress *start_progress(const char *title, uint64_t total);\n struct progress *start_delayed_progress(const char *title, uint64_t total);\n void stop_progress(struct progress **progress);\n-- \n2.21.0.746.gd74f1657d3\n\n"},{"id":"373756","messageId":"20190412194515.6244-3-szeder.dev@gmail.com","threadId":"50817","inReplyTo":"20190412194515.6244-1-szeder.dev@gmail.com","subject":"[PATCH v4 2/4] progress: assemble percentage and counters in a strbuf before printing","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-04-12T19:45:13Z","receivedAt":"2019-04-12T19:45:28Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The following patches in this series want to handle the progress bar's\ntitle and changing parts (i.e. the counter and the optional percentage\nand throughput combined) differently, and need to know the length\nof the changing parts of the previously displayed progress bar.\n\nTo prepare for those changes assemble the changing parts in a separate\nstrbuf kept in 'struct progress' before printing.\n\nSigned-off-by: SZEDER Gábor \n---\n progress.c | 35 +++++++++++++++++++++++------------\n 1 file changed, 23 insertions(+), 12 deletions(-)\n\ndiff --git a/progress.c b/progress.c\nindex 9010032446..564845a36b 100644\n--- a/progress.c\n+++ b/progress.c\n@@ -36,6 +36,7 @@ struct progress {\n \tunsigned delay;\n \tstruct throughput *throughput;\n \tuint64_t start_ns;\n+\tstruct strbuf counters_sb;\n };\n \n static volatile sig_atomic_t progress_update;\n@@ -80,31 +81,39 @@ static int is_foreground_fd(int fd)\n \n static void display(struct progress *progress, uint64_t n, const char *done)\n {\n-\tconst char *eol, *tp;\n+\tconst char *tp;\n+\tstruct strbuf *counters_sb = &progress->counters_sb;\n+\tint show_update = 0;\n \n \tif (progress->delay && (!progress_update || --progress->delay))\n \t\treturn;\n \n \tprogress->last_value = n;\n \ttp = (progress->throughput) ? progress->throughput->display.buf : \"\";\n-\teol = done ? done : \" \\r\";\n \tif (progress->total) {\n \t\tunsigned percent = n * 100 / progress->total;\n \t\tif (percent != progress->last_percent || progress_update) {\n \t\t\tprogress->last_percent = percent;\n-\t\t\tif (is_foreground_fd(fileno(stderr)) || done) {\n-\t\t\t\tfprintf(stderr, \"%s: %3u%% (%\"PRIuMAX\"/%\"PRIuMAX\")%s%s\",\n-\t\t\t\t\tprogress->title, percent,\n-\t\t\t\t\t(uintmax_t)n, (uintmax_t)progress->total,\n-\t\t\t\t\ttp, eol);\n-\t\t\t\tfflush(stderr);\n-\t\t\t}\n-\t\t\tprogress_update = 0;\n+\n+\t\t\tstrbuf_reset(counters_sb);\n+\t\t\tstrbuf_addf(counters_sb,\n+\t\t\t\t \"%3u%% (%\"PRIuMAX\"/%\"PRIuMAX\")%s\", percent,\n+\t\t\t\t (uintmax_t)n, (uintmax_t)progress->total,\n+\t\t\t\t tp);\n+\t\t\tshow_update = 1;\n \t\t}\n \t} else if (progress_update) {\n+\t\tstrbuf_reset(counters_sb);\n+\t\tstrbuf_addf(counters_sb, \"%\"PRIuMAX\"%s\", (uintmax_t)n, tp);\n+\t\tshow_update = 1;\n+\t}\n+\n+\tif (show_update) {\n \t\tif (is_foreground_fd(fileno(stderr)) || done) {\n-\t\t\tfprintf(stderr, \"%s: %\"PRIuMAX\"%s%s\",\n-\t\t\t\tprogress->title, (uintmax_t)n, tp, eol);\n+\t\t\tconst char *eol = done ? done : \" \\r\";\n+\n+\t\t\tfprintf(stderr, \"%s: %s%s\", progress->title,\n+\t\t\t\tcounters_sb->buf, eol);\n \t\t\tfflush(stderr);\n \t\t}\n \t\tprogress_update = 0;\n@@ -207,6 +216,7 @@ static struct progress *start_progress_delay(const char *title, uint64_t total,\n \tprogress->delay = delay;\n \tprogress->throughput = NULL;\n \tprogress->start_ns = getnanotime();\n+\tstrbuf_init(&progress->counters_sb, 0);\n \tset_progress_signal();\n \treturn progress;\n }\n@@ -250,6 +260,7 @@ void stop_progress_msg(struct progress **p_progress, const char *msg)\n \t\tfree(buf);\n \t}\n \tclear_progress_signal();\n+\tstrbuf_release(&progress->counters_sb);\n \tif (progress->throughput)\n \t\tstrbuf_release(&progress->throughput->display);\n \tfree(progress->throughput);\n-- \n2.21.0.746.gd74f1657d3\n\n"},{"id":"373757","messageId":"20190412194515.6244-4-szeder.dev@gmail.com","threadId":"50817","inReplyTo":"20190412194515.6244-1-szeder.dev@gmail.com","subject":"[PATCH v4 3/4] progress: clear previous progress update dynamically","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-04-12T19:45:14Z","receivedAt":"2019-04-12T19:45:30Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"When the progress bar includes throughput, its length can shorten as\nthe unit of display changes from KiB to MiB. To cover up remnants of\nthe previous progress bar when such a change of units happens we\nalways print three spaces at the end of the progress bar.\n\nAlas, covering only three characters is not quite enough: when both\nthe total and the throughput happen to change units from KiB to MiB in\nthe same update, then the progress bar's length is shortened by four\ncharacters (or maybe even more!):\n\n Receiving objects: 25% (2901/11603), 772.01 KiB | 733.00 KiB/s\n Receiving objects: 27% (3133/11603), 1.43 MiB | 1.16 MiB/s s\n\nand a stray 's' is left behind.\n\nSo instead of hard-coding the three characters to cover, let's compare\nthe length of the current progress bar with the previous one, and\ncover up as many characters as needed.\n\nSure, it would be much simpler to just print more spaces at the end of\nthe progress bar, but this approach is more future-proof, and it won't\nprint extra spaces when none are needed, notably when the progress bar\ndoesn't show throughput and thus never shrinks.\n\nSigned-off-by: SZEDER Gábor \n---\n progress.c | 11 +++++++----\n 1 file changed, 7 insertions(+), 4 deletions(-)\n\ndiff --git a/progress.c b/progress.c\nindex 564845a36b..949a2a576d 100644\n--- a/progress.c\n+++ b/progress.c\n@@ -84,6 +84,7 @@ static void display(struct progress *progress, uint64_t n, const char *done)\n \tconst char *tp;\n \tstruct strbuf *counters_sb = &progress->counters_sb;\n \tint show_update = 0;\n+\tint last_count_len = counters_sb->len;\n \n \tif (progress->delay && (!progress_update || --progress->delay))\n \t\treturn;\n@@ -110,10 +111,12 @@ static void display(struct progress *progress, uint64_t n, const char *done)\n \n \tif (show_update) {\n \t\tif (is_foreground_fd(fileno(stderr)) || done) {\n-\t\t\tconst char *eol = done ? done : \" \\r\";\n-\n-\t\t\tfprintf(stderr, \"%s: %s%s\", progress->title,\n-\t\t\t\tcounters_sb->buf, eol);\n+\t\t\tconst char *eol = done ? done : \"\\r\";\n+\t\t\tsize_t clear_len = counters_sb->len < last_count_len ?\n+\t\t\t\t\tlast_count_len - counters_sb->len + 1 :\n+\t\t\t\t\t0;\n+\t\t\tfprintf(stderr, \"%s: %s%*s\", progress->title,\n+\t\t\t\tcounters_sb->buf, (int) clear_len, eol);\n \t\t\tfflush(stderr);\n \t\t}\n \t\tprogress_update = 0;\n-- \n2.21.0.746.gd74f1657d3\n\n"},{"id":"373758","messageId":"20190412194515.6244-5-szeder.dev@gmail.com","threadId":"50817","inReplyTo":"20190412194515.6244-1-szeder.dev@gmail.com","subject":"[PATCH v4 4/4] progress: break too long progress bar lines","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-04-12T19:45:15Z","receivedAt":"2019-04-12T19:45:32Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Some of the recently added progress indicators have quite long titles,\nwhich might be even longer when translated to some languages, and when\nthey are shown while operating on bigger repositories, then the\nprogress bar grows longer than the default 80 column terminal width.\n\nWhen the progress bar exceeds the width of the terminal it gets\nline-wrapped, and after that the CR at the end doesn't return to the\nbeginning of the progress bar, but to the first column of its last\nline. Consequently, the first line of the previously shown progress\nbar is not overwritten by the next, and we end up with a bunch of\ntruncated progress bar lines scrolling past:\n\n $ LANG=es_ES.UTF-8 git commit-graph write\n Encontrando commits para commit graph entre los objetos empaquetados: 2% (1599\n Encontrando commits para commit graph entre los objetos empaquetados: 3% (1975\n Encontrando commits para commit graph entre los objetos empaquetados: 4% (2633\n Encontrando commits para commit graph entre los objetos empaquetados: 5% (3292\n [...]\n\nPrevent this by breaking progress bars after the title once they\nexceed the width of the terminal, so the counter and optional\npercentage and throughput, i.e. all changing parts, are on the last\nline. Subsequent updates will from then on only refresh the changing\nparts, but not the title, and it will look like this:\n\n $ LANG=es_ES.UTF-8 ~/src/git/git commit-graph write\n Encontrando commits para commit graph entre los objetos empaquetados:\n 100% (6584502/6584502), listo.\n Calculando números de generación de commit graph: 100% (824705/824705), listo.\n Escribiendo commit graph en 4 pasos: 100% (3298820/3298820), listo.\n\nNote that the number of columns in the terminal is cached by\nterm_columns(), so this might not kick in when it should when a\nterminal window is resized while the operation is running.\nFurthermore, this change won't help if the terminal is so narrow that\nthe counters don't fit on one line, but I would put this in the \"If it\nhurts, don't do it\" box.\n\nSigned-off-by: SZEDER Gábor \n---\n progress.c | 27 ++++++++++++++++++++++++---\n 1 file changed, 24 insertions(+), 3 deletions(-)\n\ndiff --git a/progress.c b/progress.c\nindex 949a2a576d..2d8022a622 100644\n--- a/progress.c\n+++ b/progress.c\n@@ -8,11 +8,12 @@\n * published by the Free Software Foundation.\n */\n \n-#include \"git-compat-util.h\"\n+#include \"cache.h\"\n #include \"gettext.h\"\n #include \"progress.h\"\n #include \"strbuf.h\"\n #include \"trace.h\"\n+#include \"utf8.h\"\n \n #define TP_IDX_MAX 8\n \n@@ -37,6 +38,8 @@ struct progress {\n \tstruct throughput *throughput;\n \tuint64_t start_ns;\n \tstruct strbuf counters_sb;\n+\tint title_len;\n+\tint split;\n };\n \n static volatile sig_atomic_t progress_update;\n@@ -115,8 +118,24 @@ static void display(struct progress *progress, uint64_t n, const char *done)\n \t\t\tsize_t clear_len = counters_sb->len < last_count_len ?\n \t\t\t\t\tlast_count_len - counters_sb->len + 1 :\n \t\t\t\t\t0;\n-\t\t\tfprintf(stderr, \"%s: %s%*s\", progress->title,\n-\t\t\t\tcounters_sb->buf, (int) clear_len, eol);\n+\t\t\tsize_t progress_line_len = progress->title_len +\n+\t\t\t\t\t\tcounters_sb->len + 2;\n+\t\t\tint cols = term_columns();\n+\n+\t\t\tif (progress->split) {\n+\t\t\t\tfprintf(stderr, \" %s%*s\", counters_sb->buf,\n+\t\t\t\t\t(int) clear_len, eol);\n+\t\t\t} else if (!done && cols < progress_line_len) {\n+\t\t\t\tclear_len = progress->title_len + 1 < cols ?\n+\t\t\t\t\t cols - progress->title_len : 0;\n+\t\t\t\tfprintf(stderr, \"%s:%*s\\n %s%s\",\n+\t\t\t\t\tprogress->title, (int) clear_len, \"\",\n+\t\t\t\t\tcounters_sb->buf, eol);\n+\t\t\t\tprogress->split = 1;\n+\t\t\t} else {\n+\t\t\t\tfprintf(stderr, \"%s: %s%*s\", progress->title,\n+\t\t\t\t\tcounters_sb->buf, (int) clear_len, eol);\n+\t\t\t}\n \t\t\tfflush(stderr);\n \t\t}\n \t\tprogress_update = 0;\n@@ -220,6 +239,8 @@ static struct progress *start_progress_delay(const char *title, uint64_t total,\n \tprogress->throughput = NULL;\n \tprogress->start_ns = getnanotime();\n \tstrbuf_init(&progress->counters_sb, 0);\n+\tprogress->title_len = utf8_strwidth(title);\n+\tprogress->split = 0;\n \tset_progress_signal();\n \treturn progress;\n }\n-- \n2.21.0.746.gd74f1657d3\n\n"}]}