On Fri, Oct 09, 2020 at 02:33:02PM -0400, Derrick Stolee wrote: > > 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. OK, I feel even better knowing that you and I both agree on the cause of this buglet ;-). This also makes me think that this has probably existed since the beginning of commit-graphs, and that it only became easier to tickle in recent releases with things like '--split=no-merge'. > > - 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. Yeah; that sounds much better. > > 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. Sounds good. I'm certainly on board, but I want to hear what others think, too. I thought that we had a configuration variable to control whether or not we write changed-path Bloom filters, so I was going to ask about what we should do if that was set to false, and the caller passed '--changed-paths'. But, I guess that my memory was wrong, since I couldn't find such a variable to begin with (we _do_ have 'commitGraph.readChangedPaths', but since that only controls reading no additional special care has to be taken). Thanks for working on this. > Thanks, > -Stolee Thanks, Taylor
