{"thread":{"id":"41086","subject":"[PATCH] notes: allow merging from arbitrary references","startedAt":"2015-12-28T18:19:23Z","lastAt":"2015-12-29T18:28:04Z","messageCount":4,"participants":["Jacob Keller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"275048","messageId":"1451326763-8447-1-git-send-email-jacob.e.keller@intel.com","threadId":"41086","inReplyTo":null,"subject":"[PATCH] notes: allow merging from arbitrary references","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2015-12-28T18:19:23Z","receivedAt":"2015-12-28T18:19:23Z","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 first\ncheck whether the ref can be found using get_sha1. If it can't be found\nthen it will fallback to using expand_notes_ref. The content of the\nstrbuf will not be changed if the notes ref can be located using\nget_sha1. Otherwise, it may be updated as done by expand_notes_ref.\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---\nDropping the version number since this is a resend of a very old patch\nand Junio already said he had dropped it from queue.\n\n builtin/notes.c        |  4 ++--\n notes.c                | 10 ++++++++++\n notes.h                |  7 +++++++\n t/t3308-notes-merge.sh | 22 +++++++++++-----------\n 4 files changed, 30 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 515cebbeb8a3..0d3e49ef66a8 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 db77922130b4..086cc483e518 100644\n--- a/notes.c\n+++ b/notes.c\n@@ -1303,3 +1303,13 @@ 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}\n+}\ndiff --git a/notes.h b/notes.h\nindex 2a3f92338076..431f14378817 100644\n--- a/notes.h\n+++ b/notes.h\n@@ -294,4 +294,11 @@ 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 will check whether the ref can be located\n+ * via get_sha1 first, and only falls back to expand_notes_ref in the case\n+ * where get_sha1 fails.\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.3.505.g5cc1fd1\n"},{"id":"275060","messageId":"xmqqbn9ajg1x.fsf@gitster.mtv.corp.google.com","threadId":"41086","inReplyTo":"1451326763-8447-1-git-send-email-jacob.e.keller@intel.com","subject":"Re: [PATCH] notes: allow merging from arbitrary references","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-28T23:42:50Z","receivedAt":"2015-12-28T23:42:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller <jacob.e.keller@intel.com> writes:\n\n> From: Jacob Keller <jacob.keller@gmail.com>\n>\n> Create a new expansion function, expand_loose_notes_ref which will first\n> check whether the ref can be found using get_sha1. If it can't be found\n> then it will fallback to using expand_notes_ref. The content of the\n> strbuf will not be changed if the notes ref can be located using\n> get_sha1. Otherwise, it may be updated as done by expand_notes_ref.\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> Dropping the version number since this is a resend of a very old patch\n> and Junio already said he had dropped it from queue.\n\nOK, will try to queue again (but I am cutting an -rc today so it may\nhave to wait a bit).  Those who have been involved in the notes topics\nneed to review it before the patch can make progress, though.\n\nThanks.\n\n\n>\n>  builtin/notes.c        |  4 ++--\n>  notes.c                | 10 ++++++++++\n>  notes.h                |  7 +++++++\n>  t/t3308-notes-merge.sh | 22 +++++++++++-----------\n>  4 files changed, 30 insertions(+), 13 deletions(-)\n>\n> diff --git a/builtin/notes.c b/builtin/notes.c\n> index 515cebbeb8a3..0d3e49ef66a8 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);\n> diff --git a/notes.c b/notes.c\n> index db77922130b4..086cc483e518 100644\n> --- a/notes.c\n> +++ b/notes.c\n> @@ -1303,3 +1303,13 @@ 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}\n> +}\n> diff --git a/notes.h b/notes.h\n> index 2a3f92338076..431f14378817 100644\n> --- a/notes.h\n> +++ b/notes.h\n> @@ -294,4 +294,11 @@ 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 will check whether the ref can be located\n> + * via get_sha1 first, and only falls back to expand_notes_ref in the case\n> + * where get_sha1 fails.\n> + */\n> +void expand_loose_notes_ref(struct strbuf *sb);\n> +\n>  #endif\n> diff --git a/t/t3308-notes-merge.sh b/t/t3308-notes-merge.sh\n> index 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"},{"id":"275066","messageId":"CA+P7+xp5+zqL43UJfdzsnQ9dBbufkUbaHk95W0j6L8j-L6KzAw@mail.gmail.com","threadId":"41086","inReplyTo":"xmqqbn9ajg1x.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-29T02:56:09Z","receivedAt":"2015-12-29T02:56:09Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Mon, Dec 28, 2015 at 3:42 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Jacob Keller <jacob.e.keller@intel.com> writes:\n>\n>> From: Jacob Keller <jacob.keller@gmail.com>\n>>\n>> Create a new expansion function, expand_loose_notes_ref which will first\n>> check whether the ref can be found using get_sha1. If it can't be found\n>> then it will fallback to using expand_notes_ref. The content of the\n>> strbuf will not be changed if the notes ref can be located using\n>> get_sha1. Otherwise, it may be updated as done by expand_notes_ref.\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>> Dropping the version number since this is a resend of a very old patch\n>> and Junio already said he had dropped it from queue.\n>\n> OK, will try to queue again (but I am cutting an -rc today so it may\n> have to wait a bit).  Those who have been involved in the notes topics\n> need to review it before the patch can make progress, though.\n>\n> Thanks.\n>\n>\n\nSome of them have reviewed previous versions, and I believe I have\nCc'ed them again with this patch.\n\nhttp://article.gmane.org/gmane.comp.version-control.git/281323\n\nHopefully they will have some time to look at this again.\n\nRegards,\nJake\n"},{"id":"275141","messageId":"xmqqfuylhzyj.fsf@gitster.mtv.corp.google.com","threadId":"41086","inReplyTo":"xmqqbn9ajg1x.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] notes: allow merging from arbitrary references","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-29T18:28:04Z","receivedAt":"2015-12-29T18:28:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> OK, will try to queue again (but I am cutting an -rc today so it may\n> have to wait a bit).  Those who have been involved in the notes topics\n> need to review it before the patch can make progress, though.\n\nJust FYI, with this the tip of 'pu' seems to fail a few tests in\nt3310.\n"}]}