Junio C Hamano <gitster@xxxxxxxxx> writes:
> "Linus Arver via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
>
>> diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
>> index 9f0ba39b317..9a902012912 100644
>> --- a/builtin/interpret-trailers.c
>> +++ b/builtin/interpret-trailers.c
>> @@ -45,23 +45,17 @@ static int option_parse_if_missing(const struct option *opt,
>> return trailer_set_if_missing(opt->value, arg);
>> }
>>
>> -static void free_new_trailers(struct list_head *trailers)
>> -{
>> - struct list_head *pos, *tmp;
>> - struct new_trailer_item *item;
>> -
>> - list_for_each_safe(pos, tmp, trailers) {
>> - item = list_entry(pos, struct new_trailer_item, list);
>> - list_del(pos);
>> - free(item);
>> - }
>> -}
>> +static char *cl_separators;
>
> It somehow feels ugly and goes backward to depend on a new global,
> especially when you are aiming to libify more things.
Removed in the next reroll (we can just use a regular stack variable,
not a global).
> Even if you plan to later make this function callable from outside
> the context of parse_options() callback, and if you do not want "CLI
> allows '=' as well" for such new callers, we should be able to have
> a shared helper function that is the bulk of this function but takes
> additional parameter to tweak its behaviour slightly depending on
> the needs of the callers?
I'll refrain from exploring this possibility in this series (and just
make do with the non-global variable as described above) because of the
additional pending changes around trailer configuration handling in the
larger series. FWIW even in the larger series I don't think there's a
need to have such a helper function.
>> + trailer_conf_set(where, if_exists, if_missing, conf_current);
>
> I am getting an impression that the use of and the introduction of
> the new helper, mixed with movement of the responsibility, makes
> reviewing the change unnecessarily more cumbersome. An equivalent
> series with a few more steps, each of them focusing on a small
> change (like, "updating the conf_current members is done with
> assignments that appear as a pattern very often---introduce a helper
> to reduce boilerplate") would have been more enticing to reviewers.
I've broken this patch down into smaller steps (along with the other
notable patches in this series).
>> + trailer_add_arg_item(strbuf_detach(&tok, NULL),
>> + strbuf_detach(&val, NULL),
>> + conf_current,
>> + trailers);
>> + } else {
>> + struct strbuf sb = STRBUF_INIT;
>> + strbuf_addstr(&sb, arg);
>> + strbuf_trim(&sb);
>> + error(_("empty trailer token in trailer '%.*s'"),
>> + (int) sb.len, sb.buf);
>> + strbuf_release(&sb);
>> + }
>> +
>> + free(conf_current);
>> +
>> return 0;
>> }
>>
>> @@ -135,7 +148,7 @@ static void read_input_file(struct strbuf *sb, const char *file)
>> }
>>
>> static void interpret_trailers(const struct process_trailer_options *opts,
>> - struct list_head *new_trailer_head,
>> + struct list_head *arg_trailers,
>> const char *file)
>> {
>> LIST_HEAD(head);
>> @@ -144,8 +157,6 @@ static void interpret_trailers(const struct process_trailer_options *opts,
>> struct trailer_block *trailer_block;
>> FILE *outfile = stdout;
>>
>> - trailer_config_init();
>> -
>> read_input_file(&sb, file);
>>
>> if (opts->in_place)
>> @@ -162,12 +173,7 @@ static void interpret_trailers(const struct process_trailer_options *opts,
>>
>>
>> if (!opts->only_input) {
>> - LIST_HEAD(config_head);
>> - LIST_HEAD(arg_head);
>> - parse_trailers_from_config(&config_head);
>> - parse_trailers_from_command_line_args(&arg_head, new_trailer_head);
>> - list_splice(&config_head, &arg_head);
>> - process_trailers_lists(&head, &arg_head);
>> + process_trailers_lists(&head, arg_trailers);
>> }
>
> So, the bulk of the parsing is no longer responsibility of this
> function. Instead, the caller (e.g., cmd_interpret_trailers()) is
> expected to do that before they call us.
Yup. This movement of "parsing responsibility" logic should be easy to
see in the next reroll (as it will be broken out separately).
>> ...
>> git_config(git_default_config, NULL);
>> + trailer_config_init();
>> +
>> + if (!opts.only_input) {
>> + parse_trailers_from_config(&configured_trailers);
>> + }
>
> By the way, until we add more statements in this block, lose the
> unnecessary {} around it.
Oops. Done.
>> @@ -424,6 +429,25 @@ int trailer_set_if_missing(enum trailer_if_missing *item, const char *value)
>> return 0;
>> }
>>
>> +void trailer_conf_set(enum trailer_where where,
>> + enum trailer_if_exists if_exists,
>> + enum trailer_if_missing if_missing,
>> + struct trailer_conf *conf)
>> +{
>> + if (where != WHERE_DEFAULT)
>> + conf->where = where;
>> + if (if_exists != EXISTS_DEFAULT)
>> + conf->if_exists = if_exists;
>> + if (if_missing != MISSING_DEFAULT)
>> + conf->if_missing = if_missing;
>> +}
>
> So, this is such a helper function. It is curious that it
> deliberately lacks the ability to reset what has already been
> configured back to the default.
>
> For example, we earlier saw this original code ...
>
>> - item = xmalloc(sizeof(*item));
>> - item->text = arg;
>> - item->where = where;
>> - item->if_exists = if_exists;
>> - item->if_missing = if_missing;
>
> ... gets replaced with this call.
>
>> + struct trailer_conf *conf_current = new_trailer_conf();
>> ...
>> + trailer_conf_set(where, if_exists, if_missing, conf_current);
>
> For this conversion to be correct, we require that the value of the
> *_DEFAULT macro to be 0 and/or these three values getting assigned
> are not *_DEFAULT values. Maybe that may happen to be the case in
> today's code, but such an unwritten assumption makes me feel nervous.
>
> I am not sure if it is worth the confusion factor to make a function
> whose name is $anything_set() to be making a conditional assignment.
> If such a conditional assignment *also* happens often and deserves
> to have its own helper, it is fine to have such a helper for
> conditional assignment, but calling it $anything_set() is probably a
> mistake.
Agreed. I've split this out into separate helpers that do not have
conditionals built into them.
> Other than that, the main thrust of this step seems sensible.
Ack.
Overall, for the next reroll I have the commits broken down and it is
mostly the same. I just need to wait for CI to pass and update the cover
letter, before sending it (as v4). Should be ready later tonight.
Thanks.
