Jeff King [off-list ref] writes:
For the case of 2 traversals, I suspect that clearing everything between
is not so different from clearing from the tips, since most everything
parsed was probably from the first traversal. But as we lib-ify more, we
may end up with more and more traversals in a single program, so it's
probably better to go the more efficient route from the beginning.
"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.
Of course, C could be coded defensively to clear marks other than what it
cares about (namely, the ones other than what A would have left and the
ones that would affect itself e.g. UNINTERESTING), and whoever inserts B
into existing codeflow needs to make sure that B does not stomp on marks
deliberately left by earlier traversals like A for later users like C
regardless of how the latter is coded.
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 think 3/3 was way overdue and it was my fault. Thanks for cleaning up my
mess.
On Sun, Mar 20, 2011 at 12:00:38PM -0700, Junio C Hamano wrote:
"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.
quoted
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