threads / patch / 40049

v6, 2 partsworktrees: add find_shared_symref

Subject: [PATCH v6 1/2] worktrees: add find_shared_symref

## tl;dr

6 messages between Aug 10, 2015 and Aug 11, 2015. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

David Turner· Aug 10, 2015, 17:52 UTC · lore

Add a new function, find_shared_symref, which contains the heart of die_if_checked_out, but works for any symref, not just HEAD. Refactor die_if_checked_out to use the same infrastructure as find_shared_symref.

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

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

This version addresses Eric Sunshine's comments on v5. It fixes an error message and cleans up the code.

---
 branch.c | 46 ++++++++++++++++++++++++++++++++++------------
 branch.h |  8 ++++++++
 2 files changed, 42 insertions(+), 12 deletions(-)
Show changes to 2 files +42 −12

branch.c, branch.h

diff --git a/branch.c b/branch.c
index c85be07..07c8b1e 100644
--- a/branch.c
+++ b/branch.c
@@ -311,21 +311,23 @@ void remove_branch_state(void)
 	unlink(git_path("SQUASH_MSG"));
 }
 
-static void check_linked_checkout(const char *branch, const char *id)
+static char *find_linked_symref(const char *symref, const char *branch,
+				const char *id)
 {
 	struct strbuf sb = STRBUF_INIT;
 	struct strbuf path = STRBUF_INIT;
 	struct strbuf gitdir = STRBUF_INIT;
+	char *existing = NULL;
 
 	/*
-	 * $GIT_COMMON_DIR/HEAD is practically outside
-	 * $GIT_DIR so resolve_ref_unsafe() won't work (it
-	 * uses git_path). Parse the ref ourselves.
+	 * $GIT_COMMON_DIR/$symref (e.g. HEAD) 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/") ||
@@ -347,33 +349,53 @@ static void check_linked_checkout(const char *branch, const char *id)
 		strbuf_rtrim(&gitdir);
 	} else
 		strbuf_addstr(&gitdir, get_git_common_dir());
-	skip_prefix(branch, "refs/heads/", &branch);
 	strbuf_strip_suffix(&gitdir, ".git");
-	die(_("'%s' is already checked out at '%s'"), branch, gitdir.buf);
+
+	existing = strbuf_detach(&gitdir, NULL);
 done:
 	strbuf_release(&path);
 	strbuf_release(&sb);
 	strbuf_release(&gitdir);
+
+	return existing;
 }
 
-void die_if_checked_out(const char *branch)
+char *find_shared_symref(const char *symref, const char *target)
 {
 	struct strbuf path = STRBUF_INIT;
 	DIR *dir;
 	struct dirent *d;
+	char *existing;
 
-	check_linked_checkout(branch, NULL);
+	if ((existing = find_linked_symref(symref, target, NULL)))
+		return existing;
 
 	strbuf_addf(&path, "%s/worktrees", get_git_common_dir());
 	dir = opendir(path.buf);
 	strbuf_release(&path);
 	if (!dir)
-		return;
+		return NULL;
 
 	while ((d = readdir(dir)) != NULL) {
 		if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, ".."))
 			continue;
-		check_linked_checkout(branch, d->d_name);
+		existing = find_linked_symref(symref, target, d->d_name);
+		if (existing)
+			goto done;
 	}
+done:
 	closedir(dir);
+
+	return existing;
+}
+
+void die_if_checked_out(const char *branch)
+{
+	char *existing;
+
+	existing = find_shared_symref("HEAD", branch);
+	if (existing) {
+		skip_prefix(branch, "refs/heads/", &branch);
+		die(_("'%s' is already checked out at '%s'"), branch, existing);
+	}
 }
diff --git a/branch.h b/branch.h
index 58aa45f..d3446ed 100644
--- a/branch.h
+++ b/branch.h
@@ -59,4 +59,12 @@ extern int read_branch_desc(struct strbuf *, const char *branch_name);
  */
 extern void die_if_checked_out(const char *branch);
 
+/*
+ * Check if a per-worktree symref points to a ref in the main worktree
+ * or any linked worktree, and return the path to the exising worktree
+ * if it is.  Returns NULL if there is no existing ref.  The caller is
+ * responsible for freeing the returned path.
+ */
+extern char *find_shared_symref(const char *symref, const char *target);
+
 #endif
-- 
2.0.4.315.gad8727a-twtrsrc
David Turner· Aug 10, 2015, 17:52 UTC · re: David Turner · lore

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

Before creating NOTES_MERGE_REF, check NOTES_MERGE_REF using find_shared_symref and die if we find one. This prevents simultaneous merges to the same notes branch from different worktrees.

Signed-off-by: David Turner <dturner@twopensource.com>
---
 builtin/notes.c                  |  6 ++++
 t/t3320-notes-merge-worktrees.sh | 72 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 78 insertions(+)
 create mode 100755 t/t3320-notes-merge-worktrees.sh
Show changes to 2 files +78 −0

builtin/notes.c, t/t3320-notes-merge-worktrees.sh

diff --git a/builtin/notes.c b/builtin/notes.c
index 63f95fc..0423480 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>]]"),
@@ -825,10 +826,15 @@ static int merge(int argc, const char **argv, const char *prefix)
 		update_ref(msg.buf, default_notes_ref(), result_sha1, NULL,
 			   0, UPDATE_REFS_DIE_ON_ERR);
 	else { /* Merge has unresolved conflicts */
+		char *existing;
 		/* Update .git/NOTES_MERGE_PARTIAL with partial merge result */
 		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 */
+		existing = find_shared_symref("NOTES_MERGE_REF", default_notes_ref());
+		if (existing)
+			die(_("A notes merge into %s is already in-progress at %s"),
+			    default_notes_ref(), existing);
 		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..a7beef2
--- /dev/null
+++ b/t/t3320-notes-merge-worktrees.sh
@@ -0,0 +1,72 @@
+#!/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 "A notes merge into refs/notes/y is already in-progress at" 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 "Automatic notes merge failed" out &&
+		grep -v "A notes merge into refs/notes/x is already in-progress in" out
+	) &&
+	echo "ref: refs/notes/x" > expect &&
+	test_cmp .git/worktrees/worktree2/NOTES_MERGE_REF expect
+'
+
+test_done
-- 
2.0.4.315.gad8727a-twtrsrc
Johan Herland· Aug 11, 2015, 13:56 UTC · re: David Turner · lore

