[PATCH] notes: Use get_sha1_committish instead of read_ref in init_notes()

[PATCH] notes: Use get_sha1_committish instead of read_ref in init_notes()

[Mike Hommey]

init_notes() is essentially the only point of entry to the notes API.
It is an arbitrary restriction that all it allows as input is a strict
ref name, when callers may want to give an arbitrary committish.

This has the side effect of enabling the use of committish as notes refs
in commands allowing them, e.g. git log --notes=foo@{1}, although
I haven't checked whether that's the case for all of them.

Signed-off-by: Mike Hommey <mh@glandium.org>
---

My motivation for this change is to allow to use init_notes from a
third-party helper that links to libgit.a with a commit that is not
pointed at directly by a ref. The side effect of making git log
--notes=foo@{1} work is nice IMHO. As noted, though, I haven't checked if
all code paths using init_notes don't prefilter the ref, but I assume
they don't. 

 notes.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/notes.c b/notes.c
index df08209..402e4ce 100644
--- a/notes.c
+++ b/notes.c
@@ -1012,7 +1012,7 @@ void init_notes(struct notes_tree *t, const char *notes_ref,
 	t->dirty = 0;
 
 	if (flags & NOTES_INIT_EMPTY || !notes_ref ||
-	    read_ref(notes_ref, object_sha1))
+	    get_sha1_committish(notes_ref, object_sha1))
 		return;
 	if (get_tree_entry(object_sha1, "", sha1, &mode))
 		die("Failed to read notes tree referenced by %s (%s)",
-- 
2.4.3.2.gf0a024e.dirty

Re: [PATCH] notes: Use get_sha1_committish instead of read_ref in init_notes()

[Junio C Hamano]

Mike Hommey <mh@glandium.org> writes:

> init_notes() is essentially the only point of entry to the notes API.
> It is an arbitrary restriction that all it allows as input is a strict
> ref name, when callers may want to give an arbitrary committish.

While it may be a good idea to allow reading from any note-shaped
tree-ish, not just what is at the tip of a ref, I suspect that the
use of read_ref() is not an arbitrary restriction, but is an
effective way to achieve safety against callers that update notes.

That is, you can feed refs/notes/commit@{4.days.ago} to the
machinery and show you notes from 4 days ago, but you cannot update
that as if it were a ref.

Hence, if you are loosening the safety at init_notes() site, you
would at least need to add a similar safety in the write codepath, I
would think.  

One thing you would need to be careful about is that you would give
users a crappy experience, if an operation reads notes, does its own
thing, and then tries to write updated notes (think: rebase that
transplants notes from original to rewritten commits), and you fail
the operation only at the very last phase of updating.  In order to
prevent that, "in the write codepath" above has to be "reject any
non-ref note, e.g. --notes=refs/notes/commit@{4.days.ago} upfront,
if the operation will write updated notes".

Re: [PATCH] notes: Use get_sha1_committish instead of read_ref in init_notes()

[Mike Hommey]

On Tue, Jun 16, 2015 at 08:17:03PM -0700, Junio C Hamano wrote:
> Mike Hommey <mh@glandium.org> writes:
> 
> > init_notes() is essentially the only point of entry to the notes API.
> > It is an arbitrary restriction that all it allows as input is a strict
> > ref name, when callers may want to give an arbitrary committish.
> 
> While it may be a good idea to allow reading from any note-shaped
> tree-ish, not just what is at the tip of a ref, I suspect that the
> use of read_ref() is not an arbitrary restriction, but is an
> effective way to achieve safety against callers that update notes.
> 
> That is, you can feed refs/notes/commit@{4.days.ago} to the
> machinery and show you notes from 4 days ago, but you cannot update
> that as if it were a ref.
> 
> Hence, if you are loosening the safety at init_notes() site, you
> would at least need to add a similar safety in the write codepath, I
> would think.  
> 
> One thing you would need to be careful about is that you would give
> users a crappy experience, if an operation reads notes, does its own
> thing, and then tries to write updated notes (think: rebase that
> transplants notes from original to rewritten commits), and you fail
> the operation only at the very last phase of updating.  In order to
> prevent that, "in the write codepath" above has to be "reject any
> non-ref note, e.g. --notes=refs/notes/commit@{4.days.ago} upfront,
> if the operation will write updated notes".

Looking around in the code, I'm not sure how to address this.
Considering the APIs, it would seem logical to have individual callers
care about filtering the refs themselves, on the other hand, there's
room for shooting oneself in the foot when adding new code paths needing
writes.

I'm tempted to make init_notes itself do the check, based on the value
it is given for a "read_only" argument. On the other hand, some commands
do their ref resolving themselves already. For example, notes_merge uses
read_ref_full and check_refname_format itself (not necessarily in the
right order, btw). OTOH, I'm not even sure it ends up using init_notes
at all.

Mike

Re: [PATCH] notes: Use get_sha1_committish instead of read_ref in init_notes()

[Junio C Hamano]

Mike Hommey <mh@glandium.org> writes:

> I'm tempted to make init_notes itself do the check, based on the value
> it is given for a "read_only" argument.

Yeah, that would be one sensible way to go after making sure that
everything goes thru this interface.

> On the other hand, some commands
> do their ref resolving themselves already.

Again, as long as they do not bypass the "read-only" safety you are
suggesting to add to init_notes(), that is OK.

Re: [PATCH] notes: Use get_sha1_committish instead of read_ref in init_notes()

[Johan Herland]

On Wed, Jun 17, 2015 at 5:18 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Mike Hommey <mh@glandium.org> writes:
> > I'm tempted to make init_notes itself do the check, based on the value
> > it is given for a "read_only" argument.
>
> Yeah, that would be one sensible way to go after making sure that
> everything goes thru this interface.

Agreed. Furthermore, consider adding the read_only flag (or however
you choose to encode it internally) to struct notes_tree, so that the
API functions that _manipulate_ notes trees can immediately bail out
when used on a read-only tree (i.e. we want them to fail as early as
possible).

> > On the other hand, some commands
> > do their ref resolving themselves already.
>
> Again, as long as they do not bypass the "read-only" safety you are
> suggesting to add to init_notes(), that is OK.

Agreed. An alternative to adding a simple read_only flag argument is
to modify the const char *notes_ref argument into two separate
arguments: const char *notes_treeish, and const char *update_ref,
where the latter should be NULL for read-only trees. That said,
currently the logic for actually updating notes ref lives outside the
notes.h API (see commit_notes() in notes-utils.h/c), so there might be
room for more consolidation/refactoring here...


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

[PATCH v2 - RFH] notes: Allow committish expressions as notes ref

[Mike Hommey]

init_notes() is the main point of entry to the notes API. It is an arbitrary
restriction that all it allows as input is a strict ref name, when callers
may want to give an arbitrary committish.

However, some operations that require updating the notes tree require a
strict ref name, because they wouldn't be able to update e.g. foo@{1}.

So we allow committish expressions to be used in the case the notes tree
is going to be used without write "permissions", and to distinguish whether
the notes tree is intended to be used for reads only, or will be updated,
a flag is added.

This has the side effect of enabling the use of committish as notes refs
in commands allowing them, e.g. git log --notes=foo@{1}.

Signed-off-by: Mike Hommey <mh@glandium.org>
---
The die() message in init_notes is left to be desired. But I don't have a
better idea at the moment.

Note that die()ing in an API is not exactly nice, but since init_notes was
already die()ing, I figured I wasn't making things worse than they already
are. Further refactoring feels like scope bloat.

Now, for the RFH, this actually doesn't entirely work, because *creating*
a new notes ref fails on that die() I'm adding. So the question is, how do
I distinguish between read_ref failing because the ref is not a proper ref,
and read_ref failing because the ref doesn't exist?

(I figured I'd ask instead of looking it up myself because it's the end of
the day here, and by the time I look at it again, there should be an answer
from someone who knows the code already :) )

 builtin/notes.c | 27 +++++++++++++++------------
 notes-cache.c   |  7 ++++---
 notes-utils.c   |  4 ++--
 notes.c         |  9 +++++++--
 notes.h         |  8 ++++++++
 5 files changed, 36 insertions(+), 19 deletions(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index 63f95fc..0712178 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -328,15 +328,18 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)
 	return ret;
 }
 
-static struct notes_tree *init_notes_check(const char *subcommand)
+static struct notes_tree *init_notes_check(const char *subcommand,
+					   int flags)
 {
 	struct notes_tree *t;
-	init_notes(NULL, NULL, NULL, 0);
+	const char *ref;
+	init_notes(NULL, NULL, NULL, flags);
 	t = &default_notes_tree;
 
-	if (!starts_with(t->ref, "refs/notes/"))
+	ref = (flags & NOTES_INIT_WRITABLE) ? t->update_ref : t->ref;
+	if (!starts_with(ref, "refs/notes/"))
 		die("Refusing to %s notes in %s (outside of refs/notes/)",
-		    subcommand, t->ref);
+		    subcommand, ref);
 	return t;
 }
 
@@ -359,7 +362,7 @@ static int list(int argc, const char **argv, const char *prefix)
 		usage_with_options(git_notes_list_usage, options);
 	}
 
