threads / patch / 54068

patch, 2 partst6300: unify %(trailers) and %(contents:trailers) tests

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

## tl;dr

31 messages between Aug 19, 2020 and Aug 26, 2020. Diffs are folded; open one to read it.

replies: 30people: 5as markdown or json

Hariom Verma via GitGitGadget· Aug 19, 2020, 12:52 UTC · lore

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

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
Hariom Verma via GitGitGadget· Aug 19, 2020, 12:52 UTC · re: Hariom Verma via GitGitGadget · lore
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(-)
Show changes to t/t6300-for-each-ref.sh +11 −39
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
Junio C Hamano· Aug 19, 2020, 17:31 UTC · re: Hariom Verma via GitGitGadget · lore

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

"Hariom Verma via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 18 quoted lines
> 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.

Show 12 quoted lines
>  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 &&
Show 16 quoted lines
> @@ -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.
Hariom verma· Aug 21, 2020, 10:03 UTC · re: Junio C Hamano · lore

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

Hi,
On Wed, Aug 19, 2020 at 11:01 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 25 quoted lines
>
> "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

Hariom Verma via GitGitGadget· Aug 19, 2020, 12:52 UTC · re: Hariom Verma via GitGitGadget · lore

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

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(-)
Show changes to 2 files +27 −3

ref-filter.c, t/t6300-for-each-ref.sh

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
Junio C Hamano· Aug 19, 2020, 17:55 UTC · re: Hariom Verma via GitGitGadget · lore

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

"Hariom Verma via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 15 quoted lines
> From: Hariom Verma <hariom18599@gmail.com>
>
> The 'contents' atom does not show any error if used with 'trailers'
> atom and semicolon is missing before trailers arguments.
>
> e.g %(contents:trailersonly) works, while it shouldn't.
>
> It is definitely not an expected behavior.
>
> Let's fix this bug.
>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Mentored-by: Heba Waly <heba.waly@gmail.com>
> Signed-off-by: Hariom Verma <hariom18599@gmail.com>
> ---

Nice spotting. 7a5edbdb (ref-filter.c: parse trailers arguments with %(contents) atom, 2017-10-01) talks about being deliberate about the case where skip_prefix(":") does not find a colon after the "trailers" token, but from the message it is clear that it expected that the case happens only when "trailers" is at the end of the string.

