Re: [PATCH] experimental: default to fetch.writeCommitGraph=false

[Derrick Stolee <stolee@gmail.com>]

On 7/7/2020 2:20 AM, Jonathan Nieder wrote:
> The fetch.writeCommitGraph feature makes fetches write out a commit
> graph file for the newly downloaded pack on fetch.  This improves the
> performance of various commands that would perform a revision walk and
> eventually ought to be the default for everyone.  To prepare for that
> future, it's enabled by default for users that set
> feature.experimental=true to experience such future defaults.
> 
> Alas, for --unshallow fetches from a shallow clone it runs into a
> snag: by the time Git has fetched the new objects and is writing a
> commit graph, it has performed a revision walk and r->parsed_objects
> contains information about the shallow boundary from *before* the
> fetch.  The commit graph writing code is careful to avoid writing a
> commit graph file in shallow repositories, but the new state is not
> shallow, and the result is that from that point on, commands like "git
> log" make use of a newly written commit graph file representing a
> fictional history with the old shallow boundary.
> 
> We could fix this by making the commit graph writing code more careful
> to avoid writing a commit graph that could have used any grafts or
> shallow state, but it is possible that there are other pieces of
> mutated state that fetch's commit graph writing code may be relying
> on.  So disable it in the feature.experimental configuration.
> 
> Google developers have been running in this configuration (by setting
> fetch.writeCommitGraph=false in the system config) to work around this
> bug since it was discovered in April.  Once the fix lands, we'll
> enable fetch.writeCommitGraph=true again to give it some early testing
> before rolling out to a wider audience.
> 
> In other words:
> 
> - this patch only affects behavior with feature.experimental=true
> 
> - it makes feature.experimental match the configuration Google has
>   been using for the last few months, meaning it would leave users in
>   a better tested state than without it
> 
> - this should improve testing for other features guarded by
>   feature.experimental, by making feature.experimental safer to use
> 
> Reported-by: Jay Conrod <jayconrod@google.com>
> Helped-by: Taylor Blau <me@ttaylorr.com>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> I realize this is late to send.  That said, as described above, I
> think it's a good way to buy time by minimizing user exposure to
> fetch.writeCommitGraph=true until a fix for it is well cooked.

While it would certainly be better to fix the commit_graph_compatible()
method, I understand that that is a more complicated problem.

> In other words, I'd like to see this patch in Git 2.28-rc0.

This patch is 100% correct.

Normally, I would say "this is experimental, and a user can always
disable fetch.writeCommitGraph manually if they are running into this."
This is especially true because unshallowing a repo is (probably)
a rare operation. At least, I expect that very few users actually do
it, and those who do are expert users.

But, it is best to reduce user pain, even in rare cases like this.

Further, I hope to submit new maintenance tasks soon which can
replace fetch.writeCommitGraph.

Thanks,
-Stolee