Re: False positive from orphaned_commit_warning() ?

From: Jeff King <peff@peff.net>
Date: Jul 25, 2012, 22:31 UTC
Message-ID: <20120725223159.GA31134@sigill.intra.peff.net>
In-Reply-To: <7v629bbio9.fsf@alter.siamese.dyndns.org>

On Wed, Jul 25, 2012 at 03:05:42PM -0700, Junio C Hamano wrote:

Show 6 quoted lines

> > I've just been hunting the same bug and came up with the same answer.
> > Here's a commit message. Feel free to apply or steal text for your
> > commit.
> 
> Heh, let's try not to waste duplicated efforts by being silent next
> time, OK?  Winning such a race by 5 minutes does not buy us much.

I usually do, but this bug was surprisingly easy to find once I bisected. I don't think it took more than 5 minutes total. :)

> I wish we had some type safe way to say "This uint and the other
> uint are to hold different kinds of flag bits; do not mix them by
> bitwise operators".

I believe that bitwise operations are defined for enums, but I might be misremembering my C89.

Show 11 quoted lines

> > However, it confused the "flags" parameter to the
> > each_ref_fn clalback, which is about the flags we found
> > while looking up the ref (e.g., REF_ISSYMREF) with the
> > object flag (UNINTERESTING), leading to unpredictable
> 
> s/UNINTERESTING/SEEN/; I think.
> 
> What was happening was that the remotes/origin/HEAD symref happened
> to point at the same commit as "master", and ^master that was in the
> pending array was not transferred to the commit list used by the
> revision traversal.

Yeah, I that was my guess, too, but I didn't investigate exactly which bits were twiddled. By mentioning UNINTERESTING, I meant "this is the flag we actually _care_ about, but we are setting other random ones depending on the ref flags".

Show 6 quoted lines

> What's interesting still is that
> 
> 	git checkout master~
>         git checkout master
> 
> does not exhibit this problem in the same repository.

Perhaps we still look at and mark the parents of a SEEN commit during our traversal (but not any further). I suspect it is that mark_parents_uninteresting does so, but does not bother to parse the parent. I didn't check carefully. It may be that we have an over-conservative off-by-one at the boundaries of our traversal in some cases, but I doubt it is worth the effort to optimize.

-Peff