Re: [bug] git checkout lies about number of ahead commits when switching from detached HEAD

From: Jeff King <peff@peff.net>
Date: Mar 21, 2011, 10:35 UTC
Message-ID: <20110321103549.GA16334@sigill.intra.peff.net>
In-Reply-To: <7vd3llwrah.fsf@alter.siamese.dyndns.org>

On Sun, Mar 20, 2011 at 12:00:38PM -0700, Junio C Hamano wrote:

Show 13 quoted lines

> "Let the one that has more information to do the clearing" is not about
> performance but about correctness and reusability potential.
> 
> When you insert a new traversal B in the existing codeflow before another
> traversal C, B knows not just which commits it started from (hence knows
> which commits were marked by it), but more importantly it also knows what
> mark bits were potentially modified. If the existing codeflow had another
> traversal A before you added B, and C took the marks A left on the objects
> into account while doing its work, the only sensible clean-up is to clear
> marks B touched (and no other marks) immediately after B, and we obviously
> do not want C (and any later traversals in other codepaths that we may
> later want to insert B) to have too much knowledge of which marks may have
> been clobbered by B.

I see what you are saying, but from a software engineering perspective, I doubt it works very well in practice. The commit flags are basically global variables. So imagine you have two traversals, A and C, and C cares about some intermediate result from A. You want to add a new one, B, in between the two, and have it clean up after itself. This is going to work only if one of the two is the case:

  1. B does not use any of the same marks as A. Otherwise, it will end
     up clearing part of A's result during cleanup (not to mention that
     it may get a bogus result because of what was left from A).

  2. B works on a totally disjoint set of history from A and C.

I don't think (1) is that realistic in the general case. Sure, you might only want to use TMP_MARK. But the revision walker behind the scenes is using SEEN and UNINTERESTING, which is going to bring you into conflict.

For (2), there are cases where it works. In this recent bug, for example, it would sometimes produce the correct results depending on the exact traversal done in the orphan-warning (e.g., if you were exploring way back in history near a tag, the traversal wouldn't come near the commits needed to get the tracking info for the new branch).

But you can only know that it's a reasonable thing to do if you manually analyze A, B, and C as a set, and even then only if you can know pretty specifically what the inputs are. So there's no modularity, and in any reasonably complex case you are not going to have enough control of the inputs to be sure it isn't buggy.

So in my mind, the only way to keep sanity is to never insert a "B" between an A and C who care about each other's results. And unless you are specifically depending on a previous walk, each walk will want all marks cleared (whether it's because the previous walker cleaned up, or because we are clearing them before starting).

This whole global marks thing is really pretty nasty when you come right down to it. I know why we do it; it's faster to dereference the pointer rather than looking up the flags in some external per-revision-walker hash table. But I wonder if we could come up with something else close to it in speed that would allow per-walker flags.

We allocate commits from a custom allocator pool. I wonder if we could use the offset from the commit pointer to the pool base to assign an index to each commit, and then use that index to access an array of flags in struct rev_info. There are a few problems:

  1. Our allocator has many pools, and we don't know which one is the
     right base. So I don't think the offset thing works, and we would
     be stuck assigning some index into each allocated object.

  2. We'd have to grow the rev_info array dynamically. The growth itself
     is probably not a huge deal, as its amortized constant effort. But
     on each flag access we'd have to do a bounds-check. I don't know if
     that would be measurable or not.

Show 8 quoted lines

> > So how about this?
> >
> >   [1/3]: checkout: add basic tests for detached-orphan warning
> >   [2/3]: checkout: clear commit marks after detached-orphan check
> >   [3/3]: checkout: tweak detached-orphan warning format
> 
> Looks very sensible, except for the clear_marks(-1) that clears everything
> I have a slight doubt about.

I can change it to UNINTERESTING. As far as I can tell, that is the only one set by prepare_revision_walk. It just felt like a leaky abstraction for this code to have to know which flags might get set by the underlying code. But obviously callers do need to know and care which flags might be set and when.

-Peff