Re: [PATCH 1/2] commit-graph write: add progress output

[Derrick Stolee <stolee@gmail.com>]

On 9/5/2018 5:46 PM, Junio C Hamano wrote:
> Derrick Stolee <stolee@gmail.com> writes:
>
>>>>    	for (i = 0; i < commits->nr; i++) {
>>>> +		display_progress(progress, i);
>>>>    		if (commits->list[i]->generation != GENERATION_NUMBER_INFINITY &&
>>>>    		    commits->list[i]->generation != GENERATION_NUMBER_ZERO)
>>>>    			continue;
>>> I am wondering if the progress call should be moved after this
>>> conditional continue; would we want to count the entry whose
>>> generation is already known here?  Of course, as we give commits->nr
>>> as the 100% ceiling, we cannot avoid doing so, but it somehow smells
>>> wrong.
>> If we wanted to be completely right, we would count the commits in the
>> list that do not have a generation number and report that as the 100%
>> ceiling.
> Yeah, but I realize that the definition of "right" really depends on
> what we consider a task being accomplished is in this loop.  If we
> define the task to "we have some number of commits that lack
> generation numbers and our task is to assign numbers to them.", then
> yes counting the ones without generation number and culling the ones
> that already have generation number is outside the work and we need
> another loop to count them.  But the position the posted patch takes
> is also a valid one: we have some commits and we are making sure
> each and every one of them has assigned a generation number.
>
> So I do not think it is necessary to introduce another loop just for
> counting.
>
> Thanks.
Makes sense to me. Thanks!