[PATCH 0/2] trivial progress.c API usage fixes

[PATCH 0/2] trivial progress.c API usage fixes

[Ævar Arnfjörð Bjarmason]

A couple of small fixes to the usage of the progress.c API. I have
some subsequent changes queued up locally, but thought it would be
better to submit this more incrementally, and to focus on smaller
cleanups in the immediate post-release period.

Ævar Arnfjörð Bjarmason (2):
  read-cache.c: don't guard calls to progress.c API
  read-cache: fix incorrect count and progress bar stalling

 read-cache.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

-- 
2.32.0.rc3.434.gd8aed1f08a7

[PATCH 1/2] read-cache.c: don't guard calls to progress.c API

[Ævar Arnfjörð Bjarmason]

Don't guard the calls to the progress.c API with "if (progress)". The
API itself will check this. This doesn't change any behavior, but
makes this code consistent with the rest of the codebase.

See ae9af12287b (status: show progress bar if refreshing the index
takes too long, 2018-09-15) for the commit that added the pattern
we're changing here.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 read-cache.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index 1b3c2eb408..470f800855 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1627,8 +1627,7 @@ int refresh_index(struct index_state *istate, unsigned int flags,
 		t2_sum_scan += t2_did_scan;
 		if (new_entry == ce)
 			continue;
