Re: [PATCH 2/7] progress: catch nested/overlapping progresses with GIT_TEST_CHECK_PROGRESS
From: Taylor Blau <hidden>
Date: 2021-06-22 16:00:14
On Sun, Jun 20, 2021 at 10:02:58PM +0200, SZEDER Gábor wrote:
Note that this will trigger even in cases where the output is not
visibly wrong, e.g. consider this simplified sequence of calls:
progress1 = start_delayed_progress();
progress2 = start_delayed_progress();
for (i = 0; ...)
display_progress(progress2, i + 1);
stop_progres(&progress2);
for (j = 0; ...)
display_progress(progress1, j + 1);
stop_progres(&progress1);s/stop_progres/&s, but no big deal. Everything else here looks good.
quoted hunk ↗ jump to hunk
diff --git a/progress.c b/progress.c index 255995406f..549e8d1fe7 100644 --- a/progress.c +++ b/progress.c@@ -48,6 +48,8 @@ struct progress { static volatile sig_atomic_t progress_update; static int test_check_progress; +/* Used to catch nested/overlapping progresses with GIT_TEST_CHECK_PROGRESS. */ +static struct progress *current_progress = NULL; /* * These are only intended for testing the progress output, i.e. exclusively@@ -258,8 +260,12 @@ static struct progress *start_progress_delay(const char *title, uint64_t total, struct progress *progress; test_check_progress = git_env_bool("GIT_TEST_CHECK_PROGRESS", 0); + if (test_check_progress && current_progress) + BUG("progress \"%s\" is still active when starting new progress \"%s\"", + current_progress->title, title); progress = xmalloc(sizeof(*progress));
Ah. This is why you moved the allocation down further, since we don't have to free anything up when calling BUG() if it wasn't allocated in the first place (and we had no such conditional that would cause us to abort early before). For what it's worth, I probably would have preferred to see that change from the previous patch included in this one rather than in the first of the series, since it's much clearer here than it is in the first patch. Thanks, Taylor