[PATCH 17/25] progress.c: refactor display() for less confusion, and fix bug

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

As tested for in 2bb74b53a49 (Test the progress display, 2019-09-16)
we would redundantly emit extra spaces to clear output we never
emitted under the split mode. Now we'll always clear precisely as many
columns as we need, and no more.

The root cause of that issue is that since the progress code was
originally written we've grown support for various new features, and
ended up with a function where we didn't build the output we were
about to emit once, and then emitted it.

We thus couldn't easily track the length of the output we really did
emit, with everything going downhill from there.

The alternative approach is longer (largely due to added comments),
but I think much clearer.

We no longer rely on magic constants like "2" for ": " or "
" (although we do still rely on the two separators being the same
length, but now have a related BUG(...) assertion).

We don't update "status_len_utf8" (or rather, the now-gone
"last_count_len") or "progress->last_value" until after we've emitted
all the output.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 progress.c                  | 137 +++++++++++++++++++++++++++---------
 t/t0500-progress-display.sh |   8 +--
 2 files changed, 104 insertions(+), 41 deletions(-)

diff --git a/progress.c b/progress.c
index e17490964c4..6c4038df791 100644
--- a/progress.c
+++ b/progress.c
@@ -25,17 +25,24 @@ static int is_foreground_fd(int fd)
 	return tpgrp < 0 || tpgrp == getpgid(0);
 }
 
+static const char *counter_prefix(int split)
+{
+	switch (split) {
+	case 1: return "  ";
+	case 0: return ": ";
+	default: BUG("unknown split value");
+	}
+}
+
 static void display(struct progress *progress, uint64_t n,
 		    const char *update_msg, int last_update)
 {
 	const char *tp;
 	int show_update = 0;
-	size_t last_count_len = progress->status_len_utf8;
 
 	if (progress->delay && (!progress_update || --progress->delay))
 		return;
 
-	progress->last_value = n;
 	tp = (progress->throughput) ? progress->throughput->display.buf : "";
 	if (progress->total) {
 		unsigned percent = n * 100 / progress->total;
@@ -44,61 +51,121 @@ static void display(struct progress *progress, uint64_t n,
 
 			strbuf_reset(&progress->status);
 			strbuf_addf(&progress->status,
-				    "%3u%% (%"PRIuMAX"/%"PRIuMAX")%s", percent,
+				    "%s%3u%% (%"PRIuMAX"/%"PRIuMAX")%s",
+				    counter_prefix(progress->split), percent,
 				    (uintmax_t)n, (uintmax_t)progress->total,
 				    tp);
 			show_update = 1;
 		}
 	} else if (progress_update) {
 		strbuf_reset(&progress->status);
-		strbuf_addf(&progress->status, "%"PRIuMAX"%s", (uintmax_t)n, tp);
+		strbuf_addf(&progress->status, "%s%"PRIuMAX"%s", counter_prefix(progress->split),
+			    (uintmax_t)n, tp);
 		show_update = 1;
 	}
 
 	if (show_update && update_msg)
-		strbuf_addf(&progress->status, ", %s.", update_msg);
+		strbuf_addstr(&progress->status, update_msg);
 
 	if (show_update) {
 		int stderr_is_foreground_fd = is_foreground_fd(fileno(stderr));
 		if (stderr_is_foreground_fd || update_msg) {
 			const char *eol = last_update ? "\n" : "\r";
-			size_t clear_len = progress->status.len < last_count_len ?
-					last_count_len - progress->status.len + 1 :
-					0;
-			/* The "+ 2" accounts for the ": ". */
-			size_t progress_line_len = progress->title_len_utf8 +
-						progress->status.len + 2;
-			int cols = term_columns();
-			progress->status_len_utf8 = utf8_strwidth(progress->status.buf);
-
-			if (progress->split) {
-				fprintf(stderr, "  %*s%*s",
-					(int)progress->status_len_utf8,
-					progress->status.buf,
-					(int)clear_len, eol);
-			} else if (!update_msg && cols < progress_line_len) {
-				clear_len = progress->title_len_utf8 + 1 < cols ?
-					    cols - progress->title_len_utf8 - 1 : 0;
-				fprintf(stderr, "%*s:%*s\n  %*s%s",
-					(int)progress->title_len_utf8,
-					progress->title.buf,
-					(int)clear_len, "",
-					(int)progress->status_len_utf8,
-					progress->status.buf, eol);
+			size_t status_len_utf8 = utf8_strwidth(progress->status.buf);
+			size_t progress_line_len = progress->title_len_utf8 + status_len_utf8;
+
+			/*
+			 * We're back at the beginning, so we'll
+			 * always print out the title, unless we're
+			 * already split, then the title is on an
+			 * earlier line.
+			 */
+			if (!progress->split)
+				fprintf(stderr, "%*s",
+					(int)(progress->title_len_utf8),
+					progress->title.buf);
+
+			/*
+			 * Did the user resize the terminal and we're
+			 * splitting this progress bar? Clear previous
+			 * ": (X/Y) [msg]"
+			 */
+			if (!progress->split &&
+			    term_columns() < progress_line_len) {
+				const char *split_prefix = counter_prefix(0);
+				const char *unsplit_prefix = counter_prefix(1);
+				const char *split_colon = ":";
 				progress->split = 1;
+
+				if (progress->last_value == -1) {
+					/*
+					 * We've got no previous
+					 * output whatsoever, so we
+					 * were "always split". No
+					 * previous status output to
+					 * erase.
+					 */
+					fprintf(stderr, "%s\n", split_colon);
+				} else {
+					const char *split_colon = ":";
+					const size_t split_colon_len = strlen(split_colon);
+
+					/*
+					 * Erase whatever we had, adding a
+					 * trailing ":" (not ": ") to indicate
+					 * the progress on the next line.
+					 */
+					fprintf(stderr, "%s%*s\n", split_colon,
+						(int)(progress->status_len_utf8 - split_colon_len),
+						"");
+				}
+
+				/*
+				 * For the one-off switching from
+				 * "!progress->split" to
+				 * "progress->split" fake up the
+				 * expected strbuf and replace the ":
+				 * " with a " ".
+				 *
+				 * The length of the two delimiters
+				 * must be the same for this trick to
+				 * work.
+				 */
+				if (!starts_with(progress->status.buf, split_prefix))
+					BUG("switching from already true split mode to split mode?");
+
+				strbuf_splice(&progress->status, 0,
+					      strlen(split_prefix),
+					      unsplit_prefix,
+					      strlen(unsplit_prefix));
+
+				fprintf(stderr, "%*s%s", (int)status_len_utf8,
+					progress->status.buf, eol);
 			} else {
-				fprintf(stderr, "%*s: %*s%*s",
-					(int)progress->title_len_utf8,
-					progress->title.buf,
-					(int)progress->status_len_utf8,
-					progress->status.buf,
-					(int)clear_len, eol);
+				/*
+				 * Our current
+				 * message may be larger or smaller than the
+				 * last one. Either the progress bar went
+				 * backards (smaller numbers), or we went back
+				 * and forth with a status message.
+				 */
+				size_t clear_len = progress->status_len_utf8 > status_len_utf8
+					? progress->status_len_utf8 - status_len_utf8
+					: 0;
+				fprintf(stderr, "%*s%*s%s",
+					(int) status_len_utf8, progress->status.buf,
+					(int) clear_len, "",
+					eol);
 			}
+			progress->status_len_utf8 = status_len_utf8;
+
 			if (stderr_is_foreground_fd)
 				fflush(stderr);
 		}
 		progress_update = 0;
 	}
+	progress->last_value = n;
+
 }
 
 static void throughput_string(struct strbuf *buf, uint64_t total,
@@ -303,7 +370,7 @@ void stop_progress(struct progress **p_progress)
 		trace2_region_leave("progress", progress->title.buf, the_repository);
 	}
 
-	stop_progress_msg(p_progress, _("done"));
+	stop_progress_msg(p_progress, _(", done."));
 }
 
 void stop_progress_msg(struct progress **p_progress, const char *msg)