-		if (progress)
-			display_progress(progress, i);
+		display_progress(progress, i);
 		if (!new_entry) {
 			const char *fmt;
 
@@ -1663,10 +1662,8 @@ int refresh_index(struct index_state *istate, unsigned int flags,
 	trace2_data_intmax("index", NULL, "refresh/sum_lstat", t2_sum_lstat);
 	trace2_data_intmax("index", NULL, "refresh/sum_scan", t2_sum_scan);
 	trace2_region_leave("index", "refresh", NULL);
-	if (progress) {
-		display_progress(progress, istate->cache_nr);
-		stop_progress(&progress);
-	}
+	display_progress(progress, istate->cache_nr);
+	stop_progress(&progress);
 	trace_performance_leave("refresh index");
 	return has_errors;
 }
-- 
2.32.0.rc3.434.gd8aed1f08a7

Re: [PATCH 1/2] read-cache.c: don't guard calls to progress.c API

[Derrick Stolee]

On 6/7/2021 10:43 AM, Ævar Arnfjörð Bjarmason wrote:
> Don't guard the calls to the progress.c API with "if (progress)". The
> API itself will check this. This doesn't change any behavior, but
> makes this code consistent with the rest of the codebase.

Since stop_progress() closes a trace2 region, this actually
does make a change in behavior, I think. In a good way.

Thanks,
-Stolee

Re: [PATCH 1/2] read-cache.c: don't guard calls to progress.c API

[Ævar Arnfjörð Bjarmason]

On Mon, Jun 07 2021, Derrick Stolee wrote:

> On 6/7/2021 10:43 AM, Ævar Arnfjörð Bjarmason wrote:
>> Don't guard the calls to the progress.c API with "if (progress)". The
>> API itself will check this. This doesn't change any behavior, but
>> makes this code consistent with the rest of the codebase.
>
> Since stop_progress() closes a trace2 region, this actually
> does make a change in behavior, I think. In a good way.

I don't see the behavior change.

Yes start_delayed_progress() will call start_progress_delay() which
mallocs and enters the trace2 region, and then if you don't call
stop_progress() at all you won't leave it.

But in read-cache.c both before & after my change we only malloc & only
enter the region if we're actually displaying the progress, there's an
isatty() guard on it.

Once we start that progress bar we will leave the trace2 region, via
stop_progress(), but note that stop_progress() will exit early if the
pointer you dereference is NULL and not do that, and since we'll have
"*progress = NULL" in the case of not wanting the progress bar we won't
leave the region we didn't enter in the first place.

The change here is just to remove the needless nano-optimization of
guarding the calls with a NULL check of "progress", which the API itself
does, no?

Re: [PATCH 1/2] read-cache.c: don't guard calls to progress.c API

[Derrick Stolee]

On 6/7/2021 11:52 AM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Mon, Jun 07 2021, Derrick Stolee wrote:
> 
>> On 6/7/2021 10:43 AM, Ævar Arnfjörð Bjarmason wrote:
>>> Don't guard the calls to the progress.c API with "if (progress)". The
>>> API itself will check this. This doesn't change any behavior, but
>>> makes this code consistent with the rest of the codebase.
>>
>> Since stop_progress() closes a trace2 region, this actually
>> does make a change in behavior, I think. In a good way.
> 
> I don't see the behavior change.
> 
> Yes start_delayed_progress() will call start_progress_delay() which
> mallocs and enters the trace2 region, and then if you don't call
> stop_progress() at all you won't leave it.
> 
> But in read-cache.c both before & after my change we only malloc & only
> enter the region if we're actually displaying the progress, there's an
> isatty() guard on it.

That's right. I misremembered where this trace2 stuff was.

The idea we didn't pursue was to create the progress struct
unconditionally, and just leave it as "quiet" based on an
input parameter. That would keep the trace2 regions consistent.

But I'm wrong and there is no behavior change here.

Thanks,
-Stolee

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

[Ævar Arnfjörð Bjarmason]

Fix a potential incorrect display of the number of items (off by one)
and stalling of the progress bar in refresh_index().

The off-by-one error is minor, we should say we're processing the 1st
item, not the 0th. This along with the next change also allows us to
remove the last display_progress() call outside the loop, as we'll
always have reached 100% now.

Let's also move the display_progress() call to the very start of the
loop refresh_index() loop. In the loop we first check whether e.g. we
ignore submodules and the entry we're processing is a submodule,
whether we ignore certain paths etc.. Thus we could have a
pathological case where we have a huge index consisting of such
ignored entries, and we'd stall on the progress bar.

See ae9af12287 (status: show progress bar if refreshing the index
takes too long, 2018-09-15) for the initial addition of this progress
bar to refresh_index().

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 read-cache.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index 470f800855..8b0073a839 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1594,6 +1594,8 @@ int refresh_index(struct index_state *istate, unsigned int flags,
 		int t2_did_lstat = 0;
 		int t2_did_scan = 0;
 
+		display_progress(progress, i + 1);
+
 		ce = istate->cache[i];
 		if (ignore_submodules && S_ISGITLINK(ce->ce_mode))
 			continue;
@@ -1627,7 +1629,6 @@ int refresh_index(struct index_state *istate, unsigned int flags,
 		t2_sum_scan += t2_did_scan;
 		if (new_entry == ce)
 			continue;
-		display_progress(progress, i);
 		if (!new_entry) {
 			const char *fmt;
 
@@ -1662,7 +1663,6 @@ int refresh_index(struct index_state *istate, unsigned int flags,
 	trace2_data_intmax("index", NULL, "refresh/sum_lstat", t2_sum_lstat);
 	trace2_data_intmax("index", NULL, "refresh/sum_scan", t2_sum_scan);
 	trace2_region_leave("index", "refresh", NULL);
-	display_progress(progress, istate->cache_nr);
 	stop_progress(&progress);
 	trace_performance_leave("refresh index");
 	return has_errors;
-- 
2.32.0.rc3.434.gd8aed1f08a7

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

[Derrick Stolee]

On 6/7/2021 10:43 AM, Ævar Arnfjörð Bjarmason wrote:
> Fix a potential incorrect display of the number of items (off by one)
> and stalling of the progress bar in refresh_index().
> 
> The off-by-one error is minor, we should say we're processing the 1st
> item, not the 0th. This along with the next change also allows us to
> remove the last display_progress() call outside the loop, as we'll
> always have reached 100% now.

This "pre-announce the progress" seems correct and is unlikely
to have a user sitting at "100%" while the loop is actually doing
work on that last cache entry.

Thanks,
-Stolee

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

[Ævar Arnfjörð Bjarmason]

On Mon, Jun 07 2021, Derrick Stolee wrote:

> On 6/7/2021 10:43 AM, Ævar Arnfjörð Bjarmason wrote:
>> Fix a potential incorrect display of the number of items (off by one)
>> and stalling of the progress bar in refresh_index().
>> 
>> The off-by-one error is minor, we should say we're processing the 1st
>> item, not the 0th. This along with the next change also allows us to
>> remove the last display_progress() call outside the loop, as we'll
>> always have reached 100% now.
>
> This "pre-announce the progress" seems correct and is unlikely
> to have a user sitting at "100%" while the loop is actually doing
> work on that last cache entry.

I guess pre-announce v.s. post-announce is a matter of some philosophy,
for O(n) when can we be said to be doing work on n[0]? We entered the
for-loop and are doing work on that istate->cache[i] item, so I'd like
to think of it more as post-announce :)

In any case, I'm changing this to the established pattern we use in most
other places in the codebase, this one was an odd one out.

Thanks for the review of this.

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

[René Scharfe]

Am 07.06.21 um 17:58 schrieb Ævar Arnfjörð Bjarmason:
>
> On Mon, Jun 07 2021, Derrick Stolee wrote:
>
>> On 6/7/2021 10:43 AM, Ævar Arnfjörð Bjarmason wrote:
>>> Fix a potential incorrect display of the number of items (off by one)
>>> and stalling of the progress bar in refresh_index().
>>>
>>> The off-by-one error is minor, we should say we're processing the 1st
>>> item, not the 0th. This along with the next change also allows us to
>>> remove the last display_progress() call outside the loop, as we'll
>>> always have reached 100% now.
>>
>> This "pre-announce the progress" seems correct and is unlikely
>> to have a user sitting at "100%" while the loop is actually doing
>> work on that last cache entry.
>
> I guess pre-announce v.s. post-announce is a matter of some philosophy,
> for O(n) when can we be said to be doing work on n[0]? We entered the
> for-loop and are doing work on that istate->cache[i] item, so I'd like
> to think of it more as post-announce :)

Say you have a single item to process and it takes a minute.  The
original code shows 0% for a minute, then 100% at the end.  With your
change you'd get 100% for a minute.  Both would be annoying, but the
latter would have me raging.  "If you're done", I'd yell at the uncaring
machine, "what are you still doing!?".

Showing only the completed items makes sense.  That the next one is
being processed is self-understood.  Once all of them are done, 100% is
shown and the progress line is finished.

So I think this pattern works:

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

Alternatively, if the work part doesn't contain continue statements:

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

> In any case, I'm changing this to the established pattern we use in most
> other places in the codebase, this one was an odd one out.

Consistency is a good thing, but perhaps some of these other places
should be changed.  It doesn't matter much because most items git deals
with are processed quickly, so an off-by-one error should barely be
noticeable, but still it would be nice to get it right.  It's hard to
test, though.

René

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

[Ævar Arnfjörð Bjarmason]

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

> Am 07.06.21 um 17:58 schrieb Ævar Arnfjörð Bjarmason:
>>
>> On Mon, Jun 07 2021, Derrick Stolee wrote:
>>
>>> On 6/7/2021 10:43 AM, Ævar Arnfjörð Bjarmason wrote:
>>>> Fix a potential incorrect display of the number of items (off by one)
>>>> and stalling of the progress bar in refresh_index().
>>>>
>>>> The off-by-one error is minor, we should say we're processing the 1st
>>>> item, not the 0th. This along with the next change also allows us to
>>>> remove the last display_progress() call outside the loop, as we'll
>>>> always have reached 100% now.
>>>
>>> This "pre-announce the progress" seems correct and is unlikely
>>> to have a user sitting at "100%" while the loop is actually doing
>>> work on that last cache entry.
>>
>> I guess pre-announce v.s. post-announce is a matter of some philosophy,
>> for O(n) when can we be said to be doing work on n[0]? We entered the
>> for-loop and are doing work on that istate->cache[i] item, so I'd like
>> to think of it more as post-announce :)
>
> Say you have a single item to process and it takes a minute.  The
> original code shows 0% for a minute, then 100% at the end.  With your
> change you'd get 100% for a minute.  Both would be annoying, but the
> latter would have me raging.  "If you're done", I'd yell at the uncaring
> machine, "what are you still doing!?".

Perhaps if we said "100% and Reticulating splines[...]" :)

> Showing only the completed items makes sense.  That the next one is
> being processed is self-understood.  Once all of them are done, 100% is
> shown and the progress line is finished.
>
> So I think this pattern works:
>
> 	for (i = 0; i < nr; i++) {
> 		display_progress(p, i);
> 		/* work work work */
> 	}
> 	display_progress(p, nr);
>
> Alternatively, if the work part doesn't contain continue statements:
>
> 	for (i = 0; i < nr; i++) {
> 		/* work work work */
> 		display_progress(p, i + 1);
> 	}

But yes, I agree with the issue in theory, but I think in practice we
don't need to worry about these 100% cases.

We usually only display this anyway with a really big O(n), or (if we
correctly use the API) one where each item isn't that expensive, we just
do a lot of work in the aggregate.

So having a display_progress() at the top of the for-loop with "i + 1"
avoids needing two of them, or worrying about "continue" statements etc,
or (as in this case) where the data we're processing can be 10k items
with the first 8k being items we skip, so we'd be seen to hang, or
"jump" from 10% to 50%, then smoothly update 50%..60%, and jump again
etc.

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

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

>> So I think this pattern works:
>>
>> 	for (i = 0; i < nr; i++) {
>> 		display_progress(p, i);
>> 		/* work work work */
>> 	}
>> 	display_progress(p, nr);
>>
>> Alternatively, if the work part doesn't contain continue statements:
>>
>> 	for (i = 0; i < nr; i++) {
>> 		/* work work work */
>> 		display_progress(p, i + 1);
>> 	}
>
> But yes, I agree with the issue in theory, but I think in practice we
> don't need to worry about these 100% cases.

Hmph, but in practice we do need to worry, don't we?  Otherwise you
wouldn't have started this thread and René wouldn't have responded.

I agree with the issue and I think we should count what we have
finished.

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

[Ævar Arnfjörð Bjarmason]

On Tue, Jun 08 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>>> So I think this pattern works:
>>>
>>> 	for (i = 0; i < nr; i++) {
>>> 		display_progress(p, i);
>>> 		/* work work work */
>>> 	}
>>> 	display_progress(p, nr);
>>>
>>> Alternatively, if the work part doesn't contain continue statements:
>>>
>>> 	for (i = 0; i < nr; i++) {
>>> 		/* work work work */
>>> 		display_progress(p, i + 1);
>>> 	}
>>
>> But yes, I agree with the issue in theory, but I think in practice we
>> don't need to worry about these 100% cases.
>
> Hmph, but in practice we do need to worry, don't we?  Otherwise you
> wouldn't have started this thread and René wouldn't have responded.

I started this thread because of:

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

Mainly because it's a special snowflake in how the process.c API is
used, with most other callsites doing:

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

Which yes, as René points out *could* hang on 100%, but I think in
practice isn't an issue here, and changing the code per my patch here
solves the practical issue with us always taking the maybe_branch_here()
(or enough that the progress bar hangs).

> I agree with the issue and I think we should count what we have
> finished.

Fair enough, but in the meantime can we take this patch? I think fixing
that (IMO in practice hypothetical issue) is much easier when we
consistently use that "i + 1" pattern above (which we mostly do
already). We can just search-replace "++i" to "i++" and "i + 1" to "i"
and have stop_progress() be what bumps it to 100%.

I have some unsent patches queued on top of this which has some general
fixes to edge cases in the progress.c API making that & more easier...q

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

[René Scharfe]

Am 08.06.21 um 12:58 schrieb Ævar Arnfjörð Bjarmason:
>
> On Tue, Jun 08 2021, Junio C Hamano wrote:
>
>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>>
>>>> So I think this pattern works:
>>>>
>>>> 	for (i = 0; i < nr; i++) {
>>>> 		display_progress(p, i);
>>>> 		/* work work work */
>>>> 	}
>>>> 	display_progress(p, nr);
>>>>
>>>> Alternatively, if the work part doesn't contain continue statements:
>>>>
>>>> 	for (i = 0; i < nr; i++) {
>>>> 		/* work work work */
>>>> 		display_progress(p, i + 1);
>>>> 	}
>>>
>>> But yes, I agree with the issue in theory, but I think in practice we
>>> don't need to worry about these 100% cases.
>>
>> Hmph, but in practice we do need to worry, don't we?  Otherwise you
>> wouldn't have started this thread and René wouldn't have responded.
>
> I started this thread because of:
>
> 	for (i = 0; i < large_number; i++) {
> 		if (maybe_branch_here())
> 			continue;
> 		/* work work work */
> 		display_progress(p, i);
> 	}
> 	display_progress(p, large_number);
>
> Mainly because it's a special snowflake in how the process.c API is
> used, with most other callsites doing:
>
> 	for (i = 0; i < large_number; i++) {
> 		display_progress(p, i + 1);
> 		/* work work work */
> 	}

Moving the first call to the top of the loop makes sense.  It ensures
all kind of progress -- skipping and actual work -- is reported without
undue delay.

Adding one would introduce an off-by-one error.  Removing the call after
the loop would leave the progress report at one short of 100%.  I don't
see any benefits of these additional changes, only downsides.

If other callsites have an off-by-one error and we care enough then we
should fix them.  Copying their style and spreading the error doesn't
make sense -- correctness trumps consistency.

> Fair enough, but in the meantime can we take this patch? I think fixing
> that (IMO in practice hypothetical issue) is much easier when we
> consistently use that "i + 1" pattern above (which we mostly do
> already). We can just search-replace "++i" to "i++" and "i + 1" to "i"
> and have stop_progress() be what bumps it to 100%.

This assumes the off-by-one error is consistent.  Even if that is the
case you could apply your mechanical fix and leave out read-cache.
This would happen automatically because when keeping i there is no ++i
to be found.

stop_progress() doesn't set the progress to 100%:

   $ (echo progress 0; echo update) |
     ./t/helper/test-tool progress --total 1 test
   test:   0% (0/1), done.

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

René

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

[Ævar Arnfjörð Bjarmason]

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

> Am 08.06.21 um 12:58 schrieb Ævar Arnfjörð Bjarmason:
>>
>> On Tue, Jun 08 2021, Junio C Hamano wrote:
>>
>>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>>>
>>>>> So I think this pattern works:
>>>>>
>>>>> 	for (i = 0; i < nr; i++) {
>>>>> 		display_progress(p, i);
>>>>> 		/* work work work */
>>>>> 	}
>>>>> 	display_progress(p, nr);
>>>>>
>>>>> Alternatively, if the work part doesn't contain continue statements:
>>>>>
>>>>> 	for (i = 0; i < nr; i++) {
>>>>> 		/* work work work */
>>>>> 		display_progress(p, i + 1);
>>>>> 	}
>>>>
>>>> But yes, I agree with the issue in theory, but I think in practice we
>>>> don't need to worry about these 100% cases.
>>>
>>> Hmph, but in practice we do need to worry, don't we?  Otherwise you
>>> wouldn't have started this thread and René wouldn't have responded.
>>
>> I started this thread because of:
>>
>> 	for (i = 0; i < large_number; i++) {
>> 		if (maybe_branch_here())
>> 			continue;
>> 		/* work work work */
>> 		display_progress(p, i);
>> 	}
>> 	display_progress(p, large_number);
>>
>> Mainly because it's a special snowflake in how the process.c API is
>> used, with most other callsites doing:
>>
>> 	for (i = 0; i < large_number; i++) {
>> 		display_progress(p, i + 1);
>> 		/* work work work */
>> 	}
>
> Moving the first call to the top of the loop makes sense.  It ensures
> all kind of progress -- skipping and actual work -- is reported without
> undue delay.
>
> Adding one would introduce an off-by-one error.  Removing the call after
> the loop would leave the progress report at one short of 100%.  I don't
> see any benefits of these additional changes, only downsides.
>
> If other callsites have an off-by-one error and we care enough then we
> should fix them.  Copying their style and spreading the error doesn't
> make sense -- correctness trumps consistency.
>
>> Fair enough, but in the meantime can we take this patch? I think fixing
>> that (IMO in practice hypothetical issue) is much easier when we
>> consistently use that "i + 1" pattern above (which we mostly do
>> already). We can just search-replace "++i" to "i++" and "i + 1" to "i"
>> and have stop_progress() be what bumps it to 100%.
>
> This assumes the off-by-one error is consistent.  Even if that is the
> case you could apply your mechanical fix and leave out read-cache.
> This would happen automatically because when keeping i there is no ++i
> to be found.
>
> stop_progress() doesn't set the progress to 100%:
>
>    $ (echo progress 0; echo update) |
>      ./t/helper/test-tool progress --total 1 test
>    test:   0% (0/1), done.
>

I was too quick with that "But yes, I agree with the issue in theory".

Having thought about it some more I think you're wrong, it doesn't make
sense to use the API in the way you're suggesting.

There's an off-by-one error, but it's in the pattern you're
suggesting. Calling "i + 1" on the "first item" doesn't just make for
less verbose code, it's also more correct.

Why? Because:

    /* 1. Setup progress */
    large_number = 3;
    progress = start_progress(title, total);

    /* 2. Before we "actually" do anything */
    for (i = 0; i < 3; i++) {
        /* 3. Now doing O(large_number) work */
        display_progress(progress, i + 1);
    }

    /* 4. Done */
    stop_progress(&progress);

As you'll see if you insert a sleep or whatever at "2" we'll signal and
display the progress bar even if we haven't called display_progress()
yet.

Thus calling display_progress with n=0 makes "we are doing the first
item" indistinguishable from "we haven't gotten to the first item
yet". When you're in the loop the first item should be n=1.

This isn't just more correct with this API. I think it also makes sense
not to leak the zero indexed C abstraction to human output. As in:

    I sat down at the table with my three apples (stages 1..2 above),
    and now I'm eating my first apple (stage 3), I'm not starting to eat
    my zeroeth apple. At some point I'm done eating all 3 apples (stage
    4).

