Re: [PATCH 2/7] progress: catch nested/overlapping progresses with GIT_TEST_CHECK_PROGRESS

On Tue, Jun 22, 2021 at 12:00:07PM -0400, Taylor Blau wrote:

On Sun, Jun 20, 2021 at 10:02:58PM +0200, SZEDER Gábor wrote:

quoted

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.

Well, at least I was consistent :)

quoted

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.

Yeah.  It must have been a rebase mishap.  (I started working on this
after I reported yet another commit-graph related progress bug around
v2.31.0-rc0, and I had the first two checks on the same evening.  But
then some time later Peff came along and found a backwards counting
progress line, so I decided to add a check for that as well, which
necessitated a bit of refactoring in the other two checks, and then a
hunk somehow ended up in the wrong patch.)