[PATCH v3 1/2] worktrees: add die_if_shared_symref

[PATCH v3 1/2] worktrees: add die_if_shared_symref

[David Turner]

Add a new function, die_if_shared_symref, which works like
die_if_checked_out, but for all references.  Refactor
die_if_checked_out to work in terms of die_if_shared_symref.

Soon, we will use die_if_shared_symref to protect notes merges in
worktrees.

Signed-off-by: David Turner <dturner@twopensource.com>
---

Oops, forgot I had split this into two patches.  This v3 includes the first patch.

---
 branch.c | 20 +++++++++++++-------
 branch.h |  7 +++++++
 2 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/branch.c b/branch.c
index c85be07..1f1b385 100644
--- a/branch.c
+++ b/branch.c
@@ -311,21 +311,22 @@ void remove_branch_state(void)
 	unlink(git_path("SQUASH_MSG"));
 }
 
-static void check_linked_checkout(const char *branch, const char *id)
+static void check_linked_checkout(const char *symref, const char *branch,
+				  const char *id)
 {
 	struct strbuf sb = STRBUF_INIT;
 	struct strbuf path = STRBUF_INIT;
 	struct strbuf gitdir = STRBUF_INIT;
 
 	/*
-	 * $GIT_COMMON_DIR/HEAD is practically outside
+	 * $GIT_COMMON_DIR/$symref is practically outside
 	 * $GIT_DIR so resolve_ref_unsafe() won't work (it
 	 * uses git_path). Parse the ref ourselves.
 	 */
 	if (id)
-		strbuf_addf(&path, "%s/worktrees/%s/HEAD", get_git_common_dir(), id);
+		strbuf_addf(&path, "%s/worktrees/%s/%s", get_git_common_dir(), id, symref);
 	else
-		strbuf_addf(&path, "%s/HEAD", get_git_common_dir());
+		strbuf_addf(&path, "%s/%s", get_git_common_dir(), symref);
 
 	if (!strbuf_readlink(&sb, path.buf, 0)) {
 		if (!starts_with(sb.buf, "refs/") ||
@@ -356,13 +357,13 @@ done:
 	strbuf_release(&gitdir);
 }
 
-void die_if_checked_out(const char *branch)
+void die_if_shared_symref(const char *symref, const char *branch)
 {
 	struct strbuf path = STRBUF_INIT;
 	DIR *dir;
 	struct dirent *d;
 
-	check_linked_checkout(branch, NULL);
+	check_linked_checkout(symref, branch, NULL);
 
 	strbuf_addf(&path, "%s/worktrees", get_git_common_dir());
 	dir = opendir(path.buf);
@@ -373,7 +374,12 @@ void die_if_checked_out(const char *branch)
 	while ((d = readdir(dir)) != NULL) {
 		if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, ".."))
 			continue;
-		check_linked_checkout(branch, d->d_name);
+		check_linked_checkout(symref, branch, d->d_name);
 	}
 	closedir(dir);
 }
+
+void die_if_checked_out(const char *branch)
+{
+	die_if_shared_symref("HEAD", branch);
+}
diff --git a/branch.h b/branch.h
index 58aa45f..b2fca30 100644
--- a/branch.h
+++ b/branch.h
@@ -59,4 +59,11 @@ extern int read_branch_desc(struct strbuf *, const char *branch_name);
  */
 extern void die_if_checked_out(const char *branch);
 
+/*
+ * Check if a symref points to a branch in the main worktree or any linked
+ * worktree and die (with a message describing its checkout location) if
+ * it is.
+ */
+extern void die_if_shared_symref(const char *symref, const char *branch);
+
 #endif
-- 
2.0.4.315.gad8727a-twtrsrc

[PATCH v3 2/2] notes: handle multiple worktrees

[David Turner]

Before creating NOTES_MERGE_REF, check NOTES_MERGE_REF using
die_if_shared_symref.  This prevents simultaneous merges to the same
notes branch from different worktrees.

Signed-off-by: David Turner <dturner@twopensource.com>
---
 builtin/notes.c                  |  2 ++
 t/t3320-notes-merge-worktrees.sh | 71 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 73 insertions(+)
 create mode 100755 t/t3320-notes-merge-worktrees.sh

