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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 12, 2016, 06:23 UTC
Message-ID: <xmqqr37m5bmz.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <4b8616732b719ede04b90c87ab240c29b4e3a0bb.1476232683.git.jonathantanmy@google.com>

Jonathan Tan <jonathantanmy@google.com> writes:

Show 10 quoted lines

> 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?".

Show 11 quoted 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.

Show 5 quoted lines

> @@ -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.