Re: [PATCH v2] sha1-file: test the error behavior of alt_odb_usable()

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Fri, Mar 29 2019, Ævar Arnfjörð Bjarmason wrote:

> On Fri, Mar 29 2019, Jeff King wrote:
>
>> On Thu, Mar 28, 2019 at 09:04:56PM +0100, Ævar Arnfjörð Bjarmason wrote:
>>
>>> Add a test for the error() case in alt_odb_usable() where an alternate
>>> directory doesn't exist. This behavior has been the same since
>>> 26125f6b9b ("detect broken alternates.", 2006-02-22), but if that
>>> error() was turned into die() the entire test suite would still pass.
>>>
>>> Perhaps we should die() in that case, but let's start by adding a test
>>> here to assert the long-standing existing behavior.
>>
>> I think if anything we might go the other direction, and downgrade the
>> error() to a warning() or even omit it entirely. It's not an error to
>> have a missing or transient alternate. Unless of course it has objects
>> you need, but then those generate their own errors.
>
> Yeah that sounds fine. FWIW it's just an "error" in the sense of being
> printed out by error(), but we proceed, so it's really a warning,
> sort-of.
>
>> I actually think in an ideal world we wouldn't say anything at all about
>> alternates which aren't present, don't appear to contain objects, etc,
>> on their own. And then when we hit an error because an object is
>> missing, only _then_ diagnose and say "hey, you have this alternate but
>> it doesn't have anything in it. Maybe that's an error?". Doing that
>> diagnosis in the error path helps in two ways:
>>
>>   - we don't have to worry about it being slow
>>
>>   - we can be a bit more loose about things that _might_ be an issue.
>>     E.g., it's not an error to point to an alternate directory that has
>>     no files in it. It might be a misconfiguration, or it might just not
>>     have any objects right now. It's hard to justify complaining about
>>     it in _every_ git command that loads alternates. But after hitting a
>>     fatal error due to a missing object, it seems like a convenient
>>     thing to mention to the user.
>>
>> I suspect that implementing it that way might be a pain, though. Even if
>> we had a convenient diagnose_missing_object() one-liner, there are
>> probably dozens of separate places it would need to be called from.
>>
>>> diff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh
>>> index 895f46bb91..d2964c57b7 100755
>>> --- a/t/t5613-info-alternate.sh
>>> +++ b/t/t5613-info-alternate.sh
>>> @@ -136,4 +136,11 @@ test_expect_success CASE_INSENSITIVE_FS 'dup finding can be case-insensitive' '
>>>  	test_cmp expect actual.alternates
>>>  '
>>>
>>> +test_expect_success 'print "error" on non-existing alternate' '
>>> +	git init --bare I &&
>>> +	echo DOES_NOT_EXIST >I/objects/info/alternates &&
>>> +	git -C I fsck 2>stderr &&
>>> +	test_i18ngrep "does not exist; check" stderr
>>> +'
>>
>> All that said, I don't really have an objection against this patch,
>> since it's just testing the current behavior. Anybody who wants to
>> change it would find it pretty easy to tweak this test, too.
>
> Yup. Just wanted to get the patch to test what we do *currently* out,
> might loop back to finishing up the rest of this.

Junio: *ping* about picking up this trivial test coverage improvement
(missed in the latest What's Cooking).