Re: [PATCH v2 2/2] shallow.c: use '{commit,rollback}_shallow_file'

Hi,

Derrick Stolee wrote:

On 6/3/2020 1:16 AM, Taylor Blau wrote:

quoted

  * 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.

[...]

quoted

  * 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@google.com/ (local)