threads / rfc / 58256

RFC patch, 2 partsnotes: create interface to iterate over notes for a given oid

Subject: [RFC PATCH 2/2] notes: create interface to iterate over notes for a given oid

## tl;dr

8 messages between Aug 2, 2022 and Oct 19, 2022. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

Vegard Nossum· Aug 2, 2022, 07:54 UTC · lore

[RFC PATCH 1/2] notes: support fetching notes from an external repo

Notes are currently always fetched from the current repo. However, in certain situations you may want to keep notes in a separate repository altogether.

In my specific case, I am using cgit to display notes for repositories that are owned by others but hosted on a shared machine, so I cannot really add the notes directly to their repositories.

This patch makes it so that you can do:
    const char *notes_repo_path = "path/to/notes.git";
    const char *notes_ref = "refs/notes/commits";
    struct repository notes_repo;
    struct display_notes_opt notes_opt;
    repo_init(&notes_repo, notes_repo_path, NULL);
    add_to_alternates_memory(notes_repo.objects->odb->path);
    init_display_notes(&notes_opt);
    notes_opt.repo = &notes_repo;
    notes_opt.use_default_notes = 0;
    string_list_append(&notes_opt.extra_notes_refs, notes_ref);
    load_display_notes(&notes_opt);

...and then notes will be taken from the given ref in the external repository.

Given that the functionality is not exposed through the command line, there is currently no way to add regression tests for this.

Cc: Johan Herland <johan@herland.net>
Cc: Jason A. Donenfeld <Jason@zx2c4.com>
Cc: Christian Hesse <mail@eworm.de>
Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
---
 common-main.c |  2 ++
 notes.c       | 15 ++++++++++++---
 notes.h       |  3 +++
 refs.c        | 12 +++++++++---
 refs.h        |  2 ++
 5 files changed, 28 insertions(+), 6 deletions(-)
Show changes to 5 files +28 −6

common-main.c, notes.c, notes.h, refs.c, refs.h

diff --git a/common-main.c b/common-main.c
index c531372f3f..74b69a4632 100644
--- a/common-main.c
+++ b/common-main.c
@@ -1,6 +1,7 @@
 #include "cache.h"
 #include "exec-cmd.h"
 #include "attr.h"
