On Tue, Oct 22, 2019 at 05:28:55PM +0000, Derrick Stolee via GitGitGadget wrote:

However, the UNINTERESTING flag is used in lots of places in the
codebase. This flag usually means some barrier to stop a commit walk,
such as in revision-walking to compare histories. It is not often
cleared after the walk completes because the starting points of those
walks do not have the UNINTERESTING flag, and clear_commit_marks() would
stop immediately.

Oof. Nicely explained, and your fix makes sense.

The global-ness of revision flags always makes me nervous about doing
more things in-process (this isn't the first such bug we've had).

I have a dream of converting most uses of flags into using a
commit-slab. That provides cheap access to an auxiliary structure, so
each traversal, etc, could keep its own flag structure. I'm not sure if
it would have a performance impact, though. Even though it's O(1), it is
an indirect lookup, which could have some memory-access impact (though
my guess is it would be lost in the noise).

One of the sticking points is that all object types, not just commits,
use flags. But we only assign slab ids to commits. I noticed recently
that "struct object" has quite a few spare bits in it these days,
because the switch to a 32-byte oid means 64-bit machines now have an
extra 4 bytes of padding. I wonder if we could use that to store an
index field.

Anyway, that's getting far off the topic; clearly we need a fix in the
meantime, and what you have here looks good to me.

I tested running clear_commit_marks_many() to clear the UNINTERESTING
flag inside close_reachable(), but the tips did not have the flag, so
that did nothing.

Another option would be clear_object_flags(), which just walks all of
the in-memory structs. Your REACHABLE solution is cheaper, though.

Instead, I finally arrived on the conclusion that I should use a flag
that is not used in any other part of the code. In commit-reach.c, a
number of flags were defined for commit walk algorithms. The REACHABLE
flag seemed like it made the most sense, and it seems it was not
actually used in the file.

Yeah, being able to remove it from commit-reach.c surprised me for a
moment. To further add to the confusion, builtin/fsck.c has its own
REACHABLE flag (with a different bit and a totally different purpose). I
don't think there's any practical problem there, though.

I have failed to produce a test using the file:// protocol that
demonstrates this bug.

Hmm, from the description, it sounds like it should be easy. I might
poke at it a bit.

-Peff