{"thread":{"id":"40791","subject":"[PATCH] notes: allow merging from arbitrary references","startedAt":"2015-11-13T16:34:22Z","lastAt":"2015-12-11T19:58:47Z","messageCount":12,"participants":["Jacob Keller","Johan Herland","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"273281","messageId":"1447432462-21192-1-git-send-email-jacob.e.keller@intel.com","threadId":"40791","inReplyTo":null,"subject":"[PATCH] notes: allow merging from arbitrary references","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2015-11-13T16:34:22Z","receivedAt":"2015-11-13T16:34:22Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller \n\nCreate a new expansion function, expand_loose_notes_ref which will\nexpand any ref using get_sha1, but falls back to expand_notes_ref if\nthis fails. The contents of the strbuf will be either the hex string of\nthe sha1, or the expanded notes ref. It is expected to be re-expanded\nusing get_sha1 inside the notes merge machinery, and there is no real\nerror checking provided at this layer.\n\nSince we now support merging from non-notes refs, remove the test case\nassociated with that behavior. Add a test case for merging from a\nnon-notes ref.\n\nSigned-off-by: Jacob Keller \n---\nI do not remember what version this was since it has been an age ago\nthat I sent the previous code. This is mostly just a rebase onto current\nnext. I believe I have covered everything previous reviewers noted.\n\nI'm interested in whether this is the right direction, as my longterm\ngoal is to be able to push/pull notes to a specific namespace (probably\nrefs/remote-notes/*, since actually modifying to use\nrefs/remotes/notes/* is difficult to send to users, and remote-notes\nmakes the most useful sense). The first part of this is allowing merge\nto come from an arbitrary reference, as currently it is not really\npossible to merge from refs/remote-notes as we'd need it to be.\n\n builtin/notes.c | 4 ++--\n notes.c | 14 ++++++++++++++\n notes.h | 8 ++++++++\n t/t3308-notes-merge.sh | 22 +++++++++++-----------\n 4 files changed, 35 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex e0f5d308d206..4a86cc90ee92 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -809,7 +809,7 @@ static int merge(int argc, const char **argv, const char *prefix)\n \n \to.local_ref = default_notes_ref();\n \tstrbuf_addstr(&remote_ref, argv[0]);\n-\texpand_notes_ref(&remote_ref);\n+\texpand_loose_notes_ref(&remote_ref);\n \to.remote_ref = remote_ref.buf;\n \n \tt = init_notes_check(\"merge\", NOTES_INIT_WRITABLE);\n@@ -836,7 +836,7 @@ static int merge(int argc, const char **argv, const char *prefix)\n \t}\n \n \tstrbuf_addf(&msg, \"notes: Merged notes from %s into %s\",\n-\t\t remote_ref.buf, default_notes_ref());\n+\t\t argv[0], default_notes_ref());\n \tstrbuf_add(&(o.commit_msg), msg.buf + 7, msg.len - 7); /* skip \"notes: \" */\n \n \tresult = notes_merge(&o, t, result_sha1);\ndiff --git a/notes.c b/notes.c\nindex 358e2fdb74eb..c92c22aa217a 100644\n--- a/notes.c\n+++ b/notes.c\n@@ -1306,3 +1306,17 @@ void expand_notes_ref(struct strbuf *sb)\n \telse\n \t\tstrbuf_insert(sb, 0, \"refs/notes/\", 11);\n }\n+\n+void expand_loose_notes_ref(struct strbuf *sb)\n+{\n+\tunsigned char object[20];\n+\n+\tif (get_sha1(sb->buf, object)) {\n+\t\t/* fallback to expand_notes_ref */\n+\t\texpand_notes_ref(sb);\n+\t} else {\n+\t\t/* we got an object, so replace the strbuf with the hex string */\n+\t\tstrbuf_reset(sb);\n+\t\tstrbuf_addstr(sb, sha1_to_hex(object));\n+\t}\n+}\ndiff --git a/notes.h b/notes.h\nindex e5d67fd3754a..658caf7d6e99 100644\n--- a/notes.h\n+++ b/notes.h\n@@ -302,4 +302,12 @@ void string_list_add_refs_from_colon_sep(struct string_list *list,\n /* Expand inplace a note ref like \"foo\" or \"notes/foo\" into \"refs/notes/foo\" */\n void expand_notes_ref(struct strbuf *sb);\n \n+/*\n+ * Similar to expand_notes_ref, but allows arbitrary refs to be expanded via\n+ * get_sha1 first. If get_sha1 fails to find a ref, fall back to traditional\n+ * expand_notes_ref. The contents of the strbuf will be suitable to attempt\n+ * passing to get_sha1 again inside the notes machinery.\n+ */\n+void expand_loose_notes_ref(struct strbuf *sb);\n+\n #endif\ndiff --git a/t/t3308-notes-merge.sh b/t/t3308-notes-merge.sh\nindex 24d82b49bbea..19aed7ec953b 100755\n--- a/t/t3308-notes-merge.sh\n+++ b/t/t3308-notes-merge.sh\n@@ -18,7 +18,9 @@ test_expect_success setup '\n \tgit notes add -m \"Notes on 1st commit\" 1st &&\n \tgit notes add -m \"Notes on 2nd commit\" 2nd &&\n \tgit notes add -m \"Notes on 3rd commit\" 3rd &&\n-\tgit notes add -m \"Notes on 4th commit\" 4th\n+\tgit notes add -m \"Notes on 4th commit\" 4th &&\n+\t# Copy notes to remote-notes\n+\tgit fetch . refs/notes/*:refs/remote-notes/origin/*\n '\n \n commit_sha1=$(git rev-parse 1st^{commit})\n@@ -66,7 +68,9 @@ test_expect_success 'verify initial notes (x)' '\n '\n \n cp expect_notes_x expect_notes_y\n+cp expect_notes_x expect_notes_v\n cp expect_log_x expect_log_y\n+cp expect_log_x expect_log_v\n \n test_expect_success 'fail to merge empty notes ref into empty notes ref (z => y)' '\n \ttest_must_fail git -c \"core.notesRef=refs/notes/y\" notes merge z\n@@ -84,16 +88,12 @@ test_expect_success 'fail to merge into various non-notes refs' '\n \ttest_must_fail git -c \"core.notesRef=refs/notes/foo^{bar\" notes merge x\n '\n \n-test_expect_success 'fail to merge various non-note-trees' '\n-\tgit config core.notesRef refs/notes/y &&\n-\ttest_must_fail git notes merge refs/notes &&\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+test_expect_success 'merge non-notes ref into empty notes ref (remote-notes/origin/x => v)' '\n+\tgit config core.notesRef refs/notes/v &&\n+\tgit notes merge refs/remote-notes/origin/x &&\n+\tverify_notes v &&\n+\t# refs/remote-notes/origin/x and v should point to the same notes commit\n+\ttest \"$(git rev-parse refs/remote-notes/origin/x)\" = \"$(git rev-parse refs/notes/v)\"\n '\n \n test_expect_success 'merge notes into empty notes ref (x => y)' '\n-- \n2.6.1.264.gbab76a9\n"},{"id":"273349","messageId":"CALKQrgcKxJqJn+3-rg4DCbT5CFDZW8o9GtCS=kh-iSy0YyGAUA@mail.gmail.com","threadId":"40791","inReplyTo":"1447432462-21192-1-git-send-email-jacob.e.keller@intel.com","subject":"Re: [PATCH] notes: allow merging from arbitrary references","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2015-11-15T22:14:11Z","receivedAt":"2015-11-15T22:14:11Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Fri, Nov 13, 2015 at 5:34 PM, Jacob Keller wrote:\n> From: Jacob Keller \n>\n> Create a new expansion function, expand_loose_notes_ref which will\n> expand any ref using get_sha1, but falls back to expand_notes_ref if\n> this fails. The contents of the strbuf will be either the hex string of\n> the sha1, or the expanded notes ref. It is expected to be re-expanded\n> using get_sha1 inside the notes merge machinery, and there is no real\n> error checking provided at this layer.\n>\n> Since we now support merging from non-notes refs, remove the test case\n> associated with that behavior. Add a test case for merging from a\n> non-notes ref.\n>\n> Signed-off-by: Jacob Keller \n> ---\n> I do not remember what version this was since it has been an age ago\n> that I sent the previous code. This is mostly just a rebase onto current\n> next. I believe I have covered everything previous reviewers noted.\n\nLooks good to me.\n\n> I'm interested in whether this is the right direction, as my longterm\n> goal is to be able to push/pull notes to a specific namespace (probably\n> refs/remote-notes/*, since actually modifying to use\n> refs/remotes/notes/* is difficult to send to users, and remote-notes\n> makes the most useful sense). The first part of this is allowing merge\n> to come from an arbitrary reference, as currently it is not really\n> possible to merge from refs/remote-notes as we'd need it to be.\n\nYes, I agree that merging from refs outside refs/notes/ should become possible.\n\nA related topic that has been discussed (although I cannot remember if\nany conclusion was reached) is whether to allow more notes operations\n- specifically _read-only_ operations - on notes trees outside\nrefs/notes/. I believe this should also become possible, although I\nhaven't thoroughly examined all implications.\n\n...Johan\n\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"273350","messageId":"CA+P7+xoyCwgYWaiVj0FNVHuaY=kUZA5a3LBMtpe6SirOVeK9rA@mail.gmail.com","threadId":"40791","inReplyTo":"CALKQrgcKxJqJn+3-rg4DCbT5CFDZW8o9GtCS=kh-iSy0YyGAUA@mail.gmail.com","subject":"Re: [PATCH] notes: allow merging from arbitrary references","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2015-11-15T23:23:51Z","receivedAt":"2015-11-15T23:23:51Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Sun, Nov 15, 2015 at 2:14 PM, Johan Herland wrote:\n>> ---\n>> I do not remember what version this was since it has been an age ago\n>> that I sent the previous code. This is mostly just a rebase onto current\n>> next. I believe I have covered everything previous reviewers noted.\n>\n> Looks good to me.\n>\n>> I'm interested in whether this is the right direction, as my longterm\n>> goal is to be able to push/pull notes to a specific namespace (probably\n>> refs/remote-notes/*, since actually modifying to use\n>> refs/remotes/notes/* is difficult to send to users, and remote-notes\n>> makes the most useful sense). The first part of this is allowing merge\n>> to come from an arbitrary reference, as currently it is not really\n>> possible to merge from refs/remote-notes as we'd need it to be.\n>\n> Yes, I agree that merging from refs outside refs/notes/ should become possible.\n>\n\nThanks.\n\n> A related topic that has been discussed (although I cannot remember if\n> any conclusion was reached) is whether to allow more notes operations\n> - specifically _read-only_ operations - on notes trees outside\n> refs/notes/. I believe this should also become possible, although I\n> haven't thoroughly examined all implications.\n>\n> ...Johan\n>\n>\n\nThis was discussed at some point on one of the versions of my patch.\nThe tricky part is in how to get it implemented correctly.\n\nWe need to be able to correctly handle DWIM logic for things, and\nensure that what we're operating on actually looks \"note-like\" since\nwe don't really want to perform read-only ops on refs that don't hold\nnotes like objects.\n\nRegards,\nJake\n"},{"id":"273353","messageId":"CALKQrgdDH2WZc-xi3ROLUBxdk=yVqfFGN3jN1GjQq4qJj_K+-A@mail.gmail.com","threadId":"40791","inReplyTo":"CA+P7+xoyCwgYWaiVj0FNVHuaY=kUZA5a3LBMtpe6SirOVeK9rA@mail.gmail.com","subject":"Re: [PATCH] notes: allow merging from arbitrary references","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2015-11-16T07:55:04Z","receivedAt":"2015-11-16T07:55:04Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Mon, Nov 16, 2015 at 12:23 AM, Jacob Keller wrote:\n> On Sun, Nov 15, 2015 at 2:14 PM, Johan Herland wrote:\n>> A related topic that has been discussed (although I cannot remember if\n>> any conclusion was reached) is whether to allow more notes operations\n>> - specifically _read-only_ operations - on notes trees outside\n>> refs/notes/. I believe this should also become possible, although I\n>> haven't thoroughly examined all implications.\n>\n> This was discussed at some point on one of the versions of my patch.\n> The tricky part is in how to get it implemented correctly.\n>\n> We need to be able to correctly handle DWIM logic for things, and\n> ensure that what we're operating on actually looks \"note-like\" since\n> we don't really want to perform read-only ops on refs that don't hold\n> notes like objects.\n\nI believe read-only operations on non-notes trees is harmless\n(although suboptimal). When reading in a notes tree, the notes code\nmaintains non-note entries in a sorted linked list. Only paths that\ncontain exactly 40 hex characters (modulo '/') ends up as \"notes\"\n(i.e. false positives). The rest ends up in the non-notes list. The\noverwhelming majority of non-notes trees will have no \"notes\" in them\n(zero false positives).\n\nFor those few trees that do contain note-like paths: since we never\nwrite out the tree again, we don't end up corrupting the non-notes\ntree itself (which would typically look like changing the \"fanout\" of\nnote-like paths, e.g. moving 'de/adbeef...' to 'deadbeef...'). Hence,\nthe only damage we can get from reading in a non-notes tree depend on\nwhat we subsequently do with the \"notes\" information read from that\ntree.\n\nAgain, since the number of \"notes\" read from a non-notes tree is\ntypically zero, the subsequent damage is typically, also, zero.\n\nFor \"git notes merge\", false positives from a non-notes tree are\nmerged into the first (proper) notes tree.\n\nFor \"git log --notes\", false positives end up being displayed as part\nof the output. Note that here, a false positive must not only match\nthe above criteria (40 hex chars, modulo '/'), but must also correctly\nname a commit that occurs in the log.\n\nAre there other cases where a false positive would wreak considerable havoc?\n\nAdditionally, if we suspect that passing non-notes trees to read-only\noperations will be a common error, we could add a simple heuristic to\nthe notes code, to warn (or even abort) if we strongly suspect that we\nare reading in a non-notes tree. For example, if the ratio of\nnon-notes to notes entries goes above, say, 1:1 (or even 10:1), then\nwhat we're reading is probably not a proper notes tree...\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"273380","messageId":"CA+P7+xqGwb6yejh+HZMt8cwx=4arR6+YKCNVdftuQe5SBY_X9w@mail.gmail.com","threadId":"40791","inReplyTo":"CALKQrgdDH2WZc-xi3ROLUBxdk=yVqfFGN3jN1GjQq4qJj_K+-A@mail.gmail.com","subject":"Re: [PATCH] notes: allow merging from arbitrary references","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2015-11-16T19:41:13Z","receivedAt":"2015-11-16T19:41:13Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Sun, Nov 15, 2015 at 11:55 PM, Johan Herland wrote:\n> Additionally, if we suspect that passing non-notes trees to read-only\n> operations will be a common error, we could add a simple heuristic to\n> the notes code, to warn (or even abort) if we strongly suspect that we\n> are reading in a non-notes tree. For example, if the ratio of\n> non-notes to notes entries goes above, say, 1:1 (or even 10:1), then\n> what we're reading is probably not a proper notes tree...\n>\n\nI agree here for this part, a possible heuristic check would maybe be\nvaluable.. but not sure it's super worth the effort. I doubt it would\nbe a common error, and I don't think the issues above would actually\ncause too many problems.\n\nThe main other issue is how to get notes DWIM things to work for all\ncases where we want to use notes refs, since right now the DWIM is\nbasically done at the top level and only handles notes like things.\nThe problem with it is that if you specify a full ref that *isn't*\nrefs/notes, you will always prefix it with refs/notes, like so:\n\nrefs/remote-notes/origin => refs/notes/refs/remote-notes/origin,\n\nThis makes it really difficult to expand a ref. However, Julio seemed\nto think this was a possibly valuable expansion under normal\ncircumstances. The current solution is to try to do a normal lookup\nfirst and only use the notes DWIM after we fail a lookup, which I\nthink is what the above patch attempts to do. This seems ok enough to\nme.\n\nRegards,\nJake\n\n>\n> ...Johan\n>\n"},{"id":"273487","messageId":"CALKQrge+agZ2NstnjkkVKmgqQRtE1cwiq6d7B9bP4_VApq+e_Q@mail.gmail.com","threadId":"40791","inReplyTo":"CA+P7+xqGwb6yejh+HZMt8cwx=4arR6+YKCNVdftuQe5SBY_X9w@mail.gmail.com","subject":"Re: [PATCH] notes: allow merging from arbitrary references","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2015-11-18T22:29:26Z","receivedAt":"2015-11-18T22:29:26Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Mon, Nov 16, 2015 at 8:41 PM, Jacob Keller wrote:\n> The main other issue is how to get notes DWIM things to work for all\n> cases where we want to use notes refs, since right now the DWIM is\n> basically done at the top level and only handles notes like things.\n> The problem with it is that if you specify a full ref that *isn't*\n> refs/notes, you will always prefix it with refs/notes, like so:\n>\n> refs/remote-notes/origin => refs/notes/refs/remote-notes/origin,\n\nI am becoming convinced that this is a bug. I don't know anywhere else\nin Git, where a fully qualified ref name (i.e. anything starting with\n\"refs/\") is not interpreted verbatim. For the notes code to do just\nthat adds unnecessary confusion.\n\n> This makes it really difficult to expand a ref. However, Junio seemed\n> to think this was a possibly valuable expansion under normal\n> circumstances.\n\nI doubt it. It carries its own set of problems in that refs/foo/bar\n(=> refs/notes/refs/foo/bar) is treated differently from refs/notes/bar\n(=> refs/notes/bar). If users _really_ want to create\nrefs/notes/refs/$whatever, they should have to be explicit about that\n(i.e. we should require them to say refs/notes/refs/$whatever instead\nof allowing them to lazily say refs/$whatever). (It even saves them\nfrom a potential bug if their $whatever happens to start with \"notes/\",\nin which case the current code already forces them to fully qualify...)\n\nI realize this is a backwards-incompatible change in behavior, but I\ndon't think it'll matter much in practice. Given e.g.\n\n git notes --ref refs/foo list\n\nwhen refs/foo and refs/notes/refs/foo is both missing:\n Current behavior: refs/notes/refs/foo lookup fails.\n Treat like empty notes tree; no output, exit code 0\n Proposed behavior: refs/foo lookup fails -> refs/notes/refs/foo\n lookup fails. Same behavior as current.\n\nwhen refs/notes/refs/foo exists:\n Current behavior: refs/notes/refs/foo lookup succeeds.\n Shows notes in that tree\n Proposed behavior: refs/foo lookup fails -> refs/notes/refs/foo\n lookup succeeds. Same as current.\n\nwhen refs/foo exists:\n Current behavior: refs/notes/refs/foo lookup fails. Treat like empty\n notes tree; no output, exit code 0\n Proposed behavior: refs/foo lookup succeeds. Load as notes tree,\n probably empty, hence no output, exit code 0\n\nwhen both refs/foo and refs/notes/refs/foo exist:\n Current behavior: refs/notes/refs/foo lookup succeeds. Shows notes\n in that tree\n Proposed behavior: refs/foo lookup succeeds. Load as notes tree,\n probably empty, hence no output, exit code 0\n\nIn other words, this change requires both refs/foo and\nrefs/notes/refs/foo to be present in order to cause any real confusion.\nAnd in that case, the proposed behavior forces you to use fully-\nqualified refs (which will be interpreted as such) whereas the current\nbehavior takes what looks like a fully-qualified ref (refs/foo) and\ninterprets it like a notes-shorthand (-> refs/notes/refs/foo), which\nI argue is probably more confusing to most users.\n\n> The current solution is to try to do a normal lookup\n> first and only use the notes DWIM after we fail a lookup, which I\n> think is what the above patch attempts to do. This seems ok enough to\n> me.\n\nYes, given $whatever, we should first lookup $whatever, and only\nfailing that, we should try refs/notes/$whatever. Maybe it's also\nworth trying refs/$whatever (before refs/notes/$whatever), since that\nwould be consistent with what's currently done for other refs (e.g.\ntry \"git log heads/master\" or \"git log tags/v2.6.3\" in git.git).\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"273488","messageId":"CA+P7+xoDi54ZQFJp7eK5p7QqtWHqpsjLNO5oo_Q5kpuwOXVbww@mail.gmail.com","threadId":"40791","inReplyTo":"CALKQrge+agZ2NstnjkkVKmgqQRtE1cwiq6d7B9bP4_VApq+e_Q@mail.gmail.com","subject":"Re: [PATCH] notes: allow merging from arbitrary references","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2015-11-18T22:38:29Z","receivedAt":"2015-11-18T22:38:29Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Wed, Nov 18, 2015 at 2:29 PM, Johan Herland wrote:\n> On Mon, Nov 16, 2015 at 8:41 PM, Jacob Keller wrote:\n>> The main other issue is how to get notes DWIM things to work for all\n>> cases where we want to use notes refs, since right now the DWIM is\n>> basically done at the top level and only handles notes like things.\n>> The problem with it is that if you specify a full ref that *isn't*\n>> refs/notes, you will always prefix it with refs/notes, like so:\n>>\n>> refs/remote-notes/origin => refs/notes/refs/remote-notes/origin,\n>\n> I am becoming convinced that this is a bug. I don't know anywhere else\n> in Git, where a fully qualified ref name (i.e. anything starting with\n> \"refs/\") is not interpreted verbatim. For the notes code to do just\n> that adds unnecessary confusion.\n>\n\nI agree, and I had a patch to change this behavior, but the main issue\nbeing I think it didn't fallback to the suggested proposal below.\n\n>> This makes it really difficult to expand a ref. However, Junio seemed\n>> to think this was a possibly valuable expansion under normal\n>> circumstances.\n>\n> I doubt it. It carries its own set of problems in that refs/foo/bar\n> (=> refs/notes/refs/foo/bar) is treated differently from refs/notes/bar\n> (=> refs/notes/bar). If users _really_ want to create\n> refs/notes/refs/$whatever, they should have to be explicit about that\n> (i.e. we should require them to say refs/notes/refs/$whatever instead\n> of allowing them to lazily say refs/$whatever). (It even saves them\n> from a potential bug if their $whatever happens to start with \"notes/\",\n> in which case the current code already forces them to fully qualify...)\n>\n\nThe question is whether we do:\n\na) check for refs/abc/xyz and fail if we can't find it or\n\nb) check for refs/abc/xyz and then expand to refs/notes/refs/abc/xyz\nif we can't?\n\nI think that the first is generally preferable but with read-only ops\nit's not a big deal since we won't be writing to notes trees.\n\n> I realize this is a backwards-incompatible change in behavior, but I\n> don't think it'll matter much in practice. Given e.g.\n>\n> git notes --ref refs/foo list\n>\n> when refs/foo and refs/notes/refs/foo is both missing:\n> Current behavior: refs/notes/refs/foo lookup fails.\n> Treat like empty notes tree; no output, exit code 0\n> Proposed behavior: refs/foo lookup fails -> refs/notes/refs/foo\n> lookup fails. Same behavior as current.\n>\n> when refs/notes/refs/foo exists:\n> Current behavior: refs/notes/refs/foo lookup succeeds.\n> Shows notes in that tree\n> Proposed behavior: refs/foo lookup fails -> refs/notes/refs/foo\n> lookup succeeds. Same as current.\n>\n> when refs/foo exists:\n> Current behavior: refs/notes/refs/foo lookup fails. Treat like empty\n> notes tree; no output, exit code 0\n> Proposed behavior: refs/foo lookup succeeds. Load as notes tree,\n> probably empty, hence no output, exit code 0\n>\n> when both refs/foo and refs/notes/refs/foo exist:\n> Current behavior: refs/notes/refs/foo lookup succeeds. Shows notes\n> in that tree\n> Proposed behavior: refs/foo lookup succeeds. Load as notes tree,\n> probably empty, hence no output, exit code 0\n>\n> In other words, this change requires both refs/foo and\n> refs/notes/refs/foo to be present in order to cause any real confusion.\n> And in that case, the proposed behavior forces you to use fully-\n> qualified refs (which will be interpreted as such) whereas the current\n> behavior takes what looks like a fully-qualified ref (refs/foo) and\n> interprets it like a notes-shorthand (-> refs/notes/refs/foo), which\n> I argue is probably more confusing to most users.\n>\n>> The current solution is to try to do a normal lookup\n>> first and only use the notes DWIM after we fail a lookup, which I\n>> think is what the above patch attempts to do. This seems ok enough to\n>> me.\n>\n> Yes, given $whatever, we should first lookup $whatever, and only\n> failing that, we should try refs/notes/$whatever. Maybe it's also\n> worth trying refs/$whatever (before refs/notes/$whatever), since that\n> would be consistent with what's currently done for other refs (e.g.\n> try \"git log heads/master\" or \"git log tags/v2.6.3\" in git.git).\n>\n>\n\nThe biggest implementation issue here is that the notes code that does\nDWIM happens before lookup of whether the ref exists, and the code\nthat does lookup of refs in the notes.c code won't fallback and try a\ndifferent expansion.\n\nI think that is why my proposed change was dropped, if I remember correctly.\n\nI am in agreement with you, and think we should use the proposed\nbehavior above, as it is very unlikely to cause any issues with\ncurrent cases, especially since we already try not to allow operation\non refs outside of the notes tree today.\n\nRegards,\nJake\n\n> ...Johan\n>\n> --\n> Johan Herland, \n> www.herland.net\n"},{"id":"273669","messageId":"20151124224709.GA13691@sigill.intra.peff.net","threadId":"40791","inReplyTo":"1447432462-21192-1-git-send-email-jacob.e.keller@intel.com","subject":"Re: [PATCH] notes: allow merging from arbitrary references","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-11-24T22:47:10Z","receivedAt":"2015-11-24T22:47:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 13, 2015 at 08:34:22AM -0800, Jacob Keller wrote:\n\n> ---\n> I do not remember what version this was since it has been an age ago\n> that I sent the previous code. This is mostly just a rebase onto current\n> next. I believe I have covered everything previous reviewers noted.\n\nPlease keep topics branched from master where possible. And if not\npossible, please indicate which topic in 'next' is required to build on.\n\nWe never merge 'next' itself, only individual topics from it. So I can't\njust apply your patch on top of 'next'.\n\nI did get it to apply on the current master with \"am -3\", but some tests\nin t3310 seem to fail. Can you take a look?\n\nI skimmed the discussion with Johan that followed this. Are we happy\nwith this as a first step, or would people rather look at re-working the\nnotes-ref lookups everywhere?\n\n-Peff\n"},{"id":"273676","messageId":"20151124234206.GA31949@sigill.intra.peff.net","threadId":"40791","inReplyTo":"20151124224709.GA13691@sigill.intra.peff.net","subject":"Re: [PATCH] notes: allow merging from arbitrary references","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-11-24T23:42:06Z","receivedAt":"2015-11-24T23:42:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 24, 2015 at 05:47:09PM -0500, Jeff King wrote:\n\n> On Fri, Nov 13, 2015 at 08:34:22AM -0800, Jacob Keller wrote:\n> \n> > ---\n> > I do not remember what version this was since it has been an age ago\n> > that I sent the previous code. This is mostly just a rebase onto current\n> > next. I believe I have covered everything previous reviewers noted.\n> \n> Please keep topics branched from master where possible. And if not\n> possible, please indicate which topic in 'next' is required to build on.\n> \n> We never merge 'next' itself, only individual topics from it. So I can't\n> just apply your patch on top of 'next'.\n> \n> I did get it to apply on the current master with \"am -3\", but some tests\n> in t3310 seem to fail. Can you take a look?\n\nI just noticed v2, which I missed earlier. But the same complaints\napply. :)\n\n-Peff\n"},{"id":"273783","messageId":"CA+P7+xpp4mF5iuZ+i_8vuB4BoHBvpmS=jq-xAeae7bdT9UGHhA@mail.gmail.com","threadId":"40791","inReplyTo":"20151124234206.GA31949@sigill.intra.peff.net","subject":"Re: [PATCH] notes: allow merging from arbitrary references","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2015-11-26T06:20:36Z","receivedAt":"2015-11-26T06:20:36Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Tue, Nov 24, 2015 at 3:42 PM, Jeff King wrote:\n> On Tue, Nov 24, 2015 at 05:47:09PM -0500, Jeff King wrote:\n>\n>> On Fri, Nov 13, 2015 at 08:34:22AM -0800, Jacob Keller wrote:\n>>\n>> > ---\n>> > I do not remember what version this was since it has been an age ago\n>> > that I sent the previous code. This is mostly just a rebase onto current\n>> > next. I believe I have covered everything previous reviewers noted.\n>>\n>> Please keep topics branched from master where possible. And if not\n>> possible, please indicate which topic in 'next' is required to build on.\n>>\n>> We never merge 'next' itself, only individual topics from it. So I can't\n>> just apply your patch on top of 'next'.\n>>\n>> I did get it to apply on the current master with \"am -3\", but some tests\n>> in t3310 seem to fail. Can you take a look?\n>\n> I just noticed v2, which I missed earlier. But the same complaints\n> apply. :)\n>\n> -Peff\n\nYea.. sorry about that. I normally work off next since this is what I\nuse day to day for general git use, as I like to run the bleedy edge.\n\nI can respin these on master, but it may take a bit of time as I am on\nvacation at the moment.\n\nI'm also curious if people would rather go the more difficult route\nfirst or not.\n\nRegards,\nJake\n"},{"id":"274288","messageId":"xmqqr3isepit.fsf@gitster.mtv.corp.google.com","threadId":"40791","inReplyTo":"20151124234206.GA31949@sigill.intra.peff.net","subject":"Re: [PATCH] notes: allow merging from arbitrary references","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-11T19:47:22Z","receivedAt":"2015-12-11T19:47:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Nov 24, 2015 at 05:47:09PM -0500, Jeff King wrote:\n>\n>> On Fri, Nov 13, 2015 at 08:34:22AM -0800, Jacob Keller wrote:\n>> \n>> > ---\n>> > I do not remember what version this was since it has been an age ago\n>> > that I sent the previous code. This is mostly just a rebase onto current\n>> > next. I believe I have covered everything previous reviewers noted.\n>> \n>> Please keep topics branched from master where possible. And if not\n>> possible, please indicate which topic in 'next' is required to build on.\n>> \n>> We never merge 'next' itself, only individual topics from it. So I can't\n>> just apply your patch on top of 'next'.\n>> \n>> I did get it to apply on the current master with \"am -3\", but some tests\n>> in t3310 seem to fail. Can you take a look?\n>\n> I just noticed v2, which I missed earlier. But the same complaints\n> apply. :)\n\nI tried to queue\n<1447432462-21192-1-git-send-email-jacob.e.keller@intel.com> from\nNov 13 directly on top of 'master', and t3310 does seem to fail.\n\nI'll discard the topic for now, expecting the resurrection sometime\nlater.\n"},{"id":"274291","messageId":"CA+P7+xp_4Mm+Y=jxOnva055ZWL05mVE3eWUyanzz8t4GU-bdsg@mail.gmail.com","threadId":"40791","inReplyTo":"xmqqr3isepit.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] notes: allow merging from arbitrary references","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2015-12-11T19:58:47Z","receivedAt":"2015-12-11T19:58:47Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Fri, Dec 11, 2015 at 11:47 AM, Junio C Hamano wrote:\n> I'll discard the topic for now, expecting the resurrection sometime\n> later.\n>\n\nHi,\n\nYes I fully intend to work this against master once I have some time\nagain. It may be a few weeks as I am extra busy for the holidays, and\nthe next posting will be fresh on the tip of the future-master :)\n\nRegards,\nJake\n"}]}