{"thread":{"id":"41202","subject":"[PATCH v2] notes: allow merging from arbitrary references","startedAt":"2016-01-15T18:47:16Z","lastAt":"2016-01-17T10:00:27Z","messageCount":2,"participants":["Jacob Keller","Johan Herland"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"276184","messageId":"1452883636-27753-1-git-send-email-jacob.e.keller@intel.com","threadId":"41202","inReplyTo":null,"subject":"[PATCH v2] notes: allow merging from arbitrary references","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2016-01-15T18:47:16Z","receivedAt":"2016-01-15T18:47:16Z","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---\n\nNotes:\n    - v2\n    * don't expand notes-ref to the sha1, in order to support get-ref better\n    * fix failed tests due to mis-use of argv[0] instead of remote_ref.buf\n\nThis is a resend, since no one reviewed this last time, and it's been a\ncouple of weeks. At least one person has reviewed a previous version,\nbut I'd like some fresh eyes on this latest version. This should be\nidentical to the previous patch.\n\n builtin/notes.c        |  2 +-\n notes.c                | 10 ++++++++++\n notes.h                |  7 +++++++\n t/t3308-notes-merge.sh | 22 +++++++++++-----------\n 4 files changed, 29 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 515cebbeb8a3..c090e33dcadb 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\");\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":"276233","messageId":"CALKQrgfsnhpvqoEMjv40mvb_ZifgXddvimFugnXcMoQH-Hhi7A@mail.gmail.com","threadId":"41202","inReplyTo":"1452883636-27753-1-git-send-email-jacob.e.keller@intel.com","subject":"Re: [PATCH v2] notes: allow merging from arbitrary references","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2016-01-17T10:00:27Z","receivedAt":"2016-01-17T10:00:27Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Fri, Jan 15, 2016 at 7:47 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 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>\n> Notes:\n>     - v2\n>     * don't expand notes-ref to the sha1, in order to support get-ref better\n>     * fix failed tests due to mis-use of argv[0] instead of remote_ref.buf\n>\n> This is a resend, since no one reviewed this last time, and it's been a\n> couple of weeks. At least one person has reviewed a previous version,\n> but I'd like some fresh eyes on this latest version. This should be\n> identical to the previous patch.\n\nReviewed-by: Johan Herland <johan@herland.net>\n\n\n-- \nJohan Herland, <johan@herland.net>\nwww.herland.net\n"}]}