patch, 2 partspretty.c: refactor trailer logic to `format_set_trailers_options()`

Currently, there exists a separate logic for %(trailers) in "pretty.{c,h}" and "ref-filter.{c,h}". Both are actually doing the same thing, why not use the same code for both of them?

This patch series is focused on unifying the "%(trailers)" logic for both 'pretty.{c,h}' and 'ref-filter.{c,h}'. So, we can have one logic for trailers.

Note: We used pretty's logic in ref-filter because it supports more
%(trailers) options.

Hariom Verma (2):
  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                       |  35 ++++-----
 t/t6300-for-each-ref.sh            | 115 +++++++++++++++++++++++++----
 6 files changed, 215 insertions(+), 65 deletions(-)
 create mode 100644 Hariom Verma via GitGitGadget

base-commit: 3a238e539bcdfe3f9eb5010fd218640c1b499f7a
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-726%2Fharry-hov%2Funify-trailers-logic-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-726/harry-hov/unify-trailers-logic-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/726

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

Am 05.09.20 um 21:48 schrieb Hariom Verma via GitGitGadget:

This error reporting feature is probably worth its own separate commit.

This function is supposed to allocate *err, so it shouldn't be const.

This is owned by this block and released a few lines down, so it shouldn't be const.

Weird indentation of the second and later strcmp() calls. And they duplicate the checks done by the parsing code below, right?

Here's the *err allocation mentioned above.

You didn't add this, but I wonder what's up with the arglen-in-util trickery here -- strcasecmp() might even be faster.

And also why all this expensive-looking %(trailers) parsing is done for each commit and not just once.

(Both outside the scope of this series, unless you want to address them as well.)

Weird indentation of the second and third match_placeholder_bool_arg() calls.

If you need to capture an invalid argument value, you can do that here without adding duplicate checks.

This function is going to free() unused later, so it shouldn't be const.

The match_placeholder_bool_arg() calls were still properly indented here.

Here is the free() call I mentioned above.

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

Currently, there exists a separate logic for %(trailers) in "pretty.{c,h}" and "ref-filter.{c,h}". Both are actually doing the same thing, why not use the same code for both of them?

This is the 2nd version of the patch series that I sent few months back. It is focused on unifying the "%(trailers)" logic for both 'pretty.{c,h}' and 'ref-filter.{c,h}'. So, we can have one logic for trailers.

