ref-filter: add "notes" atom

Commit Message

Kousik Sanagavarapu Nov. 20, 2024, 5:04 p.m. UTC

On Sat, Nov 16, 2024 at 04:11:17PM +0100, Bence Ferdinandy wrote:
> 
> On Sat Nov 16, 2024 at 01:06, Junio C Hamano <[e-mail]> wrote:
> > "Bence Ferdinandy" <[e-mail]> writes:
> >
> >> based on the man pages it doesn't seem possible, but maybe I'm missing something.
> >>
> >> I would like to put together a "log --format=" which is similar to --oneline,
> >> but where if there's a note for the commit it's marked with e.g. a notebook
> >> symbol. There's %N, but that prints the entire note, so it doesn't work well
> >> with one commit per line.
> >
> > I do not think it is doable.  Unlike the format language in the
> > for-each-ref/branch --list family of commands, the pretty-format
> > language in the log family of commands lack more involved
> > conditional formatting features.
> >
> > Unifying these two formatting languages to port features from one to
> > the other would be needed, I guess.  
> 
> Is this already on the roadmap with a plan on how to do it? It sounds like
> switching log format to the same formatting as the other commands would make
> life easier in the long run.

Yes, but a fixed plan doesn't exist really.  There has been effort in
the past (and even now) to re-implement formats from pretty in ref-filter
and when that is done, change everything to use ref-filter, but the parsing
mechanism in ref-filter is a bit broken in some really really rare cases [1].

> > If we had a note support in the
> > latter,  something like
> >
> > $ git branch -l --format='%(subject)%(if)%(note:amlog)%(then)

Comments

Junio C Hamano Nov. 20, 2024, 11:51 p.m. UTC | #1

Kousik Sanagavarapu <[e-mail]> writes:

> +static void grab_notes_values(struct atom_value *val, int deref,
> +			      struct object *obj)
> +{
> +	for (int i = 0; i < used_atom_cnt; i++) {
> +		struct used_atom *atom = &used_atom[i];
> +		const char *name = atom->name;
> +		struct atom_value *v = &val[i];
> +
> +		struct child_process cmd = CHILD_PROCESS_INIT;
> +		struct strbuf out = STRBUF_INIT;
> +		struct strbuf err = STRBUF_INIT;
> +
> +		if (atom->atom_type != ATOM_NOTES)
> +			continue;
> +
> +		if (!!deref != (*name == '*'))
> +			continue;
> +
> +		cmd.git_cmd = 1;
> +		strvec_push(&cmd.args, "notes");
> +		if (atom->u.notes_refname) {
> +			strvec_push(&cmd.args, "--ref");
> +			strvec_push(&cmd.args, atom->u.notes_refname);
> +		}
> +		strvec_push(&cmd.args, "show");
> +		strvec_push(&cmd.args, oid_to_hex(&obj->oid));
> +		if (pipe_command(&cmd, NULL, 0, &out, 0, &err, 0) < 0) {
> +			error(_("failed to run 'notes'"));
> +			v->s = xstrdup("");
> +			continue;
> +		}
> +		strbuf_rtrim(&out);
> +		v->s = strbuf_detach(&out, NULL);
> +
> +		strbuf_release(&err);
> +	}
> +}

I suspect that this was written to mimick what is done for describe.

The describe codepath has a (semi-)valid reason to fork out to a
subprocess, as computation of describe smudges the object flags of
in-core object database and it is not trivial to call into the
helper functions twice.

But showing notes for a single commit is merely an internal call to
get_note() away, so unless the note object is not a blob (which
should be absolutely rare), spawning a subprocess for each and every
ref tip feels a bit heavier than acceptable.  We'd probably need to
maintain a table of notes_trees, one per <note-ref> used as
%(notes:<note-ref>) in the format string, and init_notes() on them
while parsing the atoms, and in this codepath it would be a look-up
of notes_tree from the table based on the u.notes_refname by calling
get_note() to learn the object name, plus reading the object
contents into the v->s member when the note object is a blob (and
fallback the above code when it is not a blob, which is a rare-case,
if we really want to handle them).

Patch

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index d3764401a2..406e4a0390 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -329,6 +329,13 @@  exclude=<pattern>;;
 	in linkgit:git-describe[1] for details.
 --
 
+notes:<notes-ref>::
+	Show notes associated with a ref.  Defaults to getting notes
+	from `refs/notes/commits`.  If `"notes-ref"` is given then notes
+	retrieved from that ref are shown; for the given ref.  For
+	example, `%(notes:amlog)` will retrieve the notes from
+	`refs/notes/amlog` for the given ref.  See linkgit:git-notes[1].
+
 In addition to the above, for commit and tag objects, the header
 field names (`tree`, `parent`, `object`, `type`, and `tag`) can
 be used to specify the value in the header field.
diff --git a/ref-filter.c b/ref-filter.c
index 84c6036107..76c06178f2 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -155,6 +155,7 @@  enum atom_type {
 	ATOM_TRAILERS,
 	ATOM_CONTENTS,
 	ATOM_SIGNATURE,
+	ATOM_NOTES,
 	ATOM_RAW,
 	ATOM_UPSTREAM,
 	ATOM_PUSH,
@@ -235,6 +236,7 @@  static struct used_atom {
 			       S_FINGERPRINT, S_PRI_KEY_FP, S_TRUST_LEVEL } option;
 		} signature;
 		struct strvec describe_args;
+		const char *notes_refname;
 		struct refname_atom refname;
 		char *head;
 	} u;
