Re: [PATCH 2/2] ref-filter: 'contents:trailers' show error if `:` is missing
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 19, 2020, 17:55 UTC
- Message-ID
- <xmqqv9hettag.fsf@gitster.c.googlers.com>
- In-Reply-To
- <7daf9335a501b99c29e299f72823fcb7e549e748.1597841551.git.gitgitgadget@gmail.com>
"Hariom Verma via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 15 quoted lines
> From: Hariom Verma <hariom18599@gmail.com> > > The 'contents' atom does not show any error if used with 'trailers' > atom and semicolon is missing before trailers arguments. > > e.g %(contents:trailersonly) works, while it shouldn't. > > It is definitely not an expected behavior. > > Let's fix this bug. > > Mentored-by: Christian Couder <chriscool@tuxfamily.org> > Mentored-by: Heba Waly <heba.waly@gmail.com> > Signed-off-by: Hariom Verma <hariom18599@gmail.com> > ---
Nice spotting. 7a5edbdb (ref-filter.c: parse trailers arguments with %(contents) atom, 2017-10-01) talks about being deliberate about the case where skip_prefix(":") does not find a colon after the "trailers" token, but from the message it is clear that it expected that the case happens only when "trailers" is at the end of the string.
The new helper that is overly verbose and may be overkill.
Shouldn't this be clear enough, equivalent and sufficient?
else if (skip_prefix(arg, "trailers", &arg) &&
(!*arg || *arg == ':'))) {
if (trailers_atom_parser(...);That is, we not just make sure the string begins with "trailers", but also make sure it either (1) ends the string (i.e. the token is just "trailers"), or (2) is followed by a colon ':', before entering the block to handle "trailers[:anything]". If we later add a new atom "trailersonly", that will not be handled here, but elsewhere in the "else if" cascade.
Show 43 quoted lines
> ref-filter.c | 21 ++++++++++++++++++---
> t/t6300-for-each-ref.sh | 9 +++++++++
> 2 files changed, 27 insertions(+), 3 deletions(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index ba85869755..dc31fbbe51 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -332,6 +332,22 @@ static int trailers_atom_parser(const struct ref_format *format, struct used_ato
> return 0;
> }
>
> +static int check_format_field(const char *arg, const char *field, const char **option)
> +{
> + const char *opt;
> + if (skip_prefix(arg, field, &opt)) {
> + if (*opt == '\0') {
> + *option = NULL;
> + return 1;
> + }
> + else if (*opt == ':') {
> + *option = ++opt;
> + return 1;
> + }
> + }
> + return 0;
> +}
> +
> static int contents_atom_parser(const struct ref_format *format, struct used_atom *atom,
> const char *arg, struct strbuf *err)
> {
> @@ -345,9 +361,8 @@ static int contents_atom_parser(const struct ref_format *format, struct used_ato
> atom->u.contents.option = C_SIG;
> else if (!strcmp(arg, "subject"))
> atom->u.contents.option = C_SUB;
> - else if (skip_prefix(arg, "trailers", &arg)) {
> - skip_prefix(arg, ":", &arg);
> - if (trailers_atom_parser(format, atom, *arg ? arg : NULL, err))
> + else if (check_format_field(arg, "trailers", &arg)) {
> + if (trailers_atom_parser(format, atom, arg, err))
> return -1;
> } else if (skip_prefix(arg, "lines=", &arg)) {
> atom->u.contents.option = C_LINES;