diff --git a/t/t0500-progress-display.sh b/t/t0500-progress-display.sh
index 476a31222a3..883e044fe64 100755
--- a/t/t0500-progress-display.sh
+++ b/t/t0500-progress-display.sh
@@ -85,12 +85,10 @@ EOF
 '
 
 test_expect_success 'progress display breaks long lines #2' '
-	# Note: we do not need that many spaces after the title to cover up
-	# the last line before breaking the progress line.
 	sed -e "s/Z$//" >expect <<\EOF &&
 Working hard.......2.........3.........4.........5.........6:   0% (1/100000)<CR>
 Working hard.......2.........3.........4.........5.........6:   0% (2/100000)<CR>
-Working hard.......2.........3.........4.........5.........6:                   Z
+Working hard.......2.........3.........4.........5.........6:                Z
    10% (10000/100000)<CR>
   100% (100000/100000)<CR>
   100% (100000/100000), done.
@@ -112,10 +110,8 @@ EOF
 '
 
 test_expect_success 'progress display breaks long lines #3 - even the first is too long' '
-	# Note: we do not actually need any spaces at the end of the title
-	# line, because there is no previous progress line to cover up.
 	sed -e "s/Z$//" >expect <<\EOF &&
-Working hard.......2.........3.........4.........5.........6:                   Z
+Working hard.......2.........3.........4.........5.........6:
    25% (25000/100000)<CR>
    50% (50000/100000)<CR>
    75% (75000/100000)<CR>
-- 
2.32.0.599.g3967b4fa4ac