@@ -712,6 +714,15 @@  static int describe_atom_parser(struct ref_format *format UNUSED,
 	return 0;
 }
 
+static int notes_atom_parser(struct ref_format *format UNUSED,
+			     struct used_atom *atom,
+			     const char *arg, struct strbuf *err UNUSED)
+{
+	if (arg)
+		atom->u.notes_refname = arg;
+	return 0;
+}
+
 static int raw_atom_parser(struct ref_format *format UNUSED,
 			   struct used_atom *atom,
 			   const char *arg, struct strbuf *err)
@@ -974,6 +985,7 @@  static struct {
 	[ATOM_TRAILERS] = { "trailers", SOURCE_OBJ, FIELD_STR, trailers_atom_parser },
 	[ATOM_CONTENTS] = { "contents", SOURCE_OBJ, FIELD_STR, contents_atom_parser },
 	[ATOM_SIGNATURE] = { "signature", SOURCE_OBJ, FIELD_STR, signature_atom_parser },
+	[ATOM_NOTES] = { "notes", SOURCE_OBJ, FIELD_STR, notes_atom_parser },
 	[ATOM_RAW] = { "raw", SOURCE_OBJ, FIELD_STR, raw_atom_parser },
 	[ATOM_UPSTREAM] = { "upstream", SOURCE_NONE, FIELD_STR, remote_ref_atom_parser },
 	[ATOM_PUSH] = { "push", SOURCE_NONE, FIELD_STR, remote_ref_atom_parser },
@@ -1957,6 +1969,44 @@  static void grab_describe_values(struct atom_value *val, int deref,
 	}
 }
 
+static void grab_notes_values(struct atom_value *val, int deref,
+			      struct object *obj)
+{
+	for (int i = 0; i < used_atom_cnt; i++) {
+		struct used_atom *atom = &used_atom[i];
+		const char *name = atom->name;
+		struct atom_value *v = &val[i];
+
+		struct child_process cmd = CHILD_PROCESS_INIT;
+		struct strbuf out = STRBUF_INIT;
+		struct strbuf err = STRBUF_INIT;
+
+		if (atom->atom_type != ATOM_NOTES)
+			continue;
+
+		if (!!deref != (*name == '*'))
+			continue;
+
+		cmd.git_cmd = 1;
+		strvec_push(&cmd.args, "notes");
+		if (atom->u.notes_refname) {
+			strvec_push(&cmd.args, "--ref");
+			strvec_push(&cmd.args, atom->u.notes_refname);
+		}
+		strvec_push(&cmd.args, "show");
+		strvec_push(&cmd.args, oid_to_hex(&obj->oid));
+		if (pipe_command(&cmd, NULL, 0, &out, 0, &err, 0) < 0) {
+			error(_("failed to run 'notes'"));
+			v->s = xstrdup("");
+			continue;
+		}
+		strbuf_rtrim(&out);
+		v->s = strbuf_detach(&out, NULL);
+
+		strbuf_release(&err);
+	}
+}
+
 /* See grab_values */
 static void grab_sub_body_contents(struct atom_value *val, int deref, struct expand_data *data)
 {
@@ -2076,6 +2126,7 @@  static void grab_values(struct atom_value *val, int deref, struct object *obj, s
 		grab_sub_body_contents(val, deref, data);
 		grab_person("tagger", val, deref, buf);
 		grab_describe_values(val, deref, obj);
+		grab_notes_values(val, deref, obj);
 		break;
 	case OBJ_COMMIT:
 		grab_commit_values(val, deref, obj);
@@ -2084,14 +2135,17 @@  static void grab_values(struct atom_value *val, int deref, struct object *obj, s
 		grab_person("committer", val, deref, buf);
 		grab_signature(val, deref, obj);
 		grab_describe_values(val, deref, obj);
+		grab_notes_values(val, deref, obj);
 		break;
 	case OBJ_TREE:
 		/* grab_tree_values(val, deref, obj, buf, sz); */
 		grab_sub_body_contents(val, deref, data);
+		grab_notes_values(val, deref, obj);
 		break;
 	case OBJ_BLOB:
 		/* grab_blob_values(val, deref, obj, buf, sz); */
 		grab_sub_body_contents(val, deref, data);
+		grab_notes_values(val, deref, obj);
 		break;
 	default:
 		die("Eh?  Object of type %d?", obj->type);
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index c39d4e7e9c..cf2ff26fd1 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -847,6 +847,34 @@  test_expect_success 'err on bad describe atom arg' '
 	)
 '
 
+test_expect_success 'bare notes atom' '
+	test_when_finished "git checkout main && git branch -D notes-atom " &&
+
+	git checkout -b notes-atom &&
+	test_commit --no-tag "commit on notes-atom" &&
+
+	git notes add -m "some msg" refs/heads/notes-atom &&
+	git notes show refs/heads/notes-atom >expect &&
+	git for-each-ref --format="%(notes)" refs/heads/notes-atom >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'notes atom with notes ref' '
+	test_when_finished \
+		"git checkout main && git branch -D notes-atom-refname" &&
+
+	git checkout -b notes-atom-refname &&
+	test_commit --no-tag "commit on notes-atom-refname" &&
+
+	git notes --ref notes-ref add -m "some msg" \
+		 refs/heads/notes-atom-refname &&
+	git notes --ref notes-ref show \
+		refs/heads/notes-atom-refname >expect &&
+	git for-each-ref --format="%(notes:notes-ref)" \
+		refs/heads/notes-atom-refname >actual &&
+	test_cmp expect actual
+'
+
 cat >expected <<\EOF
 heads/main
 tags/main