[PATCH 0/2] Fix trailers atom bug and improved tests

[PATCH 0/2] Fix trailers atom bug and improved tests

[Hariom Verma via GitGitGadget]

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 t6300 and made tests more compact.

Hariom Verma (2):
  t6300: unify %(trailers) and %(contents:trailers) tests
  ref-filter: 'contents:trailers' show error if `:` is missing

 ref-filter.c            | 21 +++++++++++++++---
 t/t6300-for-each-ref.sh | 49 +++++++++++++----------------------------
 2 files changed, 33 insertions(+), 37 deletions(-)


base-commit: 2befe97201e1f3175cce557866c5822793624b5a
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-707%2Fharry-hov%2Ffix-trailers-atom-bug-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-707/harry-hov/fix-trailers-atom-bug-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/707
-- 
gitgitgadget

[PATCH 1/2] t6300: unify %(trailers) and %(contents:trailers) tests

[Hariom Verma via GitGitGadget]

From: Hariom Verma <hariom18599@gmail.com>

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>
---
 t/t6300-for-each-ref.sh | 50 +++++++++--------------------------------
 1 file changed, 11 insertions(+), 39 deletions(-)

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
 '
 
 test_expect_success '%(trailers:only) shows only "key: value" trailers' '
-	git for-each-ref --format="%(trailers:only)" refs/heads/master >actual &&
 	{
 		grep -v patch.description <trailers &&
 		echo
 	} >expect &&
+	git for-each-ref --format="%(trailers:only)" refs/heads/master >actual &&
+	test_cmp expect actual &&
+	git for-each-ref --format="%(contents:trailers:only)" refs/heads/master >actual &&
 	test_cmp expect actual
 '
 
 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 &&
-	test_cmp expect actual
-'
-
-test_expect_success '%(contents:trailers:unfold) unfolds trailers' '
-	git for-each-ref --format="%(contents:trailers:unfold)" refs/heads/master >actual &&
-	{
-		unfold <trailers
-		echo
-	} >expect &&
-	test_cmp expect actual
-'
-
-test_expect_success '%(contents:trailers:only) shows only "key: value" trailers' '
-	git for-each-ref --format="%(contents:trailers:only)" refs/heads/master >actual &&
-	{
-		grep -v patch.description <trailers &&
-		echo
-	} >expect &&
-	test_cmp expect actual
-'
-
-test_expect_success '%(contents:trailers:only) and %(contents: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 &&
+	test_cmp expect actual &&
 	git for-each-ref --format="%(contents:trailers:only,unfold)" refs/heads/master >actual &&
 	git for-each-ref --format="%(contents:trailers:unfold,only)" refs/heads/master >reverse &&
 	test_cmp actual reverse &&
-	{
-		grep -v patch.description <trailers | unfold &&
-		echo
-	} >expect &&
 	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
 '
-- 
gitgitgadget

Re: [PATCH 1/2] t6300: unify %(trailers) and %(contents:trailers) tests

[Junio C Hamano]

"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.

Re: [PATCH 1/2] t6300: unify %(trailers) and %(contents:trailers) tests

[Hariom verma]

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

[PATCH 2/2] ref-filter: 'contents:trailers' show error if `:` is missing

[Hariom Verma via GitGitGadget]

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>
---
 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;
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 &&
-- 
gitgitgadget

Re: [PATCH 2/2] ref-filter: 'contents:trailers' show error if `:` is missing

[Junio C Hamano]

"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;

Re: [PATCH 2/2] ref-filter: 'contents:trailers' show error if `:` is missing

[Junio C Hamano]

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.

Re: [PATCH 2/2] ref-filter: 'contents:trailers' show error if `:` is missing

[Eric Sunshine]

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).

Re: [PATCH 2/2] ref-filter: 'contents:trailers' show error if `:` is missing

[Junio C Hamano]

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.

Re: [PATCH 2/2] ref-filter: 'contents:trailers' show error if `:` is missing

[Hariom verma]

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

[PATCH v2 0/2] Fix trailers atom bug and improved tests

[Hariom Verma via GitGitGadget]

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 t6300 and made tests more compact.

Hariom Verma (2):
  t6300: unify %(trailers) and %(contents:trailers) tests
  ref-filter: 'contents:trailers' show error if `:` is missing

 ref-filter.c            | 21 +++++++++++++---
 t/t6300-for-each-ref.sh | 55 ++++++++++++++---------------------------
 2 files changed, 36 insertions(+), 40 deletions(-)