diff --git a/builtin/notes.c b/builtin/notes.c
index 63f95fc..78a264e 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -19,6 +19,7 @@
 #include "string-list.h"
 #include "notes-merge.h"
 #include "notes-utils.h"
+#include "branch.h"
 
 static const char * const git_notes_usage[] = {
 	N_("git notes [--ref <notes-ref>] [list [<object>]]"),
@@ -829,6 +830,7 @@ static int merge(int argc, const char **argv, const char *prefix)
 		update_ref(msg.buf, "NOTES_MERGE_PARTIAL", result_sha1, NULL,
 			   0, UPDATE_REFS_DIE_ON_ERR);
 		/* Store ref-to-be-updated into .git/NOTES_MERGE_REF */
+		die_if_shared_symref("NOTES_MERGE_REF", default_notes_ref());
 		if (create_symref("NOTES_MERGE_REF", default_notes_ref(), NULL))
 			die("Failed to store link to current notes ref (%s)",
 			    default_notes_ref());
diff --git a/t/t3320-notes-merge-worktrees.sh b/t/t3320-notes-merge-worktrees.sh
new file mode 100755
index 0000000..b102c23
--- /dev/null
+++ b/t/t3320-notes-merge-worktrees.sh
@@ -0,0 +1,71 @@
+#!/bin/sh
+#
+# Copyright (c) 2015 Twitter, Inc
+#
+
+test_description='Test merging of notes trees in multiple worktrees'
+
+. ./test-lib.sh
+
+test_expect_success 'setup commit' '
+	test_commit tantrum
+'
+
+commit_tantrum=$(git rev-parse tantrum^{commit})
+
+test_expect_success 'setup notes ref (x)' '
+	git config core.notesRef refs/notes/x &&
+	git notes add -m "x notes on tantrum" tantrum
+'
+
+test_expect_success 'setup local branch (y)' '
+	git update-ref refs/notes/y refs/notes/x &&
+	git config core.notesRef refs/notes/y &&
+	git notes remove tantrum
+'
+
+test_expect_success 'setup remote branch (z)' '
+	git update-ref refs/notes/z refs/notes/x &&
+	git config core.notesRef refs/notes/z &&
+	git notes add -f -m "conflicting notes on tantrum" tantrum
+'
+
+test_expect_success 'modify notes ref ourselves (x)' '
+	git config core.notesRef refs/notes/x &&
+	git notes add -f -m "more conflicting notes on tantrum" tantrum
+'
+
+test_expect_success 'create some new worktrees' '
+	git worktree add -b newbranch worktree master &&
+	git worktree add -b newbranch2 worktree2 master
+'
+
+test_expect_success 'merge z into y fails and sets NOTES_MERGE_REF' '
+	git config core.notesRef refs/notes/y &&
+	test_must_fail git notes merge z &&
+	echo "ref: refs/notes/y" > expect &&
+	test_cmp .git/NOTES_MERGE_REF expect
+'
+
+test_expect_success 'merge z into y while mid-merge in another workdir fails' '
+	(
+		cd worktree &&
+		git config core.notesRef refs/notes/y &&
+		test_must_fail git notes merge z 2>err &&
+		grep "is already checked out" err
+	) &&
+	test_path_is_missing .git/worktrees/worktree/NOTES_MERGE_REF
+'
+
+test_expect_success 'merge z into x while mid-merge on y succeeds' '
+	(
+		cd worktree2 &&
+		git config core.notesRef refs/notes/x &&
+		test_must_fail git notes merge z 2>&1 >out &&
+		grep -v "is already checked out" out
+	) &&
+	echo "ref: refs/notes/x" > expect &&
+	test_cmp .git/worktrees/worktree2/NOTES_MERGE_REF expect
+'
+
+test_done
-- 
2.0.4.315.gad8727a-twtrsrc

Re: [PATCH v3 1/2] worktrees: add die_if_shared_symref

[Eric Sunshine]

On Fri, Jul 31, 2015 at 3:01 PM, David Turner <dturner@twopensource.com> wrote:
> Add a new function, die_if_shared_symref, which works like
> die_if_checked_out, but for all references.  Refactor
> die_if_checked_out to work in terms of die_if_shared_symref.
>
> Soon, we will use die_if_shared_symref to protect notes merges in
> worktrees.

I wonder if the diagnostic:

    'blorp' is already checked out at '/path/name/'

emitted by check_linked_checkout() needs to be adjusted for this
change. It still makes sense for die_if_checked_out(), but sounds odd
for die_if_shared_symref().

> Signed-off-by: David Turner <dturner@twopensource.com>
> ---
> diff --git a/branch.h b/branch.h
> index 58aa45f..b2fca30 100644
> --- a/branch.h
> +++ b/branch.h
> @@ -59,4 +59,11 @@ extern int read_branch_desc(struct strbuf *, const char *branch_name);
>   */
>  extern void die_if_checked_out(const char *branch);
>
> +/*
> + * Check if a symref points to a branch in the main worktree or any linked
> + * worktree and die (with a message describing its checkout location) if
> + * it is.

Ditto here. This is still talking about "branch" and "checkout",
whereas it's actually checking if a symref in some worktree already
references the given referent.

> + */
> +extern void die_if_shared_symref(const char *symref, const char *branch);

s/branch/referent/ or something?

Re: [PATCH v3 1/2] worktrees: add die_if_shared_symref

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Fri, Jul 31, 2015 at 3:01 PM, David Turner <dturner@twopensource.com> wrote:
>> Add a new function, die_if_shared_symref, which works like
>> die_if_checked_out, but for all references.  Refactor
>> die_if_checked_out to work in terms of die_if_shared_symref.
>>
>> Soon, we will use die_if_shared_symref to protect notes merges in
>> worktrees.
>
> I wonder if the diagnostic:
>
>     'blorp' is already checked out at '/path/name/'
>
> emitted by check_linked_checkout() needs to be adjusted for this
> change. It still makes sense for die_if_checked_out(), but sounds odd
> for die_if_shared_symref().

True.  Lift the dying one callstack up, and make the lower level
helper check_shared_symref() or something that returns NULL (ok) or
that '/path/name' upon an error?

Also I suspect that this comment will become hard to grok after the
commit is actually made:

>  	/*
> -	 * $GIT_COMMON_DIR/HEAD is practically outside
> +	 * $GIT_COMMON_DIR/$symref is practically outside
>  	 * $GIT_DIR so resolve_ref_unsafe() won't work (it
>  	 * uses git_path). Parse the ref ourselves.
>  	 */

A reviewer who is viewing both the pre- and post- text of the patch
can see it used to say HEAD and now it is extended to $symref, but
it would help to have "(e.g. HEAD)" after "...DIR/$symref", I think.

Re: [PATCH v3 1/2] worktrees: add die_if_shared_symref

[David Turner]

On Fri, 2015-07-31 at 15:35 -0400, Eric Sunshine wrote:
> On Fri, Jul 31, 2015 at 3:01 PM, David Turner <dturner@twopensource.com> wrote:
> > Add a new function, die_if_shared_symref, which works like
> > die_if_checked_out, but for all references.  Refactor
> > die_if_checked_out to work in terms of die_if_shared_symref.
> >
> > Soon, we will use die_if_shared_symref to protect notes merges in
> > worktrees.
> 
> I wonder if the diagnostic:
> 
>     'blorp' is already checked out at '/path/name/'
> 
> emitted by check_linked_checkout() needs to be adjusted for this
> change. It still makes sense for die_if_checked_out(), but sounds odd
> for die_if_shared_symref().

How about:

'refs/notes/y' is already referenced from 'NOTES_MERGE_REF' in
'/home/dturner/git/t/trash directory.t3320-notes-merge-worktrees/'

Does that make sense?

It's not the only place in the error messages that mentions
NOTES_MERGE_REF, so I think we expect users to understand
NOTES_MERGE_REF.  The alternative is to move the error handling to an
even higher level so we can give a notes-specific message.  I don't
think that's necessary, but I'll do it if others do.

<snip; will fix>
> > + */
> > +extern void die_if_shared_symref(const char *symref, const char *branch);
> 
> s/branch/referent/ or something?

I went with "target" (by analogy to the target of a symlink).  

Re: [PATCH v3 1/2] worktrees: add die_if_shared_symref

[Eric Sunshine]

On Fri, Jul 31, 2015 at 5:15 PM, David Turner <dturner@twopensource.com> wrote:
> On Fri, 2015-07-31 at 15:35 -0400, Eric Sunshine wrote:
>> On Fri, Jul 31, 2015 at 3:01 PM, David Turner <dturner@twopensource.com> wrote:
>> > Add a new function, die_if_shared_symref, which works like
>> > die_if_checked_out, but for all references.  Refactor
>> > die_if_checked_out to work in terms of die_if_shared_symref.
>> >
>> > Soon, we will use die_if_shared_symref to protect notes merges in
>> > worktrees.
>>
>> I wonder if the diagnostic:
>>
>>     'blorp' is already checked out at '/path/name/'
>>
>> emitted by check_linked_checkout() needs to be adjusted for this
>> change. It still makes sense for die_if_checked_out(), but sounds odd
>> for die_if_shared_symref().
>
> How about:
>
> 'refs/notes/y' is already referenced from 'NOTES_MERGE_REF' in
> '/home/dturner/git/t/trash directory.t3320-notes-merge-worktrees/'
>
> Does that make sense?

That might be the best we can do for the generic case of
die_if_shared_symref(), but I wonder how easily the typical user would
understand it when trying to checkout a branch already checked out
elsewhere. One concern is that that message almost comes across as an
internal Git error (thus inscrutable), whereas:

    % git checkout blorp
    'blorp' is already checked out at '/some/path/'

seems (to me) pretty clearly a user error, thus more easily understood.

> It's not the only place in the error messages that mentions
> NOTES_MERGE_REF, so I think we expect users to understand
> NOTES_MERGE_REF.  The alternative is to move the error handling to an
> even higher level so we can give a notes-specific message.  I don't
> think that's necessary, but I'll do it if others do.

Given such precedence, the generic error message might indeed be fine
for the notes merge case, however, in general, you'd probably get
better and more understandable error messages by tailoring them at the
call sites. (That's a positive vote from me for for lifting error
handling to a higher level.)

Re: [PATCH v3 1/2] worktrees: add die_if_shared_symref

[David Turner]

On Fri, 2015-07-31 at 17:36 -0400, Eric Sunshine wrote:
> On Fri, Jul 31, 2015 at 5:15 PM, David Turner <dturner@twopensource.com> wrote:
> > On Fri, 2015-07-31 at 15:35 -0400, Eric Sunshine wrote:
> >> On Fri, Jul 31, 2015 at 3:01 PM, David Turner <dturner@twopensource.com> wrote:
> >> > Add a new function, die_if_shared_symref, which works like
> >> > die_if_checked_out, but for all references.  Refactor
> >> > die_if_checked_out to work in terms of die_if_shared_symref.
> >> >
> >> > Soon, we will use die_if_shared_symref to protect notes merges in
> >> > worktrees.
> >>
> >> I wonder if the diagnostic:
> >>
> >>     'blorp' is already checked out at '/path/name/'
> >>
> >> emitted by check_linked_checkout() needs to be adjusted for this
> >> change. It still makes sense for die_if_checked_out(), but sounds odd
> >> for die_if_shared_symref().
> >
> > How about:
> >
> > 'refs/notes/y' is already referenced from 'NOTES_MERGE_REF' in
> > '/home/dturner/git/t/trash directory.t3320-notes-merge-worktrees/'
> >
> > Does that make sense?
> 
> That might be the best we can do for the generic case of
> die_if_shared_symref(), but I wonder how easily the typical user would
> understand it when trying to checkout a branch already checked out
> elsewhere. One concern is that that message almost comes across as an
> internal Git error (thus inscrutable), whereas:
> 
>     % git checkout blorp
>     'blorp' is already checked out at '/some/path/'
> 
> seems (to me) pretty clearly a user error, thus more easily understood.

Sorry, I meant that for the non-HEAD case.  For the
die_if_already_checked_out case, the original error message would
remain.

But I'll just go ahead and do it the nice way.