Re: [PATCH v3 03/10] trailer: unify trailer formatting machinery

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 31, 2024, 20:13 UTC
Message-ID: <xmqqy1c545y0.fsf@gitster.g>
In-Reply-To: <5c7a2354df0f4a29841f9ab8294ead0e1c3b9cf5.1706664145.git.gitgitgadget@gmail.com>

"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 29 quoted lines

> From: Linus Arver <linusa@google.com>
>
> Currently have two functions for formatting trailers exposed in
> trailer.h:
>
>     void format_trailers(FILE *outfile, struct list_head *head,
>                         const struct process_trailer_options *opts);
>
>     void format_trailers_from_commit(struct strbuf *out, const char *msg,
>                                     const struct process_trailer_options *opts);
>
> and previously these functions, although similar enough (even taking the
> same process_trailer_options struct pointer), did not build on each
> other.
>
> Make format_trailers_from_commit() rely on format_trailers(). Teach
> format_trailers() to process trailers with the additional
> process_trailer_options fields like opts->key_only which is only used by
> format_trailers_from_commit() and not builtin/interpret-trailers.c.
> While we're at it, reorder parameters to put the trailer processing
> options first, and the out parameter (strbuf we write into) at the end.
>
> This unification will allow us to delete the format_trailer_info() and
> print_tok_val() functions in the next patch. They are not deleted here
> in order to keep the diff small.
>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Linus Arver <linusa@google.com>
> ---

I am not sure how I helped this particularly (say, compared to all the other patches on the list from everybody), though.

The updated format_trailers() does so much more than the original and does so quite differently, it is hard to tell if the existing caller is getting the same behaviour (modulo obviously it now needs to prepare a strbuf and print the resulting string in the strbuf, which is expected) as before.

The conversion for the sole existing caller looks like this, ...

Show 10 quoted lines

>  	LIST_HEAD(head);
>  	struct strbuf sb = STRBUF_INIT;
> +	struct strbuf trailer_block = STRBUF_INIT;
>  	struct trailer_info info;
> ...
> -	format_trailers(opts, &head, outfile);
> +	/* Print trailer block. */
> +	format_trailers(opts, &head, &trailer_block);
> +	fwrite(trailer_block.buf, 1, trailer_block.len, outfile);
> +	strbuf_release(&trailer_block);

... and it is very straight-forward.

But what it called was this:

Show 30 quoted lines

> -static void print_tok_val(FILE *outfile, const char *tok, const char *val)
> -{
> -	char c;
> -
> -	if (!tok) {
> -		fprintf(outfile, "%s\n", val);
> -		return;
> -	}
> -
> -	c = last_non_space_char(tok);
> -	if (!c)
> -		return;
> -	if (strchr(separators, c))
> -		fprintf(outfile, "%s%s\n", tok, val);
> -	else
> -		fprintf(outfile, "%s%c %s\n", tok, separators[0], val);
> -}
> -
> -void format_trailers(const struct process_trailer_options *opts,
> -		     struct list_head *trailers, FILE *outfile)
> -{
> -	struct list_head *pos;
> -	struct trailer_item *item;
> -	list_for_each(pos, trailers) {
> -		item = list_entry(pos, struct trailer_item, list);
> -		if ((!opts->trim_empty || strlen(item->value) > 0) &&
> -		    (!opts->only_trailers || item->token))
> -			print_tok_val(outfile, item->token, item->value);
> -	}
> -}

i.e. iterate over trailers, and sometimes call print_tok_val(), which does not do all that much. Print only the val, or when tok is not empty, show "tok: val" while taking into account the possibility that the last byte of tok may already be the separator.

But the new thing is way more complex.

Show 26 quoted lines

> +void format_trailers(const struct process_trailer_options *opts,
> +		     struct list_head *trailers,
> +		     struct strbuf *out)
> +{
> +	struct list_head *pos;
> +	struct trailer_item *item;
> +	int need_separator = 0;
> +
> +	list_for_each(pos, trailers) {
> +		item = list_entry(pos, struct trailer_item, list);
> +		if (item->token) {
> +			char c;
> +
> +			struct strbuf tok = STRBUF_INIT;
> +			struct strbuf val = STRBUF_INIT;
> +			strbuf_addstr(&tok, item->token);
> +			strbuf_addstr(&val, item->value);
> +
> +			/*
> +			 * Skip key/value pairs where the value was empty. This
> +			 * can happen from trailers specified without a
> +			 * separator, like `--trailer "Reviewed-by"` (no
> +			 * corresponding value).
> +			 */
> +			if (opts->trim_empty && !strlen(item->value))
> +				continue;

OK, this matches the first condition to stay silent in the original.

> +			if (!opts->filter || opts->filter(&tok, opts->filter_data)) {

For the original caller of format_trailers(), which is the interpret_trailers(), .filter member is never set, so we always come here, I presume. cmd_interpret_trailers(), before calling interpret_trailers() could affect the following members:

    .in_place .trim_empty .only_trailers .only_input .unfold .no_divider

so we should make sure this new implementation does not change the behaviour from the original with various combination of these options.

> +				if (opts->unfold)
> +					unfold_value(&val);

So, ... how would this affect an invocation of

    $ git interpret-trailers --unfold

which would have set opts.unfold to true in cmd_interpret_trailers() and eventually called process_trailers() with it, which eventually called print_all(), which conditionally called print_tok_val() but never gave the val to unfold_value()?

 ... goes and digs a bit more ...

Ahhhh, original process_trailers() did this by calling parse_trailers() that munged the value. So its print_all() codepath only had to print what has already been munged.

But then in this new code, do we unfold only here? I thought that in the new code you moved around, the caller, whose name is now interpret_trailers(), still calls parse_trailers() and that calls the unfold_value(). So, are we doing redundant work that may merely be unneeded (if we are lucky) or could be harmful (if things, other than the unfold thing I just noticed, that are done both in parse_trailers() and this function are not idempotent)?

It could be that a bit more preparatory refactoring would clarify. I dunno.

Show 40 quoted lines

> +				if (opts->separator && need_separator)
> +					strbuf_addbuf(out, opts->separator);
> +				if (!opts->value_only)
> +					strbuf_addbuf(out, &tok);
> +				if (!opts->key_only && !opts->value_only) {
> +					if (opts->key_value_separator)
> +						strbuf_addbuf(out, opts->key_value_separator);
> +					else {
> +						c = last_non_space_char(tok.buf);
> +						if (c) {
> +							if (!strchr(separators, c))
> +								strbuf_addf(out, "%c ", separators[0]);
> +						}
> +					}
> +				}
> +				if (!opts->key_only)
> +					strbuf_addbuf(out, &val);
> +				if (!opts->separator)
> +					strbuf_addch(out, '\n');
> +
> +				need_separator = 1;
> +			}
> +
> +			strbuf_release(&tok);
> +			strbuf_release(&val);
> +		} else if (!opts->only_trailers) {
> +			if (opts->separator && need_separator) {
> +				strbuf_addbuf(out, opts->separator);
> +			}
> +			strbuf_addstr(out, item->value);
> +			if (opts->separator)
> +				strbuf_rtrim(out);
> +			else
> +				strbuf_addch(out, '\n');
> +
> +			need_separator = 1;
> +		}
> +
> +	}
> +}