base-commit: 675a4aaf3b226c0089108221b96559e0baae5de9
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-707%2Fharry-hov%2Ffix-trailers-atom-bug-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-707/harry-hov/fix-trailers-atom-bug-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/707

Range-diff vs v1:

 1:  bd0bb8d0ef ! 1:  4816aa3cfa t6300: unify %(trailers) and %(contents:trailers) tests
     @@ t/t6300-for-each-ref.sh: test_expect_success 'set up trailers for next test' '
      -
      -test_expect_success '%(contents:trailers:only) and %(contents: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 &&
      +	test_cmp expect actual &&
     ++	git for-each-ref --format="%(trailers:unfold,only)" refs/heads/master >actual &&
     ++	test_cmp actual actual &&
       	git for-each-ref --format="%(contents:trailers:only,unfold)" refs/heads/master >actual &&
     - 	git for-each-ref --format="%(contents:trailers:unfold,only)" refs/heads/master >reverse &&
     - 	test_cmp actual reverse &&
     +-	git for-each-ref --format="%(contents:trailers:unfold,only)" refs/heads/master >reverse &&
     +-	test_cmp actual reverse &&
      -	{
      -		grep -v patch.description <trailers | unfold &&
      -		echo
      -	} >expect &&
     - 	test_cmp expect actual
     +-	test_cmp expect actual
     ++	test_cmp expect actual &&
     ++	git for-each-ref --format="%(contents:trailers:unfold,only)" refs/heads/master >actual &&
     ++	test_cmp actual actual
       '
       
     + test_expect_success '%(trailers) rejects unknown trailers arguments' '
      @@ t/t6300-for-each-ref.sh: test_expect_success '%(trailers) rejects unknown trailers arguments' '
       	fatal: unknown %(trailers) argument: unsupported
       	EOF
 2:  7daf9335a5 ! 2:  39aa46bce7 ref-filter: 'contents:trailers' show error if `:` is missing
     @@ ref-filter.c: static int trailers_atom_parser(const struct ref_format *format, s
      +			return 1;
      +		}
      +		else if (*opt == ':') {
     -+			*option = ++opt;
     ++			*option = opt + 1;
      +			return 1;
      +		}
      +	}

-- 
gitgitgadget

[PATCH v2 1/2] t6300: unify %(trailers) and %(contents:trailers) tests

[Hariom Verma via GitGitGadget]

From: Hariom Verma <hariom18599@gmail.com>

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>
---
 t/t6300-for-each-ref.sh | 56 +++++++++++------------------------------
 1 file changed, 14 insertions(+), 42 deletions(-)

diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index a83579fbdf..0570380344 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -776,61 +776,40 @@ 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
 '
 
 test_expect_success '%(trailers:only) shows only "key: value" trailers' '
-	git for-each-ref --format="%(trailers:only)" refs/heads/master >actual &&
 	{
 		grep -v patch.description <trailers &&
 		echo
 	} >expect &&
+	git for-each-ref --format="%(trailers:only)" refs/heads/master >actual &&
+	test_cmp expect actual &&
+	git for-each-ref --format="%(contents:trailers:only)" refs/heads/master >actual &&
 	test_cmp expect actual
 '
 
 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 &&
-	test_cmp expect actual
-'
-
-test_expect_success '%(contents:trailers:unfold) unfolds trailers' '
-	git for-each-ref --format="%(contents:trailers:unfold)" refs/heads/master >actual &&
-	{
-		unfold <trailers
-		echo
-	} >expect &&
-	test_cmp expect actual
-'
-
-test_expect_success '%(contents:trailers:only) shows only "key: value" trailers' '
-	git for-each-ref --format="%(contents:trailers:only)" refs/heads/master >actual &&
-	{
-		grep -v patch.description <trailers &&
-		echo
-	} >expect &&
-	test_cmp expect actual
-'
-
-test_expect_success '%(contents:trailers:only) and %(contents:trailers:unfold) work together' '
+	git for-each-ref --format="%(trailers:only,unfold)" refs/heads/master >actual &&
+	test_cmp expect actual &&
+	git for-each-ref --format="%(trailers:unfold,only)" refs/heads/master >actual &&
+	test_cmp actual actual &&
 	git for-each-ref --format="%(contents:trailers:only,unfold)" refs/heads/master >actual &&
