On 10/13/2021 11:57 AM, Ævar Arnfjörð Bjarmason wrote: > > On Wed, Oct 13 2021, Derrick Stolee wrote: > >> On 10/12/2021 4:34 PM, Ævar Arnfjörð Bjarmason wrote:... >>> If I comment yout your addition of GIT_TEST_COMMIT_GRAPH=0 in that file >>> I see that we fail N number of tests, but all of them are actually >>> fallout of just this test: >>> >>> git replace $head_parent $head && >>> git replace -f $tree $blob >>> >>> I.e. we've created a replacement object replacing a tree with a blob, as >>> part of tests I added to test how mktag handles those sorts of weird >>> edge cases. >>> >>> This then causes the graph verify code to throw a hissy fit with: >>> >>> root tree OID for commit 0ddfaf193ff13d6ab39b7cbd9eed645e3ee2f050 in >>> commit-graph is da5497437fd67ca928333aab79c4b4b55036ea66 != >>> 0fbca9850869684085d654f9e1380c9780802570 >>> >>> I.e. when we wrote the graph we somehow didn't notice that the root tree >>> node we wrote is to an object that's not actually a tree? Isn't this a >>> bug where some part of the commit graph writing should be doing its own >>> extended OID lookup that's replacement-object aware, it didn't, and we >>> wrote a corrupt graph as a result? >>> >>> If there is a legitimate reason why we're not just hiding a bug we've >>> turned up with these fixes let's disable that one test, not the entire >>> test file. >> >> The commit-graph should be disabled if replace-objects are enabled. If >> there is a bug being introduced here it is because the commit-graph is >> being checked during fsck even though it would never be read when the >> replace-objects exist. >> >> Thanks, >> -Stolee > > Thanks, isn't the obvious fix for this to extend your d6538246d3d > (commit-graph: not compatible with replace objects, 2018-08-20) to do > "read_replace_refs = 0;" in graph_verify()? That works for me on this > case. Ignoring the replace refs while verifying will allow you to verify the on-disk commit-graph file without issue. > I.e. if we ignore replace refs when writing the graph, and we don't use > it if there are any due to commit_graph_compatible(), why would we > consider them when verifying it? I generally consider any interaction with the commit-graph while replace refs are enabled to be the bug. We don't read the commit-graph when replace refs are on, so why are we verifying it? But, I understand the desire to check the on-disk content even though it is not being used, so disabling replace refs to do the verify should be sufficient. Thanks, -Stolee
