diff mbox series

[GSOC,RFC] ref-filter: add %(notes) atom and --notes

Message ID pull.952.git.1621069448064.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series [GSOC,RFC] ref-filter: add %(notes) atom and --notes | expand

Commit Message

ZheNing Hu May 15, 2021, 9:04 a.m. UTC 
From: ZheNing Hu <adlternative@gmail.com>

Note that `--pretty="%N"` can view notes related
to the commit. So add `%(notes)` to ref-filter
seem is a good choice. New atom `%(notes)` can show
the notes associated with the object pointed at by
the ref. At the same time we can use `%(*notes)`
to show the notes of the object which dereferenced
by the ref.

Add `--notes=<ref>` option and `--no-notes` option
to `git for-each-ref`. Multiple `--notes=<ref>`
options can be combined to control which notes are
being displayed. `--no-notes` will reset the list of
notes refs from which notes are shown.

By default, `git for-each-ref --format="%(notes)"`
will output notes of "refs/notes/commits", and
in the case of using `--notes=foo`, will suppress
the output notes of `refs/notes/commits`, unless
use `--notes=refs/notes/commits` at the same time,
"refs/notes/commit"'s notes will output. Therefore,
the semantics of `--notes=<ref>` here are different
from that of `git log --notes=<ref>`.

Signed-off-by: ZheNing Hu <adlternative@gmail.com>
---
    [GSOC][RFC] ref-filter: add %(notes) atom and --notes
    
    In https://lore.kernel.org/git/xmqqsg34a5j8.fsf@gitster.g/
    
    Junio think that %(notes) atom can be used with --notes. René Scharfe
    think %(notes) can make use of load_display_notes() and
    format_display_notes() to reduce overhead.
    
    So in this new version of this patch, I may have implemented this
    feature in some troublesome ways.
    
    First, I parsed --notes and --no-notes in for-each-ref.c. use new struct
    notes_info as a carrier for parsing --notes, The parsed notes_info is
    passed to ref-filter.c through a global variable "struct notes_info
    notes_info".
    
    Then in ref-filter.c I use notes_atom_parser() to parse %(notes), the
    parsing of all notes atoms only calls load_display_notes() once.
    
    Finally, use grab_notes() to get the notes corresponding to the object.
    
    However, the current implementation is still not elegant enough,
    
    First, can we directly learn the processing of --notes and --no-notes in
    revision.c? It is very cumbersome to make new --notes and --no-notes in
    for-each-ref.c. Since we need to output "refs/notes/commits" notes by
    default when using --format="%(notes)", should we reject the default
    notes output of "refs/notes/commits" when using --notes=<ref> as it has
    been done now? And here --notes=<ref> does not support --notes (no ref),
    is there any good way to implement it?
    
    Second, I can not easily make struct notes_info notes_info a member of
    struct ref_format format, because as a parameter of *_atom_parser in
    ref-flter.c, const struct ref_format *format is used to prevent us from
    modifying it. However, struct notes_info notes_info is a variable that
    needs to be modified. So here I have to use a global variable to pass it
    (very urgly).
    
    Third, I am not sure whether the "raw" parameter of
    format_display_notes() should be passed 0 or 1.
    
    So, I feel very confused, any good suggestions?

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-952%2Fadlternative%2Fref-filter-notes-atom-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-952/adlternative/ref-filter-notes-atom-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/952

 Documentation/git-for-each-ref.txt | 14 +++++++
 builtin/for-each-ref.c             | 26 ++++++++++++
 ref-filter.c                       | 35 +++++++++++++++-
 ref-filter.h                       |  7 ++++
 t/t6300-for-each-ref.sh            | 64 ++++++++++++++++++++++++++++++
 5 files changed, 144 insertions(+), 2 deletions(-)


base-commit: 97eea85a0a1ec66d356567808a1e4ca2367e0ce7
diff mbox series

Patch

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 2ae2478de706..1157e8dda63d 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -13,6 +13,7 @@  SYNOPSIS
 		   [--points-at=<object>]
 		   [--merged[=<object>]] [--no-merged[=<object>]]
 		   [--contains[=<object>]] [--no-contains[=<object>]]
+		   [--no-notes | --notes=<ref>]
 
 DESCRIPTION
 -----------
@@ -57,6 +58,15 @@  OPTIONS
 	`xx`; for example `%00` interpolates to `\0` (NUL),
 	`%09` to `\t` (TAB) and `%0a` to `\n` (LF).
 