I think this gets to the crux of the issue for you, per your upthread:

    [...]With your change you'd get 100% for a minute.  Both would be
    annoying, but the latter would have me raging.  "If you're done",
    I'd yell at the uncaring machine, "what are you still doing!?".

If we said we're done and we're not then yes, that would be a bug.

But that's not what we said. Distinguishing "I'm on 3/3" from "I'm done"
is exactly what the progress.c output does:

    $ perl -wE 'for (0..3) { 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:  66% (2/3)^M
    Apples: 100% (3/3)^M
    Apples: 100% (3/3), done.

It's not immediately obvious from that output, but the last line ending
in ", done" isn't added by any display_progress() call, but by calling
stop_progress(). That's when we're done.

Arguably this is too subtle and we should say ", but not done yet!" or
something on that penultimate line. But that's bikeshedding a display
issue; The API use in my patch is correct.

As I noted upthread that's a "matter of some philosophy". I guess you
might report your progress as being at 2/3 apples, even though you're
one bite away from finishing the third apple. Personally I'd say I'm on
the third apple, I'm just not done with it.

But the philosophizing of whether your "progress" is the zeroeth apple
of the day as you're chewing on it aside, I think it's clear that in the
context of the progress.c API using n=0 for the first apple would be a
bug.

You'll conflate "setup" with "work" per the above, and you'll say you're
on the second apple even if you're a bite away from finishing it. We
don't conflate that "100%" state with being "done", so I don't see why
we'd do that.

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

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.

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

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> I was too quick with that "But yes, I agree with the issue in theory".
>
> Having thought about it some more I think you're wrong, it doesn't make
> sense to use the API in the way you're suggesting.

