{"thread":{"id":"55694","subject":"[PATCH] [GSOC][RFC] ref-filter: add %(notes) atom and --notes","startedAt":"2021-05-15T09:04:15Z","lastAt":"2021-05-15T09:04:15Z","messageCount":1,"participants":["ZheNing Hu via GitGitGadget"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"424632","messageId":"pull.952.git.1621069448064.gitgitgadget@gmail.com","threadId":"55694","inReplyTo":null,"subject":"[PATCH] [GSOC][RFC] ref-filter: add %(notes) atom and --notes","fromName":"ZheNing Hu via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-05-15T09:04:07Z","receivedAt":"2021-05-15T09:04:15Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"From: ZheNing Hu <adlternative@gmail.com>\n\nNote that `--pretty=\"%N\"` can view notes related\nto the commit. So add `%(notes)` to ref-filter\nseem is a good choice. New atom `%(notes)` can show\nthe notes associated with the object pointed at by\nthe ref. At the same time we can use `%(*notes)`\nto show the notes of the object which dereferenced\nby the ref.\n\nAdd `--notes=<ref>` option and `--no-notes` option\nto `git for-each-ref`. Multiple `--notes=<ref>`\noptions can be combined to control which notes are\nbeing displayed. `--no-notes` will reset the list of\nnotes refs from which notes are shown.\n\nBy default, `git for-each-ref --format=\"%(notes)\"`\nwill output notes of \"refs/notes/commits\", and\nin the case of using `--notes=foo`, will suppress\nthe output notes of `refs/notes/commits`, unless\nuse `--notes=refs/notes/commits` at the same time,\n\"refs/notes/commit\"'s notes will output. Therefore,\nthe semantics of `--notes=<ref>` here are different\nfrom that of `git log --notes=<ref>`.\n\nSigned-off-by: ZheNing Hu <adlternative@gmail.com>\n---\n    [GSOC][RFC] ref-filter: add %(notes) atom and --notes\n    \n    In https://lore.kernel.org/git/xmqqsg34a5j8.fsf@gitster.g/\n    \n    Junio think that %(notes) atom can be used with --notes. René Scharfe\n    think %(notes) can make use of load_display_notes() and\n    format_display_notes() to reduce overhead.\n    \n    So in this new version of this patch, I may have implemented this\n    feature in some troublesome ways.\n    \n    First, I parsed --notes and --no-notes in for-each-ref.c. use new struct\n    notes_info as a carrier for parsing --notes, The parsed notes_info is\n    passed to ref-filter.c through a global variable \"struct notes_info\n    notes_info\".\n    \n    Then in ref-filter.c I use notes_atom_parser() to parse %(notes), the\n    parsing of all notes atoms only calls load_display_notes() once.\n    \n    Finally, use grab_notes() to get the notes corresponding to the object.\n    \n    However, the current implementation is still not elegant enough,\n    \n    First, can we directly learn the processing of --notes and --no-notes in\n    revision.c? It is very cumbersome to make new --notes and --no-notes in\n    for-each-ref.c. Since we need to output \"refs/notes/commits\" notes by\n    default when using --format=\"%(notes)\", should we reject the default\n    notes output of \"refs/notes/commits\" when using --notes=<ref> as it has\n    been done now? And here --notes=<ref> does not support --notes (no ref),\n    is there any good way to implement it?\n    \n    Second, I can not easily make struct notes_info notes_info a member of\n    struct ref_format format, because as a parameter of *_atom_parser in\n    ref-flter.c, const struct ref_format *format is used to prevent us from\n    modifying it. However, struct notes_info notes_info is a variable that\n    needs to be modified. So here I have to use a global variable to pass it\n    (very urgly).\n    \n    Third, I am not sure whether the \"raw\" parameter of\n    format_display_notes() should be passed 0 or 1.\n    \n    So, I feel very confused, any good suggestions?\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-952%2Fadlternative%2Fref-filter-notes-atom-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-952/adlternative/ref-filter-notes-atom-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/952\n\n Documentation/git-for-each-ref.txt | 14 +++++++\n builtin/for-each-ref.c             | 26 ++++++++++++\n ref-filter.c                       | 35 +++++++++++++++-\n ref-filter.h                       |  7 ++++\n t/t6300-for-each-ref.sh            | 64 ++++++++++++++++++++++++++++++\n 5 files changed, 144 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\nindex 2ae2478de706..1157e8dda63d 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -13,6 +13,7 @@ SYNOPSIS\n \t\t   [--points-at=<object>]\n \t\t   [--merged[=<object>]] [--no-merged[=<object>]]\n \t\t   [--contains[=<object>]] [--no-contains[=<object>]]\n+\t\t   [--no-notes | --notes=<ref>]\n \n DESCRIPTION\n -----------\n@@ -57,6 +58,15 @@ OPTIONS\n \t`xx`; for example `%00` interpolates to `\\0` (NUL),\n \t`%09` to `\\t` (TAB) and `%0a` to `\\n` (LF).\n \n+--notes=<ref>::\n+\tShow the notes that annotate the object. With an optional '<ref>' argument,\n+\tuse the ref to find the notes to display.\n+\t(see linkgit:git-notes[1] and linkgit:pretty-options.txt[])\n+--no-notes::\n+\tDo not show notes. This negates the above `--notes` option, by\n+\tresetting the list of notes refs from which notes are shown.\n+\t(see linkgit:git-notes[1] and linkgit:pretty-options.txt[])\n+\n --color[=<when>]::\n \tRespect any colors specified in the `--format` option. The\n \t`<when>` field must be one of `always`, `never`, or `auto` (if\n@@ -139,6 +149,9 @@ deltabase::\n \tgiven object, if it is stored as a delta.  Otherwise it\n \texpands to the null object name (all zeroes).\n \n+notes::\n+\tThe notes associated with the object pointed at by the ref.\n+\n upstream::\n \tThe name of a local ref which can be considered ``upstream''\n \tfrom the displayed ref. Respects `:short`, `:lstrip` and\n@@ -302,6 +315,7 @@ git for-each-ref --count=3 --sort='-*authordate' \\\n Subject: %(*subject)\n Date: %(*authordate)\n Ref: %(*refname)\n+Notes: %(*notes)\n \n %(*body)\n ' 'refs/tags'\ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex 89cb6307d46f..12e6e673d48e 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -14,6 +14,26 @@ static char const * const for_each_ref_usage[] = {\n \tNULL\n };\n \n+extern struct notes_info notes_info;\n+\n+int parse_opt_notes(const struct option *opt, const char *arg, int unset)\n+{\n+\tstruct notes_info *ni = opt->value;\n+\n+\tif (unset) {\n+\t\tdisable_display_notes(&ni->notes_option, &ni->show_notes);\n+\t\treturn 0;\n+\t}\n+\n+\tif (!arg)\n+\t\treturn -1;\n+\n+\tenable_ref_display_notes(&ni->notes_option, &ni->show_notes, arg);\n+\tni->notes_option.use_default_notes = 0;\n+\n+\treturn 0;\n+}\n+\n int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n {\n \tint i;\n@@ -38,6 +58,8 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n \t\tOPT_GROUP(\"\"),\n \t\tOPT_INTEGER( 0 , \"count\", &maxcount, N_(\"show only <n> matched refs\")),\n \t\tOPT_STRING(  0 , \"format\", &format.format, N_(\"format\"), N_(\"format to use for the output\")),\n+\t\tOPT_CALLBACK(0, \"notes\", &notes_info, N_(\"notes\"), N_(\"the notes associated\"\n+\t\t\t     \"with the object pointed at by the ref\"), parse_opt_notes),\n \t\tOPT__COLOR(&format.use_color, N_(\"respect format colors\")),\n \t\tOPT_REF_SORT(sorting_tail),\n \t\tOPT_CALLBACK(0, \"points-at\", &filter.points_at,\n@@ -51,6 +73,9 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n \t\tOPT_END(),\n \t};\n \n+\tinit_display_notes(&notes_info.notes_option);\n+\tenable_default_display_notes(&notes_info.notes_option, &notes_info.show_notes);\n+\n \tmemset(&array, 0, sizeof(array));\n \tmemset(&filter, 0, sizeof(filter));\n \n@@ -97,5 +122,6 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n \tfree_commit_list(filter.with_commit);\n \tfree_commit_list(filter.no_commit);\n \tUNLEAK(sorting);\n+\tstring_list_clear(&notes_info.notes_option.extra_notes_refs, 0);\n \treturn 0;\n }\ndiff --git a/ref-filter.c b/ref-filter.c\nindex e2eac50d9508..ffcc1e500ed3 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -84,6 +84,8 @@ static struct expand_data {\n \tstruct object_info info;\n } oi, oi_deref;\n \n+struct notes_info notes_info;\n+\n struct ref_to_worktree_entry {\n \tstruct hashmap_entry ent;\n \tstruct worktree *wt; /* key is wt->head_ref */\n@@ -295,6 +297,16 @@ static int deltabase_atom_parser(const struct ref_format *format, struct used_at\n \treturn 0;\n }\n \n+static int notes_atom_parser(const struct ref_format *format, struct used_atom *atom,\n+\t\t\t\t const char *arg, struct strbuf *err)\n+{\n+\tif(!notes_info.show_notes_given && notes_info.show_notes) {\n+\t\t\tload_display_notes(&notes_info.notes_option);\n+\t\t\tnotes_info.show_notes_given = 1;\n+\t}\n+\treturn 0;\n+}\n+\n static int body_atom_parser(const struct ref_format *format, struct used_atom *atom,\n \t\t\t    const char *arg, struct strbuf *err)\n {\n@@ -506,6 +518,7 @@ static struct {\n \t{ \"objectsize\", SOURCE_OTHER, FIELD_ULONG, objectsize_atom_parser },\n \t{ \"objectname\", SOURCE_OTHER, FIELD_STR, oid_atom_parser },\n \t{ \"deltabase\", SOURCE_OTHER, FIELD_STR, deltabase_atom_parser },\n+\t{ \"notes\", SOURCE_OTHER, FIELD_STR, notes_atom_parser },\n \t{ \"tree\", SOURCE_OBJ, FIELD_STR, oid_atom_parser },\n \t{ \"parent\", SOURCE_OBJ, FIELD_STR, oid_atom_parser },\n \t{ \"numparent\", SOURCE_OBJ, FIELD_ULONG },\n@@ -953,6 +966,17 @@ static int grab_oid(const char *name, const char *field, const struct object_id\n \treturn 0;\n }\n \n+static void grab_notes(const struct object_id *oid, struct atom_value *v)\n+{\n+\tstruct strbuf notebuf = STRBUF_INIT;\n+\n+\tif (notes_info.show_notes)\n+\t\t\tformat_display_notes(oid, &notebuf,\n+\t\t\t     get_log_output_encoding(), 1);\n+\tstrbuf_trim_trailing_newline(&notebuf);\n+\tv->s = strbuf_detach(&notebuf, NULL);\n+}\n+\n /* See grab_values */\n static void grab_common_values(struct atom_value *val, int deref, struct expand_data *oi)\n {\n@@ -975,8 +999,12 @@ static void grab_common_values(struct atom_value *val, int deref, struct expand_\n \t\t\tv->s = xstrfmt(\"%\"PRIuMAX , (uintmax_t)oi->size);\n \t\t} else if (!strcmp(name, \"deltabase\"))\n \t\t\tv->s = xstrdup(oid_to_hex(&oi->delta_base_oid));\n-\t\telse if (deref)\n-\t\t\tgrab_oid(name, \"objectname\", &oi->oid, v, &used_atom[i]);\n+\t\telse if (deref) {\n+\t\t\tif (!strcmp(name, \"notes\"))\n+\t\t\t\tgrab_notes(&oi->oid, v);\n+\t\t\telse\n+\t\t\t\tgrab_oid(name, \"objectname\", &oi->oid, v, &used_atom[i]);\n+\t\t}\n \t}\n }\n \n@@ -1767,6 +1795,9 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)\n \t\t\tcontinue;\n \t\t} else if (!deref && grab_oid(name, \"objectname\", &ref->objectname, v, atom)) {\n \t\t\tcontinue;\n+\t\t} else if (!deref && !strcmp(name, \"notes\")) {\n+\t\t\tgrab_notes(&ref->objectname, v);\n+\t\t\tcontinue;\n \t\t} else if (!strcmp(name, \"HEAD\")) {\n \t\t\tif (atom->u.head && !strcmp(ref->refname, atom->u.head))\n \t\t\t\tv->s = xstrdup(\"*\");\ndiff --git a/ref-filter.h b/ref-filter.h\nindex baf72a718965..b2f97bbd164e 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -5,6 +5,7 @@\n #include \"refs.h\"\n #include \"commit.h\"\n #include \"parse-options.h\"\n+#include \"notes.h\"\n \n /* Quoting styles */\n #define QUOTE_NONE 0\n@@ -85,6 +86,12 @@ struct ref_format {\n \n #define REF_FORMAT_INIT { NULL, 0, -1 }\n \n+struct notes_info {\n+\tint show_notes;\n+\tint show_notes_given;\n+\tstruct display_notes_opt notes_option;\n+};\n+\n /*  Macros for checking --merged and --no-merged options */\n #define _OPT_MERGED_NO_MERGED(option, filter, h) \\\n \t{ OPTION_CALLBACK, 0, option, (filter), N_(\"commit\"), (h), \\\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex 9e0214076b4d..a92636236dfb 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -32,8 +32,12 @@ test_expect_success setup '\n \tgit add one &&\n \tgit commit -m \"Initial\" &&\n \tgit branch -M main &&\n+\tgit notes add -m \"commit-notes\" HEAD &&\n+\tgit notes --ref=refs/notes/sky-walker add \\\n+\t    -m \"sky-notes\" HEAD &&\n \tsetdate_and_increment &&\n \tgit tag -a -m \"Tagging at $datestamp\" testtag &&\n+\tgit notes add -m \"tag-notes\" testtag &&\n \tgit update-ref refs/remotes/origin/main main &&\n \tgit remote add origin nowhere &&\n \tgit config branch.main.remote origin &&\n@@ -162,6 +166,7 @@ test_atom head contents:signature ''\n test_atom head contents 'Initial\n '\n test_atom head HEAD '*'\n+test_atom head notes $(git notes show refs/heads/main)\n \n test_atom tag refname refs/tags/testtag\n test_atom tag refname:short testtag\n@@ -220,6 +225,8 @@ test_atom tag contents:signature ''\n test_atom tag contents 'Tagging at 1151968727\n '\n test_atom tag HEAD ' '\n+test_atom tag notes $(git notes show refs/tags/testtag)\n+test_atom tag \"*notes\" $(git notes show refs/heads/main)\n \n test_expect_success 'Check invalid atoms names are errors' '\n \ttest_must_fail git for-each-ref --format=\"%(INVALID)\" refs/heads\n@@ -380,6 +387,8 @@ test_expect_success 'exercise strftime with odd fields' '\n \n cat >expected <<\\EOF\n refs/heads/main\n+refs/notes/commits\n+refs/notes/sky-walker\n refs/remotes/origin/main\n refs/tags/testtag\n EOF\n@@ -393,6 +402,8 @@ test_expect_success 'Verify ascending sort' '\n cat >expected <<\\EOF\n refs/tags/testtag\n refs/remotes/origin/main\n+refs/notes/sky-walker\n+refs/notes/commits\n refs/heads/main\n EOF\n \n@@ -429,6 +440,8 @@ test_expect_success 'exercise glob patterns with prefixes' '\n \n cat >expected <<\\EOF\n 'refs/heads/main'\n+'refs/notes/commits'\n+'refs/notes/sky-walker'\n 'refs/remotes/origin/main'\n 'refs/tags/testtag'\n EOF\n@@ -450,6 +463,8 @@ test_expect_success 'Quoting style: python' '\n \n cat >expected <<\\EOF\n \"refs/heads/main\"\n+\"refs/notes/commits\"\n+\"refs/notes/sky-walker\"\n \"refs/remotes/origin/main\"\n \"refs/tags/testtag\"\n EOF\n@@ -509,6 +524,8 @@ test_expect_success 'Check for invalid refname format' '\n test_expect_success 'set up color tests' '\n \tcat >expected.color <<-EOF &&\n \t$(git rev-parse --short refs/heads/main) <GREEN>main<RESET>\n+\t$(git rev-parse --short refs/notes/commits) <GREEN>notes/commits<RESET>\n+\t$(git rev-parse --short refs/notes/sky-walker) <GREEN>notes/sky-walker<RESET>\n \t$(git rev-parse --short refs/remotes/myfork/main) <GREEN>myfork/main<RESET>\n \t$(git rev-parse --short refs/remotes/origin/main) <GREEN>origin/main<RESET>\n \t$(git rev-parse --short refs/tags/testtag) <GREEN>testtag<RESET>\n@@ -1007,6 +1024,53 @@ test_expect_success 'Add symbolic ref for the following tests' '\n \tgit symbolic-ref refs/heads/sym refs/heads/main\n '\n \n+test_expect_success 'for-each-ref --notes and --no-notes' '\n+\tcat >expected <<-\\EOF &&\n+\tsky-notes\n+\tEOF\n+\tgit for-each-ref --format=\"%(notes)\" --notes=refs/notes/sky-walker \\\n+\t    refs/heads/ambiguous >actual &&\n+\ttest_cmp expected actual &&\n+\tgit for-each-ref --format=\"%(notes)\" --notes=sky-walker refs/heads/ambiguous >actual &&\n+\ttest_cmp expected actual &&\n+\tgit for-each-ref --format=\"%(notes)\" --notes=sky-walker --notes=commits \\\n+\t    --no-notes --notes=sky-walker refs/heads/ambiguous >actual &&\n+\ttest_cmp expected actual &&\n+\tcat >expected <<-\\EOF &&\n+\tcommit-notes\n+\tEOF\n+\tgit for-each-ref --format=\"%(notes)\" --notes=commits refs/heads/ambiguous >actual &&\n+\tcat >expected <<-\\EOF &&\n+\tsky-notes\n+\tcommit-notes\n+\tEOF\n+\tgit for-each-ref --format=\"%(notes)\" --notes=sky-walker --notes=commits \\\n+\t    refs/heads/ambiguous >actual &&\n+\ttest_cmp expected actual &&\n+\tcat >expected <<-\\EOF &&\n+\tcommit-notes\n+\tsky-notes\n+\tEOF\n+\tgit for-each-ref --format=\"%(notes)\" --notes=commits --notes=sky-walker \\\n+\t    refs/heads/ambiguous >actual &&\n+\ttest_cmp expected actual &&\n+\tcat >expected <<-\\EOF &&\n+\n+\tEOF\n+\tgit for-each-ref --format=\"%(notes)\" --notes=sky-walker --notes=commits --no-notes \\\n+\t    refs/heads/ambiguous >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'for-each-ref --notes with remote ref' '\n+\tcat >expected <<-\\EOF &&\n+\trefs/remotes/origin/main sky-notes\n+\tEOF\n+\tgit for-each-ref --format=\"%(refname) %(notes)\" --notes=refs/notes/sky-walker \\\n+\t    refs/remotes/origin/main >actual &&\n+\ttest_cmp expected actual\n+'\n+\n cat >expected <<EOF\n refs/heads/main\n EOF\n\nbase-commit: 97eea85a0a1ec66d356567808a1e4ca2367e0ce7\n-- \ngitgitgadget\n"}]}