-	git for-each-ref --format="%(contents:trailers:unfold,only)" refs/heads/master >reverse &&
-	test_cmp actual reverse &&
-	{
-		grep -v patch.description <trailers | unfold &&
-		echo
-	} >expect &&
-	test_cmp expect actual
+	test_cmp expect actual &&
+	git for-each-ref --format="%(contents:trailers:unfold,only)" refs/heads/master >actual &&
+	test_cmp actual actual
 '
 
 test_expect_success '%(trailers) rejects unknown trailers arguments' '
@@ -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
 '
-- 
gitgitgadget

[PATCH v2 2/2] ref-filter: 'contents:trailers' show error if `:` is missing

[Hariom Verma via GitGitGadget]

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>
---
 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..fa131c4854 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 + 1;
+			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 0570380344..6d535653d9 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 &&
-- 
gitgitgadget

Re: [PATCH v2 2/2] ref-filter: 'contents:trailers' show error if `:` is missing

[Eric Sunshine]

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
> +'

Re: [PATCH v2 2/2] ref-filter: 'contents:trailers' show error if `:` is missing

[Junio C Hamano]

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
>> +'

Re: [PATCH v2 2/2] ref-filter: 'contents:trailers' show error if `:` is missing

[Hariom verma]

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

Re: [PATCH v2 2/2] ref-filter: 'contents:trailers' show error if `:` is missing