Sorry, I've read the message I am responding to a few times, but I
couldn't tell what you are arguing against in the suggestion given
earlier by René, which offered two possibile ways to consistently
call display() with the number of things that we have DONE
processing (not the number of things that we are about to touch) [*1*].

> Why? Because:
>
>     /* 1. Setup progress */
>     large_number = 3;
>     progress = start_progress(title, total);
>
>     /* 2. Before we "actually" do anything */
>     for (i = 0; i < 3; i++) {
>         /* 3. Now doing O(large_number) work */
>         display_progress(progress, i + 1);
>     }
>
>     /* 4. Done */
>     stop_progress(&progress);
>
> As you'll see if you insert a sleep or whatever at "2" we'll signal and
> display the progress bar even if we haven't called display_progress()
> yet.

That is, the signal start_progress_delay() kicking in?  But
progress_test_force_update() is a curiosity that appears only in
test-tool and in real life programs, you'd have to call display
yourself, so a long delay at "3" will appear as a long silence,
I would think.

In any case, if we somehow managed to cause display_progress() to
happen somewhere near "2", we haven't finished even a single item
yet at that point, so "we are giving progress bar, and we have
finished 0%" that is an output from such a call would make sense.
As we finish one item, we'd increment it to 1 (that happens after we
spent time at "3" during the first iteration, and display_progress()
is called with "i+1").

