{"thread":{"id":"40003","subject":"[PATCH v5 1/2] worktrees: add find_shared_symref","startedAt":"2015-08-03T18:48:25Z","lastAt":"2015-08-09T02:56:53Z","messageCount":4,"participants":["David Turner","Eric Sunshine"],"isPatch":true,"patchVersion":5,"patchTotal":2},"messages":[{"id":"267332","messageId":"1438627706-6743-1-git-send-email-dturner@twopensource.com","threadId":"40003","inReplyTo":null,"subject":"[PATCH v5 1/2] worktrees: add find_shared_symref","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-08-03T18:48:25Z","receivedAt":"2015-08-03T18:48:25Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"Add a new function, find_shared_symref, which contains the heart of\ndie_if_checked_out, but works for any symref, not just HEAD.  Refactor\ndie_if_checked_out to use the same infrastructure as\nfind_shared_symref.\n\nSoon, we will use find_shared_symref to protect notes merges in\nworktrees.\n\nSigned-off-by: David Turner <dturner@twopensource.com>\n---\n\nThis reroll fixes issues reported by Eric Sunshine: leaks\nand Johan Herland: prepositions and broken &&\n\n---\n branch.c | 45 ++++++++++++++++++++++++++++++++++-----------\n branch.h |  8 ++++++++\n 2 files changed, 42 insertions(+), 11 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex c85be07..d2b3586 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 char *find_linked_symref(const char *symref, const char *branch,\n+\t\t\t\tconst 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_DIR so resolve_ref_unsafe() won't work (it\n-\t * uses git_path). Parse the ref ourselves.\n+\t * $GIT_COMMON_DIR/$symref (e.g. HEAD) is practically outside\n+\t * $GIT_DIR so resolve_ref_unsafe() won't work (it uses\n+\t * 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@@ -349,31 +350,53 @@ static void check_linked_checkout(const char *branch, const char *id)\n \t\tstrbuf_addstr(&gitdir, get_git_common_dir());\n \tskip_prefix(branch, \"refs/heads/\", &branch);\n \tstrbuf_strip_suffix(&gitdir, \".git\");\n-\tdie(_(\"'%s' is already checked out at '%s'\"), branch, gitdir.buf);\n+\n+\tstrbuf_release(&path);\n+\tstrbuf_release(&sb);\n+\treturn strbuf_detach(&gitdir, NULL);\n done:\n \tstrbuf_release(&path);\n \tstrbuf_release(&sb);\n \tstrbuf_release(&gitdir);\n+\n+\treturn NULL;\n }\n \n-void die_if_checked_out(const char *branch)\n+char *find_shared_symref(const char *symref, const char *target)\n {\n \tstruct strbuf path = STRBUF_INIT;\n \tDIR *dir;\n \tstruct dirent *d;\n+\tchar *existing;\n \n-\tcheck_linked_checkout(branch, NULL);\n+\tif ((existing = find_linked_symref(symref, target, NULL)))\n+\t\treturn existing;\n \n \tstrbuf_addf(&path, \"%s/worktrees\", get_git_common_dir());\n \tdir = opendir(path.buf);\n \tstrbuf_release(&path);\n \tif (!dir)\n-\t\treturn;\n+\t\treturn NULL;\n \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\texisting = find_linked_symref(symref, target, d->d_name);\n+\t\tif (existing) {\n+\t\t\tclosedir(dir);\n+\t\t\treturn existing;\n+\t\t}\n \t}\n \tclosedir(dir);\n+\n+\treturn NULL;\n+}\n+\n+void die_if_checked_out(const char *branch)\n+{\n+\tchar *existing;\n+\n+\texisting = find_shared_symref(\"HEAD\", branch);\n+\tif (existing)\n+\t\tdie(_(\"'%s' is already checked out at '%s'\"), branch, existing);\n }\ndiff --git a/branch.h b/branch.h\nindex 58aa45f..d3446ed 100644\n--- a/branch.h\n+++ b/branch.h\n@@ -59,4 +59,12 @@ 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 per-worktree symref points to a ref in the main worktree\n+ * or any linked worktree, and return the path to the exising worktree\n+ * if it is.  Returns NULL if there is no existing ref.  The caller is\n+ * responsible for freeing the returned path.\n+ */\n+extern char *find_shared_symref(const char *symref, const char *target);\n+\n #endif\n-- \n2.0.4.315.gad8727a-twtrsrc\n"},{"id":"267333","messageId":"1438627706-6743-2-git-send-email-dturner@twopensource.com","threadId":"40003","inReplyTo":"1438627706-6743-1-git-send-email-dturner@twopensource.com","subject":"[PATCH v5 2/2] notes: handle multiple worktrees","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-08-03T18:48:26Z","receivedAt":"2015-08-03T18:48:26Z","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\nfind_shared_symref and die if we find one.  This prevents simultaneous\nmerges to the same notes branch from different worktrees.\n\nSigned-off-by: David Turner <dturner@twopensource.com>\n---\n builtin/notes.c                  |  6 ++++\n t/t3320-notes-merge-worktrees.sh | 72 ++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 78 insertions(+)\n create mode 100755 t/t3320-notes-merge-worktrees.sh\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 63f95fc..0423480 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@@ -825,10 +826,15 @@ static int merge(int argc, const char **argv, const char *prefix)\n \t\tupdate_ref(msg.buf, default_notes_ref(), result_sha1, NULL,\n \t\t\t   0, UPDATE_REFS_DIE_ON_ERR);\n \telse { /* Merge has unresolved conflicts */\n+\t\tchar *existing;\n \t\t/* Update .git/NOTES_MERGE_PARTIAL with partial merge result */\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\texisting = find_shared_symref(\"NOTES_MERGE_REF\", default_notes_ref());\n+\t\tif (existing)\n+\t\t\tdie(_(\"A notes merge into %s is already in-progress at %s\"),\n+\t\t\t    default_notes_ref(), existing);\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..a7beef2\n--- /dev/null\n+++ b/t/t3320-notes-merge-worktrees.sh\n@@ -0,0 +1,72 @@\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 \"A notes merge into refs/notes/y is already in-progress at\" 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 \"Automatic notes merge failed\" out &&\n+\t\tgrep -v \"A notes merge into refs/notes/x is already in-progress in\" 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":"267371","messageId":"CAPig+cTFWn6mWtmmG9KrDhK1++r7fgXM_mK4BvCktThoErnBzA@mail.gmail.com","threadId":"40003","inReplyTo":"1438627706-6743-1-git-send-email-dturner@twopensource.com","subject":"Re: [PATCH v5 1/2] worktrees: add find_shared_symref","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-03T23:06:11Z","receivedAt":"2015-08-03T23:06:11Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Aug 3, 2015 at 2:48 PM, David Turner <dturner@twopensource.com> wrote:\n> Add a new function, find_shared_symref, which contains the heart of\n> die_if_checked_out, but works for any symref, not just HEAD.  Refactor\n> die_if_checked_out to use the same infrastructure as\n> find_shared_symref.\n>\n> Soon, we will use find_shared_symref to protect notes merges in\n> worktrees.\n\nA couple comments below. The first may be worth a re-roll; the second\nis more a taste thing. Neither is blocking if you don't happen to\nre-roll.\n\n> Signed-off-by: David Turner <dturner@twopensource.com>\n> ---\n> diff --git a/branch.c b/branch.c\n> index c85be07..d2b3586 100644\n> --- a/branch.c\n> +++ b/branch.c\n> @@ -349,31 +350,53 @@ static void check_linked_checkout(const char *branch, const char *id)\n>                 strbuf_addstr(&gitdir, get_git_common_dir());\n>         skip_prefix(branch, \"refs/heads/\", &branch);\n>         strbuf_strip_suffix(&gitdir, \".git\");\n> -       die(_(\"'%s' is already checked out at '%s'\"), branch, gitdir.buf);\n> +\n> +       strbuf_release(&path);\n> +       strbuf_release(&sb);\n> +       return strbuf_detach(&gitdir, NULL);\n>  done:\n>         strbuf_release(&path);\n>         strbuf_release(&sb);\n>         strbuf_release(&gitdir);\n> +\n> +       return NULL;\n>  }\n\nThis would be cleaner and less redundant if you assign the existing\nlocation to a variable and just fall through to the 'done' label:\n\n    char *existing = NULL;\n    ...\n        skip_prefix(branch, \"refs/heads/\", &branch);\n        strbuf_strip_suffix(&gitdir, \".git\");\n        existing = strbuf_detach(&gitdir, NULL);\n    done:\n        strbuf_release(&path);\n        strbuf_release(&sb);\n        strbuf_release(&gitdir);\n        return existing;\n\nThere's no worry that the \"existing\" path will be clobbered by\nstrbuf_release(&gitdir) since it's been detached already (and it's\nsafe to release the strbuf without affecting what has been detached\nfrom it).\n\n> -void die_if_checked_out(const char *branch)\n> +char *find_shared_symref(const char *symref, const char *target)\n>  {\n>         struct strbuf path = STRBUF_INIT;\n>         DIR *dir;\n>         struct dirent *d;\n> +       char *existing;\n>\n> -       check_linked_checkout(branch, NULL);\n> +       if ((existing = find_linked_symref(symref, target, NULL)))\n> +               return existing;\n>\n>         strbuf_addf(&path, \"%s/worktrees\", get_git_common_dir());\n>         dir = opendir(path.buf);\n>         strbuf_release(&path);\n>         if (!dir)\n> -               return;\n> +               return NULL;\n>\n>         while ((d = readdir(dir)) != NULL) {\n>                 if (!strcmp(d->d_name, \".\") || !strcmp(d->d_name, \"..\"))\n>                         continue;\n> -               check_linked_checkout(branch, d->d_name);\n> +               existing = find_linked_symref(symref, target, d->d_name);\n> +               if (existing) {\n> +                       closedir(dir);\n> +                       return existing;\n\nFor consistency with code nearby, this could have been handled by\nadding a 'done' label above the closedir() below and jumping to it\nfrom here, and then 'return existing'.\n\n> +               }\n>         }\n>         closedir(dir);\n> +\n> +       return NULL;\n> +}\n> +\n> +void die_if_checked_out(const char *branch)\n> +{\n> +       char *existing;\n> +\n> +       existing = find_shared_symref(\"HEAD\", branch);\n> +       if (existing)\n> +               die(_(\"'%s' is already checked out at '%s'\"), branch, existing);\n>  }\n"},{"id":"267665","messageId":"CAPig+cS9s4Odk6dXtBZdq8++KEjiYOa05uYc571Oa8FJG99srg@mail.gmail.com","threadId":"40003","inReplyTo":"CAPig+cTFWn6mWtmmG9KrDhK1++r7fgXM_mK4BvCktThoErnBzA@mail.gmail.com","subject":"Re: [PATCH v5 1/2] worktrees: add find_shared_symref","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-09T02:56:53Z","receivedAt":"2015-08-09T02:56:53Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Aug 3, 2015 at 7:06 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Mon, Aug 3, 2015 at 2:48 PM, David Turner <dturner@twopensource.com> wrote:\n>> Add a new function, find_shared_symref, which contains the heart of\n>> die_if_checked_out, but works for any symref, not just HEAD.  Refactor\n>> die_if_checked_out to use the same infrastructure as\n>> find_shared_symref.\n>>\n>> Soon, we will use find_shared_symref to protect notes merges in\n>> worktrees.\n>>\n>> Signed-off-by: David Turner <dturner@twopensource.com>\n>> ---\n>> diff --git a/branch.c b/branch.c\n>> index c85be07..d2b3586 100644\n>> --- a/branch.c\n>> +++ b/branch.c\n>> @@ -349,31 +350,53 @@ static void check_linked_checkout(const char *branch, const char *id)\n>>                 strbuf_addstr(&gitdir, get_git_common_dir());\n>>         skip_prefix(branch, \"refs/heads/\", &branch);\n>>         strbuf_strip_suffix(&gitdir, \".git\");\n>> -       die(_(\"'%s' is already checked out at '%s'\"), branch, gitdir.buf);\n\nIn addition to the other issues already mentioned, this one is a bit\nmore serious (though still rather superficial):\n\nWith this change, the skip_prefix(branch, \"refs/heads/\", &branch)\nbecomes meaningless. It was used by the die() to provide a nicer\nlooking error message, however, 'branch' now is never used after\nskip_prefix(). More below...\n\n>> +\n>> +       strbuf_release(&path);\n>> +       strbuf_release(&sb);\n>> +       return strbuf_detach(&gitdir, NULL);\n>>  done:\n>>         strbuf_release(&path);\n>>         strbuf_release(&sb);\n>>         strbuf_release(&gitdir);\n>> +\n>> +       return NULL;\n>>  }\n>\n> This would be cleaner and less redundant if you assign the existing\n> location to a variable and just fall through to the 'done' label:\n>\n>     char *existing = NULL;\n>     ...\n>         skip_prefix(branch, \"refs/heads/\", &branch);\n>         strbuf_strip_suffix(&gitdir, \".git\");\n>         existing = strbuf_detach(&gitdir, NULL);\n>     done:\n>         strbuf_release(&path);\n>         strbuf_release(&sb);\n>         strbuf_release(&gitdir);\n>         return existing;\n>\n> There's no worry that the \"existing\" path will be clobbered by\n> strbuf_release(&gitdir) since it's been detached already (and it's\n> safe to release the strbuf without affecting what has been detached\n> from it).\n>\n>> -void die_if_checked_out(const char *branch)\n>> +char *find_shared_symref(const char *symref, const char *target)\n>>  {\n>>         struct strbuf path = STRBUF_INIT;\n>>         DIR *dir;\n>>         struct dirent *d;\n>> +       char *existing;\n>>\n>> -       check_linked_checkout(branch, NULL);\n>> +       if ((existing = find_linked_symref(symref, target, NULL)))\n>> +               return existing;\n>>\n>>         strbuf_addf(&path, \"%s/worktrees\", get_git_common_dir());\n>>         dir = opendir(path.buf);\n>>         strbuf_release(&path);\n>>         if (!dir)\n>> -               return;\n>> +               return NULL;\n>>\n>>         while ((d = readdir(dir)) != NULL) {\n>>                 if (!strcmp(d->d_name, \".\") || !strcmp(d->d_name, \"..\"))\n>>                         continue;\n>> -               check_linked_checkout(branch, d->d_name);\n>> +               existing = find_linked_symref(symref, target, d->d_name);\n>> +               if (existing) {\n>> +                       closedir(dir);\n>> +                       return existing;\n>\n> For consistency with code nearby, this could have been handled by\n> adding a 'done' label above the closedir() below and jumping to it\n> from here, and then 'return existing'.\n>\n>> +               }\n>>         }\n>>         closedir(dir);\n>> +\n>> +       return NULL;\n>> +}\n>> +\n>> +void die_if_checked_out(const char *branch)\n>> +{\n>> +       char *existing;\n>> +\n>> +       existing = find_shared_symref(\"HEAD\", branch);\n>> +       if (existing)\n>> +               die(_(\"'%s' is already checked out at '%s'\"), branch, existing);\n\nUnlike the original die() in check_linked_checkout() which printed the\nbranch named shortened by skip_prefix(), this one still prints the\nlong-form branch name. An easy fix would be to move the skip_prefix()\ninvocation down to here to skip the \"refs/heads/\" prefix just before\ndie().\n\n>>  }\n"}]}