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

From: Taylor Blau <me@ttaylorr.com>
Date: Feb 3, 2021, 15:34 UTC
Message-ID: <YBrCli7AR/XrB3Pr@nand.local>
In-Reply-To: <6dc1520f-8130-75e1-6617-67b54cb03933@gmail.com>

On Tue, Feb 02, 2021 at 10:07:32PM -0500, Derrick Stolee wrote:

Show 12 quoted lines

> > Can we package this as something more user-friendly?  E.g.
> >
> > 	git commit-graph clear
> > 	git commit-graph write --reachable
> >
> > If that makes sense to you, I'm happy to send a patch (or to review
> > one if someone else gets to it first).  I'm mostly asking to find out
> > whether this matches your idea of what the UI should be like.
>
> 'clear' is probably fine. I was thinking it might be good to have
> an option to the 'write' subcommand to clear the existing data, but
> it's probably better as separate steps.

Wouldn't 'git commit-graph write --split=replace --reachable' do the same thing? I know that you changed some of the spots where we load an existing commit graph, so my claim might be out-of-date, but I'm pretty sure that this would get you out of a broken state.

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.

Somewhere in the middle would be something like:

  git -c core.commitGraph=false commit-graph write --reachable

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.

I think that reverting that patch and advertising setting 'core.commitGraph=false' in the documentation makes the most sense.

Thanks, Taylor