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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 10, 2021, 05:30 UTC
Message-ID: <xmqq4ke6ffqv.fsf@gitster.g>
In-Reply-To: <87lf7k2bem.fsf@evledraar.gmail.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*].

Show 18 quoted lines

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

Show 18 quoted lines

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