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

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

[Vegard Nossum]

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

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

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

[Vegard Nossum]

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

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

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

[Junio C Hamano]

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.  

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.

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

[Vegard Nossum]

On 8/2/22 17:40, Junio C Hamano wrote:
> 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

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

[Junio C Hamano]

Vegard Nossum <vegard.nossum@oracle.com> writes:

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

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

[Philip Oakley]

Sorry for late comment.

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

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

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

[Vegard Nossum]

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?

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

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

[Philip Oakley]

Hi Vegard,

On 17/10/2022 14:14, Vegard Nossum wrote:
>
> 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