| Message ID | 20210620200303.2328957-7-szeder.dev@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | progress: verify progress counters in the test suite | expand |
Am 20.06.21 um 22:03 schrieb SZEDER Gábor: > RFC!! Alas, not calling stop_progress() on error has drawbacks: > > - All memory allocated for the progress bar is leaked. > - This progress line remains "active", in the sense that if we were > to start a new progress later in the same git process, then with > GIT_TEST_CHECK_PROGRESS it would trigger the other BUG() catching > nested/overlapping progresses. > > Do we care?! TBH I don't :) > Anyway, if we do, then we might need some sort of an abort_progress() > function... I think the abort_progress() idea makes sense; to clean up allocations, tell the user what happened and avoid the BUG(). Showing just "aborted" instead of "done" should suffice here -- the explanation is given a few lines later ("'foo' was not filtered properly"). It could be a cheesy stop_progress_msg() wrapper that temporarily sets test_check_progress to zero.. René
On Mon, Jun 21, 2021 at 08:32:56PM +0200, René Scharfe wrote: > Am 20.06.21 um 22:03 schrieb SZEDER Gábor: > > RFC!! Alas, not calling stop_progress() on error has drawbacks: > > > > - All memory allocated for the progress bar is leaked. > > - This progress line remains "active", in the sense that if we were > > to start a new progress later in the same git process, then with > > GIT_TEST_CHECK_PROGRESS it would trigger the other BUG() catching > > nested/overlapping progresses. > > > > Do we care?! TBH I don't :) > > Anyway, if we do, then we might need some sort of an abort_progress() > > function... > > I think the abort_progress() idea makes sense; to clean up allocations, > tell the user what happened and avoid the BUG(). Showing just > "aborted" instead of "done" should suffice here -- the explanation is > given a few lines later ("'foo' was not filtered properly"). Very well put. I concur that having an abort_progress() API makes sense for all of the reasons that you suggest, but also because we shouldn't encourage not using what seems like an appropriate API in order to not fail tests when GIT_TEST_CHECK_PROGRESS is set. Thanks, Taylor
On Tue, Jun 22, 2021 at 09:52:32PM -0400, Taylor Blau wrote: > On Mon, Jun 21, 2021 at 08:32:56PM +0200, René Scharfe wrote: > > Am 20.06.21 um 22:03 schrieb SZEDER Gábor: > > > RFC!! Alas, not calling stop_progress() on error has drawbacks: > > > > > > - All memory allocated for the progress bar is leaked. > > > - This progress line remains "active", in the sense that if we were > > > to start a new progress later in the same git process, then with > > > GIT_TEST_CHECK_PROGRESS it would trigger the other BUG() catching > > > nested/overlapping progresses. > > > > > > Do we care?! TBH I don't :) > > > Anyway, if we do, then we might need some sort of an abort_progress() > > > function... > > > > I think the abort_progress() idea makes sense; to clean up allocations, > > tell the user what happened and avoid the BUG(). Showing just > > "aborted" instead of "done" should suffice here -- the explanation is > > given a few lines later ("'foo' was not filtered properly"). > > Very well put. I concur that having an abort_progress() API makes sense > for all of the reasons that you suggest, but also because we shouldn't > encourage not using what seems like an appropriate API in order to not > fail tests when GIT_TEST_CHECK_PROGRESS is set. Ah, damn, I was hoping that I can get away with it :)
diff --git a/entry.c b/entry.c index bc4b8fcc98..38baefe22a 100644 --- a/entry.c +++ b/entry.c @@ -232,7 +232,8 @@ int finish_delayed_checkout(struct checkout *state, int *nr_checkouts) } string_list_remove_empty_items(&dco->filters, 0); } - stop_progress(&progress); + if (!errs && !dco->paths.nr) + stop_progress(&progress); string_list_clear(&dco->filters, 0); /* At this point we should not have any delayed paths anymore. */
The test 'missing file in delayed checkout' in 't0021-conversion.sh' fails when run with GIT_TEST_CHECK_PROGRESS=1, because the final value of the "Filtering content" progress counter doesn't match the expected total, triggering BUG(). This is not caused by a bug in how we count progress, but because the test involves a purposefully buggy filter process that doesn't process any paths, so the progress counter doesn't have a chance to reach the expected total. Arguably, it is wrong to show "done" at the end of the progress line when not all work was done. So let's check whether there were any errors while processing or that there are still unprocessed paths at the end (which a few lines later will in fact be considered as error) and don't show the final "done" line, i.e. don't call stop_progress(), if there were any. And if we don't call stop_progress(), then we won't verify that the progress counter matches the expected total, won't trigger BUG() on mismatch, and t0021 will succeed even with GIT_TEST_CHECK_PROGRESS=1. After this change the test suite passes with GIT_TEST_CHECK_PROGRESS=1. RFC!! Alas, not calling stop_progress() on error has drawbacks: - All memory allocated for the progress bar is leaked. - This progress line remains "active", in the sense that if we were to start a new progress later in the same git process, then with GIT_TEST_CHECK_PROGRESS it would trigger the other BUG() catching nested/overlapping progresses. Do we care?! TBH I don't :) Anyway, if we do, then we might need some sort of an abort_progress() function... Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> --- entry.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)