Re: [PATCH] Always check the return value of `repo_read_object_file()`

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 6, 2024, 18:42 UTC
Message-ID: <xmqq1q9pige1.fsf@gitster.g>
In-Reply-To: <ZcHW_bc6N5umk2G4@tanuki>

Patrick Steinhardt <ps@pks.im> writes:

Show 14 quoted lines

>>  			mmfile[i].ptr = repo_read_object_file(the_repository,
>>  							      &ce->oid, &type,
>>  							      &size);
>> +			if (!mmfile[i].ptr)
>> +				die(_("unable to read %s"),
>> +				    oid_to_hex(&ce->oid));
>>  			mmfile[i].size = size;
>>  		}
>>  	}
>
> A few lines below this we check whether `mmfile[i].ptr` is `NULL` and
> replace it with the empty string if so. So this patch here is basically
> a change in behaviour where we now die instead of falling back to the
> empty value.

I think that one is trying to cope with cases where we genuinely do not have all three variants, not "we thought we had this variant so we tried to read it into mmfile[i].{ptr,size}, but it turns out that the object name we had was bad". So the fallback code for an entirely different case was masking the breakage the above hunk fixes, and this being "rerere", it is better to be cautious than sorry.

Thanks for reading the original code carefully.