Re: [PATCH v5 9/9] format_trailers_from_commit(): indirectly call trailer_info_get()

From: Linus Arver <linusa@google.com>
Date: Feb 29, 2024, 23:00 UTC
Message-ID: <owlyo7by511o.fsf@fine.c.googlers.com>
In-Reply-To: <CAP8UFD0JV8VEC-MDu86Mzrya9G7JBZaP2vXjFKwcKddEkh=y5g@mail.gmail.com>

Christian Couder <christian.couder@gmail.com> writes:

Show 14 quoted lines

> On Sat, Feb 17, 2024 at 12:09 AM Linus Arver via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> From: Linus Arver <linusa@google.com>
>>
>> This is another preparatory refactor to unify the trailer formatters.
>>
>> Instead of calling trailer_info_get() directly, call parse_trailers()
>> which already calls trailer_info_get(). This change is a NOP because
>> format_trailer_info() only looks at the "trailers" string array, not the
>> trailer_item objects which parse_trailers() populates.
>
> Is the extra processing done by parse_trailers() compared to
> trailer_info_get() impacting performance?

I was going to answer this now but I see that you've already reached the same conclusion as me further below. ;)

Show 7 quoted lines

> Also when looking only at the patch, it's a bit difficult to
> understand that the "trailers" string array is the `char **trailers`
> field in `struct trailer_info` and that the trailer_item objects are
> the elements of the `struct list_head *head` linked list. It could
> also be confusing because the patch is adding a new 'trailers'
> variable with `LIST_HEAD(trailers);`. So a few more details could help
> understand what's going on.

Makes sense. Admittedly, this is one thing that did bother me at some point but which I forgot about as I got more familiar with my own patch.

I will avoid shadowing the "trailers" word (and maybe at least add a suffix or prefix to disambiguate it).

Show 5 quoted lines

>> In a future patch, we'll change format_trailer_info() to use the parsed
>> trailer_item objects instead of the string array.
>
> Ok, so I guess the possible performance issue would disappear then, as
> populating the trailer_item objects will be useful.

Yep, exactly. In the larger series (some 20? 30?) commits down the road in my local tree, I have it so that we remove `char **trailers` entirely (because we should be using the (smarter) trailer_item objects, not raw strings, where possible).