Re: [PATCH v4 28/28] trailer: introduce "template" term for readability
- From
Christian Couder <christian.couder@gmail.com>
- Date
- Feb 12, 2024, 23:40 UTC
- Message-ID
- <CAP8UFD01SPF8bqAb2Kn-q7SfX9cYiU_tpDSZz1CnEFbUqQpXYA@mail.gmail.com>
- In-Reply-To
- <14927038d855020f9ae7594ad5cc646257613cc1.1707196348.git.gitgitgadget@gmail.com>
On Tue, Feb 6, 2024 at 6:12 AM Linus Arver via GitGitGadget <gitgitgadget@gmail.com> wrote:
Show 40 quoted lines
> > 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.
Show 21 quoted lines
> 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.