Re: [PATCH 2/2] read-cache: fix incorrect count and progress bar stalling
- From
René Scharfe <l.s.r@web.de>
- Date
- Jun 10, 2021, 15:14 UTC
- Message-ID
- <f5e181fc-af94-bccf-051f-a58a04fe1ffc@web.de>
- In-Reply-To
- <87lf7k2bem.fsf@evledraar.gmail.com>
Am 09.06.21 um 00:12 schrieb Ævar Arnfjörð Bjarmason:
Show 33 quoted lines
>
> On Tue, Jun 08 2021, 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..
>
> That's intentional. We started eating 3 apples, got to one, but now our
> house is on fire and we're eating no more apples today, even if we
> planned to eat 3 when we sat down.
>
> The progress bar reflects this unexpected but recoverable state:
>
> $ perl -wE 'for (0..1) { say "update"; say "progress $_" }' |
> ./helper/test-tool progress --total=3 Apples 2>&1 |
> cat -v | perl -pe 's/\^M\K/\n/g'
> Apples: 0% (0/3)^M
> Apples: 33% (1/3)^M
> Apples: 33% (1/3), done.
>
> We're at 1/3, but we're done. No more apples.
>
> This isn't just some hypothetical, e.g. consider neeing to unlink() or
> remove files/directories one at a time in a directory and getting the
> estimated number from st_nlink (yeah yeah, unportable, but it was the
> first thing I thought of).
>
> We might think we're processing 10 entries, but another other processes
> might make our progress bar end at more or less than the 100% we
> expected. That's OK, not something we should invoke BUG() about.It doesn't have to be a BUG; a warning would suffice. And I hope not finishing the expected number of items due to a catastrophic event is rare enough that an additional warning wouldn't cause too much pain.
Loops that *regularly* end early are not a good fit for progress percentages, I think.
Show 7 quoted lines
> Similarly, the n=0 being distinguishable from the first > display_progress() is actually useful in practice. It's something I've > seen git.git emit (not recently, I patched the relevant code to emit > more granular progress). > > It's useful to know that we're stalling on the setup code before the > for-loop, not on the first item.
Hmm, preparations that take a noticeable time might deserve their own progress line.
Anyway, if no guard rails can be built then we have to rely on our math skills alone. Off-by-one errors may look silly, but are no joke -- they are surprisingly easy to make.
René