{"thread":{"id":"39983","subject":"[PATCH v2] notes: handle multiple worktrees","startedAt":"2015-07-31T17:44:57Z","lastAt":"2015-07-31T22:00:48Z","messageCount":4,"participants":["David Turner","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"267208","messageId":"1438364697-6647-1-git-send-email-dturner@twopensource.com","threadId":"39983","inReplyTo":null,"subject":"[PATCH v2] notes: handle multiple worktrees","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-07-31T17:44:57Z","receivedAt":"2015-07-31T17:44:57Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"Before creating NOTES_MERGE_REF, check NOTES_MERGE_REF using\ndie_if_shared_symref. This prevents simultaneous merges to the same\nnotes branch from different worktrees.\n\nSigned-off-by: David Turner \n---\n\nThis version addresses Eric Sunshine's critiques of v1. It breaks out\nthe symref-checking functionality into die_if_shared_symref, leaving\ndie_if_checked_out to handle HEAD.\n\n---\n builtin/notes.c | 2 ++\n t/t3320-notes-merge-worktrees.sh | 71 ++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 73 insertions(+)\n create mode 100755 t/t3320-notes-merge-worktrees.sh\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 63f95fc..78a264e 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -19,6 +19,7 @@\n #include \"string-list.h\"\n #include \"notes-merge.h\"\n #include \"notes-utils.h\"\n+#include \"branch.h\"\n \n static const char * const git_notes_usage[] = {\n \tN_(\"git notes [--ref ] [list []]\"),\n@@ -829,6 +830,7 @@ static int merge(int argc, const char **argv, const char *prefix)\n \t\tupdate_ref(msg.buf, \"NOTES_MERGE_PARTIAL\", result_sha1, NULL,\n \t\t\t 0, UPDATE_REFS_DIE_ON_ERR);\n \t\t/* Store ref-to-be-updated into .git/NOTES_MERGE_REF */\n+\t\tdie_if_shared_symref(\"NOTES_MERGE_REF\", default_notes_ref());\n \t\tif (create_symref(\"NOTES_MERGE_REF\", default_notes_ref(), NULL))\n \t\t\tdie(\"Failed to store link to current notes ref (%s)\",\n \t\t\t default_notes_ref());\ndiff --git a/t/t3320-notes-merge-worktrees.sh b/t/t3320-notes-merge-worktrees.sh\nnew file mode 100755\nindex 0000000..b102c23\n--- /dev/null\n+++ b/t/t3320-notes-merge-worktrees.sh\n@@ -0,0 +1,71 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2015 Twitter, Inc\n+#\n+\n+test_description='Test merging of notes trees in multiple worktrees'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup commit' '\n+\ttest_commit tantrum\n+'\n+\n+commit_tantrum=$(git rev-parse tantrum^{commit})\n+\n+test_expect_success 'setup notes ref (x)' '\n+\tgit config core.notesRef refs/notes/x &&\n+\tgit notes add -m \"x notes on tantrum\" tantrum\n+'\n+\n+test_expect_success 'setup local branch (y)' '\n+\tgit update-ref refs/notes/y refs/notes/x &&\n+\tgit config core.notesRef refs/notes/y &&\n+\tgit notes remove tantrum\n+'\n+\n+test_expect_success 'setup remote branch (z)' '\n+\tgit update-ref refs/notes/z refs/notes/x &&\n+\tgit config core.notesRef refs/notes/z &&\n+\tgit notes add -f -m \"conflicting notes on tantrum\" tantrum\n+'\n+\n+test_expect_success 'modify notes ref ourselves (x)' '\n+\tgit config core.notesRef refs/notes/x &&\n+\tgit notes add -f -m \"more conflicting notes on tantrum\" tantrum\n+'\n+\n+test_expect_success 'create some new worktrees' '\n+\tgit worktree add -b newbranch worktree master &&\n+\tgit worktree add -b newbranch2 worktree2 master\n+'\n+\n+test_expect_success 'merge z into y fails and sets NOTES_MERGE_REF' '\n+\tgit config core.notesRef refs/notes/y &&\n+\ttest_must_fail git notes merge z &&\n+\techo \"ref: refs/notes/y\" > expect &&\n+\ttest_cmp .git/NOTES_MERGE_REF expect\n+'\n+\n+test_expect_success 'merge z into y while mid-merge in another workdir fails' '\n+\t(\n+\t\tcd worktree &&\n+\t\tgit config core.notesRef refs/notes/y &&\n+\t\ttest_must_fail git notes merge z 2>err &&\n+\t\tgrep \"is already checked out\" err\n+\t) &&\n+\ttest_path_is_missing .git/worktrees/worktree/NOTES_MERGE_REF\n+'\n+\n+test_expect_success 'merge z into x while mid-merge on y succeeds' '\n+\t(\n+\t\tcd worktree2 &&\n+\t\tgit config core.notesRef refs/notes/x &&\n+\t\ttest_must_fail git notes merge z 2>&1 >out &&\n+\t\tgrep -v \"is already checked out\" out\n+\t) &&\n+\techo \"ref: refs/notes/x\" > expect &&\n+\ttest_cmp .git/worktrees/worktree2/NOTES_MERGE_REF expect\n+'\n+\n+test_done\n-- \n2.0.4.315.gad8727a-twtrsrc\n"},{"id":"267211","messageId":"xmqq8u9w5fnd.fsf@gitster.dls.corp.google.com","threadId":"39983","inReplyTo":"1438364697-6647-1-git-send-email-dturner@twopensource.com","subject":"Re: [PATCH v2] notes: handle multiple worktrees","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-31T18:46:30Z","receivedAt":"2015-07-31T18:46:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Turner writes:\n\n> Before creating NOTES_MERGE_REF, check NOTES_MERGE_REF using\n> die_if_shared_symref. This prevents simultaneous merges to the same\n> notes branch from different worktrees.\n>\n> Signed-off-by: David Turner \n> ---\n>\n> This version addresses Eric Sunshine's critiques of v1. It breaks out\n> the symref-checking functionality into die_if_shared_symref, leaving\n> die_if_checked_out to handle HEAD.\n\n>\n> ---\n> builtin/notes.c | 2 ++\n> t/t3320-notes-merge-worktrees.sh | 71 ++++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 73 insertions(+)\n> create mode 100755 t/t3320-notes-merge-worktrees.sh\n\nHmm, is this the whole thing?\n\nOr is a prerequisite patch to add that \"symref check\" infrastructure\nmissing on the receiving end?\n\nThis is a tangent, but there is a bigger spanner in \"bisect\". It\nuses refs/bisect/* to keep track of the current bisection state, but\nthat state is tied to a particular worktree, and it should not be\nshared across other worktrees.\n\nWe need to reserve a part of refs/* hierarchy (e.g. refs/private/*)\nfor per worktree private stuff and handle them just like we do\nper-worktree pseudorefs, or something like that.\n"},{"id":"267231","messageId":"1438379086.4735.25.camel@twopensource.com","threadId":"39983","inReplyTo":"xmqq8u9w5fnd.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2] notes: handle multiple worktrees","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-07-31T21:44:46Z","receivedAt":"2015-07-31T21:44:46Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Fri, 2015-07-31 at 11:46 -0700, Junio C Hamano wrote:\n> David Turner writes:\n> \n> > Before creating NOTES_MERGE_REF, check NOTES_MERGE_REF using\n> > die_if_shared_symref. This prevents simultaneous merges to the same\n> > notes branch from different worktrees.\n> >\n> > Signed-off-by: David Turner \n> > ---\n> >\n> > This version addresses Eric Sunshine's critiques of v1. It breaks out\n> > the symref-checking functionality into die_if_shared_symref, leaving\n> > die_if_checked_out to handle HEAD.\n> \n> >\n> > ---\n> > builtin/notes.c | 2 ++\n> > t/t3320-notes-merge-worktrees.sh | 71 ++++++++++++++++++++++++++++++++++++++++\n> > 2 files changed, 73 insertions(+)\n> > create mode 100755 t/t3320-notes-merge-worktrees.sh\n> \n> Hmm, is this the whole thing?\n> \n> Or is a prerequisite patch to add that \"symref check\" infrastructure\n> missing on the receiving end?\n\nOops, resent with first patch included -- I forgot I had split it.\n\n> This is a tangent, but there is a bigger spanner in \"bisect\". It\n> uses refs/bisect/* to keep track of the current bisection state, but\n> that state is tied to a particular worktree, and it should not be\n> shared across other worktrees.\n\nGood point. \n\n> We need to reserve a part of refs/* hierarchy (e.g. refs/private/*)\n> for per worktree private stuff and handle them just like we do\n> per-worktree pseudorefs, or something like that.\n\nIn the pseudorefs series, is_per_worktree_ref is a separate function\nprecisely so that we can make this sort of change. To bikeshed a bit, I\npropose worktree_refs/*, since:\n\n(a) \"private\" can have many meanings, and\n(b) common_list in path.c doesn't do exceptions (\"refs/ except\nrefs/worktree/\"). We could of course do a manual exception, but that's\nnot pretty.\n\nI further propose to add some patches (to go on top of the pseudorefs\nseries):\n\n(a) include worktree-refs/ in per_worktree_refs and include\nworktree-refs/ in per_worktree_refs in common_list, and\n(b) include changes to bisect which switches it over to use\nworktree_refs/*. To make this change compatible with bisections started\nwith old git versions, we should just unconditionally rename refs/bisect\nto worktree-refs/bisect, and, in the event of EISDIR (meaning that both\nrefs/bisect and worktree-refs/bisect exist), die with a message\nexplaining the situation.\n"},{"id":"267232","messageId":"xmqqmvyc3s33.fsf@gitster.dls.corp.google.com","threadId":"39983","inReplyTo":"1438379086.4735.25.camel@twopensource.com","subject":"Re: [PATCH v2] notes: handle multiple worktrees","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-31T22:00:48Z","receivedAt":"2015-07-31T22:00:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Turner writes:\n\n> I further propose to add some patches (to go on top of the pseudorefs\n> series):\n>\n> (a) include worktree-refs/ in per_worktree_refs and include\n> worktree-refs/ in per_worktree_refs in common_list, and\n\nDo you mean .git/worktree-refs/, just next to .git/refs? Then I\nthink it makes sense.\n\n> (b) include changes to bisect which switches it over to use\n> worktree_refs/*. To make this change compatible with bisections started\n> with old git versions,...\n\nI personally am not sure if it is worth the effort to cater to those\nwho start bisection, stop and then upgrade their git version and\nexpect to continue.\n"}]}