Re: [PATCH] Always check the return value of `repo_read_object_file()`
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 6, 2024, 18:36 UTC
- Message-ID
- <xmqq8r3xign7.fsf@gitster.g>
- In-Reply-To
- <CAOLa=ZQOALZRNqp7dDH0qDWoHwo6_3G8VgVuMbb3C20UdJ4C5A@mail.gmail.com>
Karthik Nayak <karthik.188@gmail.com> writes:
Show 19 quoted lines
> Hello,
>
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:
>> diff --git a/bisect.c b/bisect.c
>> index f1273c787d9..f75e50c3397 100644
>> --- a/bisect.c
>> +++ b/bisect.c
>> @@ -158,6 +158,9 @@ static void show_list(const char *debug, int counted, int nr,
>> const char *subject_start;
>> int subject_len;
>>
>> + if (!buf)
>> + die(_("unable to read %s"), oid_to_hex(&commit->object.oid));
>> +
>
> Nit: We know that `repo_read_object_file()` fails on corrupt objects, so
> this means that this is only happening when the object doesn't exist. I
> wonder if it makes more sense to replace "unable to read %s" which is a
> little ambiguous with something like "object %q doesn't exist".I am not sure if that is a good move in the longer run. We may "fix" the called function to return NULL to allow callers to deal with errors from object corruption better, at which time between "doesn't exist" and "unable to read", the latter becomes far closer to what actually happened (it is debatable if a corrupt thing really exists in the first place, too).
> Otherwise, the patch looks good, thanks!
I haven't read the remainder of the patch, but to me this hunk looks OK.
Thanks.