From: Junio C Hamano Date: Tue, 05 Feb 2008 23:44:11 GMT Subject: Re: [PATCH 5/9] Add "skip_unmerged" option to unpack_trees. Message-ID: <7vve536kac.fsf@gitster.siamese.dyndns.org> In-Reply-To: Daniel Barkalow writes: > On Tue, 5 Feb 2008, Johannes Schindelin wrote: > >> Hi, >> >> On Mon, 4 Feb 2008, Daniel Barkalow wrote: >> >> >> > if (any_files) { >> > - if (o->merge) { >> > + if (skip_entry) { >> > + do >> > + o->pos++; >> > + while (o->pos < active_nr && >> > + !strcmp(active_cache[o->pos]->name, >> > + src[0]->name)); >> > + } else if (o->merge) { >> >> Maybe it is just me, but I would have thought >> >> while (++o->pos < active_nr) >> if (strcmp(active_cache[o->pos]->name, >> src[0]->name)) >> break; >> >> more readable. But that's maybe because I have trouble with do ... while >> constructs logically (I like to see the loop condition first, then the >> loop body). > > I find yours less readable, because the loop condition is an exceptional > case (this is the last entry, so we run out of active_cache before finding > anything else), and you've got the actual effect of the loop in the > condition instead of the body, and I find using the value of ++x or x++ a > bit confusing outside of regular idioms. I'd go for: > > o->pos++; > while (o->pos < active_nr && > !strcmp(active_cache[o->pos]->name, > src[0]->name)) > o->pos++; > > if you care, though; it's not bad to make it clear we're skipping the > first of these entries based on a different consideration from the rest > (the first is the entry we decided to skip, and the rest are ones that > match it in filename). The last one is vastly more readable than your original, and moderately easier to follow than Dscho's, at least to me. Thanks.