{"thread":{"id":"47115","subject":"[PATCH v1 0/2] Add option to git log to choose which refs receive decoration","startedAt":"2017-11-04T00:42:43Z","lastAt":"2017-11-22T04:19:07Z","messageCount":24,"participants":["Rafael Ascensão","Junio C Hamano","Kevin Daudt","Michael Haggerty","Jacob Keller"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"331809","messageId":"20171104004144.5975-1-rafa.almas@gmail.com","threadId":"47115","inReplyTo":null,"subject":"[PATCH v1 0/2] Add option to git log to choose which refs receive decoration","fromName":"Rafael Ascensão","fromEmail":"rafa.almas@gmail.com","sentAt":"2017-11-04T00:41:42Z","receivedAt":"2017-11-04T00:42:43Z","isPatch":true,"sender":{"key":"rafa.almas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1923789?v=4"},"body":"As suggested by Documentation/SubmittingPatches\nHi, this is my first patch.\\n\n\nI basically stumbled on the same issue mentioned here:\nhttps://public-inbox.org/git/xmqqzim1pp4m.fsf@gitster.mtv.corp.google.com/\n\nThis patch implements two new command line options for `git log`:\n`--decorate-refs=` and `--decorate-refs-exlcude=`\n\nBoth options accept a glob pattern which determines what decorations\ncommits receive.\n\nAt first I considered adding '--trim-decoration', that would filter refs\nbased on values passed to '--branches=' '--remotes=' '--tags=' and\n'--exclude='.\n\nAfter reading the email, I think it's better to have those two\nbehaviours decoupled.\n\nI also had plans to add:\n(Not sure if others deserve having their own command)\n--decorate-branches=\n--decorate-remotes=\n--decorate-tags=\n\nBut was not sure if a 'niche' function like this is worth 5+ command\nline options. I personally find that those two are enough.\n\n---\nRafael Ascensão\n\nRafael Ascensão (2):\n refs: extract function to normalize partial refs\n log: add option to choose which refs to decorate\n\n Documentation/git-log.txt | 12 ++++++\n builtin/log.c | 10 ++++-\n log-tree.c | 37 ++++++++++++++---\n log-tree.h | 6 ++-\n pretty.c | 4 +-\n refs.c | 34 +++++++++-------\n refs.h | 16 ++++++++\n revision.c | 2 +-\n t/t4202-log.sh | 101 ++++++++++++++++++++++++++++++++++++++++++++++\n 9 files changed, 198 insertions(+), 24 deletions(-)\n\n-- \n2.15.0\n\n"},{"id":"331810","messageId":"20171104004144.5975-2-rafa.almas@gmail.com","threadId":"47115","inReplyTo":"20171104004144.5975-1-rafa.almas@gmail.com","subject":"[PATCH v1 1/2] refs: extract function to normalize partial refs","fromName":"Rafael Ascensão","fromEmail":"rafa.almas@gmail.com","sentAt":"2017-11-04T00:41:43Z","receivedAt":"2017-11-04T00:42:51Z","isPatch":true,"sender":{"key":"rafa.almas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1923789?v=4"},"body":"`for_each_glob_ref_in` has some code built into it that converts\npartial refs like 'heads/master' to their full qualified form\n'refs/heads/master'. It also assume a trailing '/*' if no glob\ncharacters are present in the pattern.\n\nExtract that logic to its own function which can be reused elsewhere\nwhere the same behaviour is needed, and add an ENSURE_GLOB flag\nto toggle if a trailing '/*' is to be appended to the result.\n\nSigned-off-by: Kevin Daudt \nSigned-off-by: Rafael Ascensão \n---\n refs.c | 34 ++++++++++++++++++++--------------\n refs.h | 16 ++++++++++++++++\n 2 files changed, 36 insertions(+), 14 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex c590a992f..1e74b48e6 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -369,32 +369,38 @@ int head_ref_namespaced(each_ref_fn fn, void *cb_data)\n \treturn ret;\n }\n \n-int for_each_glob_ref_in(each_ref_fn fn, const char *pattern,\n-\tconst char *prefix, void *cb_data)\n+void normalize_glob_ref(struct strbuf *normalized_pattern, const char *prefix,\n+\t\tconst char *pattern, int flags)\n {\n-\tstruct strbuf real_pattern = STRBUF_INIT;\n-\tstruct ref_filter filter;\n-\tint ret;\n-\n \tif (!prefix && !starts_with(pattern, \"refs/\"))\n-\t\tstrbuf_addstr(&real_pattern, \"refs/\");\n+\t\tstrbuf_addstr(normalized_pattern, \"refs/\");\n \telse if (prefix)\n-\t\tstrbuf_addstr(&real_pattern, prefix);\n-\tstrbuf_addstr(&real_pattern, pattern);\n+\t\tstrbuf_addstr(normalized_pattern, prefix);\n+\tstrbuf_addstr(normalized_pattern, pattern);\n \n-\tif (!has_glob_specials(pattern)) {\n+\tif (!has_glob_specials(pattern) && (flags & ENSURE_GLOB)) {\n \t\t/* Append implied '/' '*' if not present. */\n-\t\tstrbuf_complete(&real_pattern, '/');\n+\t\tstrbuf_complete(normalized_pattern, '/');\n \t\t/* No need to check for '*', there is none. */\n-\t\tstrbuf_addch(&real_pattern, '*');\n+\t\tstrbuf_addch(normalized_pattern, '*');\n \t}\n+}\n+\n+int for_each_glob_ref_in(each_ref_fn fn, const char *pattern,\n+\tconst char *prefix, void *cb_data)\n+{\n+\tstruct strbuf normalized_pattern = STRBUF_INIT;\n+\tstruct ref_filter filter;\n+\tint ret;\n+\n+\tnormalize_glob_ref(&normalized_pattern, prefix, pattern, ENSURE_GLOB);\n \n-\tfilter.pattern = real_pattern.buf;\n+\tfilter.pattern = normalized_pattern.buf;\n \tfilter.fn = fn;\n \tfilter.cb_data = cb_data;\n \tret = for_each_ref(filter_refs, &filter);\n \n-\tstrbuf_release(&real_pattern);\n+\tstrbuf_release(&normalized_pattern);\n \treturn ret;\n }\n \ndiff --git a/refs.h b/refs.h\nindex a02b628c8..9f9a8bb27 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -312,6 +312,22 @@ int for_each_namespaced_ref(each_ref_fn fn, void *cb_data);\n int refs_for_each_rawref(struct ref_store *refs, each_ref_fn fn, void *cb_data);\n int for_each_rawref(each_ref_fn fn, void *cb_data);\n \n+/*\n+ * Normalizes partial refs to their full qualified form.\n+ * If prefix is NULL, will prepend 'refs/' to the pattern if it doesn't start\n+ * with 'refs/'. Results in refs/\n+ *\n+ * If prefix is not NULL will result in /\n+ *\n+ * If ENSURE_GLOB is set and no glob characters are found in the\n+ * pattern, a trailing <*> will be appended to the result.\n+ * (<> characters to avoid breaking C comment syntax)\n+ */\n+\n+#define ENSURE_GLOB 1\n+void normalize_glob_ref (struct strbuf *normalized_pattern, const char *prefix,\n+\t\t\t\tconst char *pattern, int flags);\n+\n static inline const char *has_glob_specials(const char *pattern)\n {\n \treturn strpbrk(pattern, \"?*[\");\n-- \n2.15.0\n\n"},{"id":"331811","messageId":"20171104004144.5975-3-rafa.almas@gmail.com","threadId":"47115","inReplyTo":"20171104004144.5975-1-rafa.almas@gmail.com","subject":"[PATCH v1 2/2] log: add option to choose which refs to decorate","fromName":"Rafael Ascensão","fromEmail":"rafa.almas@gmail.com","sentAt":"2017-11-04T00:41:44Z","receivedAt":"2017-11-04T00:42:54Z","isPatch":true,"sender":{"key":"rafa.almas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1923789?v=4"},"body":"When `log --decorate` is used, git will decorate commits with all\navailable refs. While in most cases this the desired effect, under some\nconditions it can lead to excessively verbose output.\n\nUsing `--exclude=` can help mitigate that verboseness by\nremoving unnecessary 'branches' from the output. However, if the tip of\nan excluded ref points to an ancestor of a non-excluded ref, git will\ndecorate it regardless.\n\nWith `--decorate-refs=`, only refs that match are\ndecorated while `--decorate-refs-exclude=` allows to do the\nreverse, remove ref decorations that match \n\nBoth can be used together but --decorate-refs-exclude patterns have\nprecedence over --decorate-refs patterns.\n\nThe pattern follows similar rules as `--glob` except it doesn't assume a\ntrailing '/*' if glob characters are missing.\n\nBoth `--decorate-refs` and `--decorate-refs-exclude` can be used\nmultiple times.\n\nSigned-off-by: Kevin Daudt \nSigned-off-by: Rafael Ascensão \n---\n Documentation/git-log.txt | 12 ++++++\n builtin/log.c | 10 ++++-\n log-tree.c | 37 ++++++++++++++---\n log-tree.h | 6 ++-\n pretty.c | 4 +-\n revision.c | 2 +-\n t/t4202-log.sh | 101 ++++++++++++++++++++++++++++++++++++++++++++++\n 7 files changed, 162 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/git-log.txt b/Documentation/git-log.txt\nindex 32246fdb0..314417d89 100644\n--- a/Documentation/git-log.txt\n+++ b/Documentation/git-log.txt\n@@ -38,6 +38,18 @@ OPTIONS\n \tare shown as if 'short' were given, otherwise no ref names are\n \tshown. The default option is 'short'.\n \n+--decorate-refs=::\n+\tOnly print ref names that match the specified pattern. Uses the same\n+\trules as `git rev-list --glob` except it doesn't assume a trailing a\n+\ttrailing '/{asterisk}' if pattern lacks '?', '{asterisk}', or '['.\n+\t`--decorate-refs-exlclude` has precedence.\n+\n+--decorate-refs-exclude=::\n+\tDo not print ref names that match the specified pattern. Uses the same\n+\trules as `git rev-list --glob` except it doesn't assume a trailing a\n+\ttrailing '/{asterisk}' if pattern lacks '?', '{asterisk}', or '['.\n+\tHas precedence over `--decorate-refs`.\n+\n --source::\n \tPrint out the ref name given on the command line by which each\n \tcommit was reached.\ndiff --git a/builtin/log.c b/builtin/log.c\nindex d81a09051..3587c0055 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -143,11 +143,19 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,\n \tstruct userformat_want w;\n \tint quiet = 0, source = 0, mailmap = 0;\n \tstatic struct line_opt_callback_data line_cb = {NULL, NULL, STRING_LIST_INIT_DUP};\n+\tstatic struct string_list decorate_refs_exclude = STRING_LIST_INIT_DUP;\n+\tstatic struct string_list decorate_refs_include = STRING_LIST_INIT_DUP;\n+\tstruct ref_include_exclude_list ref_filter_lists = {&decorate_refs_include,\n+\t\t\t\t\t\t\t &decorate_refs_exclude};\n \n \tconst struct option builtin_log_options[] = {\n \t\tOPT__QUIET(&quiet, N_(\"suppress diff output\")),\n \t\tOPT_BOOL(0, \"source\", &source, N_(\"show source\")),\n \t\tOPT_BOOL(0, \"use-mailmap\", &mailmap, N_(\"Use mail map file\")),\n+\t\tOPT_STRING_LIST(0, \"decorate-refs\", &decorate_refs_include,\n+\t\t\t\tN_(\"ref\"), N_(\"only decorate these refs\")),\n+\t\tOPT_STRING_LIST(0, \"decorate-refs-exclude\", &decorate_refs_exclude,\n+\t\t\t\tN_(\"ref\"), N_(\"do not decorate these refs\")),\n \t\t{ OPTION_CALLBACK, 0, \"decorate\", NULL, NULL, N_(\"decorate options\"),\n \t\t PARSE_OPT_OPTARG, decorate_callback},\n \t\tOPT_CALLBACK('L', NULL, &line_cb, \"n,m:file\",\n@@ -206,7 +214,7 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,\n \n \tif (decoration_style) {\n \t\trev->show_decorations = 1;\n-\t\tload_ref_decorations(decoration_style);\n+\t\tload_ref_decorations(decoration_style, &ref_filter_lists);\n \t}\n \n \tif (rev->line_level_traverse)\ndiff --git a/log-tree.c b/log-tree.c\nindex cea056234..8efc7ac3d 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -94,9 +94,33 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid,\n {\n \tstruct object *obj;\n \tenum decoration_type type = DECORATION_NONE;\n+\tstruct ref_include_exclude_list *filter = (struct ref_include_exclude_list *)cb_data;\n+\tstruct string_list_item *item;\n+\tstruct strbuf real_pattern = STRBUF_INIT;\n+\n+\tif(filter && filter->exclude->nr > 0) {\n+\t\t/* if current ref is on the exclude list skip */\n+\t\tfor_each_string_list_item(item, filter->exclude) {\n+\t\t\tstrbuf_reset(&real_pattern);\n+\t\t\tnormalize_glob_ref(&real_pattern, NULL, item->string, 0);\n+\t\t\tif (!wildmatch(real_pattern.buf, refname, 0))\n+\t\t\t\tgoto finish;\n+\t\t}\n+\t}\n \n-\tassert(cb_data == NULL);\n+\tif (filter && filter->include->nr > 0) {\n+\t\t/* if current ref is present on the include jump to decorate */\n+\t\tfor_each_string_list_item(item, filter->include) {\n+\t\t\tstrbuf_reset(&real_pattern);\n+\t\t\tnormalize_glob_ref(&real_pattern, NULL, item->string, 0);\n+\t\t\tif (!wildmatch(real_pattern.buf, refname, 0))\n+\t\t\t\tgoto decorate;\n+\t\t}\n+\t\t/* Filter was given, but no match was found, skip */\n+\t\tgoto finish;\n+\t}\n \n+decorate:\n \tif (starts_with(refname, git_replace_ref_base)) {\n \t\tstruct object_id original_oid;\n \t\tif (!check_replace_refs)\n@@ -136,6 +160,9 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid,\n \t\t\tparse_object(&obj->oid);\n \t\tadd_name_decoration(DECORATION_REF_TAG, refname, obj);\n \t}\n+\n+finish:\n+\tstrbuf_release(&real_pattern);\n \treturn 0;\n }\n \n@@ -148,15 +175,15 @@ static int add_graft_decoration(const struct commit_graft *graft, void *cb_data)\n \treturn 0;\n }\n \n-void load_ref_decorations(int flags)\n+void load_ref_decorations(int flags, struct ref_include_exclude_list *data)\n {\n \tif (!decoration_loaded) {\n \n \t\tdecoration_loaded = 1;\n \t\tdecoration_flags = flags;\n-\t\tfor_each_ref(add_ref_decoration, NULL);\n-\t\thead_ref(add_ref_decoration, NULL);\n-\t\tfor_each_commit_graft(add_graft_decoration, NULL);\n+\t\tfor_each_ref(add_ref_decoration, data);\n+\t\thead_ref(add_ref_decoration, data);\n+\t\tfor_each_commit_graft(add_graft_decoration, data);\n \t}\n }\n \ndiff --git a/log-tree.h b/log-tree.h\nindex 48f11fb74..66563af88 100644\n--- a/log-tree.h\n+++ b/log-tree.h\n@@ -7,6 +7,10 @@ struct log_info {\n \tstruct commit *commit, *parent;\n };\n \n+struct ref_include_exclude_list {\n+\tstruct string_list *include, *exclude;\n+};\n+\n int parse_decorate_color_config(const char *var, const char *slot_name, const char *value);\n void init_log_tree_opt(struct rev_info *);\n int log_tree_diff_flush(struct rev_info *);\n@@ -24,7 +28,7 @@ void show_decorations(struct rev_info *opt, struct commit *commit);\n void log_write_email_headers(struct rev_info *opt, struct commit *commit,\n \t\t\t const char **extra_headers_p,\n \t\t\t int *need_8bit_cte_p);\n-void load_ref_decorations(int flags);\n+void load_ref_decorations(int flags, struct ref_include_exclude_list *);\n \n #define FORMAT_PATCH_NAME_MAX 64\n void fmt_output_commit(struct strbuf *, struct commit *, struct rev_info *);\ndiff --git a/pretty.c b/pretty.c\nindex 2f6b0ae6c..87a6cc4f9 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1186,11 +1186,11 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\tstrbuf_addstr(sb, get_revision_mark(NULL, commit));\n \t\treturn 1;\n \tcase 'd':\n-\t\tload_ref_decorations(DECORATE_SHORT_REFS);\n+\t\tload_ref_decorations(DECORATE_SHORT_REFS, NULL);\n \t\tformat_decorations(sb, commit, c->auto_color);\n \t\treturn 1;\n \tcase 'D':\n-\t\tload_ref_decorations(DECORATE_SHORT_REFS);\n+\t\tload_ref_decorations(DECORATE_SHORT_REFS, NULL);\n \t\tformat_decorations_extended(sb, commit, c->auto_color, \"\", \", \", \"\");\n \t\treturn 1;\n \tcase 'g':\t\t/* reflog info */\ndiff --git a/revision.c b/revision.c\nindex d167223e6..298ff054b 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1822,7 +1822,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->simplify_by_decoration = 1;\n \t\trevs->limited = 1;\n \t\trevs->prune = 1;\n-\t\tload_ref_decorations(DECORATE_SHORT_REFS);\n+\t\tload_ref_decorations(DECORATE_SHORT_REFS, NULL);\n \t} else if (!strcmp(arg, \"--date-order\")) {\n \t\trevs->sort_order = REV_SORT_BY_COMMIT_DATE;\n \t\trevs->topo_order = 1;\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex 8f155da7a..e26d09a5c 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -737,6 +737,107 @@ test_expect_success 'log.decorate configuration' '\n \n '\n \n+test_expect_success 'decorate-refs with glob' '\n+\tcat >expect.decorate <<-\\EOF &&\n+\tMerge-tag-reach\n+\tMerge-tags-octopus-a-and-octopus-b\n+\tseventh\n+\toctopus-b (octopus-b)\n+\toctopus-a (octopus-a)\n+\treach\n+\tEOF\n+\tgit log -n6 --decorate=short --pretty=\"%f%d\" \\\n+\t\t--decorate-refs=\"heads/octopus*\" >actual &&\n+\ttest_cmp expect.decorate actual\n+'\n+\n+test_expect_success 'decorate-refs without globs' '\n+\tcat >expect.decorate <<-\\EOF &&\n+\tMerge-tag-reach\n+\tMerge-tags-octopus-a-and-octopus-b\n+\tseventh\n+\toctopus-b\n+\toctopus-a\n+\treach (tag: reach)\n+\tEOF\n+\tgit log -n6 --decorate=short --pretty=\"tformat:%f%d\" \\\n+\t\t--decorate-refs=\"tags/reach\" >actual &&\n+\ttest_cmp expect.decorate actual\n+'\n+\n+test_expect_success 'multiple decorate-refs' '\n+\tcat >expect.decorate <<-\\EOF &&\n+\tMerge-tag-reach\n+\tMerge-tags-octopus-a-and-octopus-b\n+\tseventh\n+\toctopus-b (octopus-b)\n+\toctopus-a (octopus-a)\n+\treach (tag: reach)\n+\tEOF\n+\tgit log -n6 --decorate=short --pretty='tformat:%f%d' \\\n+\t\t--decorate-refs='heads/octopus*' \\\n+\t\t--decorate-refs='tags/reach' >actual &&\n+ test_cmp expect.decorate actual\n+'\n+\n+test_expect_success 'decorate-refs-exclude with glob' '\n+\tcat >expect.decorate <<-\\EOF &&\n+\tMerge-tag-reach (HEAD -> master)\n+\tMerge-tags-octopus-a-and-octopus-b\n+\tseventh (tag: seventh)\n+\toctopus-b (tag: octopus-b)\n+\toctopus-a (tag: octopus-a)\n+\treach (tag: reach, reach)\n+\tEOF\n+\tgit log -n6 --decorate=short --pretty=\"%f%d\" \\\n+\t\t--decorate-refs-exclude=\"heads/octopus*\" >actual &&\n+\ttest_cmp expect.decorate actual\n+'\n+\n+test_expect_success 'decorate-refs-exclude without globs' '\n+\tcat >expect.decorate <<-\\EOF &&\n+\tMerge-tag-reach (HEAD -> master)\n+\tMerge-tags-octopus-a-and-octopus-b\n+\tseventh (tag: seventh)\n+\toctopus-b (tag: octopus-b, octopus-b)\n+\toctopus-a (tag: octopus-a, octopus-a)\n+\treach (reach)\n+\tEOF\n+\tgit log -n6 --decorate=short --pretty=\"tformat:%f%d\" \\\n+\t\t--decorate-refs-exclude=\"tags/reach\" >actual &&\n+\ttest_cmp expect.decorate actual\n+'\n+\n+test_expect_success 'multiple decorate-refs-exclude' '\n+\tcat >expect.decorate <<-\\EOF &&\n+\tMerge-tag-reach (HEAD -> master)\n+\tMerge-tags-octopus-a-and-octopus-b\n+\tseventh (tag: seventh)\n+\toctopus-b (tag: octopus-b)\n+\toctopus-a (tag: octopus-a)\n+\treach (reach)\n+\tEOF\n+\tgit log -n6 --decorate=short --pretty='tformat:%f%d' \\\n+\t\t--decorate-refs-exclude='heads/octopus*' \\\n+\t\t--decorate-refs-exclude='tags/reach' >actual &&\n+\ttest_cmp expect.decorate actual\n+'\n+\n+test_expect_success 'decorate-refs and decorate-refs-exclude' '\n+\tcat >expect.decorate <<-\\EOF &&\n+\tMerge-tag-reach (master)\n+\tMerge-tags-octopus-a-and-octopus-b\n+\tseventh\n+\toctopus-b\n+\toctopus-a\n+\treach (reach)\n+\tEOF\n+\tgit log -n6 --decorate=short --pretty='tformat:%f%d' \\\n+\t\t--decorate-refs='heads/*' \\\n+\t\t--decorate-refs-exclude='heads/oc*' >actual &&\n+\ttest_cmp expect.decorate actual\n+'\n+\n test_expect_success 'log.decorate config parsing' '\n \tgit log --oneline --decorate=full >expect.full &&\n \tgit log --oneline --decorate=short >expect.short &&\n-- \n2.15.0\n\n"},{"id":"331813","messageId":"xmqqo9oiok10.fsf@gitster.mtv.corp.google.com","threadId":"47115","inReplyTo":"20171104004144.5975-2-rafa.almas@gmail.com","subject":"Re: [PATCH v1 1/2] refs: extract function to normalize partial refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-04T02:27:39Z","receivedAt":"2017-11-04T02:27:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rafael Ascensão writes:\n\n> `for_each_glob_ref_in` has some code built into it that converts\n> partial refs like 'heads/master' to their full qualified form\n\ns/full/&y/ (read: that \"full\" needs \"y\" at the end).\n\n> 'refs/heads/master'. It also assume a trailing '/*' if no glob\n\ns/assume/&s/\n\n> characters are present in the pattern.\n>\n> Extract that logic to its own function which can be reused elsewhere\n> where the same behaviour is needed, and add an ENSURE_GLOB flag\n> to toggle if a trailing '/*' is to be appended to the result.\n>\n> Signed-off-by: Kevin Daudt \n> Signed-off-by: Rafael Ascensão \n\nCould you explain Kevin's sign-off we see above? It is a bit\nunusual (I am not yet saying it is wrong---I cannot judge until I\nfind out why it is there) to see a patch from person X with sign off\nfrom person Y and then person X in that order. It is normal for a\npatch authored by person X to have sign-off by X and then Y if X\nwrote it, signed it off and passed to Y, and then Y resent it after\nsigning it off (while preserving the authorship of X by adding an\nin-body From: header), but I do not think that is what we have here.\n\nIt could be that you did pretty much all the work on this patch\nand Kevin helped you to polish this patch off-list, in which case\nthe usual thing to do is to use \"Helped-by: Kevin\" instead.\n\n> ---\n> refs.c | 34 ++++++++++++++++++++--------------\n> refs.h | 16 ++++++++++++++++\n> 2 files changed, 36 insertions(+), 14 deletions(-)\n>\n> diff --git a/refs.c b/refs.c\n> index c590a992f..1e74b48e6 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -369,32 +369,38 @@ int head_ref_namespaced(each_ref_fn fn, void *cb_data)\n> \treturn ret;\n> }\n> \n> -int for_each_glob_ref_in(each_ref_fn fn, const char *pattern,\n> -\tconst char *prefix, void *cb_data)\n> +void normalize_glob_ref(struct strbuf *normalized_pattern, const char *prefix,\n> +\t\tconst char *pattern, int flags)\n\nIt is better to use \"unsigned\" for a single word \"flags\" used as a\ncollection of bits. In older parts of the codebase, we have\ncodepaths that pass signed int as a flags word, simply because we\ndidn't know better, but we do not have to spread that practice to\nnew code.\n\n> {\n> -\tstruct strbuf real_pattern = STRBUF_INIT;\n> -\tstruct ref_filter filter;\n> -\tint ret;\n> -\n> \tif (!prefix && !starts_with(pattern, \"refs/\"))\n> -\t\tstrbuf_addstr(&real_pattern, \"refs/\");\n> +\t\tstrbuf_addstr(normalized_pattern, \"refs/\");\n> \telse if (prefix)\n> -\t\tstrbuf_addstr(&real_pattern, prefix);\n> -\tstrbuf_addstr(&real_pattern, pattern);\n> +\t\tstrbuf_addstr(normalized_pattern, prefix);\n> +\tstrbuf_addstr(normalized_pattern, pattern);\n> \n> -\tif (!has_glob_specials(pattern)) {\n> +\tif (!has_glob_specials(pattern) && (flags & ENSURE_GLOB)) {\n> \t\t/* Append implied '/' '*' if not present. */\n> -\t\tstrbuf_complete(&real_pattern, '/');\n> +\t\tstrbuf_complete(normalized_pattern, '/');\n> \t\t/* No need to check for '*', there is none. */\n> -\t\tstrbuf_addch(&real_pattern, '*');\n> +\t\tstrbuf_addch(normalized_pattern, '*');\n> \t}\n> +}\n\nThe above looks like a pure and regression-free code movement (plus\na small new feature) that is faithful to the original, which is good.\n\nI however notice that addition of /* to the tail is trying to be\ncareful by using strbuf_complete('/'), but prefixing with \"refs/\"\ndoes not and we would end up with a double-slash if pattern begins\nwith a slash. The contract between the caller of this function (or\nits original, which is for_each_glob_ref_in()) and the callee is\nthat prefix must not begin with '/', so it may be OK, but we might\nwant to add \"if (*pattern == '/') BUG(...)\" at the beginning. \n\nI dunno. In any case, that is totally outside the scope of this two\npatch series.\n\n> diff --git a/refs.h b/refs.h\n> index a02b628c8..9f9a8bb27 100644\n> --- a/refs.h\n> +++ b/refs.h\n> @@ -312,6 +312,22 @@ int for_each_namespaced_ref(each_ref_fn fn, void *cb_data);\n> int refs_for_each_rawref(struct ref_store *refs, each_ref_fn fn, void *cb_data);\n> int for_each_rawref(each_ref_fn fn, void *cb_data);\n> \n> +/*\n> + * Normalizes partial refs to their full qualified form.\n\ns/full/&y/;\n\n> + * If prefix is NULL, will prepend 'refs/' to the pattern if it doesn't start\n> + * with 'refs/'. Results in refs/\n> + *\n> + * If prefix is not NULL will result in /\n\ns/NULL/&,/;\n\n> + *\n> + * If ENSURE_GLOB is set and no glob characters are found in the\n> + * pattern, a trailing <*> will be appended to the result.\n> + * (<> characters to avoid breaking C comment syntax)\n> + */\n> +\n> +#define ENSURE_GLOB 1\n> +void normalize_glob_ref (struct strbuf *normalized_pattern, const char *prefix,\n> +\t\t\t\tconst char *pattern, int flags);\n> +\n> static inline const char *has_glob_specials(const char *pattern)\n> {\n> \treturn strpbrk(pattern, \"?*[\");\n\nThanks. Other than the above minor points, looks good to me.\n"},{"id":"331814","messageId":"xmqq60aqn1ok.fsf@gitster.mtv.corp.google.com","threadId":"47115","inReplyTo":"20171104004144.5975-3-rafa.almas@gmail.com","subject":"Re: [PATCH v1 2/2] log: add option to choose which refs to decorate","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-04T03:49:15Z","receivedAt":"2017-11-04T04:07:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rafael Ascensão writes:\n\n> When `log --decorate` is used, git will decorate commits with all\n> available refs. While in most cases this the desired effect, under some\n> conditions it can lead to excessively verbose output.\n\nCorrect.\n\n> Using `--exclude=` can help mitigate that verboseness by\n> removing unnecessary 'branches' from the output. However, if the tip of\n> an excluded ref points to an ancestor of a non-excluded ref, git will\n> decorate it regardless.\n\nIs this even relevant? I think the above would only serve to\nconfuse the readers. --exclude, --branches, etc. are ways to\nspecify what starting points \"git log\" history traversal should\nbegin and has nothing to do with what set of refs are to be used to\ndecorate the commits that are shown. But the paragraph makes\nreaders wonder if it might have any effect in some circumstances.\n\n> With `--decorate-refs=`, only refs that match are\n> decorated while `--decorate-refs-exclude=` allows to do the\n> reverse, remove ref decorations that match \n\nAnd \"Only refs that match ... are decorated\" is also confusing. The\nthing is, refs are never decorated, they are used for decorating\ncommits in the output from \"git log\". For example, if you have \n\n\t---A---B---C---D\n\nand B is at the tip of the 'master' branch, the output from \"git log\nD\" would decorate B with 'master', even if you do not say 'master'\non the command line as the commit to start the traversal from.\n\nPerhaps drop the irrelevant paragraph about \"--exclude\" and write\nsomething like this instead?\n\n\tWhen \"--decorate-refs=\" is given, only the refs\n\tthat match the pattern is used in decoration. The refs that\n\tmatch the pattern, when \"--decorate-refs-exclude=\"\n\tis given, are never used in decoration.\n\n> Both can be used together but --decorate-refs-exclude patterns have\n> precedence over --decorate-refs patterns.\n\nA reasonable and an easy-to-explain way to mix zero or more positive\nand zero or more negagive patterns that follows the convention used\nelsewhere in the system (e.g. how negative pathspecs work) is\n\n (1) if there is no positive pattern given, pretend as if an\n inclusive default positive pattern was given;\n\n (2) for each candidate, reject it if it matches no positive\n pattern, or if it matches any one of negative patterns.\n\nFor pathspecs, we use \"everything\" as the inclusive default positive\npattern, I think, and for the set of refs used for decoration, a\nreasonable choice would also be to use \"everything\", which matches\nthe current behaviour.\n\n> The pattern follows similar rules as `--glob` except it doesn't assume a\n> trailing '/*' if glob characters are missing.\n\nWhy should this be a special case that burdens users to remember one\nmore rule? Wouldn't users find \"--decorate-refs=refs/tags\" useful\nand it woulld be shorter and nicer than having to say \"refs/tags/*\"?\n\n> diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt\n> index 32246fdb0..314417d89 100644\n> --- a/Documentation/git-log.txt\n> +++ b/Documentation/git-log.txt\n> @@ -38,6 +38,18 @@ OPTIONS\n> \tare shown as if 'short' were given, otherwise no ref names are\n> \tshown. The default option is 'short'.\n> \n> +--decorate-refs=::\n> +\tOnly print ref names that match the specified pattern. Uses the same\n> +\trules as `git rev-list --glob` except it doesn't assume a trailing a\n> +\ttrailing '/{asterisk}' if pattern lacks '?', '{asterisk}', or '['.\n> +\t`--decorate-refs-exlclude` has precedence.\n> +\n> +--decorate-refs-exclude=::\n> +\tDo not print ref names that match the specified pattern. Uses the same\n> +\trules as `git rev-list --glob` except it doesn't assume a trailing a\n> +\ttrailing '/{asterisk}' if pattern lacks '?', '{asterisk}', or '['.\n> +\tHas precedence over `--decorate-refs`.\n\nThese two may be technically correct, but I wonder if we can make it\neasier to understand (I found \"precedence\" bit hard to follow, as in\nmy mind, these are ANDed conditions and between (A & ~B), there is\nno \"precedence\"). Also we'd want to clarify what happens when only\n\"--decorate-refs-exclude\"s are given, which in turn necessitates us\nto describe what happens when only \"--decorate-refs\"s are given.\n\n> diff --git a/log-tree.c b/log-tree.c\n> index cea056234..8efc7ac3d 100644\n> --- a/log-tree.c\n> +++ b/log-tree.c\n> @@ -94,9 +94,33 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid,\n> {\n> \tstruct object *obj;\n> \tenum decoration_type type = DECORATION_NONE;\n> +\tstruct ref_include_exclude_list *filter = (struct ref_include_exclude_list *)cb_data;\n> +\tstruct string_list_item *item;\n> +\tstruct strbuf real_pattern = STRBUF_INIT;\n> +\n> +\tif(filter && filter->exclude->nr > 0) {\n\nHave SP before '('.\n\n> +\t\t/* if current ref is on the exclude list skip */\n> +\t\tfor_each_string_list_item(item, filter->exclude) {\n> +\t\t\tstrbuf_reset(&real_pattern);\n> +\t\t\tnormalize_glob_ref(&real_pattern, NULL, item->string, 0);\n> +\t\t\tif (!wildmatch(real_pattern.buf, refname, 0))\n> +\t\t\t\tgoto finish;\n> +\t\t}\n> +\t}\n> \n> -\tassert(cb_data == NULL);\n> +\tif (filter && filter->include->nr > 0) {\n> +\t\t/* if current ref is present on the include jump to decorate */\n> +\t\tfor_each_string_list_item(item, filter->include) {\n> +\t\t\tstrbuf_reset(&real_pattern);\n> +\t\t\tnormalize_glob_ref(&real_pattern, NULL, item->string, 0);\n> +\t\t\tif (!wildmatch(real_pattern.buf, refname, 0))\n> +\t\t\t\tgoto decorate;\n> +\t\t}\n> +\t\t/* Filter was given, but no match was found, skip */\n> +\t\tgoto finish;\n> +\t}\n\nThe above seems to implement the natural mixing of negative and\npositive patterns, which is good.\n\nUnless I am missing something, I think these normalize_grob_ref()\ncalls should be removed from this function; add_ref_decoration() is\ncalled once for EVERY ref the repository has, so you are normalizing\na handful of patterns you got from the user over and over to get the\nsame normalization, possibly thousands of times in a repository of a\nproject with long history.\n\nYou have finished collecting patterns on filter->{exclude,include}\nlist from the user by the time \"for_each_ref(add_ref_decoration)\" is\ncalled in load_ref_decorations(), and these patterns never changes\nafter that. \n\nPerhaps normalize the patterns inside load_ref_decorations() only\nonce and have the normalized patterns in the filter lists?\n\n> +decorate:\n> \tif (starts_with(refname, git_replace_ref_base)) {\n> \t\tstruct object_id original_oid;\n> \t\tif (!check_replace_refs)\n> @@ -136,6 +160,9 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid,\n> \t\t\tparse_object(&obj->oid);\n> \t\tadd_name_decoration(DECORATION_REF_TAG, refname, obj);\n> \t}\n> +\n> +finish:\n> +\tstrbuf_release(&real_pattern);\n> \treturn 0;\n> }\n> \n> @@ -148,15 +175,15 @@ static int add_graft_decoration(const struct commit_graft *graft, void *cb_data)\n> \treturn 0;\n> }\n> \n> -void load_ref_decorations(int flags)\n> +void load_ref_decorations(int flags, struct ref_include_exclude_list *data)\n> {\n> \tif (!decoration_loaded) {\n> \n> \t\tdecoration_loaded = 1;\n> \t\tdecoration_flags = flags;\n> -\t\tfor_each_ref(add_ref_decoration, NULL);\n> -\t\thead_ref(add_ref_decoration, NULL);\n> -\t\tfor_each_commit_graft(add_graft_decoration, NULL);\n> +\t\tfor_each_ref(add_ref_decoration, data);\n> +\t\thead_ref(add_ref_decoration, data);\n> +\t\tfor_each_commit_graft(add_graft_decoration, data);\n\nDon't name that variable \"data\".\n\nfor_each_*() and friends that take a callback with callback specific\ndata MUST call the callback specific data as generic, e.g. cb_data,\nbecause they do not know what they are passing. The callers of\nthese functions, like this one, however, know what they are passing.\nAlso load_ref_decorations() itself knows what its second parameter\nis.\n\n void load_ref_decorations(int flags, struct decoration_filter *filter)\n\nor something (see below).\n\n> \t}\n> }\n> \n> diff --git a/log-tree.h b/log-tree.h\n> index 48f11fb74..66563af88 100644\n> --- a/log-tree.h\n> +++ b/log-tree.h\n> @@ -7,6 +7,10 @@ struct log_info {\n> \tstruct commit *commit, *parent;\n> };\n> \n> +struct ref_include_exclude_list {\n> +\tstruct string_list *include, *exclude;\n> +};\n\nThe \"decoration\" is not the only thing related to \"ref\" in the\nlog-tree API; calling this structure that filters what refs to be\nused for decoration with the above name without saying that this is\nabout \"decoration\" is too selfish and unmaintainable.\n\nHow about \"struct decoration_filter\" and rename the fields to say\n\"{include,exclude}_ref_pattern\" or something like that? The\nrenaming of the fields to include \"ref\" somewhere is coming from the\nsame concern---it will be selfish and narrow-minded to imagine that\nthe ways to filter refs used for decoration will stay forever only\nbased on refnames and nothing else, which would be the reason not to\nhave \"ref\" somewhere in the names.\n\n"},{"id":"331815","messageId":"1e2e8f85-e13d-47f9-6661-1e685250c775@gmail.com","threadId":"47115","inReplyTo":"xmqqo9oiok10.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v1 1/2] refs: extract function to normalize partial refs","fromName":"Rafael Ascensão","fromEmail":"rafa.almas@gmail.com","sentAt":"2017-11-04T07:33:43Z","receivedAt":"2017-11-04T07:33:53Z","isPatch":true,"sender":{"key":"rafa.almas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1923789?v=4"},"body":"On 04/11/17 02:27, Junio C Hamano wrote:\n> Rafael Ascensão writes:\n> \n>> Signed-off-by: Kevin Daudt \n>> Signed-off-by: Rafael Ascensão \n> \n> Could you explain Kevin's sign-off we see above? It is a bit\n> unusual (I am not yet saying it is wrong---I cannot judge until I\n> find out why it is there) to see a patch from person X with sign off\n> from person Y and then person X in that order. It is normal for a\n> patch authored by person X to have sign-off by X and then Y if X\n> wrote it, signed it off and passed to Y, and then Y resent it after\n> signing it off (while preserving the authorship of X by adding an\n> in-body From: header), but I do not think that is what we have here.\n> \n> It could be that you did pretty much all the work on this patch\n> and Kevin helped you to polish this patch off-list, in which case\n> the usual thing to do is to use \"Helped-by: Kevin\" instead.\n\nThat's more or less what happened. I wouldn't say I did \"pretty much all \nthe work\". Yes, I wrote the code but with great help of Kevin. The \nintention of the dual Signed-off-by was to equally attribute authorship \nof the patch. But if that creates ambiguity I will change it to \n\"Helped-by\" as suggested.\n\n> It is better to use \"unsigned\" for a single word \"flags\" used as a\n> collection of bits. In older parts of the codebase, we have\n> codepaths that pass signed int as a flags word, simply because we\n> didn't know better, but we do not have to spread that practice to\n> new code.\n\nI noticed this, but chose to \"mimic\" the code around me. I'll correct it.\nOn a related note is there a guideline for defining flags or are\n`#define FLAG (1u << 0)`, `#define FLAG (1 << 0)`\n`#define FLAG 1` and `#define FLAG 0x1` equally accepted?\n\n>> {\n>> -\tstruct strbuf real_pattern = STRBUF_INIT;\n>> -\tstruct ref_filter filter;\n>> -\tint ret;\n>> -\n>> \tif (!prefix && !starts_with(pattern, \"refs/\"))\n>> -\t\tstrbuf_addstr(&real_pattern, \"refs/\");\n>> +\t\tstrbuf_addstr(normalized_pattern, \"refs/\");\n>> \telse if (prefix)\n>> -\t\tstrbuf_addstr(&real_pattern, prefix);\n>> -\tstrbuf_addstr(&real_pattern, pattern);\n>> +\t\tstrbuf_addstr(normalized_pattern, prefix);\n>> +\tstrbuf_addstr(normalized_pattern, pattern);\n>> \n>> -\tif (!has_glob_specials(pattern)) {\n>> +\tif (!has_glob_specials(pattern) && (flags & ENSURE_GLOB)) {\n>> \t\t/* Append implied '/' '*' if not present. */\n>> -\t\tstrbuf_complete(&real_pattern, '/');\n>> +\t\tstrbuf_complete(normalized_pattern, '/');\n>> \t\t/* No need to check for '*', there is none. */\n>> -\t\tstrbuf_addch(&real_pattern, '*');\n>> +\t\tstrbuf_addch(normalized_pattern, '*');\n>> \t}\n>> +}\n> \n> The above looks like a pure and regression-free code movement (plus\n> a small new feature) that is faithful to the original, which is good.\n> \n> I however notice that addition of /* to the tail is trying to be\n> careful by using strbuf_complete('/'), but prefixing with \"refs/\"\n> does not and we would end up with a double-slash if pattern begins\n> with a slash. The contract between the caller of this function (or\n> its original, which is for_each_glob_ref_in()) and the callee is\n> that prefix must not begin with '/', so it may be OK, but we might\n> want to add \"if (*pattern == '/') BUG(...)\" at the beginning.\n> \n> I dunno. In any case, that is totally outside the scope of this two\n> patch series.\n\nI guess it doesn't hurt adding that safety net.\n\n> Thanks. Other than the above minor points, looks good to me.\nI'll fix the mentioned issues. Thanks.\n"},{"id":"331816","messageId":"b0e3856b-e627-0d22-90da-3da1781f98b3@gmail.com","threadId":"47115","inReplyTo":"xmqq60aqn1ok.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v1 2/2] log: add option to choose which refs to decorate","fromName":"Rafael Ascensão","fromEmail":"rafa.almas@gmail.com","sentAt":"2017-11-04T07:34:20Z","receivedAt":"2017-11-04T07:34:27Z","isPatch":true,"sender":{"key":"rafa.almas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1923789?v=4"},"body":"On 04/11/17 03:49, Junio C Hamano wrote:\n> Rafael Ascensão writes:\n> \n>> Using `--exclude=` can help mitigate that verboseness by\n>> removing unnecessary 'branches' from the output. However, if the tip of\n>> an excluded ref points to an ancestor of a non-excluded ref, git will\n>> decorate it regardless.\n> \n> Is this even relevant? I think the above would only serve to\n> confuse the readers. --exclude, --branches, etc. are ways to\n> specify what starting points \"git log\" history traversal should\n> begin and has nothing to do with what set of refs are to be used to\n> decorate the commits that are shown. But the paragraph makes\n> readers wonder if it might have any effect in some circumstances.\n> \n>> With `--decorate-refs=`, only refs that match are\n>> decorated while `--decorate-refs-exclude=` allows to do the\n>> reverse, remove ref decorations that match \n> \n> And \"Only refs that match ... are decorated\" is also confusing. The\n> thing is, refs are never decorated, they are used for decorating\n> commits in the output from \"git log\". For example, if you have \n> \n> \t---A---B---C---D\n> \n> and B is at the tip of the 'master' branch, the output from \"git log\n> D\" would decorate B with 'master', even if you do not say 'master'\n> on the command line as the commit to start the traversal from. >\n> Perhaps drop the irrelevant paragraph about \"--exclude\" and write\n> something like this instead?\n> \n> \tWhen \"--decorate-refs=\" is given, only the refs\n> \tthat match the pattern is used in decoration. The refs that\n> \tmatch the pattern, when \"--decorate-refs-exclude=\"\n> \tis given, are never used in decoration.\n> \n\nWhat you explained was the reason I mentioned that. Because some users \nwere wrongfully trying to remove decorations by trying to exclude the \nstarting points. But I agree this adds little value and can generate \nfurther confusion. I will remove that section.\n\n>> Both can be used together but --decorate-refs-exclude patterns have\n>> precedence over --decorate-refs patterns.\n> \n> A reasonable and an easy-to-explain way to mix zero or more positive\n> and zero or more negagive patterns that follows the convention used\n> elsewhere in the system (e.g. how negative pathspecs work) is\n> \n> (1) if there is no positive pattern given, pretend as if an\n> inclusive default positive pattern was given;\n> \n> (2) for each candidate, reject it if it matches no positive\n> pattern, or if it matches any one of negative patterns.\n> \n> For pathspecs, we use \"everything\" as the inclusive default positive\n> pattern, I think, and for the set of refs used for decoration, a\n> reasonable choice would also be to use \"everything\", which matches\n> the current behaviour.\n> \n\nThat's a nice explanation that fits the current \"--decorate-refs\" behavior.\n\n>> The pattern follows similar rules as `--glob` except it doesn't assume a\n>> trailing '/*' if glob characters are missing.\n> \n> Why should this be a special case that burdens users to remember one\n> more rule? Wouldn't users find \"--decorate-refs=refs/tags\" useful\n> and it woulld be shorter and nicer than having to say \"refs/tags/*\"?\n> \n\nI wanted to allow exact patterns like:\n\"--decorate-refs=refs/heads/master\" and for that I disabled the flag \nthat adds the trailing '/*' if no globs are found. As a side effect, I \nlost the shortcut.\n\nIs adding a yet another flag that appends '/*' only if the pattern \nequals \"refs/{heads,remotes,tags}\" a good idea?\n\nBecause changing the default behavior of that function has implications \non multiple commands which I think shouldn't change. But at the same \ntime, would be nice to have the logic that deals with glob-ref patterns \nall in one place.\n\nWhat's the sane way to do this?\n\n>> diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt\n>> index 32246fdb0..314417d89 100644\n>> --- a/Documentation/git-log.txt\n>> +++ b/Documentation/git-log.txt\n>> @@ -38,6 +38,18 @@ OPTIONS\n>> \tare shown as if 'short' were given, otherwise no ref names are\n>> \tshown. The default option is 'short'.\n>> \n>> +--decorate-refs=::\n>> +\tOnly print ref names that match the specified pattern. Uses the same\n>> +\trules as `git rev-list --glob` except it doesn't assume a trailing a\n>> +\ttrailing '/{asterisk}' if pattern lacks '?', '{asterisk}', or '['.\n>> +\t`--decorate-refs-exlclude` has precedence.\n>> +\n>> +--decorate-refs-exclude=::\n>> +\tDo not print ref names that match the specified pattern. Uses the same\n>> +\trules as `git rev-list --glob` except it doesn't assume a trailing a\n>> +\ttrailing '/{asterisk}' if pattern lacks '?', '{asterisk}', or '['.\n>> +\tHas precedence over `--decorate-refs`.\n\n> These two may be technically correct, but I wonder if we can make it\n> easier to understand (I found \"precedence\" bit hard to follow, as in\n> my mind, these are ANDed conditions and between (A & ~B), there is\n> no \"precedence\"). Also we'd want to clarify what happens when only\n> \"--decorate-refs-exclude\"s are given, which in turn necessitates us\n> to describe what happens when only \"--decorate-refs\"s are given.\n\nI believe the same explanation mentioned earlier fits nicely here too.\n\n>> diff --git a/log-tree.c b/log-tree.c\n>> index cea056234..8efc7ac3d 100644\n>> --- a/log-tree.c\n>> +++ b/log-tree.c\n>> @@ -94,9 +94,33 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid,\n>> {\n>> \tstruct object *obj;\n>> \tenum decoration_type type = DECORATION_NONE;\n>> +\tstruct ref_include_exclude_list *filter = (struct ref_include_exclude_list *)cb_data;\n>> +\tstruct string_list_item *item;\n>> +\tstruct strbuf real_pattern = STRBUF_INIT;\n>> +\n>> +\tif(filter && filter->exclude->nr > 0) {\n> \n> Have SP before '('.\n> \n>> +\t\t/* if current ref is on the exclude list skip */\n>> +\t\tfor_each_string_list_item(item, filter->exclude) {\n>> +\t\t\tstrbuf_reset(&real_pattern);\n>> +\t\t\tnormalize_glob_ref(&real_pattern, NULL, item->string, 0);\n>> +\t\t\tif (!wildmatch(real_pattern.buf, refname, 0))\n>> +\t\t\t\tgoto finish;\n>> +\t\t}\n>> +\t}\n>> \n>> -\tassert(cb_data == NULL);\n>> +\tif (filter && filter->include->nr > 0) {\n>> +\t\t/* if current ref is present on the include jump to decorate */\n>> +\t\tfor_each_string_list_item(item, filter->include) {\n>> +\t\t\tstrbuf_reset(&real_pattern);\n>> +\t\t\tnormalize_glob_ref(&real_pattern, NULL, item->string, 0);\n>> +\t\t\tif (!wildmatch(real_pattern.buf, refname, 0))\n>> +\t\t\t\tgoto decorate;\n>> +\t\t}\n>> +\t\t/* Filter was given, but no match was found, skip */\n>> +\t\tgoto finish;\n>> +\t}\n> \n> The above seems to implement the natural mixing of negative and\n> positive patterns, which is good.\n> \n> Unless I am missing something, I think these normalize_grob_ref()\n> calls should be removed from this function; add_ref_decoration() is\n> called once for EVERY ref the repository has, so you are normalizing\n> a handful of patterns you got from the user over and over to get the\n> same normalization, possibly thousands of times in a repository of a\n> project with long history.\n> \n> You have finished collecting patterns on filter->{exclude,include}\n> list from the user by the time \"for_each_ref(add_ref_decoration)\" is\n> called in load_ref_decorations(), and these patterns never changes\n> after that.\n> \n> Perhaps normalize the patterns inside load_ref_decorations() only\n> once and have the normalized patterns in the filter lists?\n> \nThis would be what a sane person would do. This detail went over my \nhead. Will move it to load_ref_decorations()\n\n>> +decorate:\n>> \tif (starts_with(refname, git_replace_ref_base)) {\n>> \t\tstruct object_id original_oid;\n>> \t\tif (!check_replace_refs)\n>> @@ -136,6 +160,9 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid,\n>> \t\t\tparse_object(&obj->oid);\n>> \t\tadd_name_decoration(DECORATION_REF_TAG, refname, obj);\n>> \t}\n>> +\n>> +finish:\n>> +\tstrbuf_release(&real_pattern);\n>> \treturn 0;\n>> }\n>> \n>> @@ -148,15 +175,15 @@ static int add_graft_decoration(const struct commit_graft *graft, void *cb_data)\n>> \treturn 0;\n>> }\n>> \n>> -void load_ref_decorations(int flags)\n>> +void load_ref_decorations(int flags, struct ref_include_exclude_list *data)\n>> {\n>> \tif (!decoration_loaded) {\n>> \n>> \t\tdecoration_loaded = 1;\n>> \t\tdecoration_flags = flags;\n>> -\t\tfor_each_ref(add_ref_decoration, NULL);\n>> -\t\thead_ref(add_ref_decoration, NULL);\n>> -\t\tfor_each_commit_graft(add_graft_decoration, NULL);\n>> +\t\tfor_each_ref(add_ref_decoration, data);\n>> +\t\thead_ref(add_ref_decoration, data);\n>> +\t\tfor_each_commit_graft(add_graft_decoration, data);\n> \n> Don't name that variable \"data\".\n> \n> for_each_*() and friends that take a callback with callback specific\n> data MUST call the callback specific data as generic, e.g. cb_data,\n> because they do not know what they are passing. The callers of\n> these functions, like this one, however, know what they are passing.\n> Also load_ref_decorations() itself knows what its second parameter\n> is.\n> \n> void load_ref_decorations(int flags, struct decoration_filter *filter)\n> \n> or something (see below).\n> \n>> \t}\n>> }\n>> \n>> diff --git a/log-tree.h b/log-tree.h\n>> index 48f11fb74..66563af88 100644\n>> --- a/log-tree.h\n>> +++ b/log-tree.h\n>> @@ -7,6 +7,10 @@ struct log_info {\n>> \tstruct commit *commit, *parent;\n>> };\n>> \n>> +struct ref_include_exclude_list {\n>> +\tstruct string_list *include, *exclude;\n>> +};\n> \n> The \"decoration\" is not the only thing related to \"ref\" in the\n> log-tree API; calling this structure that filters what refs to be\n> used for decoration with the above name without saying that this is\n> about \"decoration\" is too selfish and unmaintainable.\n> \n> How about \"struct decoration_filter\" and rename the fields to say\n> \"{include,exclude}_ref_pattern\" or something like that? The\n> renaming of the fields to include \"ref\" somewhere is coming from the\n> same concern---it will be selfish and narrow-minded to imagine that\n> the ways to filter refs used for decoration will stay forever only\n> based on refnames and nothing else, which would be the reason not to\n> have \"ref\" somewhere in the names.\n> \nI will make the corrections. Thanks for the feedback.\n"},{"id":"331832","messageId":"20171104224511.22609-1-me@ikke.info","threadId":"47115","inReplyTo":"xmqqo9oiok10.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v1 1/2] refs: extract function to normalize partial refs","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2017-11-04T22:45:11Z","receivedAt":"2017-11-04T22:46:11Z","isPatch":true,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"On Sat, Nov 04, 2017 at 11:27:39AM +0900, Junio C Hamano wrote:\n> I however notice that addition of /* to the tail is trying to be\n> careful by using strbuf_complete('/'), but prefixing with \"refs/\"\n> does not and we would end up with a double-slash if pattern begins\n> with a slash. The contract between the caller of this function (or\n> its original, which is for_each_glob_ref_in()) and the callee is\n> that prefix must not begin with '/', so it may be OK, but we might\n> want to add \"if (*pattern == '/') BUG(...)\" at the beginning.\n>\n> I dunno. In any case, that is totally outside the scope of this two\n> patch series.\n\nI do think it's a good idea to make future readers of the code aware of\nthis contract, and adding a BUG assert does that quite well. Here is a\npatch that implements it.\n\nThis applies of course on top of this patch series.\n\n-- >8 --\nSubject: [PATCH] normalize_glob_ref: assert implicit contract of prefix\n\nnormalize_glob_ref has an implicit contract of expecting 'prefix' to not\nstart with a '/', otherwise the pattern would end up with a\ndouble-slash.\n\nMark it as a BUG when the prefix argument of normalize_glob_ref starts\nwith a '/' so that future callers will be aware of this contract.\n\nSigned-off-by: Kevin Daudt \n---\n refs.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/refs.c b/refs.c\nindex e9ae659ae..6747981d1 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -372,6 +372,8 @@ int head_ref_namespaced(each_ref_fn fn, void *cb_data)\n void normalize_glob_ref(struct strbuf *normalized_pattern, const char *prefix,\n \t\tconst char *pattern, int flags)\n {\n+\tif (prefix && *prefix == '/') BUG(\"prefix cannot not start with '/'\");\n+\n \tif (!prefix && !starts_with(pattern, \"refs/\"))\n \t\tstrbuf_addstr(normalized_pattern, \"refs/\");\n \telse if (prefix)\n-- \n2.15.0.rc2.57.g2f899857a9\n\n"},{"id":"331836","messageId":"xmqq1sldmqms.fsf@gitster.mtv.corp.google.com","threadId":"47115","inReplyTo":"b0e3856b-e627-0d22-90da-3da1781f98b3@gmail.com","subject":"Re: [PATCH v1 2/2] log: add option to choose which refs to decorate","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-05T02:00:11Z","receivedAt":"2017-11-05T02:03:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rafael Ascensão writes:\n\n>>> The pattern follows similar rules as `--glob` except it doesn't assume a\n>>> trailing '/*' if glob characters are missing.\n>>\n>> Why should this be a special case that burdens users to remember one\n>> more rule? Wouldn't users find \"--decorate-refs=refs/tags\" useful\n>> and it woulld be shorter and nicer than having to say \"refs/tags/*\"?\n>\n> I wanted to allow exact patterns like:\n> \"--decorate-refs=refs/heads/master\" and for that I disabled the flag\n> that adds the trailing '/*' if no globs are found. As a side effect, I\n> lost the shortcut.\n>\n> Is adding a yet another flag that appends '/*' only if the pattern\n> equals \"refs/{heads,remotes,tags}\" a good idea?\n\nNo.\n\n> Because changing the default behavior of that function has\n> implications on multiple commands which I think shouldn't change. But\n> at the same time, would be nice to have the logic that deals with\n> glob-ref patterns all in one place.\n>\n> What's the sane way to do this?\n\nLearn to type \"--decorate-refs=\"refs/heads/[m]aster\", and not twewak\nthe code at all, perhaps. The users of existing \"with no globbing,\n/* is appended\" interface are already used to that way and they do\nnot have to learn a new and inconsistent interface.\n\nAfter all, \"I only want to see 'git log' output with 'master'\ndecorated\" (i.e. not specifying \"this class of refs I can glob by\nusing the naming convention I am using\" and instead enumerating the\nones you care about) does not sound like a sensible thing people\noften want to do, so making it follow the other codepath so that\npeople can say \"refs/tags\" to get \"refs/tags/*\", while still allowing\nsuch a rare but specific and exact one possible, may not sound too\nbad to me.\n\n"},{"id":"331842","messageId":"xmqqshdtl057.fsf@gitster.mtv.corp.google.com","threadId":"47115","inReplyTo":"xmqq1sldmqms.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v1 2/2] log: add option to choose which refs to decorate","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-05T06:17:40Z","receivedAt":"2017-11-05T06:17:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n> Rafael Ascensão writes:\n> ...\n>> Because changing the default behavior of that function has\n>> implications on multiple commands which I think shouldn't change. But\n>> at the same time, would be nice to have the logic that deals with\n>> glob-ref patterns all in one place.\n>>\n>> What's the sane way to do this?\n>\n> Learn to type \"--decorate-refs=\"refs/heads/[m]aster\", and not twewak\n> the code at all, perhaps. The users of existing \"with no globbing,\n> /* is appended\" interface are already used to that way and they do\n> not have to learn a new and inconsistent interface.\n>\n> After all, \"I only want to see 'git log' output with 'master'\n> decorated\" (i.e. not specifying \"this class of refs I can glob by\n> using the naming convention I am using\" and instead enumerating the\n> ones you care about) does not sound like a sensible thing people\n> often want to do, so making it follow the other codepath so that\n> people can say \"refs/tags\" to get \"refs/tags/*\", while still allowing\n> such a rare but specific and exact one possible, may not sound too\n> bad to me.\n\nHaving said all that, I can imagine another way out might be to\nchange the behaviour of this \"normalize\" thing to add two patterns,\nthe original pattern in addition to the original pattern plus \"/*\",\nwhen it sees a pattern without any glob. Many users who relied on\nthe current behaviour fed \"refs/tags\" knowing that it will match\neverything under \"refs/tags\" i.e. \"refs/tags/*\", and they cannot\nhave a ref that is exactly \"refs/tags\", so adding the original\npattern without an extra trailing \"/*\" would not hurt them. And\nthis will allow you to say \"refs/heads/master\" when you know you\nwant that exact ref, and in such a repository where that original\npattern without trailing \"/*\" would be useful, because you cannot\nhave \"refs/heads/master/one\" at the same time, having an extra\npattern that is the original plus \"/*\" would not hurt you, either.\n\nThis however needs a bit of thought to see if there are corner cases\nthat may result in unexpected and unwanted fallout, and something I\nam reluctant to declare unilaterally that it is a better way to go.\n\nThoughts?\n"},{"id":"331867","messageId":"21ec16d1-cca1-0f6d-f389-b64de91dcef3@alum.mit.edu","threadId":"47115","inReplyTo":"20171104224511.22609-1-me@ikke.info","subject":"Re: [PATCH v1 1/2] refs: extract function to normalize partial refs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-11-05T13:21:03Z","receivedAt":"2017-11-05T13:21:15Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 11/04/2017 11:45 PM, Kevin Daudt wrote:\n> On Sat, Nov 04, 2017 at 11:27:39AM +0900, Junio C Hamano wrote:\n>> I however notice that addition of /* to the tail is trying to be\n>> careful by using strbuf_complete('/'), but prefixing with \"refs/\"\n>> does not and we would end up with a double-slash if pattern begins\n>> with a slash. The contract between the caller of this function (or\n>> its original, which is for_each_glob_ref_in()) and the callee is\n>> that prefix must not begin with '/', so it may be OK, but we might\n>> want to add \"if (*pattern == '/') BUG(...)\" at the beginning.\n>>\n>> I dunno. In any case, that is totally outside the scope of this two\n>> patch series.\n> \n> I do think it's a good idea to make future readers of the code aware of\n> this contract, and adding a BUG assert does that quite well. Here is a\n> patch that implements it.\n> \n> This applies of course on top of this patch series.\n> \n> -- >8 --\n> Subject: [PATCH] normalize_glob_ref: assert implicit contract of prefix\n> \n> normalize_glob_ref has an implicit contract of expecting 'prefix' to not\n> start with a '/', otherwise the pattern would end up with a\n> double-slash.\n> \n> Mark it as a BUG when the prefix argument of normalize_glob_ref starts\n> with a '/' so that future callers will be aware of this contract.\n> \n> Signed-off-by: Kevin Daudt \n> ---\n> refs.c | 2 ++\n> 1 file changed, 2 insertions(+)\n> \n> diff --git a/refs.c b/refs.c\n> index e9ae659ae..6747981d1 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -372,6 +372,8 @@ int head_ref_namespaced(each_ref_fn fn, void *cb_data)\n> void normalize_glob_ref(struct strbuf *normalized_pattern, const char *prefix,\n> \t\tconst char *pattern, int flags)\n> {\n> +\tif (prefix && *prefix == '/') BUG(\"prefix cannot not start with '/'\");\n\nThis should be split onto two lines.\n\nAlso, \"prefix cannot not start ...\" has two \"not\". I suggest changing it\nto \"prefix must not start ...\", because that makes it clearer that the\ncaller is at fault.\n\nWhat if the caller passes the empty string as prefix? In that case, the\nend result would be \"/\", which is also bogus.\n\n> +\n> \tif (!prefix && !starts_with(pattern, \"refs/\"))\n> \t\tstrbuf_addstr(normalized_pattern, \"refs/\");\n> \telse if (prefix)\n\nMichael\n"},{"id":"331868","messageId":"4dc4eefc-56b9-1b13-ae46-83a3af9c7ee3@alum.mit.edu","threadId":"47115","inReplyTo":"20171104004144.5975-2-rafa.almas@gmail.com","subject":"Re: [PATCH v1 1/2] refs: extract function to normalize partial refs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-11-05T13:42:34Z","receivedAt":"2017-11-05T13:42:46Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 11/04/2017 01:41 AM, Rafael Ascensão wrote:\n> `for_each_glob_ref_in` has some code built into it that converts\n> partial refs like 'heads/master' to their full qualified form\n> 'refs/heads/master'. It also assume a trailing '/*' if no glob\n> characters are present in the pattern.\n> \n> Extract that logic to its own function which can be reused elsewhere\n> where the same behaviour is needed, and add an ENSURE_GLOB flag\n> to toggle if a trailing '/*' is to be appended to the result.\n> \n> Signed-off-by: Kevin Daudt \n> Signed-off-by: Rafael Ascensão \n> ---\n> refs.c | 34 ++++++++++++++++++++--------------\n> refs.h | 16 ++++++++++++++++\n> 2 files changed, 36 insertions(+), 14 deletions(-)\n> \n> diff --git a/refs.c b/refs.c\n> index c590a992f..1e74b48e6 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -369,32 +369,38 @@ int head_ref_namespaced(each_ref_fn fn, void *cb_data)\n> \treturn ret;\n> }\n> \n> -int for_each_glob_ref_in(each_ref_fn fn, const char *pattern,\n> -\tconst char *prefix, void *cb_data)\n> +void normalize_glob_ref(struct strbuf *normalized_pattern, const char *prefix,\n> +\t\tconst char *pattern, int flags)\n> {\n> -\tstruct strbuf real_pattern = STRBUF_INIT;\n> -\tstruct ref_filter filter;\n> -\tint ret;\n> -\n> \tif (!prefix && !starts_with(pattern, \"refs/\"))\n> -\t\tstrbuf_addstr(&real_pattern, \"refs/\");\n> +\t\tstrbuf_addstr(normalized_pattern, \"refs/\");\n> \telse if (prefix)\n> -\t\tstrbuf_addstr(&real_pattern, prefix);\n> -\tstrbuf_addstr(&real_pattern, pattern);\n> +\t\tstrbuf_addstr(normalized_pattern, prefix);\n> +\tstrbuf_addstr(normalized_pattern, pattern);\n\nI realize that the old code did this too, but while you are in the area\nit might be nice to simplify the logic from\n\n\tif (!prefix && !starts_with(pattern, \"refs/\"))\n\t\tstrbuf_addstr(normalized_pattern, \"refs/\");\n\telse if (prefix)\n\t\tstrbuf_addstr(normalized_pattern, prefix);\n\nto\n\n\tif (prefix)\n\t\tstrbuf_addstr(normalized_pattern, prefix);\n\telse if (!starts_with(pattern, \"refs/\"))\n\t\tstrbuf_addstr(normalized_pattern, \"refs/\");\n\nThis would avoid having to check twice whether `prefix` is NULL.\n\n> -\tif (!has_glob_specials(pattern)) {\n> +\tif (!has_glob_specials(pattern) && (flags & ENSURE_GLOB)) {\n> \t\t/* Append implied '/' '*' if not present. */\n> -\t\tstrbuf_complete(&real_pattern, '/');\n> +\t\tstrbuf_complete(normalized_pattern, '/');\n> \t\t/* No need to check for '*', there is none. */\n> -\t\tstrbuf_addch(&real_pattern, '*');\n> +\t\tstrbuf_addch(normalized_pattern, '*');\n> \t}\n> +}\n> +\n> +int for_each_glob_ref_in(each_ref_fn fn, const char *pattern,\n> +\tconst char *prefix, void *cb_data)\n> +{\n> +\tstruct strbuf normalized_pattern = STRBUF_INIT;\n> +\tstruct ref_filter filter;\n> +\tint ret;\n> +\n> +\tnormalize_glob_ref(&normalized_pattern, prefix, pattern, ENSURE_GLOB);\n> \n> -\tfilter.pattern = real_pattern.buf;\n> +\tfilter.pattern = normalized_pattern.buf;\n> \tfilter.fn = fn;\n> \tfilter.cb_data = cb_data;\n> \tret = for_each_ref(filter_refs, &filter);\n> \n> -\tstrbuf_release(&real_pattern);\n> +\tstrbuf_release(&normalized_pattern);\n> \treturn ret;\n> }\n> \n> diff --git a/refs.h b/refs.h\n> index a02b628c8..9f9a8bb27 100644\n> --- a/refs.h\n> +++ b/refs.h\n> @@ -312,6 +312,22 @@ int for_each_namespaced_ref(each_ref_fn fn, void *cb_data);\n> int refs_for_each_rawref(struct ref_store *refs, each_ref_fn fn, void *cb_data);\n> int for_each_rawref(each_ref_fn fn, void *cb_data);\n> \n> +/*\n> + * Normalizes partial refs to their full qualified form.\n> + * If prefix is NULL, will prepend 'refs/' to the pattern if it doesn't start\n> + * with 'refs/'. Results in refs/\n> + *\n> + * If prefix is not NULL will result in /\n> + *\n> + * If ENSURE_GLOB is set and no glob characters are found in the\n> + * pattern, a trailing <*> will be appended to the result.\n> + * (<> characters to avoid breaking C comment syntax)\n> + */\n> +\n> +#define ENSURE_GLOB 1\n> +void normalize_glob_ref (struct strbuf *normalized_pattern, const char *prefix,\n> +\t\t\t\tconst char *pattern, int flags);\n\nThere shouldn't be a space between the function name and the open\nparenthesis.\n\nYou have complicated the interface by allowing an `ENSURE_BLOB` flag.\nThis would make sense if the logic for normalizing the prefix were\ntangled up with the logic for adding the suffix. But in fact they are\nalmost entirely orthogonal [1].\n\nSo the interface might be simplified by having two functions,\n\n void normalize_glob_ref(normalized_pattern, prefix, pattern);\n void ensure_blob(struct strbuf *pattern);\n\nThe caller in this patch would call the functions one after the other\n(or the `ensure_blob` behavior could be inlined in\n`for_each_glob_ref_in()`, since it doesn't yet have any callers). And\nthe callers introduced in patch 2 would only need to call the first\nfunction.\n\n> static inline const char *has_glob_specials(const char *pattern)\n> {\n> \treturn strpbrk(pattern, \"?*[\");\n> \n\nMichael\n\n[1] I say \"almost entirely\" because putting them in one function means\nthat only `pattern` needs to be scanned for glob characters. But that is\nan unimportant detail.\n"},{"id":"331895","messageId":"xmqqwp34jj3h.fsf@gitster.mtv.corp.google.com","threadId":"47115","inReplyTo":"4dc4eefc-56b9-1b13-ae46-83a3af9c7ee3@alum.mit.edu","subject":"Re: [PATCH v1 1/2] refs: extract function to normalize partial refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-06T01:23:30Z","receivedAt":"2017-11-06T01:23:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> [1] I say \"almost entirely\" because putting them in one function means\n> that only `pattern` needs to be scanned for glob characters. But that is\n> an unimportant detail.\n\nThat could actually be an important detail, in that even if prefix\nhas wildcard, we'd still append the trailing \"/*\" as long as the\npattern does not, right?\n"},{"id":"331902","messageId":"4b0fd3ff-3331-14be-2478-03f44ae3b0a9@gmail.com","threadId":"47115","inReplyTo":"xmqqwp34jj3h.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v1 1/2] refs: extract function to normalize partial refs","fromName":"Rafael Ascensão","fromEmail":"rafa.almas@gmail.com","sentAt":"2017-11-06T02:37:55Z","receivedAt":"2017-11-06T02:38:03Z","isPatch":true,"sender":{"key":"rafa.almas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1923789?v=4"},"body":"On 06/11/17 01:23, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> [1] I say \"almost entirely\" because putting them in one function means\n>> that only `pattern` needs to be scanned for glob characters. But that is\n>> an unimportant detail.\n> \n> That could actually be an important detail, in that even if prefix\n> has wildcard, we'd still append the trailing \"/*\" as long as the\n> pattern does not, right?\n> \n\n> So the interface might be simplified by having two functions,\n> \n> void normalize_glob_ref(normalized_pattern, prefix, pattern);\n> void ensure_blob(struct strbuf *pattern);\n\nI think that flag no longer makes sense. I added it just to allow\n'--decorate-refs' work with \"exact patterns\". And since that has the\nugly side effect of losing the ability to use \"shortcut patterns\" like\n'tags' to refer to 'refs/tags/*', I believe it's a good idea to remove it.\n"},{"id":"331904","messageId":"9168f7b8-3b9d-a933-e542-ae5b741cb824@gmail.com","threadId":"47115","inReplyTo":"xmqqshdtl057.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v1 2/2] log: add option to choose which refs to decorate","fromName":"Rafael Ascensão","fromEmail":"rafa.almas@gmail.com","sentAt":"2017-11-06T03:24:02Z","receivedAt":"2017-11-06T03:24:11Z","isPatch":true,"sender":{"key":"rafa.almas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1923789?v=4"},"body":"Would checking the output of ref_exists() make sense here?\nBy that I mean, only add a trailing '/*' if the ref doesn't exist.\n\nUnless I am missing something obvious this would allow us to keep both\nshortcuts and exact patterns.\n"},{"id":"331905","messageId":"xmqqpo8whxot.fsf@gitster.mtv.corp.google.com","threadId":"47115","inReplyTo":"9168f7b8-3b9d-a933-e542-ae5b741cb824@gmail.com","subject":"Re: [PATCH v1 2/2] log: add option to choose which refs to decorate","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-06T03:51:14Z","receivedAt":"2017-11-06T03:51:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rafael Ascensão writes:\n\n> Would checking the output of ref_exists() make sense here?\n> By that I mean, only add a trailing '/*' if the ref doesn't exist.\n\nI do not think it would hurt, but it is not immediately obvious if\nthe benefit of doing so outweighs the cost of having to make an\nextra call to ref_exists().\n\n\n"},{"id":"331909","messageId":"1ae0c199-f0fe-4468-1481-c7217ef6cc11@alum.mit.edu","threadId":"47115","inReplyTo":"xmqqwp34jj3h.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v1 1/2] refs: extract function to normalize partial refs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-11-06T07:00:29Z","receivedAt":"2017-11-06T07:00:47Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 11/06/2017 02:23 AM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> [1] I say \"almost entirely\" because putting them in one function means\n>> that only `pattern` needs to be scanned for glob characters. But that is\n>> an unimportant detail.\n> \n> That could actually be an important detail, in that even if prefix\n> has wildcard, we'd still append the trailing \"/*\" as long as the\n> pattern does not, right?\n\nThat's correct, but I was assuming that the prefix would always be a\nhard-coded string like \"refs/tags/\" or maybe \"refs/\". (That is the case\nnow.) It doesn't seem very useful to use a prefix like \"refs/*/\".\n\nMichael\n"},{"id":"331910","messageId":"c1b9cb69-0fdf-a58c-62cc-343a6abdbb84@alum.mit.edu","threadId":"47115","inReplyTo":"xmqqshdtl057.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v1 2/2] log: add option to choose which refs to decorate","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-11-06T07:09:57Z","receivedAt":"2017-11-06T07:10:07Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 11/05/2017 07:17 AM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>> Rafael Ascensão writes:\n>> ...\n>>> Because changing the default behavior of that function has\n>>> implications on multiple commands which I think shouldn't change. But\n>>> at the same time, would be nice to have the logic that deals with\n>>> glob-ref patterns all in one place.\n>>>\n>>> What's the sane way to do this?\n>>\n>> Learn to type \"--decorate-refs=\"refs/heads/[m]aster\", and not twewak\n>> the code at all, perhaps. The users of existing \"with no globbing,\n>> /* is appended\" interface are already used to that way and they do\n>> not have to learn a new and inconsistent interface.\n>>\n>> After all, \"I only want to see 'git log' output with 'master'\n>> decorated\" (i.e. not specifying \"this class of refs I can glob by\n>> using the naming convention I am using\" and instead enumerating the\n>> ones you care about) does not sound like a sensible thing people\n>> often want to do, so making it follow the other codepath so that\n>> people can say \"refs/tags\" to get \"refs/tags/*\", while still allowing\n>> such a rare but specific and exact one possible, may not sound too\n>> bad to me.\n> \n> Having said all that, I can imagine another way out might be to\n> change the behaviour of this \"normalize\" thing to add two patterns,\n> the original pattern in addition to the original pattern plus \"/*\",\n> when it sees a pattern without any glob. Many users who relied on\n> the current behaviour fed \"refs/tags\" knowing that it will match\n> everything under \"refs/tags\" i.e. \"refs/tags/*\", and they cannot\n> have a ref that is exactly \"refs/tags\", so adding the original\n> pattern without an extra trailing \"/*\" would not hurt them. And\n> this will allow you to say \"refs/heads/master\" when you know you\n> want that exact ref, and in such a repository where that original\n> pattern without trailing \"/*\" would be useful, because you cannot\n> have \"refs/heads/master/one\" at the same time, having an extra\n> pattern that is the original plus \"/*\" would not hurt you, either.\n> \n> This however needs a bit of thought to see if there are corner cases\n> that may result in unexpected and unwanted fallout, and something I\n> am reluctant to declare unilaterally that it is a better way to go.\n\nThere's some glob-matching code (somewhere? I don't know if it's allowed\neverywhere) that allows \"**\" to mean \"zero or one path components. If\n\"refs/tags\" were massaged to be \"refs/tags/**\", then it would match not only\n\n refs/tags\n refs/tags/foo\n\nbut also\n\n refs/tags/foo/bar\n\n, which is probably another thing that the user would expect to see.\n\nThere's at least some precedent for this kind of expansion: `git\nfor-each-ref refs/remotes` lists *all* references under that prefix,\neven if they have multiple levels.\n\nMichael\n"},{"id":"331935","messageId":"B24042DB-BB27-41DE-82B7-5F3ED502D7D0@gmail.com","threadId":"47115","inReplyTo":"xmqq60aqn1ok.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v1 2/2] log: add option to choose which refs to decorate","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2017-11-06T20:10:40Z","receivedAt":"2017-11-06T20:10:52Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"\n\nOn November 3, 2017 8:49:15 PM PDT, Junio C Hamano wrote:\n>Rafael Ascensão writes:\n>\n>Why should this be a special case that burdens users to remember one\n>more rule? Wouldn't users find \"--decorate-refs=refs/tags\" useful\n>and it woulld be shorter and nicer than having to say \"refs/tags/*\"?\n>\n\nActually, I would expect these to behave more like git describes match and exclude which don't have an extra /*. It seems natural to me that glob would always add an extra glob, but.. I don't recall if match and exlude do so.\n\nThat being said, if we think the extra glob would not cause problems and generally do what people mean... I guess consistent with --glob would be good... But it's definitely not what I'd expect at first glance.\n-- \nSent from my Android device with K-9 Mail. Please excuse my brevity.\n"},{"id":"331952","messageId":"xmqqbmkfhrf3.fsf@gitster.mtv.corp.google.com","threadId":"47115","inReplyTo":"B24042DB-BB27-41DE-82B7-5F3ED502D7D0@gmail.com","subject":"Re: [PATCH v1 2/2] log: add option to choose which refs to decorate","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-07T00:18:56Z","receivedAt":"2017-11-07T00:20:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller writes:\n\n> On November 3, 2017 8:49:15 PM PDT, Junio C Hamano wrote:\n>>Rafael Ascensão writes:\n>>\n>>Why should this be a special case that burdens users to remember one\n>>more rule? Wouldn't users find \"--decorate-refs=refs/tags\" useful\n>>and it woulld be shorter and nicer than having to say \"refs/tags/*\"?\n>\n> Actually, I would expect these to behave more like git describes\n> match and exclude which don't have an extra /*. It seems natural\n> to me that glob would always add an extra glob, but.. I don't\n> recall if match and exlude do so.\n\nI would have to say that the describe's one is wrong if it does not\nmatch what for_each_glob_ref() does for the log family of commands'\n\"--branches=\" etc. describe.c::get_name() uses positive\nand negative patterns, just like log-tree.c::add_ref_decoration()\nwould with the patch we are discussing, so perhaps the items in\nthese lists should get the same \"normalize\" treatment the patch 1/2\nof this series brings in to make things consistent?\n\n> That being said, if we think the extra glob would not cause\n> problems and generally do what people mean... I guess consistent\n> with --glob would be good... But it's definitely not what I'd\n> expect at first glance.\n\nFWIW, what describe --match/--exclude do is not what I'd have\nexpected ;-) In any case, we spotted an existing inconsistency that\nwe would want to resolve (the resolution could be \"leave it as-is\";\nI do not think we have thought this through enough yet), which is\ngood.\n\nThanks.\n\n"},{"id":"332181","messageId":"89e7f8e0-8b0d-fde0-5e28-31173213a26e@gmail.com","threadId":"47115","inReplyTo":"xmqqbmkfhrf3.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v1 2/2] log: add option to choose which refs to decorate","fromName":"Rafael Ascensão","fromEmail":"rafa.almas@gmail.com","sentAt":"2017-11-10T13:38:48Z","receivedAt":"2017-11-10T13:38:56Z","isPatch":true,"sender":{"key":"rafa.almas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1923789?v=4"},"body":"On 07/11/17 00:18, Junio C Hamano wrote:\n> Jacob Keller writes:\n> \n> I would have to say that the describe's one is wrong if it does not\n> match what for_each_glob_ref() does for the log family of commands'\n> \"--branches=\" etc. describe.c::get_name() uses positive\n> and negative patterns, just like log-tree.c::add_ref_decoration()\n> would with the patch we are discussing, so perhaps the items in\n> these lists should get the same \"normalize\" treatment the patch 1/2\n> of this series brings in to make things consistent?\n> \n\nI agree that describe should receive the \"normalize\" treatment. However,\nand following the same reasoning, why should describe users adopt the\nrules imposed by --glob? I could argue they're also used to the way it\nworks now.\n\nThat being said, the suggestion I mentioned earlier would allow to keep\nboth current behaviors consistent at the expense of the extra call to\nrefs.c::ref_exists().\n\n+if (!has_glob_specials(pattern) && !ref_exists(normalized_pattern->buf)) {\n+ /* Append implied '/' '*' if not present. */\n+ strbuf_complete(normalized_pattern, '/');\n+ /* No need to check for '*', there is none. */\n+ strbuf_addch(normalized_pattern, '*');\n+}\n\nBut I don't have enough expertise to decide if this consistency is worth \nthe extra call to refs.c::ref_exists() or if there are other side-effects\nI am not considering.\n\n>> That being said, if we think the extra glob would not cause\n>> problems and generally do what people mean... I guess consistent\n>> with --glob would be good... But it's definitely not what I'd\n>> expect at first glance.\n\nMy position is that consistency is good, but the \"first glance\nexpectation\" is definitely something important we should take into\nconsideration.\n"},{"id":"332193","messageId":"xmqqo9oaf2ss.fsf@gitster.mtv.corp.google.com","threadId":"47115","inReplyTo":"89e7f8e0-8b0d-fde0-5e28-31173213a26e@gmail.com","subject":"Re: [PATCH v1 2/2] log: add option to choose which refs to decorate","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-10T17:42:43Z","receivedAt":"2017-11-10T17:42:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rafael Ascensão writes:\n\n> I agree that describe should receive the \"normalize\" treatment. However,\n> and following the same reasoning, why should describe users adopt the\n> rules imposed by --glob? I could argue they're also used to the way it\n> works now.\n>\n> That being said, the suggestion I mentioned earlier would allow to keep\n> both current behaviors consistent at the expense of the extra call to\n> refs.c::ref_exists().\n\nIn any case, updating the \"describe\" for consistency is something we\ncan and should leave for later, to be done as a separate topic.\n\nWhile I agree with you that the consistent behaviour between\ncommands is desirable, and also I agree with you that given a\npattern $X that does not have any glob char, trying to match $X when\na ref whose name exactly is $X exists and trying to match $X/*\notherwise would give us a consistent semantics without hurting any\nexisting uses, I do not think you need to pay any extra expense of\ncalling ref_exists() at all to achieve that.\n\nThat is because when $X exists, you already know $X/otherthing does\nnot exist. And when $X does not exist, $X/otherthing might. So a\nnaive implementation would be just to add two patterns $X and $X/*\nto the filter list and be done with it. If you exactly have\nrefs/heads/master, even with the naive logic may throw both\nrefs/heads/master and refs/heads/master/* to the filter list,\nnothing will match the latter to contaminate your result (and vice\nversa).\n\nA bit more clever implementation \"just throw in two items\" would go\nlike this. It is not all that involved:\n\n - In load_ref_decorations(), before running add_ref_decoration for\n each ref and head ref, iterate over the elements in the refname\n filter list. For each element:\n\n - if item->string has a trailing '/', trim that.\n\n - store NULL in the item->util field for item whose string field\n has a glob char.\n\n - store something non-NULL (e.g. item->string) for item whose\n string field does not have a glob char.\n\n - In add_ref_decoration(), where your previous round iterates over\n filter->{include,exclude}, get rid of normalize_glob_ref() and\n use of real_pattern. Instead do something like:\n\n\tmatched = 0;\n\tif (item->util == NULL) {\n\t\tif (!wildmatch(item->string, refname, 0))\n \tmatched = 1;\n\t} else {\n\t\tconst char *rest;\n\t\tif (skip_prefix(refname, item->string, &rest) &&\n (!*rest || *rest == '/'))\n\t\t\tmatched = 1;\n\t}\n\tif (matched)\n\t\t...\n\n Of course, you would probably want to encapsulate the logic to\n set matched = 1/0 in a helper function, e.g.\n\n\tstatic int match_ref_pattern(const char *refname,\n\t\t\t\t const struct string_list_item *item) {\n\t\tint matched = 0;\n\t\t... do either wildmatch or head match with tail validation\n\t\t... depending on the item->util's NULLness (see above)\n\t\treturn matched;\n\t}\n\n and call that from the two loops for exclude and include list.\n\nHmm?\n"},{"id":"333168","messageId":"20171121213341.13939-1-rafa.almas@gmail.com","threadId":"47115","inReplyTo":"20171104004144.5975-1-rafa.almas@gmail.com","subject":"[PATCH v2] log: add option to choose which refs to decorate","fromName":"Rafael Ascensão","fromEmail":"rafa.almas@gmail.com","sentAt":"2017-11-21T21:33:41Z","receivedAt":"2017-11-21T21:37:25Z","isPatch":true,"sender":{"key":"rafa.almas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1923789?v=4"},"body":"When `log --decorate` is used, git will decorate commits with all\navailable refs. While in most cases this the desired effect, under some\nconditions it can lead to excessively verbose output.\n\nIntroduce two command line options, `--decorate-refs=` and\n`--decorate-refs-exclude=` to allow the user to select which\nrefs are used in decoration.\n\nWhen \"--decorate-refs=\" is given, only the refs that match the\npattern are used in decoration. The refs that match the pattern when\n\"--decorate-refs-exclude=\" is given, are never used in\ndecoration.\n\nThese options follow the same convention for mixing negative and\npositive patterns across the system, assuming that the inclusive default\nis to match all refs available.\n\n (1) if there is no positive pattern given, pretend as if an\n inclusive default positive pattern was given;\n\n (2) for each candidate, reject it if it matches no positive\n pattern, or if it matches any one of the negative patterns.\n\nThe rules for what is considered a match are slightly different from the\nrules used elsewhere.\n\nCommands like `log --glob` assume a trailing '/*' when glob chars are\nnot present in the pattern. This makes it difficult to specify a single\nref. On the other hand, commands like `describe --match --all` allow\nspecifying exact refs, but do not have the convenience of allowing\n\"shorthand refs\" like 'refs/heads' or 'heads' to refer to\n'refs/heads/*'.\n\nThe commands introduced in this patch consider a match if:\n\n (a) the pattern contains globs chars,\n\tand regular pattern matching returns a match.\n\n (b) the pattern does not contain glob chars,\n and ref '' exists, or if ref exists under '/'\n\nThis allows both behaviours (allowing single refs and shorthand refs)\nyet remaining compatible with existent commands.\n\nHelped-by: Kevin Daudt \nHelped-by: Junio C Hamano \nSigned-off-by: Rafael Ascensão \n---\n\nNotable changes since v1:\n\n * Do not change refs.c:for_each_glob_ref. Those changes were meant\n to address inconsistencies between commands, but they should be done\n in a separate topic.\n\n * Use new matching behaviour suggested by Junio for '--decorate-refs*'\n and change documentation/comments to reflect that.\n\n * Change help strings to clarify the commands expects a pattern\n instead of 'ref'.\n\n * Fix small inconsistencies on tests, and issues pointed by the\n feedback on the previous version.\n\n\n Documentation/git-log.txt | 7 ++++\n builtin/log.c | 10 ++++-\n log-tree.c | 24 ++++++++---\n log-tree.h | 6 ++-\n pretty.c | 4 +-\n refs.c | 65 +++++++++++++++++++++++++++++\n refs.h | 24 +++++++++++\n revision.c | 2 +-\n t/t4202-log.sh | 101 ++++++++++++++++++++++++++++++++++++++++++++++\n 9 files changed, 232 insertions(+), 11 deletions(-)\n\ndiff --git a/Documentation/git-log.txt b/Documentation/git-log.txt\nindex 32246fdb0..5437f8b0f 100644\n--- a/Documentation/git-log.txt\n+++ b/Documentation/git-log.txt\n@@ -38,6 +38,13 @@ OPTIONS\n \tare shown as if 'short' were given, otherwise no ref names are\n \tshown. The default option is 'short'.\n \n+--decorate-refs=::\n+--decorate-refs-exclude=::\n+\tIf no `--decorate-refs` is given, pretend as if all refs were\n+\tincluded. For each candidate, do not use it for decoration if it\n+\tmatches any patterns given to `--decorate-refs-exclude` or if it\n+\tdoesn't match any of the patterns given to `--decorate-refs`.\n+\n --source::\n \tPrint out the ref name given on the command line by which each\n \tcommit was reached.\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 6c1fa896a..14fdf3916 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -142,11 +142,19 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,\n \tstruct userformat_want w;\n \tint quiet = 0, source = 0, mailmap = 0;\n \tstatic struct line_opt_callback_data line_cb = {NULL, NULL, STRING_LIST_INIT_DUP};\n+\tstatic struct string_list decorate_refs_exclude = STRING_LIST_INIT_NODUP;\n+\tstatic struct string_list decorate_refs_include = STRING_LIST_INIT_NODUP;\n+\tstruct decoration_filter decoration_filter = {&decorate_refs_include,\n+\t\t\t\t\t\t &decorate_refs_exclude};\n \n \tconst struct option builtin_log_options[] = {\n \t\tOPT__QUIET(&quiet, N_(\"suppress diff output\")),\n \t\tOPT_BOOL(0, \"source\", &source, N_(\"show source\")),\n \t\tOPT_BOOL(0, \"use-mailmap\", &mailmap, N_(\"Use mail map file\")),\n+\t\tOPT_STRING_LIST(0, \"decorate-refs\", &decorate_refs_include,\n+\t\t\t\tN_(\"pattern\"), N_(\"only decorate refs that match \")),\n+\t\tOPT_STRING_LIST(0, \"decorate-refs-exclude\", &decorate_refs_exclude,\n+\t\t\t\tN_(\"pattern\"), N_(\"do not decorate refs that match \")),\n \t\t{ OPTION_CALLBACK, 0, \"decorate\", NULL, NULL, N_(\"decorate options\"),\n \t\t PARSE_OPT_OPTARG, decorate_callback},\n \t\tOPT_CALLBACK('L', NULL, &line_cb, \"n,m:file\",\n@@ -205,7 +213,7 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,\n \n \tif (decoration_style) {\n \t\trev->show_decorations = 1;\n-\t\tload_ref_decorations(decoration_style);\n+\t\tload_ref_decorations(&decoration_filter, decoration_style);\n \t}\n \n \tif (rev->line_level_traverse)\ndiff --git a/log-tree.c b/log-tree.c\nindex 3b904f037..fca29d479 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -94,8 +94,12 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid,\n {\n \tstruct object *obj;\n \tenum decoration_type type = DECORATION_NONE;\n+\tstruct decoration_filter *filter = (struct decoration_filter *)cb_data;\n \n-\tassert(cb_data == NULL);\n+\tif (filter && !ref_filter_match(refname,\n+\t\t\t filter->include_ref_pattern,\n+\t\t\t filter->exclude_ref_pattern))\n+\t\treturn 0;\n \n \tif (starts_with(refname, git_replace_ref_base)) {\n \t\tstruct object_id original_oid;\n@@ -148,15 +152,23 @@ static int add_graft_decoration(const struct commit_graft *graft, void *cb_data)\n \treturn 0;\n }\n \n-void load_ref_decorations(int flags)\n+void load_ref_decorations(struct decoration_filter *filter, int flags)\n {\n \tif (!decoration_loaded) {\n-\n+\t\tif (filter) {\n+\t\t\tstruct string_list_item *item;\n+\t\t\tfor_each_string_list_item(item, filter->exclude_ref_pattern) {\n+\t\t\t\tnormalize_glob_ref(item, NULL, item->string);\n+\t\t\t}\n+\t\t\tfor_each_string_list_item(item, filter->include_ref_pattern) {\n+\t\t\t\tnormalize_glob_ref(item, NULL, item->string);\n+\t\t\t}\n+\t\t}\n \t\tdecoration_loaded = 1;\n \t\tdecoration_flags = flags;\n-\t\tfor_each_ref(add_ref_decoration, NULL);\n-\t\thead_ref(add_ref_decoration, NULL);\n-\t\tfor_each_commit_graft(add_graft_decoration, NULL);\n+\t\tfor_each_ref(add_ref_decoration, filter);\n+\t\thead_ref(add_ref_decoration, filter);\n+\t\tfor_each_commit_graft(add_graft_decoration, filter);\n \t}\n }\n \ndiff --git a/log-tree.h b/log-tree.h\nindex 48f11fb74..deba03518 100644\n--- a/log-tree.h\n+++ b/log-tree.h\n@@ -7,6 +7,10 @@ struct log_info {\n \tstruct commit *commit, *parent;\n };\n \n+struct decoration_filter {\n+\tstruct string_list *include_ref_pattern, *exclude_ref_pattern;\n+};\n+\n int parse_decorate_color_config(const char *var, const char *slot_name, const char *value);\n void init_log_tree_opt(struct rev_info *);\n int log_tree_diff_flush(struct rev_info *);\n@@ -24,7 +28,7 @@ void show_decorations(struct rev_info *opt, struct commit *commit);\n void log_write_email_headers(struct rev_info *opt, struct commit *commit,\n \t\t\t const char **extra_headers_p,\n \t\t\t int *need_8bit_cte_p);\n-void load_ref_decorations(int flags);\n+void load_ref_decorations(struct decoration_filter *filter, int flags);\n \n #define FORMAT_PATCH_NAME_MAX 64\n void fmt_output_commit(struct strbuf *, struct commit *, struct rev_info *);\ndiff --git a/pretty.c b/pretty.c\nindex 2f6b0ae6c..f7ce49023 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1186,11 +1186,11 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\tstrbuf_addstr(sb, get_revision_mark(NULL, commit));\n \t\treturn 1;\n \tcase 'd':\n-\t\tload_ref_decorations(DECORATE_SHORT_REFS);\n+\t\tload_ref_decorations(NULL, DECORATE_SHORT_REFS);\n \t\tformat_decorations(sb, commit, c->auto_color);\n \t\treturn 1;\n \tcase 'D':\n-\t\tload_ref_decorations(DECORATE_SHORT_REFS);\n+\t\tload_ref_decorations(NULL, DECORATE_SHORT_REFS);\n \t\tformat_decorations_extended(sb, commit, c->auto_color, \"\", \", \", \"\");\n \t\treturn 1;\n \tcase 'g':\t\t/* reflog info */\ndiff --git a/refs.c b/refs.c\nindex 339d4318e..20ba82b43 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -242,6 +242,50 @@ int ref_exists(const char *refname)\n \treturn !!resolve_ref_unsafe(refname, RESOLVE_REF_READING, NULL, NULL);\n }\n \n+static int match_ref_pattern(const char *refname,\n+\t\t\t const struct string_list_item *item)\n+{\n+\tint matched = 0;\n+\tif (item->util == NULL) {\n+\t\tif (!wildmatch(item->string, refname, 0))\n+\t\t\tmatched = 1;\n+\t} else {\n+\t\tconst char *rest;\n+\t\tif (skip_prefix(refname, item->string, &rest) &&\n+\t\t (!*rest || *rest == '/'))\n+\t\t\tmatched = 1;\n+\t}\n+\treturn matched;\n+}\n+\n+int ref_filter_match(const char *refname,\n+\t\t const struct string_list *include_patterns,\n+\t\t const struct string_list *exclude_patterns)\n+{\n+\tstruct string_list_item *item;\n+\n+\tif (exclude_patterns && exclude_patterns->nr) {\n+\t\tfor_each_string_list_item(item, exclude_patterns) {\n+\t\t\tif (match_ref_pattern(refname, item))\n+\t\t\t\treturn 0;\n+\t\t}\n+\t}\n+\n+\tif (include_patterns && include_patterns->nr) {\n+\t\tint found = 0;\n+\t\tfor_each_string_list_item(item, include_patterns) {\n+\t\t\tif (match_ref_pattern(refname, item)) {\n+\t\t\t\tfound = 1;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\n+\t\tif (!found)\n+\t\t\treturn 0;\n+\t}\n+\treturn 1;\n+}\n+\n static int filter_refs(const char *refname, const struct object_id *oid,\n \t\t\t int flags, void *data)\n {\n@@ -369,6 +413,27 @@ int head_ref_namespaced(each_ref_fn fn, void *cb_data)\n \treturn ret;\n }\n \n+void normalize_glob_ref(struct string_list_item *item, const char *prefix,\n+\t\t\tconst char *pattern)\n+{\n+\tstruct strbuf normalized_pattern = STRBUF_INIT;\n+\n+\tif (*pattern == '/')\n+\t\tBUG(\"pattern must not start with '/'\");\n+\n+\tif (prefix) {\n+\t\tstrbuf_addstr(&normalized_pattern, prefix);\n+\t}\n+\telse if (!starts_with(pattern, \"refs/\"))\n+\t\tstrbuf_addstr(&normalized_pattern, \"refs/\");\n+\tstrbuf_addstr(&normalized_pattern, pattern);\n+\tstrbuf_strip_suffix(&normalized_pattern, \"/\");\n+\n+\titem->string = strbuf_detach(&normalized_pattern, NULL);\n+\titem->util = has_glob_specials(pattern) ? NULL : item->string;\n+\tstrbuf_release(&normalized_pattern);\n+}\n+\n int for_each_glob_ref_in(each_ref_fn fn, const char *pattern,\n \tconst char *prefix, void *cb_data)\n {\ndiff --git a/refs.h b/refs.h\nindex 18582a408..01be5ae32 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -312,6 +312,30 @@ int for_each_namespaced_ref(each_ref_fn fn, void *cb_data);\n int refs_for_each_rawref(struct ref_store *refs, each_ref_fn fn, void *cb_data);\n int for_each_rawref(each_ref_fn fn, void *cb_data);\n \n+/*\n+ * Normalizes partial refs to their fully qualified form.\n+ * Will prepend to the if it doesn't start with 'refs/'.\n+ * will default to 'refs/' if NULL.\n+ *\n+ * item.string will be set to the result.\n+ * item.util will be set to NULL if contains glob characters, or\n+ * non-NULL if it doesn't.\n+ */\n+void normalize_glob_ref(struct string_list_item *item, const char *prefix,\n+\t\t\tconst char *pattern);\n+\n+/*\n+ * Returns 0 if refname matches any of the exclude_patterns, or if it doesn't\n+ * match any of the include_patterns. Returns 1 otherwise.\n+ *\n+ * If pattern list is NULL or empty, matching against that list is skipped.\n+ * This has the effect of matching everything by default, unless the user\n+ * specifies rules otherwise.\n+ */\n+int ref_filter_match(const char *refname,\n+\t\t const struct string_list *include_patterns,\n+\t\t const struct string_list *exclude_patterns);\n+\n static inline const char *has_glob_specials(const char *pattern)\n {\n \treturn strpbrk(pattern, \"?*[\");\ndiff --git a/revision.c b/revision.c\nindex e2e691dd5..f6a3da5cd 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1832,7 +1832,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->simplify_by_decoration = 1;\n \t\trevs->limited = 1;\n \t\trevs->prune = 1;\n-\t\tload_ref_decorations(DECORATE_SHORT_REFS);\n+\t\tload_ref_decorations(NULL, DECORATE_SHORT_REFS);\n \t} else if (!strcmp(arg, \"--date-order\")) {\n \t\trevs->sort_order = REV_SORT_BY_COMMIT_DATE;\n \t\trevs->topo_order = 1;\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex 8f155da7a..25b1f8cc7 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -737,6 +737,107 @@ test_expect_success 'log.decorate configuration' '\n \n '\n \n+test_expect_success 'decorate-refs with glob' '\n+\tcat >expect.decorate <<-\\EOF &&\n+\tMerge-tag-reach\n+\tMerge-tags-octopus-a-and-octopus-b\n+\tseventh\n+\toctopus-b (octopus-b)\n+\toctopus-a (octopus-a)\n+\treach\n+\tEOF\n+\tgit log -n6 --decorate=short --pretty=\"tformat:%f%d\" \\\n+\t\t--decorate-refs=\"heads/octopus*\" >actual &&\n+\ttest_cmp expect.decorate actual\n+'\n+\n+test_expect_success 'decorate-refs without globs' '\n+\tcat >expect.decorate <<-\\EOF &&\n+\tMerge-tag-reach\n+\tMerge-tags-octopus-a-and-octopus-b\n+\tseventh\n+\toctopus-b\n+\toctopus-a\n+\treach (tag: reach)\n+\tEOF\n+\tgit log -n6 --decorate=short --pretty=\"tformat:%f%d\" \\\n+\t\t--decorate-refs=\"tags/reach\" >actual &&\n+\ttest_cmp expect.decorate actual\n+'\n+\n+test_expect_success 'multiple decorate-refs' '\n+\tcat >expect.decorate <<-\\EOF &&\n+\tMerge-tag-reach\n+\tMerge-tags-octopus-a-and-octopus-b\n+\tseventh\n+\toctopus-b (octopus-b)\n+\toctopus-a (octopus-a)\n+\treach (tag: reach)\n+\tEOF\n+\tgit log -n6 --decorate=short --pretty=\"tformat:%f%d\" \\\n+\t\t--decorate-refs=\"heads/octopus*\" \\\n+\t\t--decorate-refs=\"tags/reach\" >actual &&\n+ test_cmp expect.decorate actual\n+'\n+\n+test_expect_success 'decorate-refs-exclude with glob' '\n+\tcat >expect.decorate <<-\\EOF &&\n+\tMerge-tag-reach (HEAD -> master)\n+\tMerge-tags-octopus-a-and-octopus-b\n+\tseventh (tag: seventh)\n+\toctopus-b (tag: octopus-b)\n+\toctopus-a (tag: octopus-a)\n+\treach (tag: reach, reach)\n+\tEOF\n+\tgit log -n6 --decorate=short --pretty=\"tformat:%f%d\" \\\n+\t\t--decorate-refs-exclude=\"heads/octopus*\" >actual &&\n+\ttest_cmp expect.decorate actual\n+'\n+\n+test_expect_success 'decorate-refs-exclude without globs' '\n+\tcat >expect.decorate <<-\\EOF &&\n+\tMerge-tag-reach (HEAD -> master)\n+\tMerge-tags-octopus-a-and-octopus-b\n+\tseventh (tag: seventh)\n+\toctopus-b (tag: octopus-b, octopus-b)\n+\toctopus-a (tag: octopus-a, octopus-a)\n+\treach (reach)\n+\tEOF\n+\tgit log -n6 --decorate=short --pretty=\"tformat:%f%d\" \\\n+\t\t--decorate-refs-exclude=\"tags/reach\" >actual &&\n+\ttest_cmp expect.decorate actual\n+'\n+\n+test_expect_success 'multiple decorate-refs-exclude' '\n+\tcat >expect.decorate <<-\\EOF &&\n+\tMerge-tag-reach (HEAD -> master)\n+\tMerge-tags-octopus-a-and-octopus-b\n+\tseventh (tag: seventh)\n+\toctopus-b (tag: octopus-b)\n+\toctopus-a (tag: octopus-a)\n+\treach (reach)\n+\tEOF\n+\tgit log -n6 --decorate=short --pretty=\"tformat:%f%d\" \\\n+\t\t--decorate-refs-exclude=\"heads/octopus*\" \\\n+\t\t--decorate-refs-exclude=\"tags/reach\" >actual &&\n+\ttest_cmp expect.decorate actual\n+'\n+\n+test_expect_success 'decorate-refs and decorate-refs-exclude' '\n+\tcat >expect.decorate <<-\\EOF &&\n+\tMerge-tag-reach (master)\n+\tMerge-tags-octopus-a-and-octopus-b\n+\tseventh\n+\toctopus-b\n+\toctopus-a\n+\treach (reach)\n+\tEOF\n+\tgit log -n6 --decorate=short --pretty=\"tformat:%f%d\" \\\n+\t\t--decorate-refs=\"heads/*\" \\\n+\t\t--decorate-refs-exclude=\"heads/oc*\" >actual &&\n+\ttest_cmp expect.decorate actual\n+'\n+\n test_expect_success 'log.decorate config parsing' '\n \tgit log --oneline --decorate=full >expect.full &&\n \tgit log --oneline --decorate=short >expect.short &&\n-- \n2.15.0\n\n"},{"id":"333236","messageId":"xmqqpo8buerz.fsf@gitster.mtv.corp.google.com","threadId":"47115","inReplyTo":"20171121213341.13939-1-rafa.almas@gmail.com","subject":"Re: [PATCH v2] log: add option to choose which refs to decorate","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-22T04:18:56Z","receivedAt":"2017-11-22T04:19:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rafael Ascensão writes:\n\n> When `log --decorate` is used, git will decorate commits with all\n> available refs. While in most cases this the desired effect, under some\n\nMissing verb. s/this the/this may give the/; perhaps.\n\n> conditions it can lead to excessively verbose output.\n\nOther than that, I didn't find anything questionable in the\nimplementation, tests or doc updates. Nicely done.\n\nWill queue.\n\nThanks.\n"}]}