+--notes=<ref>::
+	Show the notes that annotate the object. With an optional '<ref>' argument,
+	use the ref to find the notes to display.
+	(see linkgit:git-notes[1] and linkgit:pretty-options.txt[])
+--no-notes::
+	Do not show notes. This negates the above `--notes` option, by
+	resetting the list of notes refs from which notes are shown.
+	(see linkgit:git-notes[1] and linkgit:pretty-options.txt[])
+
 --color[=<when>]::
 	Respect any colors specified in the `--format` option. The
 	`<when>` field must be one of `always`, `never`, or `auto` (if
@@ -139,6 +149,9 @@  deltabase::
 	given object, if it is stored as a delta.  Otherwise it
 	expands to the null object name (all zeroes).
 
+notes::
+	The notes associated with the object pointed at by the ref.
+
 upstream::
 	The name of a local ref which can be considered ``upstream''
 	from the displayed ref. Respects `:short`, `:lstrip` and
@@ -302,6 +315,7 @@  git for-each-ref --count=3 --sort='-*authordate' \
 Subject: %(*subject)
 Date: %(*authordate)
 Ref: %(*refname)
+Notes: %(*notes)
 
 %(*body)
 ' 'refs/tags'
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 89cb6307d46f..12e6e673d48e 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -14,6 +14,26 @@  static char const * const for_each_ref_usage[] = {
 	NULL
 };
 
+extern struct notes_info notes_info;
+
+int parse_opt_notes(const struct option *opt, const char *arg, int unset)
+{
+	struct notes_info *ni = opt->value;
+
+	if (unset) {
+		disable_display_notes(&ni->notes_option, &ni->show_notes);
+		return 0;
+	}
+
+	if (!arg)
+		return -1;
+
+	enable_ref_display_notes(&ni->notes_option, &ni->show_notes, arg);
+	ni->notes_option.use_default_notes = 0;
+
+	return 0;
+}
+
 int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 {
 	int i;
@@ -38,6 +58,8 @@  int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 		OPT_GROUP(""),
 		OPT_INTEGER( 0 , "count", &maxcount, N_("show only <n> matched refs")),
 		OPT_STRING(  0 , "format", &format.format, N_("format"), N_("format to use for the output")),
+		OPT_CALLBACK(0, "notes", &notes_info, N_("notes"), N_("the notes associated"
+			     "with the object pointed at by the ref"), parse_opt_notes),
 		OPT__COLOR(&format.use_color, N_("respect format colors")),
 		OPT_REF_SORT(sorting_tail),
 		OPT_CALLBACK(0, "points-at", &filter.points_at,
@@ -51,6 +73,9 @@  int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 		OPT_END(),
 	};
 
+	init_display_notes(&notes_info.notes_option);
+	enable_default_display_notes(&notes_info.notes_option, &notes_info.show_notes);
+
 	memset(&array, 0, sizeof(array));
 	memset(&filter, 0, sizeof(filter));
 
@@ -97,5 +122,6 @@  int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 	free_commit_list(filter.with_commit);
 	free_commit_list(filter.no_commit);
 	UNLEAK(sorting);
+	string_list_clear(&notes_info.notes_option.extra_notes_refs, 0);
 	return 0;
 }
diff --git a/ref-filter.c b/ref-filter.c
index e2eac50d9508..ffcc1e500ed3 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -84,6 +84,8 @@  static struct expand_data {
 	struct object_info info;
 } oi, oi_deref;
 
+struct notes_info notes_info;
+
 struct ref_to_worktree_entry {
 	struct hashmap_entry ent;
 	struct worktree *wt; /* key is wt->head_ref */
@@ -295,6 +297,16 @@  static int deltabase_atom_parser(const struct ref_format *format, struct used_at
 	return 0;
 }
 
+static int notes_atom_parser(const struct ref_format *format, struct used_atom *atom,
+				 const char *arg, struct strbuf *err)
+{
+	if(!notes_info.show_notes_given && notes_info.show_notes) {
+			load_display_notes(&notes_info.notes_option);
+			notes_info.show_notes_given = 1;
+	}
+	return 0;
+}
+
 static int body_atom_parser(const struct ref_format *format, struct used_atom *atom,
 			    const char *arg, struct strbuf *err)
 {
@@ -506,6 +518,7 @@  static struct {
 	{ "objectsize", SOURCE_OTHER, FIELD_ULONG, objectsize_atom_parser },
 	{ "objectname", SOURCE_OTHER, FIELD_STR, oid_atom_parser },
 	{ "deltabase", SOURCE_OTHER, FIELD_STR, deltabase_atom_parser },
+	{ "notes", SOURCE_OTHER, FIELD_STR, notes_atom_parser },
 	{ "tree", SOURCE_OBJ, FIELD_STR, oid_atom_parser },
 	{ "parent", SOURCE_OBJ, FIELD_STR, oid_atom_parser },
 	{ "numparent", SOURCE_OBJ, FIELD_ULONG },
@@ -953,6 +966,17 @@  static int grab_oid(const char *name, const char *field, const struct object_id
 	return 0;
 }
 
+static void grab_notes(const struct object_id *oid, struct atom_value *v)
+{
+	struct strbuf notebuf = STRBUF_INIT;
+
+	if (notes_info.show_notes)
+			format_display_notes(oid, &notebuf,
+			     get_log_output_encoding(), 1);
+	strbuf_trim_trailing_newline(&notebuf);
+	v->s = strbuf_detach(&notebuf, NULL);
+}
+
 /* See grab_values */
 static void grab_common_values(struct atom_value *val, int deref, struct expand_data *oi)
 {
@@ -975,8 +999,12 @@  static void grab_common_values(struct atom_value *val, int deref, struct expand_
 			v->s = xstrfmt("%"PRIuMAX , (uintmax_t)oi->size);
 		} else if (!strcmp(name, "deltabase"))
 			v->s = xstrdup(oid_to_hex(&oi->delta_base_oid));
-		else if (deref)
-			grab_oid(name, "objectname", &oi->oid, v, &used_atom[i]);
+		else if (deref) {
+			if (!strcmp(name, "notes"))
+				grab_notes(&oi->oid, v);
+			else
+				grab_oid(name, "objectname", &oi->oid, v, &used_atom[i]);
+		}
 	}
 }
 
@@ -1767,6 +1795,9 @@  static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 			continue;
 		} else if (!deref && grab_oid(name, "objectname", &ref->objectname, v, atom)) {
 			continue;
+		} else if (!deref && !strcmp(name, "notes")) {
+			grab_notes(&ref->objectname, v);
+			continue;
 		} else if (!strcmp(name, "HEAD")) {
 			if (atom->u.head && !strcmp(ref->refname, atom->u.head))
 				v->s = xstrdup("*");
diff --git a/ref-filter.h b/ref-filter.h
index baf72a718965..b2f97bbd164e 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -5,6 +5,7 @@ 
 #include "refs.h"
 #include "commit.h"
 #include "parse-options.h"
+#include "notes.h"
 
 /* Quoting styles */
 #define QUOTE_NONE 0
@@ -85,6 +86,12 @@  struct ref_format {
 
 #define REF_FORMAT_INIT { NULL, 0, -1 }
 
+struct notes_info {
+	int show_notes;
+	int show_notes_given;
+	struct display_notes_opt notes_option;
+};
+
 /*  Macros for checking --merged and --no-merged options */
 #define _OPT_MERGED_NO_MERGED(option, filter, h) \
 	{ OPTION_CALLBACK, 0, option, (filter), N_("commit"), (h), \
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 9e0214076b4d..a92636236dfb 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -32,8 +32,12 @@  test_expect_success setup '
 	git add one &&
 	git commit -m "Initial" &&
 	git branch -M main &&
+	git notes add -m "commit-notes" HEAD &&
+	git notes --ref=refs/notes/sky-walker add \
+	    -m "sky-notes" HEAD &&
 	setdate_and_increment &&
 	git tag -a -m "Tagging at $datestamp" testtag &&
+	git notes add -m "tag-notes" testtag &&
 	git update-ref refs/remotes/origin/main main &&
 	git remote add origin nowhere &&
 	git config branch.main.remote origin &&
@@ -162,6 +166,7 @@  test_atom head contents:signature ''
 test_atom head contents 'Initial
 '
 test_atom head HEAD '*'
+test_atom head notes $(git notes show refs/heads/main)
 
 test_atom tag refname refs/tags/testtag
 test_atom tag refname:short testtag
@@ -220,6 +225,8 @@  test_atom tag contents:signature ''
 test_atom tag contents 'Tagging at 1151968727
 '
 test_atom tag HEAD ' '
+test_atom tag notes $(git notes show refs/tags/testtag)
+test_atom tag "*notes" $(git notes show refs/heads/main)
 
 test_expect_success 'Check invalid atoms names are errors' '
 	test_must_fail git for-each-ref --format="%(INVALID)" refs/heads
@@ -380,6 +387,8 @@  test_expect_success 'exercise strftime with odd fields' '
 
 cat >expected <<\EOF
 refs/heads/main
+refs/notes/commits
+refs/notes/sky-walker
 refs/remotes/origin/main
 refs/tags/testtag
 EOF
@@ -393,6 +402,8 @@  test_expect_success 'Verify ascending sort' '
 cat >expected <<\EOF
 refs/tags/testtag
 refs/remotes/origin/main
+refs/notes/sky-walker
+refs/notes/commits
 refs/heads/main
 EOF
 
@@ -429,6 +440,8 @@  test_expect_success 'exercise glob patterns with prefixes' '
 
 cat >expected <<\EOF
 'refs/heads/main'
+'refs/notes/commits'
+'refs/notes/sky-walker'
 'refs/remotes/origin/main'
 'refs/tags/testtag'
 EOF
@@ -450,6 +463,8 @@  test_expect_success 'Quoting style: python' '
 
 cat >expected <<\EOF
 "refs/heads/main"
+"refs/notes/commits"
+"refs/notes/sky-walker"
 "refs/remotes/origin/main"
 "refs/tags/testtag"
 EOF
@@ -509,6 +524,8 @@  test_expect_success 'Check for invalid refname format' '
 test_expect_success 'set up color tests' '
 	cat >expected.color <<-EOF &&
 	$(git rev-parse --short refs/heads/main) <GREEN>main<RESET>
+	$(git rev-parse --short refs/notes/commits) <GREEN>notes/commits<RESET>
+	$(git rev-parse --short refs/notes/sky-walker) <GREEN>notes/sky-walker<RESET>
 	$(git rev-parse --short refs/remotes/myfork/main) <GREEN>myfork/main<RESET>
 	$(git rev-parse --short refs/remotes/origin/main) <GREEN>origin/main<RESET>
 	$(git rev-parse --short refs/tags/testtag) <GREEN>testtag<RESET>
@@ -1007,6 +1024,53 @@  test_expect_success 'Add symbolic ref for the following tests' '
 	git symbolic-ref refs/heads/sym refs/heads/main
 '
 
+test_expect_success 'for-each-ref --notes and --no-notes' '
+	cat >expected <<-\EOF &&
+	sky-notes
+	EOF
+	git for-each-ref --format="%(notes)" --notes=refs/notes/sky-walker \
+	    refs/heads/ambiguous >actual &&
+	test_cmp expected actual &&
+	git for-each-ref --format="%(notes)" --notes=sky-walker refs/heads/ambiguous >actual &&
+	test_cmp expected actual &&
+	git for-each-ref --format="%(notes)" --notes=sky-walker --notes=commits \
+	    --no-notes --notes=sky-walker refs/heads/ambiguous >actual &&
+	test_cmp expected actual &&
+	cat >expected <<-\EOF &&
+	commit-notes
+	EOF
+	git for-each-ref --format="%(notes)" --notes=commits refs/heads/ambiguous >actual &&
+	cat >expected <<-\EOF &&
+	sky-notes
+	commit-notes
+	EOF
+	git for-each-ref --format="%(notes)" --notes=sky-walker --notes=commits \
+	    refs/heads/ambiguous >actual &&
+	test_cmp expected actual &&
+	cat >expected <<-\EOF &&
+	commit-notes
+	sky-notes
+	EOF
+	git for-each-ref --format="%(notes)" --notes=commits --notes=sky-walker \
+	    refs/heads/ambiguous >actual &&
+	test_cmp expected actual &&
+	cat >expected <<-\EOF &&
+
+	EOF
+	git for-each-ref --format="%(notes)" --notes=sky-walker --notes=commits --no-notes \
+	    refs/heads/ambiguous >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'for-each-ref --notes with remote ref' '
+	cat >expected <<-\EOF &&
+	refs/remotes/origin/main sky-notes
+	EOF
+	git for-each-ref --format="%(refname) %(notes)" --notes=refs/notes/sky-walker \
+	    refs/remotes/origin/main >actual &&
+	test_cmp expected actual
+'
+
 cat >expected <<EOF
 refs/heads/main
 EOF