Re: [PATCH v6 2/2] notes: handle multiple worktrees

On Mon, Aug 10, 2015 at 7:52 PM, David Turner <dturner@twopensource.com> wrote:
Show 5 quoted lines
> Before creating NOTES_MERGE_REF, check NOTES_MERGE_REF using
> find_shared_symref and die if we find one.  This prevents simultaneous
> merges to the same notes branch from different worktrees.
>
> Signed-off-by: David Turner <dturner@twopensource.com>
Still looks good to me, AFAICS. Feel free to add my Reviewed-by.
...Johan
-- 
Johan Herland, <johan@herland.net>
www.herland.net
Eric Sunshine· Aug 10, 2015, 22:30 UTC · re: David Turner · lore

Re: [PATCH v6 1/2] worktrees: add find_shared_symref

On Mon, Aug 10, 2015 at 1:52 PM, David Turner <dturner@twopensource.com> wrote:
> worktrees: add find_shared_symref
s/worktrees/branch/ perhaps?
Show 12 quoted lines
> Add a new function, find_shared_symref, which contains the heart of
> die_if_checked_out, but works for any symref, not just HEAD.  Refactor
> die_if_checked_out to use the same infrastructure as
> find_shared_symref.
>
> Soon, we will use find_shared_symref to protect notes merges in
> worktrees.
>
> Signed-off-by: David Turner <dturner@twopensource.com>
> ---
> This version addresses Eric Sunshine's comments on v5.  It fixes an error
> message and cleans up the code.

All issues identified in previous versions seem to have been addressed, and nothing else pops out at me. Thanks.

David Turner· Aug 10, 2015, 22:42 UTC · re: Eric Sunshine · lore

Re: [PATCH v6 1/2] worktrees: add find_shared_symref

On Mon, 2015-08-10 at 18:30 -0400, Eric Sunshine wrote:
> On Mon, Aug 10, 2015 at 1:52 PM, David Turner <dturner@twopensource.com> wrote:
> > worktrees: add find_shared_symref
> 
> s/worktrees/branch/ perhaps?
Do you mean "this is in branch.c, so should be labeled with branch"?  

Because this change is mostly about non-branch refs, so I think saying "branch" is confusing. That's why I labelled it "worktrees"; that's the broad topic that's being addressed.

Show 15 quoted lines
> > Add a new function, find_shared_symref, which contains the heart of
> > die_if_checked_out, but works for any symref, not just HEAD.  Refactor
> > die_if_checked_out to use the same infrastructure as
> > find_shared_symref.
> >
> > Soon, we will use find_shared_symref to protect notes merges in
> > worktrees.
> >
> > Signed-off-by: David Turner <dturner@twopensource.com>
> > ---
> > This version addresses Eric Sunshine's comments on v5.  It fixes an error
> > message and cleans up the code.
> 
> All issues identified in previous versions seem to have been
> addressed, and nothing else pops out at me. Thanks.
Eric Sunshine· Aug 10, 2015, 22:49 UTC · re: David Turner · lore

Re: [PATCH v6 1/2] worktrees: add find_shared_symref

On Mon, Aug 10, 2015 at 6:42 PM, David Turner <dturner@twopensource.com> wrote:
Show 11 quoted lines
> On Mon, 2015-08-10 at 18:30 -0400, Eric Sunshine wrote:
>> On Mon, Aug 10, 2015 at 1:52 PM, David Turner <dturner@twopensource.com> wrote:
>> > worktrees: add find_shared_symref
>>
>> s/worktrees/branch/ perhaps?
>
> Do you mean "this is in branch.c, so should be labeled with branch"?
>
> Because this change is mostly about non-branch refs, so I think saying
> "branch" is confusing.  That's why I labelled it "worktrees"; that's the
> broad topic that's being addressed.
Okay.

← back to recent threads