{"thread":{"id":"40366","subject":"[PATCH 0/2] notes: allow read only notes actions on refs outside of refs/notes","startedAt":"2015-09-16T22:06:32Z","lastAt":"2015-09-22T18:48:21Z","messageCount":13,"participants":["Jacob Keller","Junio C Hamano","Mike Hommey"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"270172","messageId":"1442441194-5506-1-git-send-email-jacob.e.keller@intel.com","threadId":"40366","inReplyTo":null,"subject":"[PATCH 0/2] notes: allow read only notes actions on refs outside of refs/notes","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2015-09-16T22:06:32Z","receivedAt":"2015-09-16T22:06:32Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller \n\nThe primary purpose of this series is to allow fetching remote notes\ninto a namespace such as \"refs/remote-notes//foo\". Currently,\ngit-notes refuses to operate on refs outside of refs/notes/* including\nmerging from them, listing or showing them. This makes it difficult to\nshare notes from a remote repository.\n\nFix expand_notes_ref to not always prepend refs/notes to fully qualified\nrefs. Allow git-notes actions which do not specify NOTES_INIT_WRITABLE\nto be performed on refs outside of refs/notes/*\n\nFuture work will include more coupling of \"refs/remote-notes/\"\ninto git-notes so that you can specify refs as \"/foo\" similar to\nhow remotes work for branches.\n\nIn addition, long term goal is to make it default to fetch notes into\nrefs/remote-notes//*, and possibly to add some status for\ntracking similar to how tracking branches work.\n\nThe one downside currently is that a test case for prevention of merge\nfrom \"refs/heads/master\" had to be removed as \"git notes merge\nrefs/heads/master\" now works. I am not sure how this could be fixed.. I\ndid not find any way to tell if a treeish actually was a notes tree or\nnot...\n\nThis topic depends on mh/notes-allow-reading-treeish and actually\nexpands what this topic allowed before. Previously, treeishes such as\nnotes@{1} were made allowable, but the ref still had to be found under\nrefs/notes.\n\nNo documentation changes were made as from the looks of it,\ndocumentation for --ref and core.notesRef is already correct despite the\nprevious behavior of expand_notes_ref. In that sense, documentation was\nwrong before.\n\nJacob Keller (2):\n notes: don't expand qualified refs in expand_notes_ref\n notes: allow non-writable actions on refs outside of refs/notes\n\n builtin/notes.c | 11 ++++++-----\n notes.c | 4 ++--\n t/t3308-notes-merge.sh | 1 -\n 3 files changed, 8 insertions(+), 8 deletions(-)\n\n-- \n2.6.0.rc2.248.g5b5be23\n"},{"id":"270174","messageId":"1442441194-5506-2-git-send-email-jacob.e.keller@intel.com","threadId":"40366","inReplyTo":"1442441194-5506-1-git-send-email-jacob.e.keller@intel.com","subject":"[PATCH 1/2] notes: don't expand qualified refs in expand_notes_ref","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2015-09-16T22:06:33Z","receivedAt":"2015-09-16T22:06:33Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller \n\nThe documentation for --refs says that it will treat unqualified refs as\nunder refs/notes. Current behavior is to prefix refs/notes to all\nstrings that do not start with refs/notes or notes/, resulting in\nperforming actions on refs such as \"refs/notes/refs/foo/bar\" instead of\nattempting to perform actions on \"refs/foo/bar\". A future patch will\nintroduce the idea of performing non-writable actions to refs outside of\nrefs/notes. Change the behavior of expand_notes_ref to leave qualified\nrefs under refs/* alone.\n\nIn addition, fix git notes merge to prevent merges from refs which\nare not under refs/notes, in a similar way to how we already check note\nrefs in init_notes_check. This is required in order to keep current\ntests passing without change. A future patch will change the behavior of\ngit notes merge so that it can merge from a ref outside of refs/notes.\n\nSigned-off-by: Jacob Keller \n---\n builtin/notes.c | 4 ++++\n notes.c | 4 ++--\n 2 files changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 6371d536d164..0f55d38983f0 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -810,6 +810,10 @@ static int merge(int argc, const char **argv, const char *prefix)\n \to.local_ref = default_notes_ref();\n \tstrbuf_addstr(&remote_ref, argv[0]);\n \texpand_notes_ref(&remote_ref);\n+\tif (!starts_with(remote_ref.buf, \"refs/notes\"))\n+\t\tdie(\"Refusing to merge notes from %s (outside of refs/notes/)\",\n+\t\t remote_ref.buf);\n+\n \to.remote_ref = remote_ref.buf;\n \n \tt = init_notes_check(\"merge\", NOTES_INIT_WRITABLE);\ndiff --git a/notes.c b/notes.c\nindex 6ef347ca3ac4..d49168fb3f01 100644\n--- a/notes.c\n+++ b/notes.c\n@@ -1296,8 +1296,8 @@ int copy_note(struct notes_tree *t,\n \n void expand_notes_ref(struct strbuf *sb)\n {\n-\tif (starts_with(sb->buf, \"refs/notes/\"))\n-\t\treturn; /* we're happy */\n+\tif (starts_with(sb->buf, \"refs/\"))\n+\t\treturn; /* fully qualified, so we're happy */\n \telse if (starts_with(sb->buf, \"notes/\"))\n \t\tstrbuf_insert(sb, 0, \"refs/\", 5);\n \telse\n-- \n2.6.0.rc2.248.g5b5be23\n"},{"id":"270173","messageId":"1442441194-5506-3-git-send-email-jacob.e.keller@intel.com","threadId":"40366","inReplyTo":"1442441194-5506-1-git-send-email-jacob.e.keller@intel.com","subject":"[PATCH 2/2] notes: allow non-writable actions on refs outside of refs/notes","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2015-09-16T22:06:34Z","receivedAt":"2015-09-16T22:06:34Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller \n\nAllow non-destructive notes actions which do not require write\npermission to be performed on refs outside of refs/notes/. The primary\nadvantage of this is to allow fetching remote refs to such location as\n\"refs/remote-notes//foo\" and then performing merges into\nrefs/notes/\n\nIt is not reasonable to put remote notes inside refs/notes as users may\nalready have conflicting names inside the notes namespace.\n\nRemove one test case regarding merge from refs/heads/master, which will\nnow pass under current code. It may be worth looking how to prevent\nsome of these more obviously wrong merges.\n\nSigned-off-by: Jacob Keller \n---\n builtin/notes.c | 13 +++++--------\n t/t3308-notes-merge.sh | 1 -\n 2 files changed, 5 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 0f55d38983f0..ff056014d953 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -333,14 +333,14 @@ 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-\tconst char *ref;\n \tinit_notes(NULL, NULL, NULL, flags);\n \tt = &default_notes_tree;\n \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, ref);\n+\tif (flags & NOTES_INIT_WRITABLE) {\n+\t\tif (!starts_with(t->update_ref, \"refs/notes\"))\n+\t\t\tdie(\"Refusing to %s notes in %s (outside of refs/notes/)\",\n+\t\t\t subcommand, t->update_ref);\n+\t}\n \treturn t;\n }\n \n@@ -810,9 +810,6 @@ static int merge(int argc, const char **argv, const char *prefix)\n \to.local_ref = default_notes_ref();\n \tstrbuf_addstr(&remote_ref, argv[0]);\n \texpand_notes_ref(&remote_ref);\n-\tif (!starts_with(remote_ref.buf, \"refs/notes\"))\n-\t\tdie(\"Refusing to merge notes from %s (outside of refs/notes/)\",\n-\t\t remote_ref.buf);\n \n \to.remote_ref = remote_ref.buf;\n \ndiff --git a/t/t3308-notes-merge.sh b/t/t3308-notes-merge.sh\nindex 24d82b49bbea..f0feb64bae6e 100755\n--- a/t/t3308-notes-merge.sh\n+++ b/t/t3308-notes-merge.sh\n@@ -90,7 +90,6 @@ test_expect_success 'fail to merge various non-note-trees' '\n \ttest_must_fail git notes merge refs/notes/ &&\n \ttest_must_fail git notes merge refs/notes/dir &&\n \ttest_must_fail git notes merge refs/notes/dir/ &&\n-\ttest_must_fail git notes merge refs/heads/master &&\n \ttest_must_fail git notes merge x: &&\n \ttest_must_fail git notes merge x:foo &&\n \ttest_must_fail git notes merge foo^{bar\n-- \n2.6.0.rc2.248.g5b5be23\n"},{"id":"270177","messageId":"xmqqzj0mkn7r.fsf@gitster.mtv.corp.google.com","threadId":"40366","inReplyTo":"1442441194-5506-2-git-send-email-jacob.e.keller@intel.com","subject":"Re: [PATCH 1/2] notes: don't expand qualified refs in expand_notes_ref","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-16T22:34:32Z","receivedAt":"2015-09-16T22:34:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller writes:\n\n> From: Jacob Keller \n>\n> The documentation for --refs says that it will treat unqualified refs as\n> under refs/notes. Current behavior is to prefix refs/notes to all\n> strings that do not start with refs/notes or notes/, resulting in\n> performing actions on refs such as \"refs/notes/refs/foo/bar\" instead of\n> attempting to perform actions on \"refs/foo/bar\".\n\nThat actually sounds like a sensible thing to do, if you replace\n'foo' with 'heads', for example, i.e. refs/notes/refs/heads/bar is a\nnotes about commits reachable from the branch whose name is 'bar'.\n\nSo given \"refs/heads/bar\", which is unqualified in the context of\ntalking about references that hold notes trees, the current\nbehaviour to turn it into \"refs/notes/refs/heads/bar\" is very\nsensible, I would think.\n"},{"id":"270183","messageId":"20150916223602.GA4197@glandium.org","threadId":"40366","inReplyTo":"1442441194-5506-1-git-send-email-jacob.e.keller@intel.com","subject":"Re: [PATCH 0/2] notes: allow read only notes actions on refs outside of refs/notes","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2015-09-16T22:36:02Z","receivedAt":"2015-09-16T22:36:02Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Wed, Sep 16, 2015 at 03:06:32PM -0700, Jacob Keller wrote:\n> This topic depends on mh/notes-allow-reading-treeish and actually\n> expands what this topic allowed before. Previously, treeishes such as\n> notes@{1} were made allowable, but the ref still had to be found under\n> refs/notes.\n\nI haven't found the time to finish that topic yet, but I haven't\nforgotten it. Sorry about that.\n\nMike\n"},{"id":"270182","messageId":"CA+P7+xp1aSWJJ5VpAOa6Pje9Qp2sWTjj742xfh5nVJDcD9vCFA@mail.gmail.com","threadId":"40366","inReplyTo":"xmqqzj0mkn7r.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] notes: don't expand qualified refs in expand_notes_ref","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2015-09-16T23:00:39Z","receivedAt":"2015-09-16T23:00:39Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Wed, Sep 16, 2015 at 3:34 PM, Junio C Hamano wrote:\n> Jacob Keller writes:\n>\n>> From: Jacob Keller \n>>\n>> The documentation for --refs says that it will treat unqualified refs as\n>> under refs/notes. Current behavior is to prefix refs/notes to all\n>> strings that do not start with refs/notes or notes/, resulting in\n>> performing actions on refs such as \"refs/notes/refs/foo/bar\" instead of\n>> attempting to perform actions on \"refs/foo/bar\".\n>\n> That actually sounds like a sensible thing to do, if you replace\n> 'foo' with 'heads', for example, i.e. refs/notes/refs/heads/bar is a\n> notes about commits reachable from the branch whose name is 'bar'.\n>\n> So given \"refs/heads/bar\", which is unqualified in the context of\n> talking about references that hold notes trees, the current\n> behaviour to turn it into \"refs/notes/refs/heads/bar\" is very\n> sensible, I would think.\n>\n\nThe end goal is to allow refs inside \"refs/remote-notes/\".. it seems\nreally weird that other fully qualified refs don't get \"expanded\" in\nthe same way as notes, and documentation does not make it explicit\nthat this is how it would work. I think that users who actually want\nthis behavior are already free to say \"refs/notes/refs/heads/bar\"...\nthat wouldn't change..\n\nHow would you propose allowing merging from \"refs/remote-notes//bar\"?\n\nWe could easily just hard-code acceptance of refs/remote-notes/ as\nwell as refs/notes... But that felt weird to me...\n\nBut honestly I don't really care how it is done as long as we can \"git\nnotes show\", \"git notes list\" on refs/remote-notes//commits\n(or similar, remote-notes may not be the actual location if someone\ncame up with a better name?)\n\nHow would you propose we allow that?\n\nIf we keep the current behavior of \"expand_notes_ref\" then we\nabsolutely can't because use of \"--ref\" will auto expand\n\"refs/remote-notes//commits\" into\n\"refs/notes/refs/remote-notes//commits\" which wouldn't work...\n\nRegards,\nJake\n"},{"id":"270184","messageId":"CA+P7+xqxajmGysjyenMv=UeS3DdofKvMTDXtDzrnGnBAQwzyBQ@mail.gmail.com","threadId":"40366","inReplyTo":"20150916223602.GA4197@glandium.org","subject":"Re: [PATCH 0/2] notes: allow read only notes actions on refs outside of refs/notes","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2015-09-16T23:01:29Z","receivedAt":"2015-09-16T23:01:29Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Wed, Sep 16, 2015 at 3:36 PM, Mike Hommey wrote:\n> On Wed, Sep 16, 2015 at 03:06:32PM -0700, Jacob Keller wrote:\n>> This topic depends on mh/notes-allow-reading-treeish and actually\n>> expands what this topic allowed before. Previously, treeishes such as\n>> notes@{1} were made allowable, but the ref still had to be found under\n>> refs/notes.\n>\n> I haven't found the time to finish that topic yet, but I haven't\n> forgotten it. Sorry about that.\n>\n> Mike\n\nThat's ok. I don't think any of the remaining re-works left for your\npatch impact mine..? I haven't really read it, but I know I make use\nif NOTES_INIT_WRITABLE is the main reason.\n\nRegards,\nJake\n"},{"id":"270475","messageId":"CA+P7+xpv_0Sf94FqMKJa0v0pSSEWXPRD2KQ0kmNBKC=2hrunhw@mail.gmail.com","threadId":"40366","inReplyTo":"xmqqzj0mkn7r.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] notes: don't expand qualified refs in expand_notes_ref","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2015-09-22T06:50:05Z","receivedAt":"2015-09-22T06:50:05Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Wed, Sep 16, 2015 at 3:34 PM, Junio C Hamano wrote:\n> Jacob Keller writes:\n>\n>> From: Jacob Keller \n>>\n>> The documentation for --refs says that it will treat unqualified refs as\n>> under refs/notes. Current behavior is to prefix refs/notes to all\n>> strings that do not start with refs/notes or notes/, resulting in\n>> performing actions on refs such as \"refs/notes/refs/foo/bar\" instead of\n>> attempting to perform actions on \"refs/foo/bar\".\n>\n> That actually sounds like a sensible thing to do, if you replace\n> 'foo' with 'heads', for example, i.e. refs/notes/refs/heads/bar is a\n> notes about commits reachable from the branch whose name is 'bar'.\n>\n> So given \"refs/heads/bar\", which is unqualified in the context of\n> talking about references that hold notes trees, the current\n> behaviour to turn it into \"refs/notes/refs/heads/bar\" is very\n> sensible, I would think.\n>\n\nI never got any better suggestion on how to allow the behavior\ndesired, which is to enable merging from a non-notes location, in\norder to provide a standard location for remote notes, ie:\nrefs/remote-notes//\n\nAny suggestions, if you're against this particular change?\n\nRegards,\nJake\n"},{"id":"270484","messageId":"xmqqsi665yjm.fsf@gitster.mtv.corp.google.com","threadId":"40366","inReplyTo":"CA+P7+xpv_0Sf94FqMKJa0v0pSSEWXPRD2KQ0kmNBKC=2hrunhw@mail.gmail.com","subject":"Re: [PATCH 1/2] notes: don't expand qualified refs in expand_notes_ref","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-22T14:17:01Z","receivedAt":"2015-09-22T14:17:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller writes:\n\n> I never got any better suggestion on how to allow the behavior\n> desired, which is to enable merging from a non-notes location, in\n> order to provide a standard location for remote notes, ie:\n> refs/remote-notes//\n\nStep back a bit and think again. I think you are blinded by your\nrefs/remote-notes/*.\n\nIt is fine to wish that\n\n $ notes merge refs/notes/commits refs/remote-notes/origin/commits\n\nto work, but you shouldn't force your users to use remote-tracking\nrefs in the first place. Your users should be allowed to do this as\nwell:\n\n $ fetch origin refs/notes/commits\n $ THEIRS=$(git rev-parse FETCH_HEAD)\n $ notes merge refs/notes/commits $THEIRS\n\nWe need to realize that \"notes merge\" involves two notes trees and\nthey are of different nature. The notes tree you are merging into\nand recording the result (the destination), which will be a local\nnote, and the other notes tree you obtained from elsewhere and\nupdate that local note with (the source).\n\nThe current code before your patch limits the allowed pair of notes\ntrees by insisting that both appear as the tips of refs somewhere in\nrefs/notes/*. For allowing to merge from outside refs/notes/, you\nneed to loosen the location the latter notes tree, the one to update\nyour local notes tree with, can come from. But that does not mean\nyou would want to loosen the location where the resulting notes tree\ncan go.\n\nI think the proposed patch conflates them, and that conflation does\nnot help anything. The rule of that function used to be \"It must\ncome from refs/notes/ and nowhere else\". That made sense in the old\nworld order where both must be from refs/notes/, and the rule still\nmakes sense in the new world order for the destination of the merge.\n\nThe new rule of that function after the proposed patch says \"It must\ncome from either refs/notes or refs/ somewhere\". This does not make\nsense for the destination because it is too loose (and we didn't see\nany justification why loosening it is a good idea), and it does not\nmake sense for the source because it still is too tight. It should\nbe able to take anything get_sha1() understands (including $THEIRS\nin the above example).\n\nIn addition you might also want to allow additional DWIMs from X to\nrefs/remote-notes/*/X as well as from X to refs/notes/X, but that is\nsecondary and should be done as a follow-up \"nice to have\" change,\nbecause both \"notes/remote-notes/origin/commits\" and \"notes/commits\"\nwould be understood by get_sha1() already, and it is questinable if\nit is a good idea to introduce special DWIMs that kick in only when\nthe commands are talking about notes in the first place (an equally\nquestionable alternative is to teach get_sha1() about refs/notes/*\nand refs/remote-notes/*/*, which will make the disambiguation noisy\nin the normal non-notes codepath---my knee-jerk reaction is to\nsuggest not to go there, either).\n\nIn any case, to get us closer to that endgame, change in the\nproposed patch does not help. It tries to cover two different cases\nwith a logic that is not a good match to either. You need to have\ntwo separate helpers to interpret the source and the destination.\n\nCalls expand_notes_ref() made on a command line argument that\nspecifies the source (which I think is similar to what the other\nrecent topic calls \"read-only\") should be made to calls to a more\nlenient version (and you can start with get_sha1() for that purpose\nwithout introducing your own DWIMs in the first step), while leaving\ncalls to expand_notes_ref() for destination and the implementation\nof expand_notes_ref() itself unmolested, so that we can keep the\nsafety in expands_notes_ref() that makes sure that the destination\nof a local operation is under refs/notes/*.\n"},{"id":"270486","messageId":"CA+P7+xr_rXtxpuV0SpyB0ysXz0zi=ZWTRHt+TANzOqVN5g=n-g@mail.gmail.com","threadId":"40366","inReplyTo":"xmqqsi665yjm.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] notes: don't expand qualified refs in expand_notes_ref","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2015-09-22T15:26:42Z","receivedAt":"2015-09-22T15:26:42Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Tue, Sep 22, 2015 at 7:17 AM, Junio C Hamano wrote:\n> The current code before your patch limits the allowed pair of notes\n> trees by insisting that both appear as the tips of refs somewhere in\n> refs/notes/*. For allowing to merge from outside refs/notes/, you\n> need to loosen the location the latter notes tree, the one to update\n> your local notes tree with, can come from. But that does not mean\n> you would want to loosen the location where the resulting notes tree\n> can go.\n\nI do not loosen where the written location can go. My patch series\nsort of does the opposite of where you suggest. I change\nexpand_notes_ref to be more \"loose\" but not still keep the current\nchecks in place which ensure that refs outside of refs/notes don't\nwrite to them.\n\n>\n> I think the proposed patch conflates them, and that conflation does\n> not help anything. The rule of that function used to be \"It must\n> come from refs/notes/ and nowhere else\". That made sense in the old\n> world order where both must be from refs/notes/, and the rule still\n> makes sense in the new world order for the destination of the merge.\n>\n\nYes, I don't change the destination rule, though I do change how we\ncan \"DWIM\" so that \"attempting\" to merge into \"refs/balagrg/foo\" does\nnot actually merge into \"refs/notes/refs/balagrg/foo\". I think this is\nsane.\n\nThe actual \"init_notes_check\" code prevents merging into refs outside\nof refs/notes/*\n\nI think it's incredibly confusing that other DWIM's do not expand\n\"refs/*\" into \"refs/something/refs/*\"\n\n> The new rule of that function after the proposed patch says \"It must\n> come from either refs/notes or refs/ somewhere\". This does not make\n> sense for the destination because it is too loose (and we didn't see\n> any justification why loosening it is a good idea), and it does not\n> make sense for the source because it still is too tight. It should\n> be able to take anything get_sha1() understands (including $THEIRS\n> in the above example).\n\nAgreed, we should use something more expansive for the non-write\noperations. I still think the proposed change to expand_notes_refs\nwould be good... but that's because I find it incredibly confusing.\nApparently we disagree on that.\n\n>\n> In addition you might also want to allow additional DWIMs from X to\n> refs/remote-notes/*/X as well as from X to refs/notes/X, but that is\n> secondary and should be done as a follow-up \"nice to have\" change,\n> because both \"notes/remote-notes/origin/commits\" and \"notes/commits\"\n> would be understood by get_sha1() already, and it is questinable if\n> it is a good idea to introduce special DWIMs that kick in only when\n> the commands are talking about notes in the first place (an equally\n> questionable alternative is to teach get_sha1() about refs/notes/*\n> and refs/remote-notes/*/*, which will make the disambiguation noisy\n> in the normal non-notes codepath---my knee-jerk reaction is to\n> suggest not to go there, either).\n\nThe DWIM's I suggest are \"foo\" -> \"refs/notes/foo\", \"notes/foo\" ->\n\"refs/notes/foo\", as these already work, in addition to \"/foo\"\n-> \"refs/remote-notes//foo\" and \"/notes/foo\" ->\n\"refs/remote-notes//foo\" Obviously only kicking in for notes\nreferences.\n\n>\n> In any case, to get us closer to that endgame, change in the\n> proposed patch does not help. It tries to cover two different cases\n> with a logic that is not a good match to either. You need to have\n> two separate helpers to interpret the source and the destination.\n>\n\nMy patch does the opposite of your suggestion: Loosen expand_notes_ref\nwhile keeping the same overall restrictions, which has the same result\nif being considered a little backward.\n\n> Calls expand_notes_ref() made on a command line argument that\n> specifies the source (which I think is similar to what the other\n> recent topic calls \"read-only\") should be made to calls to a more\n> lenient version (and you can start with get_sha1() for that purpose\n> without introducing your own DWIMs in the first step), while leaving\n> calls to expand_notes_ref() for destination and the implementation\n> of expand_notes_ref() itself unmolested, so that we can keep the\n> safety in expands_notes_ref() that makes sure that the destination\n> of a local operation is under refs/notes/*.\n\nexpand_notes_ref doesn't actually provide the safety net. You can\nbypass this entirely by using the command line, at least for the\nnon-write operations. In addition, documentation says \"non qualified\nrefs will be expanded\" which usually means \"refs/*\" will be left\nalone, but in this case \"refs/*\" outside of refs/notes will be\nexpanded.\n\nEven with just this patch the only difference is an attempt to use\nrefs/foo/bad *won't* expand into \"refs/notes/refs/foo/bar\" which is an\nexpansion I find incredibly confusing to begin with.\n\nRegards,\nJake\n"},{"id":"270499","messageId":"CA+P7+xrwM44tMHMW+dEJx_tMtcMVEQELNq=J=rqs1uQ+feuZqQ@mail.gmail.com","threadId":"40366","inReplyTo":"xmqqsi665yjm.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] notes: don't expand qualified refs in expand_notes_ref","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2015-09-22T17:54:12Z","receivedAt":"2015-09-22T17:54:12Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Tue, Sep 22, 2015 at 7:17 AM, Junio C Hamano wrote:\n> Calls expand_notes_ref() made on a command line argument that\n> specifies the source (which I think is similar to what the other\n> recent topic calls \"read-only\") should be made to calls to a more\n> lenient version (and you can start with get_sha1() for that purpose\n> without introducing your own DWIMs in the first step), while leaving\n> calls to expand_notes_ref() for destination and the implementation\n> of expand_notes_ref() itself unmolested, so that we can keep the\n> safety in expands_notes_ref() that makes sure that the destination\n> of a local operation is under refs/notes/*.\n\nThe other issue here is that expand_notes_ref is called on the --ref\nargument waaay before the current code even decides if the operation\nis \"read\" or \"write\". Thus we'd have to break this out and handle\nthings very differently.\n\nI don't believe expand_notes_ref() is actually providing any safety.\nThat is all done by init_notes_check. Note that passing the\nenvironment variable bypasses the expand_notes_ref entirely, though I\nthink we did use it as part of the refs configuration.\n\nIt seems like a lot more heavy lifting to change the entire flow of\nhow we decide when to expand \"--ref\" for \"read\" operations vs \"write\"\noperations.\n\nThat is, git notes list.\n\nIt's easy to change behavior of git notes merge as it handles its\nargument for where to merge from separately, but it's not so easy to\nchange git notes show...\n\nRegards,\nJake\n"},{"id":"270504","messageId":"xmqqa8se47wa.fsf@gitster.mtv.corp.google.com","threadId":"40366","inReplyTo":"CA+P7+xrwM44tMHMW+dEJx_tMtcMVEQELNq=J=rqs1uQ+feuZqQ@mail.gmail.com","subject":"Re: [PATCH 1/2] notes: don't expand qualified refs in expand_notes_ref","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-22T18:37:57Z","receivedAt":"2015-09-22T18:37:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller writes:\n\n> The other issue here is that expand_notes_ref is called on the --ref\n> argument waaay before the current code even decides if the operation\n> is \"read\" or \"write\". Thus we'd have to break this out and handle\n> things very differently.\n\nI think you hit the right nail here. The handling of --ref argument\nis what you need to adjust to the new world order.\n\nAnd \"safety\" is a red herring. Those who are used to run\n\n\tgit log --notes=refs/heads/master\n\nand relies on it to refer to refs/notes/refs/heads/master must\ncontinue to be able to do so. Changing expand_notes_ref() the\nway the proposed patch does will break them, won't it? \"safety\"\nis not the only (or even the primary) requirement, but the\ncorrectness must also be kept.\n\n> It seems like a lot more heavy lifting to change the entire flow of\n> how we decide when to expand \"--ref\" for \"read\" operations vs \"write\"\n> operations.\n>\n> That is, git notes list.\n>\n> It's easy to change behavior of git notes merge as it handles its\n> argument for where to merge from separately, but it's not so easy to\n> change git notes show...\n\nYes, exactly.\n\nI am _more than_ OK to see that read-only accesses to the notes tree\nallowed anything under refs/ (like the proposed patch did) and also\na raw 40-hex object name in the endgame, but I really would not want\nto see \"we meant well and attempted to enhance 'notes merge' but we\nended up regressing the behaviour of unrelated operations all of a\nsudden\".\n\nIf you cannot do your change without breaking the existing users,\nthen you at least need a sound transition plan. But I am not sure\nyet if you have to break the world (yet). Let me think aloud a bit\nmore.\n\nThere aren't all that many callers of expand_notes_ref().\n\nMy preference is to leave that function as-is, especially keep the\nbehaviour of the caller in revision.c that handles --show-notes=\n(and --notes= that is its synonym) the same as before.\n\nAll the other callers are only reachable from the codepath that\noriginates from cmd_notes(), if I am reading the code correctly, and\nthat can be enhanced without breaking the existing users. One\nobvious way to do so would be to make \"--ref\" to keep the call to\nexpand_notes_ref(), and add another \"--notes-rawref\" or whatever\noption that does not restrict it to \"refs/notes\", but there may be\nother ways.\n"},{"id":"270509","messageId":"CA+P7+xqDs4OokEeNhsvYeP8dDRhC1jRh2hLMBqc1v8D+ABCnmg@mail.gmail.com","threadId":"40366","inReplyTo":"xmqqa8se47wa.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] notes: don't expand qualified refs in expand_notes_ref","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2015-09-22T18:48:21Z","receivedAt":"2015-09-22T18:48:21Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Tue, Sep 22, 2015 at 11:37 AM, Junio C Hamano wrote:\n> Jacob Keller writes:\n>\n>> The other issue here is that expand_notes_ref is called on the --ref\n>> argument waaay before the current code even decides if the operation\n>> is \"read\" or \"write\". Thus we'd have to break this out and handle\n>> things very differently.\n>\n> I think you hit the right nail here. The handling of --ref argument\n> is what you need to adjust to the new world order.\n>\n> And \"safety\" is a red herring. Those who are used to run\n>\n> git log --notes=refs/heads/master\n>\n> and relies on it to refer to refs/notes/refs/heads/master must\n> continue to be able to do so. Changing expand_notes_ref() the\n> way the proposed patch does will break them, won't it? \"safety\"\n> is not the only (or even the primary) requirement, but the\n> correctness must also be kept.\n\nI think that's more confusing than helpful, but we really shouldn't\nchange behavior here. I think we need to update documentation to\nclearly indicate the current behavior, as it was not obvious at least\nto me. I can submit a patch for this at least.\n\n\n>\n>> It seems like a lot more heavy lifting to change the entire flow of\n>> how we decide when to expand \"--ref\" for \"read\" operations vs \"write\"\n>> operations.\n>>\n>> That is, git notes list.\n>>\n>> It's easy to change behavior of git notes merge as it handles its\n>> argument for where to merge from separately, but it's not so easy to\n>> change git notes show...\n>\n> Yes, exactly.\n>\n> I am _more than_ OK to see that read-only accesses to the notes tree\n> allowed anything under refs/ (like the proposed patch did) and also\n> a raw 40-hex object name in the endgame, but I really would not want\n> to see \"we meant well and attempted to enhance 'notes merge' but we\n> ended up regressing the behaviour of unrelated operations all of a\n> sudden\".\n>\n> If you cannot do your change without breaking the existing users,\n> then you at least need a sound transition plan. But I am not sure\n> yet if you have to break the world (yet). Let me think aloud a bit\n> more.\n>\n> There aren't all that many callers of expand_notes_ref().\n>\n> My preference is to leave that function as-is, especially keep the\n> behaviour of the caller in revision.c that handles --show-notes=\n> (and --notes= that is its synonym) the same as before.\n>\n\nOk.\n\n> All the other callers are only reachable from the codepath that\n> originates from cmd_notes(), if I am reading the code correctly, and\n> that can be enhanced without breaking the existing users. One\n> obvious way to do so would be to make \"--ref\" to keep the call to\n> expand_notes_ref(), and add another \"--notes-rawref\" or whatever\n> option that does not restrict it to \"refs/notes\", but there may be\n> other ways.\n>\n\nI think the easiest way would be to have --ref check ahead of time\nwhich commands are read or write, and perform the init_notes_check\ncode there instead of inside each command. I'll look at this again\nwhen I have time in the next few days.\n\nRegards,\nJake\n"}]}