> Thus calling display_progress with n=0 makes "we are doing the first
> item" indistinguishable from "we haven't gotten to the first item
> yet".

I am guessing that you are talking about "what value should we call
display() if '2' takes a long time and we want to give progress
early even before we enter the loop?"

I do not view such a call as "we haven't gotten to" or "we are
doing"; an extra call we would make with display(n=0) around "2" is
"how much have we finished?  we have completed none".

> When you're in the loop the first item should be n=1.

Doesn't that depend on where in the loop you do the work?

If you spend cycles and finish processing the first item _before_
calling display_progress(), you are reporting how many you have
finished, so at the end of the first iteration of the loop, you'd
call with n=1.

If on the other hand you somehow report at the beginning (perhaps
because otherwise you'd have tough time structuring the code when
the loop body has a continue etc.) before finishing the processing
for the first item, you'd call with n=0 (and make sure before
calling stop_progress(), you'd call another display() after exiting
the loop).

And I think that is consistent with what Réne suggested earlier, so
I am not sure where your "I am right you are wrong" is coming from.
To me, you two seem to be agreeing.


[Footnote]

*1* <eaf2b6b0-4202-d5ea-87a2-b828fdbc60a1@web.de>

> Showing only the completed items makes sense.  That the next one is
> being processed is self-understood.  Once all of them are done, 100% is
> shown and the progress line is finished.
> 
> So I think this pattern works:
> 
> 	for (i = 0; i < nr; i++) {
> 		display_progress(p, i);
> 		/* work work work */
> 	}
> 	display_progress(p, nr);
> 
> Alternatively, if the work part doesn't contain continue statements:
> 
> 	for (i = 0; i < nr; i++) {
> 		/* work work work */
> 		display_progress(p, i + 1);
> 	}

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

