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@xxxxxxxxxx> > Helped-by: Taylor Blau <me@xxxxxxxxxxxx> > Signed-off-by: Jonathan Nieder <jrnieder@xxxxxxxxx> > --- > 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
