Re: [PATCH v3 2/5] pretty: allow showing specific trailers

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 20, 2018, 05:45 UTC
Message-ID: <xmqq36rwb8v5.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20181118114427.1397-3-anders@0x63.nu>

Anders Waldenborg <anders@0x63.nu> writes:

Show 10 quoted lines

> +  followed by a colon and zero or more comma-separated options:
> +  ** 'only': omit non-trailer lines from the trailer block.
> +  ** 'unfold': make it behave as if interpret-trailer's `--unfold`
> +     option was given.
> +  ** 'key=<K>': only show trailers with specified key. Matching is
> +     done case-insensitively and trailing colon is optional. If option
> +     is given multiple times only last one is used.
> +  ** Examples: `%(trailers:only,unfold)` unfolds and shows all trailer
> +     lines, `%(trailers:key=Reviewed-by,unfold)` unfolds and shows
> +     trailer lines with key `Reviewed-by`.

The last "Examples" item does not logically belong to the other three, which bothers me a bit.

Show 18 quoted lines

> diff --git a/pretty.c b/pretty.c
> index aa03d5b23..aaedc8447 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -1074,6 +1074,17 @@ static int match_placeholder_arg(const char *to_parse, const char *candidate,
>  	return 0;
>  }
>  
> +struct format_trailer_match_data {
> +	const char *trailer;
> +	size_t trailer_len;
> +};
> +
> +static int format_trailer_match_cb(const struct strbuf *sb, void *ud)
> +{
> +	struct format_trailer_match_data *data = ud;
> +	return data->trailer_len == sb->len && !strncasecmp (data->trailer, sb->buf, sb->len);
> +}

Show 31 quoted lines

>  	if (skip_prefix(placeholder, "(trailers", &arg)) {
>  		struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
> +		struct format_trailer_match_data filter_data;
>  		size_t ret = 0;
>  
>  		opts.no_divider = 1;
> @@ -1323,7 +1335,24 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
>  					opts.only_trailers = 1;
>  				else if (match_placeholder_arg(arg, "unfold", &arg))
>  					opts.unfold = 1;
> -				else
> +				else if (skip_prefix(arg, "key=", &arg)) {
> +					const char *end = arg + strcspn(arg, ",)");
> +
> +					filter_data.trailer = arg;
> +					filter_data.trailer_len = end - arg;
> +
> +					if (filter_data.trailer_len &&
> +					    filter_data.trailer[filter_data.trailer_len - 1] == ':')
> +						filter_data.trailer_len--;
> +
> +					opts.filter = format_trailer_match_cb;
> +					opts.filter_data = &filter_data;
> +					opts.only_trailers = 1;
> +
> +					arg = end;
> +					if (*arg == ',')
> +						arg++;
> +				} else
>  					break;
>  			}

This is part of a loop that is entered after seeing "%(trailers:" and existing one looks for 'unfold' and 'only' by using the match_placeholder_arg() helper, which

 - returns false if the keyword is not what is being sought after;
 - returns 1 if it finds the keyword, followed by ',' or ')', and
   updates the end pointer to point at either the closing ')' or one
   place after the ','.

The new part cannot directly reuse the same helper because it expects some non-constant string after "key=", but shouldn't we be introducing a similar helper with extended feature to help this part of the code to stay readable? Exposing the minute details of the logic to parse "key=<value>,..." while hiding the counterpart to parse "(only|unfold),..." makes the implementation of the above loop uneven and hard to follow.

I wonder if a helper like this would help:

static int match_placeholder(const char *to_parse, const char *keyword,
			     const char **value, size_t *valuelen,
			     const char **end)
{
	const char *p;

	if (!(skip_prefix(to_parse, keyword, &p)))
		return 0;

	if (value && valuelen) {
		/* expect "<keyword>=<value>" */
		size_t vlen;
		if (*p++ != '=')
			return 1;
		vlen = strcspn(p, ",)");
		if (!p[vlen])
			return 0;
		*value = p;
		*valuelen = vlen;
		p = p + vlen;
	}

	if (*p == ',') {
		*end = p + 1;
		return 1;
	}
	if (*p == ')') {
		*end = p;
		return 1;
	}
	return 0;
}

which would allow the existing one to become a thin wrapper

static int match_placeholder_arg(const char *a, const char *b, const char **c)
{
	return match_placeholder(a, b, NULL, NULL, c);
}

or can simply be eliminated by updating its only two callsites.

In the version you wrote, it is not clear what would happen if the format string ends with "%(trailers:key=" (no value, no comma, not even the closing paren). I do not think it would fall into infinite loop, but I do not think the code structure without the helper that makes the loop's logic uniform would allow it to properly diagnose such a malformed string.