On Tue, Jun 08 2021, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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
