Re: [PATCH v4 1/6] pretty.c: delimit "%(trailers)" arguments with ","

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

Taylor Blau <me@ttaylorr.com> writes:

> In preparation for adding consistent "%(trailers)" atom options to
> `git-for-each-ref(1)`'s "--format" argument, change "%(trailers)" in
> pretty.c to separate sub-arguments with a ",", instead of a ":".
>
> Multiple sub-arguments are given either as "%(trailers:unfold,only)" or
> "%(trailers:only,unfold)".
>
> This change disambiguates between "top-level" arguments, and arguments
> given to the trailers atom itself. It is consistent with the behavior of
> "%(upstream)" and "%(push)" atoms.

Looks good.  

Ignore the remainder unless you are interested in the recent "make
style" discussion.

>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
>  pretty.c                      | 34 +++++++++++++++++++++++++++++-----
>  t/t4205-log-pretty-formats.sh |  4 ++--
>  2 files changed, 31 insertions(+), 7 deletions(-)
>
> diff --git a/pretty.c b/pretty.c
> index 94eab5c89..eec128bc1 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -1056,6 +1056,25 @@ static size_t parse_padding_placeholder(struct strbuf *sb,
>  	return 0;
>  }
>  
> +static int match_placeholder_arg(const char *to_parse,
> +				const char *candidate,
> +				const char **end)

"make style" wants to format this like so:

static int match_placeholder_arg(const char *to_parse, const char *candidate,
				 const char **end)

I think I can live with either versions ;-)

> +{
> +	const char *p;
> +	if (!(skip_prefix(to_parse, candidate, &p)))
> +		return 0;
> +	if (*p == ',') {
> +		*end = p + 1;
> +		return 1;
> +	}
> +	if (*p == ')') {
> +		*end = p;
> +		return 1;
> +	}
> +	return 0;
> +}
> +
> +

"make style" seems to be unhappy to see double blank here, and I
tend to agree.

>  static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
>  				const char *placeholder,
>  				void *context)
> @@ -1285,11 +1304,16 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
>  
>  	if (skip_prefix(placeholder, "(trailers", &arg)) {
>  		struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
> -		while (*arg == ':') {
> -			if (skip_prefix(arg, ":only", &arg))
> -				opts.only_trailers = 1;
> -			else if (skip_prefix(arg, ":unfold", &arg))
> -				opts.unfold = 1;
> +		if (*arg == ':') {
> +			arg++;
> +			for (;;) {
> +				if (match_placeholder_arg(arg, "only", &arg))
> +					opts.only_trailers = 1;
> +				else if (match_placeholder_arg(arg, "unfold", &arg))
> +					opts.unfold = 1;
> +				else
> +					break;
> +			}
>  		}
>  		if (*arg == ')') {
>  			format_trailers_from_commit(sb, msg + c->subject_off, &opts);
> diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
> index ec5f53010..977472f53 100755
> --- a/t/t4205-log-pretty-formats.sh
> +++ b/t/t4205-log-pretty-formats.sh
> @@ -588,8 +588,8 @@ test_expect_success '%(trailers:unfold) unfolds trailers' '
>  '
>  
>  test_expect_success ':only and :unfold work together' '
> -	git log --no-walk --pretty="%(trailers:only:unfold)" >actual &&
> -	git log --no-walk --pretty="%(trailers:unfold:only)" >reverse &&
> +	git log --no-walk --pretty="%(trailers:only,unfold)" >actual &&
> +	git log --no-walk --pretty="%(trailers:unfold,only)" >reverse &&
>  	test_cmp actual reverse &&
>  	{
>  		grep -v patch.description <trailers | unfold &&