Re: [PATCH 2/2] read-cache: fix incorrect count and progress bar stalling

On Tue, Jun 15 2021, René Scharfe wrote:

Am 14.06.21 um 21:08 schrieb Ævar Arnfjörð Bjarmason:

quoted

On Mon, Jun 14 2021, René Scharfe wrote:

quoted

Am 14.06.21 um 13:07 schrieb Ævar Arnfjörð Bjarmason:

quoted

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

quoted

Am 09.06.21 um 00:12 schrieb Ævar Arnfjörð Bjarmason:

quoted

On Tue, Jun 08 2021, René Scharfe wrote:

quoted

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.

If a planned work item is cancelled then it can still be counted as
done.  Or the total could be adjusted, but that might look awkward.

quoted

quoted

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.

Your patch turns a loop that doesn't immediately report skipped items
into one with contiguous progress updates.  That's a good way to deal
with the imagined restrictions for error detection.  Another would be
to make the warnings optional.

I don't see how there's anything wrong with the API use, how it needs a
warning etc.

You pointed out that many callsites do:

	for (i = 0; i < large_number; i++) {
		display_progress(p, i + 1);
		/* work work work */
	}

This is an off-by-one error because a finished item is reported before
work on it starts.  Adding a warning can help find these cases reliably.

I understand that we're respectfully disagreeing and that's what you
think, but I really don't think it helps anyone if we just repeat our
respective points.

I don't think it's off-by-one, but you do.

Yes, I understand that you think that progress bars should absolutely
never ever show 1/5 if the first item is not finished. I disagree and
think that's not intuitive, per my "eating an Apple" example
upthread.

We disagree, and I for one think I understand what you mean, perhaps you
don't understand what I mean, but let's try to move on.

quoted

quoted

quoted

quoted

quoted

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

Subtle.  If preparation takes a long time and each item less than that
then the progress display is likely to jump from "0/n" to "i/n", where
i > 1, and the meaning of "1/n" becomes moot.

In practice we're making humongous jumps all over the place, we don't
write to the terminal for every item processed, and if we did it would
be too fast to be perceptable to the user.

So I don't think this is an issue in the first place, as noted upthread
in [ref]. Regardless of what we think of
the supposed off-by-one issue you seemed to think that it was enough of
an issue to justify complexity at the API use level (needing to think
about "continue" statements in loops, etc.), but now you think it's
moot?

I don't understand your question.  Let me rephrase what I find moot:

You wrote that the first display_progress() call being made with n>0
would be useful to you to see long-running preparations.  If items are
processed quicker than one per second, then whatever number the first
display_progress() call writes to the screen will be overwritten within
a second.  So the value of the first update shouldn't actually matter
much for your use case -- unless items takes a long time to process.

I think it would be better if you replied specifically to the comments I
had later about throughput progress bars, i.e.:

    How does the idea that we show "has been done" make sense when you
    combine the progress.c API with the display_throughput(). I.e. output
    like[...]

Anyway, in this case I understood you to mean that you thought the
off-by-one wasn't a big deal in practice most of the time, I don't think
so either for e.g. counting objects in pack files.

I do think it's useful to be consistent though, and for e.g. cases of
downloading 5 files it makes sense to show 1/5 if we are currently in
the process of downloading files 1 out of 5, not 0/5 or whatever.