{"thread":{"id":"39984","subject":"[PATCH v3 1/2] worktrees: add die_if_shared_symref","startedAt":"2015-07-31T19:01:36Z","lastAt":"2015-07-31T22:08:45Z","messageCount":7,"participants":["David Turner","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":2},"messages":[{"id":"267213","messageId":"1438369297-4838-1-git-send-email-dturner@twopensource.com","threadId":"39984","inReplyTo":null,"subject":"[PATCH v3 1/2] worktrees: add die_if_shared_symref","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-07-31T19:01:36Z","receivedAt":"2015-07-31T19:01:36Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"Add a new function, die_if_shared_symref, which works like\ndie_if_checked_out, but for all references.  Refactor\ndie_if_checked_out to work in terms of die_if_shared_symref.\n\nSoon, we will use die_if_shared_symref to protect notes merges in\nworktrees.\n\nSigned-off-by: David Turner <dturner@twopensource.com>\n---\n\nOops, forgot I had split this into two patches.  This v3 includes the first patch.\n\n---\n branch.c | 20 +++++++++++++-------\n branch.h |  7 +++++++\n 2 files changed, 20 insertions(+), 7 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex c85be07..1f1b385 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -311,21 +311,22 @@ void remove_branch_state(void)\n \tunlink(git_path(\"SQUASH_MSG\"));\n }\n \n-static void check_linked_checkout(const char *branch, const char *id)\n+static void check_linked_checkout(const char *symref, const char *branch,\n+\t\t\t\t  const char *id)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n \tstruct strbuf path = STRBUF_INIT;\n \tstruct strbuf gitdir = STRBUF_INIT;\n \n \t/*\n-\t * $GIT_COMMON_DIR/HEAD is practically outside\n+\t * $GIT_COMMON_DIR/$symref is practically outside\n \t * $GIT_DIR so resolve_ref_unsafe() won't work (it\n \t * uses git_path). Parse the ref ourselves.\n \t */\n \tif (id)\n-\t\tstrbuf_addf(&path, \"%s/worktrees/%s/HEAD\", get_git_common_dir(), id);\n+\t\tstrbuf_addf(&path, \"%s/worktrees/%s/%s\", get_git_common_dir(), id, symref);\n \telse\n-\t\tstrbuf_addf(&path, \"%s/HEAD\", get_git_common_dir());\n+\t\tstrbuf_addf(&path, \"%s/%s\", get_git_common_dir(), symref);\n \n \tif (!strbuf_readlink(&sb, path.buf, 0)) {\n \t\tif (!starts_with(sb.buf, \"refs/\") ||\n@@ -356,13 +357,13 @@ done:\n \tstrbuf_release(&gitdir);\n }\n \n-void die_if_checked_out(const char *branch)\n+void die_if_shared_symref(const char *symref, const char *branch)\n {\n \tstruct strbuf path = STRBUF_INIT;\n \tDIR *dir;\n \tstruct dirent *d;\n \n-\tcheck_linked_checkout(branch, NULL);\n+\tcheck_linked_checkout(symref, branch, NULL);\n \n \tstrbuf_addf(&path, \"%s/worktrees\", get_git_common_dir());\n \tdir = opendir(path.buf);\n@@ -373,7 +374,12 @@ void die_if_checked_out(const char *branch)\n \twhile ((d = readdir(dir)) != NULL) {\n \t\tif (!strcmp(d->d_name, \".\") || !strcmp(d->d_name, \"..\"))\n \t\t\tcontinue;\n-\t\tcheck_linked_checkout(branch, d->d_name);\n+\t\tcheck_linked_checkout(symref, branch, d->d_name);\n \t}\n \tclosedir(dir);\n }\n+\n+void die_if_checked_out(const char *branch)\n+{\n+\tdie_if_shared_symref(\"HEAD\", branch);\n+}\ndiff --git a/branch.h b/branch.h\nindex 58aa45f..b2fca30 100644\n--- a/branch.h\n+++ b/branch.h\n@@ -59,4 +59,11 @@ extern int read_branch_desc(struct strbuf *, const char *branch_name);\n  */\n extern void die_if_checked_out(const char *branch);\n \n+/*\n+ * Check if a symref points to a branch in the main worktree or any linked\n+ * worktree and die (with a message describing its checkout location) if\n+ * it is.\n+ */\n+extern void die_if_shared_symref(const char *symref, const char *branch);\n+\n #endif\n-- \n2.0.4.315.gad8727a-twtrsrc\n"},{"id":"267214","messageId":"1438369297-4838-2-git-send-email-dturner@twopensource.com","threadId":"39984","inReplyTo":"1438369297-4838-1-git-send-email-dturner@twopensource.com","subject":"[PATCH v3 2/2] notes: handle multiple worktrees","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-07-31T19:01:37Z","receivedAt":"2015-07-31T19:01:37Z","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 <dturner@twopensource.com>\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 <notes-ref>] [list [<object>]]\"),\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":"267221","messageId":"CAPig+cTX0Wav2QSFi734uLVpjdmwFKzcesgkpmVKCPomMB_HyA@mail.gmail.com","threadId":"39984","inReplyTo":"1438369297-4838-1-git-send-email-dturner@twopensource.com","subject":"Re: [PATCH v3 1/2] worktrees: add die_if_shared_symref","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-07-31T19:35:18Z","receivedAt":"2015-07-31T19:35:18Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Jul 31, 2015 at 3:01 PM, David Turner <dturner@twopensource.com> wrote:\n> Add a new function, die_if_shared_symref, which works like\n> die_if_checked_out, but for all references.  Refactor\n> die_if_checked_out to work in terms of die_if_shared_symref.\n>\n> Soon, we will use die_if_shared_symref to protect notes merges in\n> worktrees.\n\nI wonder if the diagnostic:\n\n    'blorp' is already checked out at '/path/name/'\n\nemitted by check_linked_checkout() needs to be adjusted for this\nchange. It still makes sense for die_if_checked_out(), but sounds odd\nfor die_if_shared_symref().\n\n> Signed-off-by: David Turner <dturner@twopensource.com>\n> ---\n> diff --git a/branch.h b/branch.h\n> index 58aa45f..b2fca30 100644\n> --- a/branch.h\n> +++ b/branch.h\n> @@ -59,4 +59,11 @@ extern int read_branch_desc(struct strbuf *, const char *branch_name);\n>   */\n>  extern void die_if_checked_out(const char *branch);\n>\n> +/*\n> + * Check if a symref points to a branch in the main worktree or any linked\n> + * worktree and die (with a message describing its checkout location) if\n> + * it is.\n\nDitto here. This is still talking about \"branch\" and \"checkout\",\nwhereas it's actually checking if a symref in some worktree already\nreferences the given referent.\n\n> + */\n> +extern void die_if_shared_symref(const char *symref, const char *branch);\n\ns/branch/referent/ or something?\n"},{"id":"267223","messageId":"xmqqr3no3xau.fsf@gitster.dls.corp.google.com","threadId":"39984","inReplyTo":"CAPig+cTX0Wav2QSFi734uLVpjdmwFKzcesgkpmVKCPomMB_HyA@mail.gmail.com","subject":"Re: [PATCH v3 1/2] worktrees: add die_if_shared_symref","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-31T20:08:09Z","receivedAt":"2015-07-31T20:08:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> On Fri, Jul 31, 2015 at 3:01 PM, David Turner <dturner@twopensource.com> wrote:\n>> Add a new function, die_if_shared_symref, which works like\n>> die_if_checked_out, but for all references.  Refactor\n>> die_if_checked_out to work in terms of die_if_shared_symref.\n>>\n>> Soon, we will use die_if_shared_symref to protect notes merges in\n>> worktrees.\n>\n> I wonder if the diagnostic:\n>\n>     'blorp' is already checked out at '/path/name/'\n>\n> emitted by check_linked_checkout() needs to be adjusted for this\n> change. It still makes sense for die_if_checked_out(), but sounds odd\n> for die_if_shared_symref().\n\nTrue.  Lift the dying one callstack up, and make the lower level\nhelper check_shared_symref() or something that returns NULL (ok) or\nthat '/path/name' upon an error?\n\nAlso I suspect that this comment will become hard to grok after the\ncommit is actually made:\n\n>  \t/*\n> -\t * $GIT_COMMON_DIR/HEAD is practically outside\n> +\t * $GIT_COMMON_DIR/$symref is practically outside\n>  \t * $GIT_DIR so resolve_ref_unsafe() won't work (it\n>  \t * uses git_path). Parse the ref ourselves.\n>  \t */\n\nA reviewer who is viewing both the pre- and post- text of the patch\ncan see it used to say HEAD and now it is extended to $symref, but\nit would help to have \"(e.g. HEAD)\" after \"...DIR/$symref\", I think.\n"},{"id":"267226","messageId":"1438377312.4735.10.camel@twopensource.com","threadId":"39984","inReplyTo":"CAPig+cTX0Wav2QSFi734uLVpjdmwFKzcesgkpmVKCPomMB_HyA@mail.gmail.com","subject":"Re: [PATCH v3 1/2] worktrees: add die_if_shared_symref","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-07-31T21:15:12Z","receivedAt":"2015-07-31T21:15:12Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Fri, 2015-07-31 at 15:35 -0400, Eric Sunshine wrote:\n> On Fri, Jul 31, 2015 at 3:01 PM, David Turner <dturner@twopensource.com> wrote:\n> > Add a new function, die_if_shared_symref, which works like\n> > die_if_checked_out, but for all references.  Refactor\n> > die_if_checked_out to work in terms of die_if_shared_symref.\n> >\n> > Soon, we will use die_if_shared_symref to protect notes merges in\n> > worktrees.\n> \n> I wonder if the diagnostic:\n> \n>     'blorp' is already checked out at '/path/name/'\n> \n> emitted by check_linked_checkout() needs to be adjusted for this\n> change. It still makes sense for die_if_checked_out(), but sounds odd\n> for die_if_shared_symref().\n\nHow about:\n\n'refs/notes/y' is already referenced from 'NOTES_MERGE_REF' in\n'/home/dturner/git/t/trash directory.t3320-notes-merge-worktrees/'\n\nDoes that make sense?\n\nIt's not the only place in the error messages that mentions\nNOTES_MERGE_REF, so I think we expect users to understand\nNOTES_MERGE_REF.  The alternative is to move the error handling to an\neven higher level so we can give a notes-specific message.  I don't\nthink that's necessary, but I'll do it if others do.\n\n<snip; will fix>\n> > + */\n> > +extern void die_if_shared_symref(const char *symref, const char *branch);\n> \n> s/branch/referent/ or something?\n\nI went with \"target\" (by analogy to the target of a symlink).  \n"},{"id":"267228","messageId":"CAPig+cRZ4PicP7J0Hdwb1+E9JbE=wsfqEaVX6X4ERST2_nJrxw@mail.gmail.com","threadId":"39984","inReplyTo":"1438377312.4735.10.camel@twopensource.com","subject":"Re: [PATCH v3 1/2] worktrees: add die_if_shared_symref","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-07-31T21:36:17Z","receivedAt":"2015-07-31T21:36:17Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Jul 31, 2015 at 5:15 PM, David Turner <dturner@twopensource.com> wrote:\n> On Fri, 2015-07-31 at 15:35 -0400, Eric Sunshine wrote:\n>> On Fri, Jul 31, 2015 at 3:01 PM, David Turner <dturner@twopensource.com> wrote:\n>> > Add a new function, die_if_shared_symref, which works like\n>> > die_if_checked_out, but for all references.  Refactor\n>> > die_if_checked_out to work in terms of die_if_shared_symref.\n>> >\n>> > Soon, we will use die_if_shared_symref to protect notes merges in\n>> > worktrees.\n>>\n>> I wonder if the diagnostic:\n>>\n>>     'blorp' is already checked out at '/path/name/'\n>>\n>> emitted by check_linked_checkout() needs to be adjusted for this\n>> change. It still makes sense for die_if_checked_out(), but sounds odd\n>> for die_if_shared_symref().\n>\n> How about:\n>\n> 'refs/notes/y' is already referenced from 'NOTES_MERGE_REF' in\n> '/home/dturner/git/t/trash directory.t3320-notes-merge-worktrees/'\n>\n> Does that make sense?\n\nThat might be the best we can do for the generic case of\ndie_if_shared_symref(), but I wonder how easily the typical user would\nunderstand it when trying to checkout a branch already checked out\nelsewhere. One concern is that that message almost comes across as an\ninternal Git error (thus inscrutable), whereas:\n\n    % git checkout blorp\n    'blorp' is already checked out at '/some/path/'\n\nseems (to me) pretty clearly a user error, thus more easily understood.\n\n> It's not the only place in the error messages that mentions\n> NOTES_MERGE_REF, so I think we expect users to understand\n> NOTES_MERGE_REF.  The alternative is to move the error handling to an\n> even higher level so we can give a notes-specific message.  I don't\n> think that's necessary, but I'll do it if others do.\n\nGiven such precedence, the generic error message might indeed be fine\nfor the notes merge case, however, in general, you'd probably get\nbetter and more understandable error messages by tailoring them at the\ncall sites. (That's a positive vote from me for for lifting error\nhandling to a higher level.)\n"},{"id":"267233","messageId":"1438380525.4735.26.camel@twopensource.com","threadId":"39984","inReplyTo":"CAPig+cRZ4PicP7J0Hdwb1+E9JbE=wsfqEaVX6X4ERST2_nJrxw@mail.gmail.com","subject":"Re: [PATCH v3 1/2] worktrees: add die_if_shared_symref","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-07-31T22:08:45Z","receivedAt":"2015-07-31T22:08:45Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Fri, 2015-07-31 at 17:36 -0400, Eric Sunshine wrote:\n> On Fri, Jul 31, 2015 at 5:15 PM, David Turner <dturner@twopensource.com> wrote:\n> > On Fri, 2015-07-31 at 15:35 -0400, Eric Sunshine wrote:\n> >> On Fri, Jul 31, 2015 at 3:01 PM, David Turner <dturner@twopensource.com> wrote:\n> >> > Add a new function, die_if_shared_symref, which works like\n> >> > die_if_checked_out, but for all references.  Refactor\n> >> > die_if_checked_out to work in terms of die_if_shared_symref.\n> >> >\n> >> > Soon, we will use die_if_shared_symref to protect notes merges in\n> >> > worktrees.\n> >>\n> >> I wonder if the diagnostic:\n> >>\n> >>     'blorp' is already checked out at '/path/name/'\n> >>\n> >> emitted by check_linked_checkout() needs to be adjusted for this\n> >> change. It still makes sense for die_if_checked_out(), but sounds odd\n> >> for die_if_shared_symref().\n> >\n> > How about:\n> >\n> > 'refs/notes/y' is already referenced from 'NOTES_MERGE_REF' in\n> > '/home/dturner/git/t/trash directory.t3320-notes-merge-worktrees/'\n> >\n> > Does that make sense?\n> \n> That might be the best we can do for the generic case of\n> die_if_shared_symref(), but I wonder how easily the typical user would\n> understand it when trying to checkout a branch already checked out\n> elsewhere. One concern is that that message almost comes across as an\n> internal Git error (thus inscrutable), whereas:\n> \n>     % git checkout blorp\n>     'blorp' is already checked out at '/some/path/'\n> \n> seems (to me) pretty clearly a user error, thus more easily understood.\n\nSorry, I meant that for the non-HEAD case.  For the\ndie_if_already_checked_out case, the original error message would\nremain.\n\nBut I'll just go ahead and do it the nice way.\n"}]}