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
