Re: [PATCH v4 28/28] trailer: introduce "template" term for readability

[Christian Couder <christian.couder@gmail.com>]

On Tue, Feb 6, 2024 at 6:12 AM Linus Arver via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Linus Arver <linusa@google.com>
>
> The term "arg_item" is ambiguous because we use it to hold data for
>
>   (1) trailers specified as command line arguments (in
>       builtin/interpret-trailers.c), and
>
>   (2) trailers specified in configuration,
>
> and these are both used to ultimately insert new trailers (based
> on the contents of arg_item, acting as a kind of template) into some
> other set of existing trailers (such as those found in a trailer block
> inside a log message) that have already been parsed.
>
> Rename "arg_item" to "trailer_template". This necessitates further
> renames to make the functions that act on these templates match the data
> structures (parameters) they act on:
>
>   - [*] add_arg_to_input_list()      to apply_template_to_trailers()
>   - [*] apply_arg_if_exists()        to maybe_add_if_exists()
>   - [*] apply_arg_if_missing()       to maybe_add_if_missing()
>   -     apply_command()              to run_command_from_template()
>   - [*] apply_item_command()         to populate_template_value()
>   -     free_arg_item()              to free_template() (non-API function)
>   -     free_new_trailers()          to free_trailer_templates() (API function)
>   -     get_conf_item()              to get_or_add_template_by()
>   -     option_parse_trailer()       to option_parse_trailer_template()
>   -     parse_trailers_from_config() to parse_trailer_templates_from_config()
>   - [*] process_trailers_lists()     to apply_trailer_templates()
>   -     token_from_item()            to token_from_template()
>   -     token_matches_item           to token_matches_template
>   - [*] trailer_add_arg_item()       to add_trailer_template()
>   -     trailer_from_arg()           to trailer_from()
>   - [*] check_if_different()         (reorder parameters only)
>   - [*] find_same_and_apply_arg()    (reorder parameters only)
>
> Reorder parameters to prefer input parameters toward the beginning and
> out parameters at the end; these functions have been marked with an
> asterisk ([*]).

That's a lot of changes in a single patch.

> This removes the "arg" terminology (standing for "CLI arguments") from
> the trailer implementation, which makes sense because trailers
> themselves have nothing to do with CLI argument handling.
>
> Also note that these renames expose the previously liberal use of
> "trailer" to mean both trailers we read from the input text (trailer
> block) and trailer templates that are defined as CLI args or
> configurations. Some functions implied a single action when they could
> do two different things, so introduce words like "maybe" and "or" to
> make their behavior more explicit.
>
> In summary this patch renames and reorders parameters for readability,
> without any behavioral change. We don't rename
> find_same_and_apply_arg(), because it will be refactored soon.
>
> For parse_trailers_from_config() (renamed to
> parse_trailer_templates_from_config()), add a NEEDSWORK discussion about
> how the deprecated trailer.*.command configuration option is oddly more
> featureful than trailer.*.cmd (if we were to remove support for
> trailer.*.command, users would not be able to replicate the behavior
> with trailer.*.cmd and would lose out on functionality).

This change could be in a separate patch. Also there were discussions
when trailer.*.command was deprecated and trailer.*.cmd introduced. I
think it might be useful to talk about them in the commit message of
the separate patch introducing the NEEDSWORK.