On Tue, Oct 12 2021, Glen Choo wrote: > Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes: > >> ...how isn't disabling those t3800-mktag.sh tests just plasting over >> corruption that we're noticing because of your changes to (rightly) fix >> the bug where "fsck" wasn't checking the graph at all? >> >> IOW haven't we just found exactly the sort of bug that >> "GIT_TEST_COMMIT_GRAPH" is put in place to find for us, but now instead >> of fixing it we're hiding it? >> >> 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. >> >> If you don't run the one test that fails (which is split up into 3 >> individual pieces) there's still 143 other tests that are run, all of >> those presumably benefit from finding future bugs with >> GIT_TEST_COMMIT_GRAPH=true, particularly since the test file seems to >> just have turned up one just now... > > I think this falls on my shoulders. I assumed that the failures were > expected behavior, not bugs. You are right that we shouldn't be > plastering over bugs. > > I'll have to ask for help here because I don't know enough about mktag > to distinguish between 'expected' and 'unexpected' failures. The best I > can do is to add GIT_TEST_COMMIT_GRAPH=0 + NEEDSWORK for the failing > tests. But if that's good enough for now, I'll just do that :) I don't think your series needs to be tasked with fixing a generic issue in the commit-graph you stumbled across. So for your series I'd think the only required fix would be to narrowlry skip *just* the broken tests, not the entire test file. But in this case (see http://lore.kernel.org/git/87pms8hq4s.fsf@xxxxxxxxxxxxxxxxxxx) the fix seems rather trivial to me. I.e. just adding one line to builtin/commit-graph.c, so perhaps you can see if that works for you and such a "this bug was missed because this mode didn't work" fix could be part of a re-roll of yours?