-	t = init_notes_check("list");
+	t = init_notes_check("list", 0);
 	if (argc) {
 		if (get_sha1(argv[0], object))
 			die(_("Failed to resolve '%s' as a valid ref."), argv[0]);
@@ -419,7 +422,7 @@ static int add(int argc, const char **argv, const char *prefix)
 	if (get_sha1(object_ref, object))
 		die(_("Failed to resolve '%s' as a valid ref."), object_ref);
 
-	t = init_notes_check("add");
+	t = init_notes_check("add", NOTES_INIT_WRITABLE);
 	note = get_note(t, object);
 
 	if (note) {
@@ -510,7 +513,7 @@ static int copy(int argc, const char **argv, const char *prefix)
 	if (get_sha1(object_ref, object))
 		die(_("Failed to resolve '%s' as a valid ref."), object_ref);
 
-	t = init_notes_check("copy");
+	t = init_notes_check("copy", NOTES_INIT_WRITABLE);
 	note = get_note(t, object);
 
 	if (note) {
@@ -588,7 +591,7 @@ static int append_edit(int argc, const char **argv, const char *prefix)
 	if (get_sha1(object_ref, object))
 		die(_("Failed to resolve '%s' as a valid ref."), object_ref);
 
-	t = init_notes_check(argv[0]);
+	t = init_notes_check(argv[0], NOTES_INIT_WRITABLE);
 	note = get_note(t, object);
 
 	prepare_note_data(object, &d, edit ? note : NULL);
@@ -651,7 +654,7 @@ static int show(int argc, const char **argv, const char *prefix)
 	if (get_sha1(object_ref, object))
 		die(_("Failed to resolve '%s' as a valid ref."), object_ref);
 
-	t = init_notes_check("show");
+	t = init_notes_check("show", 0);
 	note = get_note(t, object);
 
 	if (!note)
@@ -812,7 +815,7 @@ static int merge(int argc, const char **argv, const char *prefix)
 		}
 	}
 
-	t = init_notes_check("merge");
+	t = init_notes_check("merge", NOTES_INIT_WRITABLE);
 
 	strbuf_addf(&msg, "notes: Merged notes from %s into %s",
 		    remote_ref.buf, default_notes_ref());
@@ -878,7 +881,7 @@ static int remove_cmd(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, options,
 			     git_notes_remove_usage, 0);
 
-	t = init_notes_check("remove");
+	t = init_notes_check("remove", NOTES_INIT_WRITABLE);
 
 	if (!argc && !from_stdin) {
 		retval = remove_one_note(t, "HEAD", flag);
@@ -920,7 +923,7 @@ static int prune(int argc, const char **argv, const char *prefix)
 		usage_with_options(git_notes_prune_usage, options);
 	}
 
-	t = init_notes_check("prune");
+	t = init_notes_check("prune", NOTES_INIT_WRITABLE);
 
 	prune_notes(t, (verbose ? NOTES_PRUNE_VERBOSE : 0) |
 		(show_only ? NOTES_PRUNE_VERBOSE|NOTES_PRUNE_DRYRUN : 0) );
diff --git a/notes-cache.c b/notes-cache.c
index c4e9bb7..3ee8d2c 100644
--- a/notes-cache.c
+++ b/notes-cache.c
@@ -49,7 +49,8 @@ int notes_cache_write(struct notes_cache *c)
 	unsigned char tree_sha1[20];
 	unsigned char commit_sha1[20];
 
-	if (!c || !c->tree.initialized || !c->tree.ref || !*c->tree.ref)
+	if (!c || !c->tree.initialized || !c->tree.update_ref ||
+	    !*c->tree.update_ref)
 		return -1;
 	if (!c->tree.dirty)
 		return 0;
@@ -59,8 +60,8 @@ int notes_cache_write(struct notes_cache *c)
 	if (commit_tree(c->validity, strlen(c->validity), tree_sha1, NULL,
 			commit_sha1, NULL, NULL) < 0)
 		return -1;
-	if (update_ref("update notes cache", c->tree.ref, commit_sha1, NULL,
-		       0, UPDATE_REFS_QUIET_ON_ERR) < 0)
+	if (update_ref("update notes cache", c->tree.update_ref, commit_sha1,
+		       NULL, 0, UPDATE_REFS_QUIET_ON_ERR) < 0)
 		return -1;
 
 	return 0;
diff --git a/notes-utils.c b/notes-utils.c
index ccbf073..937e1f0 100644
--- a/notes-utils.c
+++ b/notes-utils.c
@@ -37,7 +37,7 @@ void commit_notes(struct notes_tree *t, const char *msg)
 
 	if (!t)
 		t = &default_notes_tree;
-	if (!t->initialized || !t->ref || !*t->ref)
+	if (!t->initialized || !t->update_ref || !*t->update_ref)
 		die(_("Cannot commit uninitialized/unreferenced notes tree"));
 	if (!t->dirty)
 		return; /* don't have to commit an unchanged tree */
@@ -48,7 +48,7 @@ void commit_notes(struct notes_tree *t, const char *msg)
 
 	create_notes_commit(t, NULL, buf.buf, buf.len, commit_sha1);
 	strbuf_insert(&buf, 0, "notes: ", 7); /* commit message starts at index 7 */
-	update_ref(buf.buf, t->ref, commit_sha1, NULL, 0,
+	update_ref(buf.buf, t->update_ref, commit_sha1, NULL, 0,
 		   UPDATE_REFS_DIE_ON_ERR);
 
 	strbuf_release(&buf);
diff --git a/notes.c b/notes.c
index df08209..8c6019d 100644
--- a/notes.c
+++ b/notes.c
@@ -1007,12 +1007,17 @@ void init_notes(struct notes_tree *t, const char *notes_ref,
 	t->first_non_note = NULL;
 	t->prev_non_note = NULL;
 	t->ref = xstrdup_or_null(notes_ref);
+	t->update_ref = (flags & NOTES_INIT_WRITABLE) ? t->ref : NULL;
 	t->combine_notes = combine_notes;
 	t->initialized = 1;
 	t->dirty = 0;
 
-	if (flags & NOTES_INIT_EMPTY || !notes_ref ||
-	    read_ref(notes_ref, object_sha1))
+	if (flags & NOTES_INIT_EMPTY || !notes_ref)
+		return;
+	if (flags & NOTES_INIT_WRITABLE && read_ref(notes_ref, object_sha1))
+		die("Invalid ref %s", notes_ref);;
+	if (!(flags & NOTES_INIT_WRITABLE) &&
+	    get_sha1_committish(notes_ref, object_sha1))
 		return;
 	if (get_tree_entry(object_sha1, "", sha1, &mode))
 		die("Failed to read notes tree referenced by %s (%s)",
diff --git a/notes.h b/notes.h
index 2a3f923..9b2b96c 100644
--- a/notes.h
+++ b/notes.h
@@ -44,6 +44,7 @@ extern struct notes_tree {
 	struct int_node *root;
 	struct non_note *first_non_note, *prev_non_note;
 	char *ref;
+	char *update_ref;
 	combine_notes_fn combine_notes;
 	int initialized;
 	int dirty;
@@ -72,6 +73,13 @@ const char *default_notes_ref(void);
 #define NOTES_INIT_EMPTY 1
 
 /*
+ * By default, the notes tree is only readable, and the notes ref can be
+ * any committish. The notes tree can however be made writable with this
+ * flag, in which case only strict ref names can be used.
+ */
+#define NOTES_INIT_WRITABLE 2
+
+/*
  * Initialize the given notes_tree with the notes tree structure at the given
  * ref. If given ref is NULL, the value of the $GIT_NOTES_REF environment
  * variable is used, and if that is missing, the default notes ref is used
-- 
2.4.3.2.gf0a024e.dirty

[PATCH v3] notes: Allow committish expressions as notes ref

[Mike Hommey]

init_notes() is the main point of entry to the notes API. It is an arbitrary
restriction that all it allows as input is a strict ref name, when callers
may want to give an arbitrary committish.

However, some operations that require updating the notes tree require a
strict ref name, because they wouldn't be able to update e.g. foo@{1}.

So we allow committish expressions to be used in the case the notes tree
is going to be used without write "permissions", and to distinguish whether
the notes tree is intended to be used for reads only, or will be updated,
a flag is added.

This has the side effect of enabling the use of committish as notes refs
in commands allowing them, e.g. git log --notes=foo@{1}.

Signed-off-by: Mike Hommey <mh@glandium.org>
---
 builtin/notes.c | 29 ++++++++++++++++-------------
 notes-cache.c   | 11 ++++++-----
 notes-utils.c   |  6 +++---
 notes.c         | 11 +++++++----
 notes.h         | 10 +++++++++-
 5 files changed, 41 insertions(+), 26 deletions(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index 63f95fc..0fc6e7a 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -285,7 +285,7 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)
 		if (!c)
 			return 0;
 	} else {
-		init_notes(NULL, NULL, NULL, 0);
+		init_notes(NULL, NULL, NULL, NOTES_INIT_WRITABLE);
 		t = &default_notes_tree;
 	}
 
@@ -328,15 +328,18 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)
 	return ret;
 }
 
-static struct notes_tree *init_notes_check(const char *subcommand)
+static struct notes_tree *init_notes_check(const char *subcommand,
+					   int flags)
 {
 	struct notes_tree *t;
-	init_notes(NULL, NULL, NULL, 0);
+	const char *ref;
+	init_notes(NULL, NULL, NULL, flags);
 	t = &default_notes_tree;
 
-	if (!starts_with(t->ref, "refs/notes/"))
+	ref = (flags & NOTES_INIT_WRITABLE) ? t->update_ref : t->ref;
+	if (!starts_with(ref, "refs/notes/"))
 		die("Refusing to %s notes in %s (outside of refs/notes/)",
-		    subcommand, t->ref);
+		    subcommand, ref);
 	return t;
 }
 
@@ -359,7 +362,7 @@ static int list(int argc, const char **argv, const char *prefix)
 		usage_with_options(git_notes_list_usage, options);
 	}
 
-	t = init_notes_check("list");
+	t = init_notes_check("list", 0);
 	if (argc) {
 		if (get_sha1(argv[0], object))
 			die(_("Failed to resolve '%s' as a valid ref."), argv[0]);
@@ -419,7 +422,7 @@ static int add(int argc, const char **argv, const char *prefix)
 	if (get_sha1(object_ref, object))
 		die(_("Failed to resolve '%s' as a valid ref."), object_ref);
 
-	t = init_notes_check("add");
+	t = init_notes_check("add", NOTES_INIT_WRITABLE);
 	note = get_note(t, object);
 
 	if (note) {
@@ -510,7 +513,7 @@ static int copy(int argc, const char **argv, const char *prefix)
 	if (get_sha1(object_ref, object))
 		die(_("Failed to resolve '%s' as a valid ref."), object_ref);
 
-	t = init_notes_check("copy");
+	t = init_notes_check("copy", NOTES_INIT_WRITABLE);
 	note = get_note(t, object);
 
 	if (note) {
@@ -588,7 +591,7 @@ static int append_edit(int argc, const char **argv, const char *prefix)
 	if (get_sha1(object_ref, object))
 		die(_("Failed to resolve '%s' as a valid ref."), object_ref);
 
-	t = init_notes_check(argv[0]);
+	t = init_notes_check(argv[0], NOTES_INIT_WRITABLE);
 	note = get_note(t, object);
 
 	prepare_note_data(object, &d, edit ? note : NULL);
@@ -651,7 +654,7 @@ static int show(int argc, const char **argv, const char *prefix)
 	if (get_sha1(object_ref, object))
 		die(_("Failed to resolve '%s' as a valid ref."), object_ref);
 
-	t = init_notes_check("show");
+	t = init_notes_check("show", 0);
 	note = get_note(t, object);
 
 	if (!note)
@@ -812,7 +815,7 @@ static int merge(int argc, const char **argv, const char *prefix)
 		}
 	}
 
-	t = init_notes_check("merge");
+	t = init_notes_check("merge", NOTES_INIT_WRITABLE);
 
 	strbuf_addf(&msg, "notes: Merged notes from %s into %s",
 		    remote_ref.buf, default_notes_ref());
@@ -878,7 +881,7 @@ static int remove_cmd(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, options,
 			     git_notes_remove_usage, 0);
 
-	t = init_notes_check("remove");
+	t = init_notes_check("remove", NOTES_INIT_WRITABLE);
 
 	if (!argc && !from_stdin) {
 		retval = remove_one_note(t, "HEAD", flag);
@@ -920,7 +923,7 @@ static int prune(int argc, const char **argv, const char *prefix)
 		usage_with_options(git_notes_prune_usage, options);
 	}
 
-	t = init_notes_check("prune");
+	t = init_notes_check("prune", NOTES_INIT_WRITABLE);
 
 	prune_notes(t, (verbose ? NOTES_PRUNE_VERBOSE : 0) |
 		(show_only ? NOTES_PRUNE_VERBOSE|NOTES_PRUNE_DRYRUN : 0) );
diff --git a/notes-cache.c b/notes-cache.c
index c4e9bb7..5dfc5cb 100644
--- a/notes-cache.c
+++ b/notes-cache.c
@@ -32,14 +32,14 @@ void notes_cache_init(struct notes_cache *c, const char *name,
 		     const char *validity)
 {
 	struct strbuf ref = STRBUF_INIT;
-	int flags = 0;
+	int flags = NOTES_INIT_WRITABLE;
 
 	memset(c, 0, sizeof(*c));
 	c->validity = xstrdup(validity);
 
 	strbuf_addf(&ref, "refs/notes/%s", name);
 	if (!notes_cache_match_validity(ref.buf, validity))
-		flags = NOTES_INIT_EMPTY;
+		flags |= NOTES_INIT_EMPTY;
 	init_notes(&c->tree, ref.buf, combine_notes_overwrite, flags);
 	strbuf_release(&ref);
 }
@@ -49,7 +49,8 @@ int notes_cache_write(struct notes_cache *c)
 	unsigned char tree_sha1[20];
 	unsigned char commit_sha1[20];
 
-	if (!c || !c->tree.initialized || !c->tree.ref || !*c->tree.ref)
+	if (!c || !c->tree.initialized || !c->tree.update_ref ||
+	    !*c->tree.update_ref)
 		return -1;
 	if (!c->tree.dirty)
 		return 0;
@@ -59,8 +60,8 @@ int notes_cache_write(struct notes_cache *c)
 	if (commit_tree(c->validity, strlen(c->validity), tree_sha1, NULL,
 			commit_sha1, NULL, NULL) < 0)
 		return -1;
-	if (update_ref("update notes cache", c->tree.ref, commit_sha1, NULL,
-		       0, UPDATE_REFS_QUIET_ON_ERR) < 0)
+	if (update_ref("update notes cache", c->tree.update_ref, commit_sha1,
+		       NULL, 0, UPDATE_REFS_QUIET_ON_ERR) < 0)
 		return -1;
 
 	return 0;
diff --git a/notes-utils.c b/notes-utils.c
index ccbf073..f65edba 100644
--- a/notes-utils.c
+++ b/notes-utils.c
@@ -37,7 +37,7 @@ void commit_notes(struct notes_tree *t, const char *msg)
 
 	if (!t)
 		t = &default_notes_tree;
-	if (!t->initialized || !t->ref || !*t->ref)
+	if (!t->initialized || !t->update_ref || !*t->update_ref)
 		die(_("Cannot commit uninitialized/unreferenced notes tree"));
 	if (!t->dirty)
 		return; /* don't have to commit an unchanged tree */
@@ -48,7 +48,7 @@ void commit_notes(struct notes_tree *t, const char *msg)
 
 	create_notes_commit(t, NULL, buf.buf, buf.len, commit_sha1);
 	strbuf_insert(&buf, 0, "notes: ", 7); /* commit message starts at index 7 */
-	update_ref(buf.buf, t->ref, commit_sha1, NULL, 0,
+	update_ref(buf.buf, t->update_ref, commit_sha1, NULL, 0,
 		   UPDATE_REFS_DIE_ON_ERR);
 
 	strbuf_release(&buf);
@@ -130,7 +130,7 @@ struct notes_rewrite_cfg *init_copy_notes_for_rewrite(const char *cmd)
 		free(c);
 		return NULL;
 	}
-	c->trees = load_notes_trees(c->refs);
+	c->trees = load_notes_trees(c->refs, NOTES_INIT_WRITABLE);
 	string_list_clear(c->refs, 0);
 	free(c->refs);
 	return c;
diff --git a/notes.c b/notes.c
index df08209..84f8a47 100644
--- a/notes.c
+++ b/notes.c
@@ -1007,13 +1007,16 @@ void init_notes(struct notes_tree *t, const char *notes_ref,
 	t->first_non_note = NULL;
 	t->prev_non_note = NULL;
 	t->ref = xstrdup_or_null(notes_ref);
+	t->update_ref = (flags & NOTES_INIT_WRITABLE) ? t->ref : NULL;
 	t->combine_notes = combine_notes;
 	t->initialized = 1;
 	t->dirty = 0;
 
 	if (flags & NOTES_INIT_EMPTY || !notes_ref ||
-	    read_ref(notes_ref, object_sha1))
+	    get_sha1_committish(notes_ref, object_sha1))
 		return;
+	if (flags & NOTES_INIT_WRITABLE &&  read_ref(notes_ref, object_sha1))
+		die("Cannot use notes ref %s", notes_ref);
 	if (get_tree_entry(object_sha1, "", sha1, &mode))
 		die("Failed to read notes tree referenced by %s (%s)",
 		    notes_ref, sha1_to_hex(object_sha1));
@@ -1023,7 +1026,7 @@ void init_notes(struct notes_tree *t, const char *notes_ref,
 	load_subtree(t, &root_tree, t->root, 0);
 }
 
-struct notes_tree **load_notes_trees(struct string_list *refs)
+struct notes_tree **load_notes_trees(struct string_list *refs, int flags)
 {
 	struct string_list_item *item;
 	int counter = 0;
@@ -1031,7 +1034,7 @@ struct notes_tree **load_notes_trees(struct string_list *refs)
 	trees = xmalloc((refs->nr+1) * sizeof(struct notes_tree *));
 	for_each_string_list_item(item, refs) {
 		struct notes_tree *t = xcalloc(1, sizeof(struct notes_tree));
-		init_notes(t, item->string, combine_notes_ignore, 0);
+		init_notes(t, item->string, combine_notes_ignore, flags);
 		trees[counter++] = t;
 	}
 	trees[counter] = NULL;
@@ -1067,7 +1070,7 @@ void init_display_notes(struct display_notes_opt *opt)
 						     item->string);
 	}
 
-	display_notes_trees = load_notes_trees(&display_notes_refs);
+	display_notes_trees = load_notes_trees(&display_notes_refs, 0);
 	string_list_clear(&display_notes_refs, 0);
 }
 
diff --git a/notes.h b/notes.h
index 2a3f923..aa9960c 100644
--- a/notes.h
+++ b/notes.h
@@ -44,6 +44,7 @@ extern struct notes_tree {
 	struct int_node *root;
 	struct non_note *first_non_note, *prev_non_note;
 	char *ref;
+	char *update_ref;
 	combine_notes_fn combine_notes;
 	int initialized;
 	int dirty;
@@ -72,6 +73,13 @@ const char *default_notes_ref(void);
 #define NOTES_INIT_EMPTY 1
 
 /*
+ * By default, the notes tree is only readable, and the notes ref can be
+ * any committish. The notes tree can however be made writable with this
+ * flag, in which case only strict ref names can be used.
+ */
+#define NOTES_INIT_WRITABLE 2
+
+/*
  * Initialize the given notes_tree with the notes tree structure at the given
  * ref. If given ref is NULL, the value of the $GIT_NOTES_REF environment
  * variable is used, and if that is missing, the default notes ref is used
@@ -276,7 +284,7 @@ void format_display_notes(const unsigned char *object_sha1,
  * Load the notes tree from each ref listed in 'refs'.  The output is
  * an array of notes_tree*, terminated by a NULL.
  */
-struct notes_tree **load_notes_trees(struct string_list *refs);
+struct notes_tree **load_notes_trees(struct string_list *refs, int flags);
 
 /*
  * Add all refs that match 'glob' to the 'list'.
-- 
2.4.3.2.gf0a024e.dirty

Re: [PATCH v3] notes: Allow committish expressions as notes ref

[Johan Herland]

On Thu, Jul 9, 2015 at 4:48 AM, Mike Hommey <mh@glandium.org> wrote:
> init_notes() is the main point of entry to the notes API. It is an arbitrary
> restriction that all it allows as input is a strict ref name, when callers
> may want to give an arbitrary committish.
>
> However, some operations that require updating the notes tree require a
> strict ref name, because they wouldn't be able to update e.g. foo@{1}.
>
> So we allow committish expressions to be used in the case the notes tree
> is going to be used without write "permissions", and to distinguish whether
> the notes tree is intended to be used for reads only, or will be updated,
> a flag is added.
>
> This has the side effect of enabling the use of committish as notes refs
> in commands allowing them, e.g. git log --notes=foo@{1}.
>
> Signed-off-by: Mike Hommey <mh@glandium.org>

Reviewed-by: Johan Herland <johan@herland.net>

...modulo some comments below.

> ---
>  builtin/notes.c | 29 ++++++++++++++++-------------
>  notes-cache.c   | 11 ++++++-----
>  notes-utils.c   |  6 +++---
>  notes.c         | 11 +++++++----
>  notes.h         | 10 +++++++++-
>  5 files changed, 41 insertions(+), 26 deletions(-)
>
> diff --git a/builtin/notes.c b/builtin/notes.c
> index 63f95fc..0fc6e7a 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> @@ -285,7 +285,7 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)
>                 if (!c)
>                         return 0;
>         } else {
> -               init_notes(NULL, NULL, NULL, 0);
> +               init_notes(NULL, NULL, NULL, NOTES_INIT_WRITABLE);
>                 t = &default_notes_tree;
>         }
>
> @@ -328,15 +328,18 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)
>         return ret;
>  }
>
> -static struct notes_tree *init_notes_check(const char *subcommand)
> +static struct notes_tree *init_notes_check(const char *subcommand,
> +                                          int flags)
>  {
>         struct notes_tree *t;
> -       init_notes(NULL, NULL, NULL, 0);
> +       const char *ref;
> +       init_notes(NULL, NULL, NULL, flags);
>         t = &default_notes_tree;
>
> -       if (!starts_with(t->ref, "refs/notes/"))
> +       ref = (flags & NOTES_INIT_WRITABLE) ? t->update_ref : t->ref;

Hmm, AFAICS from the code in notes.c, when NOTES_INIT_WRITABLE is set,
then t->ref == t->update_ref, which means the above line is redundant,
and t->ref will work just as well. That said, it's also bad to depend on
that fact from here, as the notes code might at some point change to
make t->ref != t->update_ref (e.g. if we want to allow a notes tree
which reads from one ref, but writes to another), so I guess the current
code is good.

> +       if (!starts_with(ref, "refs/notes/"))
>                 die("Refusing to %s notes in %s (outside of refs/notes/)",
> -                   subcommand, t->ref);
> +                   subcommand, ref);
>         return t;
>  }
>
[...]
> diff --git a/notes.c b/notes.c
> index df08209..84f8a47 100644
> --- a/notes.c
> +++ b/notes.c
> @@ -1007,13 +1007,16 @@ void init_notes(struct notes_tree *t, const char *notes_ref,
>         t->first_non_note = NULL;
>         t->prev_non_note = NULL;
>         t->ref = xstrdup_or_null(notes_ref);
> +       t->update_ref = (flags & NOTES_INIT_WRITABLE) ? t->ref : NULL;

This is the only place in notes.c that references t->update_ref. Which
points to a latent design flaw in the notes API: struct notes_tree keeps
track of the notes ref (previously t->ref, but now also t->update_ref),
but it is actually never used by the core notes implementation (except
for display purposes in format_note() and an error message in
load_subtree()). I guess a better design would be to either

 (a) move the ref out of the API entirely, leaving notes.h/.c to only
     worry about reading and writing notes tree objects, and letting
     the caller deal with resolving and updatiung notes refs and
     commit objects.

 (b) move the ref handling _into_ the API, basically moving
     commit_notes() from notes-utils.h/.c into notes.h/.c

Of these, I believe (a) is better, escpecially if we also provide a
wrapper API (in notes-utils.h/.c?) around the inner/core API to deal
with the commit/ref details.

However, this is largely independent of your current effort, and
belongs in a different patch (series).

>         t->combine_notes = combine_notes;
>         t->initialized = 1;
>         t->dirty = 0;
>
>         if (flags & NOTES_INIT_EMPTY || !notes_ref ||
> -           read_ref(notes_ref, object_sha1))
> +           get_sha1_committish(notes_ref, object_sha1))

I believe this should be get_sha1_treeish() instead. The only place we use
the result (object_sha1) is when calling get_tree_entry() below, which
will peel to a tree object anyway, so there's no need to place the
stricter commitish requirement on the caller (in the read-only case).

As a bonus, you can also s/commitish/treeish/ in the commit subject line.

Also, I do not immediately remember whether we support notes refs that
point directly at tree objects (bypassing the commit object entirely),
but if we do, the get_sha1_committish() would break that use case...

>                 return;
> +       if (flags & NOTES_INIT_WRITABLE &&  read_ref(notes_ref, object_sha1))

Drop the extra space after &&.

[....]
> diff --git a/notes.h b/notes.h
> index 2a3f923..aa9960c 100644
> --- a/notes.h
> +++ b/notes.h
> @@ -44,6 +44,7 @@ extern struct notes_tree {
>         struct int_node *root;
>         struct non_note *first_non_note, *prev_non_note;
>         char *ref;
> +       char *update_ref;
>         combine_notes_fn combine_notes;
>         int initialized;
>         int dirty;
> @@ -72,6 +73,13 @@ const char *default_notes_ref(void);
>  #define NOTES_INIT_EMPTY 1
>
>  /*
> + * By default, the notes tree is only readable, and the notes ref can be
> + * any committish. The notes tree can however be made writable with this

s/commitish/treeish/


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

[PATCH v4] notes: Allow treeish expressions as notes ref

[Mike Hommey]

init_notes() is the main point of entry to the notes API. It is an arbitrary
restriction that all it allows as input is a strict ref name, when callers
may want to give an arbitrary treeish.

However, some operations that require updating the notes tree require a
strict ref name, because they wouldn't be able to update e.g. foo@{1}.

So we allow treeish expressions to be used in the case the notes tree is
going to be used without write "permissions", and to distinguish whether
the notes tree is intended to be used for reads only, or will be updated,
a flag is added.

This has the side effect of enabling the use of treeish as notes refs in
commands allowing them, e.g. git log --notes=foo@{1}.

Signed-off-by: Mike Hommey <mh@glandium.org>
Reviewed-by: Johan Herland <johan@herland.net>
---
 builtin/notes.c | 29 ++++++++++++++++-------------
 notes-cache.c   | 11 ++++++-----
 notes-utils.c   |  6 +++---
 notes.c         | 11 +++++++----
 notes.h         | 10 +++++++++-
 5 files changed, 41 insertions(+), 26 deletions(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index 63f95fc..0fc6e7a 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -285,7 +285,7 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)
 		if (!c)
 			return 0;
 	} else {
-		init_notes(NULL, NULL, NULL, 0);
+		init_notes(NULL, NULL, NULL, NOTES_INIT_WRITABLE);
 		t = &default_notes_tree;
 	}
 
@@ -328,15 +328,18 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)
 	return ret;
 }
 
-static struct notes_tree *init_notes_check(const char *subcommand)
+static struct notes_tree *init_notes_check(const char *subcommand,
+					   int flags)
 {
 	struct notes_tree *t;
-	init_notes(NULL, NULL, NULL, 0);
+	const char *ref;
+	init_notes(NULL, NULL, NULL, flags);
 	t = &default_notes_tree;
 
-	if (!starts_with(t->ref, "refs/notes/"))
+	ref = (flags & NOTES_INIT_WRITABLE) ? t->update_ref : t->ref;
+	if (!starts_with(ref, "refs/notes/"))
 		die("Refusing to %s notes in %s (outside of refs/notes/)",
-		    subcommand, t->ref);
+		    subcommand, ref);
 	return t;
 }
 
@@ -359,7 +362,7 @@ static int list(int argc, const char **argv, const char *prefix)
 		usage_with_options(git_notes_list_usage, options);
 	}
 
-	t = init_notes_check("list");
+	t = init_notes_check("list", 0);
 	if (argc) {
 		if (get_sha1(argv[0], object))
 			die(_("Failed to resolve '%s' as a valid ref."), argv[0]);
@@ -419,7 +422,7 @@ static int add(int argc, const char **argv, const char *prefix)
 	if (get_sha1(object_ref, object))
 		die(_("Failed to resolve '%s' as a valid ref."), object_ref);
 
-	t = init_notes_check("add");
+	t = init_notes_check("add", NOTES_INIT_WRITABLE);
 	note = get_note(t, object);
 
 	if (note) {
@@ -510,7 +513,7 @@ static int copy(int argc, const char **argv, const char *prefix)
 	if (get_sha1(object_ref, object))
 		die(_("Failed to resolve '%s' as a valid ref."), object_ref);
 
-	t = init_notes_check("copy");
+	t = init_notes_check("copy", NOTES_INIT_WRITABLE);
 	note = get_note(t, object);
 
 	if (note) {
@@ -588,7 +591,7 @@ static int append_edit(int argc, const char **argv, const char *prefix)
 	if (get_sha1(object_ref, object))
 		die(_("Failed to resolve '%s' as a valid ref."), object_ref);
 
-	t = init_notes_check(argv[0]);
+	t = init_notes_check(argv[0], NOTES_INIT_WRITABLE);
 	note = get_note(t, object);
 
 	prepare_note_data(object, &d, edit ? note : NULL);
@@ -651,7 +654,7 @@ static int show(int argc, const char **argv, const char *prefix)
 	if (get_sha1(object_ref, object))
 		die(_("Failed to resolve '%s' as a valid ref."), object_ref);
 
-	t = init_notes_check("show");
+	t = init_notes_check("show", 0);
 	note = get_note(t, object);
 
 	if (!note)
@@ -812,7 +815,7 @@ static int merge(int argc, const char **argv, const char *prefix)
 		}
 	}
 
-	t = init_notes_check("merge");
+	t = init_notes_check("merge", NOTES_INIT_WRITABLE);
 
 	strbuf_addf(&msg, "notes: Merged notes from %s into %s",
 		    remote_ref.buf, default_notes_ref());
@@ -878,7 +881,7 @@ static int remove_cmd(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, options,
 			     git_notes_remove_usage, 0);
 
-	t = init_notes_check("remove");
+	t = init_notes_check("remove", NOTES_INIT_WRITABLE);
 
 	if (!argc && !from_stdin) {
 		retval = remove_one_note(t, "HEAD", flag);
@@ -920,7 +923,7 @@ static int prune(int argc, const char **argv, const char *prefix)
 		usage_with_options(git_notes_prune_usage, options);
 	}
 
-	t = init_notes_check("prune");
+	t = init_notes_check("prune", NOTES_INIT_WRITABLE);
 
 	prune_notes(t, (verbose ? NOTES_PRUNE_VERBOSE : 0) |
 		(show_only ? NOTES_PRUNE_VERBOSE|NOTES_PRUNE_DRYRUN : 0) );
diff --git a/notes-cache.c b/notes-cache.c
index c4e9bb7..5dfc5cb 100644
--- a/notes-cache.c
+++ b/notes-cache.c
@@ -32,14 +32,14 @@ void notes_cache_init(struct notes_cache *c, const char *name,
 		     const char *validity)
 {
 	struct strbuf ref = STRBUF_INIT;
-	int flags = 0;
+	int flags = NOTES_INIT_WRITABLE;
 
 	memset(c, 0, sizeof(*c));
 	c->validity = xstrdup(validity);
 
 	strbuf_addf(&ref, "refs/notes/%s", name);
 	if (!notes_cache_match_validity(ref.buf, validity))
-		flags = NOTES_INIT_EMPTY;
+		flags |= NOTES_INIT_EMPTY;
 	init_notes(&c->tree, ref.buf, combine_notes_overwrite, flags);
 	strbuf_release(&ref);
 }
@@ -49,7 +49,8 @@ int notes_cache_write(struct notes_cache *c)
 	unsigned char tree_sha1[20];
 	unsigned char commit_sha1[20];
 
-	if (!c || !c->tree.initialized || !c->tree.ref || !*c->tree.ref)
+	if (!c || !c->tree.initialized || !c->tree.update_ref ||
+	    !*c->tree.update_ref)
 		return -1;
 	if (!c->tree.dirty)
 		return 0;
@@ -59,8 +60,8 @@ int notes_cache_write(struct notes_cache *c)
 	if (commit_tree(c->validity, strlen(c->validity), tree_sha1, NULL,
 			commit_sha1, NULL, NULL) < 0)
 		return -1;
-	if (update_ref("update notes cache", c->tree.ref, commit_sha1, NULL,
-		       0, UPDATE_REFS_QUIET_ON_ERR) < 0)
+	if (update_ref("update notes cache", c->tree.update_ref, commit_sha1,
+		       NULL, 0, UPDATE_REFS_QUIET_ON_ERR) < 0)
 		return -1;
 
 	return 0;
diff --git a/notes-utils.c b/notes-utils.c
index ccbf073..f65edba 100644
--- a/notes-utils.c
+++ b/notes-utils.c
@@ -37,7 +37,7 @@ void commit_notes(struct notes_tree *t, const char *msg)
 
 	if (!t)
 		t = &default_notes_tree;
-	if (!t->initialized || !t->ref || !*t->ref)
+	if (!t->initialized || !t->update_ref || !*t->update_ref)
 		die(_("Cannot commit uninitialized/unreferenced notes tree"));
 	if (!t->dirty)
 		return; /* don't have to commit an unchanged tree */
@@ -48,7 +48,7 @@ void commit_notes(struct notes_tree *t, const char *msg)
 
 	create_notes_commit(t, NULL, buf.buf, buf.len, commit_sha1);
 	strbuf_insert(&buf, 0, "notes: ", 7); /* commit message starts at index 7 */
-	update_ref(buf.buf, t->ref, commit_sha1, NULL, 0,
+	update_ref(buf.buf, t->update_ref, commit_sha1, NULL, 0,
 		   UPDATE_REFS_DIE_ON_ERR);
 
 	strbuf_release(&buf);
@@ -130,7 +130,7 @@ struct notes_rewrite_cfg *init_copy_notes_for_rewrite(const char *cmd)
 		free(c);
 		return NULL;
 	}
-	c->trees = load_notes_trees(c->refs);
+	c->trees = load_notes_trees(c->refs, NOTES_INIT_WRITABLE);
 	string_list_clear(c->refs, 0);
 	free(c->refs);
 	return c;
diff --git a/notes.c b/notes.c
index df08209..dc95ebb 100644
--- a/notes.c
+++ b/notes.c
@@ -1007,13 +1007,16 @@ void init_notes(struct notes_tree *t, const char *notes_ref,
 	t->first_non_note = NULL;
 	t->prev_non_note = NULL;
 	t->ref = xstrdup_or_null(notes_ref);
+	t->update_ref = (flags & NOTES_INIT_WRITABLE) ? t->ref : NULL;
 	t->combine_notes = combine_notes;
 	t->initialized = 1;
 	t->dirty = 0;
 
 	if (flags & NOTES_INIT_EMPTY || !notes_ref ||
-	    read_ref(notes_ref, object_sha1))
+	    get_sha1_treeish(notes_ref, object_sha1))
 		return;
+	if (flags & NOTES_INIT_WRITABLE && read_ref(notes_ref, object_sha1))
+		die("Cannot use notes ref %s", notes_ref);
 	if (get_tree_entry(object_sha1, "", sha1, &mode))
 		die("Failed to read notes tree referenced by %s (%s)",
 		    notes_ref, sha1_to_hex(object_sha1));
@@ -1023,7 +1026,7 @@ void init_notes(struct notes_tree *t, const char *notes_ref,
 	load_subtree(t, &root_tree, t->root, 0);
 }
 
-struct notes_tree **load_notes_trees(struct string_list *refs)
+struct notes_tree **load_notes_trees(struct string_list *refs, int flags)
 {
 	struct string_list_item *item;
 	int counter = 0;
@@ -1031,7 +1034,7 @@ struct notes_tree **load_notes_trees(struct string_list *refs)
 	trees = xmalloc((refs->nr+1) * sizeof(struct notes_tree *));
 	for_each_string_list_item(item, refs) {
 		struct notes_tree *t = xcalloc(1, sizeof(struct notes_tree));
-		init_notes(t, item->string, combine_notes_ignore, 0);
+		init_notes(t, item->string, combine_notes_ignore, flags);
 		trees[counter++] = t;
 	}
 	trees[counter] = NULL;
@@ -1067,7 +1070,7 @@ void init_display_notes(struct display_notes_opt *opt)
 						     item->string);
 	}
 
-	display_notes_trees = load_notes_trees(&display_notes_refs);
+	display_notes_trees = load_notes_trees(&display_notes_refs, 0);
 	string_list_clear(&display_notes_refs, 0);
 }
 
diff --git a/notes.h b/notes.h
index 2a3f923..e5d67fd 100644
--- a/notes.h
+++ b/notes.h
@@ -44,6 +44,7 @@ extern struct notes_tree {
 	struct int_node *root;
 	struct non_note *first_non_note, *prev_non_note;
 	char *ref;
+	char *update_ref;
 	combine_notes_fn combine_notes;
 	int initialized;
 	int dirty;
@@ -72,6 +73,13 @@ const char *default_notes_ref(void);
 #define NOTES_INIT_EMPTY 1
 
 /*
+ * By default, the notes tree is only readable, and the notes ref can be
+ * any treeish. The notes tree can however be made writable with this flag,
+ * in which case only strict ref names can be used.
+ */
+#define NOTES_INIT_WRITABLE 2
+
+/*
  * Initialize the given notes_tree with the notes tree structure at the given
  * ref. If given ref is NULL, the value of the $GIT_NOTES_REF environment
  * variable is used, and if that is missing, the default notes ref is used
@@ -276,7 +284,7 @@ void format_display_notes(const unsigned char *object_sha1,
  * Load the notes tree from each ref listed in 'refs'.  The output is
  * an array of notes_tree*, terminated by a NULL.
  */
-struct notes_tree **load_notes_trees(struct string_list *refs);
+struct notes_tree **load_notes_trees(struct string_list *refs, int flags);
 
 /*
  * Add all refs that match 'glob' to the 'list'.
-- 
2.4.3.3.g2dde64c

Re: [PATCH v4] notes: Allow treeish expressions as notes ref

[Johan Herland]

On Fri, Jul 10, 2015 at 3:28 AM, Mike Hommey <mh@glandium.org> wrote:
> init_notes() is the main point of entry to the notes API. It is an arbitrary
> restriction that all it allows as input is a strict ref name, when callers
> may want to give an arbitrary treeish.
>
> However, some operations that require updating the notes tree require a
> strict ref name, because they wouldn't be able to update e.g. foo@{1}.
>
> So we allow treeish expressions to be used in the case the notes tree is
> going to be used without write "permissions", and to distinguish whether
> the notes tree is intended to be used for reads only, or will be updated,
> a flag is added.
>
> This has the side effect of enabling the use of treeish as notes refs in
> commands allowing them, e.g. git log --notes=foo@{1}.

Looks good. However, on a second pass I noticed that the patch comes
with no tests. I'd like at least a couple of tests thrown in there to verify
correctness; e.g. reading notes from refs/notes/commits^{tree} shall
succeed, and trying to write notes to refs/notes/commits^{tree} shall fail.


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCH v4] notes: Allow treeish expressions as notes ref

[Mike Hommey]

On Fri, Jul 10, 2015 at 09:16:16AM +0200, Johan Herland wrote:
> On Fri, Jul 10, 2015 at 3:28 AM, Mike Hommey <mh@glandium.org> wrote:
> > init_notes() is the main point of entry to the notes API. It is an arbitrary
> > restriction that all it allows as input is a strict ref name, when callers
> > may want to give an arbitrary treeish.
> >
> > However, some operations that require updating the notes tree require a
> > strict ref name, because they wouldn't be able to update e.g. foo@{1}.
> >
> > So we allow treeish expressions to be used in the case the notes tree is
> > going to be used without write "permissions", and to distinguish whether
> > the notes tree is intended to be used for reads only, or will be updated,
> > a flag is added.
> >
> > This has the side effect of enabling the use of treeish as notes refs in
> > commands allowing them, e.g. git log --notes=foo@{1}.
> 
> Looks good. However, on a second pass I noticed that the patch comes
> with no tests. I'd like at least a couple of tests thrown in there to verify
> correctness; e.g. reading notes from refs/notes/commits^{tree} shall
> succeed, and trying to write notes to refs/notes/commits^{tree} shall fail.

Fair enough. Will update.

Mike

[PATCH] notes: Allow treeish expressions as notes ref

[Mike Hommey]

init_notes() is the main point of entry to the notes API. It is an arbitrary
restriction that all it allows as input is a strict ref name, when callers
may want to give an arbitrary treeish.

However, some operations that require updating the notes tree require a
strict ref name, because they wouldn't be able to update e.g. foo@{1}.

So we allow treeish expressions to be used in the case the notes tree is
going to be used without write "permissions", and to distinguish whether
the notes tree is intended to be used for reads only, or will be updated,
a flag is added.

This has the side effect of enabling the use of treeish as notes refs in
commands allowing them, e.g. git log --notes=foo@{1}.

Signed-off-by: Mike Hommey <mh@glandium.org>
---
 builtin/notes.c  | 29 ++++++++++++++++-------------
 notes-cache.c    | 11 ++++++-----
 notes-utils.c    |  6 +++---
 notes.c          | 11 +++++++----
 notes.h          | 10 +++++++++-
 t/t3301-notes.sh | 10 ++++++++++
 6 files changed, 51 insertions(+), 26 deletions(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index 63f95fc..0fc6e7a 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -285,7 +285,7 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)
 		if (!c)
 			return 0;
 	} else {
-		init_notes(NULL, NULL, NULL, 0);
+		init_notes(NULL, NULL, NULL, NOTES_INIT_WRITABLE);
 		t = &default_notes_tree;
 	}
 
@@ -328,15 +328,18 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)
 	return ret;
 }
 
-static struct notes_tree *init_notes_check(const char *subcommand)
+static struct notes_tree *init_notes_check(const char *subcommand,
+					   int flags)
 {
 	struct notes_tree *t;
-	init_notes(NULL, NULL, NULL, 0);
+	const char *ref;
+	init_notes(NULL, NULL, NULL, flags);
 	t = &default_notes_tree;
 
-	if (!starts_with(t->ref, "refs/notes/"))
+	ref = (flags & NOTES_INIT_WRITABLE) ? t->update_ref : t->ref;
+	if (!starts_with(ref, "refs/notes/"))
 		die("Refusing to %s notes in %s (outside of refs/notes/)",
-		    subcommand, t->ref);
+		    subcommand, ref);
 	return t;
 }
 
@@ -359,7 +362,7 @@ static int list(int argc, const char **argv, const char *prefix)
 		usage_with_options(git_notes_list_usage, options);
 	}
 
-	t = init_notes_check("list");
+	t = init_notes_check("list", 0);
 	if (argc) {
 		if (get_sha1(argv[0], object))
 			die(_("Failed to resolve '%s' as a valid ref."), argv[0]);
@@ -419,7 +422,7 @@ static int add(int argc, const char **argv, const char *prefix)
 	if (get_sha1(object_ref, object))
 		die(_("Failed to resolve '%s' as a valid ref."), object_ref);
 
-	t = init_notes_check("add");
+	t = init_notes_check("add", NOTES_INIT_WRITABLE);
 	note = get_note(t, object);
 
 	if (note) {
@@ -510,7 +513,7 @@ static int copy(int argc, const char **argv, const char *prefix)
 	if (get_sha1(object_ref, object))
 		die(_("Failed to resolve '%s' as a valid ref."), object_ref);
 
-	t = init_notes_check("copy");
+	t = init_notes_check("copy", NOTES_INIT_WRITABLE);
 	note = get_note(t, object);
 
 	if (note) {
@@ -588,7 +591,7 @@ static int append_edit(int argc, const char **argv, const char *prefix)
 	if (get_sha1(object_ref, object))
 		die(_("Failed to resolve '%s' as a valid ref."), object_ref);
 
-	t = init_notes_check(argv[0]);
+	t = init_notes_check(argv[0], NOTES_INIT_WRITABLE);
 	note = get_note(t, object);
 
 	prepare_note_data(object, &d, edit ? note : NULL);
@@ -651,7 +654,7 @@ static int show(int argc, const char **argv, const char *prefix)
 	if (get_sha1(object_ref, object))
 		die(_("Failed to resolve '%s' as a valid ref."), object_ref);
 
-	t = init_notes_check("show");
+	t = init_notes_check("show", 0);
 	note = get_note(t, object);
 
 	if (!note)
@@ -812,7 +815,7 @@ static int merge(int argc, const char **argv, const char *prefix)
 		}
 	}
 
-	t = init_notes_check("merge");
+	t = init_notes_check("merge", NOTES_INIT_WRITABLE);
 
 	strbuf_addf(&msg, "notes: Merged notes from %s into %s",
 		    remote_ref.buf, default_notes_ref());
@@ -878,7 +881,7 @@ static int remove_cmd(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, options,
 			     git_notes_remove_usage, 0);
 
-	t = init_notes_check("remove");
+	t = init_notes_check("remove", NOTES_INIT_WRITABLE);
 
 	if (!argc && !from_stdin) {
 		retval = remove_one_note(t, "HEAD", flag);
@@ -920,7 +923,7 @@ static int prune(int argc, const char **argv, const char *prefix)
 		usage_with_options(git_notes_prune_usage, options);
 	}
 
-	t = init_notes_check("prune");
+	t = init_notes_check("prune", NOTES_INIT_WRITABLE);
 
 	prune_notes(t, (verbose ? NOTES_PRUNE_VERBOSE : 0) |
 		(show_only ? NOTES_PRUNE_VERBOSE|NOTES_PRUNE_DRYRUN : 0) );
diff --git a/notes-cache.c b/notes-cache.c
index c4e9bb7..5dfc5cb 100644
--- a/notes-cache.c
+++ b/notes-cache.c
@@ -32,14 +32,14 @@ void notes_cache_init(struct notes_cache *c, const char *name,
 		     const char *validity)
 {
 	struct strbuf ref = STRBUF_INIT;
-	int flags = 0;
+	int flags = NOTES_INIT_WRITABLE;
 
 	memset(c, 0, sizeof(*c));
 	c->validity = xstrdup(validity);
 
 	strbuf_addf(&ref, "refs/notes/%s", name);
 	if (!notes_cache_match_validity(ref.buf, validity))
-		flags = NOTES_INIT_EMPTY;
+		flags |= NOTES_INIT_EMPTY;
 	init_notes(&c->tree, ref.buf, combine_notes_overwrite, flags);
 	strbuf_release(&ref);
 }
@@ -49,7 +49,8 @@ int notes_cache_write(struct notes_cache *c)
 	unsigned char tree_sha1[20];
 	unsigned char commit_sha1[20];
 
-	if (!c || !c->tree.initialized || !c->tree.ref || !*c->tree.ref)
+	if (!c || !c->tree.initialized || !c->tree.update_ref ||
+	    !*c->tree.update_ref)
 		return -1;
 	if (!c->tree.dirty)
 		return 0;
@@ -59,8 +60,8 @@ int notes_cache_write(struct notes_cache *c)
 	if (commit_tree(c->validity, strlen(c->validity), tree_sha1, NULL,
 			commit_sha1, NULL, NULL) < 0)
 		return -1;
-	if (update_ref("update notes cache", c->tree.ref, commit_sha1, NULL,
-		       0, UPDATE_REFS_QUIET_ON_ERR) < 0)
+	if (update_ref("update notes cache", c->tree.update_ref, commit_sha1,
+		       NULL, 0, UPDATE_REFS_QUIET_ON_ERR) < 0)
 		return -1;
 
 	return 0;
diff --git a/notes-utils.c b/notes-utils.c
index ccbf073..f65edba 100644
--- a/notes-utils.c
+++ b/notes-utils.c
@@ -37,7 +37,7 @@ void commit_notes(struct notes_tree *t, const char *msg)
 
 	if (!t)
 		t = &default_notes_tree;
-	if (!t->initialized || !t->ref || !*t->ref)
+	if (!t->initialized || !t->update_ref || !*t->update_ref)
 		die(_("Cannot commit uninitialized/unreferenced notes tree"));
 	if (!t->dirty)
 		return; /* don't have to commit an unchanged tree */
@@ -48,7 +48,7 @@ void commit_notes(struct notes_tree *t, const char *msg)
 
 	create_notes_commit(t, NULL, buf.buf, buf.len, commit_sha1);
 	strbuf_insert(&buf, 0, "notes: ", 7); /* commit message starts at index 7 */
-	update_ref(buf.buf, t->ref, commit_sha1, NULL, 0,
+	update_ref(buf.buf, t->update_ref, commit_sha1, NULL, 0,
 		   UPDATE_REFS_DIE_ON_ERR);
 
 	strbuf_release(&buf);
@@ -130,7 +130,7 @@ struct notes_rewrite_cfg *init_copy_notes_for_rewrite(const char *cmd)
 		free(c);
 		return NULL;
 	}
-	c->trees = load_notes_trees(c->refs);
+	c->trees = load_notes_trees(c->refs, NOTES_INIT_WRITABLE);
 	string_list_clear(c->refs, 0);
 	free(c->refs);
 	return c;
diff --git a/notes.c b/notes.c
index 2be4d7f..25ed506 100644
--- a/notes.c
+++ b/notes.c
@@ -1007,13 +1007,16 @@ void init_notes(struct notes_tree *t, const char *notes_ref,
 	t->first_non_note = NULL;
 	t->prev_non_note = NULL;
 	t->ref = xstrdup_or_null(notes_ref);
+	t->update_ref = (flags & NOTES_INIT_WRITABLE) ? t->ref : NULL;
 	t->combine_notes = combine_notes;
 	t->initialized = 1;
 	t->dirty = 0;
 
 	if (flags & NOTES_INIT_EMPTY || !notes_ref ||
-	    read_ref(notes_ref, object_sha1))
+	    get_sha1_treeish(notes_ref, object_sha1))
 		return;
+	if (flags & NOTES_INIT_WRITABLE && read_ref(notes_ref, object_sha1))
+		die("Cannot use notes ref %s", notes_ref);
 	if (get_tree_entry(object_sha1, "", sha1, &mode))
 		die("Failed to read notes tree referenced by %s (%s)",
 		    notes_ref, sha1_to_hex(object_sha1));
@@ -1023,7 +1026,7 @@ void init_notes(struct notes_tree *t, const char *notes_ref,
 	load_subtree(t, &root_tree, t->root, 0);
 }
 
-struct notes_tree **load_notes_trees(struct string_list *refs)
+struct notes_tree **load_notes_trees(struct string_list *refs, int flags)
 {
 	struct string_list_item *item;
 	int counter = 0;
@@ -1031,7 +1034,7 @@ struct notes_tree **load_notes_trees(struct string_list *refs)
 	trees = xmalloc((refs->nr+1) * sizeof(struct notes_tree *));
 	for_each_string_list_item(item, refs) {
 		struct notes_tree *t = xcalloc(1, sizeof(struct notes_tree));
-		init_notes(t, item->string, combine_notes_ignore, 0);
+		init_notes(t, item->string, combine_notes_ignore, flags);
 		trees[counter++] = t;
 	}
 	trees[counter] = NULL;
@@ -1067,7 +1070,7 @@ void init_display_notes(struct display_notes_opt *opt)
 						     item->string);
 	}
 
-	display_notes_trees = load_notes_trees(&display_notes_refs);
+	display_notes_trees = load_notes_trees(&display_notes_refs, 0);
 	string_list_clear(&display_notes_refs, 0);
 }
 
diff --git a/notes.h b/notes.h
index 2a3f923..e5d67fd 100644
--- a/notes.h
+++ b/notes.h
@@ -44,6 +44,7 @@ extern struct notes_tree {
 	struct int_node *root;
 	struct non_note *first_non_note, *prev_non_note;
 	char *ref;
+	char *update_ref;
 	combine_notes_fn combine_notes;
 	int initialized;
 	int dirty;
@@ -72,6 +73,13 @@ const char *default_notes_ref(void);
 #define NOTES_INIT_EMPTY 1
 
 /*
+ * By default, the notes tree is only readable, and the notes ref can be
+ * any treeish. The notes tree can however be made writable with this flag,
+ * in which case only strict ref names can be used.
+ */
+#define NOTES_INIT_WRITABLE 2
+
+/*
  * Initialize the given notes_tree with the notes tree structure at the given
  * ref. If given ref is NULL, the value of the $GIT_NOTES_REF environment
  * variable is used, and if that is missing, the default notes ref is used
@@ -276,7 +284,7 @@ void format_display_notes(const unsigned char *object_sha1,
  * Load the notes tree from each ref listed in 'refs'.  The output is
  * an array of notes_tree*, terminated by a NULL.
  */
-struct notes_tree **load_notes_trees(struct string_list *refs);
+struct notes_tree **load_notes_trees(struct string_list *refs, int flags);
 
 /*
  * Add all refs that match 'glob' to the 'list'.
diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index 8cffd35..06b4584 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh
@@ -83,6 +83,16 @@ test_expect_success 'edit existing notes' '
 	test_must_fail git notes show HEAD^
 '
 
+test_expect_success 'show notes from treeish' '
+	test "b3" = "$(git notes --ref commits^{tree} show)" &&
+	test "b4" = "$(git notes --ref commits@{1} show)"
+'
+
+test_expect_success 'cannot edit notes from non-ref' '
+	test_must_fail git notes --ref commits^{tree} edit &&
+	test_must_fail git notes --ref commits@{1} edit
+'
+
 test_expect_success 'cannot "git notes add -m" where notes already exists' '
 	test_must_fail git notes add -m "b2" &&
 	test_path_is_missing .git/NOTES_EDITMSG &&
-- 
2.4.3.3.g2dde64c.dirty

Re: [PATCH] notes: Allow treeish expressions as notes ref

[Junio C Hamano]

Mike Hommey <mh@glandium.org> writes:

> init_notes() is the main point of entry to the notes API. It is an arbitrary
> restriction that all it allows as input is a strict ref name, when callers
> may want to give an arbitrary treeish.
>
> However, some operations that require updating the notes tree require a
> strict ref name, because they wouldn't be able to update e.g. foo@{1}.
>
> So we allow treeish expressions to be used in the case the notes tree is
> going to be used without write "permissions", and to distinguish whether
> the notes tree is intended to be used for reads only, or will be updated,
> a flag is added.

It is unfair to call the current check arbitrary, though.  From the
point of view of the person who views notes as a database, it is
entirely a sensible thing to treat it as an read/write data store.

> This has the side effect of enabling the use of treeish as notes refs in
> commands allowing them, e.g. git log --notes=foo@{1}.

I do not think it is a "side effect".  It's the primary benefit this
change brings in.

I'd flip the attitude around and sell this as "an enhancement",
perhaps like this:

    notes: allow treeish expressions as notes ref
    
    init_notes() is the main point of entry to the notes API. It ensures
    that the input can be used as ref, because it needs a ref to update
    to store notes tree after modifying it.
    
    There however are many use cases where notes tree is only read, e.g.
    "git log --notes=...".  Any notes-shaped treeish could be used for
    such purpose, but it is not allowed due to existing restriction.
    
    Allow treeish expressions to be used in the case the notes tree is
    going to be used without write "permissions".  Add a flag to
    distinguish whether the notes tree is intended to be used read-only,
    or will be updated.
    
    With this change, operations that use notes read-only can be fed any
    notes-shaped tree-ish can be used, e.g. git log --notes=notes@{1}.
    
    Signed-off-by: Mike Hommey <mh@glandium.org>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>


One thing I found puzzling is that the "arbitrary restriction" does
not seem to do anything visibly "restricting".

I taught my copy of "git am" to add the message-ID of the original
patch to the resulting commit, and that is kept in "amlog" notes.

Without this patch, I am getting this:

    $ git log --notes=amlog -1 --pretty=short
    commit 7a3744f95d4d21b27470e5bacaed8c434150ecac
    Author: Mike Hommey <mh@glandium.org>

        notes: allow treeish expressions as notes ref

    Notes (amlog):
        Message-Id: <1436517551-12172-1-git-send-email-mh@glandium.org>

The above is very much expected.  Now without your patch, I get
this:

    $ git log --notes='amlog@{1}' -1 --pretty=short
    commit 7a3744f95d4d21b27470e5bacaed8c434150ecac
    Author: Mike Hommey <mh@glandium.org>

        notes: allow treeish expressions as notes ref

It seems to be ignoring what is not a refname without complaining.
However, that does not seem to be the whole story.  Look:

    $ git log --notes='amlog@{1' -1 --pretty=short
    warning: notes ref refs/notes/amlog@{1 is invalid
    commit 7a3744f95d4d21b27470e5bacaed8c434150ecac
    Author: Mike Hommey <mh@glandium.org>

        notes: allow treeish expressions as notes ref

    $ git log --notes='amlog@{5000}' -1 --pretty=short
    fatal: Log for 'refs/notes/amlog' only has 3072 entries.

So sometimes we silently ignore, sometimes we warn, and sometimes
we refuse to work.  The above are all without your patch applied.

With your patch, the only change I see is this:

    $ git log --notes=amlog@{0} -1 --pretty=short
    commit 7a3744f95d4d21b27470e5bacaed8c434150ecac
    Author: Mike Hommey <mh@glandium.org>

        notes: allow treeish expressions as notes ref

    Notes (amlog@{0}):
        Message-Id: <1436517551-12172-1-git-send-email-mh@glandium.org>

We read from a tree-ish ;-) Whee!

What is interesting is to think what should happen when amlog@{1}
is given.  The version with your patch gives the same output as we
saw earlier, because amlog@{1} tree exists, but does not know about
your patch (yet), so does not add "Notes" section, which makes sense
by itself.

But we cannot tell if amlog@{1} was somehow malformed or it was OK
and there was no notes on the commit.

We probably should do a few more things:

 - Make sure that we show "there is no such tree-ish, no way to look
   up any note to any commit from there" and "I understood the tree
   you gave me, but there is no note for that commit" differently.

 - Decide if we want to "fail" the operation when the notes tree
   given by the user is not even a tree-ish or just "warn" and keep
   going.  And do so consistently.

Thanks.

Re: [PATCH] notes: Allow treeish expressions as notes ref

[Mike Hommey]

On Mon, Jul 13, 2015 at 09:35:40AM -0700, Junio C Hamano wrote:
> Mike Hommey <mh@glandium.org> writes:
> 
> > init_notes() is the main point of entry to the notes API. It is an arbitrary
> > restriction that all it allows as input is a strict ref name, when callers
> > may want to give an arbitrary treeish.
> >
> > However, some operations that require updating the notes tree require a
> > strict ref name, because they wouldn't be able to update e.g. foo@{1}.
> >
> > So we allow treeish expressions to be used in the case the notes tree is
> > going to be used without write "permissions", and to distinguish whether
> > the notes tree is intended to be used for reads only, or will be updated,
> > a flag is added.
> 
> It is unfair to call the current check arbitrary, though.  From the
> point of view of the person who views notes as a database, it is
> entirely a sensible thing to treat it as an read/write data store.
> 
> > This has the side effect of enabling the use of treeish as notes refs in
> > commands allowing them, e.g. git log --notes=foo@{1}.
> 
> I do not think it is a "side effect".  It's the primary benefit this
> change brings in.

My original motivation for this change was for init_notes() to take a
committish so that I can feed it with one in a C program that uses the
notes API. So from that perspective, that command lines can now use them
is a side effect.

> I'd flip the attitude around and sell this as "an enhancement",
> perhaps like this:
> 
>     notes: allow treeish expressions as notes ref
>     
>     init_notes() is the main point of entry to the notes API. It ensures
>     that the input can be used as ref, because it needs a ref to update
>     to store notes tree after modifying it.
>     
>     There however are many use cases where notes tree is only read, e.g.
>     "git log --notes=...".  Any notes-shaped treeish could be used for
>     such purpose, but it is not allowed due to existing restriction.
>     
>     Allow treeish expressions to be used in the case the notes tree is
>     going to be used without write "permissions".  Add a flag to
>     distinguish whether the notes tree is intended to be used read-only,
>     or will be updated.
>     
>     With this change, operations that use notes read-only can be fed any
>     notes-shaped tree-ish can be used, e.g. git log --notes=notes@{1}.
>     
>     Signed-off-by: Mike Hommey <mh@glandium.org>
>     Signed-off-by: Junio C Hamano <gitster@pobox.com>

WFM.

> We probably should do a few more things:
> 
>  - Make sure that we show "there is no such tree-ish, no way to look
>    up any note to any commit from there" and "I understood the tree
>    you gave me, but there is no note for that commit" differently.

How would you reconcile that with the usual "there are only a couple
commits with a note in the hundreds you make me display"?

>  - Decide if we want to "fail" the operation when the notes tree
>    given by the user is not even a tree-ish or just "warn" and keep
>    going.  And do so consistently.

Is this something you want to be figured before merging this patch?

Mike

Re: [PATCH] notes: Allow treeish expressions as notes ref

[Junio C Hamano]

Mike Hommey <mh@glandium.org> writes:

>>  - Make sure that we show "there is no such tree-ish, no way to look
>>    up any note to any commit from there" and "I understood the tree
>>    you gave me, but there is no note for that commit" differently.
>
> How would you reconcile that with the usual "there are only a couple
> commits with a note in the hundreds you make me display"?

I am talking about the difference between "a tree exists (which may
lack notes for a given commit)" and "a tree does not even exist in
the first place".  This patch removed "a tree exists but that is not
a ref so we silently ignore", but I do not know if that change alone
covers everything---do you?

>>  - Decide if we want to "fail" the operation when the notes tree
>>    given by the user is not even a tree-ish or just "warn" and keep
>>    going.  And do so consistently.
>
> Is this something you want to be figured before merging this patch?

Depends on the definition of 'merging'.  I queued this one on 'pu',
and have no intention to merge it down to 'master' by the end of
this month; in the meantime either incremental or replacement
refinement can certainly address that inconsistency I'd hope ;-)

Thanks.

Re: [PATCH] notes: Allow treeish expressions as notes ref

[Mike Hommey]

On Mon, Jul 13, 2015 at 09:35:40AM -0700, Junio C Hamano wrote:
> What is interesting is to think what should happen when amlog@{1}
> is given.  The version with your patch gives the same output as we
> saw earlier, because amlog@{1} tree exists, but does not know about
> your patch (yet), so does not add "Notes" section, which makes sense
> by itself.
> 
> But we cannot tell if amlog@{1} was somehow malformed or it was OK
> and there was no notes on the commit.
> 
> We probably should do a few more things:
> 
>  - Make sure that we show "there is no such tree-ish, no way to look
>    up any note to any commit from there" and "I understood the tree
>    you gave me, but there is no note for that commit" differently.

After refreshing the patch against current "next", it appears that
there is such a distinction:

$ ./git-log --notes=fdsfgsfdg HEAD^! --pretty=short
warning: notes ref refs/notes/fdsfgsfdg is invalid
commit e5b68b2e879608d881c2e3600ce84962fcdefc88
Author: Mike Hommey <mh@glandium.org>

    notes: allow treeish expressions as notes ref

$ ./git-log --notes=foo HEAD^! --pretty=short
commit e5b68b2e879608d881c2e3600ce84962fcdefc88
Author: Mike Hommey <mh@glandium.org>

    notes: allow treeish expressions as notes ref

(I do have a refs/notes/foo ref)

>  - Decide if we want to "fail" the operation when the notes tree
>    given by the user is not even a tree-ish or just "warn" and keep
>    going.  And do so consistently.

Currently, it warns and doesn't fail.

Cheers,

Mike

[PATCH v6] notes: allow treeish expressions as notes ref

[Mike Hommey]

init_notes() is the main point of entry to the notes API. It ensures
that the input can be used as ref, because it needs a ref to update to
store notes tree after modifying it.

There however are many use cases where notes tree is only read, e.g.
"git log --notes=...".  Any notes-shaped treeish could be used for such
purpose, but it is not allowed due to existing restriction.

Allow treeish expressions to be used in the case the notes tree is going
to be used without write "permissions".  Add a flag to distinguish
whether the notes tree is intended to be used read-only, or will be
updated.

With this change, operations that use notes read-only can be fed any
notes-shaped tree-ish can be used, e.g. git log --notes=notes@{1}.

Signed-off-by: Mike Hommey <mh@glandium.org>
---

I'm really not satisfied with the documentation change, but I don't have
a better idea :(. It didn't feel like Documentation/git-notes.txt needed
an update.

 Documentation/pretty-options.txt |  8 ++++----
 builtin/notes.c                  | 29 ++++++++++++++++-------------
 notes-cache.c                    | 11 ++++++-----
 notes-utils.c                    |  6 +++---
 notes.c                          | 11 +++++++----
 notes.h                          | 10 +++++++++-
 t/t3301-notes.sh                 | 10 ++++++++++
 7 files changed, 55 insertions(+), 30 deletions(-)

diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt
index 4b659ac..54b88b6 100644
--- a/Documentation/pretty-options.txt
+++ b/Documentation/pretty-options.txt
@@ -43,7 +43,7 @@ people using 80-column terminals.
 	commit may be copied to the output.
 
 ifndef::git-rev-list[]
---notes[=<ref>]::
+--notes[=<treeish>]::
 	Show the notes (see linkgit:git-notes[1]) that annotate the
 	commit, when showing the commit log message.  This is the default
 	for `git log`, `git show` and `git whatchanged` commands when
@@ -54,8 +54,8 @@ By default, the notes shown are from the notes refs listed in the
 'core.notesRef' and 'notes.displayRef' variables (or corresponding
 environment overrides). See linkgit:git-config[1] for more details.
 +
-With an optional '<ref>' argument, show this notes ref instead of the
-default notes ref(s). The ref specifies the full refname when it begins
+With an optional '<treeish>' argument, use the treeish to find the notes
+to display.  The treeish can specify the full refname when it begins
 with `refs/notes/`; when it begins with `notes/`, `refs/` and otherwise
 `refs/notes/` is prefixed to form a full name of the ref.
 +
@@ -71,7 +71,7 @@ being displayed. Examples: "--notes=foo" will show only notes from
 	"--notes --notes=foo --no-notes --notes=bar" will only show notes
 	from "refs/notes/bar".
 
---show-notes[=<ref>]::
+--show-notes[=<treeish>]::
 --[no-]standard-notes::
 	These options are deprecated. Use the above --notes/--no-notes
 	options instead.
diff --git a/builtin/notes.c b/builtin/notes.c
index 3608c64..6371d53 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -286,7 +286,7 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)
 		if (!c)
 			return 0;
 	} else {
-		init_notes(NULL, NULL, NULL, 0);
+		init_notes(NULL, NULL, NULL, NOTES_INIT_WRITABLE);
 		t = &default_notes_tree;
 	}
 
@@ -329,15 +329,18 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)
 	return ret;
 }
 
-static struct notes_tree *init_notes_check(const char *subcommand)
+static struct notes_tree *init_notes_check(const char *subcommand,
+					   int flags)
 {
 	struct notes_tree *t;
-	init_notes(NULL, NULL, NULL, 0);
+	const char *ref;
+	init_notes(NULL, NULL, NULL, flags);
 	t = &default_notes_tree;
 
-	if (!starts_with(t->ref, "refs/notes/"))
+	ref = (flags & NOTES_INIT_WRITABLE) ? t->update_ref : t->ref;
+	if (!starts_with(ref, "refs/notes/"))
 		die("Refusing to %s notes in %s (outside of refs/notes/)",
-		    subcommand, t->ref);
+		    subcommand, ref);
 	return t;
 }
 
@@ -360,7 +363,7 @@ static int list(int argc, const char **argv, const char *prefix)
 		usage_with_options(git_notes_list_usage, options);
 	}
 
-	t = init_notes_check("list");
+	t = init_notes_check("list", 0);
 	if (argc) {
 		if (get_sha1(argv[0], object))
 			die(_("Failed to resolve '%s' as a valid ref."), argv[0]);
@@ -420,7 +423,7 @@ static int add(int argc, const char **argv, const char *prefix)
 	if (get_sha1(object_ref, object))
 		die(_("Failed to resolve '%s' as a valid ref."), object_ref);
 
-	t = init_notes_check("add");
+	t = init_notes_check("add", NOTES_INIT_WRITABLE);
 	note = get_note(t, object);
 
 	if (note) {
@@ -511,7 +514,7 @@ static int copy(int argc, const char **argv, const char *prefix)
 	if (get_sha1(object_ref, object))
 		die(_("Failed to resolve '%s' as a valid ref."), object_ref);
 
-	t = init_notes_check("copy");
+	t = init_notes_check("copy", NOTES_INIT_WRITABLE);
 	note = get_note(t, object);
 
 	if (note) {
@@ -589,7 +592,7 @@ static int append_edit(int argc, const char **argv, const char *prefix)
 	if (get_sha1(object_ref, object))
 		die(_("Failed to resolve '%s' as a valid ref."), object_ref);
 
-	t = init_notes_check(argv[0]);
+	t = init_notes_check(argv[0], NOTES_INIT_WRITABLE);
 	note = get_note(t, object);
 
 	prepare_note_data(object, &d, edit ? note : NULL);
@@ -652,7 +655,7 @@ static int show(int argc, const char **argv, const char *prefix)
 	if (get_sha1(object_ref, object))
 		die(_("Failed to resolve '%s' as a valid ref."), object_ref);
 
-	t = init_notes_check("show");
+	t = init_notes_check("show", 0);
 	note = get_note(t, object);
 
 	if (!note)
@@ -809,7 +812,7 @@ static int merge(int argc, const char **argv, const char *prefix)
 	expand_notes_ref(&remote_ref);
 	o.remote_ref = remote_ref.buf;
 
-	t = init_notes_check("merge");
+	t = init_notes_check("merge", NOTES_INIT_WRITABLE);
 
 	if (strategy) {
 		if (parse_notes_merge_strategy(strategy, &o.strategy)) {
@@ -901,7 +904,7 @@ static int remove_cmd(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, options,
 			     git_notes_remove_usage, 0);
 
-	t = init_notes_check("remove");
+	t = init_notes_check("remove", NOTES_INIT_WRITABLE);
 
 	if (!argc && !from_stdin) {
 		retval = remove_one_note(t, "HEAD", flag);
@@ -943,7 +946,7 @@ static int prune(int argc, const char **argv, const char *prefix)
 		usage_with_options(git_notes_prune_usage, options);
 	}
 
-	t = init_notes_check("prune");
+	t = init_notes_check("prune", NOTES_INIT_WRITABLE);
 
 	prune_notes(t, (verbose ? NOTES_PRUNE_VERBOSE : 0) |
 		(show_only ? NOTES_PRUNE_VERBOSE|NOTES_PRUNE_DRYRUN : 0) );
diff --git a/notes-cache.c b/notes-cache.c
index c4e9bb7..5dfc5cb 100644
--- a/notes-cache.c
+++ b/notes-cache.c
@@ -32,14 +32,14 @@ void notes_cache_init(struct notes_cache *c, const char *name,
 		     const char *validity)
 {
 	struct strbuf ref = STRBUF_INIT;
-	int flags = 0;
+	int flags = NOTES_INIT_WRITABLE;
 
 	memset(c, 0, sizeof(*c));
 	c->validity = xstrdup(validity);
 
 	strbuf_addf(&ref, "refs/notes/%s", name);
 	if (!notes_cache_match_validity(ref.buf, validity))
-		flags = NOTES_INIT_EMPTY;
+		flags |= NOTES_INIT_EMPTY;
 	init_notes(&c->tree, ref.buf, combine_notes_overwrite, flags);
 	strbuf_release(&ref);
 }
@@ -49,7 +49,8 @@ int notes_cache_write(struct notes_cache *c)
 	unsigned char tree_sha1[20];
 	unsigned char commit_sha1[20];
 
-	if (!c || !c->tree.initialized || !c->tree.ref || !*c->tree.ref)
+	if (!c || !c->tree.initialized || !c->tree.update_ref ||
+	    !*c->tree.update_ref)
 		return -1;
 	if (!c->tree.dirty)
 		return 0;
@@ -59,8 +60,8 @@ int notes_cache_write(struct notes_cache *c)
 	if (commit_tree(c->validity, strlen(c->validity), tree_sha1, NULL,
 			commit_sha1, NULL, NULL) < 0)
 		return -1;
-	if (update_ref("update notes cache", c->tree.ref, commit_sha1, NULL,
-		       0, UPDATE_REFS_QUIET_ON_ERR) < 0)
+	if (update_ref("update notes cache", c->tree.update_ref, commit_sha1,
+		       NULL, 0, UPDATE_REFS_QUIET_ON_ERR) < 0)
 		return -1;
 
 	return 0;
diff --git a/notes-utils.c b/notes-utils.c
index 299e34b..24a3361 100644
--- a/notes-utils.c
+++ b/notes-utils.c
@@ -37,7 +37,7 @@ void commit_notes(struct notes_tree *t, const char *msg)
 
 	if (!t)
 		t = &default_notes_tree;
-	if (!t->initialized || !t->ref || !*t->ref)
+	if (!t->initialized || !t->update_ref || !*t->update_ref)
 		die(_("Cannot commit uninitialized/unreferenced notes tree"));
 	if (!t->dirty)
 		return; /* don't have to commit an unchanged tree */
@@ -48,7 +48,7 @@ void commit_notes(struct notes_tree *t, const char *msg)
 
 	create_notes_commit(t, NULL, buf.buf, buf.len, commit_sha1);
 	strbuf_insert(&buf, 0, "notes: ", 7); /* commit message starts at index 7 */
-	update_ref(buf.buf, t->ref, commit_sha1, NULL, 0,
+	update_ref(buf.buf, t->update_ref, commit_sha1, NULL, 0,
 		   UPDATE_REFS_DIE_ON_ERR);
 
 	strbuf_release(&buf);
@@ -148,7 +148,7 @@ struct notes_rewrite_cfg *init_copy_notes_for_rewrite(const char *cmd)
 		free(c);
 		return NULL;
 	}
-	c->trees = load_notes_trees(c->refs);
+	c->trees = load_notes_trees(c->refs, NOTES_INIT_WRITABLE);
 	string_list_clear(c->refs, 0);
 	free(c->refs);
 	return c;
diff --git a/notes.c b/notes.c
index eacd2a6..6ef347c 100644
--- a/notes.c
+++ b/notes.c
@@ -1008,13 +1008,16 @@ void init_notes(struct notes_tree *t, const char *notes_ref,
 	t->first_non_note = NULL;
 	t->prev_non_note = NULL;
 	t->ref = xstrdup_or_null(notes_ref);
+	t->update_ref = (flags & NOTES_INIT_WRITABLE) ? t->ref : NULL;
 	t->combine_notes = combine_notes;
 	t->initialized = 1;
 	t->dirty = 0;
 
 	if (flags & NOTES_INIT_EMPTY || !notes_ref ||
-	    read_ref(notes_ref, object_sha1))
+	    get_sha1_treeish(notes_ref, object_sha1))
 		return;
+	if (flags & NOTES_INIT_WRITABLE && read_ref(notes_ref, object_sha1))
+		die("Cannot use notes ref %s", notes_ref);
 	if (get_tree_entry(object_sha1, "", sha1, &mode))
 		die("Failed to read notes tree referenced by %s (%s)",
 		    notes_ref, sha1_to_hex(object_sha1));
@@ -1024,7 +1027,7 @@ void init_notes(struct notes_tree *t, const char *notes_ref,
 	load_subtree(t, &root_tree, t->root, 0);
 }
 
-struct notes_tree **load_notes_trees(struct string_list *refs)
+struct notes_tree **load_notes_trees(struct string_list *refs, int flags)
 {
 	struct string_list_item *item;
 	int counter = 0;
@@ -1032,7 +1035,7 @@ struct notes_tree **load_notes_trees(struct string_list *refs)
 	trees = xmalloc((refs->nr+1) * sizeof(struct notes_tree *));
 	for_each_string_list_item(item, refs) {
 		struct notes_tree *t = xcalloc(1, sizeof(struct notes_tree));
-		init_notes(t, item->string, combine_notes_ignore, 0);
+		init_notes(t, item->string, combine_notes_ignore, flags);
 		trees[counter++] = t;
 	}
 	trees[counter] = NULL;
@@ -1068,7 +1071,7 @@ void init_display_notes(struct display_notes_opt *opt)
 						     item->string);
 	}
 
-	display_notes_trees = load_notes_trees(&display_notes_refs);
+	display_notes_trees = load_notes_trees(&display_notes_refs, 0);
 	string_list_clear(&display_notes_refs, 0);
 }
 
