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).

Kousik Sanagavarapu Dec. 1, 2024, 8:19 a.m. UTC | #2

On Thu, Nov 21, 2024 at 08:51:47AM +0900, Junio C Hamano wrote:
> 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).

Hi,
I was supposed to send a v2 a lot earlier but due to time constraints
(my semester is ending and there's work related to that) I've not been
able to work on this.  So if anyone is interested, they may work on this
and submit v2.  I personally think this format would be a nice addition
to the ref-filter framework and Junio's msg above gives a really nice
explanation on how to do things.

I will work on it in the earliest when I find time but just in case
someone else is interested, they may pick this up and work on it.

Thanks

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