From: Ævar Arnfjörð Bjarmason Date: Thu, 31 Mar 2022 12:42:34 GMT Subject: Re: [PATCH v12 7/8] unpack-objects: refactor away unpack_non_delta_entry() Message-ID: <220331.86wngap9vo.gmgdl@evledraar.gmail.com> In-Reply-To: <2f98c63d-f2c9-26fe-cfd3-9eed6b79047a@web.de> 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/patch-v10-6.6-6a70e49a346-20220204T135538Z-avarab@gmail.com/ 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 >> --- >> 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.