+#include "notes.h"
 
 /*
  * Many parts of Git have subprograms communicate via pipe, expect the
@@ -43,6 +44,7 @@ int main(int argc, const char **argv)
 	git_setup_gettext();
 
 	initialize_the_repository();
+	init_default_notes_repository();
 
 	attr_start();
 
diff --git a/notes.c b/notes.c
index 7452e71cc8..90ec625192 100644
--- a/notes.c
+++ b/notes.c
@@ -73,11 +73,17 @@ struct non_note {
 #define SUBTREE_SHA1_PREFIXCMP(key_sha1, subtree_sha1) \
 	(memcmp(key_sha1, subtree_sha1, subtree_sha1[KEY_INDEX]))
 
+struct repository *default_notes_repo;
 struct notes_tree default_notes_tree;
 
 static struct string_list display_notes_refs = STRING_LIST_INIT_NODUP;
 static struct notes_tree **display_notes_trees;
 
+void init_default_notes_repository()
+{
+	default_notes_repo = the_repository;
+}
+
 static void load_subtree(struct notes_tree *t, struct leaf_node *subtree,
 		struct int_node *node, unsigned int n);
 
@@ -940,10 +946,10 @@ void string_list_add_refs_by_glob(struct string_list *list, const char *glob)
 {
 	assert(list->strdup_strings);
 	if (has_glob_specials(glob)) {
-		for_each_glob_ref(string_list_add_one_ref, glob, list);
+		repo_for_each_glob_ref_in(default_notes_repo, string_list_add_one_ref, glob, NULL, list);
 	} else {
 		struct object_id oid;
-		if (get_oid(glob, &oid))
+		if (repo_get_oid(default_notes_repo, glob, &oid))
 			warning("notes ref %s is invalid", glob);
 		if (!unsorted_string_list_has_string(list, glob))
 			string_list_append(list, glob);
@@ -1019,7 +1025,7 @@ void init_notes(struct notes_tree *t, const char *notes_ref,
 	t->dirty = 0;
 
 	if (flags & NOTES_INIT_EMPTY || !notes_ref ||
-	    get_oid_treeish(notes_ref, &object_oid))
+	    repo_get_oid_treeish(default_notes_repo, notes_ref, &object_oid))
 		return;
 	if (flags & NOTES_INIT_WRITABLE && read_ref(notes_ref, &object_oid))
 		die("Cannot use notes ref %s", notes_ref);
@@ -1088,6 +1094,9 @@ void load_display_notes(struct display_notes_opt *opt)
 
 	assert(!display_notes_trees);
 
+	if (opt->repo)
+		default_notes_repo = opt->repo;
+
 	if (!opt || opt->use_default_notes > 0 ||
 	    (opt->use_default_notes == -1 && !opt->extra_notes_refs.nr)) {
 		string_list_append(&display_notes_refs, default_notes_ref());
diff --git a/notes.h b/notes.h
index c1682c39a9..c7aae85ea6 100644
--- a/notes.h
+++ b/notes.h
@@ -6,6 +6,8 @@
 struct object_id;
 struct strbuf;
 
+void init_default_notes_repository();
+
 /*
  * Function type for combining two notes annotating the same object.
  *
@@ -256,6 +258,7 @@ void free_notes(struct notes_tree *t);
 struct string_list;
 
 struct display_notes_opt {
+	struct repository *repo;
 	int use_default_notes;
 	struct string_list extra_notes_refs;
 };
diff --git a/refs.c b/refs.c
index 90bcb27168..cf0dc85872 100644
--- a/refs.c
+++ b/refs.c
@@ -468,8 +468,8 @@ void normalize_glob_ref(struct string_list_item *item, const char *prefix,
 	strbuf_release(&normalized_pattern);
 }
 
-int for_each_glob_ref_in(each_ref_fn fn, const char *pattern,
-	const char *prefix, void *cb_data)
+int repo_for_each_glob_ref_in(struct repository *r, each_ref_fn fn,
+	const char *pattern, const char *prefix, void *cb_data)
 {
 	struct strbuf real_pattern = STRBUF_INIT;
 	struct ref_filter filter;
@@ -492,12 +492,18 @@ int for_each_glob_ref_in(each_ref_fn fn, const char *pattern,
 	filter.prefix = prefix;
 	filter.fn = fn;
 	filter.cb_data = cb_data;
-	ret = for_each_ref(filter_refs, &filter);
+	ret = refs_for_each_ref(get_main_ref_store(r), filter_refs, &filter);
 
 	strbuf_release(&real_pattern);
 	return ret;
 }
 
+int for_each_glob_ref_in(each_ref_fn fn, const char *pattern,
+	const char *prefix, void *cb_data)
+{
+	return repo_for_each_glob_ref_in(the_repository, fn, pattern, prefix, cb_data);
+}
+
 int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data)
 {
 	return for_each_glob_ref_in(fn, pattern, NULL, cb_data);
diff --git a/refs.h b/refs.h
index 47cb9edbaa..1375c8531f 100644
--- a/refs.h
+++ b/refs.h
@@ -366,6 +366,8 @@ int for_each_replace_ref(struct repository *r, each_repo_ref_fn fn, void *cb_dat
 /* iterates all refs that match the specified glob pattern. */
 int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data);
 
+int repo_for_each_glob_ref_in(struct repository *r, each_ref_fn fn, const char *pattern,
+			 const char *prefix, void *cb_data);
 int for_each_glob_ref_in(each_ref_fn fn, const char *pattern,
 			 const char *prefix, void *cb_data);
 
-- 
2.35.1.46.g38062e73e0
Vegard Nossum· Aug 2, 2022, 07:54 UTC · re: Vegard Nossum · lore

format_display_notes() outputs notes in a specific format which is suitable for displaying in a terminal with "git log"/"git show". Other users may want a different format.

This patch adds a new function -- for_each_oid_note() -- which, given the oid for a commit, iterates over notes refs and calls the given callback function for each note ref that contains a corresponding note.

The old functionality can easily be implemented using the new interface, so I'm doing that at the same time.

Cc: Johan Herland <johan@herland.net>
Cc: Jason A. Donenfeld <Jason@zx2c4.com>
Cc: Christian Hesse <mail@eworm.de>
Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
---
 notes.c | 108 ++++++++++++++++++++++++++++++++++----------------------
 notes.h |   5 +++
 2 files changed, 70 insertions(+), 43 deletions(-)
Show changes to 2 files +70 −43

notes.c, notes.h

diff --git a/notes.c b/notes.c
index 90ec625192..4c7e883758 100644
--- a/notes.c
+++ b/notes.c
@@ -1242,56 +1242,68 @@ void free_notes(struct notes_tree *t)
 	memset(t, 0, sizeof(struct notes_tree));
 }
 
