[PATCH 3/7] progress: catch backwards counting with GIT_TEST_CHECK_PROGRESS
From: SZEDER Gábor <hidden>
Date: 2021-06-20 20:03:31
Subsystem:
the rest · Maintainer:
Linus Torvalds
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).
Extend GIT_TEST_CHECK_PROGRESS to catch these cases as well, 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 <redacted>
---
progress.c | 6 ++++++
t/t0500-progress-display.sh | 35 +++++++++++++----------------------
2 files changed, 19 insertions(+), 22 deletions(-)
diff --git a/progress.c b/progress.c
index 549e8d1fe7..034d50cd6b 100644
--- a/progress.c
+++ b/progress.c@@ -115,6 +115,12 @@ static void display(struct progress *progress, uint64_t n, const char *done) int show_update = 0; int last_count_len = counters_sb->len; + if (test_check_progress && progress->last_value != -1 && + n < progress->last_value) + BUG("progress \"%s\" counts backwards %"PRIuMAX" -> %"PRIuMAX, + progress->title, (uintmax_t)progress->last_value, + (uintmax_t)n); + progress->last_value = n; if (progress->delay && (!progress_update || --progress->delay))
diff --git a/t/t0500-progress-display.sh b/t/t0500-progress-display.sh
index 641fa0964e..a73dd45153 100755
--- a/t/t0500-progress-display.sh
+++ b/t/t0500-progress-display.sh@@ -153,28 +153,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 && - progress 100 - progress 200 - progress 1 - progress 1000 - EOF - test-tool progress --total=1000 "Working hard" <in 2>stderr && - - show_cr <stderr >out && - test_cmp expect out -' - test_expect_success 'progress display with throughput' ' cat >expect <<-\EOF && Working hard: 10<CR>
@@ -324,13 +302,26 @@ test_expect_success 'GIT_TEST_CHECK_PROGRESS catches non-matching total' ' grep "BUG:.*total progress does not match" stderr ' +test_expect_success 'GIT_TEST_CHECK_PROGRESS catches backwards counting' ' + cat >in <<-\EOF && + progress 2 + progress 1 + EOF + + test_must_fail env GIT_TEST_CHECK_PROGRESS=1 \ + test-tool progress --total=3 "Working hard" <in 2>stderr && + grep "BUG:.*counts backwards" stderr +' + test_expect_success 'tolerate bogus progress without GIT_TEST_CHECK_PROGRESS' ' cat >expect <<-\EOF && + Working hard: 66% (2/3)<CR> Working hard: 33% (1/3)<CR> Working hard: 33% (1/3), done. EOF cat >in <<-\EOF && + progress 2 progress 1 EOF (
--
2.32.0.289.g44fbea0957