Anders Waldenborg <anders@xxxxxxx> 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.