[René Scharfe]

Am 10.06.21 um 07:30 schrieb Junio C Hamano:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> I was too quick with that "But yes, I agree with the issue in theory".
>>
>> Having thought about it some more I think you're wrong, it doesn't make
>> sense to use the API in the way you're suggesting.
>
> Sorry, I've read the message I am responding to a few times, but I
> couldn't tell what you are arguing against in the suggestion given
> earlier by René, which offered two possibile ways to consistently
> call display() with the number of things that we have DONE
> processing (not the number of things that we are about to touch) [*1*].

Same here.

Perhaps a demo helps.  The patch at the bottom adds an echo command to
the test helper.  This way we can intersperse the progress lines with
indications when items are processed.

So here's the pattern for calling display_progress at the top of the
loop and again with the number of items after the loop:

  $ (
    for i in 0 1 2
    do
      echo progress $i
      echo update
      echo echo WORK
    done
    echo progress 3
  ) | ./t/helper/test-tool progress --total 3 test 2>&1 | tr '\r' '\n'

  test:   0% (0/3)
  WORK
  test:  33% (1/3)
  WORK
  test:  66% (2/3)
  WORK
  test: 100% (3/3)
  test: 100% (3/3), done.

The progress lines reflect the number of finished items at all times.

Here's the pattern for display_progress at the bottom of the loop:


  $ (
    for i in 0 1 2
    do
      echo echo WORK
      echo progress $(( $i + 1 ))
      echo update
    done
  ) | ./t/helper/test-tool progress --total 3 test 2>&1 | tr '\r' '\n'

  WORK
  test:  33% (1/3)
  WORK
  test:  66% (2/3)
  WORK
  test: 100% (3/3)
  test: 100% (3/3), done.

Same here, the progress line shows the correct number of finished items.

Here's the pattern suggested in your patch:

  $ (
    for i in 0 1 2
    do
      echo progress $(( $i + 1 ))
      echo update
      echo echo WORK
    done
  ) | ./t/helper/test-tool progress --total 3 test 2>&1 | tr '\r' '\n'

  test:  33% (1/3)
  WORK
  test:  66% (2/3)
  WORK
  test: 100% (3/3)
  WORK
  test: 100% (3/3), done.

