Re: [PATCH 5/9] Add "skip_unmerged" option to unpack_trees.

[Junio C Hamano <gitster@pobox.com>]

Daniel Barkalow <barkalow@iabervon.org> 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.