On Mon, Jun 21, 2021 at 02:59:53AM +0200, Ævar Arnfjörð Bjarmason wrote: > > On Sun, Jun 20 2021, SZEDER Gábor wrote: > > > Splitting off from: > > > > https://public-inbox.org/git/cover-0.2-0000000000-20210607T144206Z-avarab@xxxxxxxxx/T/#me5d3176914d4268fd9f2a96fc63f4e41beb26bd6 > > > > On Tue, Jun 08, 2021 at 06:14:42PM +0200, René Scharfe wrote: > >> I wonder (only in a semi-curious way, though) if we can detect > >> off-by-one errors by adding an assertion to display_progress() that > >> requires the first update to have the value 0, and in stop_progress() > >> one that requires the previous display_progress() call to have a value > >> equal to the total number of work items. Not sure it'd be worth the > >> hassle.. > > > > I fixed and reported a number of bogus progress lines in the past, the > > last one during v2.31.0-rc phase, so I've looked into whether progress > > counters could be automatically validated in our tests, and came up > > with these patches a few months ago. It turned out that progress > > counters can be checked easily and transparently in case of progress > > lines that are shown in the tests, i.e. that are shown even when > > stderr is not a terminal or are forced with '--progress'. (In other > > cases it's still fairly easy but not quite transparent, as I think we > > need changes to the progress API; more on that later in a separate > > series.) > > I've also been working on some progress.[ch] patches that are mostly > finished, and I'm some 20 patches in at the moment. I wasn't sure about > whether to send an alternate 20-patch "let's do this (mostly) instead?" > series, hence this message. > > Much of what you're doing here becomes easier after that series, > e.g. your global process struct in 2/7 is something I ended up > implementing as part of a general feature to allow progress to be driven > by either display_progress() *or* the signal handler itself. It's difficult to know who should rebase onto who without seeing one half of the patches. I couldn't find a link to them anywhere (even if they are only available in your fork in a pre-polished state) despite looking, but my apologies if they are available and I'm just missing them. In general, I think that these patches are clear and are helpful in pinning down issues with the progress API (which I have made a hadnful of times in the past), so I would be happy to see them picked up. > I could also rebase on yours, but much of it would be rewriting the > test-only code to be more generalized, perhaps it's easier if we start > going for the more generalized solution first. Again, without knowing the substance of your patches it's hard to comment for sure, but I don't have a problem with a simple and direct approach here. > Perhaps we can just have it BUG() for now as you're doing and cross that > bridge when we come to it. I just wonder if we can't catch potential > bugs in a more gentle way somehow. I think there are compelling reasons to feel that the new mode should only be enabled during tests, as well as compelling reasons to feel that it should be enabled all of the time. One way to think about it is that we do not want users to have a BUG() abort their program just because a progress meter went rogue. So in that sense, it makes sense that we would only see that happen during tests, so that those tests could tell us where the bug is, and we could fix it. On the other hand, since we make sure that our tests pass at each patch, there's no point in having a separate mode (and instead, remove the conditionals on GIT_TEST_PROGRESS_CHECK), since successfully running the tests tells us that there are no rogue progress meters that we exercise in our (hopefully) complete set of tests. I could go either way, I think both lines of reasoning are quite reasonable. But, I think we are generally more lax about having the whole ci/run-build-and-tests.sh script pass at every commit, and that it seems we care more about having the tip of each series pass CI when integrated into 'seen'. So I don't think that hiding this new mode behind an environment variable is giving us as much confidence as we'd like, because it doesn't add anything in "make test". To me, I think a reasonable direction to take would be to *always* export GIT_TEST_PROGRESS_CHECK when running tests, not just in ci/run-build-and-tests.sh. That means we'll catch incorrect uses of the progress API during tests, without worrying that incomplete coverage will cause user-visible breakage. Thanks, Taylor
