Hi, Derrick Stolee wrote: > On 6/3/2020 1:16 AM, Taylor Blau wrote: >> * Keep the shallow bit sticky, at least for fetch.writeCommitGraph >> (i.e., pretend as if fetch.writecommitgraph=0 in the case of >> '--unshallow'). > > I'm in favor of this option, if possible. Anything that alters the > commit history in-memory at any point in the Git process is unsafe to > combine with a commit-graph read _or_ write. I'm sorry that the guards > in commit_graph_compatible() are not enough here. As described in [1], I agree --- this kind of "dirty bit" is a good option and seems like the right thing to do here. And I'm glad you said read _or_ write here. I hadn't realized that this check already applies for reads, and I'm very happy it does. [...] >> * Dump the object cache upon un-shallowing, forcing us to re-discover >> the parents when they are no longer hidden behind a graft. >> >> The latter seems like the most complete feasible fix. The former should >> work fine to address this case, but I wonder if there are other >> call-sites that are affected by this behavior. My hunch is that this is >> a unique case, since it requires going from shallow to unshallow in the >> same process. > > The latter would solve issues that could arise outside of the commit-graph > space. But it also presents an opportunity for another gap if someone edits > the shallow logic without putting in the proper guards. This, however, I don't agree with. I'm a strong believer in having clear invariants --- without them, code can only be understood empirically, and https://ieeexplore.ieee.org/document/9068304 describes how painful of a world that can be. So because shallow is specifically about changing the set of parents in objects used for traversal, I want to make sure that we as reviewers will push back on any potential other new meaning of shallow. *If* we had a safe way to invalidate the object cache, it would be sufficient and would be the right thing to do. As described in [1], we unfortunately don't have such a thing. Okay, that's all an aside. Now for the actual reason I'm replying: I had been wondering why this wasn't caught at read time, but of course --unshallow clears away the shallows so there was no way for that to happen. So what I wonder is, why isn't this caught by fsck? Can "git fsck" run "git commit-graph verify" automatically and include its result as part of what it reports? Thanks, Jonathan [1] https://lore.kernel.org/git/20200603205151.GC253041@xxxxxxxxxx/
