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

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

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

> Subject: Re: [PATCH v5 2/7] pretty: Allow %(trailers) options with explicit value

Style: s/pretty: Allow/pretty: allow/ (haven't I said this often enough?)

> +** 'only[=val]': select whether non-trailer lines from the trailer
> +   block should be included. The `only` keyword may optionally be
> +   followed by an equal sign and one of `true`, `on`, `yes` to omit or
> +   `false`, `off`, `no` to show the non-trailer lines. If option is
> +   given without value it is enabled. If given multiple times the last
> +   value is used.
> +** '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.

Sounds sensible.

> diff --git a/pretty.c b/pretty.c
> index b83a3ecd23..b8d71a57c9 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -1056,13 +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)
> +static int match_placeholder_arg_value(const char *to_parse, const char *candidate,
> +				       const char **end, const char **valuestart, size_t *valuelen)

An overlong line here.

> ...
> +static int match_placeholder_bool_arg(const char *to_parse, const char *candidate,
> +				      const char **end, int *val)
> +{
> +	char buf[8];
> +	const char *strval;
> +	size_t len;
> +	int v;
> +
> +	if (!match_placeholder_arg_value(to_parse, candidate, end, &strval, &len))
> +		return 0;
> +
> +	if (!strval) {
> +		*val = 1;
> +		return 1;
> +	}
> +
> +	strlcpy(buf, strval, sizeof(buf));
> +	if (len < sizeof(buf))
> +		buf[len] = 0;

Doesn't strlcpy() terminate buf[len] if len is short enough?
Even if the strval is longer than buf[], strlcpy() would truncate
and make sure buf[] is NUL terminated, no?

> +	v = git_parse_maybe_bool(buf);

Why?

This function would simply be buggy and incapable of parsing a
representation of a boolean value that is longer than 8 bytes (if
such a representation exists), so chomping an overlong string at the
end and feeding it to git_parse_maybe_bool() is a nonsense, isn't
it?

In this particular case, strlcpy() is inviting a buggy programming.
If there were a 7-letter representation of falsehood, strval may be
that 7-letter thing, in which case you would want to feed it to
git_parse_maybe_bool() to receive "false" from it, or strval may
have that 7-letter thing followed by a 'x' (so as a token, that is
not a correctly spelled falsehood), but strlcpy() would chomp and
show the same 7-letter falsehood to git_parse_maybe_bool().  That
robs you from an opportunity to diagnose such a bogus input as an
error.

Instead of using "char buf[8]", just using a strbuf and avoidng
strlcpy() would make the code much better, I would think.