Re: [PATCH 5/5] trailer: support values folded to multiple lines

[Junio C Hamano <gitster@pobox.com>]

Jonathan Tan <jonathantanmy@google.com> writes:

> Currently, interpret-trailers requires that a trailer be only on 1 line.
> For example:
>
>   a: first line
>      second line
>
> would be interpreted as one trailer line followed by one non-trailer line.
>
> Make interpret-trailers support RFC 822-style folding, treating those
> lines as one logical trailer.

Let's see how the code handles one minor detail when we see 822
folding, namely, "what happens to the leading whitespace that signals
the beginning of the second and subsequent lines?".

> diff --git a/trailer.c b/trailer.c
> index 97e96a9..907baa0 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -31,7 +31,7 @@ struct trailer_item {
>  	 * (excluding the terminating newline) and token is NULL.
>  	 */
>  	char *token;
> -	char *value;
> +	struct strbuf value;
>  };

Is the length of value very frequently used once the list of trailer
lines are fully parsed?  If not, I'd rather not to have "struct
strbuf" in a long-living structure like this one and instead prefer
keeping it a simple and stupid "char *value".

Yes, I know the existing code in trailers overuses strbuf when there
is no need, primarily because it uses the lazy "split into an array
of strbufs" function.  We shouldn't make it worse.

> @@ -767,16 +773,24 @@ static int process_input_file(FILE *outfile,
>  
>  	/* Parse trailer lines */
>  	for (i = trailer_start; i < trailer_end; i++) {
> +		if (last && isspace(lines[i]->buf[0])) {

It is convenient if "value" is a strbuf to do this,

> +			/* continuation line of the last trailer item */
> +			strbuf_addch(&last->value, '\n');
> +			strbuf_addbuf(&last->value, lines[i]);
> +			strbuf_strip_suffix(&last->value, "\n");

but it is easy to introduce a temporary strbuf in this scope and use
it only to create the final value and detach it to last->value, i.e.

		if (last && isspace(*lines[i]->buf)) {
			struct strbuf buf = STRBUF_INIT;
			strbuf_addf(&buf, "%s\n%s", last->value, lines[i]->buf);
			strbuf_strip_suffix(&buf, "\n");
			free(last->value);
			last->value = strbuf_detach(&buf, NULL);

By the way, I now see that the code handles the "minor detail" to
keep the leading whitespace, which is good.

Thanks.