It reports one item too many in the intermediate progress lines and
is correct only at the very end.

René


diff --git a/t/helper/test-progress.c b/t/helper/test-progress.c
index 5d05cbe789..b6589f3878 100644
--- a/t/helper/test-progress.c
+++ b/t/helper/test-progress.c
@@ -65,6 +65,8 @@ int cmd__progress(int argc, const char **argv)
 			display_throughput(progress, byte_count);
 		} else if (!strcmp(line.buf, "update"))
 			progress_test_force_update();
+		else if (skip_prefix(line.buf, "echo ", (const char **) &end))
+			fprintf(stderr, "%s\n", end);
 		else
 			die("invalid input: '%s'\n", line.buf);
 	}

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

[René Scharfe]

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.

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

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

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

[Ævar Arnfjörð Bjarmason]

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.

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

[René Scharfe]

Am 14.06.21 um 13:07 schrieb Ævar Arnfjörð Bjarmason:
>
> 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.

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.

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

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

The progress display could show just the title before the first
display_progress() call to make the distinction clear.  Would it really
be useful, though?  Perhaps a trace2 region started by the first
display_progress() call and ended by the last one (n == total) would
be better.

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

How so?  start_progress() specifies a title and the total number of
items, display_progress() reports some other number that is shown in
relation to the total, and stop_progress() finishes the progress line.
This API is not documented and thus its meaning is (strictly speaking)
left unspecified.

It can be used to show a classic "percent-done progress indicator", as
https://dl.acm.org/doi/10.1145/1165385.317459 calls it.  That's how I
read a growing percentage shown by a program, and "done" I understand
as "has been done" (completed), not as "is being done".

Wikipedia sent me to
https://chrisharrison.net/projects/progressbars/ProgBarHarrison.pdf,
which has some fun ideas on how to warp the perception of time for
users staring at a progress bar, but also states typical ones grow
with the amount of completed work.

René

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

[Ævar Arnfjörð Bjarmason]

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

> Am 14.06.21 um 13:07 schrieb Ævar Arnfjörð Bjarmason:
>>
>> 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.
>
> 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.
>
>>> 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.

>>>> 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 <8735tt4fhx.fsf@evledraar.gmail.com>. 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?

> The progress display could show just the title before the first
> display_progress() call to make the distinction clear.  Would it really
> be useful, though?  Perhaps a trace2 region started by the first
> display_progress() call and ended by the last one (n == total) would
> be better.

Yes, it should display 0/N if it's stalled. I have some WIP patches to
do that. I misrecalled that we were updating the progress from the
signal handler, but no, we wait for the first display(), but if we just
call display() from the handler with a "stalled" message we'll clearly
show these cases where we're stalling in the preparation.

>>> 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.
>
> How so?  start_progress() specifies a title and the total number of
> items, display_progress() reports some other number that is shown in
> relation to the total, and stop_progress() finishes the progress line.
> This API is not documented and thus its meaning is (strictly speaking)
> left unspecified.

I don't mean that it's clearly documented, I'm making the case that the
API is most sanely used in the way I've laid out. It seems we're
respectfully disagreeing on that, but ...

> It can be used to show a classic "percent-done progress indicator", as
> https://dl.acm.org/doi/10.1145/1165385.317459 calls it.  That's how I
> read a growing percentage shown by a program, and "done" I understand
> as "has been done" (completed), not as "is being done".

...I think that's pretty moot in the first place, but I also think
thinking about progress.c's idea of the world as "has been done" doesn't
make sense when you look at its API holistically.

> Wikipedia sent me to
> https://chrisharrison.net/projects/progressbars/ProgBarHarrison.pdf,
> which has some fun ideas on how to warp the perception of time for
> users staring at a progress bar, but also states typical ones grow
> with the amount of completed work.

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:
	
	+Working hard:  50% (1/2)<CR>
	+Working hard:  50% (1/2), 1.91 MiB | 195.00 KiB/s<CR>
	+Working hard:  50% (1/2), 2.86 MiB | 146.00 KiB/s<CR>
	+Working hard:  50% (1/2), 3.81 MiB | 130.00 KiB/s<CR>
	+Working hard:  50% (1/2), 3.81 MiB | 97.00 KiB/s, done.

