Re: [PATCH] git clean: Don't automatically remove directories when run within subdirectory

[Shawn Bohrer <shawn.bohrer@gmail.com>]

On Mon, Apr 14, 2008 at 12:18:13AM -0700, Junio C Hamano wrote:
> Shawn Bohrer <shawn.bohrer@gmail.com> writes:
> > -		int len, pos, matches;
> > +		int len, pos;
> > +		int matches = 0;
> >  		struct cache_entry *ce;
> >  		struct stat st;
> 
> Initialization of "matches" seems to be an independent clean-up.  Although
> it forces the initialization in the codepath that do not need the value of
> matches, that is not a big deal --- right?

Yes this is an independent clean-up.  I can't see any harm in forcing
the initializtion.

> > -			matches = match_pathspec(pathspec, ent->name, ent->len,
> > +			matches = match_pathspec(pathspec, ent->name, len,
> >  						 baselen, seen);
> > -		} else {
> > -			matches = 0;
> >  		}
> 
> And the essential change (fix) is to send len which could be shorter than
> ent->len because we have stripped '/' here, plus the one in match_one()
> that now allows name[] that is not NUL terminated.

Yep, I'll add that to the changelog.

> > -			if (show_only && (remove_directories || matches)) {
> > +			if (show_only && (remove_directories || (matches >= 2))) {
> >  				printf("Would remove %s\n", qname);
> > -			} else if (remove_directories || matches) {
> > +			} else if (remove_directories || (matches >= 2)) {
> 
> These magic numbers are bad.  Please update it to use symbolic constants.

Agreed I'll send an updated patch later tonight.  One additional thought
though.  2 is MATCHED_FNMATCH which worries me a little because I think
this would mean 'git clean -f *' will also remove directories (I haven't
tried though).  Perhaps this should really be 3 MATCHED_EXACTLY just to
be safe.  Does anyone have opinions either way?

--
Shawn