Re: [PATCH v4 2/7] pretty: allow %(trailers) options with explicit value

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 10, 2018, 08:45 UTC
Message-ID: <xmqqa7ldkbwr.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20181208163647.19538-3-anders@0x63.nu>

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

> In addition to old %(trailers:only) it is now allowed to write
> %(trailers:only=yes)

s/$/. Similarly the unfold option can take a boolean./

> By itself this only gives (the not quite so useful) possibility to have
> users change their mind in the middle of a formatting
> string (%(trailers:only=true,only=false)). However, it gives users the
> opportunity to override defaults from future options.

Makes sense.

> +** 'unfold[=val]': make it behave as if interpret-trailer's `--unfold`
> +   option was given. In same way as to for `only` it can be followed
> +   by an equal sign and explicit value. E.g.,
> +   `%(trailers:only,unfold=true)` unfolds and shows all trailer lines.

Show 21 quoted lines

> +static int match_placeholder_bool_arg(const char *to_parse, const char *candidate,
> +				      const char **end, int *val)
> +{
> +	const char *p;
> +	if (!skip_prefix(to_parse, candidate, &p))
> +		return 0;
> +
> +	if (match_placeholder_arg(p, "=no", end) ||
> +	    match_placeholder_arg(p, "=off", end) ||
> +	    match_placeholder_arg(p, "=false", end)) {
> +		*val = 0;
> +		return 1;
> +	}
> +
> +	if (match_placeholder_arg(p, "", end) ||
> +	    match_placeholder_arg(p, "=yes", end) ||
> +	    match_placeholder_arg(p, "=on", end) ||
> +	    match_placeholder_arg(p, "=true", end)) {
> +		*val = 1;
> +		return 1;
> +	}

Hmph. Is there a possibility to arrenge the code so that we do not have to maintain these variants of true/false representations here, when we should already have one in config.c?

The match_placeholder_arg() function is a bit too limiting as it can only recognize the value that we know about for a thing like this. Instead, perhaps we can cut what follows "=" syntactically, looking for either NUL, ',', or ')', and then call git_parse_maybe_bool() on it. That way, we can handle %(trailers:only=bogo) more sensibly, no? Syntactically we can recognize that the user wanted to give 'bogo' as the value to 'only', and say "'bogo' is not a boolean" if we did so.

> +	return 0;
> +}
> +