On Thu, Jan 09, 2014 at 09:51:24AM -0800, Junio C Hamano wrote:
> > 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html