Re: [PATCH v4 0/3] Use default values from settings instead of config

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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 :)