v2 changes:

 * Contains Improvements as suggested by "René Scharfe" l.s.r@web.de 1
   [https://public-inbox.org/git/bf4423d5-c0ee-6bef-59ff-fcde003ec463@web.de/]
 * A new trailer option was introduced to pretty.c when I was absent i.e
   "key_value_separator". Updated the patch series with latest changes.

Sorry for taking a long break.

Link to previous version: https://public-inbox.org/git/pull.726.git.1599335291.gitgitgadget@gmail.com/

Hariom Verma (3):
  pretty.c: refactor trailer logic to `format_set_trailers_options()`
  pretty.c: capture invalid trailer argument
  ref-filter: use pretty.c logic for trailers

 Documentation/git-for-each-ref.txt |  39 ++++++++--
 pretty.c                           |  95 +++++++++++++----------
 pretty.h                           |  12 +++
 ref-filter.c                       |  36 +++++----
 t/t6300-for-each-ref.sh            | 119 +++++++++++++++++++++++++----
 5 files changed, 228 insertions(+), 73 deletions(-)

base-commit: e6362826a0409539642a5738db61827e5978e2e4
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-726%2Fharry-hov%2Funify-trailers-logic-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-726/harry-hov/unify-trailers-logic-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/726

Range-diff vs v1:

 1:  4180f54c1de ! 1:  fc5fd5217df pretty.c: refactor trailer logic to `format_set_trailers_options()`
     @@ Commit message
          pretty.c: refactor trailer logic to `format_set_trailers_options()`
      
          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.
     +    `format_set_trailers_options()`. This change will allow us to reuse
     +    the same logic in other places.
      
          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 (new) ##
     -
       ## pretty.c ##
      @@ pretty.c: static int format_trailer_match_cb(const struct strbuf *key, void *ud)
       	return 0;
     @@ pretty.c: static int format_trailer_match_cb(const struct strbuf *key, void *ud)
      +int format_set_trailers_options(struct process_trailer_options *opts,
      +				struct string_list *filter_list,
      +				struct strbuf *sepbuf,
     -+				const char **arg,
     -+				const char **err)
     ++				struct strbuf *kvsepbuf,
     ++				const char **arg)
      +{
      +	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;
      +
     @@ pretty.c: static int format_trailer_match_cb(const struct strbuf *key, void *ud)
      +			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);
     ++			char *fmt;
     ++			fmt = xstrndup(argval, arglen);
      +			strbuf_expand(sepbuf, fmt, strbuf_expand_literal_cb, NULL);
      +			free(fmt);
      +			opts->separator = sepbuf;
     ++		} else if (match_placeholder_arg_value(*arg, "key_value_separator", arg, &argval, &arglen)) {
     ++			char *fmt;
     ++			fmt = xstrndup(argval, arglen);
     ++			strbuf_expand(kvsepbuf, fmt, strbuf_expand_literal_cb, NULL);
     ++			free(fmt);
     ++			opts->key_value_separator = kvsepbuf;
      +		} 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))
     ++			   !match_placeholder_bool_arg(*arg, "unfold", arg, &opts->unfold) &&
     ++			   !match_placeholder_bool_arg(*arg, "keyonly", arg, &opts->key_only) &&
     ++			   !match_placeholder_bool_arg(*arg, "valueonly", arg, &opts->value_only))
      +			break;
      +	}
      +	return 0;
     @@ pretty.c: static int format_trailer_match_cb(const struct strbuf *key, void *ud)
       				const char *placeholder,
       				void *context)
      @@ pretty.c: 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++;
     @@ pretty.c: static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
      -					strbuf_expand(&sepbuf, fmt, strbuf_expand_literal_cb, NULL);
      -					free(fmt);
      -					opts.separator = &sepbuf;
     +-				} else if (match_placeholder_arg_value(arg, "key_value_separator", &arg, &argval, &arglen)) {
     +-					char *fmt;
     +-
     +-					strbuf_reset(&kvsepbuf);
     +-					fmt = xstrndup(argval, arglen);
     +-					strbuf_expand(&kvsepbuf, fmt, strbuf_expand_literal_cb, NULL);
     +-					free(fmt);
     +-					opts.key_value_separator = &kvsepbuf;
      -				} 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, "keyonly", &arg, &opts.key_only) &&
      -					   !match_placeholder_bool_arg(arg, "valueonly", &arg, &opts.value_only))
      -					break;
      -			}
     -+			if (format_set_trailers_options(&opts, &filter_list, &sepbuf, &arg, &unused))
     ++			if (format_set_trailers_options(&opts, &filter_list, &sepbuf, &kvsepbuf, &arg))
      +				goto trailer_out;
       		}
       		if (*arg == ')') {
       			format_trailers_from_commit(sb, msg + c->subject_off, &opts);
     -@@ pretty.c: 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;
     - 	}
     - 
      
       ## pretty.h ##
      @@
     @@ pretty.h
       
       /* Commit formats */
       enum cmit_fmt {
     -@@ pretty.h: 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);
     +@@ pretty.h: int commit_format_is_empty(enum cmit_fmt);
     + /* Make subject of commit message suitable for filename */
     + void format_sanitized_subject(struct strbuf *sb, const char *msg, size_t len);
       
      +/*
      + * 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);
     ++			struct string_list *filter_list,
     ++			struct strbuf *sepbuf,
     ++			struct strbuf *kvsepbuf,
     ++			const char **arg);
      +
       #endif /* PRETTY_H */
 -:  ----------- > 2:  245e48eb683 pretty.c: capture invalid trailer argument
 2:  a3e16298262 ! 3:  7b8cfb2721c ref-filter: using pretty.c logic for trailers
     @@ Metadata
      Author: Hariom Verma <hariom18599@gmail.com>
      
       ## Commit message ##
     -    ref-filter: using pretty.c logic for trailers
     +    ref-filter: use pretty.c logic for trailers
      
          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
     +      :separator=<SEP> - inserted between trailer lines.
     +      :key_value_separator=<SEP> - inserted between trailer lines
      
          Enhancement to existing options(now can take value and its optional):
            :only[=val]
     @@ Documentation/git-for-each-ref.txt: contents:lines=N::
      +** '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.
     ++   the literal formatting codes. 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.
     ++** 'key_value_separator=<SEP>': specify a separator inserted between
     ++   key and value. The string SEP may contain the literal formatting codes.
     ++   E.g., `%(trailers:key=Ticket,key_value_separator=%x2C)` shows all trailer
     ++   lines whose key is "Ticket" with key and value separated by a comma.
      +** '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.,
     @@ ref-filter.c: struct refname_atom {
      +struct ref_trailer_buf {
      +	struct string_list filter_list;
      +	struct strbuf sepbuf;
     ++	struct strbuf kvsepbuf;
      +} ref_trailer_buf;
      +
       static struct expand_data {
     @@ ref-filter.c: static int subject_atom_parser(const struct ref_format *format, st
      -	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++) {
     @@ ref-filter.c: static int subject_atom_parser(const struct ref_format *format, st
      -				return -1;
      -			}
      +		const char *argbuf = xstrfmt("%s)", arg);
     -+		const char *err_arg = NULL;
     ++		char *invalid_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)
     ++			&ref_trailer_buf.kvsepbuf,
     ++			&argbuf, &invalid_arg)) {
     ++			if (!invalid_arg)
      +				strbuf_addf(err, _("expected %%(trailers:key=<value>)"));
      +			else
     -+				strbuf_addf(err, _("unknown %%(trailers) argument: %s"), err_arg);
     -+			free((char *)err_arg);
     ++				strbuf_addf(err, _("unknown %%(trailers) argument: %s"), invalid_arg);
     ++			free((char *)invalid_arg);
      +			return -1;
       		}
       	}
     @@ t/t6300-for-each-ref.sh: test_expect_success '%(trailers:unfold) unfolds trailer
      +			grep -v patch.description <trailers &&
      +			echo
      +		} >expect &&
     -+		git for-each-ref --format="%($option)" refs/heads/master >actual &&
     ++		git for-each-ref --format="%($option)" refs/heads/main >actual &&
      +		test_cmp expect actual &&
     -+		git for-each-ref --format="%(contents:$option)" refs/heads/master >actual &&
     ++		git for-each-ref --format="%(contents:$option)" refs/heads/main >actual &&
      +		test_cmp expect actual
      +	'
      +}
     @@ t/t6300-for-each-ref.sh: test_expect_success '%(trailers:unfold) unfolds trailer
      +		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 &&
     +-	git for-each-ref --format="%(trailers:only)" refs/heads/main >actual &&
     ++	git for-each-ref --format="%(trailers:only=no)" refs/heads/main >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 &&
     +-	git for-each-ref --format="%(contents:trailers:only)" refs/heads/main >actual &&
     ++	git for-each-ref --format="%(contents:trailers:only=no)" refs/heads/main >actual &&
       	test_cmp expect actual
       '
       
     @@ t/t6300-for-each-ref.sh: test_expect_success '%(trailers:only) and %(trailers:un
      +	expect="$3"
      +	test_expect_success "$title" '
      +		echo $expect >expect &&
     -+		git for-each-ref --format="%($option)" refs/heads/master >actual &&
     ++		git for-each-ref --format="%($option)" refs/heads/main >actual &&
      +		test_cmp expect actual &&
     -+		git for-each-ref --format="%(contents:$option)" refs/heads/master >actual &&
     ++		git for-each-ref --format="%(contents:$option)" refs/heads/main >actual &&
      +		test_cmp expect actual
      +	'
      +}
     @@ t/t6300-for-each-ref.sh: test_expect_success '%(trailers:only) and %(trailers:un
      +		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 &&
     ++	git for-each-ref --format="%(trailers:key=Acked-by)" refs/heads/main >actual &&
      +	test_cmp expect actual &&
     -+	git for-each-ref --format="%(contents:trailers:key=Acked-by)" refs/heads/master >actual &&
     ++	git for-each-ref --format="%(contents:trailers:key=Acked-by)" refs/heads/main >actual &&
      +	test_cmp expect actual
      +'
      +
     @@ t/t6300-for-each-ref.sh: test_expect_success '%(trailers:only) and %(trailers:un
      +		unfold <trailers | grep Signed-off-by &&
      +		echo
      +	} >expect &&
     -+	git for-each-ref --format="%(trailers:key=Signed-Off-by,unfold)" refs/heads/master >actual &&
     ++	git for-each-ref --format="%(trailers:key=Signed-Off-by,unfold)" refs/heads/main >actual &&
      +	test_cmp expect actual &&
     -+	git for-each-ref --format="%(contents:trailers:key=Signed-Off-by,unfold)" refs/heads/master >actual &&
     ++	git for-each-ref --format="%(contents:trailers:key=Signed-Off-by,unfold)" refs/heads/main >actual &&
      +	test_cmp expect actual
       '
       
     @@ t/t6300-for-each-ref.sh: test_expect_success '%(trailers:only) and %(trailers:un
      +		grep patch.description <trailers &&
      +		echo
      +	} >expect &&
     -+	git for-each-ref --format="%(trailers:key=Signed-off-by,only=no)" refs/heads/master >actual &&
     ++	git for-each-ref --format="%(trailers:key=Signed-off-by,only=no)" refs/heads/main >actual &&
      +	test_cmp expect actual &&
     -+	git for-each-ref --format="%(contents:trailers:key=Signed-off-by,only=no)" refs/heads/master >actual &&
     ++	git for-each-ref --format="%(contents:trailers:key=Signed-off-by,only=no)" refs/heads/main >actual &&
      +	test_cmp expect actual
      +'
      +
     @@ t/t6300-for-each-ref.sh: test_expect_success '%(trailers:only) and %(trailers:un
      +	'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_trailer_option '%(trailers:key_value_separator) changes key-value separator' \
     ++	'trailers:key_value_separator=%x2C,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:separator,key_value_separator) changes both separators' \
     ++	'trailers:separator=%x2C,key_value_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"
     @@ t/t6300-for-each-ref.sh: test_expect_success '%(trailers:only) and %(trailers:un
      +	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_must_fail git for-each-ref --format="%($option)" refs/heads/main 2>actual &&
      +		test_i18ncmp expect actual &&
     -+		test_must_fail git for-each-ref --format="%(contents:$option)" refs/heads/master 2>actual &&
     ++		test_must_fail git for-each-ref --format="%(contents:$option)" refs/heads/main 2>actual &&
      +		test_i18ncmp expect actual
      +	'
      +}

