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

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

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

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

> 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);
> +}

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