Re: [PATCH v5 2/7] pretty: Allow %(trailers) options with explicit value
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 28, 2019, 22:38 UTC
- Message-ID
- <xmqq8sz49zm1.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <20190128213337.24752-3-anders@0x63.nu>
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?)
Show 10 quoted lines
> +** '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.
Show 12 quoted lines
> 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.
Show 20 quoted lines
> ...
> +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.