[GSoC] Fix trailers atom bug and improved tests #707
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/submit |
|
Submitted as pull.707.git.1597841551.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -776,60 +776,39 @@ test_expect_success 'set up trailers for next test' ' | |||
| ' | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Hariom Verma via GitGitGadget" <gitgitgadget@gmail.com> writes:
> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index a83579fbdf..495848c881 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -776,60 +776,39 @@ test_expect_success 'set up trailers for next test' '
> '
>
> test_expect_success '%(trailers:unfold) unfolds trailers' '
> - git for-each-ref --format="%(trailers:unfold)" refs/heads/master >actual &&
> {
> unfold <trailers
> echo
> } >expect &&
> + git for-each-ref --format="%(trailers:unfold)" refs/heads/master >actual &&
> + test_cmp expect actual &&
> + git for-each-ref --format="%(contents:trailers:unfold)" refs/heads/master >actual &&
> test_cmp expect actual
> '
Hmph, what is this one doing? Ah, OK, trailers:unfold is tested as
before (just the steps to prepare 'expect' and 'actual' got swapped),
and because the same expectation holds for contents:trailers:unfold,
we can test it at the same. Makes sense.
> test_expect_success '%(trailers:only) and %(trailers:unfold) work together' '
> - git for-each-ref --format="%(trailers:only,unfold)" refs/heads/master >actual &&
> - git for-each-ref --format="%(trailers:unfold,only)" refs/heads/master >reverse &&
> - test_cmp actual reverse &&
> {
> grep -v patch.description <trailers | unfold &&
> echo
> } >expect &&
> + git for-each-ref --format="%(trailers:only,unfold)" refs/heads/master >actual &&
> + git for-each-ref --format="%(trailers:unfold,only)" refs/heads/master >reverse &&
> + test_cmp actual reverse &&
> + test_cmp expect actual &&
This uses different pattern. It may be cleaner to test one side at
a time, as we have prepared the 'expect' that should be the same for
both, and compare with the expected pattern one at a time; that would
eliminate the need for 'reverse', too. I.e.
{
grep -v patch.description trailers | unfold && echo
} >expect &&
git for-each-ref ... only,unfold ... >actual &&
test_cmp expect actual &&
git for-each-ref ... unfold,only ... >actual &&
test_cmp expect actual &&
> @@ -839,14 +818,7 @@ test_expect_success '%(trailers) rejects unknown trailers arguments' '
> fatal: unknown %(trailers) argument: unsupported
> EOF
> test_must_fail git for-each-ref --format="%(trailers:unsupported)" 2>actual &&
> - test_i18ncmp expect actual
> -'
> -
> -test_expect_success '%(contents:trailers) rejects unknown trailers arguments' '
> - # error message cannot be checked under i18n
> - cat >expect <<-EOF &&
> - fatal: unknown %(trailers) argument: unsupported
> - EOF
> + test_i18ncmp expect actual &&
> test_must_fail git for-each-ref --format="%(contents:trailers:unsupported)" 2>actual &&
> test_i18ncmp expect actual
> '
Doesn't this highlight a small bug, where an end-user request for an
unknown %(contents:trailers:unsupported) is flagged as an error
about %(trailers)? Is it OK because we expect that users who use
the longer %(contents:trailers) to know that it is a synonym for
%(trailers) and the latter is the official way to write it?
Thanks.
There was a problem hiding this comment.
On the Git mailing list, Hariom verma wrote (reply to this):
Hi,
On Wed, Aug 19, 2020 at 11:01 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Hariom Verma via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > @@ -839,14 +818,7 @@ test_expect_success '%(trailers) rejects unknown trailers arguments' '
> > fatal: unknown %(trailers) argument: unsupported
> > EOF
> > test_must_fail git for-each-ref --format="%(trailers:unsupported)" 2>actual &&
> > - test_i18ncmp expect actual
> > -'
> > -
> > -test_expect_success '%(contents:trailers) rejects unknown trailers arguments' '
> > - # error message cannot be checked under i18n
> > - cat >expect <<-EOF &&
> > - fatal: unknown %(trailers) argument: unsupported
> > - EOF
> > + test_i18ncmp expect actual &&
> > test_must_fail git for-each-ref --format="%(contents:trailers:unsupported)" 2>actual &&
> > test_i18ncmp expect actual
> > '
>
> Doesn't this highlight a small bug, where an end-user request for an
> unknown %(contents:trailers:unsupported) is flagged as an error
> about %(trailers)? Is it OK because we expect that users who use
> the longer %(contents:trailers) to know that it is a synonym for
> %(trailers) and the latter is the official way to write it?
Maybe.
Another way of thinking is...
'trailers' is an argument to 'contents', likewise here 'unsupported'
is an argument to trailers.
Technically, the error message is correct.
Again, I think views on this are highly subjective.
Thanks,
Hariom
ref-filter.c
Outdated
| @@ -332,6 +332,22 @@ static int trailers_atom_parser(const struct ref_format *format, struct used_ato | |||
| return 0; | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"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;
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
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.
There was a problem hiding this comment.
On the Git mailing list, Eric Sunshine wrote (reply to this):
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).
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
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.
There was a problem hiding this comment.
On the Git mailing list, Hariom verma wrote (reply to this):
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
harry-hov
force-pushed
the
fix-trailers-atom-bug
branch
from
7daf933
to
39aa46b
Compare
|
/submit |
|
Submitted as pull.707.v2.git.1598004663.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
ref-filter.c
Outdated
| @@ -332,6 +332,22 @@ static int trailers_atom_parser(const struct ref_format *format, struct used_ato | |||
| return 0; | |||
There was a problem hiding this comment.
On the Git mailing list, Eric Sunshine wrote (reply to this):
On Fri, Aug 21, 2020 at 6:11 AM Hariom Verma via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> The 'contents' atom does not show any error if used with 'trailers'
> atom and semicolon is missing before trailers arguments.
Do you mean s/semicolon/colon/ ?
> e.g %(contents:trailersonly) works, while it shouldn't.
>
> It is definitely not an expected behavior.
>
> Let's fix this bug.
>
> Signed-off-by: Hariom Verma <hariom18599@gmail.com>
> ---
> diff --git 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
> +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 + 1;
> + return 1;
> + }
> + }
> + return 0;
> +}
Not necessarily worth a re-roll, but rather than introducing all the
above new code...
> @@ -345,9 +361,8 @@ static int contents_atom_parser(const struct ref_format *format, struct used_ato
> - 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;
...an alternative would have been something like:
else if (!strcmp(arg, "trailers")) {
if (trailers_atom_parser(format, atom, NULL, err))
return -1;
} else if (skip_prefix(arg, "trailers:", &arg)) {
if (trailers_atom_parser(format, atom, arg, err))
return -1;
}
which is quite simple to reason about (though has the cost of a tiny
bit of duplication).
> diff --git 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_expect_success 'if arguments, %(contents:trailers) shows error if semicolon is missing' '
s/semicolon/colon/
> + # error message cannot be checked under i18n
What is this comment about? I realize that you copied it from other
nearby tests, but I find that it muddies rather than clarifies.
> + 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
> +'
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Eric Sunshine <sunshine@sunshineco.com> writes:
> ...an alternative would have been something like:
>
> else if (!strcmp(arg, "trailers")) {
> if (trailers_atom_parser(format, atom, NULL, err))
> return -1;
> } else if (skip_prefix(arg, "trailers:", &arg)) {
> if (trailers_atom_parser(format, atom, arg, err))
> return -1;
> }
>
> which is quite simple to reason about (though has the cost of a tiny
> bit of duplication).
Yeah, that looks quite simple and straight-forward.
>> diff --git 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_expect_success 'if arguments, %(contents:trailers) shows error if semicolon is missing' '
>
> s/semicolon/colon/
Definitely.
>
>> + # error message cannot be checked under i18n
>
> What is this comment about? I realize that you copied it from other
> nearby tests, but I find that it muddies rather than clarifies.
Yup. If a patch changes test_cmp with test_i18ncmp, the above
message belongs to its commit log message, but it is overkill to
have it as an in-line comment in every place where test_i18ncmp gets
used.
Thanks for a review.
>> + 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
>> +'
There was a problem hiding this comment.
On the Git mailing list, Hariom verma wrote (reply to this):
Hi,
On Sat, Aug 22, 2020 at 12:47 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
> > ...an alternative would have been something like:
> >
> > else if (!strcmp(arg, "trailers")) {
> > if (trailers_atom_parser(format, atom, NULL, err))
> > return -1;
> > } else if (skip_prefix(arg, "trailers:", &arg)) {
> > if (trailers_atom_parser(format, atom, arg, err))
> > return -1;
> > }
> >
> > which is quite simple to reason about (though has the cost of a tiny
> > bit of duplication).
>
> Yeah, that looks quite simple and straight-forward.
No doubt, it looks good for "contents:trailers".
What if In future we would like to expand functionalities of other
'contents' options?
Recently, I sent a patch series "Improvements to ref-filter"[1]. A
patch in this patch series introduced "sanitize" modifier to "subject"
atom. i.e "%(subject:sanitize)".
What if in the future we also want "%(contents:subject:sanitize)" to work?
We can duplicate code again. Something like:
```
} else if (!strcmp(arg, "trailers")) {
if (trailers_atom_parser(format, atom, NULL, err))
return -1;
} else if (skip_prefix(arg, "trailers:", &arg)) {
if (trailers_atom_parser(format, atom, arg, err))
return -1;
} else if (!strcmp(arg, "subject")) {
if (subject_atom_parser(format, atom, NULL, err))
return -1;
} else if (skip_prefix(arg, "subject:", &arg)) {
if (subject_atom_parser(format, atom, arg, err))
return -1;
}
```
OR
We can just simply use helper. Something like:
```
else if (check_format_field(arg, "subject", &arg)) {
if (subject_atom_parser(format, atom, arg, err))
return -1;
} else if (check_format_field(arg, "trailers", &arg)) {
if (trailers_atom_parser(format, atom, arg, err))
return -1;
```
We can use this helper any number of times, whenever there is a need.
Sorry, I missed saying this earlier. But I don't prefer duplicating
the code here.
Thanks,
Hariom
[1]: https://public-inbox.org/git/pull.684.v4.git.1598046110.gitgitgadget@gmail.com/#t
There was a problem hiding this comment.
On the Git mailing list, Eric Sunshine wrote (reply to this):
On Sun, Aug 23, 2020 at 8:56 PM Hariom verma <hariom18599@gmail.com> wrote:
> On Sat, Aug 22, 2020 at 12:47 AM Junio C Hamano <gitster@pobox.com> wrote:
> > Eric Sunshine <sunshine@sunshineco.com> writes:
> > > ...an alternative would have been something like:
> > >
> > > else if (!strcmp(arg, "trailers")) {
> > > if (trailers_atom_parser(format, atom, NULL, err))
> > > return -1;
> > > } else if (skip_prefix(arg, "trailers:", &arg)) {
> > > if (trailers_atom_parser(format, atom, arg, err))
> > > return -1;
> > > }
> > >
> > > which is quite simple to reason about (though has the cost of a tiny
> > > bit of duplication).
> >
> > Yeah, that looks quite simple and straight-forward.
>
> Recently, I sent a patch series "Improvements to ref-filter"[1]. A
> patch in this patch series introduced "sanitize" modifier to "subject"
> atom. i.e "%(subject:sanitize)".
>
> What if in the future we also want "%(contents:subject:sanitize)" to work?
> We can use this helper any number of times, whenever there is a need.
>
> Sorry, I missed saying this earlier. But I don't prefer duplicating
> the code here.
Pushing back on a reviewer suggestion is fine. Explaining the reason
for your position -- as you do here -- helps reviewers understand why
you feel the way you do. My review suggestion about making it easier
to reason about the code while avoiding a brand new function, at the
cost of a minor amount of duplication, was made in the context of this
one-off case in which the function increased cognitive load and was
used just once (not knowing that you envisioned future callers). If
you expect the new function to be re-used by upcoming changes, then
that may be a good reason to keep it. Stating so in the commit message
will help reviewers see beyond the immediate patch or patch series.
Aside from a couple minor style violations[1,2], I don't particularly
oppose the helper function, though I have a quibble with the name
check_format_field(), which I don't find helpful, and which (at least
for me) increases the cognitive load. The increased cognitive load, I
think, comes not only from the function name not spelling out what the
function actually does, but also because the function is dual-purpose:
it's both checking that the argument matches a particular token
("trailers", in this case) and extracting the sub-argument. Perhaps
naming it match_and_extract_subarg() or something similar would help,
though that's a mouthful.
But the observation about the function being dual-purpose (thus
potentially confusing) brings up other questions. For instance, is it
too special-purpose? If you foresee more callers in the future with
multiple-token arguments such as `%(content:subject:sanitize)`, should
the function provide more assistance by splitting out each of the
sub-arguments rather than stopping at the first? Taking that even
further, a generalized helper for "splitting" arguments like that
might be useful at the top-level of contents_atom_parser() too, rather
than only for specific arguments, such as "trailers". Of course, this
may all be way too ambitious for this little bug fix series or even
for whatever upcoming changes you're planning, thus not worth
pursuing.
As for the helper's implementation, I might have written it like this:
static int check_format_field(...)
{
const char *opt
if (!strcmp(arg, field))
*option = NULL;
else if (skip_prefix(arg, field, opt) && *opt == ':')
*option = opt + 1;
else
return 0;
return 1;
}
which is more compact and closer to what I suggested earlier for
avoiding the helper function in the first place. But, of course,
programming is quite subjective, and you may find your implementation
easier to reason about. Plus, your version has the benefit of being
slightly more optimal since it avoids an extra string scan, although
that probably is mostly immaterial considering that
contents_atom_parser() itself contains a long chain of potentially
sub-optimal strcmp() and skip_prefix() calls.
Footnotes
[1]: use `if (!*opt)` rather than `if (*opt == '\0')`
[2]: cuddle the closing brace and `else` on the same line like this:
`} else if (...) {`
There was a problem hiding this comment.
On the Git mailing list, Hariom verma wrote (reply to this):
Hi,
On Mon, Aug 24, 2020 at 9:19 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Sun, Aug 23, 2020 at 8:56 PM Hariom verma <hariom18599@gmail.com> wrote:
> > On Sat, Aug 22, 2020 at 12:47 AM Junio C Hamano <gitster@pobox.com> wrote:
> > > Eric Sunshine <sunshine@sunshineco.com> writes:
> > > > ...an alternative would have been something like:
> > > >
> > > > else if (!strcmp(arg, "trailers")) {
> > > > if (trailers_atom_parser(format, atom, NULL, err))
> > > > return -1;
> > > > } else if (skip_prefix(arg, "trailers:", &arg)) {
> > > > if (trailers_atom_parser(format, atom, arg, err))
> > > > return -1;
> > > > }
> > > >
> > > > which is quite simple to reason about (though has the cost of a tiny
> > > > bit of duplication).
> > >
> > > Yeah, that looks quite simple and straight-forward.
> >
> > Recently, I sent a patch series "Improvements to ref-filter"[1]. A
> > patch in this patch series introduced "sanitize" modifier to "subject"
> > atom. i.e "%(subject:sanitize)".
> >
> > What if in the future we also want "%(contents:subject:sanitize)" to work?
> > We can use this helper any number of times, whenever there is a need.
> >
> > Sorry, I missed saying this earlier. But I don't prefer duplicating
> > the code here.
>
> Pushing back on a reviewer suggestion is fine. Explaining the reason
> for your position -- as you do here -- helps reviewers understand why
> you feel the way you do. My review suggestion about making it easier
> to reason about the code while avoiding a brand new function, at the
> cost of a minor amount of duplication, was made in the context of this
> one-off case in which the function increased cognitive load and was
> used just once (not knowing that you envisioned future callers). If
> you expect the new function to be re-used by upcoming changes, then
> that may be a good reason to keep it. Stating so in the commit message
> will help reviewers see beyond the immediate patch or patch series.
Yeah. I should have mentioned this in the commit message.
> Aside from a couple minor style violations[1,2], I don't particularly
> oppose the helper function, though I have a quibble with the name
> check_format_field(), which I don't find helpful, and which (at least
> for me) increases the cognitive load. The increased cognitive load, I
> think, comes not only from the function name not spelling out what the
> function actually does, but also because the function is dual-purpose:
> it's both checking that the argument matches a particular token
> ("trailers", in this case) and extracting the sub-argument. Perhaps
> naming it match_and_extract_subarg() or something similar would help,
> though that's a mouthful.
I will fix those violations.
Also, "match_and_extract_subarg()" looks good to me.
> But the observation about the function being dual-purpose (thus
> potentially confusing) brings up other questions. For instance, is it
> too special-purpose? If you foresee more callers in the future with
> multiple-token arguments such as `%(content:subject:sanitize)`, should
> the function provide more assistance by splitting out each of the
> sub-arguments rather than stopping at the first? Taking that even
> further, a generalized helper for "splitting" arguments like that
> might be useful at the top-level of contents_atom_parser() too, rather
> than only for specific arguments, such as "trailers". Of course, this
> may all be way too ambitious for this little bug fix series or even
> for whatever upcoming changes you're planning, thus not worth
> pursuing.
Splitting sub-arguments is done at "<atomname>_atom_parser()".
If you mean pre-splitting every argument...
something like: ['contents', 'subject', 'sanitize'] for
`%(content:subject:sanitize)` in `contents_atom_parser()` ? I'm not
able to see how it can be useful.
Sorry, If I got your concerned wrong.
> As for the helper's implementation, I might have written it like this:
>
> static int check_format_field(...)
> {
> const char *opt
> if (!strcmp(arg, field))
> *option = NULL;
> else if (skip_prefix(arg, field, opt) && *opt == ':')
> *option = opt + 1;
> else
> return 0;
> return 1;
> }
>
> which is more compact and closer to what I suggested earlier for
> avoiding the helper function in the first place. But, of course,
> programming is quite subjective, and you may find your implementation
> easier to reason about. Plus, your version has the benefit of being
> slightly more optimal since it avoids an extra string scan, although
> that probably is mostly immaterial considering that
> contents_atom_parser() itself contains a long chain of potentially
> sub-optimal strcmp() and skip_prefix() calls.
"programming is quite subjective"
Yeah, I couldn't agree more.
The change you suggested looks good too. But I'm little inclined to my
keeping my changes. I'm curious, what others have to say on this.
Thanks,
Hariom
There was a problem hiding this comment.
On the Git mailing list, Christian Couder wrote (reply to this):
Hi,
On Tue, Aug 25, 2020 at 1:32 AM Hariom verma <hariom18599@gmail.com> wrote:
> On Mon, Aug 24, 2020 at 9:19 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> >
> > On Sun, Aug 23, 2020 at 8:56 PM Hariom verma <hariom18599@gmail.com> wrote:
> > > Recently, I sent a patch series "Improvements to ref-filter"[1]. A
> > > patch in this patch series introduced "sanitize" modifier to "subject"
> > > atom. i.e "%(subject:sanitize)".
> > >
> > > What if in the future we also want "%(contents:subject:sanitize)" to work?
> > > We can use this helper any number of times, whenever there is a need.
> > >
> > > Sorry, I missed saying this earlier. But I don't prefer duplicating
> > > the code here.
> >
> > Pushing back on a reviewer suggestion is fine. Explaining the reason
> > for your position -- as you do here -- helps reviewers understand why
> > you feel the way you do. My review suggestion about making it easier
> > to reason about the code while avoiding a brand new function, at the
> > cost of a minor amount of duplication, was made in the context of this
> > one-off case in which the function increased cognitive load and was
> > used just once (not knowing that you envisioned future callers). If
> > you expect the new function to be re-used by upcoming changes, then
> > that may be a good reason to keep it. Stating so in the commit message
> > will help reviewers see beyond the immediate patch or patch series.
>
> Yeah. I should have mentioned this in the commit message.
I agree.
> > Aside from a couple minor style violations[1,2], I don't particularly
> > oppose the helper function, though I have a quibble with the name
> > check_format_field(), which I don't find helpful, and which (at least
> > for me) increases the cognitive load. The increased cognitive load, I
> > think, comes not only from the function name not spelling out what the
> > function actually does, but also because the function is dual-purpose:
> > it's both checking that the argument matches a particular token
> > ("trailers", in this case) and extracting the sub-argument. Perhaps
> > naming it match_and_extract_subarg() or something similar would help,
> > though that's a mouthful.
>
> I will fix those violations.
> Also, "match_and_extract_subarg()" looks good to me.
I am not sure about the "subarg" part of the name. In the for-each-ref
doc, names inside %(...) are called "field names", and parts after ":"
are called "options". So it might be better to have "field_option"
instead of "subarg" in the name.
I think we could also get rid of the "match_and_" part of the
suggestion, in the same way as skip_prefix() is not called
match_and_skip_prefix(). Readers can just expect that if there is no
match the function will return 0.
So maybe "extract_field_option()".
> > But the observation about the function being dual-purpose (thus
> > potentially confusing) brings up other questions. For instance, is it
> > too special-purpose? If you foresee more callers in the future with
> > multiple-token arguments such as `%(content:subject:sanitize)`, should
> > the function provide more assistance by splitting out each of the
> > sub-arguments rather than stopping at the first? Taking that even
> > further, a generalized helper for "splitting" arguments like that
> > might be useful at the top-level of contents_atom_parser() too, rather
> > than only for specific arguments, such as "trailers". Of course, this
> > may all be way too ambitious for this little bug fix series or even
> > for whatever upcoming changes you're planning, thus not worth
> > pursuing.
>
> Splitting sub-arguments is done at "<atomname>_atom_parser()".
> If you mean pre-splitting every argument...
> something like: ['contents', 'subject', 'sanitize'] for
> `%(content:subject:sanitize)` in `contents_atom_parser()` ? I'm not
> able to see how it can be useful.
Yeah, it seems to me that such a splitting would require a complete
rewrite of the current code, so I am not sure it's an interesting way
forward for now. And anyway adding extract_field_option() goes in the
right direction of abstracting the parsing and making the code
simpler, more efficient and likely more correct.
> Sorry, If I got your concerned wrong.
>
> > As for the helper's implementation, I might have written it like this:
> >
> > static int check_format_field(...)
> > {
> > const char *opt
> > if (!strcmp(arg, field))
> > *option = NULL;
> > else if (skip_prefix(arg, field, opt) && *opt == ':')
> > *option = opt + 1;
> > else
> > return 0;
> > return 1;
> > }
> >
> > which is more compact and closer to what I suggested earlier for
> > avoiding the helper function in the first place. But, of course,
> > programming is quite subjective, and you may find your implementation
> > easier to reason about. Plus, your version has the benefit of being
> > slightly more optimal since it avoids an extra string scan, although
> > that probably is mostly immaterial considering that
> > contents_atom_parser() itself contains a long chain of potentially
> > sub-optimal strcmp() and skip_prefix() calls.
>
> "programming is quite subjective"
> Yeah, I couldn't agree more.
>
> The change you suggested looks good too. But I'm little inclined to my
> keeping my changes. I'm curious, what others have to say on this.
I also prefer a slightly more optimal one even if it's a bit less compact.
Thanks,
Christian.
There was a problem hiding this comment.
On the Git mailing list, Christian Couder wrote (reply to this):
On Wed, Aug 26, 2020 at 8:18 AM Christian Couder
<christian.couder@gmail.com> wrote:
> I think we could also get rid of the "match_and_" part of the
> suggestion, in the same way as skip_prefix() is not called
> match_and_skip_prefix(). Readers can just expect that if there is no
> match the function will return 0.
>
> So maybe "extract_field_option()".
If we want to hint more that it works in the way as skip_prefix(), we
could call it "skip_field()".
There was a problem hiding this comment.
On the Git mailing list, Hariom verma wrote (reply to this):
Hi,
On Wed, Aug 26, 2020 at 11:48 AM Christian Couder
<christian.couder@gmail.com> wrote:
>
> Hi,
>
> On Tue, Aug 25, 2020 at 1:32 AM Hariom verma <hariom18599@gmail.com> wrote:
>
> > On Mon, Aug 24, 2020 at 9:19 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> > > Aside from a couple minor style violations[1,2], I don't particularly
> > > oppose the helper function, though I have a quibble with the name
> > > check_format_field(), which I don't find helpful, and which (at least
> > > for me) increases the cognitive load. The increased cognitive load, I
> > > think, comes not only from the function name not spelling out what the
> > > function actually does, but also because the function is dual-purpose:
> > > it's both checking that the argument matches a particular token
> > > ("trailers", in this case) and extracting the sub-argument. Perhaps
> > > naming it match_and_extract_subarg() or something similar would help,
> > > though that's a mouthful.
> >
> > I will fix those violations.
> > Also, "match_and_extract_subarg()" looks good to me.
>
> I am not sure about the "subarg" part of the name. In the for-each-ref
> doc, names inside %(...) are called "field names", and parts after ":"
> are called "options". So it might be better to have "field_option"
> instead of "subarg" in the name.
>
> I think we could also get rid of the "match_and_" part of the
> suggestion, in the same way as skip_prefix() is not called
> match_and_skip_prefix(). Readers can just expect that if there is no
> match the function will return 0.
>
> So maybe "extract_field_option()".
Makes sense to me.
> > > But the observation about the function being dual-purpose (thus
> > > potentially confusing) brings up other questions. For instance, is it
> > > too special-purpose? If you foresee more callers in the future with
> > > multiple-token arguments such as `%(content:subject:sanitize)`, should
> > > the function provide more assistance by splitting out each of the
> > > sub-arguments rather than stopping at the first? Taking that even
> > > further, a generalized helper for "splitting" arguments like that
> > > might be useful at the top-level of contents_atom_parser() too, rather
> > > than only for specific arguments, such as "trailers". Of course, this
> > > may all be way too ambitious for this little bug fix series or even
> > > for whatever upcoming changes you're planning, thus not worth
> > > pursuing.
> >
> > Splitting sub-arguments is done at "<atomname>_atom_parser()".
> > If you mean pre-splitting every argument...
> > something like: ['contents', 'subject', 'sanitize'] for
> > `%(content:subject:sanitize)` in `contents_atom_parser()` ? I'm not
> > able to see how it can be useful.
>
> Yeah, it seems to me that such a splitting would require a complete
> rewrite of the current code, so I am not sure it's an interesting way
> forward for now. And anyway adding extract_field_option() goes in the
> right direction of abstracting the parsing and making the code
> simpler, more efficient and likely more correct.
>
> > Sorry, If I got your concerned wrong.
> >
> > > As for the helper's implementation, I might have written it like this:
> > >
> > > static int check_format_field(...)
> > > {
> > > const char *opt
> > > if (!strcmp(arg, field))
> > > *option = NULL;
> > > else if (skip_prefix(arg, field, opt) && *opt == ':')
> > > *option = opt + 1;
> > > else
> > > return 0;
> > > return 1;
> > > }
> > >
> > > which is more compact and closer to what I suggested earlier for
> > > avoiding the helper function in the first place. But, of course,
> > > programming is quite subjective, and you may find your implementation
> > > easier to reason about. Plus, your version has the benefit of being
> > > slightly more optimal since it avoids an extra string scan, although
> > > that probably is mostly immaterial considering that
> > > contents_atom_parser() itself contains a long chain of potentially
> > > sub-optimal strcmp() and skip_prefix() calls.
> >
> > "programming is quite subjective"
> > Yeah, I couldn't agree more.
> >
> > The change you suggested looks good too. But I'm little inclined to my
> > keeping my changes. I'm curious, what others have to say on this.
>
> I also prefer a slightly more optimal one even if it's a bit less compact.
+1
Thanks,
Hariom
39aa46b
to
d5efe49
Compare
harry-hov
changed the title
harry-hov
force-pushed
the
fix-trailers-atom-bug
branch
from
d5efe49
to
befebdb
Compare
Currently, there are different tests for testing %(trailers) and %(contents:trailers) causing redundant copy. Its time to get rid of duplicate code. Mentored-by: Christian Couder <chriscool@tuxfamily.org> Mentored-by: Heba Waly <heba.waly@gmail.com> Signed-off-by: Hariom Verma <hariom18599@gmail.com>
The 'contents' atom does not show any error if used with 'trailers' atom and colon 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. Acked-by: Eric Sunshine <sunshine@sunshineco.com> Mentored-by: Christian Couder <chriscool@tuxfamily.org> Mentored-by: Heba Waly <heba.waly@gmail.com> Signed-off-by: Hariom Verma <hariom18599@gmail.com>
Refactored trailers formatting logic inside pretty.c to a new function `format_set_trailers_options()`. Also, introduced a code to get invalid trailer arguments. As we would like to use same logic in ref-filter, it's nice to get invalid trailer argument. This will allow us to print accurate error message, while using `format_set_trailers_options()` in ref-filter. Mentored-by: Christian Couder <chriscool@tuxfamily.org> Mentored-by: Heba Waly <heba.waly@gmail.com> Signed-off-by: Hariom Verma <hariom18599@gmail.com>
Now, ref-filter is using pretty.c logic for setting trailer options. New to ref-filter: :key=<K> - only show trailers with specified key. :valueonly[=val] - only show the value part. :separator=<SEP> - inserted between trailer lines Enhancement to existing options(now can take value and its optional): :only[=val] :unfold[=val] 'val' can be: true, on, yes or false, off, no. Mentored-by: Christian Couder <chriscool@tuxfamily.org> Mentored-by: Heba Waly <heba.waly@gmail.com> Signed-off-by: Hariom Verma <hariom18599@gmail.com>
harry-hov
force-pushed
the
fix-trailers-atom-bug
branch
from
befebdb
to
d491be5
Compare
|
/submit |
|
Submitted as pull.707.v3.git.1598043976.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
| @@ -345,9 +345,11 @@ static int contents_atom_parser(const struct ref_format *format, struct used_ato | |||
| atom->u.contents.option = C_SIG; | |||
There was a problem hiding this comment.
On the Git mailing list, Eric Sunshine wrote (reply to this):
On Fri, Aug 21, 2020 at 5:06 PM Hariom Verma via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> The 'contents' atom does not show any error if used with 'trailers'
> atom and colon 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.
>
> Acked-by: Eric Sunshine <sunshine@sunshineco.com>
I didn't "ack" this patch. If you think some sort of attribution with
my name is warranted, then a "Helped-by:" would be more appropriate.
> Signed-off-by: Hariom Verma <hariom18599@gmail.com>
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> @@ -345,9 +345,11 @@ static int contents_atom_parser(const struct ref_format *format, struct used_ato
> - else if (skip_prefix(arg, "trailers", &arg)) {
> - skip_prefix(arg, ":", &arg);
> - if (trailers_atom_parser(format, atom, *arg ? arg : NULL, err))
> + else if (!strcmp(arg, "trailers")) {
> + if (trailers_atom_parser(format, atom, NULL, err))
> + return -1;
> + } else if (skip_prefix(arg, "trailers:", &arg)) {
> + if (trailers_atom_parser(format, atom, arg, err))
> return -1;
This looks better and easier to reason about (but I may be biased in
thinking so).
> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> @@ -823,6 +823,14 @@ test_expect_success '%(trailers) rejects unknown trailers arguments' '
> +test_expect_success 'if arguments, %(contents:trailers) shows error if semicolon is missing' '
This still needs a s/semicolon/colon/ (mentioned in my previous review).
There was a problem hiding this comment.
On the Git mailing list, Hariom verma wrote (reply to this):
Hi Eric,
On Sat, Aug 22, 2020 at 2:43 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Fri, Aug 21, 2020 at 5:06 PM Hariom Verma via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > The 'contents' atom does not show any error if used with 'trailers'
> > atom and colon 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.
> >
> > Acked-by: Eric Sunshine <sunshine@sunshineco.com>
>
> I didn't "ack" this patch. If you think some sort of attribution with
> my name is warranted, then a "Helped-by:" would be more appropriate.
Sorry about that. Fixing in the next version.
> > Signed-off-by: Hariom Verma <hariom18599@gmail.com>
> > ---
> > diff --git a/ref-filter.c b/ref-filter.c
> > @@ -345,9 +345,11 @@ static int contents_atom_parser(const struct ref_format *format, struct used_ato
> > - else if (skip_prefix(arg, "trailers", &arg)) {
> > - skip_prefix(arg, ":", &arg);
> > - if (trailers_atom_parser(format, atom, *arg ? arg : NULL, err))
> > + else if (!strcmp(arg, "trailers")) {
> > + if (trailers_atom_parser(format, atom, NULL, err))
> > + return -1;
> > + } else if (skip_prefix(arg, "trailers:", &arg)) {
> > + if (trailers_atom_parser(format, atom, arg, err))
> > return -1;
>
> This looks better and easier to reason about (but I may be biased in
> thinking so).
Thanks for the review.
> > diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> > @@ -823,6 +823,14 @@ test_expect_success '%(trailers) rejects unknown trailers arguments' '
> > +test_expect_success 'if arguments, %(contents:trailers) shows error if semicolon is missing' '
>
> This still needs a s/semicolon/colon/ (mentioned in my previous review).
Sorry, I missed that too.
Thanks,
Hariom
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Eric Sunshine <sunshine@sunshineco.com> writes:
> On Fri, Aug 21, 2020 at 5:06 PM Hariom Verma via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> The 'contents' atom does not show any error if used with 'trailers'
>> atom and colon 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.
>>
>> Acked-by: Eric Sunshine <sunshine@sunshineco.com>
>
> I didn't "ack" this patch. If you think some sort of attribution with
> my name is warranted, then a "Helped-by:" would be more appropriate.
Yes, I did exactly that after moving it just above Hariom's sign-off.
>> Signed-off-by: Hariom Verma <hariom18599@gmail.com>
>> ---
>> diff --git a/ref-filter.c b/ref-filter.c
>> @@ -345,9 +345,11 @@ static int contents_atom_parser(const struct ref_format *format, struct used_ato
>> - else if (skip_prefix(arg, "trailers", &arg)) {
>> - skip_prefix(arg, ":", &arg);
>> - if (trailers_atom_parser(format, atom, *arg ? arg : NULL, err))
>> + else if (!strcmp(arg, "trailers")) {
>> + if (trailers_atom_parser(format, atom, NULL, err))
>> + return -1;
>> + } else if (skip_prefix(arg, "trailers:", &arg)) {
>> + if (trailers_atom_parser(format, atom, arg, err))
>> return -1;
>
> This looks better and easier to reason about (but I may be biased in
> thinking so).
>
>> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
>> @@ -823,6 +823,14 @@ test_expect_success '%(trailers) rejects unknown trailers arguments' '
>> +test_expect_success 'if arguments, %(contents:trailers) shows error if semicolon is missing' '
>
> This still needs a s/semicolon/colon/ (mentioned in my previous review).
Yup. Tweaked while queueing.
Thanks always for sharp eyes.
|
On the Git mailing list, Junio C Hamano wrote (reply to this): |
|
On the Git mailing list, Hariom verma wrote (reply to this): |
|
User |
|
Status: Partially Merged |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, there exists a bug in 'contents' atom. It does not show any error if used with modifier 'trailers' and semicolon is missing before trailers arguments.
This small patch series is focused on fixing that bug and also unified 'trailers' and 'contents:trailers' tests. Thus, removed duplicate code from
t6300and made tests more compact.Change log since v2:
cc: Hariom verma hariom18599@gmail.com