git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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é
Previous: René ScharfeNext: Ævar Arnfjörð Bjarmason
Message 13 of 24 in “trivial progress.c API usage fixes”
  1. 0/2 trivial progress.c API usage fixesÆvar Arnfjörð Bjarmason, Jun 7, 2021
  2. 2/2 read-cache: fix incorrect count and progress bar stallingÆvar Arnfjörð Bjarmason, Jun 7, 2021
  3. Derrick StoleeJun 7, 2021
  4. Ævar Arnfjörð BjarmasonJun 7, 2021
  5. René ScharfeJun 7, 2021
  6. Ævar Arnfjörð BjarmasonJun 7, 2021
  7. Junio C HamanoJun 7, 2021
  8. Ævar Arnfjörð BjarmasonJun 8, 2021
  9. René ScharfeJun 8, 2021
  10. Ævar Arnfjörð BjarmasonJun 8, 2021
  11. Junio C HamanoJun 10, 2021
  12. René ScharfeJun 10, 2021
  13. René ScharfeJun 10, 2021
  14. Ævar Arnfjörð BjarmasonJun 14, 2021
  15. René ScharfeJun 14, 2021
  16. Ævar Arnfjörð BjarmasonJun 14, 2021
  17. Junio C HamanoJun 15, 2021
  18. René ScharfeJun 15, 2021
  19. Ævar Arnfjörð BjarmasonJun 15, 2021
  20. René ScharfeJun 20, 2021
  21. 1/2 read-cache.c: don't guard calls to progress.c APIÆvar Arnfjörð Bjarmason, Jun 7, 2021
  22. Derrick StoleeJun 7, 2021
  23. Ævar Arnfjörð BjarmasonJun 7, 2021
  24. Derrick StoleeJun 7, 2021

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.