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

> 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.

> > 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.