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

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

Ævar Arnfjörð Bjarmason [off-list ref] writes:

quoted

quoted

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