| Message ID | 20210620200303.2328957-3-szeder.dev@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | progress: verify progress counters in the test suite | expand |
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. Thanks, Taylor
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.)
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)); + current_progress = progress; progress->title = title; progress->total = total; progress->last_value = -1; @@ -383,6 +389,7 @@ void stop_progress_msg(struct progress **p_progress, const char *msg) strbuf_release(&progress->counters_sb); if (progress->throughput) strbuf_release(&progress->throughput->display); + current_progress = NULL; free(progress->throughput); free(progress); }
We had to fix two buggy progress lines in the past, where stop_progress calls were added at the wrong place [1], resulting in "done" progress lines appearing in the wrong order. Extend GIT_TEST_CHECK_PROGRESS to catch these cases as well, i.e. trigger a BUG() when a progress has already been running when start_progress() or one of its variants is called to start a new one. Running the test suite with GIT_TEST_CHECK_PROGRESS enabled doesn't reveal any new issues [2]. 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); This doesn't produce bogus output like what is shown in those two fixes [1], because 'progress2' is already "done" before the first display_progress(progress1, ...) call. Btw, this is not just a pathological example, we do have two progress lines arranged like this, but they are only shown when standard error is a terminal, and thus aren't caught by GIT_TEST_CHECK_PROGRESS in its current form. [1] 6f9d5f2fda (commit-graph: fix progress of reachable commits, 2020-07-09) 862aead24e (commit-graph: fix "Collecting commits from input" progress line, 2020-07-10) [2] This patch series applies with a minor conflict on top of 6f9d5f2fda^, and makes 37 tests fail because of that bug. Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> --- progress.c | 7 +++++++ 1 file changed, 7 insertions(+)