From: Hariom Verma <hariom18599@gmail.com>

Refactored trailers formatting logic inside pretty.c to a new function `format_set_trailers_options()`. This change will allow us to reuse the same logic in other places.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Heba Waly <heba.waly@gmail.com>
Signed-off-by: Hariom Verma <hariom18599@gmail.com>
---
 pretty.c | 85 ++++++++++++++++++++++++++++++--------------------------
 pretty.h | 11 ++++++++
 2 files changed, 57 insertions(+), 39 deletions(-)

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

The convention in this codebase is to signal unusual/error return with a negative value, especially when a successful exit is signaled by returning 0. Perhaps return -1 from here instead?

In these two else-if clauses, the original code clears sepbuf and kvsepbuf before calling strbuf_expand(), but this one does not.

As strbuf_expand() is an appending function, this distinction would matter if the for(;;) loop causes these two else-if clauses to be entered twice. The original code will implement the "last one wins" semantics, and this new one acumulates what it sees.

Intended? If so, the reason why we want the accumulating semantics, instead of the last-one-wins we've been using, needs to be explained in the log message.

Here you can see that the original clears what was in sepbuf before reading the separator anew.

Thanks.

From: Hariom Verma <hariom18599@gmail.com>

As we would like to use this same logic in ref-filter, it's nice to get invalid trailer argument. This will allow us to print precise 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>
---
 pretty.c | 18 ++++++++++++++----
 pretty.h |  3 ++-
 2 files changed, 16 insertions(+), 5 deletions(-)

On Fri, Jan 29, 2021 at 10:15 PM Hariom Verma via GitGitGadget <gitgitgadget@gmail.com> wrote:

Thanks for continuing to work on this!

A space char is missing between "if" and "(". Also no need for "{" and "}". It could just be:

Hi Christian,

On Sat, Jan 30, 2021 at 3:58 AM Christian Couder <christian.couder@gmail.com> wrote:

Thanks for pointing this out. Will fix it.

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

How about doing this only when invalid_arg is not NULL, i.e.

	} else if (!match_placeholder_bool_arg(....) &&
		   ...
        	   !match_placeholder_bool_arg(....)) {
		if (invalid_arg) {
			size_t len = strcspn(*arg, ",)");
			*invalid_arg = xstrndup(*arg, len);
		}
		return -1;
	}

Note that I just used 'len'; when the scope of a variable is so short, it is clear the length of what thing it refers to from the context, and there is no point in using a variable name that long.

In any case, by doing so, the callers that are not interested in the report can just pass NULL, which means ...

... this will become unneeded, and ...

... this will pass NULL, and ...

... this will become unneeded.

Hi,

On Sat, Jan 30, 2021 at 5:31 AM Junio C Hamano <gitster@pobox.com> wrote:

Sounds like an improvement to the current version. Will change.

Thanks, Hariom Verma

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

Would the existing caller be OK with this change?

It used to be that this parsing code simply _ignored_ unrecognised trailer keyword because the very original just did a "break" and fell though, but now because this returns non-zero, it causes the caller rewritten in the patch [v2 1/3] to "goto trailer_out".

It is not clear from your proposed log message if this would result in behaviour change, and if so if that behaviour change was intended.

I suspect that the behaviour change the code implements may be OK, but the log message needs to discuss why it is OK.

Thanks.

Hi,

On Sat, Jan 30, 2021 at 5:37 AM Junio C Hamano <gitster@pobox.com> wrote:

Yes, I made sure of that.

I should have included that change in behaviour in the commit message. Will change that too.

Thanks, Hariom Verma

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.
  :key_value_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 |  39 ++++++++--
 ref-filter.c                       |  36 +++++----
 t/t6300-for-each-ref.sh            | 119 +++++++++++++++++++++++++----
 3 files changed, 160 insertions(+), 34 deletions(-)

On Fri, Jan 29 2021, Hariom Verma via GitGitGadget wrote:

Given that the goal of this series is to unify this parsing logic between log/for-each-ref, why do we need to then copy/paste the exact same docs we have in pretty-formats.txt?

At the very least we should move this to pretty-formats-trailers.txt or something, and just include it in both places, or better yet just refer to the relevan parts of "git log"'s man page, no?

And similarly, here we have now mostly duplicated tests for this between here and t/t4205-log-pretty-formats.sh.

I think the right thing to do is to start by moving the tests that are now in t/t4205-log-pretty-formats.sh relevant to this formatting into its own file or something.

Then instead of duplicating the tests here, just prepare them to be changed so that we can add both "git log" and a "git for-each-ref" invocation to some for-loop, so we'll test both.

Hi,

On Sun, Jan 31, 2021 at 2:15 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Ok. I will refer to the trailers part of "pretty-formats"'s man page in "git-for-each-ref"'s man page.

With this unified trailer logic, "git log" and "git for-each-ref" still behave differently. For e.g.: "git log" does nothing for unknown/incorrect trailer option, whereas "git for-each-ref" stops.

Even if we move trailer related tests for both into a new file, I guess we still need to test trailers for both "git log" and "git for-each-ref" separately?

Thanks, Hariom.

On Thu, Feb 04 2021, Hariom verma wrote:

Sure, FWIW you can also (not saying it has to be this) include the same section in both, maybe with some blurb on the top saying it's not different between the two...

We have a few tests that define a test function to test these sorts of cases, t/t3070-wildmatch.sh is one, t/t3800-mktag.sh another.

So you can just do:

    test_trailers A '%(trailers:keyonly)' 'Signed-off-by' 'ERR: error from for-each-ref' # (or whatever)

And make the "test_trailers" function do the common setup, have both "log" and "for-each-ref" look at the "A" tag and assert what their output is, respectively (or an error, or whatever).

I think that's especially valuable in cases where you have similar codepaths, because it makes it easy for both the author and reviewers to eyeball intended an unintended differences.

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

ref-filter.c:74:3: error: symbol 'ref_trailer_buf' was not declared. Should it be static?
    SP refs/packed-backend.c
    SP refs/ref-cache.c

For now, I've queued this on the tip of the topic while queuing; running "make SPARSE_FLAGS=-Wsparse-error sparse" before sending your series out may reduce embarrassment in the future.

Thanks.

 ref-filter.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

With this queued directly on top of master, I am getting these failures.

t6300-for-each-ref.sh                            (Wstat: 256 Tests: 301 Failed: 6)
  Failed tests:  277-280, 285, 287
  Non-zero exit status: 1

Hi,

On Sat, Jan 30, 2021 at 6:58 AM Junio C Hamano <gitster@pobox.com> wrote:

That's strange!

All 301 tests seem to work fine in my system.

I'm clueless about these failures and can't think of any possible reason too.

Is there any way I can re-generate this or can trace the part of code which caused these failures?

