mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Ævar Arnfjörð Bjarmason" <>
To: "René Scharfe" <>
Cc: "Junio C Hamano" <>,
	"Derrick Stolee" <>,, "Nguyễn Thái Ngọc Duy" <>
Subject: Re: [PATCH 2/2] read-cache: fix incorrect count and progress bar stalling
Date: Mon, 14 Jun 2021 13:07:50 +0200	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

On Thu, Jun 10 2021, René Scharfe wrote:

> Am 09.06.21 um 00:12 schrieb Ævar Arnfjörð Bjarmason:
>> 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.

It's not a catastrophic event, just a run of the mill race condition
we'll expect if we're dealing with the real world.

E.g. you asked to unlink 1000 files, we do so, we find 10 are unlinked
already, or the command is asked to recursively unlink all files in a
directory tree, and new ones have showed up.

In those cases we should just just shrug and move on, no need for a
warning. We just don't always have perfect information about future
state at the start of the loop.

> Loops that *regularly* end early are not a good fit for progress
> percentages, I think.

Arguably yes, but in these fuzzy cases not providing a "total" means
showing no progress at all, just a counter. Perhaps we should have some
other "provide total, and it may be fuzzy" flag. Not providing it might
run into your proposed BUG(), my point was that the current API
providing this flexibility is intentional.

>> 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.

Sure, and I've split some of those up in the past, but this seems like
ducking/not addressing the point that the API use we disagree on has
your preferred use conflating these conditions, but mine does not...

> 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.

...which, regardless of whether one views a progress of "1/5 items" has
"finished 1/5" or "working on 1/5", which I think *in general* is an
arbitrary choice, I think the progress.c API we have in git.git clearly
fits the usage I'm describing better.

  reply	other threads:[~2021-06-14 11:30 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-07 14:43 [PATCH 0/2] trivial progress.c API usage fixes Ævar Arnfjörð Bjarmason
2021-06-07 14:43 ` [PATCH 1/2] read-cache.c: don't guard calls to progress.c API Ævar Arnfjörð Bjarmason
2021-06-07 15:28   ` Derrick Stolee
2021-06-07 15:52     ` Ævar Arnfjörð Bjarmason
2021-06-07 16:11       ` Derrick Stolee
2021-06-07 14:43 ` [PATCH 2/2] read-cache: fix incorrect count and progress bar stalling Ævar Arnfjörð Bjarmason
2021-06-07 15:31   ` Derrick Stolee
2021-06-07 15:58     ` Ævar Arnfjörð Bjarmason
2021-06-07 19:20       ` René Scharfe
2021-06-07 19:49         ` Ævar Arnfjörð Bjarmason
2021-06-07 23:41           ` Junio C Hamano
2021-06-08 10:58             ` Ævar Arnfjörð Bjarmason
2021-06-08 16:14               ` René Scharfe
2021-06-08 22:12                 ` Ævar Arnfjörð Bjarmason
2021-06-10  5:30                   ` Junio C Hamano
2021-06-10 15:14                     ` René Scharfe
2021-06-10 15:14                   ` René Scharfe
2021-06-14 11:07                     ` Ævar Arnfjörð Bjarmason [this message]
2021-06-14 17:18                       ` René Scharfe
2021-06-14 19:08                         ` Ævar Arnfjörð Bjarmason
2021-06-15  2:32                           ` Junio C Hamano
2021-06-15 15:14                           ` René Scharfe
2021-06-15 16:46                             ` Ævar Arnfjörð Bjarmason
2021-06-20 12:53                               ` René Scharfe

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

  List information:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).