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

From: Daniel Barkalow <barkalow@iabervon.org>
Date: Feb 5, 2008, 20:38 UTC
Message-ID: <alpine.LNX.1.00.0802042127210.13593@iabervon.org>
In-Reply-To: <alpine.LSU.1.00.0802050129570.8543@racer.site>

On Tue, 5 Feb 2008, Johannes Schindelin wrote:

Show 25 quoted lines

> 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).

	-Daniel
*This .sig left intentionally blank*