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

[Junio C Hamano <gitster@pobox.com>]

Æ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);
> 	}