The new helper that is overly verbose and may be overkill.
Shouldn't this be clear enough, equivalent and sufficient?
	else if (skip_prefix(arg, "trailers", &arg) &&
		 (!*arg || *arg == ':'))) {
		if (trailers_atom_parser(...);

That is, we not just make sure the string begins with "trailers", but also make sure it either (1) ends the string (i.e. the token is just "trailers"), or (2) is followed by a colon ':', before entering the block to handle "trailers[:anything]". If we later add a new atom "trailersonly", that will not be handled here, but elsewhere in the "else if" cascade.

Show 43 quoted lines
>  ref-filter.c            | 21 ++++++++++++++++++---
>  t/t6300-for-each-ref.sh |  9 +++++++++
>  2 files changed, 27 insertions(+), 3 deletions(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index ba85869755..dc31fbbe51 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -332,6 +332,22 @@ static int trailers_atom_parser(const struct ref_format *format, struct used_ato
>  	return 0;
>  }
>  
> +static int check_format_field(const char *arg, const char *field, const char **option)
> +{
> +	const char *opt;
> +	if (skip_prefix(arg, field, &opt)) {
> +		if (*opt == '\0') {
> +			*option = NULL;
> +			return 1;
> +		}
> +		else if (*opt == ':') {
> +			*option = ++opt;
> +			return 1;
> +		}
> +	}
> +	return 0;
> +}
> +
>  static int contents_atom_parser(const struct ref_format *format, struct used_atom *atom,
>  				const char *arg, struct strbuf *err)
>  {
> @@ -345,9 +361,8 @@ static int contents_atom_parser(const struct ref_format *format, struct used_ato
>  		atom->u.contents.option = C_SIG;
>  	else if (!strcmp(arg, "subject"))
>  		atom->u.contents.option = C_SUB;
> -	else if (skip_prefix(arg, "trailers", &arg)) {
> -		skip_prefix(arg, ":", &arg);
> -		if (trailers_atom_parser(format, atom, *arg ? arg : NULL, err))
> +	else if (check_format_field(arg, "trailers", &arg)) {
> +		if (trailers_atom_parser(format, atom, arg, err))
>  			return -1;
>  	} else if (skip_prefix(arg, "lines=", &arg)) {
>  		atom->u.contents.option = C_LINES;
Junio C Hamano· Aug 19, 2020, 19:07 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 32 quoted lines
> "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.

Show 15 quoted lines
>> +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.
Eric Sunshine· Aug 19, 2020, 19:39 UTC · re: Junio C Hamano · lore

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

On Wed, Aug 19, 2020 at 3:07 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 10 quoted lines
> 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).

Junio C Hamano· Aug 19, 2020, 22:08 UTC · re: Eric Sunshine · lore

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

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 26 quoted lines
> 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.
Hariom verma· Aug 20, 2020, 17:19 UTC · re: Junio C Hamano · lore

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

Hi,
On Thu, Aug 20, 2020 at 3:38 AM Junio C Hamano <gitster@pobox.com> wrote:
Show 31 quoted lines
>
> 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

Hariom Verma via GitGitGadget· Aug 21, 2020, 10:11 UTC · re: Hariom Verma via GitGitGadget · lore

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

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
Hariom Verma via GitGitGadget· Aug 21, 2020, 10:11 UTC · re: Hariom Verma via GitGitGadget · lore

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

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(-)
Show changes to t/t6300-for-each-ref.sh +14 −42
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
Hariom Verma via GitGitGadget· Aug 21, 2020, 10:11 UTC · re: Hariom Verma via GitGitGadget · lore

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

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(-)
Show changes to 2 files +27 −3

ref-filter.c, t/t6300-for-each-ref.sh

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
Eric Sunshine· Aug 21, 2020, 16:56 UTC · re: Hariom Verma via GitGitGadget · lore

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

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/ ?
Show 25 quoted lines
> 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...

Show 7 quoted lines
> @@ -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.

Show 6 quoted lines
> +       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
> +'
Junio C Hamano· Aug 21, 2020, 19:17 UTC · re: Eric Sunshine · lore

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

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 12 quoted lines
> ...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.
Show 5 quoted lines
>> 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.
Show 5 quoted lines
>
>> +       # 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.
Show 6 quoted lines
>> +       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
>> +'
Hariom verma· Aug 23, 2020, 19:25 UTC · re: Junio C Hamano · lore

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

Hi,
On Sat, Aug 22, 2020 at 12:47 AM Junio C Hamano <gitster@pobox.com> wrote:
Show 17 quoted lines
>
> 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
Eric Sunshine· Aug 24, 2020, 03:49 UTC · re: Hariom verma · lore

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

On Sun, Aug 23, 2020 at 8:56 PM Hariom verma <hariom18599@gmail.com> wrote:
Show 26 quoted lines
> 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 (...) {`
Hariom verma· Aug 24, 2020, 23:32 UTC · re: Eric Sunshine · lore

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

Hi,
On Mon, Aug 24, 2020 at 9:19 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 39 quoted lines
>
> 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.
Show 10 quoted lines
> 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.

Show 12 quoted lines
> 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.
Show 22 quoted lines
> 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

Christian Couder· Aug 26, 2020, 06:18 UTC · re: Hariom verma · lore

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

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:
Show 22 quoted lines
> > > 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.
Show 13 quoted lines
> > 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()".
Show 18 quoted lines
> > 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.

Show 30 quoted lines
> 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.

Christian Couder· Aug 26, 2020, 06:22 UTC · re: Christian Couder · lore

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

On Wed, Aug 26, 2020 at 8:18 AM Christian Couder <christian.couder@gmail.com> wrote:

Show 6 quoted lines
> 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()".

Hariom verma· Aug 26, 2020, 15:18 UTC · re: Christian Couder · lore

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

Hi,

On Wed, Aug 26, 2020 at 11:48 AM Christian Couder <christian.couder@gmail.com> wrote:

Show 32 quoted lines
>
> 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.
Show 57 quoted lines
> > > 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

Hariom Verma via GitGitGadget· Aug 21, 2020, 21:06 UTC · re: Hariom Verma via GitGitGadget · lore

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

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
Hariom Verma via GitGitGadget· Aug 21, 2020, 21:06 UTC · re: Hariom Verma via GitGitGadget · lore

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

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(-)
Show changes to t/t6300-for-each-ref.sh +14 −42
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
Hariom Verma via GitGitGadget· Aug 21, 2020, 21:06 UTC · re: Hariom Verma via GitGitGadget · lore

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

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(-)
Show changes to 2 files +13 −3

ref-filter.c, t/t6300-for-each-ref.sh

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
Eric Sunshine· Aug 21, 2020, 21:13 UTC · re: Hariom Verma via GitGitGadget · lore

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

On Fri, Aug 21, 2020 at 5:06 PM Hariom Verma via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 10 quoted lines
> 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.

Show 13 quoted lines
> 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).
Hariom verma· Aug 21, 2020, 16:19 UTC · re: Eric Sunshine · lore

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

Hi Eric,
On Sat, Aug 22, 2020 at 2:43 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 16 quoted lines
>
> 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.
Show 16 quoted lines
> > 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.
Show 5 quoted lines
> > 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

Junio C Hamano· Aug 21, 2020, 21:54 UTC · re: Eric Sunshine · lore

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

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 15 quoted lines
> 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.
Show 22 quoted lines
>> 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.
Hariom Verma via GitGitGadget· Aug 21, 2020, 21:06 UTC · re: Hariom Verma via GitGitGadget · lore

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

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(-)
Show changes to 3 files +151 −35

Documentation/git-for-each-ref.txt, ref-filter.c, t/t6300-for-each-ref.sh

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
Hariom Verma via GitGitGadget· Aug 21, 2020, 21:06 UTC · re: Hariom Verma via GitGitGadget · lore

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

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
Show changes to 3 files +64 −30

Hariom, pretty.c, pretty.h

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
Junio C Hamano· Aug 21, 2020, 21:56 UTC · re: Hariom Verma via GitGitGadget · lore

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

"Hariom Verma via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 5 quoted lines
> 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.
Hariom verma· Aug 22, 2020, 14:03 UTC · re: Junio C Hamano · lore

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

Hi,
On Sat, Aug 22, 2020 at 3:26 AM Junio C Hamano <gitster@pobox.com> wrote:
Show 13 quoted lines
>
> "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

← back to recent threads