Re: [PATCH] ref-filter: add "notes" atom

[Kousik Sanagavarapu <five231003@gmail.com>]

On Thu, Nov 21, 2024 at 08:51:47AM +0900, Junio C Hamano wrote:
> Kousik Sanagavarapu <five231003@gmail.com> 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