{"thread":{"id":"39653","subject":"[PATCH] notes: Use get_sha1_committish instead of read_ref in init_notes()","startedAt":"2015-06-17T01:15:31Z","lastAt":"2015-10-08T19:03:29Z","messageCount":22,"participants":["Mike Hommey","Junio C Hamano","Jeff King","Johan Herland"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"263982","messageId":"1434503731-26414-1-git-send-email-mh@glandium.org","threadId":"39653","inReplyTo":null,"subject":"[PATCH] notes: Use get_sha1_committish instead of read_ref in init_notes()","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2015-06-17T01:15:31Z","receivedAt":"2015-06-17T01:15:31Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"init_notes() is essentially the only point of entry to the notes API.\nIt is an arbitrary restriction that all it allows as input is a strict\nref name, when callers may want to give an arbitrary committish.\n\nThis has the side effect of enabling the use of committish as notes refs\nin commands allowing them, e.g. git log --notes=foo@{1}, although\nI haven't checked whether that's the case for all of them.\n\nSigned-off-by: Mike Hommey \n---\n\nMy motivation for this change is to allow to use init_notes from a\nthird-party helper that links to libgit.a with a commit that is not\npointed at directly by a ref. The side effect of making git log\n--notes=foo@{1} work is nice IMHO. As noted, though, I haven't checked if\nall code paths using init_notes don't prefilter the ref, but I assume\nthey don't. \n\n notes.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/notes.c b/notes.c\nindex df08209..402e4ce 100644\n--- a/notes.c\n+++ b/notes.c\n@@ -1012,7 +1012,7 @@ void init_notes(struct notes_tree *t, const char *notes_ref,\n \tt->dirty = 0;\n \n \tif (flags & NOTES_INIT_EMPTY || !notes_ref ||\n-\t read_ref(notes_ref, object_sha1))\n+\t get_sha1_committish(notes_ref, object_sha1))\n \t\treturn;\n \tif (get_tree_entry(object_sha1, \"\", sha1, &mode))\n \t\tdie(\"Failed to read notes tree referenced by %s (%s)\",\n-- \n2.4.3.2.gf0a024e.dirty\n"},{"id":"263983","messageId":"xmqq1thb58i8.fsf@gitster.dls.corp.google.com","threadId":"39653","inReplyTo":"1434503731-26414-1-git-send-email-mh@glandium.org","subject":"Re: [PATCH] notes: Use get_sha1_committish instead of read_ref in init_notes()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-17T03:17:03Z","receivedAt":"2015-06-17T03:17:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Hommey writes:\n\n> init_notes() is essentially the only point of entry to the notes API.\n> It is an arbitrary restriction that all it allows as input is a strict\n> ref name, when callers may want to give an arbitrary committish.\n\nWhile it may be a good idea to allow reading from any note-shaped\ntree-ish, not just what is at the tip of a ref, I suspect that the\nuse of read_ref() is not an arbitrary restriction, but is an\neffective way to achieve safety against callers that update notes.\n\nThat is, you can feed refs/notes/commit@{4.days.ago} to the\nmachinery and show you notes from 4 days ago, but you cannot update\nthat as if it were a ref.\n\nHence, if you are loosening the safety at init_notes() site, you\nwould at least need to add a similar safety in the write codepath, I\nwould think. \n\nOne thing you would need to be careful about is that you would give\nusers a crappy experience, if an operation reads notes, does its own\nthing, and then tries to write updated notes (think: rebase that\ntransplants notes from original to rewritten commits), and you fail\nthe operation only at the very last phase of updating. In order to\nprevent that, \"in the write codepath\" above has to be \"reject any\nnon-ref note, e.g. --notes=refs/notes/commit@{4.days.ago} upfront,\nif the operation will write updated notes\".\n"},{"id":"263984","messageId":"20150617032231.GA24505@peff.net","threadId":"39653","inReplyTo":"1434503731-26414-1-git-send-email-mh@glandium.org","subject":"Re: [PATCH] notes: Use get_sha1_committish instead of read_ref in init_notes()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-17T03:22:31Z","receivedAt":"2015-06-17T03:22:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 17, 2015 at 10:15:31AM +0900, Mike Hommey wrote:\n\n> init_notes() is essentially the only point of entry to the notes API.\n> It is an arbitrary restriction that all it allows as input is a strict\n> ref name, when callers may want to give an arbitrary committish.\n> \n> This has the side effect of enabling the use of committish as notes refs\n> in commands allowing them, e.g. git log --notes=foo@{1}, although\n> I haven't checked whether that's the case for all of them.\n\nWhat about expand_notes_ref? We call that on the argument to \"--notes\".\nI guess it is OK to expand \"foo@{1}\" into \"refs/notes/foo@{1}\", but what\nabout other more arcane syntaxes, like \":/\"?\n\nIn a sense that is weirdly broken already:\n\n $ git log --notes=:/foo >/dev/null\n warning: notes ref refs/notes/:/foo is invalid\n\nbut I wonder if we should be making expand_notes_ref a little more\ncareful as part of the same topic.\n\n-Peff\n"},{"id":"263996","messageId":"20150617090246.GC30948@glandium.org","threadId":"39653","inReplyTo":"20150617032231.GA24505@peff.net","subject":"Re: [PATCH] notes: Use get_sha1_committish instead of read_ref in init_notes()","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2015-06-17T09:02:46Z","receivedAt":"2015-06-17T09:02:46Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Tue, Jun 16, 2015 at 11:22:31PM -0400, Jeff King wrote:\n> On Wed, Jun 17, 2015 at 10:15:31AM +0900, Mike Hommey wrote:\n> \n> > init_notes() is essentially the only point of entry to the notes API.\n> > It is an arbitrary restriction that all it allows as input is a strict\n> > ref name, when callers may want to give an arbitrary committish.\n> > \n> > This has the side effect of enabling the use of committish as notes refs\n> > in commands allowing them, e.g. git log --notes=foo@{1}, although\n> > I haven't checked whether that's the case for all of them.\n> \n> What about expand_notes_ref? We call that on the argument to \"--notes\".\n> I guess it is OK to expand \"foo@{1}\" into \"refs/notes/foo@{1}\", but what\n> about other more arcane syntaxes, like \":/\"?\n> \n> In a sense that is weirdly broken already:\n> \n> $ git log --notes=:/foo >/dev/null\n> warning: notes ref refs/notes/:/foo is invalid\n> \n> but I wonder if we should be making expand_notes_ref a little more\n> careful as part of the same topic.\n\nInterestingly, now that I look, there's also this:\nhttps://github.com/git/git/blob/master/notes-cache.c#L40\n\nthat doesn't use expand_notes_ref, but it's apparently only used in\nuserdiff_get_textconv, and I'm not sure why.\n\nMike\n"},{"id":"263997","messageId":"20150617094036.GA536@glandium.org","threadId":"39653","inReplyTo":"xmqq1thb58i8.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] notes: Use get_sha1_committish instead of read_ref in init_notes()","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2015-06-17T09:40:36Z","receivedAt":"2015-06-17T09:40:36Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Tue, Jun 16, 2015 at 08:17:03PM -0700, Junio C Hamano wrote:\n> Mike Hommey writes:\n> \n> > init_notes() is essentially the only point of entry to the notes API.\n> > It is an arbitrary restriction that all it allows as input is a strict\n> > ref name, when callers may want to give an arbitrary committish.\n> \n> While it may be a good idea to allow reading from any note-shaped\n> tree-ish, not just what is at the tip of a ref, I suspect that the\n> use of read_ref() is not an arbitrary restriction, but is an\n> effective way to achieve safety against callers that update notes.\n> \n> That is, you can feed refs/notes/commit@{4.days.ago} to the\n> machinery and show you notes from 4 days ago, but you cannot update\n> that as if it were a ref.\n> \n> Hence, if you are loosening the safety at init_notes() site, you\n> would at least need to add a similar safety in the write codepath, I\n> would think. \n> \n> One thing you would need to be careful about is that you would give\n> users a crappy experience, if an operation reads notes, does its own\n> thing, and then tries to write updated notes (think: rebase that\n> transplants notes from original to rewritten commits), and you fail\n> the operation only at the very last phase of updating. In order to\n> prevent that, \"in the write codepath\" above has to be \"reject any\n> non-ref note, e.g. --notes=refs/notes/commit@{4.days.ago} upfront,\n> if the operation will write updated notes\".\n\nLooking around in the code, I'm not sure how to address this.\nConsidering the APIs, it would seem logical to have individual callers\ncare about filtering the refs themselves, on the other hand, there's\nroom for shooting oneself in the foot when adding new code paths needing\nwrites.\n\nI'm tempted to make init_notes itself do the check, based on the value\nit is given for a \"read_only\" argument. On the other hand, some commands\ndo their ref resolving themselves already. For example, notes_merge uses\nread_ref_full and check_refname_format itself (not necessarily in the\nright order, btw). OTOH, I'm not even sure it ends up using init_notes\nat all.\n\nMike\n"},{"id":"264029","messageId":"xmqqsi9q4b46.fsf@gitster.dls.corp.google.com","threadId":"39653","inReplyTo":"20150617094036.GA536@glandium.org","subject":"Re: [PATCH] notes: Use get_sha1_committish instead of read_ref in init_notes()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-17T15:18:17Z","receivedAt":"2015-06-17T15:18:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Hommey writes:\n\n> I'm tempted to make init_notes itself do the check, based on the value\n> it is given for a \"read_only\" argument.\n\nYeah, that would be one sensible way to go after making sure that\neverything goes thru this interface.\n\n> On the other hand, some commands\n> do their ref resolving themselves already.\n\nAgain, as long as they do not bypass the \"read-only\" safety you are\nsuggesting to add to init_notes(), that is OK.\n"},{"id":"264047","messageId":"CALKQrgfR7AwJ1XY2iPHBAssk_2uKGUJC=J-r7bJVSKaiosc5gw@mail.gmail.com","threadId":"39653","inReplyTo":"xmqqsi9q4b46.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] notes: Use get_sha1_committish instead of read_ref in init_notes()","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2015-06-17T16:35:11Z","receivedAt":"2015-06-17T16:35:11Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Wed, Jun 17, 2015 at 5:18 PM, Junio C Hamano wrote:\n> Mike Hommey writes:\n> > I'm tempted to make init_notes itself do the check, based on the value\n> > it is given for a \"read_only\" argument.\n>\n> Yeah, that would be one sensible way to go after making sure that\n> everything goes thru this interface.\n\nAgreed. Furthermore, consider adding the read_only flag (or however\nyou choose to encode it internally) to struct notes_tree, so that the\nAPI functions that _manipulate_ notes trees can immediately bail out\nwhen used on a read-only tree (i.e. we want them to fail as early as\npossible).\n\n> > On the other hand, some commands\n> > do their ref resolving themselves already.\n>\n> Again, as long as they do not bypass the \"read-only\" safety you are\n> suggesting to add to init_notes(), that is OK.\n\nAgreed. An alternative to adding a simple read_only flag argument is\nto modify the const char *notes_ref argument into two separate\narguments: const char *notes_treeish, and const char *update_ref,\nwhere the latter should be NULL for read-only trees. That said,\ncurrently the logic for actually updating notes ref lives outside the\nnotes.h API (see commit_notes() in notes-utils.h/c), so there might be\nroom for more consolidation/refactoring here...\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"264039","messageId":"20150617163536.GA22689@peff.net","threadId":"39653","inReplyTo":"20150617090246.GC30948@glandium.org","subject":"Re: [PATCH] notes: Use get_sha1_committish instead of read_ref in init_notes()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-17T16:35:36Z","receivedAt":"2015-06-17T16:35:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 17, 2015 at 06:02:46PM +0900, Mike Hommey wrote:\n\n> > In a sense that is weirdly broken already:\n> > \n> > $ git log --notes=:/foo >/dev/null\n> > warning: notes ref refs/notes/:/foo is invalid\n> > \n> > but I wonder if we should be making expand_notes_ref a little more\n> > careful as part of the same topic.\n> \n> Interestingly, now that I look, there's also this:\n> https://github.com/git/git/blob/master/notes-cache.c#L40\n> \n> that doesn't use expand_notes_ref, but it's apparently only used in\n> userdiff_get_textconv, and I'm not sure why.\n\nnotes-cache.c predates expand_notes_ref. I don't think it's a big deal.\nThe implementation of notes-cache is an internal detail (the user\ninteracts with it by setting diff.*.cacheTextConv, and then git handles\nthe cache).\n\n-Peff\n"},{"id":"265823","messageId":"1436350876-8274-1-git-send-email-mh@glandium.org","threadId":"39653","inReplyTo":"CALKQrgfR7AwJ1XY2iPHBAssk_2uKGUJC=J-r7bJVSKaiosc5gw@mail.gmail.com","subject":"[PATCH v2 - RFH] notes: Allow committish expressions as notes ref","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2015-07-08T10:21:16Z","receivedAt":"2015-07-08T10:21:16Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"init_notes() is the main point of entry to the notes API. It is an arbitrary\nrestriction that all it allows as input is a strict ref name, when callers\nmay want to give an arbitrary committish.\n\nHowever, some operations that require updating the notes tree require a\nstrict ref name, because they wouldn't be able to update e.g. foo@{1}.\n\nSo we allow committish expressions to be used in the case the notes tree\nis going to be used without write \"permissions\", and to distinguish whether\nthe notes tree is intended to be used for reads only, or will be updated,\na flag is added.\n\nThis has the side effect of enabling the use of committish as notes refs\nin commands allowing them, e.g. git log --notes=foo@{1}.\n\nSigned-off-by: Mike Hommey \n---\nThe die() message in init_notes is left to be desired. But I don't have a\nbetter idea at the moment.\n\nNote that die()ing in an API is not exactly nice, but since init_notes was\nalready die()ing, I figured I wasn't making things worse than they already\nare. Further refactoring feels like scope bloat.\n\nNow, for the RFH, this actually doesn't entirely work, because *creating*\na new notes ref fails on that die() I'm adding. So the question is, how do\nI distinguish between read_ref failing because the ref is not a proper ref,\nand read_ref failing because the ref doesn't exist?\n\n(I figured I'd ask instead of looking it up myself because it's the end of\nthe day here, and by the time I look at it again, there should be an answer\nfrom someone who knows the code already :) )\n\n builtin/notes.c | 27 +++++++++++++++------------\n notes-cache.c | 7 ++++---\n notes-utils.c | 4 ++--\n notes.c | 9 +++++++--\n notes.h | 8 ++++++++\n 5 files changed, 36 insertions(+), 19 deletions(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 63f95fc..0712178 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -328,15 +328,18 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)\n \treturn ret;\n }\n \n-static struct notes_tree *init_notes_check(const char *subcommand)\n+static struct notes_tree *init_notes_check(const char *subcommand,\n+\t\t\t\t\t int flags)\n {\n \tstruct notes_tree *t;\n-\tinit_notes(NULL, NULL, NULL, 0);\n+\tconst char *ref;\n+\tinit_notes(NULL, NULL, NULL, flags);\n \tt = &default_notes_tree;\n \n-\tif (!starts_with(t->ref, \"refs/notes/\"))\n+\tref = (flags & NOTES_INIT_WRITABLE) ? t->update_ref : t->ref;\n+\tif (!starts_with(ref, \"refs/notes/\"))\n \t\tdie(\"Refusing to %s notes in %s (outside of refs/notes/)\",\n-\t\t subcommand, t->ref);\n+\t\t subcommand, ref);\n \treturn t;\n }\n \n@@ -359,7 +362,7 @@ static int list(int argc, const char **argv, const char *prefix)\n \t\tusage_with_options(git_notes_list_usage, options);\n \t}\n \n-\tt = init_notes_check(\"list\");\n+\tt = init_notes_check(\"list\", 0);\n \tif (argc) {\n \t\tif (get_sha1(argv[0], object))\n \t\t\tdie(_(\"Failed to resolve '%s' as a valid ref.\"), argv[0]);\n@@ -419,7 +422,7 @@ static int add(int argc, const char **argv, const char *prefix)\n \tif (get_sha1(object_ref, object))\n \t\tdie(_(\"Failed to resolve '%s' as a valid ref.\"), object_ref);\n \n-\tt = init_notes_check(\"add\");\n+\tt = init_notes_check(\"add\", NOTES_INIT_WRITABLE);\n \tnote = get_note(t, object);\n \n \tif (note) {\n@@ -510,7 +513,7 @@ static int copy(int argc, const char **argv, const char *prefix)\n \tif (get_sha1(object_ref, object))\n \t\tdie(_(\"Failed to resolve '%s' as a valid ref.\"), object_ref);\n \n-\tt = init_notes_check(\"copy\");\n+\tt = init_notes_check(\"copy\", NOTES_INIT_WRITABLE);\n \tnote = get_note(t, object);\n \n \tif (note) {\n@@ -588,7 +591,7 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \tif (get_sha1(object_ref, object))\n \t\tdie(_(\"Failed to resolve '%s' as a valid ref.\"), object_ref);\n \n-\tt = init_notes_check(argv[0]);\n+\tt = init_notes_check(argv[0], NOTES_INIT_WRITABLE);\n \tnote = get_note(t, object);\n \n \tprepare_note_data(object, &d, edit ? note : NULL);\n@@ -651,7 +654,7 @@ static int show(int argc, const char **argv, const char *prefix)\n \tif (get_sha1(object_ref, object))\n \t\tdie(_(\"Failed to resolve '%s' as a valid ref.\"), object_ref);\n \n-\tt = init_notes_check(\"show\");\n+\tt = init_notes_check(\"show\", 0);\n \tnote = get_note(t, object);\n \n \tif (!note)\n@@ -812,7 +815,7 @@ static int merge(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n-\tt = init_notes_check(\"merge\");\n+\tt = init_notes_check(\"merge\", NOTES_INIT_WRITABLE);\n \n \tstrbuf_addf(&msg, \"notes: Merged notes from %s into %s\",\n \t\t remote_ref.buf, default_notes_ref());\n@@ -878,7 +881,7 @@ static int remove_cmd(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, options,\n \t\t\t git_notes_remove_usage, 0);\n \n-\tt = init_notes_check(\"remove\");\n+\tt = init_notes_check(\"remove\", NOTES_INIT_WRITABLE);\n \n \tif (!argc && !from_stdin) {\n \t\tretval = remove_one_note(t, \"HEAD\", flag);\n@@ -920,7 +923,7 @@ static int prune(int argc, const char **argv, const char *prefix)\n \t\tusage_with_options(git_notes_prune_usage, options);\n \t}\n \n-\tt = init_notes_check(\"prune\");\n+\tt = init_notes_check(\"prune\", NOTES_INIT_WRITABLE);\n \n \tprune_notes(t, (verbose ? NOTES_PRUNE_VERBOSE : 0) |\n \t\t(show_only ? NOTES_PRUNE_VERBOSE|NOTES_PRUNE_DRYRUN : 0) );\ndiff --git a/notes-cache.c b/notes-cache.c\nindex c4e9bb7..3ee8d2c 100644\n--- a/notes-cache.c\n+++ b/notes-cache.c\n@@ -49,7 +49,8 @@ int notes_cache_write(struct notes_cache *c)\n \tunsigned char tree_sha1[20];\n \tunsigned char commit_sha1[20];\n \n-\tif (!c || !c->tree.initialized || !c->tree.ref || !*c->tree.ref)\n+\tif (!c || !c->tree.initialized || !c->tree.update_ref ||\n+\t !*c->tree.update_ref)\n \t\treturn -1;\n \tif (!c->tree.dirty)\n \t\treturn 0;\n@@ -59,8 +60,8 @@ int notes_cache_write(struct notes_cache *c)\n \tif (commit_tree(c->validity, strlen(c->validity), tree_sha1, NULL,\n \t\t\tcommit_sha1, NULL, NULL) < 0)\n \t\treturn -1;\n-\tif (update_ref(\"update notes cache\", c->tree.ref, commit_sha1, NULL,\n-\t\t 0, UPDATE_REFS_QUIET_ON_ERR) < 0)\n+\tif (update_ref(\"update notes cache\", c->tree.update_ref, commit_sha1,\n+\t\t NULL, 0, UPDATE_REFS_QUIET_ON_ERR) < 0)\n \t\treturn -1;\n \n \treturn 0;\ndiff --git a/notes-utils.c b/notes-utils.c\nindex ccbf073..937e1f0 100644\n--- a/notes-utils.c\n+++ b/notes-utils.c\n@@ -37,7 +37,7 @@ void commit_notes(struct notes_tree *t, const char *msg)\n \n \tif (!t)\n \t\tt = &default_notes_tree;\n-\tif (!t->initialized || !t->ref || !*t->ref)\n+\tif (!t->initialized || !t->update_ref || !*t->update_ref)\n \t\tdie(_(\"Cannot commit uninitialized/unreferenced notes tree\"));\n \tif (!t->dirty)\n \t\treturn; /* don't have to commit an unchanged tree */\n@@ -48,7 +48,7 @@ void commit_notes(struct notes_tree *t, const char *msg)\n \n \tcreate_notes_commit(t, NULL, buf.buf, buf.len, commit_sha1);\n \tstrbuf_insert(&buf, 0, \"notes: \", 7); /* commit message starts at index 7 */\n-\tupdate_ref(buf.buf, t->ref, commit_sha1, NULL, 0,\n+\tupdate_ref(buf.buf, t->update_ref, commit_sha1, NULL, 0,\n \t\t UPDATE_REFS_DIE_ON_ERR);\n \n \tstrbuf_release(&buf);\ndiff --git a/notes.c b/notes.c\nindex df08209..8c6019d 100644\n--- a/notes.c\n+++ b/notes.c\n@@ -1007,12 +1007,17 @@ void init_notes(struct notes_tree *t, const char *notes_ref,\n \tt->first_non_note = NULL;\n \tt->prev_non_note = NULL;\n \tt->ref = xstrdup_or_null(notes_ref);\n+\tt->update_ref = (flags & NOTES_INIT_WRITABLE) ? t->ref : NULL;\n \tt->combine_notes = combine_notes;\n \tt->initialized = 1;\n \tt->dirty = 0;\n \n-\tif (flags & NOTES_INIT_EMPTY || !notes_ref ||\n-\t read_ref(notes_ref, object_sha1))\n+\tif (flags & NOTES_INIT_EMPTY || !notes_ref)\n+\t\treturn;\n+\tif (flags & NOTES_INIT_WRITABLE && read_ref(notes_ref, object_sha1))\n+\t\tdie(\"Invalid ref %s\", notes_ref);;\n+\tif (!(flags & NOTES_INIT_WRITABLE) &&\n+\t get_sha1_committish(notes_ref, object_sha1))\n \t\treturn;\n \tif (get_tree_entry(object_sha1, \"\", sha1, &mode))\n \t\tdie(\"Failed to read notes tree referenced by %s (%s)\",\ndiff --git a/notes.h b/notes.h\nindex 2a3f923..9b2b96c 100644\n--- a/notes.h\n+++ b/notes.h\n@@ -44,6 +44,7 @@ extern struct notes_tree {\n \tstruct int_node *root;\n \tstruct non_note *first_non_note, *prev_non_note;\n \tchar *ref;\n+\tchar *update_ref;\n \tcombine_notes_fn combine_notes;\n \tint initialized;\n \tint dirty;\n@@ -72,6 +73,13 @@ const char *default_notes_ref(void);\n #define NOTES_INIT_EMPTY 1\n \n /*\n+ * By default, the notes tree is only readable, and the notes ref can be\n+ * any committish. The notes tree can however be made writable with this\n+ * flag, in which case only strict ref names can be used.\n+ */\n+#define NOTES_INIT_WRITABLE 2\n+\n+/*\n * Initialize the given notes_tree with the notes tree structure at the given\n * ref. If given ref is NULL, the value of the $GIT_NOTES_REF environment\n * variable is used, and if that is missing, the default notes ref is used\n-- \n2.4.3.2.gf0a024e.dirty\n"},{"id":"265888","messageId":"1436410125-4957-1-git-send-email-mh@glandium.org","threadId":"39653","inReplyTo":"1436350876-8274-1-git-send-email-mh@glandium.org","subject":"[PATCH v3] notes: Allow committish expressions as notes ref","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2015-07-09T02:48:45Z","receivedAt":"2015-07-09T02:48:45Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"init_notes() is the main point of entry to the notes API. It is an arbitrary\nrestriction that all it allows as input is a strict ref name, when callers\nmay want to give an arbitrary committish.\n\nHowever, some operations that require updating the notes tree require a\nstrict ref name, because they wouldn't be able to update e.g. foo@{1}.\n\nSo we allow committish expressions to be used in the case the notes tree\nis going to be used without write \"permissions\", and to distinguish whether\nthe notes tree is intended to be used for reads only, or will be updated,\na flag is added.\n\nThis has the side effect of enabling the use of committish as notes refs\nin commands allowing them, e.g. git log --notes=foo@{1}.\n\nSigned-off-by: Mike Hommey \n---\n builtin/notes.c | 29 ++++++++++++++++-------------\n notes-cache.c | 11 ++++++-----\n notes-utils.c | 6 +++---\n notes.c | 11 +++++++----\n notes.h | 10 +++++++++-\n 5 files changed, 41 insertions(+), 26 deletions(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 63f95fc..0fc6e7a 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -285,7 +285,7 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)\n \t\tif (!c)\n \t\t\treturn 0;\n \t} else {\n-\t\tinit_notes(NULL, NULL, NULL, 0);\n+\t\tinit_notes(NULL, NULL, NULL, NOTES_INIT_WRITABLE);\n \t\tt = &default_notes_tree;\n \t}\n \n@@ -328,15 +328,18 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)\n \treturn ret;\n }\n \n-static struct notes_tree *init_notes_check(const char *subcommand)\n+static struct notes_tree *init_notes_check(const char *subcommand,\n+\t\t\t\t\t int flags)\n {\n \tstruct notes_tree *t;\n-\tinit_notes(NULL, NULL, NULL, 0);\n+\tconst char *ref;\n+\tinit_notes(NULL, NULL, NULL, flags);\n \tt = &default_notes_tree;\n \n-\tif (!starts_with(t->ref, \"refs/notes/\"))\n+\tref = (flags & NOTES_INIT_WRITABLE) ? t->update_ref : t->ref;\n+\tif (!starts_with(ref, \"refs/notes/\"))\n \t\tdie(\"Refusing to %s notes in %s (outside of refs/notes/)\",\n-\t\t subcommand, t->ref);\n+\t\t subcommand, ref);\n \treturn t;\n }\n \n@@ -359,7 +362,7 @@ static int list(int argc, const char **argv, const char *prefix)\n \t\tusage_with_options(git_notes_list_usage, options);\n \t}\n \n-\tt = init_notes_check(\"list\");\n+\tt = init_notes_check(\"list\", 0);\n \tif (argc) {\n \t\tif (get_sha1(argv[0], object))\n \t\t\tdie(_(\"Failed to resolve '%s' as a valid ref.\"), argv[0]);\n@@ -419,7 +422,7 @@ static int add(int argc, const char **argv, const char *prefix)\n \tif (get_sha1(object_ref, object))\n \t\tdie(_(\"Failed to resolve '%s' as a valid ref.\"), object_ref);\n \n-\tt = init_notes_check(\"add\");\n+\tt = init_notes_check(\"add\", NOTES_INIT_WRITABLE);\n \tnote = get_note(t, object);\n \n \tif (note) {\n@@ -510,7 +513,7 @@ static int copy(int argc, const char **argv, const char *prefix)\n \tif (get_sha1(object_ref, object))\n \t\tdie(_(\"Failed to resolve '%s' as a valid ref.\"), object_ref);\n \n-\tt = init_notes_check(\"copy\");\n+\tt = init_notes_check(\"copy\", NOTES_INIT_WRITABLE);\n \tnote = get_note(t, object);\n \n \tif (note) {\n@@ -588,7 +591,7 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \tif (get_sha1(object_ref, object))\n \t\tdie(_(\"Failed to resolve '%s' as a valid ref.\"), object_ref);\n \n-\tt = init_notes_check(argv[0]);\n+\tt = init_notes_check(argv[0], NOTES_INIT_WRITABLE);\n \tnote = get_note(t, object);\n \n \tprepare_note_data(object, &d, edit ? note : NULL);\n@@ -651,7 +654,7 @@ static int show(int argc, const char **argv, const char *prefix)\n \tif (get_sha1(object_ref, object))\n \t\tdie(_(\"Failed to resolve '%s' as a valid ref.\"), object_ref);\n \n-\tt = init_notes_check(\"show\");\n+\tt = init_notes_check(\"show\", 0);\n \tnote = get_note(t, object);\n \n \tif (!note)\n@@ -812,7 +815,7 @@ static int merge(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n-\tt = init_notes_check(\"merge\");\n+\tt = init_notes_check(\"merge\", NOTES_INIT_WRITABLE);\n \n \tstrbuf_addf(&msg, \"notes: Merged notes from %s into %s\",\n \t\t remote_ref.buf, default_notes_ref());\n@@ -878,7 +881,7 @@ static int remove_cmd(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, options,\n \t\t\t git_notes_remove_usage, 0);\n \n-\tt = init_notes_check(\"remove\");\n+\tt = init_notes_check(\"remove\", NOTES_INIT_WRITABLE);\n \n \tif (!argc && !from_stdin) {\n \t\tretval = remove_one_note(t, \"HEAD\", flag);\n@@ -920,7 +923,7 @@ static int prune(int argc, const char **argv, const char *prefix)\n \t\tusage_with_options(git_notes_prune_usage, options);\n \t}\n \n-\tt = init_notes_check(\"prune\");\n+\tt = init_notes_check(\"prune\", NOTES_INIT_WRITABLE);\n \n \tprune_notes(t, (verbose ? NOTES_PRUNE_VERBOSE : 0) |\n \t\t(show_only ? NOTES_PRUNE_VERBOSE|NOTES_PRUNE_DRYRUN : 0) );\ndiff --git a/notes-cache.c b/notes-cache.c\nindex c4e9bb7..5dfc5cb 100644\n--- a/notes-cache.c\n+++ b/notes-cache.c\n@@ -32,14 +32,14 @@ void notes_cache_init(struct notes_cache *c, const char *name,\n \t\t const char *validity)\n {\n \tstruct strbuf ref = STRBUF_INIT;\n-\tint flags = 0;\n+\tint flags = NOTES_INIT_WRITABLE;\n \n \tmemset(c, 0, sizeof(*c));\n \tc->validity = xstrdup(validity);\n \n \tstrbuf_addf(&ref, \"refs/notes/%s\", name);\n \tif (!notes_cache_match_validity(ref.buf, validity))\n-\t\tflags = NOTES_INIT_EMPTY;\n+\t\tflags |= NOTES_INIT_EMPTY;\n \tinit_notes(&c->tree, ref.buf, combine_notes_overwrite, flags);\n \tstrbuf_release(&ref);\n }\n@@ -49,7 +49,8 @@ int notes_cache_write(struct notes_cache *c)\n \tunsigned char tree_sha1[20];\n \tunsigned char commit_sha1[20];\n \n-\tif (!c || !c->tree.initialized || !c->tree.ref || !*c->tree.ref)\n+\tif (!c || !c->tree.initialized || !c->tree.update_ref ||\n+\t !*c->tree.update_ref)\n \t\treturn -1;\n \tif (!c->tree.dirty)\n \t\treturn 0;\n@@ -59,8 +60,8 @@ int notes_cache_write(struct notes_cache *c)\n \tif (commit_tree(c->validity, strlen(c->validity), tree_sha1, NULL,\n \t\t\tcommit_sha1, NULL, NULL) < 0)\n \t\treturn -1;\n-\tif (update_ref(\"update notes cache\", c->tree.ref, commit_sha1, NULL,\n-\t\t 0, UPDATE_REFS_QUIET_ON_ERR) < 0)\n+\tif (update_ref(\"update notes cache\", c->tree.update_ref, commit_sha1,\n+\t\t NULL, 0, UPDATE_REFS_QUIET_ON_ERR) < 0)\n \t\treturn -1;\n \n \treturn 0;\ndiff --git a/notes-utils.c b/notes-utils.c\nindex ccbf073..f65edba 100644\n--- a/notes-utils.c\n+++ b/notes-utils.c\n@@ -37,7 +37,7 @@ void commit_notes(struct notes_tree *t, const char *msg)\n \n \tif (!t)\n \t\tt = &default_notes_tree;\n-\tif (!t->initialized || !t->ref || !*t->ref)\n+\tif (!t->initialized || !t->update_ref || !*t->update_ref)\n \t\tdie(_(\"Cannot commit uninitialized/unreferenced notes tree\"));\n \tif (!t->dirty)\n \t\treturn; /* don't have to commit an unchanged tree */\n@@ -48,7 +48,7 @@ void commit_notes(struct notes_tree *t, const char *msg)\n \n \tcreate_notes_commit(t, NULL, buf.buf, buf.len, commit_sha1);\n \tstrbuf_insert(&buf, 0, \"notes: \", 7); /* commit message starts at index 7 */\n-\tupdate_ref(buf.buf, t->ref, commit_sha1, NULL, 0,\n+\tupdate_ref(buf.buf, t->update_ref, commit_sha1, NULL, 0,\n \t\t UPDATE_REFS_DIE_ON_ERR);\n \n \tstrbuf_release(&buf);\n@@ -130,7 +130,7 @@ struct notes_rewrite_cfg *init_copy_notes_for_rewrite(const char *cmd)\n \t\tfree(c);\n \t\treturn NULL;\n \t}\n-\tc->trees = load_notes_trees(c->refs);\n+\tc->trees = load_notes_trees(c->refs, NOTES_INIT_WRITABLE);\n \tstring_list_clear(c->refs, 0);\n \tfree(c->refs);\n \treturn c;\ndiff --git a/notes.c b/notes.c\nindex df08209..84f8a47 100644\n--- a/notes.c\n+++ b/notes.c\n@@ -1007,13 +1007,16 @@ void init_notes(struct notes_tree *t, const char *notes_ref,\n \tt->first_non_note = NULL;\n \tt->prev_non_note = NULL;\n \tt->ref = xstrdup_or_null(notes_ref);\n+\tt->update_ref = (flags & NOTES_INIT_WRITABLE) ? t->ref : NULL;\n \tt->combine_notes = combine_notes;\n \tt->initialized = 1;\n \tt->dirty = 0;\n \n \tif (flags & NOTES_INIT_EMPTY || !notes_ref ||\n-\t read_ref(notes_ref, object_sha1))\n+\t get_sha1_committish(notes_ref, object_sha1))\n \t\treturn;\n+\tif (flags & NOTES_INIT_WRITABLE && read_ref(notes_ref, object_sha1))\n+\t\tdie(\"Cannot use notes ref %s\", notes_ref);\n \tif (get_tree_entry(object_sha1, \"\", sha1, &mode))\n \t\tdie(\"Failed to read notes tree referenced by %s (%s)\",\n \t\t notes_ref, sha1_to_hex(object_sha1));\n@@ -1023,7 +1026,7 @@ void init_notes(struct notes_tree *t, const char *notes_ref,\n \tload_subtree(t, &root_tree, t->root, 0);\n }\n \n-struct notes_tree **load_notes_trees(struct string_list *refs)\n+struct notes_tree **load_notes_trees(struct string_list *refs, int flags)\n {\n \tstruct string_list_item *item;\n \tint counter = 0;\n@@ -1031,7 +1034,7 @@ struct notes_tree **load_notes_trees(struct string_list *refs)\n \ttrees = xmalloc((refs->nr+1) * sizeof(struct notes_tree *));\n \tfor_each_string_list_item(item, refs) {\n \t\tstruct notes_tree *t = xcalloc(1, sizeof(struct notes_tree));\n-\t\tinit_notes(t, item->string, combine_notes_ignore, 0);\n+\t\tinit_notes(t, item->string, combine_notes_ignore, flags);\n \t\ttrees[counter++] = t;\n \t}\n \ttrees[counter] = NULL;\n@@ -1067,7 +1070,7 @@ void init_display_notes(struct display_notes_opt *opt)\n \t\t\t\t\t\t item->string);\n \t}\n \n-\tdisplay_notes_trees = load_notes_trees(&display_notes_refs);\n+\tdisplay_notes_trees = load_notes_trees(&display_notes_refs, 0);\n \tstring_list_clear(&display_notes_refs, 0);\n }\n \ndiff --git a/notes.h b/notes.h\nindex 2a3f923..aa9960c 100644\n--- a/notes.h\n+++ b/notes.h\n@@ -44,6 +44,7 @@ extern struct notes_tree {\n \tstruct int_node *root;\n \tstruct non_note *first_non_note, *prev_non_note;\n \tchar *ref;\n+\tchar *update_ref;\n \tcombine_notes_fn combine_notes;\n \tint initialized;\n \tint dirty;\n@@ -72,6 +73,13 @@ const char *default_notes_ref(void);\n #define NOTES_INIT_EMPTY 1\n \n /*\n+ * By default, the notes tree is only readable, and the notes ref can be\n+ * any committish. The notes tree can however be made writable with this\n+ * flag, in which case only strict ref names can be used.\n+ */\n+#define NOTES_INIT_WRITABLE 2\n+\n+/*\n * Initialize the given notes_tree with the notes tree structure at the given\n * ref. If given ref is NULL, the value of the $GIT_NOTES_REF environment\n * variable is used, and if that is missing, the default notes ref is used\n@@ -276,7 +284,7 @@ void format_display_notes(const unsigned char *object_sha1,\n * Load the notes tree from each ref listed in 'refs'. The output is\n * an array of notes_tree*, terminated by a NULL.\n */\n-struct notes_tree **load_notes_trees(struct string_list *refs);\n+struct notes_tree **load_notes_trees(struct string_list *refs, int flags);\n \n /*\n * Add all refs that match 'glob' to the 'list'.\n-- \n2.4.3.2.gf0a024e.dirty\n"},{"id":"265962","messageId":"CALKQrgd_eB2ZUwUz54vW9OFOuZshvGGe9dx+suvd0=WA0OmOFQ@mail.gmail.com","threadId":"39653","inReplyTo":"1436410125-4957-1-git-send-email-mh@glandium.org","subject":"Re: [PATCH v3] notes: Allow committish expressions as notes ref","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2015-07-10T01:03:15Z","receivedAt":"2015-07-10T01:03:15Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Thu, Jul 9, 2015 at 4:48 AM, Mike Hommey wrote:\n> init_notes() is the main point of entry to the notes API. It is an arbitrary\n> restriction that all it allows as input is a strict ref name, when callers\n> may want to give an arbitrary committish.\n>\n> However, some operations that require updating the notes tree require a\n> strict ref name, because they wouldn't be able to update e.g. foo@{1}.\n>\n> So we allow committish expressions to be used in the case the notes tree\n> is going to be used without write \"permissions\", and to distinguish whether\n> the notes tree is intended to be used for reads only, or will be updated,\n> a flag is added.\n>\n> This has the side effect of enabling the use of committish as notes refs\n> in commands allowing them, e.g. git log --notes=foo@{1}.\n>\n> Signed-off-by: Mike Hommey \n\nReviewed-by: Johan Herland \n\n...modulo some comments below.\n\n> ---\n> builtin/notes.c | 29 ++++++++++++++++-------------\n> notes-cache.c | 11 ++++++-----\n> notes-utils.c | 6 +++---\n> notes.c | 11 +++++++----\n> notes.h | 10 +++++++++-\n> 5 files changed, 41 insertions(+), 26 deletions(-)\n>\n> diff --git a/builtin/notes.c b/builtin/notes.c\n> index 63f95fc..0fc6e7a 100644\n> --- a/builtin/notes.c\n> +++ b/builtin/notes.c\n> @@ -285,7 +285,7 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)\n> if (!c)\n> return 0;\n> } else {\n> - init_notes(NULL, NULL, NULL, 0);\n> + init_notes(NULL, NULL, NULL, NOTES_INIT_WRITABLE);\n> t = &default_notes_tree;\n> }\n>\n> @@ -328,15 +328,18 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)\n> return ret;\n> }\n>\n> -static struct notes_tree *init_notes_check(const char *subcommand)\n> +static struct notes_tree *init_notes_check(const char *subcommand,\n> + int flags)\n> {\n> struct notes_tree *t;\n> - init_notes(NULL, NULL, NULL, 0);\n> + const char *ref;\n> + init_notes(NULL, NULL, NULL, flags);\n> t = &default_notes_tree;\n>\n> - if (!starts_with(t->ref, \"refs/notes/\"))\n> + ref = (flags & NOTES_INIT_WRITABLE) ? t->update_ref : t->ref;\n\nHmm, AFAICS from the code in notes.c, when NOTES_INIT_WRITABLE is set,\nthen t->ref == t->update_ref, which means the above line is redundant,\nand t->ref will work just as well. That said, it's also bad to depend on\nthat fact from here, as the notes code might at some point change to\nmake t->ref != t->update_ref (e.g. if we want to allow a notes tree\nwhich reads from one ref, but writes to another), so I guess the current\ncode is good.\n\n> + if (!starts_with(ref, \"refs/notes/\"))\n> die(\"Refusing to %s notes in %s (outside of refs/notes/)\",\n> - subcommand, t->ref);\n> + subcommand, ref);\n> return t;\n> }\n>\n[...]\n> diff --git a/notes.c b/notes.c\n> index df08209..84f8a47 100644\n> --- a/notes.c\n> +++ b/notes.c\n> @@ -1007,13 +1007,16 @@ void init_notes(struct notes_tree *t, const char *notes_ref,\n> t->first_non_note = NULL;\n> t->prev_non_note = NULL;\n> t->ref = xstrdup_or_null(notes_ref);\n> + t->update_ref = (flags & NOTES_INIT_WRITABLE) ? t->ref : NULL;\n\nThis is the only place in notes.c that references t->update_ref. Which\npoints to a latent design flaw in the notes API: struct notes_tree keeps\ntrack of the notes ref (previously t->ref, but now also t->update_ref),\nbut it is actually never used by the core notes implementation (except\nfor display purposes in format_note() and an error message in\nload_subtree()). I guess a better design would be to either\n\n (a) move the ref out of the API entirely, leaving notes.h/.c to only\n worry about reading and writing notes tree objects, and letting\n the caller deal with resolving and updatiung notes refs and\n commit objects.\n\n (b) move the ref handling _into_ the API, basically moving\n commit_notes() from notes-utils.h/.c into notes.h/.c\n\nOf these, I believe (a) is better, escpecially if we also provide a\nwrapper API (in notes-utils.h/.c?) around the inner/core API to deal\nwith the commit/ref details.\n\nHowever, this is largely independent of your current effort, and\nbelongs in a different patch (series).\n\n> t->combine_notes = combine_notes;\n> t->initialized = 1;\n> t->dirty = 0;\n>\n> if (flags & NOTES_INIT_EMPTY || !notes_ref ||\n> - read_ref(notes_ref, object_sha1))\n> + get_sha1_committish(notes_ref, object_sha1))\n\nI believe this should be get_sha1_treeish() instead. The only place we use\nthe result (object_sha1) is when calling get_tree_entry() below, which\nwill peel to a tree object anyway, so there's no need to place the\nstricter commitish requirement on the caller (in the read-only case).\n\nAs a bonus, you can also s/commitish/treeish/ in the commit subject line.\n\nAlso, I do not immediately remember whether we support notes refs that\npoint directly at tree objects (bypassing the commit object entirely),\nbut if we do, the get_sha1_committish() would break that use case...\n\n> return;\n> + if (flags & NOTES_INIT_WRITABLE && read_ref(notes_ref, object_sha1))\n\nDrop the extra space after &&.\n\n[....]\n> diff --git a/notes.h b/notes.h\n> index 2a3f923..aa9960c 100644\n> --- a/notes.h\n> +++ b/notes.h\n> @@ -44,6 +44,7 @@ extern struct notes_tree {\n> struct int_node *root;\n> struct non_note *first_non_note, *prev_non_note;\n> char *ref;\n> + char *update_ref;\n> combine_notes_fn combine_notes;\n> int initialized;\n> int dirty;\n> @@ -72,6 +73,13 @@ const char *default_notes_ref(void);\n> #define NOTES_INIT_EMPTY 1\n>\n> /*\n> + * By default, the notes tree is only readable, and the notes ref can be\n> + * any committish. The notes tree can however be made writable with this\n\ns/commitish/treeish/\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"265963","messageId":"1436491711-19056-1-git-send-email-mh@glandium.org","threadId":"39653","inReplyTo":"CALKQrgd_eB2ZUwUz54vW9OFOuZshvGGe9dx+suvd0=WA0OmOFQ@mail.gmail.com","subject":"[PATCH v4] notes: Allow treeish expressions as notes ref","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2015-07-10T01:28:31Z","receivedAt":"2015-07-10T01:28:31Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"init_notes() is the main point of entry to the notes API. It is an arbitrary\nrestriction that all it allows as input is a strict ref name, when callers\nmay want to give an arbitrary treeish.\n\nHowever, some operations that require updating the notes tree require a\nstrict ref name, because they wouldn't be able to update e.g. foo@{1}.\n\nSo we allow treeish expressions to be used in the case the notes tree is\ngoing to be used without write \"permissions\", and to distinguish whether\nthe notes tree is intended to be used for reads only, or will be updated,\na flag is added.\n\nThis has the side effect of enabling the use of treeish as notes refs in\ncommands allowing them, e.g. git log --notes=foo@{1}.\n\nSigned-off-by: Mike Hommey \nReviewed-by: Johan Herland \n---\n builtin/notes.c | 29 ++++++++++++++++-------------\n notes-cache.c | 11 ++++++-----\n notes-utils.c | 6 +++---\n notes.c | 11 +++++++----\n notes.h | 10 +++++++++-\n 5 files changed, 41 insertions(+), 26 deletions(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 63f95fc..0fc6e7a 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -285,7 +285,7 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)\n \t\tif (!c)\n \t\t\treturn 0;\n \t} else {\n-\t\tinit_notes(NULL, NULL, NULL, 0);\n+\t\tinit_notes(NULL, NULL, NULL, NOTES_INIT_WRITABLE);\n \t\tt = &default_notes_tree;\n \t}\n \n@@ -328,15 +328,18 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)\n \treturn ret;\n }\n \n-static struct notes_tree *init_notes_check(const char *subcommand)\n+static struct notes_tree *init_notes_check(const char *subcommand,\n+\t\t\t\t\t int flags)\n {\n \tstruct notes_tree *t;\n-\tinit_notes(NULL, NULL, NULL, 0);\n+\tconst char *ref;\n+\tinit_notes(NULL, NULL, NULL, flags);\n \tt = &default_notes_tree;\n \n-\tif (!starts_with(t->ref, \"refs/notes/\"))\n+\tref = (flags & NOTES_INIT_WRITABLE) ? t->update_ref : t->ref;\n+\tif (!starts_with(ref, \"refs/notes/\"))\n \t\tdie(\"Refusing to %s notes in %s (outside of refs/notes/)\",\n-\t\t subcommand, t->ref);\n+\t\t subcommand, ref);\n \treturn t;\n }\n \n@@ -359,7 +362,7 @@ static int list(int argc, const char **argv, const char *prefix)\n \t\tusage_with_options(git_notes_list_usage, options);\n \t}\n \n-\tt = init_notes_check(\"list\");\n+\tt = init_notes_check(\"list\", 0);\n \tif (argc) {\n \t\tif (get_sha1(argv[0], object))\n \t\t\tdie(_(\"Failed to resolve '%s' as a valid ref.\"), argv[0]);\n@@ -419,7 +422,7 @@ static int add(int argc, const char **argv, const char *prefix)\n \tif (get_sha1(object_ref, object))\n \t\tdie(_(\"Failed to resolve '%s' as a valid ref.\"), object_ref);\n \n-\tt = init_notes_check(\"add\");\n+\tt = init_notes_check(\"add\", NOTES_INIT_WRITABLE);\n \tnote = get_note(t, object);\n \n \tif (note) {\n@@ -510,7 +513,7 @@ static int copy(int argc, const char **argv, const char *prefix)\n \tif (get_sha1(object_ref, object))\n \t\tdie(_(\"Failed to resolve '%s' as a valid ref.\"), object_ref);\n \n-\tt = init_notes_check(\"copy\");\n+\tt = init_notes_check(\"copy\", NOTES_INIT_WRITABLE);\n \tnote = get_note(t, object);\n \n \tif (note) {\n@@ -588,7 +591,7 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \tif (get_sha1(object_ref, object))\n \t\tdie(_(\"Failed to resolve '%s' as a valid ref.\"), object_ref);\n \n-\tt = init_notes_check(argv[0]);\n+\tt = init_notes_check(argv[0], NOTES_INIT_WRITABLE);\n \tnote = get_note(t, object);\n \n \tprepare_note_data(object, &d, edit ? note : NULL);\n@@ -651,7 +654,7 @@ static int show(int argc, const char **argv, const char *prefix)\n \tif (get_sha1(object_ref, object))\n \t\tdie(_(\"Failed to resolve '%s' as a valid ref.\"), object_ref);\n \n-\tt = init_notes_check(\"show\");\n+\tt = init_notes_check(\"show\", 0);\n \tnote = get_note(t, object);\n \n \tif (!note)\n@@ -812,7 +815,7 @@ static int merge(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n-\tt = init_notes_check(\"merge\");\n+\tt = init_notes_check(\"merge\", NOTES_INIT_WRITABLE);\n \n \tstrbuf_addf(&msg, \"notes: Merged notes from %s into %s\",\n \t\t remote_ref.buf, default_notes_ref());\n@@ -878,7 +881,7 @@ static int remove_cmd(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, options,\n \t\t\t git_notes_remove_usage, 0);\n \n-\tt = init_notes_check(\"remove\");\n+\tt = init_notes_check(\"remove\", NOTES_INIT_WRITABLE);\n \n \tif (!argc && !from_stdin) {\n \t\tretval = remove_one_note(t, \"HEAD\", flag);\n@@ -920,7 +923,7 @@ static int prune(int argc, const char **argv, const char *prefix)\n \t\tusage_with_options(git_notes_prune_usage, options);\n \t}\n \n-\tt = init_notes_check(\"prune\");\n+\tt = init_notes_check(\"prune\", NOTES_INIT_WRITABLE);\n \n \tprune_notes(t, (verbose ? NOTES_PRUNE_VERBOSE : 0) |\n \t\t(show_only ? NOTES_PRUNE_VERBOSE|NOTES_PRUNE_DRYRUN : 0) );\ndiff --git a/notes-cache.c b/notes-cache.c\nindex c4e9bb7..5dfc5cb 100644\n--- a/notes-cache.c\n+++ b/notes-cache.c\n@@ -32,14 +32,14 @@ void notes_cache_init(struct notes_cache *c, const char *name,\n \t\t const char *validity)\n {\n \tstruct strbuf ref = STRBUF_INIT;\n-\tint flags = 0;\n+\tint flags = NOTES_INIT_WRITABLE;\n \n \tmemset(c, 0, sizeof(*c));\n \tc->validity = xstrdup(validity);\n \n \tstrbuf_addf(&ref, \"refs/notes/%s\", name);\n \tif (!notes_cache_match_validity(ref.buf, validity))\n-\t\tflags = NOTES_INIT_EMPTY;\n+\t\tflags |= NOTES_INIT_EMPTY;\n \tinit_notes(&c->tree, ref.buf, combine_notes_overwrite, flags);\n \tstrbuf_release(&ref);\n }\n@@ -49,7 +49,8 @@ int notes_cache_write(struct notes_cache *c)\n \tunsigned char tree_sha1[20];\n \tunsigned char commit_sha1[20];\n \n-\tif (!c || !c->tree.initialized || !c->tree.ref || !*c->tree.ref)\n+\tif (!c || !c->tree.initialized || !c->tree.update_ref ||\n+\t !*c->tree.update_ref)\n \t\treturn -1;\n \tif (!c->tree.dirty)\n \t\treturn 0;\n@@ -59,8 +60,8 @@ int notes_cache_write(struct notes_cache *c)\n \tif (commit_tree(c->validity, strlen(c->validity), tree_sha1, NULL,\n \t\t\tcommit_sha1, NULL, NULL) < 0)\n \t\treturn -1;\n-\tif (update_ref(\"update notes cache\", c->tree.ref, commit_sha1, NULL,\n-\t\t 0, UPDATE_REFS_QUIET_ON_ERR) < 0)\n+\tif (update_ref(\"update notes cache\", c->tree.update_ref, commit_sha1,\n+\t\t NULL, 0, UPDATE_REFS_QUIET_ON_ERR) < 0)\n \t\treturn -1;\n \n \treturn 0;\ndiff --git a/notes-utils.c b/notes-utils.c\nindex ccbf073..f65edba 100644\n--- a/notes-utils.c\n+++ b/notes-utils.c\n@@ -37,7 +37,7 @@ void commit_notes(struct notes_tree *t, const char *msg)\n \n \tif (!t)\n \t\tt = &default_notes_tree;\n-\tif (!t->initialized || !t->ref || !*t->ref)\n+\tif (!t->initialized || !t->update_ref || !*t->update_ref)\n \t\tdie(_(\"Cannot commit uninitialized/unreferenced notes tree\"));\n \tif (!t->dirty)\n \t\treturn; /* don't have to commit an unchanged tree */\n@@ -48,7 +48,7 @@ void commit_notes(struct notes_tree *t, const char *msg)\n \n \tcreate_notes_commit(t, NULL, buf.buf, buf.len, commit_sha1);\n \tstrbuf_insert(&buf, 0, \"notes: \", 7); /* commit message starts at index 7 */\n-\tupdate_ref(buf.buf, t->ref, commit_sha1, NULL, 0,\n+\tupdate_ref(buf.buf, t->update_ref, commit_sha1, NULL, 0,\n \t\t UPDATE_REFS_DIE_ON_ERR);\n \n \tstrbuf_release(&buf);\n@@ -130,7 +130,7 @@ struct notes_rewrite_cfg *init_copy_notes_for_rewrite(const char *cmd)\n \t\tfree(c);\n \t\treturn NULL;\n \t}\n-\tc->trees = load_notes_trees(c->refs);\n+\tc->trees = load_notes_trees(c->refs, NOTES_INIT_WRITABLE);\n \tstring_list_clear(c->refs, 0);\n \tfree(c->refs);\n \treturn c;\ndiff --git a/notes.c b/notes.c\nindex df08209..dc95ebb 100644\n--- a/notes.c\n+++ b/notes.c\n@@ -1007,13 +1007,16 @@ void init_notes(struct notes_tree *t, const char *notes_ref,\n \tt->first_non_note = NULL;\n \tt->prev_non_note = NULL;\n \tt->ref = xstrdup_or_null(notes_ref);\n+\tt->update_ref = (flags & NOTES_INIT_WRITABLE) ? t->ref : NULL;\n \tt->combine_notes = combine_notes;\n \tt->initialized = 1;\n \tt->dirty = 0;\n \n \tif (flags & NOTES_INIT_EMPTY || !notes_ref ||\n-\t read_ref(notes_ref, object_sha1))\n+\t get_sha1_treeish(notes_ref, object_sha1))\n \t\treturn;\n+\tif (flags & NOTES_INIT_WRITABLE && read_ref(notes_ref, object_sha1))\n+\t\tdie(\"Cannot use notes ref %s\", notes_ref);\n \tif (get_tree_entry(object_sha1, \"\", sha1, &mode))\n \t\tdie(\"Failed to read notes tree referenced by %s (%s)\",\n \t\t notes_ref, sha1_to_hex(object_sha1));\n@@ -1023,7 +1026,7 @@ void init_notes(struct notes_tree *t, const char *notes_ref,\n \tload_subtree(t, &root_tree, t->root, 0);\n }\n \n-struct notes_tree **load_notes_trees(struct string_list *refs)\n+struct notes_tree **load_notes_trees(struct string_list *refs, int flags)\n {\n \tstruct string_list_item *item;\n \tint counter = 0;\n@@ -1031,7 +1034,7 @@ struct notes_tree **load_notes_trees(struct string_list *refs)\n \ttrees = xmalloc((refs->nr+1) * sizeof(struct notes_tree *));\n \tfor_each_string_list_item(item, refs) {\n \t\tstruct notes_tree *t = xcalloc(1, sizeof(struct notes_tree));\n-\t\tinit_notes(t, item->string, combine_notes_ignore, 0);\n+\t\tinit_notes(t, item->string, combine_notes_ignore, flags);\n \t\ttrees[counter++] = t;\n \t}\n \ttrees[counter] = NULL;\n@@ -1067,7 +1070,7 @@ void init_display_notes(struct display_notes_opt *opt)\n \t\t\t\t\t\t item->string);\n \t}\n \n-\tdisplay_notes_trees = load_notes_trees(&display_notes_refs);\n+\tdisplay_notes_trees = load_notes_trees(&display_notes_refs, 0);\n \tstring_list_clear(&display_notes_refs, 0);\n }\n \ndiff --git a/notes.h b/notes.h\nindex 2a3f923..e5d67fd 100644\n--- a/notes.h\n+++ b/notes.h\n@@ -44,6 +44,7 @@ extern struct notes_tree {\n \tstruct int_node *root;\n \tstruct non_note *first_non_note, *prev_non_note;\n \tchar *ref;\n+\tchar *update_ref;\n \tcombine_notes_fn combine_notes;\n \tint initialized;\n \tint dirty;\n@@ -72,6 +73,13 @@ const char *default_notes_ref(void);\n #define NOTES_INIT_EMPTY 1\n \n /*\n+ * By default, the notes tree is only readable, and the notes ref can be\n+ * any treeish. The notes tree can however be made writable with this flag,\n+ * in which case only strict ref names can be used.\n+ */\n+#define NOTES_INIT_WRITABLE 2\n+\n+/*\n * Initialize the given notes_tree with the notes tree structure at the given\n * ref. If given ref is NULL, the value of the $GIT_NOTES_REF environment\n * variable is used, and if that is missing, the default notes ref is used\n@@ -276,7 +284,7 @@ void format_display_notes(const unsigned char *object_sha1,\n * Load the notes tree from each ref listed in 'refs'. The output is\n * an array of notes_tree*, terminated by a NULL.\n */\n-struct notes_tree **load_notes_trees(struct string_list *refs);\n+struct notes_tree **load_notes_trees(struct string_list *refs, int flags);\n \n /*\n * Add all refs that match 'glob' to the 'list'.\n-- \n2.4.3.3.g2dde64c\n"},{"id":"265971","messageId":"CALKQrgdGJy6vtBRL413bbSHSi+=KTh4Q98hpbgg29j4J191=bA@mail.gmail.com","threadId":"39653","inReplyTo":"1436491711-19056-1-git-send-email-mh@glandium.org","subject":"Re: [PATCH v4] notes: Allow treeish expressions as notes ref","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2015-07-10T07:16:16Z","receivedAt":"2015-07-10T07:16:16Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Fri, Jul 10, 2015 at 3:28 AM, Mike Hommey wrote:\n> init_notes() is the main point of entry to the notes API. It is an arbitrary\n> restriction that all it allows as input is a strict ref name, when callers\n> may want to give an arbitrary treeish.\n>\n> However, some operations that require updating the notes tree require a\n> strict ref name, because they wouldn't be able to update e.g. foo@{1}.\n>\n> So we allow treeish expressions to be used in the case the notes tree is\n> going to be used without write \"permissions\", and to distinguish whether\n> the notes tree is intended to be used for reads only, or will be updated,\n> a flag is added.\n>\n> This has the side effect of enabling the use of treeish as notes refs in\n> commands allowing them, e.g. git log --notes=foo@{1}.\n\nLooks good. However, on a second pass I noticed that the patch comes\nwith no tests. I'd like at least a couple of tests thrown in there to verify\ncorrectness; e.g. reading notes from refs/notes/commits^{tree} shall\nsucceed, and trying to write notes to refs/notes/commits^{tree} shall fail.\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"265972","messageId":"20150710072228.GA5302@glandium.org","threadId":"39653","inReplyTo":"CALKQrgdGJy6vtBRL413bbSHSi+=KTh4Q98hpbgg29j4J191=bA@mail.gmail.com","subject":"Re: [PATCH v4] notes: Allow treeish expressions as notes ref","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2015-07-10T07:22:28Z","receivedAt":"2015-07-10T07:22:28Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Fri, Jul 10, 2015 at 09:16:16AM +0200, Johan Herland wrote:\n> On Fri, Jul 10, 2015 at 3:28 AM, Mike Hommey wrote:\n> > init_notes() is the main point of entry to the notes API. It is an arbitrary\n> > restriction that all it allows as input is a strict ref name, when callers\n> > may want to give an arbitrary treeish.\n> >\n> > However, some operations that require updating the notes tree require a\n> > strict ref name, because they wouldn't be able to update e.g. foo@{1}.\n> >\n> > So we allow treeish expressions to be used in the case the notes tree is\n> > going to be used without write \"permissions\", and to distinguish whether\n> > the notes tree is intended to be used for reads only, or will be updated,\n> > a flag is added.\n> >\n> > This has the side effect of enabling the use of treeish as notes refs in\n> > commands allowing them, e.g. git log --notes=foo@{1}.\n> \n> Looks good. However, on a second pass I noticed that the patch comes\n> with no tests. I'd like at least a couple of tests thrown in there to verify\n> correctness; e.g. reading notes from refs/notes/commits^{tree} shall\n> succeed, and trying to write notes to refs/notes/commits^{tree} shall fail.\n\nFair enough. Will update.\n\nMike\n"},{"id":"265976","messageId":"1436517551-12172-1-git-send-email-mh@glandium.org","threadId":"39653","inReplyTo":"CALKQrgdGJy6vtBRL413bbSHSi+=KTh4Q98hpbgg29j4J191=bA@mail.gmail.com","subject":"[PATCH] notes: Allow treeish expressions as notes ref","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2015-07-10T08:39:11Z","receivedAt":"2015-07-10T08:39:11Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"init_notes() is the main point of entry to the notes API. It is an arbitrary\nrestriction that all it allows as input is a strict ref name, when callers\nmay want to give an arbitrary treeish.\n\nHowever, some operations that require updating the notes tree require a\nstrict ref name, because they wouldn't be able to update e.g. foo@{1}.\n\nSo we allow treeish expressions to be used in the case the notes tree is\ngoing to be used without write \"permissions\", and to distinguish whether\nthe notes tree is intended to be used for reads only, or will be updated,\na flag is added.\n\nThis has the side effect of enabling the use of treeish as notes refs in\ncommands allowing them, e.g. git log --notes=foo@{1}.\n\nSigned-off-by: Mike Hommey \n---\n builtin/notes.c | 29 ++++++++++++++++-------------\n notes-cache.c | 11 ++++++-----\n notes-utils.c | 6 +++---\n notes.c | 11 +++++++----\n notes.h | 10 +++++++++-\n t/t3301-notes.sh | 10 ++++++++++\n 6 files changed, 51 insertions(+), 26 deletions(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 63f95fc..0fc6e7a 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -285,7 +285,7 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)\n \t\tif (!c)\n \t\t\treturn 0;\n \t} else {\n-\t\tinit_notes(NULL, NULL, NULL, 0);\n+\t\tinit_notes(NULL, NULL, NULL, NOTES_INIT_WRITABLE);\n \t\tt = &default_notes_tree;\n \t}\n \n@@ -328,15 +328,18 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)\n \treturn ret;\n }\n \n-static struct notes_tree *init_notes_check(const char *subcommand)\n+static struct notes_tree *init_notes_check(const char *subcommand,\n+\t\t\t\t\t int flags)\n {\n \tstruct notes_tree *t;\n-\tinit_notes(NULL, NULL, NULL, 0);\n+\tconst char *ref;\n+\tinit_notes(NULL, NULL, NULL, flags);\n \tt = &default_notes_tree;\n \n-\tif (!starts_with(t->ref, \"refs/notes/\"))\n+\tref = (flags & NOTES_INIT_WRITABLE) ? t->update_ref : t->ref;\n+\tif (!starts_with(ref, \"refs/notes/\"))\n \t\tdie(\"Refusing to %s notes in %s (outside of refs/notes/)\",\n-\t\t subcommand, t->ref);\n+\t\t subcommand, ref);\n \treturn t;\n }\n \n@@ -359,7 +362,7 @@ static int list(int argc, const char **argv, const char *prefix)\n \t\tusage_with_options(git_notes_list_usage, options);\n \t}\n \n-\tt = init_notes_check(\"list\");\n+\tt = init_notes_check(\"list\", 0);\n \tif (argc) {\n \t\tif (get_sha1(argv[0], object))\n \t\t\tdie(_(\"Failed to resolve '%s' as a valid ref.\"), argv[0]);\n@@ -419,7 +422,7 @@ static int add(int argc, const char **argv, const char *prefix)\n \tif (get_sha1(object_ref, object))\n \t\tdie(_(\"Failed to resolve '%s' as a valid ref.\"), object_ref);\n \n-\tt = init_notes_check(\"add\");\n+\tt = init_notes_check(\"add\", NOTES_INIT_WRITABLE);\n \tnote = get_note(t, object);\n \n \tif (note) {\n@@ -510,7 +513,7 @@ static int copy(int argc, const char **argv, const char *prefix)\n \tif (get_sha1(object_ref, object))\n \t\tdie(_(\"Failed to resolve '%s' as a valid ref.\"), object_ref);\n \n-\tt = init_notes_check(\"copy\");\n+\tt = init_notes_check(\"copy\", NOTES_INIT_WRITABLE);\n \tnote = get_note(t, object);\n \n \tif (note) {\n@@ -588,7 +591,7 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \tif (get_sha1(object_ref, object))\n \t\tdie(_(\"Failed to resolve '%s' as a valid ref.\"), object_ref);\n \n-\tt = init_notes_check(argv[0]);\n+\tt = init_notes_check(argv[0], NOTES_INIT_WRITABLE);\n \tnote = get_note(t, object);\n \n \tprepare_note_data(object, &d, edit ? note : NULL);\n@@ -651,7 +654,7 @@ static int show(int argc, const char **argv, const char *prefix)\n \tif (get_sha1(object_ref, object))\n \t\tdie(_(\"Failed to resolve '%s' as a valid ref.\"), object_ref);\n \n-\tt = init_notes_check(\"show\");\n+\tt = init_notes_check(\"show\", 0);\n \tnote = get_note(t, object);\n \n \tif (!note)\n@@ -812,7 +815,7 @@ static int merge(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n-\tt = init_notes_check(\"merge\");\n+\tt = init_notes_check(\"merge\", NOTES_INIT_WRITABLE);\n \n \tstrbuf_addf(&msg, \"notes: Merged notes from %s into %s\",\n \t\t remote_ref.buf, default_notes_ref());\n@@ -878,7 +881,7 @@ static int remove_cmd(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, options,\n \t\t\t git_notes_remove_usage, 0);\n \n-\tt = init_notes_check(\"remove\");\n+\tt = init_notes_check(\"remove\", NOTES_INIT_WRITABLE);\n \n \tif (!argc && !from_stdin) {\n \t\tretval = remove_one_note(t, \"HEAD\", flag);\n@@ -920,7 +923,7 @@ static int prune(int argc, const char **argv, const char *prefix)\n \t\tusage_with_options(git_notes_prune_usage, options);\n \t}\n \n-\tt = init_notes_check(\"prune\");\n+\tt = init_notes_check(\"prune\", NOTES_INIT_WRITABLE);\n \n \tprune_notes(t, (verbose ? NOTES_PRUNE_VERBOSE : 0) |\n \t\t(show_only ? NOTES_PRUNE_VERBOSE|NOTES_PRUNE_DRYRUN : 0) );\ndiff --git a/notes-cache.c b/notes-cache.c\nindex c4e9bb7..5dfc5cb 100644\n--- a/notes-cache.c\n+++ b/notes-cache.c\n@@ -32,14 +32,14 @@ void notes_cache_init(struct notes_cache *c, const char *name,\n \t\t const char *validity)\n {\n \tstruct strbuf ref = STRBUF_INIT;\n-\tint flags = 0;\n+\tint flags = NOTES_INIT_WRITABLE;\n \n \tmemset(c, 0, sizeof(*c));\n \tc->validity = xstrdup(validity);\n \n \tstrbuf_addf(&ref, \"refs/notes/%s\", name);\n \tif (!notes_cache_match_validity(ref.buf, validity))\n-\t\tflags = NOTES_INIT_EMPTY;\n+\t\tflags |= NOTES_INIT_EMPTY;\n \tinit_notes(&c->tree, ref.buf, combine_notes_overwrite, flags);\n \tstrbuf_release(&ref);\n }\n@@ -49,7 +49,8 @@ int notes_cache_write(struct notes_cache *c)\n \tunsigned char tree_sha1[20];\n \tunsigned char commit_sha1[20];\n \n-\tif (!c || !c->tree.initialized || !c->tree.ref || !*c->tree.ref)\n+\tif (!c || !c->tree.initialized || !c->tree.update_ref ||\n+\t !*c->tree.update_ref)\n \t\treturn -1;\n \tif (!c->tree.dirty)\n \t\treturn 0;\n@@ -59,8 +60,8 @@ int notes_cache_write(struct notes_cache *c)\n \tif (commit_tree(c->validity, strlen(c->validity), tree_sha1, NULL,\n \t\t\tcommit_sha1, NULL, NULL) < 0)\n \t\treturn -1;\n-\tif (update_ref(\"update notes cache\", c->tree.ref, commit_sha1, NULL,\n-\t\t 0, UPDATE_REFS_QUIET_ON_ERR) < 0)\n+\tif (update_ref(\"update notes cache\", c->tree.update_ref, commit_sha1,\n+\t\t NULL, 0, UPDATE_REFS_QUIET_ON_ERR) < 0)\n \t\treturn -1;\n \n \treturn 0;\ndiff --git a/notes-utils.c b/notes-utils.c\nindex ccbf073..f65edba 100644\n--- a/notes-utils.c\n+++ b/notes-utils.c\n@@ -37,7 +37,7 @@ void commit_notes(struct notes_tree *t, const char *msg)\n \n \tif (!t)\n \t\tt = &default_notes_tree;\n-\tif (!t->initialized || !t->ref || !*t->ref)\n+\tif (!t->initialized || !t->update_ref || !*t->update_ref)\n \t\tdie(_(\"Cannot commit uninitialized/unreferenced notes tree\"));\n \tif (!t->dirty)\n \t\treturn; /* don't have to commit an unchanged tree */\n@@ -48,7 +48,7 @@ void commit_notes(struct notes_tree *t, const char *msg)\n \n \tcreate_notes_commit(t, NULL, buf.buf, buf.len, commit_sha1);\n \tstrbuf_insert(&buf, 0, \"notes: \", 7); /* commit message starts at index 7 */\n-\tupdate_ref(buf.buf, t->ref, commit_sha1, NULL, 0,\n+\tupdate_ref(buf.buf, t->update_ref, commit_sha1, NULL, 0,\n \t\t UPDATE_REFS_DIE_ON_ERR);\n \n \tstrbuf_release(&buf);\n@@ -130,7 +130,7 @@ struct notes_rewrite_cfg *init_copy_notes_for_rewrite(const char *cmd)\n \t\tfree(c);\n \t\treturn NULL;\n \t}\n-\tc->trees = load_notes_trees(c->refs);\n+\tc->trees = load_notes_trees(c->refs, NOTES_INIT_WRITABLE);\n \tstring_list_clear(c->refs, 0);\n \tfree(c->refs);\n \treturn c;\ndiff --git a/notes.c b/notes.c\nindex 2be4d7f..25ed506 100644\n--- a/notes.c\n+++ b/notes.c\n@@ -1007,13 +1007,16 @@ void init_notes(struct notes_tree *t, const char *notes_ref,\n \tt->first_non_note = NULL;\n \tt->prev_non_note = NULL;\n \tt->ref = xstrdup_or_null(notes_ref);\n+\tt->update_ref = (flags & NOTES_INIT_WRITABLE) ? t->ref : NULL;\n \tt->combine_notes = combine_notes;\n \tt->initialized = 1;\n \tt->dirty = 0;\n \n \tif (flags & NOTES_INIT_EMPTY || !notes_ref ||\n-\t read_ref(notes_ref, object_sha1))\n+\t get_sha1_treeish(notes_ref, object_sha1))\n \t\treturn;\n+\tif (flags & NOTES_INIT_WRITABLE && read_ref(notes_ref, object_sha1))\n+\t\tdie(\"Cannot use notes ref %s\", notes_ref);\n \tif (get_tree_entry(object_sha1, \"\", sha1, &mode))\n \t\tdie(\"Failed to read notes tree referenced by %s (%s)\",\n \t\t notes_ref, sha1_to_hex(object_sha1));\n@@ -1023,7 +1026,7 @@ void init_notes(struct notes_tree *t, const char *notes_ref,\n \tload_subtree(t, &root_tree, t->root, 0);\n }\n \n-struct notes_tree **load_notes_trees(struct string_list *refs)\n+struct notes_tree **load_notes_trees(struct string_list *refs, int flags)\n {\n \tstruct string_list_item *item;\n \tint counter = 0;\n@@ -1031,7 +1034,7 @@ struct notes_tree **load_notes_trees(struct string_list *refs)\n \ttrees = xmalloc((refs->nr+1) * sizeof(struct notes_tree *));\n \tfor_each_string_list_item(item, refs) {\n \t\tstruct notes_tree *t = xcalloc(1, sizeof(struct notes_tree));\n-\t\tinit_notes(t, item->string, combine_notes_ignore, 0);\n+\t\tinit_notes(t, item->string, combine_notes_ignore, flags);\n \t\ttrees[counter++] = t;\n \t}\n \ttrees[counter] = NULL;\n@@ -1067,7 +1070,7 @@ void init_display_notes(struct display_notes_opt *opt)\n \t\t\t\t\t\t item->string);\n \t}\n \n-\tdisplay_notes_trees = load_notes_trees(&display_notes_refs);\n+\tdisplay_notes_trees = load_notes_trees(&display_notes_refs, 0);\n \tstring_list_clear(&display_notes_refs, 0);\n }\n \ndiff --git a/notes.h b/notes.h\nindex 2a3f923..e5d67fd 100644\n--- a/notes.h\n+++ b/notes.h\n@@ -44,6 +44,7 @@ extern struct notes_tree {\n \tstruct int_node *root;\n \tstruct non_note *first_non_note, *prev_non_note;\n \tchar *ref;\n+\tchar *update_ref;\n \tcombine_notes_fn combine_notes;\n \tint initialized;\n \tint dirty;\n@@ -72,6 +73,13 @@ const char *default_notes_ref(void);\n #define NOTES_INIT_EMPTY 1\n \n /*\n+ * By default, the notes tree is only readable, and the notes ref can be\n+ * any treeish. The notes tree can however be made writable with this flag,\n+ * in which case only strict ref names can be used.\n+ */\n+#define NOTES_INIT_WRITABLE 2\n+\n+/*\n * Initialize the given notes_tree with the notes tree structure at the given\n * ref. If given ref is NULL, the value of the $GIT_NOTES_REF environment\n * variable is used, and if that is missing, the default notes ref is used\n@@ -276,7 +284,7 @@ void format_display_notes(const unsigned char *object_sha1,\n * Load the notes tree from each ref listed in 'refs'. The output is\n * an array of notes_tree*, terminated by a NULL.\n */\n-struct notes_tree **load_notes_trees(struct string_list *refs);\n+struct notes_tree **load_notes_trees(struct string_list *refs, int flags);\n \n /*\n * Add all refs that match 'glob' to the 'list'.\ndiff --git a/t/t3301-notes.sh b/t/t3301-notes.sh\nindex 8cffd35..06b4584 100755\n--- a/t/t3301-notes.sh\n+++ b/t/t3301-notes.sh\n@@ -83,6 +83,16 @@ test_expect_success 'edit existing notes' '\n \ttest_must_fail git notes show HEAD^\n '\n \n+test_expect_success 'show notes from treeish' '\n+\ttest \"b3\" = \"$(git notes --ref commits^{tree} show)\" &&\n+\ttest \"b4\" = \"$(git notes --ref commits@{1} show)\"\n+'\n+\n+test_expect_success 'cannot edit notes from non-ref' '\n+\ttest_must_fail git notes --ref commits^{tree} edit &&\n+\ttest_must_fail git notes --ref commits@{1} edit\n+'\n+\n test_expect_success 'cannot \"git notes add -m\" where notes already exists' '\n \ttest_must_fail git notes add -m \"b2\" &&\n \ttest_path_is_missing .git/NOTES_EDITMSG &&\n-- \n2.4.3.3.g2dde64c.dirty\n"},{"id":"266086","messageId":"xmqqzj30yq03.fsf@gitster.dls.corp.google.com","threadId":"39653","inReplyTo":"1436517551-12172-1-git-send-email-mh@glandium.org","subject":"Re: [PATCH] notes: Allow treeish expressions as notes ref","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-13T16:35:40Z","receivedAt":"2015-07-13T16:35:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Hommey writes:\n\n> init_notes() is the main point of entry to the notes API. It is an arbitrary\n> restriction that all it allows as input is a strict ref name, when callers\n> may want to give an arbitrary treeish.\n>\n> However, some operations that require updating the notes tree require a\n> strict ref name, because they wouldn't be able to update e.g. foo@{1}.\n>\n> So we allow treeish expressions to be used in the case the notes tree is\n> going to be used without write \"permissions\", and to distinguish whether\n> the notes tree is intended to be used for reads only, or will be updated,\n> a flag is added.\n\nIt is unfair to call the current check arbitrary, though. From the\npoint of view of the person who views notes as a database, it is\nentirely a sensible thing to treat it as an read/write data store.\n\n> This has the side effect of enabling the use of treeish as notes refs in\n> commands allowing them, e.g. git log --notes=foo@{1}.\n\nI do not think it is a \"side effect\". It's the primary benefit this\nchange brings in.\n\nI'd flip the attitude around and sell this as \"an enhancement\",\nperhaps like this:\n\n notes: allow treeish expressions as notes ref\n \n init_notes() is the main point of entry to the notes API. It ensures\n that the input can be used as ref, because it needs a ref to update\n to store notes tree after modifying it.\n \n There however are many use cases where notes tree is only read, e.g.\n \"git log --notes=...\". Any notes-shaped treeish could be used for\n such purpose, but it is not allowed due to existing restriction.\n \n Allow treeish expressions to be used in the case the notes tree is\n going to be used without write \"permissions\". Add a flag to\n distinguish whether the notes tree is intended to be used read-only,\n or will be updated.\n \n With this change, operations that use notes read-only can be fed any\n notes-shaped tree-ish can be used, e.g. git log --notes=notes@{1}.\n \n Signed-off-by: Mike Hommey \n Signed-off-by: Junio C Hamano \n\n\nOne thing I found puzzling is that the \"arbitrary restriction\" does\nnot seem to do anything visibly \"restricting\".\n\nI taught my copy of \"git am\" to add the message-ID of the original\npatch to the resulting commit, and that is kept in \"amlog\" notes.\n\nWithout this patch, I am getting this:\n\n $ git log --notes=amlog -1 --pretty=short\n commit 7a3744f95d4d21b27470e5bacaed8c434150ecac\n Author: Mike Hommey \n\n notes: allow treeish expressions as notes ref\n\n Notes (amlog):\n Message-Id: <1436517551-12172-1-git-send-email-mh@glandium.org>\n\nThe above is very much expected. Now without your patch, I get\nthis:\n\n $ git log --notes='amlog@{1}' -1 --pretty=short\n commit 7a3744f95d4d21b27470e5bacaed8c434150ecac\n Author: Mike Hommey \n\n notes: allow treeish expressions as notes ref\n\nIt seems to be ignoring what is not a refname without complaining.\nHowever, that does not seem to be the whole story. Look:\n\n $ git log --notes='amlog@{1' -1 --pretty=short\n warning: notes ref refs/notes/amlog@{1 is invalid\n commit 7a3744f95d4d21b27470e5bacaed8c434150ecac\n Author: Mike Hommey \n\n notes: allow treeish expressions as notes ref\n\n $ git log --notes='amlog@{5000}' -1 --pretty=short\n fatal: Log for 'refs/notes/amlog' only has 3072 entries.\n\nSo sometimes we silently ignore, sometimes we warn, and sometimes\nwe refuse to work. The above are all without your patch applied.\n\nWith your patch, the only change I see is this:\n\n $ git log --notes=amlog@{0} -1 --pretty=short\n commit 7a3744f95d4d21b27470e5bacaed8c434150ecac\n Author: Mike Hommey \n\n notes: allow treeish expressions as notes ref\n\n Notes (amlog@{0}):\n Message-Id: <1436517551-12172-1-git-send-email-mh@glandium.org>\n\nWe read from a tree-ish ;-) Whee!\n\nWhat is interesting is to think what should happen when amlog@{1}\nis given. The version with your patch gives the same output as we\nsaw earlier, because amlog@{1} tree exists, but does not know about\nyour patch (yet), so does not add \"Notes\" section, which makes sense\nby itself.\n\nBut we cannot tell if amlog@{1} was somehow malformed or it was OK\nand there was no notes on the commit.\n\nWe probably should do a few more things:\n\n - Make sure that we show \"there is no such tree-ish, no way to look\n up any note to any commit from there\" and \"I understood the tree\n you gave me, but there is no note for that commit\" differently.\n\n - Decide if we want to \"fail\" the operation when the notes tree\n given by the user is not even a tree-ish or just \"warn\" and keep\n going. And do so consistently.\n\nThanks.\n"},{"id":"266093","messageId":"20150713205304.GA26911@glandium.org","threadId":"39653","inReplyTo":"xmqqzj30yq03.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] notes: Allow treeish expressions as notes ref","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2015-07-13T20:53:04Z","receivedAt":"2015-07-13T20:53:04Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Mon, Jul 13, 2015 at 09:35:40AM -0700, Junio C Hamano wrote:\n> Mike Hommey writes:\n> \n> > init_notes() is the main point of entry to the notes API. It is an arbitrary\n> > restriction that all it allows as input is a strict ref name, when callers\n> > may want to give an arbitrary treeish.\n> >\n> > However, some operations that require updating the notes tree require a\n> > strict ref name, because they wouldn't be able to update e.g. foo@{1}.\n> >\n> > So we allow treeish expressions to be used in the case the notes tree is\n> > going to be used without write \"permissions\", and to distinguish whether\n> > the notes tree is intended to be used for reads only, or will be updated,\n> > a flag is added.\n> \n> It is unfair to call the current check arbitrary, though. From the\n> point of view of the person who views notes as a database, it is\n> entirely a sensible thing to treat it as an read/write data store.\n> \n> > This has the side effect of enabling the use of treeish as notes refs in\n> > commands allowing them, e.g. git log --notes=foo@{1}.\n> \n> I do not think it is a \"side effect\". It's the primary benefit this\n> change brings in.\n\nMy original motivation for this change was for init_notes() to take a\ncommittish so that I can feed it with one in a C program that uses the\nnotes API. So from that perspective, that command lines can now use them\nis a side effect.\n\n> I'd flip the attitude around and sell this as \"an enhancement\",\n> perhaps like this:\n> \n> notes: allow treeish expressions as notes ref\n> \n> init_notes() is the main point of entry to the notes API. It ensures\n> that the input can be used as ref, because it needs a ref to update\n> to store notes tree after modifying it.\n> \n> There however are many use cases where notes tree is only read, e.g.\n> \"git log --notes=...\". Any notes-shaped treeish could be used for\n> such purpose, but it is not allowed due to existing restriction.\n> \n> Allow treeish expressions to be used in the case the notes tree is\n> going to be used without write \"permissions\". Add a flag to\n> distinguish whether the notes tree is intended to be used read-only,\n> or will be updated.\n> \n> With this change, operations that use notes read-only can be fed any\n> notes-shaped tree-ish can be used, e.g. git log --notes=notes@{1}.\n> \n> Signed-off-by: Mike Hommey \n> Signed-off-by: Junio C Hamano \n\nWFM.\n\n> We probably should do a few more things:\n> \n> - Make sure that we show \"there is no such tree-ish, no way to look\n> up any note to any commit from there\" and \"I understood the tree\n> you gave me, but there is no note for that commit\" differently.\n\nHow would you reconcile that with the usual \"there are only a couple\ncommits with a note in the hundreds you make me display\"?\n\n> - Decide if we want to \"fail\" the operation when the notes tree\n> given by the user is not even a tree-ish or just \"warn\" and keep\n> going. And do so consistently.\n\nIs this something you want to be figured before merging this patch?\n\nMike\n"},{"id":"266094","messageId":"xmqqio9nzrlh.fsf@gitster.dls.corp.google.com","threadId":"39653","inReplyTo":"20150713205304.GA26911@glandium.org","subject":"Re: [PATCH] notes: Allow treeish expressions as notes ref","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-13T21:15:54Z","receivedAt":"2015-07-13T21:15:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Hommey writes:\n\n>> - Make sure that we show \"there is no such tree-ish, no way to look\n>> up any note to any commit from there\" and \"I understood the tree\n>> you gave me, but there is no note for that commit\" differently.\n>\n> How would you reconcile that with the usual \"there are only a couple\n> commits with a note in the hundreds you make me display\"?\n\nI am talking about the difference between \"a tree exists (which may\nlack notes for a given commit)\" and \"a tree does not even exist in\nthe first place\". This patch removed \"a tree exists but that is not\na ref so we silently ignore\", but I do not know if that change alone\ncovers everything---do you?\n\n>> - Decide if we want to \"fail\" the operation when the notes tree\n>> given by the user is not even a tree-ish or just \"warn\" and keep\n>> going. And do so consistently.\n>\n> Is this something you want to be figured before merging this patch?\n\nDepends on the definition of 'merging'. I queued this one on 'pu',\nand have no intention to merge it down to 'master' by the end of\nthis month; in the meantime either incremental or replacement\nrefinement can certainly address that inconsistency I'd hope ;-)\n\nThanks.\n"},{"id":"266095","messageId":"xmqqegkbzrf6.fsf@gitster.dls.corp.google.com","threadId":"39653","inReplyTo":"1436517551-12172-1-git-send-email-mh@glandium.org","subject":"Re: [PATCH] notes: Allow treeish expressions as notes ref","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-13T21:19:41Z","receivedAt":"2015-07-13T21:19:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Hommey writes:\n\n> init_notes() is the main point of entry to the notes API. It is an arbitrary\n> restriction that all it allows as input is a strict ref name, when callers\n> may want to give an arbitrary treeish.\n>\n> However, some operations that require updating the notes tree require a\n> strict ref name, because they wouldn't be able to update e.g. foo@{1}.\n>\n> So we allow treeish expressions to be used in the case the notes tree is\n> going to be used without write \"permissions\", and to distinguish whether\n> the notes tree is intended to be used for reads only, or will be updated,\n> a flag is added.\n>\n> This has the side effect of enabling the use of treeish as notes refs in\n> commands allowing them, e.g. git log --notes=foo@{1}.\n>\n> Signed-off-by: Mike Hommey \n> ---\n> builtin/notes.c | 29 ++++++++++++++++-------------\n> notes-cache.c | 11 ++++++-----\n> notes-utils.c | 6 +++---\n> notes.c | 11 +++++++----\n> notes.h | 10 +++++++++-\n> t/t3301-notes.sh | 10 ++++++++++\n> 6 files changed, 51 insertions(+), 26 deletions(-)\n\nAt least Documentation/pretty-options.txt needs to be updated, as it\nexplicitly requests you to feed a ref, but you'd want to tell the\nusers that you loosened it. I suspect Documentation/git-notes.txt\nmay also need adjustment as that involves writing side, but I didn't\nlook very carefully.\n\nThanks.\n"},{"id":"271291","messageId":"20151008025018.GA29722@glandium.org","threadId":"39653","inReplyTo":"xmqqzj30yq03.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] notes: Allow treeish expressions as notes ref","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2015-10-08T02:50:18Z","receivedAt":"2015-10-08T02:50:18Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Mon, Jul 13, 2015 at 09:35:40AM -0700, Junio C Hamano wrote:\n> What is interesting is to think what should happen when amlog@{1}\n> is given. The version with your patch gives the same output as we\n> saw earlier, because amlog@{1} tree exists, but does not know about\n> your patch (yet), so does not add \"Notes\" section, which makes sense\n> by itself.\n> \n> But we cannot tell if amlog@{1} was somehow malformed or it was OK\n> and there was no notes on the commit.\n> \n> We probably should do a few more things:\n> \n> - Make sure that we show \"there is no such tree-ish, no way to look\n> up any note to any commit from there\" and \"I understood the tree\n> you gave me, but there is no note for that commit\" differently.\n\nAfter refreshing the patch against current \"next\", it appears that\nthere is such a distinction:\n\n$ ./git-log --notes=fdsfgsfdg HEAD^! --pretty=short\nwarning: notes ref refs/notes/fdsfgsfdg is invalid\ncommit e5b68b2e879608d881c2e3600ce84962fcdefc88\nAuthor: Mike Hommey \n\n notes: allow treeish expressions as notes ref\n\n$ ./git-log --notes=foo HEAD^! --pretty=short\ncommit e5b68b2e879608d881c2e3600ce84962fcdefc88\nAuthor: Mike Hommey \n\n notes: allow treeish expressions as notes ref\n\n(I do have a refs/notes/foo ref)\n\n> - Decide if we want to \"fail\" the operation when the notes tree\n> given by the user is not even a tree-ish or just \"warn\" and keep\n> going. And do so consistently.\n\nCurrently, it warns and doesn't fail.\n\nCheers,\n\nMike\n"},{"id":"271292","messageId":"1444272883-1897-1-git-send-email-mh@glandium.org","threadId":"39653","inReplyTo":"20151008025018.GA29722@glandium.org","subject":"[PATCH v6] notes: allow treeish expressions as notes ref","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2015-10-08T02:54:43Z","receivedAt":"2015-10-08T02:54:43Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"init_notes() is the main point of entry to the notes API. It ensures\nthat the input can be used as ref, because it needs a ref to update to\nstore notes tree after modifying it.\n\nThere however are many use cases where notes tree is only read, e.g.\n\"git log --notes=...\". Any notes-shaped treeish could be used for such\npurpose, but it is not allowed due to existing restriction.\n\nAllow treeish expressions to be used in the case the notes tree is going\nto be used without write \"permissions\". Add a flag to distinguish\nwhether the notes tree is intended to be used read-only, or will be\nupdated.\n\nWith this change, operations that use notes read-only can be fed any\nnotes-shaped tree-ish can be used, e.g. git log --notes=notes@{1}.\n\nSigned-off-by: Mike Hommey \n---\n\nI'm really not satisfied with the documentation change, but I don't have\na better idea :(. It didn't feel like Documentation/git-notes.txt needed\nan update.\n\n Documentation/pretty-options.txt | 8 ++++----\n builtin/notes.c | 29 ++++++++++++++++-------------\n notes-cache.c | 11 ++++++-----\n notes-utils.c | 6 +++---\n notes.c | 11 +++++++----\n notes.h | 10 +++++++++-\n t/t3301-notes.sh | 10 ++++++++++\n 7 files changed, 55 insertions(+), 30 deletions(-)\n\ndiff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt\nindex 4b659ac..54b88b6 100644\n--- a/Documentation/pretty-options.txt\n+++ b/Documentation/pretty-options.txt\n@@ -43,7 +43,7 @@ people using 80-column terminals.\n \tcommit may be copied to the output.\n \n ifndef::git-rev-list[]\n---notes[=]::\n+--notes[=]::\n \tShow the notes (see linkgit:git-notes[1]) that annotate the\n \tcommit, when showing the commit log message. This is the default\n \tfor `git log`, `git show` and `git whatchanged` commands when\n@@ -54,8 +54,8 @@ By default, the notes shown are from the notes refs listed in the\n 'core.notesRef' and 'notes.displayRef' variables (or corresponding\n environment overrides). See linkgit:git-config[1] for more details.\n +\n-With an optional '' argument, show this notes ref instead of the\n-default notes ref(s). The ref specifies the full refname when it begins\n+With an optional '' argument, use the treeish to find the notes\n+to display. The treeish can specify the full refname when it begins\n with `refs/notes/`; when it begins with `notes/`, `refs/` and otherwise\n `refs/notes/` is prefixed to form a full name of the ref.\n +\n@@ -71,7 +71,7 @@ being displayed. Examples: \"--notes=foo\" will show only notes from\n \t\"--notes --notes=foo --no-notes --notes=bar\" will only show notes\n \tfrom \"refs/notes/bar\".\n \n---show-notes[=]::\n+--show-notes[=]::\n --[no-]standard-notes::\n \tThese options are deprecated. Use the above --notes/--no-notes\n \toptions instead.\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 3608c64..6371d53 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -286,7 +286,7 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)\n \t\tif (!c)\n \t\t\treturn 0;\n \t} else {\n-\t\tinit_notes(NULL, NULL, NULL, 0);\n+\t\tinit_notes(NULL, NULL, NULL, NOTES_INIT_WRITABLE);\n \t\tt = &default_notes_tree;\n \t}\n \n@@ -329,15 +329,18 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)\n \treturn ret;\n }\n \n-static struct notes_tree *init_notes_check(const char *subcommand)\n+static struct notes_tree *init_notes_check(const char *subcommand,\n+\t\t\t\t\t int flags)\n {\n \tstruct notes_tree *t;\n-\tinit_notes(NULL, NULL, NULL, 0);\n+\tconst char *ref;\n+\tinit_notes(NULL, NULL, NULL, flags);\n \tt = &default_notes_tree;\n \n-\tif (!starts_with(t->ref, \"refs/notes/\"))\n+\tref = (flags & NOTES_INIT_WRITABLE) ? t->update_ref : t->ref;\n+\tif (!starts_with(ref, \"refs/notes/\"))\n \t\tdie(\"Refusing to %s notes in %s (outside of refs/notes/)\",\n-\t\t subcommand, t->ref);\n+\t\t subcommand, ref);\n \treturn t;\n }\n \n@@ -360,7 +363,7 @@ static int list(int argc, const char **argv, const char *prefix)\n \t\tusage_with_options(git_notes_list_usage, options);\n \t}\n \n-\tt = init_notes_check(\"list\");\n+\tt = init_notes_check(\"list\", 0);\n \tif (argc) {\n \t\tif (get_sha1(argv[0], object))\n \t\t\tdie(_(\"Failed to resolve '%s' as a valid ref.\"), argv[0]);\n@@ -420,7 +423,7 @@ static int add(int argc, const char **argv, const char *prefix)\n \tif (get_sha1(object_ref, object))\n \t\tdie(_(\"Failed to resolve '%s' as a valid ref.\"), object_ref);\n \n-\tt = init_notes_check(\"add\");\n+\tt = init_notes_check(\"add\", NOTES_INIT_WRITABLE);\n \tnote = get_note(t, object);\n \n \tif (note) {\n@@ -511,7 +514,7 @@ static int copy(int argc, const char **argv, const char *prefix)\n \tif (get_sha1(object_ref, object))\n \t\tdie(_(\"Failed to resolve '%s' as a valid ref.\"), object_ref);\n \n-\tt = init_notes_check(\"copy\");\n+\tt = init_notes_check(\"copy\", NOTES_INIT_WRITABLE);\n \tnote = get_note(t, object);\n \n \tif (note) {\n@@ -589,7 +592,7 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \tif (get_sha1(object_ref, object))\n \t\tdie(_(\"Failed to resolve '%s' as a valid ref.\"), object_ref);\n \n-\tt = init_notes_check(argv[0]);\n+\tt = init_notes_check(argv[0], NOTES_INIT_WRITABLE);\n \tnote = get_note(t, object);\n \n \tprepare_note_data(object, &d, edit ? note : NULL);\n@@ -652,7 +655,7 @@ static int show(int argc, const char **argv, const char *prefix)\n \tif (get_sha1(object_ref, object))\n \t\tdie(_(\"Failed to resolve '%s' as a valid ref.\"), object_ref);\n \n-\tt = init_notes_check(\"show\");\n+\tt = init_notes_check(\"show\", 0);\n \tnote = get_note(t, object);\n \n \tif (!note)\n@@ -809,7 +812,7 @@ static int merge(int argc, const char **argv, const char *prefix)\n \texpand_notes_ref(&remote_ref);\n \to.remote_ref = remote_ref.buf;\n \n-\tt = init_notes_check(\"merge\");\n+\tt = init_notes_check(\"merge\", NOTES_INIT_WRITABLE);\n \n \tif (strategy) {\n \t\tif (parse_notes_merge_strategy(strategy, &o.strategy)) {\n@@ -901,7 +904,7 @@ static int remove_cmd(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, options,\n \t\t\t git_notes_remove_usage, 0);\n \n-\tt = init_notes_check(\"remove\");\n+\tt = init_notes_check(\"remove\", NOTES_INIT_WRITABLE);\n \n \tif (!argc && !from_stdin) {\n \t\tretval = remove_one_note(t, \"HEAD\", flag);\n@@ -943,7 +946,7 @@ static int prune(int argc, const char **argv, const char *prefix)\n \t\tusage_with_options(git_notes_prune_usage, options);\n \t}\n \n-\tt = init_notes_check(\"prune\");\n+\tt = init_notes_check(\"prune\", NOTES_INIT_WRITABLE);\n \n \tprune_notes(t, (verbose ? NOTES_PRUNE_VERBOSE : 0) |\n \t\t(show_only ? NOTES_PRUNE_VERBOSE|NOTES_PRUNE_DRYRUN : 0) );\ndiff --git a/notes-cache.c b/notes-cache.c\nindex c4e9bb7..5dfc5cb 100644\n--- a/notes-cache.c\n+++ b/notes-cache.c\n@@ -32,14 +32,14 @@ void notes_cache_init(struct notes_cache *c, const char *name,\n \t\t const char *validity)\n {\n \tstruct strbuf ref = STRBUF_INIT;\n-\tint flags = 0;\n+\tint flags = NOTES_INIT_WRITABLE;\n \n \tmemset(c, 0, sizeof(*c));\n \tc->validity = xstrdup(validity);\n \n \tstrbuf_addf(&ref, \"refs/notes/%s\", name);\n \tif (!notes_cache_match_validity(ref.buf, validity))\n-\t\tflags = NOTES_INIT_EMPTY;\n+\t\tflags |= NOTES_INIT_EMPTY;\n \tinit_notes(&c->tree, ref.buf, combine_notes_overwrite, flags);\n \tstrbuf_release(&ref);\n }\n@@ -49,7 +49,8 @@ int notes_cache_write(struct notes_cache *c)\n \tunsigned char tree_sha1[20];\n \tunsigned char commit_sha1[20];\n \n-\tif (!c || !c->tree.initialized || !c->tree.ref || !*c->tree.ref)\n+\tif (!c || !c->tree.initialized || !c->tree.update_ref ||\n+\t !*c->tree.update_ref)\n \t\treturn -1;\n \tif (!c->tree.dirty)\n \t\treturn 0;\n@@ -59,8 +60,8 @@ int notes_cache_write(struct notes_cache *c)\n \tif (commit_tree(c->validity, strlen(c->validity), tree_sha1, NULL,\n \t\t\tcommit_sha1, NULL, NULL) < 0)\n \t\treturn -1;\n-\tif (update_ref(\"update notes cache\", c->tree.ref, commit_sha1, NULL,\n-\t\t 0, UPDATE_REFS_QUIET_ON_ERR) < 0)\n+\tif (update_ref(\"update notes cache\", c->tree.update_ref, commit_sha1,\n+\t\t NULL, 0, UPDATE_REFS_QUIET_ON_ERR) < 0)\n \t\treturn -1;\n \n \treturn 0;\ndiff --git a/notes-utils.c b/notes-utils.c\nindex 299e34b..24a3361 100644\n--- a/notes-utils.c\n+++ b/notes-utils.c\n@@ -37,7 +37,7 @@ void commit_notes(struct notes_tree *t, const char *msg)\n \n \tif (!t)\n \t\tt = &default_notes_tree;\n-\tif (!t->initialized || !t->ref || !*t->ref)\n+\tif (!t->initialized || !t->update_ref || !*t->update_ref)\n \t\tdie(_(\"Cannot commit uninitialized/unreferenced notes tree\"));\n \tif (!t->dirty)\n \t\treturn; /* don't have to commit an unchanged tree */\n@@ -48,7 +48,7 @@ void commit_notes(struct notes_tree *t, const char *msg)\n \n \tcreate_notes_commit(t, NULL, buf.buf, buf.len, commit_sha1);\n \tstrbuf_insert(&buf, 0, \"notes: \", 7); /* commit message starts at index 7 */\n-\tupdate_ref(buf.buf, t->ref, commit_sha1, NULL, 0,\n+\tupdate_ref(buf.buf, t->update_ref, commit_sha1, NULL, 0,\n \t\t UPDATE_REFS_DIE_ON_ERR);\n \n \tstrbuf_release(&buf);\n@@ -148,7 +148,7 @@ struct notes_rewrite_cfg *init_copy_notes_for_rewrite(const char *cmd)\n \t\tfree(c);\n \t\treturn NULL;\n \t}\n-\tc->trees = load_notes_trees(c->refs);\n+\tc->trees = load_notes_trees(c->refs, NOTES_INIT_WRITABLE);\n \tstring_list_clear(c->refs, 0);\n \tfree(c->refs);\n \treturn c;\ndiff --git a/notes.c b/notes.c\nindex eacd2a6..6ef347c 100644\n--- a/notes.c\n+++ b/notes.c\n@@ -1008,13 +1008,16 @@ void init_notes(struct notes_tree *t, const char *notes_ref,\n \tt->first_non_note = NULL;\n \tt->prev_non_note = NULL;\n \tt->ref = xstrdup_or_null(notes_ref);\n+\tt->update_ref = (flags & NOTES_INIT_WRITABLE) ? t->ref : NULL;\n \tt->combine_notes = combine_notes;\n \tt->initialized = 1;\n \tt->dirty = 0;\n \n \tif (flags & NOTES_INIT_EMPTY || !notes_ref ||\n-\t read_ref(notes_ref, object_sha1))\n+\t get_sha1_treeish(notes_ref, object_sha1))\n \t\treturn;\n+\tif (flags & NOTES_INIT_WRITABLE && read_ref(notes_ref, object_sha1))\n+\t\tdie(\"Cannot use notes ref %s\", notes_ref);\n \tif (get_tree_entry(object_sha1, \"\", sha1, &mode))\n \t\tdie(\"Failed to read notes tree referenced by %s (%s)\",\n \t\t notes_ref, sha1_to_hex(object_sha1));\n@@ -1024,7 +1027,7 @@ void init_notes(struct notes_tree *t, const char *notes_ref,\n \tload_subtree(t, &root_tree, t->root, 0);\n }\n \n-struct notes_tree **load_notes_trees(struct string_list *refs)\n+struct notes_tree **load_notes_trees(struct string_list *refs, int flags)\n {\n \tstruct string_list_item *item;\n \tint counter = 0;\n@@ -1032,7 +1035,7 @@ struct notes_tree **load_notes_trees(struct string_list *refs)\n \ttrees = xmalloc((refs->nr+1) * sizeof(struct notes_tree *));\n \tfor_each_string_list_item(item, refs) {\n \t\tstruct notes_tree *t = xcalloc(1, sizeof(struct notes_tree));\n-\t\tinit_notes(t, item->string, combine_notes_ignore, 0);\n+\t\tinit_notes(t, item->string, combine_notes_ignore, flags);\n \t\ttrees[counter++] = t;\n \t}\n \ttrees[counter] = NULL;\n@@ -1068,7 +1071,7 @@ void init_display_notes(struct display_notes_opt *opt)\n \t\t\t\t\t\t item->string);\n \t}\n \n-\tdisplay_notes_trees = load_notes_trees(&display_notes_refs);\n+\tdisplay_notes_trees = load_notes_trees(&display_notes_refs, 0);\n \tstring_list_clear(&display_notes_refs, 0);\n }\n \ndiff --git a/notes.h b/notes.h\nindex 2a3f923..e5d67fd 100644\n--- a/notes.h\n+++ b/notes.h\n@@ -44,6 +44,7 @@ extern struct notes_tree {\n \tstruct int_node *root;\n \tstruct non_note *first_non_note, *prev_non_note;\n \tchar *ref;\n+\tchar *update_ref;\n \tcombine_notes_fn combine_notes;\n \tint initialized;\n \tint dirty;\n@@ -72,6 +73,13 @@ const char *default_notes_ref(void);\n #define NOTES_INIT_EMPTY 1\n \n /*\n+ * By default, the notes tree is only readable, and the notes ref can be\n+ * any treeish. The notes tree can however be made writable with this flag,\n+ * in which case only strict ref names can be used.\n+ */\n+#define NOTES_INIT_WRITABLE 2\n+\n+/*\n * Initialize the given notes_tree with the notes tree structure at the given\n * ref. If given ref is NULL, the value of the $GIT_NOTES_REF environment\n * variable is used, and if that is missing, the default notes ref is used\n@@ -276,7 +284,7 @@ void format_display_notes(const unsigned char *object_sha1,\n * Load the notes tree from each ref listed in 'refs'. The output is\n * an array of notes_tree*, terminated by a NULL.\n */\n-struct notes_tree **load_notes_trees(struct string_list *refs);\n+struct notes_tree **load_notes_trees(struct string_list *refs, int flags);\n \n /*\n * Add all refs that match 'glob' to the 'list'.\ndiff --git a/t/t3301-notes.sh b/t/t3301-notes.sh\nindex cd70274..2d200fd 100755\n--- a/t/t3301-notes.sh\n+++ b/t/t3301-notes.sh\n@@ -83,6 +83,16 @@ test_expect_success 'edit existing notes' '\n \ttest_must_fail git notes show HEAD^\n '\n \n+test_expect_success 'show notes from treeish' '\n+\ttest \"b3\" = \"$(git notes --ref commits^{tree} show)\" &&\n+\ttest \"b4\" = \"$(git notes --ref commits@{1} show)\"\n+'\n+\n+test_expect_success 'cannot edit notes from non-ref' '\n+\ttest_must_fail git notes --ref commits^{tree} edit &&\n+\ttest_must_fail git notes --ref commits@{1} edit\n+'\n+\n test_expect_success 'cannot \"git notes add -m\" where notes already exists' '\n \ttest_must_fail git notes add -m \"b2\" &&\n \ttest_path_is_missing .git/NOTES_EDITMSG &&\n-- \n2.6.1.146.ge5b68b2\n"},{"id":"271339","messageId":"xmqqziztqj26.fsf@gitster.mtv.corp.google.com","threadId":"39653","inReplyTo":"20151008025018.GA29722@glandium.org","subject":"Re: [PATCH] notes: Allow treeish expressions as notes ref","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-08T19:03:29Z","receivedAt":"2015-10-08T19:03:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Hommey writes:\n\n> After refreshing the patch against current \"next\", it appears that\n> there is such a distinction:\n>\n> $ ./git-log --notes=fdsfgsfdg HEAD^! --pretty=short\n> warning: notes ref refs/notes/fdsfgsfdg is invalid\n> commit e5b68b2e879608d881c2e3600ce84962fcdefc88\n> Author: Mike Hommey \n>\n> notes: allow treeish expressions as notes ref\n>\n> $ ./git-log --notes=foo HEAD^! --pretty=short\n> commit e5b68b2e879608d881c2e3600ce84962fcdefc88\n> Author: Mike Hommey \n>\n> notes: allow treeish expressions as notes ref\n\nGood; thanks for checking.\n"}]}