Given something like (I've locally modified this a bit):

	cat >in <<-\EOF &&
	start 2
	throughput 1 10000
	update
	progress 1
	update
	throughput 100000 10000
	update
	throughput 2000000 20000
	update
	throughput 3000000 30000
	update
	throughput 4000000 40000
	update
	stop
	EOF
	test-tool progress <in 2>stderr &&

That's another reason I'm maintaining that reporting progress as "is
being done" is the only sane way to look at it, because if you think it's
"has been done" you preclude the API from being used for cases where you
e.g. want to download 2 files, each file takes 1 minute, and you want to
show progress on the item itself.

Our API mostly just stops before the "progress on the item itself", but
if you think that layer should be "has been done" doesn't that mean that
display_throughput() would have to be entirely redesigned?

After all there's no way to feed a N for item number into it, it
optionally *updates* the progress on an existing item, such a thing
can't exist if we only call display_progress() to report items as being
done.

I do think the actual output we show is pretty crappy for that use-case,
but that output can be changed, but not really if we insist on the
invariant you're maintaining that display_progress() must only be called
if the item is done.

Just like showing a "stalled" message if we get a signal before we show
the 1st item doing that is actually pretty easy, i.e. we could showh 1/2
downloaded files, and then as we get signals show a spinner...

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

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> 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:
> 	
> 	+Working hard:  50% (1/2)<CR>
> 	+Working hard:  50% (1/2), 1.91 MiB | 195.00 KiB/s<CR>
> 	+Working hard:  50% (1/2), 2.86 MiB | 146.00 KiB/s<CR>
> 	+Working hard:  50% (1/2), 3.81 MiB | 130.00 KiB/s<CR>
> 	+Working hard:  50% (1/2), 3.81 MiB | 97.00 KiB/s, done.
> ...
> That's another reason I'm maintaining that reporting progress as "is
> being done" is the only sane way to look at it, because if you think it's
> "has been done" you preclude the API from being used for cases where you
> e.g. want to download 2 files, each file takes 1 minute, and you want to
> show progress on the item itself.

Sorry, but I do not understand your argument here at all.

If you show "has been completed", when such a thing starts, I'd see
for a minute this:

 Downloading (0/2) ... X MiB | Y kiB/s

and then it will switch to

 Downloading (1/2) ... progress ...

and after staring at it for another minute, I'd see

 Downloading (2/2) ... done.

And such an output makes sense to me.  It is obvious, at least to
me, that the output during the first minute is telling me that it
hasn't finished but working hard to turn that "0" into "1" at the
pace the throughput thing is showing.

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

[René Scharfe]

Am 14.06.21 um 21:08 schrieb Ævar Arnfjörð Bjarmason:
>
> On Mon, Jun 14 2021, René Scharfe wrote:
>
>> Am 14.06.21 um 13:07 schrieb Ævar Arnfjörð Bjarmason:
>>>
>>> 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.
>>
>> 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.
>>
>>>> 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.

>>>>> 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 <8735tt4fhx.fsf@evledraar.gmail.com>. 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.

René

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

[Ævar Arnfjörð Bjarmason]

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

> Am 14.06.21 um 21:08 schrieb Ævar Arnfjörð Bjarmason:
>>
>> On Mon, Jun 14 2021, René Scharfe wrote:
>>
>>> Am 14.06.21 um 13:07 schrieb Ævar Arnfjörð Bjarmason:
>>>>
>>>> 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.
>>>
>>> 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.
>>>
>>>>> 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.

>>>>>> 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 <8735tt4fhx.fsf@evledraar.gmail.com>. 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.

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

[René Scharfe]

Am 15.06.21 um 18:46 schrieb Ævar Arnfjörð Bjarmason:
> 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.

Perhaps.  You seem to think of progress as being represented by a real
number.  We show integers, though, and you want to round up.

The progress display's function is to inform the user that work is being
done and how much there is still to do.  It allows them to decide
whether to keep the program running.

A progress of "100%" being shown for an extended duration would lead me
to the conclusion that the program hangs and I'd cancel it.  Rounding
down (truncating) prevents that.

Showing an estimated time of completion as well would be even nicer,
but is only practical if the time taken for each work item is roughly
the same.

But let's move on indeed.  The part of your patch that moves the
display_progress() call to the top of the loop to avoid stalling is a
good idea and worth splitting out into its own patch (keeping the "i").

In general it seems that changes described with "Let's also ..." or
"While at it ..." almost always deserve their own patch.  I need to
follow that insight more myself..

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

Junio already replied to that, but since you ask, here are my thoughts:

Progress and throughput are separate metrics.  Adding one doesn't change
the other.  The throughput value is not specific to the currently
processed item.

Say we download a number of files of different sizes and want to show
our progress.  Then from time to time we display the number of processed
files and how many bytes we got since the last update, divided by the
time passed since then.  The reported bytes could belong to multiple
files.  Or we could process lots of zero-sized files, which would keep
throughput low.

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

Not exactly.  While I think a difference of one isn't a big deal most
of the time, also think there is a correct way, i.e. to show the number
of completed items.  You have found ways to use an off-by-one error, and
my point was that this usage is not reliable.  Perhaps that's a weak
and convoluted argument.

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

I agree that we should be consistent.  If we have downloaded 70% of the
first of five files then we have 0.7 files, which is not yet 1 file, so
we have to say 0/5.

But let's move on, for real.

René