On Tue, Feb 01 2022, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:
>
>> Provide a new hash_object_file_oideq() for those callers of
>> check_object_signature() that don't care about its streaming
>> interface. I.e. at the start of that function we do:
>>
>> if (map) {
>> hash_object_file(r->hash_algo, map, size, type, real_oid);
>> return !oideq(oid, real_oid) ? -1 : 0;
>> }
>>
>> These callers always provide a "map" (or "buf[fer]"). Let's have them
>> call this simpler hash_object_file_oideq() function instead.
>>
>> None of them use a non-NULL "real_oid" argument, but let's provide it
>> like check_object_signature() did. This'll make it easy to have these
>> emit better error messages in the future as was done in
>> 96e41f58fe1 (fsck: report invalid object type-path combinations,
>> 2021-10-01), i.e. the die() calls here can emit not only the OID we
>> expected, but also what we got.
>
> This has a potential to moving us in the wrong direction when made
> to code paths that currently fully slurp an object in-core but may
> want to shrink the memory footprint by using streaming API more.
>
> Having said that, I think all of these want to also use the in-core
> version of hte object data (mostly to write them out), and they need
> more work to move them in the other direction, so I'd say it is OK
> to introduce the new helper to simplify these callsites.
Yes, I'm avoiding any value-judgement on whether the callers that use
these APIs should or shouldn't be using something else.
But I think as the end-state shows splitting apart the two very
different API users that do and don't provide the "map" parameter makes
sense. We're effectively calling two different functions in all but name
before this change.
