{"thread":{"id":"40470","subject":"[PATCH] notes: allow merging from arbitrary references","startedAt":"2015-10-01T22:39:25Z","lastAt":"2015-10-08T00:28:24Z","messageCount":3,"participants":["Jacob Keller"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"270990","messageId":"1443739165-17526-1-git-send-email-jacob.e.keller@intel.com","threadId":"40470","inReplyTo":null,"subject":"[PATCH] notes: allow merging from arbitrary references","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2015-10-01T22:39:25Z","receivedAt":"2015-10-01T22:39:25Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller <jacob.keller@gmail.com>\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 <jacob.keller@gmail.com>\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 3608c6478528..a029c3b976cc 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -806,7 +806,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\");\n@@ -833,7 +833,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 eacd2a61daf9..754405dcdb92 100644\n--- a/notes.c\n+++ b/notes.c\n@@ -1300,3 +1300,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 2a3f92338076..99944eb8507e 100644\n--- a/notes.h\n+++ b/notes.h\n@@ -294,4 +294,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.0.rc3.238.gc07a1e8\n"},{"id":"270991","messageId":"CA+P7+xp6XtsZ3jCN91+2Mt4Ad0VxRvd3Ra3yAWHktpYW2Ho7uw@mail.gmail.com","threadId":"40470","inReplyTo":"1443739165-17526-1-git-send-email-jacob.e.keller@intel.com","subject":"Re: [PATCH] notes: allow merging from arbitrary references","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2015-10-01T22:51:19Z","receivedAt":"2015-10-01T22:51:19Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Thu, Oct 1, 2015 at 3:39 PM, Jacob Keller <jacob.e.keller@intel.com> wrote:\n> From: Jacob Keller <jacob.keller@gmail.com>\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 <jacob.keller@gmail.com>\n> ---\n\nI haven't yet figured out how to handle showing of notes on non-notes\nrefs, since the core notes command uses expand_notes_ref very early on\nin the flow before we even know what kind of command we're using. I\nthink this will take a larger structural change to get right.\n\nFor merges it's pretty straight forward. This patch may seem weird,\nbut it's expected that the internal notes machinery will (re)expand\nthe remote_ref again and properly error out if it doesn't exist. We\nneed this since merging from a notes ref that doesn't exist isn't\nconsidered an error today (it just falls back to using an empty ref).\n\nNote that this does have some weird consequences, in that if you typo\na reference name such as refs/foo/bar which doesn't exist, then it\nwill be expanded into refs/notes/refs/foo/bar, which I am not sure is\nthe best approach, but otherwise things such as \"x\" don't properly\nexpand... I think in practice this will always result in the \"empty\"\nnotes ref being used for the source of the merge, so it may not be a\nbig deal.\n\nRegards,\nJake\n"},{"id":"271289","messageId":"CA+P7+xpu1rMHfM-Jk8X8H8Bw4bE+LhWJaG1v0KqZapbdCjH35g@mail.gmail.com","threadId":"40470","inReplyTo":"1443739165-17526-1-git-send-email-jacob.e.keller@intel.com","subject":"Re: [PATCH] notes: allow merging from arbitrary references","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2015-10-08T00:28:24Z","receivedAt":"2015-10-08T00:28:24Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Thu, Oct 1, 2015 at 3:39 PM, Jacob Keller <jacob.e.keller@intel.com> wrote:\n> From: Jacob Keller <jacob.keller@gmail.com>\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 <jacob.keller@gmail.com>\n> ---\n\n\nAnyone have any comments on this one? It's basically Junio's\nsuggestion of just using get_sha1 directly for the remote side of the\nnotes merge...\n\nRegards,\nJake\n"}]}