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