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

[Jonathan Nieder <jrnieder@gmail.com>]

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@google.com/