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@xxxxxx 1
[https://public-inbox.org/git/bf4423d5-c0ee-6bef-59ff-fcde003ec463@xxxxxx/]
* 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@xxxxxxxxx/
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@xxxxxxxxxxxxx>
Mentored-by: Heba Waly <heba.waly@xxxxxxxxx>
Signed-off-by: Hariom Verma <hariom18599@xxxxxxxxx>
- ## 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@xxxxxxxxx>
## 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(¶ms, 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@xxxxxxxxxxx>\n'
+test_trailer_option '%(trailers:separator) changes separator' \
+ 'trailers:separator=%x2C,key=Reviewed-by,key=Signed-off-by:' 'Reviewed-by: A U Thor <author@xxxxxxxxxxx>,Signed-off-by: A U Thor <author@xxxxxxxxxxx>'
++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@xxxxxxxxxxx>\nSigned-off-by,A U Thor <author@xxxxxxxxxxx>\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@xxxxxxxxxxx>,Signed-off-by,A U Thor <author@xxxxxxxxxxx>'
+
+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
+ '
+}
--
gitgitgadget
