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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jun 8, 2021, 10:58 UTC
Message-ID: <87wnr4394y.fsf@evledraar.gmail.com>
In-Reply-To: <xmqqczsxtf8g.fsf@gitster.g>

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

Show 22 quoted lines

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