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

Commit Message

Linus Arver Feb. 16, 2024, 11:09 p.m. UTC

From: Linus Arver <[e-mail]>

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.

In a future patch, we'll change format_trailer_info() to use the parsed
trailer_item objects instead of the string array.

Signed-off-by: Linus Arver <[e-mail]>
---
 trailer.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Christian Couder Feb. 19, 2024, 9:32 p.m. UTC | #1

On Sat, Feb 17, 2024 at 12:09 AM Linus Arver via GitGitGadget
<[e-mail]> wrote:
>
> From: Linus Arver <[e-mail]>
>
> 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?

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.

> 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.

Linus Arver Feb. 29, 2024, 11 p.m. UTC | #2

Christian Couder <[e-mail]> writes:

> On Sat, Feb 17, 2024 at 12:09 AM Linus Arver via GitGitGadget
> <[e-mail]> wrote:
>>
>> From: Linus Arver <[e-mail]>
>>
>> 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. ;)

> 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).

>> 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).

Patch

diff --git a/trailer.c b/trailer.c
index e92d0154d90..e6665c99cc3 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1140,9 +1140,11 @@  void format_trailers_from_commit(const struct process_trailer_options *opts,
 				 const char *msg,
 				 struct strbuf *out)
 {
+	LIST_HEAD(trailers);
 	struct trailer_info info;
 
-	trailer_info_get(opts, msg, &info);
+	parse_trailers(opts, &info, msg, &trailers);
+
 	/* If we want the whole block untouched, we can take the fast path. */
 	if (!opts->only_trailers && !opts->unfold && !opts->filter &&
 	    !opts->separator && !opts->key_only && !opts->value_only &&
@@ -1152,6 +1154,7 @@  void format_trailers_from_commit(const struct process_trailer_options *opts,
 	} else
 		format_trailer_info(opts, &info, out);
 
+	free_trailers(&trailers);
 	trailer_info_release(&info);
 }