On Fri, Oct 12 2018, Junio C Hamano wrote:
> SZEDER Gábor <szeder.dev@xxxxxxxxx> writes:
>
>>> for (i = 0; i < oids->nr; i++) {
>>> + display_progress(progress, ++j);
>>> commit = lookup_commit(the_repository, &oids->list[i]);
>>>
>>> if (commit && !parse_commit(commit))
>>> @@ -611,19 +624,28 @@ static void close_reachable(struct packed_oid_list *oids)
>>> }
>>
>> The above loops have already counted all the commits, and, more
>> importantly, did all the hard work that takes time and makes the
>> progress indicator useful.
>>
>>> for (i = 0; i < oids->nr; i++) {
>>> + display_progress(progress, ++j);
>>
>> This display_progress() call, however, doesn't seem to be necessary.
>> First, it counts all commits for a second time, resulting in the ~2x
>> difference compared to the actual number of commits, and then causing
>> my confusion. Second, all what this loop is doing is setting a flag
>> in commits that were already looked up and parsed in the above loops.
>> IOW this loop is very fast, and the progress indicator jumps from
>> ~780k right to 1.5M, even on my tiny laptop, so it doesn't need a
>> progress indicator at all.
>
> Makes sense. If this second iteration were also time consuming,
> then it probably is a good idea to split these into two separate
> phases? "Counting 1...N" followed by "Inspecting 1...N" or
> something like that. Of course, if the latter does not take much
> time, then doing without any progress indicator is also fine.
That's a good point. Derrick: If the three loops in close_reachable()
had to be split up into different progress stages and given different
names what do you think they should be? Now it's "Annotating commits in
commit graph" for all of them.