[Eric Sunshine]

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 (...) {`

Re: [PATCH v2 2/2] ref-filter: 'contents:trailers' show error if `:` is missing

[Hariom verma]

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

Re: [PATCH v2 2/2] ref-filter: 'contents:trailers' show error if `:` is missing

[Christian Couder]

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.

Re: [PATCH v2 2/2] ref-filter: 'contents:trailers' show error if `:` is missing

[Christian Couder]

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()".

Re: [PATCH v2 2/2] ref-filter: 'contents:trailers' show error if `:` is missing

[Hariom verma]

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

[PATCH v3 0/4] [GSoC] Fix trailers atom bug and improved tests

[Hariom Verma via GitGitGadget]

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 t6300 and made tests more compact.

Change log since v2:

 * Used simplified logic as per suggested by Eric (here 
   https://public-inbox.org/git/CAPig+cRxCvHG70Nd00zBxYFuecu6+Z6uDP8ooN3rx9vPagoYBA@mail.gmail.com/
   )
 * Unified trailer formatting logic for pretty.c and ref-filter.c

Hariom Verma (4):
  t6300: unify %(trailers) and %(contents:trailers) tests
  ref-filter: 'contents:trailers' show error if `:` is missing
  pretty.c: refactor trailer logic to `format_set_trailers_options()`
  ref-filter: using pretty.c logic for trailers

 Documentation/git-for-each-ref.txt |  36 ++++++--
 Hariom Verma via GitGitGadget      |   0
 pretty.c                           |  83 +++++++++++-------
 pretty.h                           |  11 +++
 ref-filter.c                       |  43 +++++-----
 t/t6300-for-each-ref.sh            | 133 ++++++++++++++++++++++-------
 6 files changed, 219 insertions(+), 87 deletions(-)
 create mode 100644 Hariom Verma via GitGitGadget


base-commit: 675a4aaf3b226c0089108221b96559e0baae5de9
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-707%2Fharry-hov%2Ffix-trailers-atom-bug-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-707/harry-hov/fix-trailers-atom-bug-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/707

Range-diff vs v2:

 1:  4816aa3cfa = 1:  383476b177 t6300: unify %(trailers) and %(contents:trailers) tests
 2:  39aa46bce7 ! 2:  659b9835dc ref-filter: 'contents:trailers' show error if `:` is missing
     @@ Commit message
          ref-filter: 'contents:trailers' show error if `:` is missing
      
          The 'contents' atom does not show any error if used with 'trailers'
     -    atom and semicolon is missing before trailers arguments.
     +    atom and colon is missing before trailers arguments.
      
          e.g %(contents:trailersonly) works, while it shouldn't.
      
     @@ Commit message
      
          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>
      
       ## ref-filter.c ##
     -@@ ref-filter.c: 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 + 1;
     -+			return 1;
     -+		}
     -+	}
     -+	return 0;
     -+}
     -+
     - static int contents_atom_parser(const struct ref_format *format, struct used_atom *atom,
     - 				const char *arg, struct strbuf *err)
     - {
      @@ ref-filter.c: static int contents_atom_parser(const struct ref_format *format, struct used_ato
       		atom->u.contents.option = C_SIG;
       	else if (!strcmp(arg, "subject"))
     @@ ref-filter.c: static int contents_atom_parser(const struct ref_format *format, s
      -	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)) {
     ++	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 (skip_prefix(arg, "lines=", &arg)) {
     @@ t/t6300-for-each-ref.sh: test_expect_success '%(trailers) rejects unknown traile
       '
       
      +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
 -:  ---------- > 3:  712ab9aacf pretty.c: refactor trailer logic to `format_set_trailers_options()`
 -:  ---------- > 4:  d491be5d10 ref-filter: using pretty.c logic for trailers

-- 
gitgitgadget

[PATCH v3 1/4] t6300: unify %(trailers) and %(contents:trailers) tests

[Hariom Verma via GitGitGadget]

From: Hariom Verma <hariom18599@gmail.com>

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>
---
 t/t6300-for-each-ref.sh | 56 +++++++++++------------------------------
 1 file changed, 14 insertions(+), 42 deletions(-)

diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index a83579fbdf..0570380344 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -776,61 +776,40 @@ 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
 '
 
 test_expect_success '%(trailers:only) shows only "key: value" trailers' '
-	git for-each-ref --format="%(trailers:only)" refs/heads/master >actual &&
 	{
 		grep -v patch.description <trailers &&
 		echo
 	} >expect &&
+	git for-each-ref --format="%(trailers:only)" refs/heads/master >actual &&
+	test_cmp expect actual &&
+	git for-each-ref --format="%(contents:trailers:only)" refs/heads/master >actual &&
 	test_cmp expect actual
 '
 
 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 &&
-	test_cmp expect actual
-'
-
-test_expect_success '%(contents:trailers:unfold) unfolds trailers' '
-	git for-each-ref --format="%(contents:trailers:unfold)" refs/heads/master >actual &&
-	{
-		unfold <trailers
-		echo
-	} >expect &&
-	test_cmp expect actual
-'
-
-test_expect_success '%(contents:trailers:only) shows only "key: value" trailers' '
-	git for-each-ref --format="%(contents:trailers:only)" refs/heads/master >actual &&
-	{
-		grep -v patch.description <trailers &&
-		echo
-	} >expect &&
-	test_cmp expect actual
-'
-
-test_expect_success '%(contents:trailers:only) and %(contents:trailers:unfold) work together' '
+	git for-each-ref --format="%(trailers:only,unfold)" refs/heads/master >actual &&
+	test_cmp expect actual &&
+	git for-each-ref --format="%(trailers:unfold,only)" refs/heads/master >actual &&
+	test_cmp actual actual &&
 	git for-each-ref --format="%(contents:trailers:only,unfold)" refs/heads/master >actual &&
-	git for-each-ref --format="%(contents:trailers:unfold,only)" refs/heads/master >reverse &&
-	test_cmp actual reverse &&
-	{
-		grep -v patch.description <trailers | unfold &&
-		echo
-	} >expect &&
-	test_cmp expect actual
+	test_cmp expect actual &&
+	git for-each-ref --format="%(contents:trailers:unfold,only)" refs/heads/master >actual &&
+	test_cmp actual actual
 '
 
 test_expect_success '%(trailers) rejects unknown trailers arguments' '
@@ -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
 '
-- 
gitgitgadget

[PATCH v3 2/4] ref-filter: 'contents:trailers' show error if `:` is missing

[Hariom Verma via GitGitGadget]

From: 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>
---
 ref-filter.c            | 8 +++++---
 t/t6300-for-each-ref.sh | 8 ++++++++
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index ba85869755..8ba0e31915 100644
--- 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
 		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 (!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 (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 0570380344..fdf2c442c5 100755
--- 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_i18ncmp expect actual
 '
 
+test_expect_success 'if arguments, %(contents:trailers) shows error if semicolon is missing' '
+	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 &&
-- 
gitgitgadget

Re: [PATCH v3 2/4] ref-filter: 'contents:trailers' show error if `:` is missing

[Eric Sunshine]

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).

Re: [PATCH v3 2/4] ref-filter: 'contents:trailers' show error if `:` is missing

[Hariom verma]

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

Re: [PATCH v3 2/4] ref-filter: 'contents:trailers' show error if `:` is missing

[Junio C Hamano]

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.

[PATCH v3 3/4] pretty.c: refactor trailer logic to `format_set_trailers_options()`

[Hariom Verma via GitGitGadget]

From: 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>
---
 Hariom Verma via GitGitGadget |  0
 pretty.c                      | 83 ++++++++++++++++++++++-------------
 pretty.h                      | 11 +++++
 3 files changed, 64 insertions(+), 30 deletions(-)
 create mode 100644 Hariom Verma via GitGitGadget

diff --git a/Hariom Verma via GitGitGadget b/Hariom Verma via GitGitGadget
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/pretty.c b/pretty.c
index 2a3d46bf42..bd8d38e27b 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1147,6 +1147,55 @@ static int format_trailer_match_cb(const struct strbuf *key, void *ud)
 	return 0;
 }
 
+int format_set_trailers_options(struct process_trailer_options *opts,
+				struct string_list *filter_list,
+				struct strbuf *sepbuf,
+				const char **arg,
+				const char **err)
+{
+	for (;;) {
+		const char *argval;
+		size_t arglen;
+
+		if (**arg != ')') {
+			size_t vallen = strcspn(*arg, "=,)");
+			const char *valstart = xstrndup(*arg, vallen);
+			if (strcmp(valstart, "key") &&
+				strcmp(valstart, "separator") &&
+				strcmp(valstart, "only") &&
+				strcmp(valstart, "valueonly") &&
+				strcmp(valstart, "unfold")) {
+					*err = xstrdup(valstart);
+					return 1;
+				}
+			free((char *)valstart);
+		}
+		if (match_placeholder_arg_value(*arg, "key", arg, &argval, &arglen)) {
+			uintptr_t len = arglen;
+
+			if (!argval)
+				return 1;
+
+			if (len && argval[len - 1] == ':')
+				len--;
+			string_list_append(filter_list, argval)->util = (char *)len;
+
+			opts->filter = format_trailer_match_cb;
+			opts->filter_data = filter_list;
+			opts->only_trailers = 1;
+		} else if (match_placeholder_arg_value(*arg, "separator", arg, &argval, &arglen)) {
+			char *fmt = xstrndup(argval, arglen);
+			strbuf_expand(sepbuf, fmt, strbuf_expand_literal_cb, NULL);
+			free(fmt);
+			opts->separator = sepbuf;
+		} else if (!match_placeholder_bool_arg(*arg, "only", arg, &opts->only_trailers) &&
+				!match_placeholder_bool_arg(*arg, "unfold", arg, &opts->unfold) &&
+				!match_placeholder_bool_arg(*arg, "valueonly", arg, &opts->value_only))
+			break;
+	}
+	return 0;
+}
+
 static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 				const char *placeholder,
 				void *context)
@@ -1417,41 +1466,14 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 		struct string_list filter_list = STRING_LIST_INIT_NODUP;
 		struct strbuf sepbuf = STRBUF_INIT;
 		size_t ret = 0;
+		const char *unused = NULL;
 
 		opts.no_divider = 1;
 
 		if (*arg == ':') {
 			arg++;
-			for (;;) {
-				const char *argval;
-				size_t arglen;
-
-				if (match_placeholder_arg_value(arg, "key", &arg, &argval, &arglen)) {
-					uintptr_t len = arglen;
-
-					if (!argval)
-						goto trailer_out;
-
-					if (len && argval[len - 1] == ':')
-						len--;
-					string_list_append(&filter_list, argval)->util = (char *)len;
-
-					opts.filter = format_trailer_match_cb;
-					opts.filter_data = &filter_list;
-					opts.only_trailers = 1;
-				} else if (match_placeholder_arg_value(arg, "separator", &arg, &argval, &arglen)) {
-					char *fmt;
-
-					strbuf_reset(&sepbuf);
-					fmt = xstrndup(argval, arglen);
-					strbuf_expand(&sepbuf, fmt, strbuf_expand_literal_cb, NULL);
-					free(fmt);
-					opts.separator = &sepbuf;
-				} else if (!match_placeholder_bool_arg(arg, "only", &arg, &opts.only_trailers) &&
-					   !match_placeholder_bool_arg(arg, "unfold", &arg, &opts.unfold) &&
-					   !match_placeholder_bool_arg(arg, "valueonly", &arg, &opts.value_only))
-					break;
-			}
+			if (format_set_trailers_options(&opts, &filter_list, &sepbuf, &arg, &unused))
+				goto trailer_out;
 		}
 		if (*arg == ')') {
 			format_trailers_from_commit(sb, msg + c->subject_off, &opts);
@@ -1460,6 +1482,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 	trailer_out:
 		string_list_clear(&filter_list, 0);
 		strbuf_release(&sepbuf);
+		free((char *)unused);
 		return ret;
 	}
 
diff --git a/pretty.h b/pretty.h
index 071f2fb8e4..cfe2e8b39b 100644
--- a/pretty.h
+++ b/pretty.h
@@ -6,6 +6,7 @@
 
 struct commit;
 struct strbuf;
+struct process_trailer_options;
 
 /* Commit formats */
 enum cmit_fmt {
@@ -139,4 +140,14 @@ const char *format_subject(struct strbuf *sb, const char *msg,
 /* Check if "cmit_fmt" will produce an empty output. */
 int commit_format_is_empty(enum cmit_fmt);
 
+/*
+ * Set values of fields in "struct process_trailer_options"
+ * according to trailers arguments.
+ */
+int format_set_trailers_options(struct process_trailer_options *opts,
+				struct string_list *filter_list,
+				struct strbuf *sepbuf,
+				const char **arg,
+				const char **err);
+
 #endif /* PRETTY_H */
-- 
gitgitgadget

[PATCH v3 4/4] ref-filter: using pretty.c logic for trailers

[Hariom Verma via GitGitGadget]

From: 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>
---
 Documentation/git-for-each-ref.txt |  36 +++++++--
 ref-filter.c                       |  35 ++++-----
 t/t6300-for-each-ref.sh            | 115 +++++++++++++++++++++++++----
 3 files changed, 151 insertions(+), 35 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 2ea71c5f6c..f8e15916bc 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -254,11 +254,37 @@ contents:lines=N::
 	The first `N` lines of the message.
 
 Additionally, the trailers as interpreted by linkgit:git-interpret-trailers[1]
-are obtained as `trailers` (or by using the historical alias
-`contents:trailers`).  Non-trailer lines from the trailer block can be omitted
-with `trailers:only`. Whitespace-continuations can be removed from trailers so
-that each trailer appears on a line by itself with its full content with
-`trailers:unfold`. Both can be used together as `trailers:unfold,only`.
+are obtained as `trailers[:options]` (or by using the historical alias
+`contents:trailers[:options]`). Valid [:option] are:
+** 'key=<K>': only show trailers with specified key. Matching is done
+   case-insensitively and trailing colon is optional. If option is
+   given multiple times trailer lines matching any of the keys are
+   shown. This option automatically enables the `only` option so that
+   non-trailer lines in the trailer block are hidden. If that is not
+   desired it can be disabled with `only=false`.  E.g.,
+   `%(trailers:key=Reviewed-by)` shows trailer lines with key
+   `Reviewed-by`.
+** '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.
+** 'separator=<SEP>': specify a separator inserted between trailer
+   lines. When this option is not given each trailer line is
+   terminated with a line feed character. The string SEP may contain
+   the literal formatting codes described above. To use comma as
+   separator one must use `%x2C` as it would otherwise be parsed as
+   next option. If separator option is given multiple times only the
+   last one is used. E.g., `%(trailers:key=Ticket,separator=%x2C )`
+   shows all trailer lines whose key is "Ticket" separated by a comma
+   and a space.
+** '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.
+** 'valueonly[=val]': skip over the key part of the trailer line and only
+   show the value part. Also this optionally allows explicit value.
 
 For sorting purposes, fields with numeric values sort in numeric order
 (`objectsize`, `authordate`, `committerdate`, `creatordate`, `taggerdate`).
diff --git a/ref-filter.c b/ref-filter.c
index 8ba0e31915..20f5b829ee 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -67,6 +67,11 @@ struct refname_atom {
 	int lstrip, rstrip;
 };
 
+struct ref_trailer_buf {
+	struct string_list filter_list;
+	struct strbuf sepbuf;
+} ref_trailer_buf;
+
 static struct expand_data {
 	struct object_id oid;
 	enum object_type type;
@@ -307,28 +312,24 @@ static int subject_atom_parser(const struct ref_format *format, struct used_atom
 static int trailers_atom_parser(const struct ref_format *format, struct used_atom *atom,
 				const char *arg, struct strbuf *err)
 {
-	struct string_list params = STRING_LIST_INIT_DUP;
-	int i;
-
 	atom->u.contents.trailer_opts.no_divider = 1;
-
 	if (arg) {
-		string_list_split(&params, arg, ',', -1);
-		for (i = 0; i < params.nr; i++) {
-			const char *s = params.items[i].string;
-			if (!strcmp(s, "unfold"))
-				atom->u.contents.trailer_opts.unfold = 1;
-			else if (!strcmp(s, "only"))
-				atom->u.contents.trailer_opts.only_trailers = 1;
-			else {
-				strbuf_addf(err, _("unknown %%(trailers) argument: %s"), s);
-				string_list_clear(&params, 0);
-				return -1;
-			}
+		const char *argbuf = xstrfmt("%s)", arg);
+		const char *err_arg = NULL;
+
+		if (format_set_trailers_options(&atom->u.contents.trailer_opts,
+			&ref_trailer_buf.filter_list,
+			&ref_trailer_buf.sepbuf,
+			&argbuf, &err_arg)) {
+			if (!err_arg)
+				strbuf_addf(err, _("expected %%(trailers:key=<value>)"));
+			else
+				strbuf_addf(err, _("unknown %%(trailers) argument: %s"), err_arg);
+			free((char *)err_arg);
+			return -1;
 		}
 	}
 	atom->u.contents.option = C_TRAILERS;
-	string_list_clear(&params, 0);
 	return 0;
 }
 
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index fdf2c442c5..664af8588a 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -786,14 +786,32 @@ test_expect_success '%(trailers:unfold) unfolds trailers' '
 	test_cmp expect actual
 '
 
-test_expect_success '%(trailers:only) shows only "key: value" trailers' '
+test_show_key_value_trailers () {
+	option="$1"
+	test_expect_success "%($option) shows only 'key: value' trailers" '
+		{
+			grep -v patch.description <trailers &&
+			echo
+		} >expect &&
+		git for-each-ref --format="%($option)" refs/heads/master >actual &&
+		test_cmp expect actual &&
+		git for-each-ref --format="%(contents:$option)" refs/heads/master >actual &&
+		test_cmp expect actual
+	'
+}
+
+test_show_key_value_trailers 'trailers:only'
+test_show_key_value_trailers 'trailers:only=no,only=true'
+test_show_key_value_trailers 'trailers:only=yes'
+
+test_expect_success '%(trailers:only=no) shows all trailers' '
 	{
-		grep -v patch.description <trailers &&
+		cat trailers &&
 		echo
 	} >expect &&
-	git for-each-ref --format="%(trailers:only)" refs/heads/master >actual &&
+	git for-each-ref --format="%(trailers:only=no)" refs/heads/master >actual &&
 	test_cmp expect actual &&
-	git for-each-ref --format="%(contents:trailers:only)" refs/heads/master >actual &&
+	git for-each-ref --format="%(contents:trailers:only=no)" refs/heads/master >actual &&
 	test_cmp expect actual
 '
 
@@ -812,17 +830,88 @@ test_expect_success '%(trailers:only) and %(trailers:unfold) work together' '
 	test_cmp actual actual
 '
 
-test_expect_success '%(trailers) rejects unknown trailers arguments' '
-	# error message cannot be checked under i18n
-	cat >expect <<-EOF &&
-	fatal: unknown %(trailers) argument: unsupported
-	EOF
-	test_must_fail git for-each-ref --format="%(trailers:unsupported)" 2>actual &&
-	test_i18ncmp expect actual &&
-	test_must_fail git for-each-ref --format="%(contents:trailers:unsupported)" 2>actual &&
-	test_i18ncmp expect actual
+test_trailer_option() {
+	title="$1"
+	option="$2"
+	expect="$3"
+	test_expect_success "$title" '
+		echo $expect >expect &&
+		git for-each-ref --format="%($option)" refs/heads/master >actual &&
+		test_cmp expect actual &&
+		git for-each-ref --format="%(contents:$option)" refs/heads/master >actual &&
+		test_cmp expect actual
+	'
+}
+
+test_trailer_option '%(trailers:key=foo) shows that trailer' \
+	'trailers:key=Signed-off-by' 'Signed-off-by: A U Thor <author@example.com>\n'
+test_trailer_option '%(trailers:key=foo) is case insensitive' \
+	'trailers:key=SiGned-oFf-bY' 'Signed-off-by: A U Thor <author@example.com>\n'
+test_trailer_option '%(trailers:key=foo:) trailing colon also works' \
+	'trailers:key=Signed-off-by:' 'Signed-off-by: A U Thor <author@example.com>\n'
+test_trailer_option '%(trailers:key=foo) multiple keys' \
+	'trailers:key=Reviewed-by:,key=Signed-off-by' 'Reviewed-by: A U Thor <author@example.com>\nSigned-off-by: A U Thor <author@example.com>\n'
+test_trailer_option '%(trailers:key=nonexistent) becomes empty' \
+	'trailers:key=Shined-off-by:' ''
+
+test_expect_success '%(trailers:key=foo) handles multiple lines even if folded' '
+	{
+		grep -v patch.description <trailers | grep -v Signed-off-by | grep -v Reviewed-by &&
+		echo
+	} >expect &&
+	git for-each-ref --format="%(trailers:key=Acked-by)" refs/heads/master >actual &&
+	test_cmp expect actual &&
+	git for-each-ref --format="%(contents:trailers:key=Acked-by)" refs/heads/master >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(trailers:key=foo,unfold) properly unfolds' '
+	{
+		unfold <trailers | grep Signed-off-by &&
+		echo
+	} >expect &&
+	git for-each-ref --format="%(trailers:key=Signed-Off-by,unfold)" refs/heads/master >actual &&
+	test_cmp expect actual &&
+	git for-each-ref --format="%(contents:trailers:key=Signed-Off-by,unfold)" refs/heads/master >actual &&
+	test_cmp expect actual
 '
 
+test_expect_success 'pretty format %(trailers:key=foo,only=no) also includes nontrailer lines' '
+	{
+		echo "Signed-off-by: A U Thor <author@example.com>" &&
+		grep patch.description <trailers &&
+		echo
+	} >expect &&
+	git for-each-ref --format="%(trailers:key=Signed-off-by,only=no)" refs/heads/master >actual &&
+	test_cmp expect actual &&
+	git for-each-ref --format="%(contents:trailers:key=Signed-off-by,only=no)" refs/heads/master >actual &&
+	test_cmp expect actual
+'
+
+test_trailer_option '%(trailers:key=foo,valueonly) shows only value' \
+	'trailers:key=Signed-off-by,valueonly' 'A U Thor <author@example.com>\n'
+test_trailer_option '%(trailers:separator) changes separator' \
+	'trailers:separator=%x2C,key=Reviewed-by,key=Signed-off-by:' 'Reviewed-by: A U Thor <author@example.com>,Signed-off-by: A U Thor <author@example.com>'
+
+test_failing_trailer_option () {
+	title="$1"
+	option="$2"
+	error="$3"
+	test_expect_success "$title" '
+		# error message cannot be checked under i18n
+		echo $error >expect &&
+		test_must_fail git for-each-ref --format="%($option)" refs/heads/master 2>actual &&
+		test_i18ncmp expect actual &&
+		test_must_fail git for-each-ref --format="%(contents:$option)" refs/heads/master 2>actual &&
+		test_i18ncmp expect actual
+	'
+}
+
+test_failing_trailer_option '%(trailers:key) without value is error' \
+	'trailers:key' 'fatal: expected %(trailers:key=<value>)'
+test_failing_trailer_option '%(trailers) rejects unknown trailers arguments' \
+	'trailers:unsupported' 'fatal: unknown %(trailers) argument: unsupported'
+
 test_expect_success 'if arguments, %(contents:trailers) shows error if semicolon is missing' '
 	cat >expect <<-EOF &&
 	fatal: unrecognized %(contents) argument: trailersonly
-- 
gitgitgadget

Re: [PATCH v3 0/4] [GSoC] Fix trailers atom bug and improved tests

[Junio C Hamano]

"Hariom Verma via GitGitGadget" <gitgitgadget@gmail.com> writes:

> 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 t6300 and made tests more compact.

I think we should focus on completing the first two patches and send
them to 'next' down to 'master', before extending the scope of the
topic by piling more patches that do not have to be part of the
topic.  Let's take the other two separately from the first two.

Thanks.

Re: [PATCH v3 0/4] [GSoC] Fix trailers atom bug and improved tests

[Hariom verma]

Hi,

On Sat, Aug 22, 2020 at 3:26 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Hariom Verma via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > 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 t6300 and made tests more compact.
>
> I think we should focus on completing the first two patches and send
> them to 'next' down to 'master', before extending the scope of the
> topic by piling more patches that do not have to be part of the
> topic.  Let's take the other two separately from the first two.

Sure.

Thanks,
Hariom