diff --git a/notes.h b/notes.h
index 2a3f923..e5d67fd 100644
--- a/notes.h
+++ b/notes.h
@@ -44,6 +44,7 @@ extern struct notes_tree {
 	struct int_node *root;
 	struct non_note *first_non_note, *prev_non_note;
 	char *ref;
+	char *update_ref;
 	combine_notes_fn combine_notes;
 	int initialized;
 	int dirty;
@@ -72,6 +73,13 @@ const char *default_notes_ref(void);
 #define NOTES_INIT_EMPTY 1
 
 /*
+ * By default, the notes tree is only readable, and the notes ref can be
+ * any treeish. The notes tree can however be made writable with this flag,
+ * in which case only strict ref names can be used.
+ */
+#define NOTES_INIT_WRITABLE 2
+
+/*
  * Initialize the given notes_tree with the notes tree structure at the given
  * ref. If given ref is NULL, the value of the $GIT_NOTES_REF environment
  * variable is used, and if that is missing, the default notes ref is used
@@ -276,7 +284,7 @@ void format_display_notes(const unsigned char *object_sha1,
  * Load the notes tree from each ref listed in 'refs'.  The output is
  * an array of notes_tree*, terminated by a NULL.
  */
-struct notes_tree **load_notes_trees(struct string_list *refs);
+struct notes_tree **load_notes_trees(struct string_list *refs, int flags);
 
 /*
  * Add all refs that match 'glob' to the 'list'.
diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index cd70274..2d200fd 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh
@@ -83,6 +83,16 @@ test_expect_success 'edit existing notes' '
 	test_must_fail git notes show HEAD^
 '
 
+test_expect_success 'show notes from treeish' '
+	test "b3" = "$(git notes --ref commits^{tree} show)" &&
+	test "b4" = "$(git notes --ref commits@{1} show)"
+'
+
+test_expect_success 'cannot edit notes from non-ref' '
+	test_must_fail git notes --ref commits^{tree} edit &&
+	test_must_fail git notes --ref commits@{1} edit
+'
+
 test_expect_success 'cannot "git notes add -m" where notes already exists' '
 	test_must_fail git notes add -m "b2" &&
 	test_path_is_missing .git/NOTES_EDITMSG &&
-- 
2.6.1.146.ge5b68b2

Re: [PATCH] notes: Allow treeish expressions as notes ref

[Junio C Hamano]

Mike Hommey <mh@glandium.org> writes:

> After refreshing the patch against current "next", it appears that
> there is such a distinction:
>
> $ ./git-log --notes=fdsfgsfdg HEAD^! --pretty=short
> warning: notes ref refs/notes/fdsfgsfdg is invalid
> commit e5b68b2e879608d881c2e3600ce84962fcdefc88
> Author: Mike Hommey <mh@glandium.org>
>
>     notes: allow treeish expressions as notes ref
>
> $ ./git-log --notes=foo HEAD^! --pretty=short
> commit e5b68b2e879608d881c2e3600ce84962fcdefc88
> Author: Mike Hommey <mh@glandium.org>
>
>     notes: allow treeish expressions as notes ref

Good; thanks for checking.

Re: [PATCH] notes: Allow treeish expressions as notes ref

[Junio C Hamano]

Mike Hommey <mh@glandium.org> writes:

> init_notes() is the main point of entry to the notes API. It is an arbitrary
> restriction that all it allows as input is a strict ref name, when callers
> may want to give an arbitrary treeish.
>
> However, some operations that require updating the notes tree require a
> strict ref name, because they wouldn't be able to update e.g. foo@{1}.
>
> So we allow treeish expressions to be used in the case the notes tree is
> going to be used without write "permissions", and to distinguish whether
> the notes tree is intended to be used for reads only, or will be updated,
> a flag is added.
>
> This has the side effect of enabling the use of treeish as notes refs in
> commands allowing them, e.g. git log --notes=foo@{1}.
>
> Signed-off-by: Mike Hommey <mh@glandium.org>
> ---
>  builtin/notes.c  | 29 ++++++++++++++++-------------
>  notes-cache.c    | 11 ++++++-----
>  notes-utils.c    |  6 +++---
>  notes.c          | 11 +++++++----
>  notes.h          | 10 +++++++++-
>  t/t3301-notes.sh | 10 ++++++++++
>  6 files changed, 51 insertions(+), 26 deletions(-)

At least Documentation/pretty-options.txt needs to be updated, as it
explicitly requests you to feed a ref, but you'd want to tell the
users that you loosened it.  I suspect Documentation/git-notes.txt
may also need adjustment as that involves writing side, but I didn't
look very carefully.

Thanks.

Re: [PATCH] notes: Use get_sha1_committish instead of read_ref in init_notes()

[Jeff King]

On Wed, Jun 17, 2015 at 10:15:31AM +0900, Mike Hommey wrote:

> init_notes() is essentially the only point of entry to the notes API.
> It is an arbitrary restriction that all it allows as input is a strict
> ref name, when callers may want to give an arbitrary committish.
> 
> This has the side effect of enabling the use of committish as notes refs
> in commands allowing them, e.g. git log --notes=foo@{1}, although
> I haven't checked whether that's the case for all of them.

What about expand_notes_ref? We call that on the argument to "--notes".
I guess it is OK to expand "foo@{1}" into "refs/notes/foo@{1}", but what
about other more arcane syntaxes, like ":/"?

In a sense that is weirdly broken already:

  $ git log --notes=:/foo >/dev/null
  warning: notes ref refs/notes/:/foo is invalid

but I wonder if we should be making expand_notes_ref a little more
careful as part of the same topic.

-Peff

Re: [PATCH] notes: Use get_sha1_committish instead of read_ref in init_notes()

[Mike Hommey]

On Tue, Jun 16, 2015 at 11:22:31PM -0400, Jeff King wrote:
> On Wed, Jun 17, 2015 at 10:15:31AM +0900, Mike Hommey wrote:
> 
> > init_notes() is essentially the only point of entry to the notes API.
> > It is an arbitrary restriction that all it allows as input is a strict
> > ref name, when callers may want to give an arbitrary committish.
> > 
> > This has the side effect of enabling the use of committish as notes refs
> > in commands allowing them, e.g. git log --notes=foo@{1}, although
> > I haven't checked whether that's the case for all of them.
> 
> What about expand_notes_ref? We call that on the argument to "--notes".
> I guess it is OK to expand "foo@{1}" into "refs/notes/foo@{1}", but what
> about other more arcane syntaxes, like ":/"?
> 
> In a sense that is weirdly broken already:
> 
>   $ git log --notes=:/foo >/dev/null
>   warning: notes ref refs/notes/:/foo is invalid
> 
> but I wonder if we should be making expand_notes_ref a little more
> careful as part of the same topic.

Interestingly, now that I look, there's also this:
https://github.com/git/git/blob/master/notes-cache.c#L40

that doesn't use expand_notes_ref, but it's apparently only used in
userdiff_get_textconv, and I'm not sure why.

Mike

Re: [PATCH] notes: Use get_sha1_committish instead of read_ref in init_notes()

[Jeff King]

On Wed, Jun 17, 2015 at 06:02:46PM +0900, Mike Hommey wrote:

> > In a sense that is weirdly broken already:
> > 
> >   $ git log --notes=:/foo >/dev/null
> >   warning: notes ref refs/notes/:/foo is invalid
> > 
> > but I wonder if we should be making expand_notes_ref a little more
> > careful as part of the same topic.
> 
> Interestingly, now that I look, there's also this:
> https://github.com/git/git/blob/master/notes-cache.c#L40
> 
> that doesn't use expand_notes_ref, but it's apparently only used in
> userdiff_get_textconv, and I'm not sure why.

notes-cache.c predates expand_notes_ref. I don't think it's a big deal.
The implementation of notes-cache is an internal detail (the user
interacts with it by setting diff.*.cacheTextConv, and then git handles
the cache).

-Peff