Subject: Re: [PATCH v12 7/8] unpack-objects: refactor away unpack_non_delta_entry()

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Thu, 31 Mar 2022 14:42:34 +0200

On Wed, Mar 30 2022, René Scharfe wrote:

> Am 29.03.22 um 15:56 schrieb Ævar Arnfjörð Bjarmason:
>> The unpack_one() function will call either a non-trivial
>> unpack_delta_entry() or a trivial unpack_non_delta_entry(). Let's
>> inline the latter in the only caller.
>>
>> Since 21666f1aae4 (convert object type handling from a string to a
>> number, 2007-02-26) the unpack_non_delta_entry() function has been
>> rather trivial, and in a preceding commit the "dry_run" condition it
>> was handling went away.
>>
>> This is not done as an optimization, as the compiler will easily
>> discover that it can do the same, rather this makes a subsequent
>> commit easier to reason about.
>
> How exactly does inlining the function make the next patch easier to
> understand or discuss?  Plugging in the stream_blob() call to handle the
> big blobs looks the same with or without the unpack_non_delta_entry()
> call to me.

The earlier version of it without this prep cleanup can be seen at
https://lore.kernel.org/git/[email-masked]/

So yes, this could be skipped, but I tought with this step it was easier
to understand.

We previously had to change "void *buf = get_data(size);" in the
function to just "void *buf", and do the assignment after the condition
that's being checked here.

I think it's also more obvious in terms of control flow if we're
checking OBJ_BLOB here to not call a function which has a special-case
just for OBJ_BLOB, we can just do that here instead.

>> As it'll be handling "OBJ_BLOB" in a
>> special manner let's re-arrange that "case" in preparation for that
>> change.
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <[email-masked]>
>> ---
>>  builtin/unpack-objects.c | 18 +++++++-----------
>>  1 file changed, 7 insertions(+), 11 deletions(-)
>
> Reducing the number of lines can be an advantage. *shrug*

There was also the (admittedly rather small) knock-on-effect on
8/8. Before this it was 8 lines added / 1 removed when it came to the
code impacted by this change, now it's a 5 added/0 removed in the below
"switch".

So I think it's worth keeping.