-/*
- * Fill the given strbuf with the notes associated with the given object.
- *
- * If the given notes_tree structure is not initialized, it will be auto-
- * initialized to the default value (see documentation for init_notes() above).
- * If the given notes_tree is NULL, the internal/default notes_tree will be
- * used instead.
- *
- * (raw != 0) gives the %N userformat; otherwise, the note message is given
- * for human consumption.
- */
-static void format_note(struct notes_tree *t, const struct object_id *object_oid,
-			struct strbuf *sb, const char *output_encoding, int raw)
+void for_each_oid_note(const struct object_id *object_oid,
+		       const char *output_encoding, int raw,
+		       each_oid_note_fn fn, void *cb_data)
 {
 	static const char utf8[] = "utf-8";
-	const struct object_id *oid;
-	char *msg, *msg_p;
-	unsigned long linelen, msglen;
-	enum object_type type;
 
-	if (!t)
-		t = &default_notes_tree;
-	if (!t->initialized)
-		init_notes(t, NULL, NULL, 0);
+	int i;
+	assert(display_notes_trees);
+	for (i = 0; display_notes_trees[i]; i++) {
+		struct notes_tree *t = display_notes_trees[i];
+		const struct object_id *oid;
+		char *msg;
+		unsigned long msglen;
+		enum object_type type;
+
+		if (!t)
+			t = &default_notes_tree;
+		if (!t->initialized)
+			init_notes(t, NULL, NULL, 0);
+
+		oid = get_note(t, object_oid);
+		if (!oid)
+			continue;
 
-	oid = get_note(t, object_oid);
-	if (!oid)
-		return;
+		if (!(msg = read_object_file(oid, &type, &msglen)) || type != OBJ_BLOB) {
+			free(msg);
+			continue;
+		}
+
+		if (output_encoding && *output_encoding &&
+		    !is_encoding_utf8(output_encoding)) {
+			char *reencoded = reencode_string(msg, output_encoding, utf8);
+			if (reencoded) {
+				free(msg);
+				msg = reencoded;
+				msglen = strlen(msg);
+			}
+		}
 
-	if (!(msg = read_object_file(oid, &type, &msglen)) || type != OBJ_BLOB) {
+		fn(t->ref, msg, msglen, cb_data);
 		free(msg);
-		return;
 	}
+}
 
-	if (output_encoding && *output_encoding &&
-	    !is_encoding_utf8(output_encoding)) {
-		char *reencoded = reencode_string(msg, output_encoding, utf8);
-		if (reencoded) {
-			free(msg);
-			msg = reencoded;
-			msglen = strlen(msg);
-		}
-	}
+struct format_display_notes_cb {
+	int raw;
+	struct strbuf *output;
+};
+
+static void format_note(const char *ref, const char *msg, unsigned long msglen, void *cb_data)
+{
+	struct format_display_notes_cb *cb = cb_data;
+	int raw = cb->raw;
+	struct strbuf *sb = cb->output;
+	const char *msg_p;
+	unsigned long linelen;
 
 	/* we will end the annotation by a newline anyway */
 	if (msglen && msg[msglen - 1] == '\n')
 		msglen--;
 
 	if (!raw) {
-		const char *ref = t->ref;
 		if (!ref || !strcmp(ref, GIT_NOTES_DEFAULT_REF)) {
 			strbuf_addstr(sb, "\nNotes:\n");
 		} else {
@@ -1309,18 +1321,28 @@ static void format_note(struct notes_tree *t, const struct object_id *object_oid
 		strbuf_add(sb, msg_p, linelen);
 		strbuf_addch(sb, '\n');
 	}
-
-	free(msg);
 }
 
+/*
+ * Fill the given strbuf with the notes associated with the given object.
+ *
+ * If the given notes_tree structure is not initialized, it will be auto-
+ * initialized to the default value (see documentation for init_notes() above).
+ * If the given notes_tree is NULL, the internal/default notes_tree will be
+ * used instead.
+ *
+ * (raw != 0) gives the %N userformat; otherwise, the note message is given
+ * for human consumption.
+ */
 void format_display_notes(const struct object_id *object_oid,
 			  struct strbuf *sb, const char *output_encoding, int raw)
 {
-	int i;
-	assert(display_notes_trees);
-	for (i = 0; display_notes_trees[i]; i++)
-		format_note(display_notes_trees[i], object_oid, sb,
-			    output_encoding, raw);
+	struct format_display_notes_cb cb = {
+		.raw = raw,
+		.output = sb,
+	};
+
+	for_each_oid_note(object_oid, output_encoding, raw, format_note, &cb);
 }
 
 int copy_note(struct notes_tree *t,
diff --git a/notes.h b/notes.h
index c7aae85ea6..833af94fae 100644
--- a/notes.h
+++ b/notes.h
@@ -309,6 +309,11 @@ void load_display_notes(struct display_notes_opt *opt);
 void format_display_notes(const struct object_id *object_oid,
 			  struct strbuf *sb, const char *output_encoding, int raw);
 
+typedef void (*each_oid_note_fn)(const char *ref, const char *msg, unsigned long msglen, void *cb_data);
+
+void for_each_oid_note(const struct object_id *object_oid,
+		       const char *output_encoding, int raw, each_oid_note_fn fn, void *cb_data);
+
 /*
  * Load the notes tree from each ref listed in 'refs'.  The output is
  * an array of notes_tree*, terminated by a NULL.
-- 
2.35.1.46.g38062e73e0
Junio C Hamano· Aug 2, 2022, 15:40 UTC · re: Vegard Nossum · lore

Re: [RFC PATCH 1/2] notes: support fetching notes from an external repo

Vegard Nossum <vegard.nossum@oracle.com> writes:
Show 7 quoted lines
> Notes are currently always fetched from the current repo. However, in
> certain situations you may want to keep notes in a separate repository
> altogether.
>
> In my specific case, I am using cgit to display notes for repositories
> that are owned by others but hosted on a shared machine, so I cannot
> really add the notes directly to their repositories.

My gut reaction is that I am not interested at all in the above approach, even though the problem you are trying to solve is interesting. Mostly because notes are not the only decorations your users may want. What if you want to "log --decorate" their repository contents with your own tags that annotate their commits? A notes-only approach to mix repositories is way too narrow.

A usable alternative _might_ be to introduce a way to "borrow" refs and objects from a different repository as if you cloned from and continuously fetching from them. We already have a mechanism to borrow objects from another repository in the form of "alternate object database" that lets us pretend objects in their repository are locally available. We can invent a similar mechanism that lets any of their ref as if it were our local ref, e.g. their "main" branch at their refs/heads/main might appear to exist at our refs/borrowed/X/heads/main.

Once the mechanism for doing so is in place, setting up such a parasite repository might be

    $ git clone --local-parasite=X /path/to/theirs mine

which would create an empty repository 'mine' that uses /path/to/theirs/.git/objects as one of its alternate object store, and their refs are borrowed under our refs/borrowed/X/.

Then you can tell your cgit to show refs/borrowed/X/{heads,tags} hierarchies as if they are the branches and tags, and use your own refs/notes/ hiearchy to store whatever notes they do not let you store in theirs.

Vegard Nossum· Aug 3, 2022, 08:09 UTC · re: Junio C Hamano · lore

Re: [RFC PATCH 1/2] notes: support fetching notes from an external repo

On 8/2/22 17:40, Junio C Hamano wrote:
Show 26 quoted lines
> Vegard Nossum <vegard.nossum@oracle.com> writes:
> 
>> Notes are currently always fetched from the current repo. However, in
>> certain situations you may want to keep notes in a separate repository
>> altogether.
>>
>> In my specific case, I am using cgit to display notes for repositories
>> that are owned by others but hosted on a shared machine, so I cannot
>> really add the notes directly to their repositories.
> 
> My gut reaction is that I am not interested at all in the above
> approach, even though the problem you are trying to solve is
> interesting.  Mostly because notes are not the only decorations your
> users may want.  What if you want to "log --decorate" their
> repository contents with your own tags that annotate their commits?
> A notes-only approach to mix repositories is way too narrow.
> 
> A usable alternative _might_ be to introduce a way to "borrow" refs
> and objects from a different repository as if you cloned from and
> continuously fetching from them.  We already have a mechanism to
> borrow objects from another repository in the form of "alternate
> object database" that lets us pretend objects in their repository
> are locally available.  We can invent a similar mechanism that lets
> any of their ref as if it were our local ref, e.g. their "main"
> branch at their refs/heads/main might appear to exist at our
> refs/borrowed/X/heads/main.  
Hi Junio,
Thanks for the reply.

To be clear, are you saying there is no way you would ever take my patches in their current form, even though they only rearrange internal workings (and have no other user-observable effects) to solve a problem I am currently facing?

The thing is, I personally have no use for displaying refs borrowed from another repository at this time, and I'm not sure I have either the time or the ability to provide what you are asking for.

I don't think my patches preclude adding "borrowed refs" as a feature at a later time, so can we not do that when somebody actually has a use for it?

Just to provide a bit more background: These two patches are just the first two in a bigger project to make extensive use of git notes to provide added value to the whole Linux kernel community -- in other words, this is not just for myself, I am trying here to upstream our internal patches for the benefit of everybody. I have cgit patches as well (but I'm waiting to submit them until git can support them) and hundreds of thousands of notes annotating Linux kernel commits with useful information.

Respectfully,
Vegard
Junio C Hamano· Aug 3, 2022, 22:32 UTC · re: Vegard Nossum · lore

Re: [RFC PATCH 1/2] notes: support fetching notes from an external repo

Vegard Nossum <vegard.nossum@oracle.com> writes:
Show 9 quoted lines
>> My gut reaction is that I am not interested at all in the above
>> approach, even though the problem you are trying to solve is
>> interesting.  Mostly because notes are not the only decorations your
>> users may want.  What if you want to "log --decorate" their
>> repository contents with your own tags that annotate their commits?
>> A notes-only approach to mix repositories is way too narrow.
> ...
> To be clear, are you saying there is no way you would ever take my
> patches in their current form,
Correct.
> Just to provide a bit more background: These two patches are just the
> first two in a bigger project to make extensive use of git notes to
> provide added value to the whole Linux kernel community ...

I already said that the problem being solved is interesting. The fact that a solution aims to address a problem worth solving does not diminish the need for the solution to be sensibly designed, and I again already said that an approach to special case notes ref specially is unwanted.

Thanks.
Philip Oakley· Aug 30, 2022, 14:17 UTC · re: Vegard Nossum · lore

Re: [RFC PATCH 1/2] notes: support fetching notes from an external repo

Sorry for late comment.
On 02/08/2022 08:54, Vegard Nossum wrote:
Show 43 quoted lines
> Notes are currently always fetched from the current repo. However, in
> certain situations you may want to keep notes in a separate repository
> altogether.
>
> In my specific case, I am using cgit to display notes for repositories
> that are owned by others but hosted on a shared machine, so I cannot
> really add the notes directly to their repositories.
>
> This patch makes it so that you can do:
>
>     const char *notes_repo_path = "path/to/notes.git";
>     const char *notes_ref = "refs/notes/commits";
>
>     struct repository notes_repo;
>     struct display_notes_opt notes_opt;
>
>     repo_init(&notes_repo, notes_repo_path, NULL);
>     add_to_alternates_memory(notes_repo.objects->odb->path);
>
>     init_display_notes(&notes_opt);
>     notes_opt.repo = &notes_repo;
>     notes_opt.use_default_notes = 0;
>
>     string_list_append(&notes_opt.extra_notes_refs, notes_ref);
>     load_display_notes(&notes_opt);
>
> ...and then notes will be taken from the given ref in the external
> repository.
>
> Given that the functionality is not exposed through the command line,
> there is currently no way to add regression tests for this.
>
> Cc: Johan Herland <johan@herland.net>
> Cc: Jason A. Donenfeld <Jason@zx2c4.com>
> Cc: Christian Hesse <mail@eworm.de>
> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
> ---
>  common-main.c |  2 ++
>  notes.c       | 15 ++++++++++++---
>  notes.h       |  3 +++
>  refs.c        | 12 +++++++++---
>  refs.h        |  2 ++
>  5 files changed, 28 insertions(+), 6 deletions(-)

Where's the documentation? Without a clarity of purpose and usage then, for me, it doesn't fly.

I feel that underlying this there may something that's interesting, but without the 'SPIN' narrative (situation, problem, implication, and need-payoff) I'm not sure what it's trying to do from a broad user perspective. (Spin is just one approach to 'selling' the patches;-)

I'd agree that Notes are 'odd' in that they are out of sequence relative to commits, and may not be common between users viewing the same repo. I'd like to understand the issues in a wider context. -- Philip

Show 144 quoted lines
>
> diff --git a/common-main.c b/common-main.c
> index c531372f3f..74b69a4632 100644
> --- a/common-main.c
> +++ b/common-main.c
> @@ -1,6 +1,7 @@
>  #include "cache.h"
>  #include "exec-cmd.h"
>  #include "attr.h"
> +#include "notes.h"
>  
>  /*
>   * Many parts of Git have subprograms communicate via pipe, expect the
> @@ -43,6 +44,7 @@ int main(int argc, const char **argv)
>  	git_setup_gettext();
>  
>  	initialize_the_repository();
> +	init_default_notes_repository();
>  
>  	attr_start();
>  
> diff --git a/notes.c b/notes.c
> index 7452e71cc8..90ec625192 100644
> --- a/notes.c
> +++ b/notes.c
> @@ -73,11 +73,17 @@ struct non_note {
>  #define SUBTREE_SHA1_PREFIXCMP(key_sha1, subtree_sha1) \
>  	(memcmp(key_sha1, subtree_sha1, subtree_sha1[KEY_INDEX]))
>  
> +struct repository *default_notes_repo;
>  struct notes_tree default_notes_tree;
>  
>  static struct string_list display_notes_refs = STRING_LIST_INIT_NODUP;
>  static struct notes_tree **display_notes_trees;
>  
> +void init_default_notes_repository()
> +{
> +	default_notes_repo = the_repository;
> +}
> +
>  static void load_subtree(struct notes_tree *t, struct leaf_node *subtree,
>  		struct int_node *node, unsigned int n);
>  
> @@ -940,10 +946,10 @@ void string_list_add_refs_by_glob(struct string_list *list, const char *glob)
>  {
>  	assert(list->strdup_strings);
>  	if (has_glob_specials(glob)) {
> -		for_each_glob_ref(string_list_add_one_ref, glob, list);
> +		repo_for_each_glob_ref_in(default_notes_repo, string_list_add_one_ref, glob, NULL, list);
>  	} else {
>  		struct object_id oid;
> -		if (get_oid(glob, &oid))
> +		if (repo_get_oid(default_notes_repo, glob, &oid))
>  			warning("notes ref %s is invalid", glob);
>  		if (!unsorted_string_list_has_string(list, glob))
>  			string_list_append(list, glob);
> @@ -1019,7 +1025,7 @@ void init_notes(struct notes_tree *t, const char *notes_ref,
>  	t->dirty = 0;
>  
>  	if (flags & NOTES_INIT_EMPTY || !notes_ref ||
> -	    get_oid_treeish(notes_ref, &object_oid))
> +	    repo_get_oid_treeish(default_notes_repo, notes_ref, &object_oid))
>  		return;
>  	if (flags & NOTES_INIT_WRITABLE && read_ref(notes_ref, &object_oid))
>  		die("Cannot use notes ref %s", notes_ref);
> @@ -1088,6 +1094,9 @@ void load_display_notes(struct display_notes_opt *opt)
>  
>  	assert(!display_notes_trees);
>  
> +	if (opt->repo)
> +		default_notes_repo = opt->repo;
> +
>  	if (!opt || opt->use_default_notes > 0 ||
>  	    (opt->use_default_notes == -1 && !opt->extra_notes_refs.nr)) {
>  		string_list_append(&display_notes_refs, default_notes_ref());
> diff --git a/notes.h b/notes.h
> index c1682c39a9..c7aae85ea6 100644
> --- a/notes.h
> +++ b/notes.h
> @@ -6,6 +6,8 @@
>  struct object_id;
>  struct strbuf;
>  
> +void init_default_notes_repository();
> +
>  /*
>   * Function type for combining two notes annotating the same object.
>   *
> @@ -256,6 +258,7 @@ void free_notes(struct notes_tree *t);
>  struct string_list;
>  
>  struct display_notes_opt {
> +	struct repository *repo;
>  	int use_default_notes;
>  	struct string_list extra_notes_refs;
>  };
> diff --git a/refs.c b/refs.c
> index 90bcb27168..cf0dc85872 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -468,8 +468,8 @@ void normalize_glob_ref(struct string_list_item *item, const char *prefix,
>  	strbuf_release(&normalized_pattern);
>  }
>  
> -int for_each_glob_ref_in(each_ref_fn fn, const char *pattern,
> -	const char *prefix, void *cb_data)
> +int repo_for_each_glob_ref_in(struct repository *r, each_ref_fn fn,
> +	const char *pattern, const char *prefix, void *cb_data)
>  {
>  	struct strbuf real_pattern = STRBUF_INIT;
>  	struct ref_filter filter;
> @@ -492,12 +492,18 @@ int for_each_glob_ref_in(each_ref_fn fn, const char *pattern,
>  	filter.prefix = prefix;
>  	filter.fn = fn;
>  	filter.cb_data = cb_data;
> -	ret = for_each_ref(filter_refs, &filter);
> +	ret = refs_for_each_ref(get_main_ref_store(r), filter_refs, &filter);
>  
>  	strbuf_release(&real_pattern);
>  	return ret;
>  }
>  
> +int for_each_glob_ref_in(each_ref_fn fn, const char *pattern,
> +	const char *prefix, void *cb_data)
> +{
> +	return repo_for_each_glob_ref_in(the_repository, fn, pattern, prefix, cb_data);
> +}
> +
>  int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data)
>  {
>  	return for_each_glob_ref_in(fn, pattern, NULL, cb_data);
> diff --git a/refs.h b/refs.h
> index 47cb9edbaa..1375c8531f 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -366,6 +366,8 @@ int for_each_replace_ref(struct repository *r, each_repo_ref_fn fn, void *cb_dat
>  /* iterates all refs that match the specified glob pattern. */
>  int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data);
>  
> +int repo_for_each_glob_ref_in(struct repository *r, each_ref_fn fn, const char *pattern,
> +			 const char *prefix, void *cb_data);
>  int for_each_glob_ref_in(each_ref_fn fn, const char *pattern,
>  			 const char *prefix, void *cb_data);
>  
Vegard Nossum· Oct 17, 2022, 13:14 UTC · re: Philip Oakley · lore

Re: [RFC PATCH 1/2] notes: support fetching notes from an external repo

On 8/30/22 16:17, Philip Oakley wrote:
> Sorry for late comment.

And sorry for late response! I didn't receive your email for some reason, but I noticed it in the list archives.

Show 58 quoted lines
> On 02/08/2022 08:54, Vegard Nossum wrote:
>> Notes are currently always fetched from the current repo. However, in
>> certain situations you may want to keep notes in a separate repository
>> altogether.
>>
>> In my specific case, I am using cgit to display notes for repositories
>> that are owned by others but hosted on a shared machine, so I cannot
>> really add the notes directly to their repositories.
>>
>> This patch makes it so that you can do:
>>
>>      const char *notes_repo_path = "path/to/notes.git";
>>      const char *notes_ref = "refs/notes/commits";
>>
>>      struct repository notes_repo;
>>      struct display_notes_opt notes_opt;
>>
>>      repo_init(&notes_repo, notes_repo_path, NULL);
>>      add_to_alternates_memory(notes_repo.objects->odb->path);
>>
>>      init_display_notes(&notes_opt);
>>      notes_opt.repo = &notes_repo;
>>      notes_opt.use_default_notes = 0;
>>
>>      string_list_append(&notes_opt.extra_notes_refs, notes_ref);
>>      load_display_notes(&notes_opt);
>>
>> ...and then notes will be taken from the given ref in the external
>> repository.
>>
>> Given that the functionality is not exposed through the command line,
>> there is currently no way to add regression tests for this.
>>
>> Cc: Johan Herland <johan@herland.net>
>> Cc: Jason A. Donenfeld <Jason@zx2c4.com>
>> Cc: Christian Hesse <mail@eworm.de>
>> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
>> ---
>>   common-main.c |  2 ++
>>   notes.c       | 15 ++++++++++++---
>>   notes.h       |  3 +++
>>   refs.c        | 12 +++++++++---
>>   refs.h        |  2 ++
>>   5 files changed, 28 insertions(+), 6 deletions(-)
> 
> Where's the documentation? Without a clarity of purpose and usage then,
> for me, it doesn't fly.
> 
> I feel that underlying this there may something that's interesting, but
> without the 'SPIN' narrative (situation, problem, implication, and
> need-payoff) I'm not sure what it's trying to do from a broad user
> perspective. (Spin is just one approach to 'selling' the patches;-)
> 
> I'd agree that Notes are 'odd' in that they are out of sequence relative
> to commits, and may not be common between users viewing the same repo.
> I'd like to understand the issues in a wider context.
> --
> Philip

Perhaps the best way to showcase this is with a screenshot of what we're trying to upstream:

https://vegard.github.io/cgit/6399f1fae4ec.png

Since git commits cannot be changed without rewriting history, git notes is the mechanism by which we can attach new information to existing commits. We're internally using these notes for cross-referencing information like references to subsequent fixes, backports in other trees, mailing list discussions, etc.

There is also a bit more information in my cgit patch submission from today: https://lists.zx2c4.com/pipermail/cgit/2022-October/004764.html

My "problem" is that there are many moving parts to this, and the two git.git patches sit at the top of the dependency chain:

1. these git patches
2. the cgit patches
3. the Linux kernel-specific notes generation scripts/logic
4. the Linux kernel notes themselves
5. displaying notes on kernel.org

Almost all of these steps involve different people with different standards, different motivations, different priorities.

As I wrote earlier, I am trying to be a good citizen and upstream as much of this as I can. But it's hard to justify what Junio asked for: scrapping my current patches (which we are currently using...) in favour of a complete rewrite with more functionality that does not buy us anything from my point of view.

Does this clarify things?

I think my patches are a good cleanup regardless of motivation and everything was fairly well documented in the changelogs, so I'm surprised to see skepticism in the git community.

Vegard
Philip Oakley· Oct 19, 2022, 09:15 UTC · re: Vegard Nossum · lore

Re: [RFC PATCH 1/2] notes: support fetching notes from an external repo

Hi Vegard,
On 17/10/2022 14:14, Vegard Nossum wrote:
Show 99 quoted lines
>
> On 8/30/22 16:17, Philip Oakley wrote:
>> Sorry for late comment.
>
> And sorry for late response! I didn't receive your email for some
> reason, but I noticed it in the list archives.
>
>> On 02/08/2022 08:54, Vegard Nossum wrote:
>>> Notes are currently always fetched from the current repo. However, in
>>> certain situations you may want to keep notes in a separate repository
>>> altogether.
>>>
>>> In my specific case, I am using cgit to display notes for repositories
>>> that are owned by others but hosted on a shared machine, so I cannot
>>> really add the notes directly to their repositories.
>>>
>>> This patch makes it so that you can do:
>>>
>>>      const char *notes_repo_path = "path/to/notes.git";
>>>      const char *notes_ref = "refs/notes/commits";
>>>
>>>      struct repository notes_repo;
>>>      struct display_notes_opt notes_opt;
>>>
>>>      repo_init(&notes_repo, notes_repo_path, NULL);
>>>      add_to_alternates_memory(notes_repo.objects->odb->path);
>>>
>>>      init_display_notes(&notes_opt);
>>>      notes_opt.repo = &notes_repo;
>>>      notes_opt.use_default_notes = 0;
>>>
>>>      string_list_append(&notes_opt.extra_notes_refs, notes_ref);
>>>      load_display_notes(&notes_opt);
>>>
>>> ...and then notes will be taken from the given ref in the external
>>> repository.
>>>
>>> Given that the functionality is not exposed through the command line,
>>> there is currently no way to add regression tests for this.
>>>
>>> Cc: Johan Herland <johan@herland.net>
>>> Cc: Jason A. Donenfeld <Jason@zx2c4.com>
>>> Cc: Christian Hesse <mail@eworm.de>
>>> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
>>> ---
>>>   common-main.c |  2 ++
>>>   notes.c       | 15 ++++++++++++---
>>>   notes.h       |  3 +++
>>>   refs.c        | 12 +++++++++---
>>>   refs.h        |  2 ++
>>>   5 files changed, 28 insertions(+), 6 deletions(-)
>>
>> Where's the documentation? Without a clarity of purpose and usage then,
>> for me, it doesn't fly.
>>
>> I feel that underlying this there may something that's interesting, but
>> without the 'SPIN' narrative (situation, problem, implication, and
>> need-payoff) I'm not sure what it's trying to do from a broad user
>> perspective. (Spin is just one approach to 'selling' the patches;-)
>>
>> I'd agree that Notes are 'odd' in that they are out of sequence relative
>> to commits, and may not be common between users viewing the same repo.
>> I'd like to understand the issues in a wider context.
>> -- 
>> Philip
>
> Perhaps the best way to showcase this is with a screenshot of what we're
> trying to upstream:
>
> https://vegard.github.io/cgit/6399f1fae4ec.png
>
> Since git commits cannot be changed without rewriting history, git notes
> is the mechanism by which we can attach new information to existing
> commits. We're internally using these notes for cross-referencing
> information like references to subsequent fixes, backports in other
> trees, mailing list discussions, etc.
>
> There is also a bit more information in my cgit patch submission from
> today: https://lists.zx2c4.com/pipermail/cgit/2022-October/004764.html
>
> My "problem" is that there are many moving parts to this, and the two
> git.git patches sit at the top of the dependency chain:
>
> 1. these git patches
> 2. the cgit patches
> 3. the Linux kernel-specific notes generation scripts/logic
> 4. the Linux kernel notes themselves
> 5. displaying notes on kernel.org
>
> Almost all of these steps involve different people with different
> standards, different motivations, different priorities.
>
> As I wrote earlier, I am trying to be a good citizen and upstream as
> much of this as I can. But it's hard to justify what Junio asked for:
> scrapping my current patches (which we are currently using...) in favour
> of a complete rewrite with more functionality that does not buy us
> anything from my point of view.
>
> Does this clarify things?

Yes, and No; Even without Junio's desire for a broader functionality of the `alternate object database` (is it that, or an ext repo?), I still felt that given the new and improved functionality, it would need some extra text to go into the documentation and man pages, along with a short abstract to go into the release notes. Somehow the prospective users (e.g. me) would need to be told - I.e. be able to read from the man pages what to expect.

The commit messages also didn't really bring out where the benefit would be seen i.e. the cgit display (as per your screen shot). Also some annotation of the screen shot with an arrow pointing to what was 'new' could help.

Also you didn't really explain the point you make above about the "shared machine", which cuts across the normal "personal machine" view of the 'distributed' in DVCS.

>
> I think my patches are a good cleanup regardless of motivation and
> everything was fairly well documented in the changelogs, so I'm
> surprised to see skepticism in the git community.

In a sense, I hear your frustration. It does feel common that that every knife has to be converted to scissors (two knives working together) or a multi-tool Swiss Army knife, and in some cases loosing the original 'obviousness' of a simple thing done well.

A first step may be to write out "what would the man pages say" that explains how the the `alternate object database` is used and set up, and then maybe look at whether Junio's example, to see if you have explained this new capability well enough.

I hope that helps clarify my original comments.
Philip

← back to recent threads