[RFC/PATCH 25/25] progress: assert counting upwards in display()
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Jun 23, 2021, 17:48 UTC
- Message-ID
- <patch-25.25-099e9c019a9-20210623T155626Z-avarab@gmail.com>
- 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