Re: [PATCH v3 3/5] refs: teach for_each_ref a flag to avoid recursion

From: Jeff King <peff@peff.net>
Date: Jan 9, 2014, 21:55 UTC
Message-ID: <20140109215536.GB32069@sigill.intra.peff.net>
In-Reply-To: <xmqqa9f5avs3.fsf@gitster.dls.corp.google.com>

On Thu, Jan 09, 2014 at 09:51:24AM -0800, Junio C Hamano wrote:

Show 18 quoted lines

> > On Tue, Jan 07, 2014 at 06:58:50PM -0500, Jeff King wrote:
> >
> >> +			if (flags & DO_FOR_EACH_NO_RECURSE) {
> >> +				struct ref_dir *subdir = get_ref_dir(entry);
> >> +				sort_ref_dir(subdir);
> >> +				retval = do_for_each_entry_in_dir(subdir, 0,
> >
> > Obviously this is totally wrong and inverts the point of the flag. And
> > causes something like half of the test suite to fail.
> >
> > Michael was nice enough to point it out to me off-list, but well, I have
> > to face the brown paper bag at some point. :) In my defense, it was a
> > last minute refactor before going to dinner. That is what I get for
> > rushing out the series.
> 
> And perhaps a bad naming that calls for double-negation in the
> normal cases, which might have been less likely to happen it the new
> flag were called "onelevel only" or something, perhaps?

That may be a nicer name, but it was not the problem here. The problem here is that I wrote:

  if (flags & DO_FOR_EACH_NO_RECURSE == 0)

to avoid the extra layer of parentheses, but of course that doesn't work. And then when I switched it back, I screwed up the reversion.

I think the nicest way to write it would be to avoid negation at all, as:

  if (flags & DO_FOR_EACH_RECURSE) {
     ... do the recursion ...

but that means flipping the default, requiring us to set the flag explicitly in the existing callers (though there really aren't that many).

-Peff