Thanks, Hariom.

Junio C Hamano <gitster@pobox.com> writes:

Judging from the way the first failure happens, it appears that some code depends on a kind of shell portability issues?

The failure is under GNU bash, version 5.1.4(1)-release (x86_64-pc-linux-gnu)

Ah, I think I know what is going on.

test_trailer_option() {
	title="$1"
	option="$2"
	expect="$3"
	test_expect_success "$title" '
		echo $expect >expect &&
		git for-each-ref --format="%($option)" refs/heads/main >actual &&
		test_cmp expect actual &&
		git for-each-ref --format="%(contents:$option)" refs/heads/main >actual &&
		test_cmp expect actual
	'
}

Not quoting "$expect" given to 'echo' inside double-quote is criminal, but I do not think that contributes to this particular failure. The problem is in the callers.

test_trailer_option '%(trailers:key=foo) shows that trailer' \
	'trailers:key=Signed-off-by' 'Signed-off-by: A U Thor <author@example.com>\n'

Doing

	expect='foo\n'
	echo $expect

and expecting that somebody would magically turn \n to a true newline is the bug. Not everybody's builtin "echo" works that way.

Here is a quick fix-up. I didn't check which parts are to be blamed on which patch of yours, if any, so you might need to split them up into multiple pieces (i.e. a preliminary clean-up patch, and a patch each to be applied to part N/3 (1 <= N <= 3) of your patch).

 t/t6300-for-each-ref.sh | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

Currently, there exists a separate logic for %(trailers) in "pretty.{c,h}" and "ref-filter.{c,h}". Both are actually doing the same thing, why not use the same code for both of them?

This is the 3rd version of the patch series. It is focused on unifying the "%(trailers)" logic for both 'pretty.{c,h}' and 'ref-filter.{c,h}'. So, we can have one logic for trailers.

v3 changes:

 * replaced echo with printf in the tests failing in previous version for
   consistent behaviour.
 * strbuf_reset() is back in format_set_trailers_options().
 * made struct ref_trailer_buf static.
 * initialised structure variable ref_trailer_buf . so we may not encounter
   any problem while doing strbuf_reset().
 * refer to the trailers part of "git-log"'s man page in
   "git-for-each-ref"'s man page.
 * improved commit messages.

/* TODO */

