On 10/9/2020 2:28 PM, Taylor Blau wrote: > On Fri, Oct 09, 2020 at 01:55:06PM -0400, Jeff King wrote: >> On Fri, Oct 09, 2020 at 01:46:07PM -0400, Derrick Stolee wrote: >> >>>> Can you reproduce it if you do >>>> >>>> git config core.commitGraph false >>>> git config fetch.writeCommitGraph true >>>> ? >>> >>> I _can_ repro it in this case! I think there must be something >>> very interesting going on where the commit-graph is parsed in >>> _some_ places, but not in others. This is something that I can >>> really start to dig into. >> >> Here's a much more minimal reproduction: >> >> git init repo >> cd repo >> >> git commit --allow-empty -m one >> git rev-parse HEAD | >> git -c core.commitGraph=false \ >> commit-graph write --split=no-merge --stdin-commits >> git rev-parse HEAD | >> git -c core.commitGraph=false \ >> commit-graph write --split=no-merge --stdin-commits >> >> git commit --allow-empty -m two >> git rev-parse HEAD | >> git commit-graph write --split --stdin-commits >> >> The final write will die() with the "unexpected duplicate" message. > > Makes sense; the second commit-graph write won't know that 'one' is > already in the graph because 'core.commitGraph' prevents > 'prepare_commit_graph()' from actually loading the graph (actually > loading the graph would be enough to stop the second write from > occurring at all.) Right. We aren't parsing from the commit-graph, so we don't see that these commits are already in the file. > I'm of two minds about what we could be doing here: > > - On the one hand we could be ignoring `core.commitGraph` setting > during commit-graph writes and forcibly loading the commit-graph > anyway. > > - But on the other hand, writing a commit graph with `core.commitGraph` set > to false makes no sense. So, I'd almost rather have us die() > immediately if core.commitGraph is set to false. I agree that we should just give up, but die() would not be correct. We should just "return 0", possibly with a warning. > I think I'd advocate for the latter, along with Stolee's patch to not > die in the case of duplicate commits in multiple layers of the graph. If we agree that writing a commit-graph makes no sense if the feature is disabled, then I can include a patch that has a test similar to Peff's and that change. Thanks, -Stolee
