From: Derrick Stolee Date: Wed, 05 Sep 2018 22:12:53 GMT Subject: Re: [PATCH 1/2] commit-graph write: add progress output Message-ID: <8ea40192-ea3a-33e6-c561-ee7b4ab7f040@gmail.com> In-Reply-To: On 9/5/2018 5:46 PM, Junio C Hamano wrote: > Derrick Stolee 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!