[RFC/PATCH 25/25] progress: assert counting upwards in display()

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: git@vger.kernel.org
Cc: "Junio C Hamano" <gitster@pobox.com>,
	"SZEDER Gábor" <szeder.dev@gmail.com>,
	"René Scharfe" <l.s.r@web.de>, "Taylor Blau" <me@ttaylorr.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: [RFC/PATCH 25/25] progress: assert counting upwards in display()
Date: Wed, 23 Jun 2021 19:48:25 +0200	[thread overview]
Message-ID: <patch-25.25-099e9c019a9-20210623T155626Z-avarab@gmail.com> (raw)
In-Reply-To: <cover-00.25-00000000000-20210623T155626Z-avarab@gmail.com>

From: SZEDER Gábor <szeder.dev@gmail.com>

We had to fix a buggy progress line recently, where the progress
counter counted backwards, see 8e118e8490 (pack-objects: update
"nr_seen" progress based on pack-reused count, 2021-04-11).

Let's add a BUG(...) assertion that makes use of the "last_update"
value to make sure this doesn't happen again, i.e.  trigger a BUG()
when the counter passed to display_progress() is smaller than the
previous value.

Note that we allow subsequent display_progress() calls with the same
counter value, because:

  - Strictly speaking, it's not wrong to do so.

  - Forbidding it might make the code calling display_progress() more
    complex; I suspect that would be the case with e.g. the "Updating
    index flags" progress line in 'unpack-trees.c', where the counter
    is increased in recursive function calls.

  - We would need to special case the internal display() call in
    stop_progress_msg(), because it uses the same counter value as the
    last display_progress() call, which would trigger this BUG().

't0500-progress-display.sh' countains a few tests that check how
shortened progress lines are covered up, and one of them ('progress
shortens - crazy caller') shortens the progress line by counting
backwards.  From now on that test would trigger this BUG(), so remove
it; the other test cases cover shortening progress lines sufficiently.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

WARNING: I belive this is subtly buggy, see the discussion in the
cover letter. It needs more fixes of the progress.c API usage in
various places before being ready.

 progress.c                  |  2 ++
 t/t0500-progress-display.sh | 36 ++++++++++++------------------------
 2 files changed, 14 insertions(+), 24 deletions(-)

diff --git a/progress.c b/progress.c
index 40043bf6601..7b59006c7c4 100644
--- a/progress.c
+++ b/progress.c
@@ -40,6 +40,8 @@ static void display(struct progress *progress, uint64_t n,
 	const char *tp;
 	int show_update = 0;
 
+	if (progress->last_update != -1 && n < progress->last_update)
+		BUG("counting backwards with display_progress()");
 	progress->last_update = n;
 
 	if (progress->delay && (!progress_update || --progress->delay))
diff --git a/t/t0500-progress-display.sh b/t/t0500-progress-display.sh
index 3f00e52ce46..de59a757f86 100755
--- a/t/t0500-progress-display.sh
+++ b/t/t0500-progress-display.sh
@@ -211,30 +211,6 @@ EOF
 	test_cmp expect out
 '
 
-# Progress counter goes backwards, this should not happen in practice.
-test_expect_success 'progress shortens - crazy caller' '
-	cat >expect <<-\EOF &&
-	Working hard:  10% (100/1000)<CR>
-	Working hard:  20% (200/1000)<CR>
-	Working hard:   0% (1/1000)  <CR>
-	Working hard: 100% (1000/1000)<CR>
-	Working hard: 100% (1000/1000), done.
-	EOF
-
-	cat >in <<-\EOF &&
-	start 1000
-	progress 100
-	progress 200
-	progress 1
-	progress 1000
-	stop
-	EOF
-	test-tool progress <in 2>stderr &&
-
-	show_cr <stderr >out &&
-	test_cmp expect out
-'
-
 test_expect_success 'progress display with throughput' '
 	cat >expect <<-\EOF &&
 	Working hard: 0, stalled.<CR>
@@ -451,4 +427,16 @@ test_expect_success 'BUG: display_progress() does not reach declared "total"' '
 	grep "BUG:.*total progress does not match" stderr
 '
 
+test_expect_success 'BUG: display_progres() counting backwards' '
+	cat >in <<-\EOF &&
+	start 3
+	progress 1
+	progress 2
+	progress 1
+	EOF
+
+	test_must_fail test-tool progress <in 2>stderr &&
+	grep "BUG:.*counting backwards" stderr
+'
+
 test_done
-- 
2.32.0.599.g3967b4fa4ac