As suggested by Ævar Arnfjörð Bjarmason avarab@gmail.com here [https://public-inbox.org/git/875z3ep30j.fsf@evledraar.gmail.com/], I plan to unify the trailers related tests for "git log" and "git for-each-ref" in new file. Maybe on top of this patch series?

Hariom Verma (3):
  pretty.c: refactor trailer logic to `format_set_trailers_options()`
  pretty.c: capture invalid trailer argument
  ref-filter: use pretty.c logic for trailers

 Documentation/git-for-each-ref.txt |   8 +-
 pretty.c                           |  98 ++++++++++++++----------
 pretty.h                           |  12 +++
 ref-filter.c                       |  36 +++++----
 t/t6300-for-each-ref.sh            | 119 +++++++++++++++++++++++++----
 5 files changed, 200 insertions(+), 73 deletions(-)

base-commit: e6362826a0409539642a5738db61827e5978e2e4
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-726%2Fharry-hov%2Funify-trailers-logic-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-726/harry-hov/unify-trailers-logic-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/726

Range-diff vs v2:

 1:  fc5fd5217dfc ! 1:  81030f00b11b pretty.c: refactor trailer logic to `format_set_trailers_options()`
     @@ Commit message
          pretty.c: refactor trailer logic to `format_set_trailers_options()`
      
          Refactored trailers formatting logic inside pretty.c to a new function
     -    `format_set_trailers_options()`. This change will allow us to reuse
     -    the same logic in other places.
     +    `format_set_trailers_options()`. This new function returns the non-zero
     +    in case of unusual. The caller handles the non-zero by "goto trailers_out".
     +
     +    This change will allow us to reuse the same logic in other places.
      
          Mentored-by: Christian Couder <chriscool@tuxfamily.org>
          Mentored-by: Heba Waly <heba.waly@gmail.com>
     @@ pretty.c: static int format_trailer_match_cb(const struct strbuf *key, void *ud)
      +			uintptr_t len = arglen;
      +
      +			if (!argval)
     -+				return 1;
     ++				return -1;
      +
      +			if (len && argval[len - 1] == ':')
      +				len--;
     @@ pretty.c: static int format_trailer_match_cb(const struct strbuf *key, void *ud)
      +			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_arg_value(*arg, "key_value_separator", arg, &argval, &arglen)) {
      +			char *fmt;
     ++
     ++			strbuf_reset(kvsepbuf);
      +			fmt = xstrndup(argval, arglen);
      +			strbuf_expand(kvsepbuf, fmt, strbuf_expand_literal_cb, NULL);
      +			free(fmt);
 2:  245e48eb6835 ! 2:  f4a6b2df1444 pretty.c: capture invalid trailer argument
     @@ Metadata
       ## Commit message ##
          pretty.c: capture invalid trailer argument
      
     -    As we would like to use this same logic in ref-filter, it's nice to
     -    get invalid trailer argument. This will allow us to print precise
     -    error message, while using `format_set_trailers_options()` in
     +    As we would like to use this trailers logic in the ref-filter, it's
     +    nice to get an invalid trailer argument. This will allow us to print
     +    precise error message while using `format_set_trailers_options()` in
          ref-filter.
      
     +    For capturing the invalid argument, we changed the working of
     +    `format_set_trailers_options()` a little bit.
     +    Original logic does "break" and fell through in mainly 2 cases -
     +        1. unknown/invalid argument
     +        2. end of the arg string
     +
     +    But now instead of "break", we capture invalid argument and return
     +    non-zero. And non-zero is handled by the caller.
     +    (We prepared the caller to handle non-zero in the previous commit).
     +
     +    Capturing invalid arguments this way will also affects the working
     +    of current logic. As at the end of the arg string it will return non-zero.
     +    So in order to make things correct, introduced an additional conditional
     +    statement i.e if encounter ")", do 'break'.
     +
          Mentored-by: Christian Couder <chriscool@tuxfamily.org>
          Mentored-by: Heba Waly <heba.waly@gmail.com>
          Signed-off-by: Hariom Verma <hariom18599@gmail.com>
     @@ pretty.c: int format_set_trailers_options(struct process_trailer_options *opts,
       		const char *argval;
       		size_t arglen;
       
     -+		if(**arg == ')') {
     ++		if (**arg == ')')
      +			break;
     -+		}
      +
       		if (match_placeholder_arg_value(*arg, "key", arg, &argval, &arglen)) {
       			uintptr_t len = arglen;
     @@ pretty.c: int format_set_trailers_options(struct process_trailer_options *opts,
      -			   !match_placeholder_bool_arg(*arg, "valueonly", arg, &opts->value_only))
      -			break;
      +			   !match_placeholder_bool_arg(*arg, "valueonly", arg, &opts->value_only)) {
     -+			size_t invalid_arg_len = strcspn(*arg, ",)");
     -+			*invalid_arg = xstrndup(*arg, invalid_arg_len);
     -+			return 1;
     ++			if (invalid_arg) {
     ++				size_t len = strcspn(*arg, ",)");
     ++				*invalid_arg = xstrndup(*arg, len);
     ++			}
     ++			return -1;
      +		}
       	}
       	return 0;
       }
      @@ pretty.c: static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
     - 		struct strbuf sepbuf = STRBUF_INIT;
     - 		struct strbuf kvsepbuf = STRBUF_INIT;
     - 		size_t ret = 0;
     -+		char *unused = NULL;
     - 
     - 		opts.no_divider = 1;
       
       		if (*arg == ':') {
       			arg++;
      -			if (format_set_trailers_options(&opts, &filter_list, &sepbuf, &kvsepbuf, &arg))
     -+			if (format_set_trailers_options(&opts, &filter_list, &sepbuf, &kvsepbuf, &arg, &unused))
     ++			if (format_set_trailers_options(&opts, &filter_list, &sepbuf, &kvsepbuf, &arg, NULL))
       				goto trailer_out;
       		}
       		if (*arg == ')') {
     -@@ pretty.c: 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;
     - 	}
     - 
      
       ## pretty.h ##
      @@ pretty.h: int format_set_trailers_options(struct process_trailer_options *opts,
 3:  7b8cfb2721c3 ! 3:  47d89f872314 ref-filter: use pretty.c logic for trailers
     @@ Commit message
            :key=<K> - only show trailers with specified key.
            :valueonly[=val] - only show the value part.
            :separator=<SEP> - inserted between trailer lines.
     -      :key_value_separator=<SEP> - inserted between trailer lines
     +      :key_value_separator=<SEP> - inserted between key and value in trailer lines
      
          Enhancement to existing options(now can take value and its optional):
            :only[=val]
     @@ Documentation/git-for-each-ref.txt: contents:lines=N::
      -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. 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.
     -+** 'key_value_separator=<SEP>': specify a separator inserted between
     -+   key and value. The string SEP may contain the literal formatting codes.
     -+   E.g., `%(trailers:key=Ticket,key_value_separator=%x2C)` shows all trailer
     -+   lines whose key is "Ticket" with key and value separated by a comma.
     -+** '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.
     ++`contents:trailers[:options]`). For valid [:option] values see `trailers`
     ++section of linkgit:git-log[1].
       
       For sorting purposes, fields with numeric values sort in numeric order
       (`objectsize`, `authordate`, `committerdate`, `creatordate`, `taggerdate`).
     @@ ref-filter.c: struct refname_atom {
       	int lstrip, rstrip;
       };
       
     -+struct ref_trailer_buf {
     ++static struct ref_trailer_buf {
      +	struct string_list filter_list;
      +	struct strbuf sepbuf;
      +	struct strbuf kvsepbuf;
     -+} ref_trailer_buf;
     ++} ref_trailer_buf = {STRING_LIST_INIT_NODUP, STRBUF_INIT, STRBUF_INIT};
      +
       static struct expand_data {
       	struct object_id oid;
     @@ ref-filter.c: static int subject_atom_parser(const struct ref_format *format, st
      +		char *invalid_arg = NULL;
      +
      +		if (format_set_trailers_options(&atom->u.contents.trailer_opts,
     -+			&ref_trailer_buf.filter_list,
     -+			&ref_trailer_buf.sepbuf,
     -+			&ref_trailer_buf.kvsepbuf,
     -+			&argbuf, &invalid_arg)) {
     ++		    &ref_trailer_buf.filter_list,
     ++		    &ref_trailer_buf.sepbuf,
     ++		    &ref_trailer_buf.kvsepbuf,
     ++		    &argbuf, &invalid_arg)) {
      +			if (!invalid_arg)
      +				strbuf_addf(err, _("expected %%(trailers:key=<value>)"));
      +			else
     @@ t/t6300-for-each-ref.sh: test_expect_success '%(trailers:only) and %(trailers:un
      +	option="$2"
      +	expect="$3"
      +	test_expect_success "$title" '
     -+		echo $expect >expect &&
     ++		printf "$expect\n" >expect &&
      +		git for-each-ref --format="%($option)" refs/heads/main >actual &&
      +		test_cmp expect actual &&
      +		git for-each-ref --format="%(contents:$option)" refs/heads/main >actual &&

From: Hariom Verma <hariom18599@gmail.com>

Refactored trailers formatting logic inside pretty.c to a new function `format_set_trailers_options()`. This new function returns the non-zero in case of unusual. The caller handles the non-zero by "goto trailers_out".

This change will allow us to reuse the same logic in other places.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Heba Waly <heba.waly@gmail.com>
Signed-off-by: Hariom Verma <hariom18599@gmail.com>
---
 pretty.c | 89 +++++++++++++++++++++++++++++++-------------------------
 pretty.h | 11 +++++++
 2 files changed, 61 insertions(+), 39 deletions(-)

From: Hariom Verma <hariom18599@gmail.com>

As we would like to use this trailers logic in the ref-filter, it's nice to get an invalid trailer argument. This will allow us to print precise error message while using `format_set_trailers_options()` in ref-filter.

For capturing the invalid argument, we changed the working of
`format_set_trailers_options()` a little bit.
Original logic does "break" and fell through in mainly 2 cases -
    1. unknown/invalid argument
    2. end of the arg string

But now instead of "break", we capture invalid argument and return non-zero. And non-zero is handled by the caller. (We prepared the caller to handle non-zero in the previous commit).

Capturing invalid arguments this way will also affects the working of current logic. As at the end of the arg string it will return non-zero. So in order to make things correct, introduced an additional conditional statement i.e if encounter ")", do 'break'.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Heba Waly <heba.waly@gmail.com>
Signed-off-by: Hariom Verma <hariom18599@gmail.com>
---
 pretty.c | 17 +++++++++++++----
 pretty.h |  3 ++-
 2 files changed, 15 insertions(+), 5 deletions(-)

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.
  :key_value_separator=<SEP> - inserted between key and value in 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 |   8 +-
 ref-filter.c                       |  36 +++++----
 t/t6300-for-each-ref.sh            | 119 +++++++++++++++++++++++++----
 3 files changed, 129 insertions(+), 34 deletions(-)

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

This is *not* an issue about the test script and its helper function, but I just noticed that --format="%(trailers:key=<key>)" is expected to write the matching trailers *AND* an empty line, and I wonder if that is a sensible thing to expect.

The "--pretty" side does not give such an extra blank line after the output, though.

 $ git show -s --pretty=format:"%(trailers:key=Signed-off-by:)" \
   js/range-diff-wo-dotdot
 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
 Signed-off-by: Junio C Hamano <gitster@pobox.com>
 $ git show -s --pretty=format:"%(trailers:key=None:)" \
   js/range-diff-wo-dotdot
 $ exit

Unlike the above, when there is no matching trailer lines, the "for-each-ref" in this series shows zero lines, and when there is one matching trailer line, it gives that single line plus an empty line, two lines in total. The inconsistency is a bit disturbing.

Is the extra blank line given on purpose? I don't see why we would want it. Or is it a bug we did not catch during the previous two rounds of reviews?

Thanks.

Hi,

On Sun, Feb 7, 2021 at 11:15 AM Junio C Hamano <gitster@pobox.com> wrote:

I don't think that "extra blank line" is due to this patch series. Wait. Let me see.

Since "for-each-ref"'s original code does not support
"trailers:key=<KEY>", Let's check original code for "trailers:only":
```
  $ git for-each-ref refs/heads/master --format="%(trailers:only)"
  Signed-off-by: Junio C Hamano <gitster@pobox.com>

  $ exit
```
I see. The original code also gives an extra blank line.

Now, let's check for this patch series:
```
  $ ./bin-wrappers/git for-each-ref refs/heads/master
--format="%(trailers:key=Signed-off-by)"
  Signed-off-by: Junio C Hamano <gitster@pobox.com>

  $ ./bin-wrappers/git for-each-ref refs/heads/master
--format="%(trailers:key=None)"

  $ exit
```
when there is no matching trailer lines, the "for-each-ref" in this
series shows one empty line, and when there is one matching trailer
line, it gives that single line plus an empty line, two lines in
total. Seems consistent to me.

So this isn't about the patch series. Question still remains the same. Why extra blank line? Let's dig a bit. Ah. I guess I found the reason. It's due to `putchar('\n');` in `show_ref_array_item() [1]`. It puts a new line after each ref item.

Do you want me to include a patch to get rid of this "extra blank line" for trailers in "for-each-ref"?

Thanks, Hariom.

[1]: https://github.com/git/git/blob/fb7fa4a1fd273f22efcafdd13c7f897814fd1eb9/ref-filter.c#L2435

Hariom verma <hariom18599@gmail.com> writes:

Thanks for digging the history.

I do not know the answer to the last question, because we haven't learned the original reason why we decided to add the extra blank line after the trailer output. Even though I find it unnecessary, the code that adds it must have been written with a good reason to do so, and I do not want to see us remove the "\n" without knowing that reason.

Thanks.

Hi,

On Sun, Feb 7, 2021 at 11:49 PM Junio C Hamano <gitster@pobox.com> wrote:

As per my understanding it works something like this:

print a ref item... put newline... print a ref item... put newline.. print a ref item... put newline... (so on)

But the catch is that trailer comes with a newline already included. So it becomes:

print trailers with newline included... put newline... print trailers with newline included... put newline.. (so on)

So we end up having 2 new lines in total.

we just can't directly remove the newline. but we introduce an option to skip at will. Something like this? https://github.com/harry-hov/git/commit/af75f5c9b0325af90831998f56d6f36b6baa928e

So we can turn off newline(extra) for trailers without disturbing "for-each-ref"'s working.

Thanks, Hariom.

Hariom verma <hariom18599@gmail.com> writes:

We know how it happens. The question is if that is a sensible behaviour, and if the trailing blank line was _intended_, or a bug that nobody has complained about so far.

If we agree that the current behaviour is *not* sensible, then we can. On the "log --pretty" side, we have "terminator semantics" and "separator semantics" between "tformat" and "format", when showing more than one commits in a row, the "terminator semantics" places one blank line after each commit we emit, while the "separator semantics" gives one blank line between each commit pair. I think we initially (incorrectly) used terminator semantics and our output for two commits looked like "CommitA <blank> CommitB <blank>" before we fixed it to use separator semantics to show "CommitA <blank> CommitB" without the useless trailing blank line. We can apply the same principle when "fixing" this issue to show a block of trailer lines (that is, the change in behaviour to remove the trailing blank line turns out to be a "fix").

Hi,

On Mon, Feb 8, 2021 at 1:39 AM Junio C Hamano <gitster@pobox.com> wrote:

I suspect that "fix" for "log --pretty" isn't going to work here.

Even if we apply the same "log --pretty"'s fix here. I think we still end up having an empty blank line between each ref item.

Thank, Hariom

Hariom verma <hariom18599@gmail.com> writes:

After sleeping on it and seeing a result of an experiment like this one, I think that might be unavoidable.

    $ git for-each-ref \
	--format="One%0a%(trailers:key=Signed-off-by:)Two%0a" \
	refs/heads/js/range-diff-wo-dotdot
    One
    Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>
    Two
    $ exit

People who write such "Two" without prefixing it with a newline "%0a" themselves may view such a "fix" a regression.

It is sad that this %(trailers) itself is relatively a new thing, and I had thought that all the other ingredients are designed to strip the trailing newline, e.g. try this:

    $ git for-each-ref \
	--format="%(subject)%0a%(trailers:key=Signed-off-by:)" \
	refs/heads/js/range-diff-wo-dotdot
    range-diff(docs): explain how to specify commit ranges
    Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

Notice that %(subject) is followed explicitly by %0a. I think %(author:date), etc. would do the same. But %(trailers) behave differently, and that is because it expects to be multi-line and perhaps to mimic %(body)? In any case, it may be too late to change its behaviour. At least I do not think of a good waoy to do so.

By the way, when merged to 'seen' (you can try the above that shows %(subject) followed by %(trailers) with the tip of 'seen'), it dies like this:

    $ git for-each-ref \
	--format="%(subject)%0a%(trailers:key=Signed-off-by:)" \
	refs/heads/js/range-diff-wo-dotdot
    free(): double free detected in tcache 2
    Aborted

There must be some interaction with another topic but I didn't dig deeper.

Thanks.

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

Are we sure that "$expect" would not ever have any '%' in it, to confuse printf? To be future-proof and safe, it would be prudent to instead use

	printf "%s\n" "$expect"

to make sure that whatever is passed in $3 gets output LITERALLY.

The callers need to adopt the change I gave you in the review of the previous round so that they do not assume backslash-en will by changed to LF by somebody---instead if they mean LF, they just say LF.

Thanks.

Junio C Hamano <gitster@pobox.com> writes:

Just to make sure we won't waste your time in useless roundtrip(s), let me say that possible unacceptable answers are:

 - no, right now nobody passes a % in it
 - no, I do not expect anybody needs to pass a % in it
 - when somebody really needs to pass a %, they can write it as %%

The last one is the worst one, by the way.

The point of adding a test_trailer_option HELPER function is to HELP the developers who write tests, now and in the future. There are some things they MUST know to use the helper successfully, like it takes three parameters, the first one being the test title, the second one is the string you'd give as the "--format=<format>" option to the for-each-ref command, and the third one is the expected output.

Forcing them to know any more than that is *not* helping them.

The shell programming language is perfectly capable of passing an argument that happens to be a multi-line string to functions and external commands, and the developers who are writing test knows that already (the last argument to test_expect_success used everywhere in the test suite, that is a multi-line code snippet, is a good example). When they need to write an expected output that is two lines, they expect to be able to write things like

	test_trailer_option title format-string \
	'expected line #1
	expected line #2'

without having to worry about the need for special formatting that is applicable *only* when passing argument to this helper. They do not need to know that they cannot pass backslash-en literally, and have to say '\\n' instead, or they have to double a per-cent sign, only when using this helper but not other helper functions.

In the message I am replying to, I used

	printf "%s\n" "$expect"

for a reason. We expect that trailer options output are complete lines, so it is annoying to force the caller to write the final newline, especially if many of the callers have only one line of expected output. So

    test_trailer_option title format-string 'expected output'

would end up doing

	printf "%s\n" "expected output" >expect

to write a complete line, i.e. a caller does not have to say any of

    test_trailer_option title format-string 'expected output\n'

    test_trailer_option title format-string 'expected output
    '

    lf='
    '
    test_trailer_option title format-string "expected output$lf"

If we do not need to extend test_trailer_option with further "features" (like "check output case insensitively this time" or "allow output lines in any order"), we can make it even nicer and easier to use for callers, by the way.

For example, with this (by the way, make sure there are SP on both sides around "()", that's our house style):

	test_trailer_option () {
		title=$1 option=$2
		shift 2
		if test $# != 0
		then
			printf "%s\n" "$@"
		fi >expect
		test_expect_success "$title" '
			... >actual &&
			test_cmp expect actual &&
			... >actual &&
			test_cmp expect actual
		'
	}

the caller can do

	test_trailer_option 'single line output' \
		'trailers:key=Signed-off-by' \
		'Signed-off-by: A U Thor <author@example.com>'

	test_trailer_option 'expect two lines' \
		'trailers:key=Reviewed-by' \
		'Reviewed-by: A U Thor <author@example.com>' \
		'Reviewed-by: R E Viewer <reviewer@example.com>'

	test_trailer_option 'no output expected' 'trailers:key=no-such:' ''

That is, instead of "the first arg is title, the second is format and the third is the entire expected output", the helper's manual can say "give title and format as the first and the second argument. Each argument after that is an expected output, one line per arg."

Another possibility is to feed the expected output from the standard input of the helper, e.g.

	test_trailer_option () {
		title=$1 option=$2
		cat >expect
		test_expect_success "$title" '
			... >actual &&
			test_cmp expect actual &&
			... >actual &&
			test_cmp expect actual
		'
	}

And the caller can now do:

	test_trailer_option 'expect two lines' 'trailers:key=Reviewed-by' <<-\EOF
        Reviewed-by: A U Thor <author@example.com>
        Reviewed-by: R E Viewer <reviewer@example.com>
	EOF

It is a bit cumbersome when the expected output is a single line:

	test_trailer_option 'single line output' 'trailers:key=Signed-off-by' <<-\EOF
	Signed-off-by: A U Thor <author@example.com>
	EOF

but the contrast between the "two-line expected" case and this one would be easy to see when reading the tests. The pattern to write "expect no output" would be quite simple, too:

	test_trailer_option 'no output expected' 'trailers:key=no-such:' </dev/null

Among the ones designed while writing this response, I would think I like the last one, i.e. "the first arg is title, the second arg is format, and the expected output is given from the standasd output" probably the best.

Thanks.

Currently, there exists a separate logic for %(trailers) in "pretty.{c,h}" and "ref-filter.{c,h}". Both are actually doing the same thing, why not use the same code for both of them?

This is the 4th version of the patch series focused on unifying the "%(trailers)" logic for both 'pretty.{c,h}' and 'ref-filter.{c,h}'. So, we can have one logic for trailers.

v4 changes:

 * improved tests

Hariom Verma (4):
  t6300: use function to test trailer options
  pretty.c: refactor trailer logic to `format_set_trailers_options()`
  pretty.c: capture invalid trailer argument
  ref-filter: use pretty.c logic for trailers

 Documentation/git-for-each-ref.txt |   8 +-
 pretty.c                           |  98 +++++++++------
 pretty.h                           |  12 ++
 ref-filter.c                       |  36 +++---
 t/t6300-for-each-ref.sh            | 185 ++++++++++++++++++++++-------
 5 files changed, 236 insertions(+), 103 deletions(-)

base-commit: 328c10930387d301560f7cbcd3351cc485a13381
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-726%2Fharry-hov%2Funify-trailers-logic-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-726/harry-hov/unify-trailers-logic-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/726

Range-diff vs v3:

 -:  ------------ > 1:  410b02dbad20 t6300: use function to test trailer options
 1:  81030f00b11b = 2:  fd275fed8347 pretty.c: refactor trailer logic to `format_set_trailers_options()`
 2:  f4a6b2df1444 = 3:  073c75dc4494 pretty.c: capture invalid trailer argument
 3:  47d89f872314 ! 4:  9ec989176993 ref-filter: use pretty.c logic for trailers
     @@ ref-filter.c: static int subject_atom_parser(const struct ref_format *format, st
       
      
       ## t/t6300-for-each-ref.sh ##
     -@@ t/t6300-for-each-ref.sh: test_expect_success '%(trailers:unfold) unfolds trailers' '
     - 	test_cmp expect actual
     - '
     +@@ t/t6300-for-each-ref.sh: test_trailer_option '%(trailers:only) shows only "key: value" trailers' \
       
     --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/main >actual &&
     -+		test_cmp expect actual &&
     -+		git for-each-ref --format="%(contents:$option)" refs/heads/main >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/main >actual &&
     -+	git for-each-ref --format="%(trailers:only=no)" refs/heads/main >actual &&
     - 	test_cmp expect actual &&
     --	git for-each-ref --format="%(contents:trailers:only)" refs/heads/main >actual &&
     -+	git for-each-ref --format="%(contents:trailers:only=no)" refs/heads/main >actual &&
     - 	test_cmp expect actual
     - '
     + 	EOF
       
     -@@ t/t6300-for-each-ref.sh: 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" '
     -+		printf "$expect\n" >expect &&
     -+		git for-each-ref --format="%($option)" refs/heads/main >actual &&
     -+		test_cmp expect actual &&
     -+		git for-each-ref --format="%(contents:$option)" refs/heads/main >actual &&
     -+		test_cmp expect actual
     -+	'
     -+}
     ++test_trailer_option '%(trailers:only=no,only=true) shows only "key: value" trailers' \
     ++	'trailers:only=no,only=true' <<-EOF
     ++	$(grep -v patch.description <trailers)
     ++
     ++	EOF
     ++
     ++test_trailer_option '%(trailers:only=yes) shows only "key: value" trailers' \
     ++	'trailers:only=yes' <<-EOF
     ++	$(grep -v patch.description <trailers)
     ++
     ++	EOF
      +
     ++test_trailer_option '%(trailers:only=no) shows all trailers' \
     ++	'trailers:only=no' <<-EOF
     ++	$(cat trailers)
     ++
     ++	EOF
     ++
     + test_trailer_option '%(trailers:only) and %(trailers:unfold) work together' \
     + 	'trailers:only,unfold' <<-EOF
     + 	$(grep -v patch.description <trailers | unfold)
     +@@ t/t6300-for-each-ref.sh: test_trailer_option '%(trailers:unfold) and %(trailers:only) work together' \
     + 
     + 	EOF
     + 
      +test_trailer_option '%(trailers:key=foo) shows that trailer' \
     -+	'trailers:key=Signed-off-by' 'Signed-off-by: A U Thor <author@example.com>\n'
     ++	'trailers:key=Signed-off-by' <<-EOF
     ++	Signed-off-by: A U Thor <author@example.com>
     ++
     ++	EOF
     ++
      +test_trailer_option '%(trailers:key=foo) is case insensitive' \
     -+	'trailers:key=SiGned-oFf-bY' 'Signed-off-by: A U Thor <author@example.com>\n'
     ++	'trailers:key=SiGned-oFf-bY' <<-EOF
     ++	Signed-off-by: A U Thor <author@example.com>
     ++
     ++	EOF
     ++
      +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'
     ++	'trailers:key=Signed-off-by:' <<-EOF
     ++	Signed-off-by: A U Thor <author@example.com>
     ++
     ++	EOF
     ++
      +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'
     ++	'trailers:key=Reviewed-by:,key=Signed-off-by' <<-EOF
     ++	Reviewed-by: A U Thor <author@example.com>
     ++	Signed-off-by: A U Thor <author@example.com>
     ++
     ++	EOF
     ++
      +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/main >actual &&
     -+	test_cmp expect actual &&
     -+	git for-each-ref --format="%(contents:trailers:key=Acked-by)" refs/heads/main >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/main >actual &&
     -+	test_cmp expect actual &&
     -+	git for-each-ref --format="%(contents:trailers:key=Signed-Off-by,unfold)" refs/heads/main >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/main >actual &&
     -+	test_cmp expect actual &&
     -+	git for-each-ref --format="%(contents:trailers:key=Signed-off-by,only=no)" refs/heads/main >actual &&
     -+	test_cmp expect actual
     -+'
     ++	'trailers:key=Shined-off-by:' <<-EOF
     ++
     ++	EOF
     ++
     ++test_trailer_option '%(trailers:key=foo) handles multiple lines even if folded' \
     ++	'trailers:key=Acked-by' <<-EOF
     ++	$(grep -v patch.description <trailers | grep -v Signed-off-by | grep -v Reviewed-by)
     ++
     ++	EOF
     ++
     ++test_trailer_option '%(trailers:key=foo,unfold) properly unfolds' \
     ++	'trailers:key=Signed-Off-by,unfold' <<-EOF
     ++	$(unfold <trailers | grep Signed-off-by)
     ++
     ++	EOF
     ++
     ++test_trailer_option '%(trailers:key=foo,only=no) also includes nontrailer lines' \
     ++	'trailers:key=Signed-off-by,only=no' <<-EOF
     ++	Signed-off-by: A U Thor <author@example.com>
     ++	$(grep patch.description <trailers)
     ++
     ++	EOF
      +
      +test_trailer_option '%(trailers:key=foo,valueonly) shows only value' \
     -+	'trailers:key=Signed-off-by,valueonly' 'A U Thor <author@example.com>\n'
     ++	'trailers:key=Signed-off-by,valueonly' <<-EOF
     ++	A U Thor <author@example.com>
     ++
     ++	EOF
     ++
      +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>'
     ++	'trailers:separator=%x2C,key=Reviewed-by,key=Signed-off-by:' <<-EOF
     ++	Reviewed-by: A U Thor <author@example.com>,Signed-off-by: A U Thor <author@example.com>
     ++	EOF
     ++
      +test_trailer_option '%(trailers:key_value_separator) changes key-value separator' \
     -+	'trailers:key_value_separator=%x2C,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'
     ++	'trailers:key_value_separator=%x2C,key=Reviewed-by,key=Signed-off-by:' <<-EOF
     ++	Reviewed-by,A U Thor <author@example.com>
     ++	Signed-off-by,A U Thor <author@example.com>
     ++
     ++	EOF
     ++
      +test_trailer_option '%(trailers:separator,key_value_separator) changes both separators' \
     -+	'trailers:separator=%x2C,key_value_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/main 2>actual &&
     -+		test_i18ncmp expect actual &&
     -+		test_must_fail git for-each-ref --format="%(contents:$option)" refs/heads/main 2>actual &&
     -+		test_i18ncmp expect actual
     -+	'
     -+}
     ++	'trailers:separator=%x2C,key_value_separator=%x2C,key=Reviewed-by,key=Signed-off-by:' <<-EOF
     ++	Reviewed-by,A U Thor <author@example.com>,Signed-off-by,A U Thor <author@example.com>
     ++	EOF
      +
     + test_failing_trailer_option () {
     + 	title=$1 option=$2
     + 	cat >expect
     +@@ t/t6300-for-each-ref.sh: test_failing_trailer_option '%(trailers) rejects unknown trailers arguments' \
     + 	fatal: unknown %(trailers) argument: unsupported
     + 	EOF
     + 
      +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'
     ++	'trailers:key' <<-\EOF
     ++	fatal: expected %(trailers:key=<value>)
     ++	EOF
      +
       test_expect_success 'if arguments, %(contents:trailers) shows error if colon is missing' '
       	cat >expect <<-EOF &&

From: Hariom Verma <hariom18599@gmail.com>

Add a function to test trailer options. This will make tests look cleaner, as well as will make it easier to add new tests for trailers in the future.

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 | 90 +++++++++++++++++++++--------------------
 1 file changed, 47 insertions(+), 43 deletions(-)

From: Hariom Verma <hariom18599@gmail.com>

Refactored trailers formatting logic inside pretty.c to a new function `format_set_trailers_options()`. This new function returns the non-zero in case of unusual. The caller handles the non-zero by "goto trailers_out".

This change will allow us to reuse the same logic in other places.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Heba Waly <heba.waly@gmail.com>
Signed-off-by: Hariom Verma <hariom18599@gmail.com>
---
 pretty.c | 89 +++++++++++++++++++++++++++++++-------------------------
 pretty.h | 11 +++++++
 2 files changed, 61 insertions(+), 39 deletions(-)

From: Hariom Verma <hariom18599@gmail.com>

As we would like to use this trailers logic in the ref-filter, it's nice to get an invalid trailer argument. This will allow us to print precise error message while using `format_set_trailers_options()` in ref-filter.

For capturing the invalid argument, we changed the working of
`format_set_trailers_options()` a little bit.
Original logic does "break" and fell through in mainly 2 cases -
    1. unknown/invalid argument
    2. end of the arg string

But now instead of "break", we capture invalid argument and return non-zero. And non-zero is handled by the caller. (We prepared the caller to handle non-zero in the previous commit).

Capturing invalid arguments this way will also affects the working of current logic. As at the end of the arg string it will return non-zero. So in order to make things correct, introduced an additional conditional statement i.e if encounter ")", do 'break'.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Heba Waly <heba.waly@gmail.com>
Signed-off-by: Hariom Verma <hariom18599@gmail.com>
---
 pretty.c | 17 +++++++++++++----
 pretty.h |  3 ++-
 2 files changed, 15 insertions(+), 5 deletions(-)

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.
  :key_value_separator=<SEP> - inserted between key and value in 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 |  8 +--
 ref-filter.c                       | 36 ++++++-----
 t/t6300-for-each-ref.sh            | 95 ++++++++++++++++++++++++++++++
 3 files changed, 118 insertions(+), 21 deletions(-)

On 2021-02-08 at 19:54:18, Junio C Hamano wrote:

I'll take a look.  Thanks for the heads up.

On 2021-02-08 at 19:54:18, Junio C Hamano wrote:

I'll send out a fixed patch tomorrow, but for the moment, here's the gist of the change if you want to an immediate fix to squash in:

------- %< ---------

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

Ah, I see. find_subpos() will only called once to find the subject and signature in the loop, and the finding will have to live even the current iteration of the loop is done, only to be released after everything is done.

Makes sense.