Re: [PATCH v2 2/6] commit-graph: always parse before commit_graph_data_at()

[Taylor Blau <me@ttaylorr.com>]

On Wed, Feb 03, 2021 at 10:41:08AM -0800, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > Thinking aloud, I'm not totally sure that we should be exposing "git
> > commit-graph clear" to users. The only time that you'd want to run this
> > is if you were trying to remove a corrupted commit-graph, so I'd rather
> > see guidance on how to do that safely show up in
> > Documentation/git-commit-graph.txt.
> >
> > On the other hand, now I'm encouraging running "rm -fr
> > $GIT_DIR/objects/info/commit-graph*", which feels dangerous.
>
> True.
>
> As this is, like pack .idx file, supposed to be "precomputed cached
> data that can be fully recreated using primary information" [*], I
> am perfectly fine to say "commit-graph may have unexplored corners,
> and when you hit a BUG(), you can safely use 'commit-graph clear'
> and recreate it from scratch, or operate without it if you feel you
> do not yet want to trust your data to it for now."  Giving safer and
> easier way to opt out for those who need to get today's release
> done, with enough performance incentive to re-enable it when the
> crunch is over, would be an honest thing to do, I would think.
>
> 	Side note: the index file also used to be considered to hold
> 	such cached data, that can be recreated from the working
> 	tree data and the tip commit.  We no longer treat it that
> 	way, though.
>
> > Somewhere in the middle would be something like:
> >
> >   git -c core.commitGraph=false commit-graph write --reachable
>
> I am a bit worried about the thinking along this line, because it
> gives the users an impression that there is no escaping from
> trusting commit-graph---the one that was created from scratch is
> bug-free and they only need to be cautious about incrementals.
>
> But (1) we do not know that, and (2) it is an unconvincing message
> to somebody who just got hit by a BUG().

This is a convincing counter-point to my proposal. Yeah, I agree that we
shouldn't be advertising that commit-graph is completely trustworthy.

> > which would disable reading existing commit-graph files. Since
> > 85102ac71b (commit-graph: don't write commit-graph when disabled,
> > 2020-10-09), that causes us to exit immediately.
>
> Meaning the three command sequence
>
> 	git commit-graph clear
> 	git commit-graph write --reachable
>         git config core.commitGraph false
>
> to force a clean build of a graph and forbid further updates until
> the bug is squashed???  But should't core.commitGraph forbid reading
> and using the data in the existing files, too?  In which case, shouldn't
> it be equivalent to "git commit-graph clear"?

I think we may be saying the same thing. I was suggesting that if we
reverted 85102ac71b, that 'git -c core.commitGraph=false commit-graph
write ...' would rewrite your commit-graph from scratch (without opening
up existing ones and propagating corruption).

So I was saying that that *would* be a viable "git commit-grpah clear"
(if 85102ac71b were reverted).

Thanks,
Taylor