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

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

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

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

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:

quoted

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

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

quoted

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