| Message ID | 7daf9335a501b99c29e299f72823fcb7e549e748.1597841551.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix trailers atom bug and improved tests | expand |
"Hariom Verma via GitGitGadget" <gitgitgadget@gmail.com> writes: > 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. > 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;
Junio C Hamano <gitster@pobox.com> writes: > "Hariom Verma via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> 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(...); Ah, no, even with "*arg++ == ':'. This moves arg past "trailers" if given "trailersandsomegarbage" and the next one in "else if" cascade would look at "andsomegarbage"---which is not what we want. >> +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; >> +} And the helper does not have such a breakage. It looks good. Thanks.
On Wed, Aug 19, 2020 at 3:07 PM Junio C Hamano <gitster@pobox.com> wrote: > Junio C Hamano <gitster@pobox.com> writes: > > "Hariom Verma via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> +static int check_format_field(const char *arg, const char *field, const char **option) > >> +{ > >> + else if (*opt == ':') { > >> + *option = ++opt; > >> + return 1; > >> + } > > And the helper does not have such a breakage. It looks good. One minor comment (not worth a re-roll): I personally found: *option = ++opt; more confusing than: *option = opt + 1; The `++opt` places a higher cognitive load on the reader. As a reviewer, I had to go back and carefully reread the function to see if the side-effect of `++opt` had some impact which I didn't notice on the first readthrough. The simpler `opt + 1` does not have a side-effect, thus is easier to reason about (and doesn't require me to re-study the function when I encounter it).
Eric Sunshine <sunshine@sunshineco.com> writes: > On Wed, Aug 19, 2020 at 3:07 PM Junio C Hamano <gitster@pobox.com> wrote: >> Junio C Hamano <gitster@pobox.com> writes: >> > "Hariom Verma via GitGitGadget" <gitgitgadget@gmail.com> writes: >> >> +static int check_format_field(const char *arg, const char *field, const char **option) >> >> +{ >> >> + else if (*opt == ':') { >> >> + *option = ++opt; >> >> + return 1; >> >> + } >> >> And the helper does not have such a breakage. It looks good. > > One minor comment (not worth a re-roll): I personally found: > > *option = ++opt; > > more confusing than: > > *option = opt + 1; > > The `++opt` places a higher cognitive load on the reader. As a > reviewer, I had to go back and carefully reread the function to see if > the side-effect of `++opt` had some impact which I didn't notice on > the first readthrough. The simpler `opt + 1` does not have a > side-effect, thus is easier to reason about (and doesn't require me to > re-study the function when I encounter it). That makes the two of us ... thanks.
Hi, On Thu, Aug 20, 2020 at 3:38 AM Junio C Hamano <gitster@pobox.com> wrote: > > Eric Sunshine <sunshine@sunshineco.com> writes: > > > On Wed, Aug 19, 2020 at 3:07 PM Junio C Hamano <gitster@pobox.com> wrote: > >> Junio C Hamano <gitster@pobox.com> writes: > >> > "Hariom Verma via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> >> +static int check_format_field(const char *arg, const char *field, const char **option) > >> >> +{ > >> >> + else if (*opt == ':') { > >> >> + *option = ++opt; > >> >> + return 1; > >> >> + } > >> > >> And the helper does not have such a breakage. It looks good. > > > > One minor comment (not worth a re-roll): I personally found: > > > > *option = ++opt; > > > > more confusing than: > > > > *option = opt + 1; > > > > The `++opt` places a higher cognitive load on the reader. As a > > reviewer, I had to go back and carefully reread the function to see if > > the side-effect of `++opt` had some impact which I didn't notice on > > the first readthrough. The simpler `opt + 1` does not have a > > side-effect, thus is easier to reason about (and doesn't require me to > > re-study the function when I encounter it). > > That makes the two of us ... thanks. It seems like the score is 2-0. I guess I'm going with winning side. Will be improved in next version. Thanks, Hariom
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; diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index 495848c881..cb1508cef5 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -823,6 +823,15 @@ test_expect_success '%(trailers) rejects unknown trailers arguments' ' test_i18ncmp expect actual ' +test_expect_success 'if arguments, %(contents:trailers) shows error if semicolon is missing' ' + # error message cannot be checked under i18n + cat >expect <<-EOF && + fatal: unrecognized %(contents) argument: trailersonly + EOF + test_must_fail git for-each-ref --format="%(contents:trailersonly)" 2>actual && + test_i18ncmp expect actual +' + test_expect_success 'basic atom: head contents:trailers' ' git for-each-ref --format="%(contents:trailers)" refs/heads/master >actual && sanitize_pgp <actual >actual.clean &&