[PATCH v3 0/6] pseudorefs

[PATCH v3 0/6] pseudorefs

[David Turner]

This version fixes documentation issues found by Eric Sunshine.

It also adds a new patch so as not to create static functions that
aren't immediately used; Eric also noticed that issue.

I refactored the functions to classify a ref into a single public
ref_type function.  This makes it easy for backends that want to treat
all non-normal refs the same; in previous patch versions, backends
would have had to say is_pseudoref || is_per_worktree_ref, and that
would have caused the strcmp in is_per_worktree_ref to be called
twice.  Now they can just say ref_type(ref) != REF_TYPE_NORMAL.

I also fixed another issues that I noticed myself: I removed a stray
debugging "&& 0" condition.

[PATCH v3 1/6] refs: Introduce pseudoref and per-worktree ref concepts

[David Turner]

Add glossary entries for both concepts.

Pseudorefs and per-worktree refs do not yet have special handling,
because the files refs backend already handles them correctly.  Later,
we will make the LMDB backend call out to the files backend to handle
per-worktree refs.

Signed-off-by: David Turner <dturner@twopensource.com>
---
 Documentation/glossary-content.txt | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt
index ab18f4b..ff14079 100644
--- a/Documentation/glossary-content.txt
+++ b/Documentation/glossary-content.txt
@@ -411,6 +411,27 @@ exclude;;
 	core Git. Porcelains expose more of a <<def_SCM,SCM>>
 	interface than the <<def_plumbing,plumbing>>.
 
+[[def_per_worktree_ref]]per-worktree ref::
+	Refs that are per-<<def_worktree,worktree>>, rather than
+	global.  This is presently only <<def_HEAD,HEAD>>, but might
+	later include other unusual refs.
+
+[[def_pseudoref]]pseudoref::
+	Pseudorefs are a class of files under `$GIT_DIR` which behave
+	like refs for the purposes of rev-parse, but which are treated
+	specially by git.  Psuedorefs both have names that are all-caps,
+	and always start with a line consisting of a
+	<<def_sha1,SHA-1>> followed by whitespace.  So, HEAD is not a
+	pseudoref, because it is sometimes a symbolic ref.  They might
+	optionally contain some additional data.  `MERGE_HEAD` and
+	`CHERRY_PICK_HEAD` are examples.  Unlike
+	<<def_per_worktree_ref,per-worktree refs>>, these files cannot
+	be symbolic refs, and never have reflogs.  They also cannot be
+	updated through the normal ref update machinery.  Instead,
+	they are updated by directly writing to the files.  However,
+	they can be read as if they were refs, so `git rev-parse
+	MERGE_HEAD` will work.
+
 [[def_pull]]pull::
 	Pulling a <<def_branch,branch>> means to <<def_fetch,fetch>> it and
 	<<def_merge,merge>> it.  See also linkgit:git-pull[1].
-- 
2.0.4.315.gad8727a-twtrsrc

[PATCH v3 2/6] notes: replace pseudorefs with real refs

[David Turner]

All-caps files like NOTES_MERGE_REF are pseudorefs, and thus are
per-worktree.  We don't want multiple notes merges happening at once
(in different worktrees), so we want to make these refs true refs.

So, we lowercase NOTES_MERGE_REF and friends.  That way, backends
that distinguish between pseudorefs and real refs can correctly
handle notes merges.

This will also enable us to prevent pseudorefs from being updated by
the ref update machinery e.g. git update-ref.

Signed-off-by: David Turner <dturner@twopensource.com>
---
 Documentation/git-notes.txt           | 12 ++---
 builtin/notes.c                       | 38 ++++++++--------
 notes-merge.c                         | 10 ++--
 notes-merge.h                         |  8 ++--
 t/t3310-notes-merge-manual-resolve.sh | 86 +++++++++++++++++------------------
 t/t3311-notes-merge-fanout.sh         |  6 +--
 6 files changed, 80 insertions(+), 80 deletions(-)

diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
index 851518d..82fa3fd 100644
--- a/Documentation/git-notes.txt
+++ b/Documentation/git-notes.txt
@@ -103,7 +103,7 @@ merge::
 If conflicts arise and a strategy for automatically resolving
 conflicting notes (see the -s/--strategy option) is not given,
 the "manual" resolver is used. This resolver checks out the
-conflicting notes in a special worktree (`.git/NOTES_MERGE_WORKTREE`),
+conflicting notes in a special worktree (`.git/notes_merge_worktree`),
 and instructs the user to manually resolve the conflicts there.
 When done, the user can either finalize the merge with
 'git notes merge --commit', or abort the merge with
@@ -189,11 +189,11 @@ OPTIONS
 --commit::
 	Finalize an in-progress 'git notes merge'. Use this option
 	when you have resolved the conflicts that 'git notes merge'
-	stored in .git/NOTES_MERGE_WORKTREE. This amends the partial
+	stored in .git/notes_merge_worktree. This amends the partial
 	merge commit created by 'git notes merge' (stored in
-	.git/NOTES_MERGE_PARTIAL) by adding the notes in
-	.git/NOTES_MERGE_WORKTREE. The notes ref stored in the
-	.git/NOTES_MERGE_REF symref is updated to the resulting commit.
+	.git/notes_merge_partial) by adding the notes in
+	.git/notes_merge_worktree. The notes ref stored in the
+	.git/notes_merge_ref symref is updated to the resulting commit.
 
 --abort::
 	Abort/reset a in-progress 'git notes merge', i.e. a notes merge
@@ -241,7 +241,7 @@ NOTES MERGE STRATEGIES
 
 The default notes merge strategy is "manual", which checks out
 conflicting notes in a special work tree for resolving notes conflicts
-(`.git/NOTES_MERGE_WORKTREE`), and instructs the user to resolve the
+(`.git/notes_merge_worktree`), and instructs the user to resolve the
 conflicts in that work tree.
 When done, the user can either finalize the merge with
 'git notes merge --commit', or abort the merge with
diff --git a/builtin/notes.c b/builtin/notes.c
index 63f95fc..e0b8a02 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -670,14 +670,14 @@ static int merge_abort(struct notes_merge_options *o)
 	int ret = 0;
 
 	/*
-	 * Remove .git/NOTES_MERGE_PARTIAL and .git/NOTES_MERGE_REF, and call
-	 * notes_merge_abort() to remove .git/NOTES_MERGE_WORKTREE.
+	 * Remove .git/notes_merge_partial and .git/notes_merge_ref, and call
+	 * notes_merge_abort() to remove .git/notes_merge_worktree.
 	 */
 
-	if (delete_ref("NOTES_MERGE_PARTIAL", NULL, 0))
-		ret += error("Failed to delete ref NOTES_MERGE_PARTIAL");
-	if (delete_ref("NOTES_MERGE_REF", NULL, REF_NODEREF))
-		ret += error("Failed to delete ref NOTES_MERGE_REF");
+	if (delete_ref("notes_merge_partial", NULL, 0))
+		ret += error("Failed to delete ref notes_merge_partial");
+	if (delete_ref("notes_merge_ref", NULL, REF_NODEREF))
+		ret += error("Failed to delete ref notes_merge_ref");
 	if (notes_merge_abort(o))
 		ret += error("Failed to remove 'git notes merge' worktree");
 	return ret;
@@ -694,16 +694,16 @@ static int merge_commit(struct notes_merge_options *o)
 	int ret;
 
 	/*
-	 * Read partial merge result from .git/NOTES_MERGE_PARTIAL,
-	 * and target notes ref from .git/NOTES_MERGE_REF.
+	 * Read partial merge result from .git/notes_merge_partial,
+	 * and target notes ref from .git/notes_merge_ref.
 	 */
 
-	if (get_sha1("NOTES_MERGE_PARTIAL", sha1))
-		die("Failed to read ref NOTES_MERGE_PARTIAL");
+	if (get_sha1("notes_merge_partial", sha1))
+		die("Failed to read ref notes_merge_partial");
 	else if (!(partial = lookup_commit_reference(sha1)))
-		die("Could not find commit from NOTES_MERGE_PARTIAL.");
+		die("Could not find commit from notes_merge_partial.");
 	else if (parse_commit(partial))
-		die("Could not parse commit from NOTES_MERGE_PARTIAL.");
+		die("Could not parse commit from notes_merge_partial.");
 
 	if (partial->parents)
 		hashcpy(parent_sha1, partial->parents->item->object.sha1);
@@ -711,12 +711,12 @@ static int merge_commit(struct notes_merge_options *o)
 		hashclr(parent_sha1);
 
 	t = xcalloc(1, sizeof(struct notes_tree));
-	init_notes(t, "NOTES_MERGE_PARTIAL", combine_notes_overwrite, 0);
+	init_notes(t, "notes_merge_partial", combine_notes_overwrite, 0);
 
 	o->local_ref = local_ref_to_free =
-		resolve_refdup("NOTES_MERGE_REF", 0, sha1, NULL);
+		resolve_refdup("notes_merge_ref", 0, sha1, NULL);
 	if (!o->local_ref)
-		die("Failed to resolve NOTES_MERGE_REF");
+		die("Failed to resolve notes_merge_ref");
 
 	if (notes_merge_commit(o, t, partial, sha1))
 		die("Failed to finalize notes merge");
@@ -825,11 +825,11 @@ 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 */
-		/* Update .git/NOTES_MERGE_PARTIAL with partial merge result */
-		update_ref(msg.buf, "NOTES_MERGE_PARTIAL", result_sha1, NULL,
+		/* 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 */
-		if (create_symref("NOTES_MERGE_REF", default_notes_ref(), NULL))
+		/* Store ref-to-be-updated into .git/notes_merge_ref */
+		if (create_symref("notes_merge_ref", default_notes_ref(), NULL))
 			die("Failed to store link to current notes ref (%s)",
 			    default_notes_ref());
 		printf("Automatic notes merge failed. Fix conflicts in %s and "
diff --git a/notes-merge.c b/notes-merge.c
index 0b2b82c..58a8637 100644
--- a/notes-merge.c
+++ b/notes-merge.c
@@ -274,10 +274,10 @@ static void check_notes_merge_worktree(struct notes_merge_options *o)
 				    "'git notes merge --commit' or 'git notes "
 				    "merge --abort' to commit/abort the "
 				    "previous merge before you start a new "
-				    "notes merge.", git_path("NOTES_MERGE_*"));
+				    "notes merge.", git_path("notes_merge_*"));
 			else
 				die("You have not concluded your notes merge "
-				    "(%s exists).", git_path("NOTES_MERGE_*"));
+				    "(%s exists).", git_path("notes_merge_*"));
 		}
 
 		if (safe_create_leading_directories_const(git_path(
@@ -663,7 +663,7 @@ int notes_merge_commit(struct notes_merge_options *o,
 		       unsigned char *result_sha1)
 {
 	/*
-	 * Iterate through files in .git/NOTES_MERGE_WORKTREE and add all
+	 * Iterate through files in .git/notes_merge_workree and add all
 	 * found notes to 'partial_tree'. Write the updated notes tree to
 	 * the DB, and commit the resulting tree object while reusing the
 	 * commit message and parents from 'partial_commit'.
@@ -734,8 +734,8 @@ int notes_merge_commit(struct notes_merge_options *o,
 int notes_merge_abort(struct notes_merge_options *o)
 {
 	/*
-	 * Remove all files within .git/NOTES_MERGE_WORKTREE. We do not remove
-	 * the .git/NOTES_MERGE_WORKTREE directory itself, since it might be
+	 * Remove all files within .git/notes_merge_workree. We do not remove
+	 * the .git/notes_merge_workree directory itself, since it might be
 	 * the current working directory of the user.
 	 */
 	struct strbuf buf = STRBUF_INIT;
diff --git a/notes-merge.h b/notes-merge.h
index 1d01f6a..18705d6 100644
--- a/notes-merge.h
+++ b/notes-merge.h
@@ -1,7 +1,7 @@
 #ifndef NOTES_MERGE_H
 #define NOTES_MERGE_H
 
-#define NOTES_MERGE_WORKTREE "NOTES_MERGE_WORKTREE"
+#define NOTES_MERGE_WORKTREE "notes_merge_worktree"
 
 enum notes_merge_verbosity {
 	NOTES_MERGE_VERBOSITY_DEFAULT = 2,
@@ -46,7 +46,7 @@ void init_notes_merge_options(struct notes_merge_options *o);
  *    are stored in 'local_tree', and the SHA1 or the resulting commit
  *    (to be amended when the conflicts have been resolved) is written into
  *    'result_sha1'. The unmerged entries are written into the
- *    .git/NOTES_MERGE_WORKTREE directory with conflict markers.
+ *    .git/notes_merge_worktree directory with conflict markers.
  *    -1 is returned.
  *
  * Both o->local_ref and o->remote_ref must be given (non-NULL), but either ref
@@ -65,7 +65,7 @@ int notes_merge(struct notes_merge_options *o,
  * the given 'partial_commit', the partial result commit created by a previous
  * call to notes_merge().
  *
- * This function will add the (now resolved) notes in .git/NOTES_MERGE_WORKTREE
+ * This function will add the (now resolved) notes in .git/notes_merge_worktree
  * to 'partial_tree', and create a final notes merge commit, the SHA1 of which
  * will be stored in 'result_sha1'.
  */
@@ -77,7 +77,7 @@ int notes_merge_commit(struct notes_merge_options *o,
 /*
  * Abort conflict resolution from an earlier notes_merge()
  *
- * Removes the notes merge worktree in .git/NOTES_MERGE_WORKTREE.
+ * Removes the notes merge worktree in .git/notes_merge_worktree.
  */
 int notes_merge_abort(struct notes_merge_options *o);
 
diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh
index 195bb97..dc9ecd5 100755
--- a/t/t3310-notes-merge-manual-resolve.sh
+++ b/t/t3310-notes-merge-manual-resolve.sh
@@ -178,12 +178,12 @@ test_expect_success 'merge z into m (== y) with default ("manual") resolver => C
 	git config core.notesRef refs/notes/m &&
 	test_must_fail git notes merge z >output &&
 	# Output should point to where to resolve conflicts
-	grep -q "\\.git/NOTES_MERGE_WORKTREE" output &&
+	grep -q "\\.git/notes_merge_worktree" output &&
 	# Inspect merge conflicts
-	ls .git/NOTES_MERGE_WORKTREE >output_conflicts &&
+	ls .git/notes_merge_worktree >output_conflicts &&
 	test_cmp expect_conflicts output_conflicts &&
 	( for f in $(cat expect_conflicts); do
-		test_cmp "expect_conflict_$f" ".git/NOTES_MERGE_WORKTREE/$f" ||
+		test_cmp "expect_conflict_$f" ".git/notes_merge_worktree/$f" ||
 		exit 1
 	done ) &&
 	# Verify that current notes tree (pre-merge) has not changed (m == y)
@@ -222,10 +222,10 @@ test_expect_success 'change notes in z' '
 '
 
 test_expect_success 'cannot do merge w/conflicts when previous merge is unfinished' '
-	test -d .git/NOTES_MERGE_WORKTREE &&
+	test -d .git/notes_merge_worktree &&
 	test_must_fail git notes merge z >output 2>&1 &&
 	# Output should indicate what is wrong
-	grep -q "\\.git/NOTES_MERGE_\\* exists" output
+	grep -q "\\.git/notes_merge_\\* exists" output
 '
 
 # Setup non-conflicting merge between x and new notes ref w
@@ -282,7 +282,7 @@ w notes on 1st commit
 EOF
 
 test_expect_success 'can do merge without conflicts even if previous merge is unfinished (x => w)' '
-	test -d .git/NOTES_MERGE_WORKTREE &&
+	test -d .git/notes_merge_worktree &&
 	git notes merge x &&
 	verify_notes w &&
 	# Verify that other notes refs has not changed (x and y)
@@ -316,15 +316,15 @@ EOF
 
 test_expect_success 'finalize conflicting merge (z => m)' '
 	# Resolve conflicts and finalize merge
-	cat >.git/NOTES_MERGE_WORKTREE/$commit_sha1 <<EOF &&
+	cat >.git/notes_merge_worktree/$commit_sha1 <<EOF &&
 y and z notes on 1st commit
 EOF
-	cat >.git/NOTES_MERGE_WORKTREE/$commit_sha4 <<EOF &&
+	cat >.git/notes_merge_worktree/$commit_sha4 <<EOF &&
 y and z notes on 4th commit
 EOF
 	git notes merge --commit &&
-	# No .git/NOTES_MERGE_* files left
-	test_might_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&
+	# No .git/notes_merge_* files left
+	test_might_fail ls .git/notes_merge_* >output 2>/dev/null &&
 	test_cmp /dev/null output &&
 	# Merge commit has pre-merge y and pre-merge z as parents
 	test "$(git rev-parse refs/notes/m^1)" = "$(cat pre_merge_y)" &&
@@ -369,12 +369,12 @@ test_expect_success 'redo merge of z into m (== y) with default ("manual") resol
 	git config core.notesRef refs/notes/m &&
 	test_must_fail git notes merge z >output &&
 	# Output should point to where to resolve conflicts
-	grep -q "\\.git/NOTES_MERGE_WORKTREE" output &&
+	grep -q "\\.git/notes_merge_worktree" output &&
 	# Inspect merge conflicts
-	ls .git/NOTES_MERGE_WORKTREE >output_conflicts &&
+	ls .git/notes_merge_worktree >output_conflicts &&
 	test_cmp expect_conflicts output_conflicts &&
 	( for f in $(cat expect_conflicts); do
-		test_cmp "expect_conflict_$f" ".git/NOTES_MERGE_WORKTREE/$f" ||
+		test_cmp "expect_conflict_$f" ".git/notes_merge_worktree/$f" ||
 		exit 1
 	done ) &&
 	# Verify that current notes tree (pre-merge) has not changed (m == y)
@@ -385,8 +385,8 @@ test_expect_success 'redo merge of z into m (== y) with default ("manual") resol
 
 test_expect_success 'abort notes merge' '
 	git notes merge --abort &&
-	# No .git/NOTES_MERGE_* files left
-	test_might_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&
+	# No .git/notes_merge_* files left
+	test_might_fail ls .git/notes_merge_* >output 2>/dev/null &&
 	test_cmp /dev/null output &&
 	# m has not moved (still == y)
 	test "$(git rev-parse refs/notes/m)" = "$(cat pre_merge_y)" &&
@@ -403,12 +403,12 @@ git rev-parse refs/notes/z > pre_merge_z
 test_expect_success 'redo merge of z into m (== y) with default ("manual") resolver => Conflicting 3-way merge' '
 	test_must_fail git notes merge z >output &&
 	# Output should point to where to resolve conflicts
-	grep -q "\\.git/NOTES_MERGE_WORKTREE" output &&
+	grep -q "\\.git/notes_merge_worktree" output &&
 	# Inspect merge conflicts
-	ls .git/NOTES_MERGE_WORKTREE >output_conflicts &&
+	ls .git/notes_merge_worktree >output_conflicts &&
 	test_cmp expect_conflicts output_conflicts &&
 	( for f in $(cat expect_conflicts); do
-		test_cmp "expect_conflict_$f" ".git/NOTES_MERGE_WORKTREE/$f" ||
+		test_cmp "expect_conflict_$f" ".git/notes_merge_worktree/$f" ||
 		exit 1
 	done ) &&
 	# Verify that current notes tree (pre-merge) has not changed (m == y)
@@ -441,19 +441,19 @@ EOF
 
 test_expect_success 'add + remove notes in finalized merge (z => m)' '
 	# Resolve one conflict
-	cat >.git/NOTES_MERGE_WORKTREE/$commit_sha1 <<EOF &&
+	cat >.git/notes_merge_worktree/$commit_sha1 <<EOF &&
 y and z notes on 1st commit
 EOF
 	# Remove another conflict
-	rm .git/NOTES_MERGE_WORKTREE/$commit_sha4 &&
+	rm .git/notes_merge_worktree/$commit_sha4 &&
 	# Remove a D/F conflict
-	rm .git/NOTES_MERGE_WORKTREE/$commit_sha3 &&
+	rm .git/notes_merge_worktree/$commit_sha3 &&
 	# Add a new note
-	echo "new note on 5th commit" > .git/NOTES_MERGE_WORKTREE/$commit_sha5 &&
+	echo "new note on 5th commit" > .git/notes_merge_worktree/$commit_sha5 &&
 	# Finalize merge
 	git notes merge --commit &&
-	# No .git/NOTES_MERGE_* files left
-	test_might_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&
+	# No .git/notes_merge_* files left
+	test_might_fail ls .git/notes_merge_* >output 2>/dev/null &&
 	test_cmp /dev/null output &&
 	# Merge commit has pre-merge y and pre-merge z as parents
 	test "$(git rev-parse refs/notes/m^1)" = "$(cat pre_merge_y)" &&
@@ -484,12 +484,12 @@ test_expect_success 'redo merge of z into m (== y) with default ("manual") resol
 	git update-ref refs/notes/m refs/notes/y &&
 	test_must_fail git notes merge z >output &&
 	# Output should point to where to resolve conflicts
-	grep -q "\\.git/NOTES_MERGE_WORKTREE" output &&
+	grep -q "\\.git/notes_merge_worktree" output &&
 	# Inspect merge conflicts
-	ls .git/NOTES_MERGE_WORKTREE >output_conflicts &&
+	ls .git/notes_merge_worktree >output_conflicts &&
 	test_cmp expect_conflicts output_conflicts &&
 	( for f in $(cat expect_conflicts); do
-		test_cmp "expect_conflict_$f" ".git/NOTES_MERGE_WORKTREE/$f" ||
+		test_cmp "expect_conflict_$f" ".git/notes_merge_worktree/$f" ||
 		exit 1
 	done ) &&
 	# Verify that current notes tree (pre-merge) has not changed (m == y)
@@ -507,31 +507,31 @@ test_expect_success 'reset notes ref m to somewhere else (w)' '
 	test "$(git rev-parse refs/notes/m)" = "$(git rev-parse refs/notes/w)"
 '
 
-test_expect_success 'fail to finalize conflicting merge if underlying ref has moved in the meantime (m != NOTES_MERGE_PARTIAL^1)' '
+test_expect_success 'fail to finalize conflicting merge if underlying ref has moved in the meantime (m != notes_merge_partial^1)' '
 	# Resolve conflicts
-	cat >.git/NOTES_MERGE_WORKTREE/$commit_sha1 <<EOF &&
+	cat >.git/notes_merge_worktree/$commit_sha1 <<EOF &&
 y and z notes on 1st commit
 EOF
-	cat >.git/NOTES_MERGE_WORKTREE/$commit_sha4 <<EOF &&
+	cat >.git/notes_merge_worktree/$commit_sha4 <<EOF &&
 y and z notes on 4th commit
 EOF
 	# Fail to finalize merge
 	test_must_fail git notes merge --commit >output 2>&1 &&
-	# .git/NOTES_MERGE_* must remain
-	test -f .git/NOTES_MERGE_PARTIAL &&
-	test -f .git/NOTES_MERGE_REF &&
-	test -f .git/NOTES_MERGE_WORKTREE/$commit_sha1 &&
-	test -f .git/NOTES_MERGE_WORKTREE/$commit_sha2 &&
-	test -f .git/NOTES_MERGE_WORKTREE/$commit_sha3 &&
-	test -f .git/NOTES_MERGE_WORKTREE/$commit_sha4 &&
+	# .git/notes_merge_* must remain
+	git rev-parse --verify notes_merge_partial &&
+	git rev-parse --verify notes_merge_ref &&
+	test -f .git/notes_merge_worktree/$commit_sha1 &&
+	test -f .git/notes_merge_worktree/$commit_sha2 &&
+	test -f .git/notes_merge_worktree/$commit_sha3 &&
+	test -f .git/notes_merge_worktree/$commit_sha4 &&
 	# Refs are unchanged
 	test "$(git rev-parse refs/notes/m)" = "$(git rev-parse refs/notes/w)" &&
-	test "$(git rev-parse refs/notes/y)" = "$(git rev-parse NOTES_MERGE_PARTIAL^1)" &&
-	test "$(git rev-parse refs/notes/m)" != "$(git rev-parse NOTES_MERGE_PARTIAL^1)" &&
+	test "$(git rev-parse refs/notes/y)" = "$(git rev-parse notes_merge_partial^1)" &&
+	test "$(git rev-parse refs/notes/m)" != "$(git rev-parse notes_merge_partial^1)" &&
 	# Mention refs/notes/m, and its current and expected value in output
 	grep -q "refs/notes/m" output &&
 	grep -q "$(git rev-parse refs/notes/m)" output &&
-	grep -q "$(git rev-parse NOTES_MERGE_PARTIAL^1)" output &&
+	grep -q "$(git rev-parse notes_merge_partial^1)" output &&
 	# Verify that other notes refs has not changed (w, x, y and z)
 	verify_notes w &&
 	verify_notes x &&
@@ -541,8 +541,8 @@ EOF
 
 test_expect_success 'resolve situation by aborting the notes merge' '
 	git notes merge --abort &&
-	# No .git/NOTES_MERGE_* files left
-	test_might_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&
+	# No .git/notes_merge_* files left
+	test_might_fail ls .git/notes_merge_* >output 2>/dev/null &&
 	test_cmp /dev/null output &&
 	# m has not moved (still == w)
 	test "$(git rev-parse refs/notes/m)" = "$(git rev-parse refs/notes/w)" &&
@@ -563,7 +563,7 @@ test_expect_success 'switch cwd before committing notes merge' '
 	git notes --ref=other add -m bar HEAD &&
 	test_must_fail git notes merge refs/notes/other &&
 	(
-		cd .git/NOTES_MERGE_WORKTREE &&
+		cd .git/notes_merge_worktree &&
 		echo "foo" > $(git rev-parse HEAD) &&
 		echo "bar" >> $(git rev-parse HEAD) &&
 		git notes merge --commit
diff --git a/t/t3311-notes-merge-fanout.sh b/t/t3311-notes-merge-fanout.sh
index 93516ef..eeaea62 100755
--- a/t/t3311-notes-merge-fanout.sh
+++ b/t/t3311-notes-merge-fanout.sh
@@ -387,10 +387,10 @@ other notes for commit4
 EOF
 
 test_expect_success 'verify conflict entries (with no fanout)' '
-	ls .git/NOTES_MERGE_WORKTREE >output_conflicts &&
+	ls .git/notes_merge_worktree >output_conflicts &&
 	test_cmp expect_conflicts output_conflicts &&
 	( for f in $(cat expect_conflicts); do
-		test_cmp "expect_conflict_$f" ".git/NOTES_MERGE_WORKTREE/$f" ||
+		test_cmp "expect_conflict_$f" ".git/notes_merge_worktree/$f" ||
 		exit 1
 	done ) &&
 	# Verify that current notes tree (pre-merge) has not changed (m == w)
@@ -417,7 +417,7 @@ other notes for commit1
 EOF
 
 test_expect_success 'resolve and finalize merge (z => w)' '
-	cat >.git/NOTES_MERGE_WORKTREE/$commit_sha3 <<EOF &&
+	cat >.git/notes_merge_worktree/$commit_sha3 <<EOF &&
 other notes for commit3
 
 appended notes for commit3
-- 
2.0.4.315.gad8727a-twtrsrc

Re: [PATCH v3 2/6] notes: replace pseudorefs with real refs

[Junio C Hamano]

David Turner <dturner@twopensource.com> writes:

> All-caps files like NOTES_MERGE_REF are pseudorefs, and thus are
> per-worktree.  We don't want multiple notes merges happening at once
> (in different worktrees), so we want to make these refs true refs.
>
> So, we lowercase NOTES_MERGE_REF and friends.  That way, backends
> that distinguish between pseudorefs and real refs can correctly
> handle notes merges.
>
> This will also enable us to prevent pseudorefs from being updated by
> the ref update machinery e.g. git update-ref.
>
> Signed-off-by: David Turner <dturner@twopensource.com>
> ---

This seems to do a bit more than what it claims to do.  As this kind
of changes are very error-prone to review, I did a bulk replace of
the three all-caps NOTES_*THING* and compared the result with what
this patch gives to spot this:

>  	# Fail to finalize merge
>  	test_must_fail git notes merge --commit >output 2>&1 &&
> -	# .git/NOTES_MERGE_* must remain
> -	test -f .git/NOTES_MERGE_PARTIAL &&
> -	test -f .git/NOTES_MERGE_REF &&
> -	test -f .git/NOTES_MERGE_WORKTREE/$commit_sha1 &&
> -	test -f .git/NOTES_MERGE_WORKTREE/$commit_sha2 &&
> -	test -f .git/NOTES_MERGE_WORKTREE/$commit_sha3 &&
> -	test -f .git/NOTES_MERGE_WORKTREE/$commit_sha4 &&
> +	# .git/notes_merge_* must remain
> +	git rev-parse --verify notes_merge_partial &&
> +	git rev-parse --verify notes_merge_ref &&
> +	test -f .git/notes_merge_worktree/$commit_sha1 &&
> +	test -f .git/notes_merge_worktree/$commit_sha2 &&
> +	test -f .git/notes_merge_worktree/$commit_sha3 &&
> +	test -f .git/notes_merge_worktree/$commit_sha4 &&

The two "rev-parse --verify" looks semi-sensible [*1*];
notes_merge_partial is all lowercase and it refers to
$GIT_DIR/notes_merge_partial, because they are shared across working
tree. 

But then why are $GIT_DIR/notes_merge_worktree/* still checked with
"test -f"?  If they are not refs or ref-like things, why should they
be downcased?  If they are, why not "rev-parse --verify"?

[Footnote]

*1* I say "semi-" sensible, because it looks ugly.  All ref-like
    things immediately below $GIT_DIR/ are all-caps by convention.
    Perhaps it is a better idea to move it under refs/; "everything
    under refs/ is shared across working trees" is probably a much
    better rule than "all caps but HEAD is special".

Re: [PATCH v3 2/6] notes: replace pseudorefs with real refs

[David Turner]

On Tue, 2015-07-28 at 12:00 -0700, Junio C Hamano wrote:
> David Turner <dturner@twopensource.com> writes:
> 
> > All-caps files like NOTES_MERGE_REF are pseudorefs, and thus are
> > per-worktree.  We don't want multiple notes merges happening at once
> > (in different worktrees), so we want to make these refs true refs.
> >
> > So, we lowercase NOTES_MERGE_REF and friends.  That way, backends
> > that distinguish between pseudorefs and real refs can correctly
> > handle notes merges.
> >
> > This will also enable us to prevent pseudorefs from being updated by
> > the ref update machinery e.g. git update-ref.
> >
> > Signed-off-by: David Turner <dturner@twopensource.com>
> > ---
> 
> This seems to do a bit more than what it claims to do.  As this kind
> of changes are very error-prone to review, I did a bulk replace of
> the three all-caps NOTES_*THING* and compared the result with what
> this patch gives to spot this:
> 
> >  	# Fail to finalize merge
> >  	test_must_fail git notes merge --commit >output 2>&1 &&
> > -	# .git/NOTES_MERGE_* must remain
> > -	test -f .git/NOTES_MERGE_PARTIAL &&
> > -	test -f .git/NOTES_MERGE_REF &&
> > -	test -f .git/NOTES_MERGE_WORKTREE/$commit_sha1 &&
> > -	test -f .git/NOTES_MERGE_WORKTREE/$commit_sha2 &&
> > -	test -f .git/NOTES_MERGE_WORKTREE/$commit_sha3 &&
> > -	test -f .git/NOTES_MERGE_WORKTREE/$commit_sha4 &&
> > +	# .git/notes_merge_* must remain
> > +	git rev-parse --verify notes_merge_partial &&
> > +	git rev-parse --verify notes_merge_ref &&
> > +	test -f .git/notes_merge_worktree/$commit_sha1 &&
> > +	test -f .git/notes_merge_worktree/$commit_sha2 &&
> > +	test -f .git/notes_merge_worktree/$commit_sha3 &&
> > +	test -f .git/notes_merge_worktree/$commit_sha4 &&
> 
> The two "rev-parse --verify" looks semi-sensible [*1*];
> notes_merge_partial is all lowercase and it refers to
> $GIT_DIR/notes_merge_partial, because they are shared across working
> tree. 
> 
> But then why are $GIT_DIR/notes_merge_worktree/* still checked with
> "test -f"?  If they are not refs or ref-like things, why should they
> be downcased?  If they are, why not "rev-parse --verify"?

They are downcased for consistency with the other notes_merge_* stuff.

> [Footnote]
> 
> *1* I say "semi-" sensible, because it looks ugly.  All ref-like
>     things immediately below $GIT_DIR/ are all-caps by convention.
>     Perhaps it is a better idea to move it under refs/; "everything
>     under refs/ is shared across working trees" is probably a much
>     better rule than "all caps but HEAD is special".

Do you mean move all current pseudorefs?  Or just the notes stuff? 

Moving MERGE_HEAD means that if someone upgrades in the middle of a
merge, they'll end up confused.  The same is true of the notes stuff,
but I imagine that many fewer people use notes than use git merge, so I
wasn't going to worry about that.  What do you think?

Re: [PATCH v3 2/6] notes: replace pseudorefs with real refs

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> David Turner <dturner@twopensource.com> writes:
>
>> All-caps files like NOTES_MERGE_REF are pseudorefs, and thus are
>> per-worktree.  We don't want multiple notes merges happening at once
>> (in different worktrees), so we want to make these refs true refs.
>> ...
> The two "rev-parse --verify" looks semi-sensible [*1*];
> notes_merge_partial is all lowercase and it refers to
> $GIT_DIR/notes_merge_partial, because they are shared across working
> tree. 
>
> But then why are $GIT_DIR/notes_merge_worktree/* still checked with
> "test -f"?  If they are not refs or ref-like things, why should they
> be downcased?  If they are, why not "rev-parse --verify"?
>
> [Footnote]
>
> *1* I say "semi-" sensible, because it looks ugly.  All ref-like
>     things immediately below $GIT_DIR/ are all-caps by convention.
>     Perhaps it is a better idea to move it under refs/; "everything
>     under refs/ is shared across working trees" is probably a much
>     better rule than "all caps but HEAD is special".

I am not sure if "we don't want multiple notes merges at once" is a
valid cause for this inconsistency in the first place.  When you try
to start merging a notes tree in one place (leaving NOTES_MERGE_REF
and friends in the ref storage shared across worktrees), should you
be prevented from merging a different notes tree in another?  Isn't
the whole point of having refs/notes/ hierarchy to allow different
notes to hang underneath there, and isn't NOTES_MERGE_REF symref
about keeping track of which one of them is being worked on in this
working tree?

We don't want multiple usual merges at once in different worktrees,
either; rather, we don't want conflicting updates to the same branch,
be it done by a merge or a single-parent commit, from different
worktrees.  And the way we protect ourselves is by forbidding two
checkout of the same branch by controlling what HEAD can point at.

Making NOTES_MERGE_REF shared across working trees feels like
solving that "no multiple checkouts" problem by making HEAD shared
across working trees, ensuring that only one of them can have usable
HEAD.  Instead, shouldn't we be solving the issue by allowing
NOTES_MERGE_REF in multiple working trees point at different refs
but ensuring that there is at most one working tree that updates one
particular ref in refs/notes/ hierarchy?  Can't we learn from (and
even better, reuse) the logic that controls HEAD for this purpose?

I think it is OK for us to admit that the "notes" subsystem is not
quite ready to work well with multiple working tree world yet [*1*],
and move this series forward without worrying about them.


[Footnote]

*1* I am not saying that the notes subsystem can forever stay to be
    second class citizen that does not know about multiple worktrees
    or pluggable ref backends.  But my brief scan of builtin/notes.c
    seems to indicate that there are quite a lot of work to be done
    to prepare it for these two big changes.  My gut feeling is
    that:

    - NOTES_MERGE_REF symref is like HEAD, that is per working tree
      but is controlled across working trees---no two working trees
      can "checkout" the same refs/notes/* tree for conflict
      resolution at the same time.  It should be all-caps and live
      directly under $GIT_DIR/.

    - NOTES_MERGE_PARTIAL is like MERGE_HEAD or the index in that it
      is merely a state of in-progress merge that is private to a
      single working tree.  $GIT_DIR/NOTES_MERGE_PARTIAL is just
      fine, and we do not have to change it.

    - NOTES_MERGE_WORKTREE is a temporary area in the filesystem
      that houses bunch of blob files (i.e. notes contents).  It
      should be per-working tree but it is not even refs.
      $GIT_DIR/NOTES_MERGE_WORKTREE is just fine, and we do not have
      to change it.

    And as to adjusting to the "ref backend with pseudo ref" world,
    I think the code is more-or-less ready, especially because your
    recent round of this series teaches the "pseudorefs private to
    working tree" to update_ref() and delete_ref(), relieving the
    ref API users like notes subsystem from having to worry about
    them.

    So the only potential issue in the notes subsystem is to ensure
    NOTES_MERGE_REF from multiple working trees do not point at the
    same thing at the same time, with a similar protection we have
    for HEAD, I think.

[PATCH] notes: handle multiple worktrees

[David Turner]

Prevent merges to the same notes branch from different worktrees.
Before creating NOTES_MERGE_REF, check NOTES_MERGE_REF using the same
code we use to check that two HEADs in different worktrees don't point
to the same branch.  Modify that code, die_if_checked_out, to take a
"head" ref to examine; previously, it just looked at HEAD.

Reported-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: David Turner <dturner@twopensource.com>
---
 branch.c                         | 15 +++++----
 branch.h                         |  2 +-
 builtin/checkout.c               |  2 +-
 builtin/notes.c                  |  2 ++
 builtin/worktree.c               |  2 +-
 t/t3320-notes-merge-worktrees.sh | 71 ++++++++++++++++++++++++++++++++++++++++
 6 files changed, 84 insertions(+), 10 deletions(-)
 create mode 100755 t/t3320-notes-merge-worktrees.sh

diff --git a/branch.c b/branch.c
index c85be07..60eadc6 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 *head, 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/$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, head);
 	else
-		strbuf_addf(&path, "%s/HEAD", get_git_common_dir());
+		strbuf_addf(&path, "%s/%s", get_git_common_dir(), head);
 
 	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_checked_out(const char *head, const char *branch)
 {
 	struct strbuf path = STRBUF_INIT;
 	DIR *dir;
 	struct dirent *d;
 
-	check_linked_checkout(branch, NULL);
+	check_linked_checkout(head, branch, NULL);
 
 	strbuf_addf(&path, "%s/worktrees", get_git_common_dir());
 	dir = opendir(path.buf);
@@ -373,7 +374,7 @@ 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(head, branch, d->d_name);
 	}
 	closedir(dir);
 }
diff --git a/branch.h b/branch.h
index 58aa45f..3577fe5 100644
--- a/branch.h
+++ b/branch.h
@@ -57,6 +57,6 @@ extern int read_branch_desc(struct strbuf *, const char *branch_name);
  * worktree and die (with a message describing its checkout location) if
  * it is.
  */
-extern void die_if_checked_out(const char *branch);
+extern void die_if_checked_out(const char *head, const char *branch);
 
 #endif
diff --git a/builtin/checkout.c b/builtin/checkout.c
index e1403be..a5e9f2d 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1107,7 +1107,7 @@ static int checkout_branch(struct checkout_opts *opts,
 		char *head_ref = resolve_refdup("HEAD", 0, sha1, &flag);
 		if (head_ref &&
 		    (!(flag & REF_ISSYMREF) || strcmp(head_ref, new->path)))
-			die_if_checked_out(new->path);
+			die_if_checked_out("HEAD", new->path);
 		free(head_ref);
 	}
 
diff --git a/builtin/notes.c b/builtin/notes.c
index 63f95fc..8794ba1 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_checked_out("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/builtin/worktree.c b/builtin/worktree.c
index 430b51e..bfdb90f 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -203,7 +203,7 @@ static int add_worktree(const char *path, const char *refname,
 	else if (!opts->detach && !strbuf_check_branch_ref(&symref, refname) &&
 		 ref_exists(symref.buf)) { /* it's a branch */
 		if (!opts->force)
-			die_if_checked_out(symref.buf);
+			die_if_checked_out("HEAD", symref.buf);
 	} else { /* must be a commit */
 		commit = lookup_commit_reference_by_name(refname);
 		if (!commit)
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] notes: handle multiple worktrees

[David Turner]

Sorry, this one is on top of next.

On Tue, 2015-07-28 at 17:23 -0400, David Turner wrote:
> Prevent merges to the same notes branch from different worktrees.
> Before creating NOTES_MERGE_REF, check NOTES_MERGE_REF using the same
> code we use to check that two HEADs in different worktrees don't point
> to the same branch.  Modify that code, die_if_checked_out, to take a
> "head" ref to examine; previously, it just looked at HEAD.
> 
> Reported-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: David Turner <dturner@twopensource.com>
> ---
>  branch.c                         | 15 +++++----
>  branch.h                         |  2 +-
>  builtin/checkout.c               |  2 +-
>  builtin/notes.c                  |  2 ++
>  builtin/worktree.c               |  2 +-
>  t/t3320-notes-merge-worktrees.sh | 71 ++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 84 insertions(+), 10 deletions(-)
>  create mode 100755 t/t3320-notes-merge-worktrees.sh
> 
> diff --git a/branch.c b/branch.c
> index c85be07..60eadc6 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 *head, 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/$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, head);
>  	else
> -		strbuf_addf(&path, "%s/HEAD", get_git_common_dir());
> +		strbuf_addf(&path, "%s/%s", get_git_common_dir(), head);
>  
>  	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_checked_out(const char *head, const char *branch)
>  {
>  	struct strbuf path = STRBUF_INIT;
>  	DIR *dir;
>  	struct dirent *d;
>  
> -	check_linked_checkout(branch, NULL);
> +	check_linked_checkout(head, branch, NULL);
>  
>  	strbuf_addf(&path, "%s/worktrees", get_git_common_dir());
>  	dir = opendir(path.buf);
> @@ -373,7 +374,7 @@ 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(head, branch, d->d_name);
>  	}
>  	closedir(dir);
>  }
> diff --git a/branch.h b/branch.h
> index 58aa45f..3577fe5 100644
> --- a/branch.h
> +++ b/branch.h
> @@ -57,6 +57,6 @@ extern int read_branch_desc(struct strbuf *, const char *branch_name);
>   * worktree and die (with a message describing its checkout location) if
>   * it is.
>   */
> -extern void die_if_checked_out(const char *branch);
> +extern void die_if_checked_out(const char *head, const char *branch);
>  
>  #endif
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index e1403be..a5e9f2d 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -1107,7 +1107,7 @@ static int checkout_branch(struct checkout_opts *opts,
>  		char *head_ref = resolve_refdup("HEAD", 0, sha1, &flag);
>  		if (head_ref &&
>  		    (!(flag & REF_ISSYMREF) || strcmp(head_ref, new->path)))
> -			die_if_checked_out(new->path);
> +			die_if_checked_out("HEAD", new->path);
>  		free(head_ref);
>  	}
>  
> diff --git a/builtin/notes.c b/builtin/notes.c
> index 63f95fc..8794ba1 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_checked_out("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/builtin/worktree.c b/builtin/worktree.c
> index 430b51e..bfdb90f 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -203,7 +203,7 @@ static int add_worktree(const char *path, const char *refname,
>  	else if (!opts->detach && !strbuf_check_branch_ref(&symref, refname) &&
>  		 ref_exists(symref.buf)) { /* it's a branch */
>  		if (!opts->force)
> -			die_if_checked_out(symref.buf);
> +			die_if_checked_out("HEAD", symref.buf);
>  	} else { /* must be a commit */
>  		commit = lookup_commit_reference_by_name(refname);
>  		if (!commit)
> 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

Re: [PATCH] notes: handle multiple worktrees

[Junio C Hamano]

David Turner <dturner@twopensource.com> writes:

> Sorry, this one is on top of next.

Thanks, but I'd appreciate if you can be more specific next time.

A topic that truly depends on everything in 'next' cannot graduate
before all the others do, but in this particular case, I think this
change only needs to depend on Eric's es/worktree-add-cleanup and no
other topic, so it is more manageable ;-)

Re: [PATCH] notes: handle multiple worktrees

[Junio C Hamano]

David Turner <dturner@twopensource.com> writes:

> Prevent merges to the same notes branch from different worktrees.
> Before creating NOTES_MERGE_REF, check NOTES_MERGE_REF using the same
> code we use to check that two HEADs in different worktrees don't point
> to the same branch.  Modify that code, die_if_checked_out, to take a
> "head" ref to examine; previously, it just looked at HEAD.
>
> Reported-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: David Turner <dturner@twopensource.com>
> ---

Thanks for following through.  As I didn't report anything, I do not
deserve that label, but it's OK ;-)

I know that it is a requirement to protect NOTES_MERGE_REF from
being used by multiple places for "notes merge" to play well with
the multi-worktree world order, but because I do not know if that is
sufficient, I'm asking a few people for further review.

This is a totally unrelated tangent, but I wonder if "branch -M" to
rename the current branch to an existing branch misbehaves in the
multi-worktree world.  I do not see any die-if-checked-out call in
rename_branch() where create_symref() is used to adjust HEAD for the
current branch.  Duy?

>  branch.c                         | 15 +++++----
>  branch.h                         |  2 +-
>  builtin/checkout.c               |  2 +-
>  builtin/notes.c                  |  2 ++
>  builtin/worktree.c               |  2 +-
>  t/t3320-notes-merge-worktrees.sh | 71 ++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 84 insertions(+), 10 deletions(-)
>  create mode 100755 t/t3320-notes-merge-worktrees.sh

> diff --git a/branch.c b/branch.c
> index c85be07..60eadc6 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 *head, 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/$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, head);
>  	else
> -		strbuf_addf(&path, "%s/HEAD", get_git_common_dir());
> +		strbuf_addf(&path, "%s/%s", get_git_common_dir(), head);
>  
>  	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_checked_out(const char *head, const char *branch)
>  {
>  	struct strbuf path = STRBUF_INIT;
>  	DIR *dir;
>  	struct dirent *d;
>  
> -	check_linked_checkout(branch, NULL);
> +	check_linked_checkout(head, branch, NULL);
>  
>  	strbuf_addf(&path, "%s/worktrees", get_git_common_dir());
>  	dir = opendir(path.buf);
> @@ -373,7 +374,7 @@ 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(head, branch, d->d_name);
>  	}
>  	closedir(dir);
>  }
> diff --git a/branch.h b/branch.h
> index 58aa45f..3577fe5 100644
> --- a/branch.h
> +++ b/branch.h
> @@ -57,6 +57,6 @@ extern int read_branch_desc(struct strbuf *, const char *branch_name);
>   * worktree and die (with a message describing its checkout location) if
>   * it is.
>   */
> -extern void die_if_checked_out(const char *branch);
> +extern void die_if_checked_out(const char *head, const char *branch);
>  
>  #endif
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index e1403be..a5e9f2d 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -1107,7 +1107,7 @@ static int checkout_branch(struct checkout_opts *opts,
>  		char *head_ref = resolve_refdup("HEAD", 0, sha1, &flag);
>  		if (head_ref &&
>  		    (!(flag & REF_ISSYMREF) || strcmp(head_ref, new->path)))
> -			die_if_checked_out(new->path);
> +			die_if_checked_out("HEAD", new->path);
>  		free(head_ref);
>  	}
>  
> diff --git a/builtin/notes.c b/builtin/notes.c
> index 63f95fc..8794ba1 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_checked_out("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/builtin/worktree.c b/builtin/worktree.c
> index 430b51e..bfdb90f 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -203,7 +203,7 @@ static int add_worktree(const char *path, const char *refname,
>  	else if (!opts->detach && !strbuf_check_branch_ref(&symref, refname) &&
>  		 ref_exists(symref.buf)) { /* it's a branch */
>  		if (!opts->force)
> -			die_if_checked_out(symref.buf);
> +			die_if_checked_out("HEAD", symref.buf);
>  	} else { /* must be a commit */
>  		commit = lookup_commit_reference_by_name(refname);
>  		if (!commit)
> 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

Re: [PATCH] notes: handle multiple worktrees

[Johan Herland]

On Wed, Jul 29, 2015 at 12:12 AM, Junio C Hamano <gitster@pobox.com> wrote:
> David Turner <dturner@twopensource.com> writes:
>> Prevent merges to the same notes branch from different worktrees.
>> Before creating NOTES_MERGE_REF, check NOTES_MERGE_REF using the same
>> code we use to check that two HEADs in different worktrees don't point
>> to the same branch.  Modify that code, die_if_checked_out, to take a
>> "head" ref to examine; previously, it just looked at HEAD.
>>
>> Reported-by: Junio C Hamano <gitster@pobox.com>
>> Signed-off-by: David Turner <dturner@twopensource.com>
>> ---
>
> Thanks for following through.  As I didn't report anything, I do not
> deserve that label, but it's OK ;-)
>
> I know that it is a requirement to protect NOTES_MERGE_REF from
> being used by multiple places for "notes merge" to play well with
> the multi-worktree world order, but because I do not know if that is
> sufficient, I'm asking a few people for further review.

As just stated in a related thread, I don't think it makes sense to
have NOTES_MERGE_REF per worktree, as the notes merge is always
completely unrelated to the current worktree (or the current branch
for that matter). AFAICS this patch is all about handling per-worktree
NOTES_MERGE_REFs, and as such I'm NAK on this patch. Instead, there
should only be one NOTES_MERGE_REF per _repo_, and although we might
want to expand to allow multiple concurrent notes merges in the
future, that is still a per-repo, and not a per-worktree thing, hence
completely unrelated to David's current effort.

...Johan

Re: [PATCH] notes: handle multiple worktrees

[Duy Nguyen]

On Wed, Jul 29, 2015 at 5:50 AM, Johan Herland <johan@herland.net> wrote:
> On Wed, Jul 29, 2015 at 12:12 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> David Turner <dturner@twopensource.com> writes:
>>> Prevent merges to the same notes branch from different worktrees.
>>> Before creating NOTES_MERGE_REF, check NOTES_MERGE_REF using the same
>>> code we use to check that two HEADs in different worktrees don't point
>>> to the same branch.  Modify that code, die_if_checked_out, to take a
>>> "head" ref to examine; previously, it just looked at HEAD.
>>>
>>> Reported-by: Junio C Hamano <gitster@pobox.com>
>>> Signed-off-by: David Turner <dturner@twopensource.com>
>>> ---
>>
>> Thanks for following through.  As I didn't report anything, I do not
>> deserve that label, but it's OK ;-)
>>
>> I know that it is a requirement to protect NOTES_MERGE_REF from
>> being used by multiple places for "notes merge" to play well with
>> the multi-worktree world order, but because I do not know if that is
>> sufficient, I'm asking a few people for further review.
>
> As just stated in a related thread, I don't think it makes sense to
> have NOTES_MERGE_REF per worktree, as the notes merge is always
> completely unrelated to the current worktree (or the current branch
> for that matter). AFAICS this patch is all about handling per-worktree
> NOTES_MERGE_REFs, and as such I'm NAK on this patch. Instead, there
> should only be one NOTES_MERGE_REF per _repo_, and although we might
> want to expand to allow multiple concurrent notes merges in the
> future, that is still a per-repo, and not a per-worktree thing, hence
> completely unrelated to David's current effort.

I agree. Luckily sharing NOTES_MERGE_REF is as short as

diff --git a/path.c b/path.c
index 10f4cbf..52d8ee4 100644
--- a/path.c
+++ b/path.c
@@ -94,7 +94,7 @@ static void replace_dir(struct strbuf *buf, int len,
const char *newdir)
 static const char *common_list[] = {
  "/branches", "/hooks", "/info", "!/logs", "/lost-found",
  "/objects", "/refs", "/remotes", "/worktrees", "/rr-cache", "/svn",
- "config", "!gc.pid", "packed-refs", "shallow",
+ "config", "!gc.pid", "packed-refs", "shallow", "NOTES_MERGE_REF",
  NULL
 };
 --
Duy

Re: [PATCH] notes: handle multiple worktrees

[Eric Sunshine]

On Tue, Jul 28, 2015 at 5:23 PM, David Turner <dturner@twopensource.com> wrote:
> Prevent merges to the same notes branch from different worktrees.
> Before creating NOTES_MERGE_REF, check NOTES_MERGE_REF using the same
> code we use to check that two HEADs in different worktrees don't point
> to the same branch.  Modify that code, die_if_checked_out, to take a
> "head" ref to examine; previously, it just looked at HEAD.

die_if_checked_out() seems to be grossly misnamed following this
change since a branch (or detached head) can be "checked out", but
asking it if a HEAD or NOTES_MERGE_REF is "checked out" sounds weird.
Perhaps rename it to die_if_symref_shared() or something?

The new argument order, die_if_checked_out("HEAD", "branchname"), also
seems backward to me, at least with the current function name
(die_if_checked_out). With a better function name, maybe it wouldn't
be so bad.

I could also see die_if_checked_out() being refactored to move the
underlying logic to a new (better named) function for checking the
value of a particular file (HEAD, NOTES_MERGE_REF) in each linked
worktree. And then die_if_checked_out() would be a thin wrapper over
that new function. A reason for preferring this approach and keeping
die_if_checked_out() as a convenience wrapper is that it's likely that
it's going to be needed in more places in the future.

> Reported-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: David Turner <dturner@twopensource.com>
> ---
> diff --git a/branch.c b/branch.c
> index c85be07..60eadc6 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 *head, 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/$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, head);
>         else
> -               strbuf_addf(&path, "%s/HEAD", get_git_common_dir());
> +               strbuf_addf(&path, "%s/%s", get_git_common_dir(), head);
>
>         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_checked_out(const char *head, const char *branch)
>  {
>         struct strbuf path = STRBUF_INIT;
>         DIR *dir;
>         struct dirent *d;
>
> -       check_linked_checkout(branch, NULL);
> +       check_linked_checkout(head, branch, NULL);
>
>         strbuf_addf(&path, "%s/worktrees", get_git_common_dir());
>         dir = opendir(path.buf);
> @@ -373,7 +374,7 @@ 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(head, branch, d->d_name);
>         }
>         closedir(dir);
>  }
> diff --git a/branch.h b/branch.h
> index 58aa45f..3577fe5 100644
> --- a/branch.h
> +++ b/branch.h
> @@ -57,6 +57,6 @@ extern int read_branch_desc(struct strbuf *, const char *branch_name);
>   * worktree and die (with a message describing its checkout location) if
>   * it is.
>   */
> -extern void die_if_checked_out(const char *branch);
> +extern void die_if_checked_out(const char *head, const char *branch);
>
>  #endif

Re: [PATCH v3 2/6] notes: replace pseudorefs with real refs

[Johan Herland]

On Tue, Jul 28, 2015 at 9:44 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>> David Turner <dturner@twopensource.com> writes:
>>> All-caps files like NOTES_MERGE_REF are pseudorefs, and thus are
>>> per-worktree.  We don't want multiple notes merges happening at once
>>> (in different worktrees), so we want to make these refs true refs.
>>> ...
>> The two "rev-parse --verify" looks semi-sensible [*1*];
>> notes_merge_partial is all lowercase and it refers to
>> $GIT_DIR/notes_merge_partial, because they are shared across working
>> tree.
>>
>> But then why are $GIT_DIR/notes_merge_worktree/* still checked with
>> "test -f"?  If they are not refs or ref-like things, why should they
>> be downcased?  If they are, why not "rev-parse --verify"?
>>
>> [Footnote]
>>
>> *1* I say "semi-" sensible, because it looks ugly.  All ref-like
>>     things immediately below $GIT_DIR/ are all-caps by convention.
>>     Perhaps it is a better idea to move it under refs/; "everything
>>     under refs/ is shared across working trees" is probably a much
>>     better rule than "all caps but HEAD is special".
>
> I am not sure if "we don't want multiple notes merges at once" is a
> valid cause for this inconsistency in the first place.  When you try
> to start merging a notes tree in one place (leaving NOTES_MERGE_REF
> and friends in the ref storage shared across worktrees), should you
> be prevented from merging a different notes tree in another?  Isn't
> the whole point of having refs/notes/ hierarchy to allow different
> notes to hang underneath there, and isn't NOTES_MERGE_REF symref
> about keeping track of which one of them is being worked on in this
> working tree?

I believe the current M.O. for notes merge is to allow only one
simultaneous notes merge per _repo_. AFAIR, the notes merge code is
completely unrelated to the work tree (you could probably even do a
notes merge in a _bare_ repo), so IINM, the NOTES_MERGE_* refs should
be bound to the _repo_, NOT to the worktree.

So if I understand the pseudoref concept correctly, that should make
NOTES_MERGE_* "regular" refs, and not pseudorefs.

> We don't want multiple usual merges at once in different worktrees,
> either; rather, we don't want conflicting updates to the same branch,
> be it done by a merge or a single-parent commit, from different
> worktrees.  And the way we protect ourselves is by forbidding two
> checkout of the same branch by controlling what HEAD can point at.
>
> Making NOTES_MERGE_REF shared across working trees feels like
> solving that "no multiple checkouts" problem by making HEAD shared
> across working trees, ensuring that only one of them can have usable
> HEAD.  Instead, shouldn't we be solving the issue by allowing
> NOTES_MERGE_REF in multiple working trees point at different refs
> but ensuring that there is at most one working tree that updates one
> particular ref in refs/notes/ hierarchy?  Can't we learn from (and
> even better, reuse) the logic that controls HEAD for this purpose?
>
> I think it is OK for us to admit that the "notes" subsystem is not
> quite ready to work well with multiple working tree world yet [*1*],
> and move this series forward without worrying about them.
>
>
> [Footnote]
>
> *1* I am not saying that the notes subsystem can forever stay to be
>     second class citizen that does not know about multiple worktrees
>     or pluggable ref backends.  But my brief scan of builtin/notes.c
>     seems to indicate that there are quite a lot of work to be done
>     to prepare it for these two big changes.  My gut feeling is
>     that:
>
>     - NOTES_MERGE_REF symref is like HEAD, that is per working tree
>       but is controlled across working trees---no two working trees
>       can "checkout" the same refs/notes/* tree for conflict
>       resolution at the same time.

This sentence does not make sense to me. IMHO, a notes merge has
nothing to do with the worktree at all. Instead, the notes merge
creates its _own_ worktree (under .git/NOTES_MERGE_WORKTREE) when
needed (i.e. when there are notes conflicts to be resolved).

>       It should be all-caps and live
>       directly under $GIT_DIR/.

It should definitely live under $GIT_DIR (and not inside each
worktree's .git/). Whether or not it's all-caps is not that important
to me.

>
>     - NOTES_MERGE_PARTIAL is like MERGE_HEAD or the index in that it
>       is merely a state of in-progress merge that is private to a
>       single working tree.  $GIT_DIR/NOTES_MERGE_PARTIAL is just
>       fine, and we do not have to change it.

Agreed.

>
>     - NOTES_MERGE_WORKTREE is a temporary area in the filesystem
>       that houses bunch of blob files (i.e. notes contents).  It
>       should be per-working tree but it is not even refs.
>       $GIT_DIR/NOTES_MERGE_WORKTREE is just fine, and we do not have
>       to change it.

>From the notes merge perspective, NOTES_MERGE_WORKTREE _is_ the
worktree. There is no other worktree that is relevant to the notes
merge code. So I'm not quite sure how to handle notes merges in the
multiple worktree scenario. Do we expect users running notes merges
simultaneously from mulitple worktrees? If that is something we need
to support, why not take it one step further and support simultaneous
notes merges from a single worktree?

>
>     And as to adjusting to the "ref backend with pseudo ref" world,
>     I think the code is more-or-less ready, especially because your
>     recent round of this series teaches the "pseudorefs private to
>     working tree" to update_ref() and delete_ref(), relieving the
>     ref API users like notes subsystem from having to worry about
>     them.
>
>     So the only potential issue in the notes subsystem is to ensure
>     NOTES_MERGE_REF from multiple working trees do not point at the
>     same thing at the same time, with a similar protection we have
>     for HEAD, I think.

Agreed. Namespacing the NOTES_MERGE_* files based on the contents of
NOTES_MERGE_REF is probably the way to go here. I.e. instead of

  .git/NOTES_MERGE_REF
  .git/NOTES_MERGE_PARTIAL
  .git/NOTES_MERGE_WORKTREE

you would have (assuming NOTES_MERGE_REF == refs/notes/foo):

  .git/NOTES_MERGE/refs/notes/foo/REF
  .git/NOTES_MERGE/refs/notes/foo/PARTIAL
  .git/NOTES_MERGE/refs/notes/foo/WORKTREE

and then a simultaneous notes-merge on a different notes ref
(refs/notes/bar) would live in

  .git/NOTES_MERGE/refs/notes/bar/REF
  .git/NOTES_MERGE/refs/notes/bar/PARTIAL
  .git/NOTES_MERGE/refs/notes/bar/WORKTREE

Also, we would need support on the command side, i.e. "git notes merge
commit/abort" needs to be told _which_ notes merge to commit/abort...

However, in any case, notes merges are always per _repo_ and never per
_worktree_, so this is all unrelated to the current patch/discussion
AFAICS.


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCH v3 2/6] notes: replace pseudorefs with real refs

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

Johan Herland <johan@herland.net> writes:

> However, in any case, notes merges are always per _repo_ and never per
> _worktree_, so this is all unrelated to the current patch/discussion
> AFAICS.

Thanks for chiming in, but I actually think you are confused.

"git merge" is always per _repo_ in the sense that the result of a
merge of a topic to the 'master' is recorded in the 'master' which
is per-repo.  In the multi-worktree world order, that does not
change.  What changes is that you could have different worktrees
that check out different branches.  Worktree A may have 'master'
checked out and do the merge there to update the tip of 'master'.
But while worktree A is doing that, worktree B may have 'next'
checked out and do an unrelated merge there.  Once worktree A leaves
'master' by checking out another branch, worktree B is free to check
out 'master' and do further merges there.  Merging into 'master' is
per _repo_, but the act of merging is per worktree.

I think merges of refs/notes/commits and refs/notes/someotherthing
works exactly the same way.  In worktree A, you may decide to merge
a notes tree refs/notes/commits with somebody else's.  It may
conflict and you may need to "lock" refs/notes/commits from being
touched by other worktrees while resolving that, but that does not
mean other worktrees cannot do a merge of refs/notes/someotherthing
at all.  The temporary area you use for merging notes, i.e. the
working tree as far as notes merge is concerned, is private to
worktree A and does not need to be seen by other worktrees.

So while you are working on merging and resolving, that intermediate
state is *NOT* per _repo_ at all.  It is at most per worktree (Yes
you could extend and have one notes_merge_ref per each refs/notes/*
ref to make it even finer grained to allow more than one notes merge
going on inside a single worktree, but I do not think it is worth
it).

Re: [PATCH v3 2/6] notes: replace pseudorefs with real refs

[Johan Herland]

On Wed, Jul 29, 2015 at 12:52 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Johan Herland <johan@herland.net> writes:
>> However, in any case, notes merges are always per _repo_ and never per
>> _worktree_, so this is all unrelated to the current patch/discussion
>> AFAICS.
>
> Thanks for chiming in, but I actually think you are confused.
>
> "git merge" is always per _repo_ in the sense that the result of a
> merge of a topic to the 'master' is recorded in the 'master' which
> is per-repo.  In the multi-worktree world order, that does not
> change.  What changes is that you could have different worktrees
> that check out different branches.  Worktree A may have 'master'
> checked out and do the merge there to update the tip of 'master'.
> But while worktree A is doing that, worktree B may have 'next'
> checked out and do an unrelated merge there.  Once worktree A leaves
> 'master' by checking out another branch, worktree B is free to check
> out 'master' and do further merges there.  Merging into 'master' is
> per _repo_, but the act of merging is per worktree.

Agreed thus far.

> I think merges of refs/notes/commits and refs/notes/someotherthing
> works exactly the same way.  In worktree A, you may decide to merge
> a notes tree refs/notes/commits with somebody else's.

Here is where we start to differ. I would say that starting a notes
merge is completely unrelated to your worktree. Consider this:

When you start a "regular" (non-notes) merge in worktree A, that merge
will "occupy" worktree A for the purpose of completing the merge, e.g.
conflicting files will be checked out inside worktree A, and it is
obvious that you cannot do other/unrelated things in worktree A until
you have resolved the conflicts and completed the merge. As such, a
regular merge is inextricably bound to a specific worktree.

This is not the case for notes merges. If I start a notes merge from
worktree A, there is no "occupation" of that worktree. Before the
notes merge is resolved, I can do whatever I want in worktree A
(including checking out a different branch, performing a rebase,
whatever...). Instead, the notes merge creates its own worktree (that
is "occupied" until the notes merge is completed), which is completely
unrelated to worktree A.

>  It may
> conflict and you may need to "lock" refs/notes/commits from being
> touched by other worktrees while resolving that, but that does not
> mean other worktrees cannot do a merge of refs/notes/someotherthing
> at all.

In principle, I agree that an ongoing notes-merge into
refs/notes/someotherthing should be able to coexist with an ongoing
notes-merge into refs/notes/commits. However, it does not make sense
to bind those notes-merges to a specific worktree.

Let's say I have two worktrees, A and B, and from worktree A, I have
started a notes-merge into refs/notes/commits. Now:

 - From worktree B I should NOT be able to start another notes-merge
into refs/notes/commits.

 - From worktree B I SHOULD be able to start another notes-merge into
refs/notes/someotherthing

But this doesn't really have anything to do with worktree B. I can
just as easily say:

 - From worktree A I should NOT be able to start another notes-merge
into refs/notes/commits.

 - From worktree A I SHOULD be able to start another notes-merge into
refs/notes/someotherthing

My conclusion is therefore that binding a notes merge to a specific
worktree does not make any sense. Preventing simultaneous notes merges
into the same notes ref is something that must be solved per _repo_
(and not per worktree), and since the worktree plays no part in the
resolution/completion of the notes merge, it makes more sense to place
all the notes-merge-related refs/files inside the _repo_, and not
inside the worktree.

Now, we do not yet support simultaneous notes merges at all, but my
follow-on point is that the addition of such support is wholly
independent of the multi-worktree support. For now, it would make more
sense to only allow a single notes-merge across all worktrees. I.e.
when starting a notes-merge from ANY worktree, it should simply fail
if there is an existing unresolved notes merge (no matter which
worktree started that unresolved notes merge).

>  The temporary area you use for merging notes, i.e. the
> working tree as far as notes merge is concerned, is private to
> worktree A and does not need to be seen by other worktrees.

Disagree. The private area used to resolve notes merges should be per
_repo_, not per worktree. That is IMHO the only sane way to (in the
future) prevent simultaneous notes merges going into the same notes
ref.

> So while you are working on merging and resolving, that intermediate
> state is *NOT* per _repo_ at all.  It is at most per worktree (Yes
> you could extend and have one notes_merge_ref per each refs/notes/*
> ref to make it even finer grained to allow more than one notes merge
> going on inside a single worktree, but I do not think it is worth
> it).

As stated above, my position is that while you are resolving a notes
merge, the worktree from which you started that notes merge is
completely irrelevant. In fact, you can easily do a notes merge in a
_bare_ repo...

...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCH v3 2/6] notes: replace pseudorefs with real refs

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> Here is where we start to differ. I would say that starting a notes
> merge is completely unrelated to your worktree. Consider this:
> ...
> This is not the case for notes merges. If I start a notes merge from
> worktree A, there is no "occupation" of that worktree. Before the
> notes merge is resolved, I can do whatever I want in worktree A
> (including checking out a different branch, performing a rebase,
> whatever...). Instead, the notes merge creates its own worktree (that
> is "occupied" until the notes merge is completed), which is completely
> unrelated to worktree A.

That does not mean the notes merge that you started when you were
sitting in worktree A has to be shared with worktree B, by say doing
"vi .git/NOTES_MERGE_WORKTREE/$commit && git notes merge --commit".

Also the above does not explain why it is sensible for you to forbid
worktree B from doing an unrelated notes merge of a different ref
under refs/notes/* while your worktree A is doing a notes merge.

> In principle, I agree that an ongoing notes-merge into
> refs/notes/someotherthing should be able to coexist with an ongoing
> notes-merge into refs/notes/commits. However, it does not make sense
> to bind those notes-merges to a specific worktree.

The thing is, the choice is between per worktree or per repository.
Taking the latter would mean you can only be doing one notes merge
at a time, even though you prepared multiple worktrees so that you
can work on different things at a time.  It is true that there is
nothing inherent that ties a note merge to a worktree (a worktree is
tied to a branch that is checed out, and there is no tie between a
branch and a notes tree), but "not inherantly tied to" does not
automatically mean "has to be one per repository".  You'd ideally
want to allow N workspaces for N refs/notes/* refs.

But people work in worktrees, and that is their unit of working
space.  From that point of view, unless you are proposing a
completely different design where the primary worktree can be used
only for manipulating notes (hence, you can have worktrees for
resolving refs/notes/A and refs/notes/B, in addition to the other
worktrees you use to advance branches), treating NOTES_MERGE_REF as
a per-worktree entity just like HEAD and the index is, would be the
most sensible comporise.

> Let's say I have two worktrees, A and B, and from worktree A, I have
> started a notes-merge into refs/notes/commits. Now:
>
>  - From worktree B I should NOT be able to start another notes-merge
> into refs/notes/commits.

Correct.

>  - From worktree B I SHOULD be able to start another notes-merge into
> refs/notes/someotherthing

Correct.

> But this doesn't really have anything to do with worktree B. 

Correct. There is nothing inherent that ties someotherthing to B and
commits to A.  The only reason why I think "per worktree" is vastly
superiour than "only one per repository" is because the end user did
set up two worktrees so that he can do two things at once
independently.

> I can
> just as easily say:
>
>  - From worktree A I should NOT be able to start another notes-merge
> into refs/notes/commits.

Correct.

>  - From worktree A I SHOULD be able to start another notes-merge into
> refs/notes/someotherthing

Irrelevant and moving the goalpost.  We are not talking about
extending capability of the system that does not use multi-worktree.
We do not do two regular merges in a single worktree at a time, and
I do not think it is sensible to claim "I SHOULD be able to".

> Now, we do not yet support simultaneous notes merges at all, but my
> follow-on point is that the addition of such support is wholly
> independent of the multi-worktree support.

Now we are getting somewhere.  So is there more that is needed than
separating NOTES_MERGE_REF per worktree to make this work (remember,
multiple notes-merge in a single worktree is a non-goal, just like
multiple merge in a single worktree is not supported today and will
not be)?  Is there some other state that is not captured by
NOTES_MERGE_REF and friends that you would end up recording a wrong
merge result, if two worktrees that have NOTES_MERGE_REF pointing at
a different ref in refs/notes/* try to do the notes-merge at the
same time?

> For now, it would make more
> sense to only allow a single notes-merge across all worktrees. I.e.
> when starting a notes-merge from ANY worktree, it should simply fail
> if there is an existing unresolved notes merge (no matter which
> worktree started that unresolved notes merge).

I do not understand why we need to have such a restriction.  It is
like saying you may have two worktrees but you cannot merge into
master in worktree A if you are doing a merge into next in worktree
B.  I'd need a clear answer to the question in the previous
paragraph to say something like "ah, ok, that limitation (either
imposed by the current code design, or perhaps reliance of the
filesystem, or whatever) I didn't think about, and now what you say
makes sense to me".

> Disagree. The private area used to resolve notes merges should be per
> _repo_, not per worktree.

I can buy "the private area for resolving refs/notes/common must be
a single place per repo that is differnet from the private area used
for refs/notes/somethingelse, which would also have its own single
place per repo".

But then what you are talkinng about is per _ref_, not per _repo_.

And I'd very much love to see this per _ref_; that is fantastic.

Because the suggestion is to allow a single notes ref refs/notes/*
to be "checked out" only in a single worktree at a time, that per
_ref_ thing falls out naturally from per _worktree_ arrangement.

And that is how people work, by creating a separate worktree, they
gain one additional workspace.

Re: [PATCH v3 2/6] notes: replace pseudorefs with real refs

[Michael Haggerty]

Johan Herland <johan@herland.net> writes:
> Here is where we start to differ. I would say that starting a notes
> merge is completely unrelated to your worktree. Consider this:

It sounds like what a notes merge really wants is a new linked worktree
that has branch refs/notes/foo checked out:

* This would allow multiple notes merges to take place at the same time
provided they target different merge references.

* This would prevent multiple notes merges to the same notes reference
at the same time by the same mechanism that prevents the same branch
from being checked out in two linked worktrees at the same time.

It's just a thought; I have no idea whether it is practical...

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH v3 2/6] notes: replace pseudorefs with real refs

[Jacob Keller]

On Tue, Jul 28, 2015 at 5:56 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> Johan Herland <johan@herland.net> writes:
>> Here is where we start to differ. I would say that starting a notes
>> merge is completely unrelated to your worktree. Consider this:
>
> It sounds like what a notes merge really wants is a new linked worktree
> that has branch refs/notes/foo checked out:
>
> * This would allow multiple notes merges to take place at the same time
> provided they target different merge references.
>
> * This would prevent multiple notes merges to the same notes reference
> at the same time by the same mechanism that prevents the same branch
> from being checked out in two linked worktrees at the same time.
>
> It's just a thought; I have no idea whether it is practical...
>
> Michael
>

That seems far more flexible and robust to me.

Regards,
Jake

Re: [PATCH v3 2/6] notes: replace pseudorefs with real refs

[Johan Herland]

On Wed, Jul 29, 2015 at 2:56 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> Johan Herland <johan@herland.net> writes:
>> Here is where we start to differ. I would say that starting a notes
>> merge is completely unrelated to your worktree. Consider this:
>
> It sounds like what a notes merge really wants is a new linked worktree
> that has branch refs/notes/foo checked out:

Yes, almost. There are some complications with the concept of
"checking out" a notes tree:

 - The notes tree fanout must be flattened (so that when merging two
note trees with different fanout, conflicting notes (e.g. deadbeef...
and de/adbeef....) are turned into a file-level conflict in the notes
merge worktree (i.e. contents with conflict markers in
.git/NOTES_MERGE_WORKTREE/deadbeef...).

 - Notes trees may be very large (e.g. one note per commit for the
entire project history), so we want to avoid checking out as many
notes as possible. Currently we only checkout the notes that actually
do conflict, and keep the rest referenced from NOTES_MERGE_PARTIAL.

> * This would allow multiple notes merges to take place at the same time
> provided they target different merge references.
>
> * This would prevent multiple notes merges to the same notes reference
> at the same time by the same mechanism that prevents the same branch
> from being checked out in two linked worktrees at the same time.
>
> It's just a thought; I have no idea whether it is practical...

I'm not sure whether it's worth trying to reuse the same linked
worktree mechanism for notes trees, when (a) the concept of "checking
out" a notes tree is so different, as explained above, and (b)
currently the only use case for a notes worktree is during a notes
merge.


...Johan

> Michael
>
> --
> Michael Haggerty
> mhagger@alum.mit.edu
>

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCH v3 2/6] notes: replace pseudorefs with real refs

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> Yes, almost. There are some complications with the concept of
> "checking out" a notes tree:
>
>  - The notes tree fanout must be flattened (so that when merging two
> note trees with different fanout, conflicting notes (e.g. deadbeef...
> and de/adbeef....) are turned into a file-level conflict in the notes
> merge worktree (i.e. contents with conflict markers in
> .git/NOTES_MERGE_WORKTREE/deadbeef...).

True.  I however think Michael was envisioning further into the
future, where the tree-level merges would take care of that detail
and populate the index to express conflicts using "canonicalised"
paths, removing these fan-out slashes, before externalising the
conflicted paths that need user's attention on the filesystem.

>  - Notes trees may be very large (e.g. one note per commit for the
> entire project history), so we want to avoid checking out as many
> notes as possible. Currently we only checkout the notes that actually
> do conflict, and keep the rest referenced from NOTES_MERGE_PARTIAL.

This is a valid point, but nobody forces us to do a full checkout to
perform a merge.  From day one, our merge machinery is prepared to
merge in an empty working tree, only checking out paths that need to
be modified by the merge.

>> * This would allow multiple notes merges to take place at the same time
>> provided they target different merge references.
>>
>> * This would prevent multiple notes merges to the same notes reference
>> at the same time by the same mechanism that prevents the same branch
>> from being checked out in two linked worktrees at the same time.
>>
>> It's just a thought; I have no idea whether it is practical...
>
> I'm not sure whether it's worth trying to reuse the same linked
> worktree mechanism for notes trees, when (a) the concept of "checking
> out" a notes tree is so different, as explained above, and (b)
> currently the only use case for a notes worktree is during a notes
> merge.

I have a very similar feeling except that I'd replace "linked
worktree mechanism" with "checkout mechanism".  If we were to
introduce such a new checkout mechanism that flattens a fan-out
paths, with "sparse checkout"-like behaviour, the current "checkout
mechanism" would not be reused, but the resulting system would
benefit from "linked worktree mechanism" just as much as the normal
multiple worktree layout benefits from it.  You'd want to make sure
only one such worktree has the checkout of one refs/notes/* ref,
everything in refs/* is shared across the repository and its
worktrees, and certain things like MERGE_HEAD and the index inside
$GIT_DIR/ are not shared, which are what the linked worktree
mechanism gives us.

Re: [PATCH v3 2/6] notes: replace pseudorefs with real refs

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> It sounds like what a notes merge really wants is a new linked worktree
> that has branch refs/notes/foo checked out:
>
> * This would allow multiple notes merges to take place at the same time
> provided they target different merge references.
>
> * This would prevent multiple notes merges to the same notes reference
> at the same time by the same mechanism that prevents the same branch
> from being checked out in two linked worktrees at the same time.
>
> It's just a thought; I have no idea whether it is practical...

That was certainly one of the possibilities that crossed my mind.

In any case, the primary thing I am interested in at this point is
to unblock David's "prepare things so that we can put primary refs
in a different ref backends more easily" topic, and I've already
made my point a few messages ago upstream:

    I think it is OK for us to admit that the "notes" subsystem is
    not quite ready to work well with multiple working tree world
    yet [*1*], and move this series forward without worrying about
    them.

So doing the absolute minimum, leaving the "now what can we do to
improve notes-merge process?" outside the scope of the topic.

Improving the notes-merge process is also an interesting topic, but
it is clear that people started thinking about it today ;-), so it
can wait without blocking David's work.  The refs/notes/* hierarchy
will be handled exactly the same way as regular branches in the
pluggable ref-backends, and how the ephemeral REF_NOTES_REF and
friends are represented (some are refs, some may be pseudo-refs,
some may be just a filesystem entity) would need to be revamped if
we really do the "improving notes-merge" thing anyway, so David's
topic shouldn't be blocked by the "possible future tweak" of the
current design that hasn't happened yet.  Instead, improving
notes-merge can be done on top, potentially undoing and redoing
Davi'd topic.

Re: [PATCH v3 2/6] notes: replace pseudorefs with real refs

[Johan Herland]

On Wed, Jul 29, 2015 at 4:00 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
>> It sounds like what a notes merge really wants is a new linked worktree
>> that has branch refs/notes/foo checked out:
>>
>> * This would allow multiple notes merges to take place at the same time
>> provided they target different merge references.
>>
>> * This would prevent multiple notes merges to the same notes reference
>> at the same time by the same mechanism that prevents the same branch
>> from being checked out in two linked worktrees at the same time.
>>
>> It's just a thought; I have no idea whether it is practical...
>
> That was certainly one of the possibilities that crossed my mind.
>
> In any case, the primary thing I am interested in at this point is
> to unblock David's "prepare things so that we can put primary refs
> in a different ref backends more easily" topic, and I've already
> made my point a few messages ago upstream:
>
>     I think it is OK for us to admit that the "notes" subsystem is
>     not quite ready to work well with multiple working tree world
>     yet [*1*], and move this series forward without worrying about
>     them.
>
> So doing the absolute minimum, leaving the "now what can we do to
> improve notes-merge process?" outside the scope of the topic.

That's exactly what I was also trying to do: David's topic should not
have to deal with NOTES_MERGE_* at all. Simply leave it all as
something that belongs in $GIT_COMMON_DIR, and let's solve concurrent
unrelated notes merges as a wholly independent, separate topic.

...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCH v3 2/6] notes: replace pseudorefs with real refs

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> On Wed, Jul 29, 2015 at 4:00 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
>> So doing the absolute minimum, leaving the "now what can we do to
>> improve notes-merge process?" outside the scope of the topic.
>
> That's exactly what I was also trying to do: David's topic should not
> have to deal with NOTES_MERGE_* at all. Simply leave it all as
> something that belongs in $GIT_COMMON_DIR, and let's solve concurrent
> unrelated notes merges as a wholly independent, separate topic.

Here, it perhaps is showing that you are unfamiliar with the linked
worktree topic.

Things directly under .git/, like HEAD, MERGE_HEAD, etc., are per
worktree (i.e. not shared across working trees).  You have to work
to make them shared, i.e. per repo.  David's earlier one downcased
them while still keeing them directly under .git/ but I think it is
ugly and in general a wrong way to mark what is shared and what is
per worktree.  E.g. index is not shared, even though the name is all
lowercase.

You will be updating that mechanism when you truly make the
notes-merge a per refs/notes/* ref thing (not per repo, not per
worktree).  Perhaps you will use refs/notes-merge/$foo that is
shared across worktrees as the replacement for NOTES_MERGE_REF that
currently is used to merge into refs/notes/$foo, or something like
that, to pursue your "per ref" goal.  At that point, NOTES_MERGE_REF
based design needs to be scrapped anyway; it should not matter if
NOTES_MERGE_REF is per worktree (not shared) or per repo (shared) in
the meantime.

Doing absolute minimum means that $GIT_DIR/NOTES_MERGE_REF should be
left per worktree, like MERGE_HEAD, etc., without doing anything
like downcasing or moving it under refs/notes-merge/ (which I think
is a better way to make it shared, if we wanted to), which is
additional special casing that will be a wasted effort that would
not matter in the end result.

Re: [PATCH v3 2/6] notes: replace pseudorefs with real refs

[Johan Herland]

On Wed, Jul 29, 2015 at 2:33 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Johan Herland <johan@herland.net> writes:
>
>> Here is where we start to differ. I would say that starting a notes
>> merge is completely unrelated to your worktree. Consider this:
>> ...
>> This is not the case for notes merges. If I start a notes merge from
>> worktree A, there is no "occupation" of that worktree. Before the
>> notes merge is resolved, I can do whatever I want in worktree A
>> (including checking out a different branch, performing a rebase,
>> whatever...). Instead, the notes merge creates its own worktree (that
>> is "occupied" until the notes merge is completed), which is completely
>> unrelated to worktree A.
>
> That does not mean the notes merge that you started when you were
> sitting in worktree A has to be shared with worktree B, by say doing
> "vi .git/NOTES_MERGE_WORKTREE/$commit && git notes merge --commit".
>
> Also the above does not explain why it is sensible for you to forbid
> worktree B from doing an unrelated notes merge of a different ref
> under refs/notes/* while your worktree A is doing a notes merge.

I do not argue that it is sensible to forbid concurrent unrelated
notes merges. I argue that using linked worktrees is a poor solution
for concurrent unrelated notes merges.

A better solution does not concern itself with worktrees at all, and
does not need to add nonsensical conditions like:

    die_if_checked_out("NOTES_MERGE_REF", default_notes_ref());

A better solution does not need to add complexity to the branch or
linked worktree code to deal with notes merges. Instead, it simply
organizes the notes merge worktrees in such a manner that the correct
semantics naturally emerge.

Again, let's compare the two approaches (against the current situation):

Current situation:
 - A single $GIT_COMMON_DIR/NOTES_MERGE_*
 - Concurrent (unrelated or not) notes merges are simply not supported

Proposal A (please correct me where I have misunderstood what's proposed):
 - Each worktree has its own $GIT_DIR/NOTES_MERGE_*
 - Concurrent unrelated notes merges are supported, provided that you
create an additional linked worktree to "host" each concurrent notes
merge.
 - Logic must be added to ensure unrelated-ness, i.e. make sure that
the NOTES_MERGE_REF in worktree X is different from all other
worktrees' NOTES_MERGE_REF.

Proposal B:
 - The repo has a $GIT_COMMON_DIR/notes-merge/$ref/* hierarchy for
organizing concurrent notes merges
 - Concurrent unrelated notes merges are supported, independently of
whether you have zero, one, or several worktrees.
 - The notes merge code must be adjusted to work with the above
hierarchy, and must naturally fail if the user attempts to start a new
notes merge that would clobber an in-progress notes merge (only notes
merges to the same notes ref will clobber).

I obviously feel proposal B is superior to A, so I wonder what I have
missed about A that makes it preferable.

>> In principle, I agree that an ongoing notes-merge into
>> refs/notes/someotherthing should be able to coexist with an ongoing
>> notes-merge into refs/notes/commits. However, it does not make sense
>> to bind those notes-merges to a specific worktree.
>
> The thing is, the choice is between per worktree or per repository.
> Taking the latter would mean you can only be doing one notes merge
> at a time, even though you prepared multiple worktrees so that you
> can work on different things at a time.  It is true that there is
> nothing inherent that ties a note merge to a worktree (a worktree is
> tied to a branch that is checed out, and there is no tie between a
> branch and a notes tree), but "not inherantly tied to" does not
> automatically mean "has to be one per repository".  You'd ideally
> want to allow N workspaces for N refs/notes/* refs.
>
> But people work in worktrees, and that is their unit of working
> space.  From that point of view, unless you are proposing a
> completely different design where the primary worktree can be used
> only for manipulating notes (hence, you can have worktrees for
> resolving refs/notes/A and refs/notes/B, in addition to the other
> worktrees you use to advance branches), treating NOTES_MERGE_REF as
> a per-worktree entity just like HEAD and the index is, would be the
> most sensible comporise.

I believe it is a bad compromise. It complicates the code, and it
provides a concurrent notes merges that is unnecessarily tied to (and
dependent on) worktrees. For example, if I wanted to perform N
concurrent notes merges in a project that happens to have a huge
worktree, I would now have to create N copies of the huge worktree...


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCH v3 2/6] notes: replace pseudorefs with real refs

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> I believe it is a bad compromise. It complicates the code, and it
> provides a concurrent notes merges that is unnecessarily tied to (and
> dependent on) worktrees. For example, if I wanted to perform N
> concurrent notes merges in a project that happens to have a huge
> worktree, I would now have to create N copies of the huge worktree...

Who said worktree has to be populated?  You should be able to have
an absolutely empty checkout just like "clone --no-checkout".

Re: [PATCH v3 2/6] notes: replace pseudorefs with real refs

[Johan Herland]

On Wed, Jul 29, 2015 at 7:01 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Johan Herland <johan@herland.net> writes:
>
>> I believe it is a bad compromise. It complicates the code, and it
>> provides a concurrent notes merges that is unnecessarily tied to (and
>> dependent on) worktrees. For example, if I wanted to perform N
>> concurrent notes merges in a project that happens to have a huge
>> worktree, I would now have to create N copies of the huge worktree...
>
> Who said worktree has to be populated?  You should be able to have
> an absolutely empty checkout just like "clone --no-checkout".

IMHO that's an insane workaround that only serves to highlight the
conceptual problems of binding notes merges (as they are implemented
today) to worktrees.

I'm much more excited about Michael's idea of formalising the concept
of a notes tree checkout (although as already discussed, checking out
a notes tree is different from checking out a "regular" tree). Then, a
notes merge (as you already suggested) should be implemented by
creating a linked worktree whose HEAD is the notes ref being merged
into. I believe there are potential complications here as well (where
a notes-merge might behave differently from a regular merge), but
those should be solvable.

But, whatever. This is unrelated to David's current effort, and I
don't want to hold that up, so please move along, nothing to see here.


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCH v3 2/6] notes: replace pseudorefs with real refs

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> On Wed, Jul 29, 2015 at 7:01 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Johan Herland <johan@herland.net> writes:
>>
>>> I believe it is a bad compromise. It complicates the code, and it
>>> provides a concurrent notes merges that is unnecessarily tied to (and
>>> dependent on) worktrees. For example, if I wanted to perform N
>>> concurrent notes merges in a project that happens to have a huge
>>> worktree, I would now have to create N copies of the huge worktree...
>>
>> Who said worktree has to be populated?  You should be able to have
>> an absolutely empty checkout just like "clone --no-checkout".
>
> IMHO that's an insane workaround that only serves to highlight the
> conceptual problems of binding notes merges (as they are implemented
> today) to worktrees.

Actually, the name "linked worktree" is probably a misnomer.

There is nothing fundamental in the mechanism or in the concept that
says that these multiple $GIT_DIR's must not be a bare one.  The
main thing the separation between $GIT_DIR and $GIT_COMMON_DIR
affords you is that you can have some things shared across them
(e.g. refs/*, objects) while making others per $GIT_DIR (e.g. HEAD,
index, etc.).

With that in mind, it is not an insane workaround but a very natural
mechanism suited exactly for what you want to do: using a feature
(e.g. "notes merge") that currently can have at most one instance
running at a time because it stores its state inside $GIT_DIR, and
you want to have N concurrent one going.  You keep that "state per
running instance" inside $GIT_DIR (i.e. not shared) and use the
"linked worktree" mechanism to have multiple $GIT_DIR, connected
to the same $GIT_COMMON_DIR.

> But, whatever. This is unrelated to David's current effort, and I
> don't want to hold that up, so please move along, nothing to see here.

I need this part from an earlier message answered to unblock David's
topic:

    Now we are getting somewhere.  So is there more that is needed
    than separating NOTES_MERGE_REF per worktree to make this work
    (remember, multiple notes-merge in a single worktree is a
    non-goal, just like multiple merge in a single worktree is not
    supported today and will not be)?  Is there some other state
    that is not captured by NOTES_MERGE_REF and friends that you
    would end up recording a wrong merge result, if two worktrees
    that have NOTES_MERGE_REF pointing at a different ref in
    refs/notes/* try to do the notes-merge at the same time?

If we do not change anything (not even applying the [v3 2/6] patch
we are discussing), all these things prefixed with NOTES_ will
become per $GIT_DIR with linked worktrees.

    NOTES_EDITMSG, NOTES_MERGE_REF, NOTES_MERGE_PARTIAL,
    NOTES_MERGE_WORKTREE

The user could attempt to start different notes merges in her
multiple $GIT_DIRs.  The question is to what degree we want to
support her.

Is it sufficient to have these per $GIT_DIR, when the user has two
$GIT_DIRs connected to the same repository and wants to do two
"notes merge" acting on different ref in refs/notes/*?  Or are there
some other states in the shared part kept, which would be stomped on
by simultaneously running "notes merge" instances in different
$GIT_DIRs, that make this not to work?  Any other problems in the
remainder of the current implementation of "notes merge"?

If there are reasons/limitations that make simultaneous "notes
merge" of different notes in different $GIT_DIRs impossible, then I
agree we shouldn't bother with [v3 2/6] patch.  We should just
declare "do not do it, it does not (yet) work".

But if there isn't, [v3 2/6] is the absolute minimum thing we could
do to make "notes merge" usable by making sure that the user does
not attempt merging the same refs/notes/commits in two different
places.

Re: [PATCH v3 2/6] notes: replace pseudorefs with real refs

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> If we do not change anything (not even applying the [v3 2/6] patch
> we are discussing),...

This one and...

> If there are reasons/limitations that make simultaneous "notes
> merge" of different notes in different $GIT_DIRs impossible, then I
> agree we shouldn't bother with [v3 2/6] patch.  We should just

... this one and ...

> declare "do not do it, it does not (yet) work".
>
> But if there isn't, [v3 2/6] is the absolute minimum thing we could

... this one.  All of these reference to [v3 2/6] are wrong.

The patch I meant was "[PATCH] notes: handle multiple worktrees",
http://thread.gmane.org/gmane.comp.version-control.git/274803/focus=274852

Sorry for the confusion.


> do to make "notes merge" usable by making sure that the user does
> not attempt merging the same refs/notes/commits in two different
> places.

Re: [PATCH v3 2/6] notes: replace pseudorefs with real refs

[Johan Herland]

On Wed, Jul 29, 2015 at 6:37 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Johan Herland <johan@herland.net> writes:
>> On Wed, Jul 29, 2015 at 7:01 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Johan Herland <johan@herland.net> writes:
>>>
>>>> I believe it is a bad compromise. It complicates the code, and it
>>>> provides a concurrent notes merges that is unnecessarily tied to (and
>>>> dependent on) worktrees. For example, if I wanted to perform N
>>>> concurrent notes merges in a project that happens to have a huge
>>>> worktree, I would now have to create N copies of the huge worktree...
>>>
>>> Who said worktree has to be populated?  You should be able to have
>>> an absolutely empty checkout just like "clone --no-checkout".
>>
>> IMHO that's an insane workaround that only serves to highlight the
>> conceptual problems of binding notes merges (as they are implemented
>> today) to worktrees.
>
> Actually, the name "linked worktree" is probably a misnomer.
>
> There is nothing fundamental in the mechanism or in the concept that
> says that these multiple $GIT_DIR's must not be a bare one.  The
> main thing the separation between $GIT_DIR and $GIT_COMMON_DIR
> affords you is that you can have some things shared across them
> (e.g. refs/*, objects) while making others per $GIT_DIR (e.g. HEAD,
> index, etc.).
>
> With that in mind, it is not an insane workaround but a very natural
> mechanism suited exactly for what you want to do: using a feature
> (e.g. "notes merge") that currently can have at most one instance
> running at a time because it stores its state inside $GIT_DIR, and
> you want to have N concurrent one going.  You keep that "state per
> running instance" inside $GIT_DIR (i.e. not shared) and use the
> "linked worktree" mechanism to have multiple $GIT_DIR, connected
> to the same $GIT_COMMON_DIR.

Makes sense, although currently, IINM, those multiple $GIT_DIRs must
be associated with strictly different branches, which is completely
unrelated to the desired notes-merge restriction (which applies to
notes refs - not branches). But this has been discussed to death,
already.

>> But, whatever. This is unrelated to David's current effort, and I
>> don't want to hold that up, so please move along, nothing to see here.
>
> I need this part from an earlier message answered to unblock David's
> topic:
>
>     Now we are getting somewhere.  So is there more that is needed
>     than separating NOTES_MERGE_REF per worktree to make this work
>     (remember, multiple notes-merge in a single worktree is a
>     non-goal, just like multiple merge in a single worktree is not
>     supported today and will not be)?  Is there some other state
>     that is not captured by NOTES_MERGE_REF and friends that you
>     would end up recording a wrong merge result, if two worktrees
>     that have NOTES_MERGE_REF pointing at a different ref in
>     refs/notes/* try to do the notes-merge at the same time?

I believe the answer to both questions is "No".

> If we do not change anything (not even applying the "[PATCH] notes: handle multiple worktrees" patch
> we are discussing), all these things prefixed with NOTES_ will
> become per $GIT_DIR with linked worktrees.
>
>     NOTES_EDITMSG, NOTES_MERGE_REF, NOTES_MERGE_PARTIAL,
>     NOTES_MERGE_WORKTREE
>
> The user could attempt to start different notes merges in her
> multiple $GIT_DIRs.  The question is to what degree we want to
> support her.
>
> Is it sufficient to have these per $GIT_DIR, when the user has two
> $GIT_DIRs connected to the same repository and wants to do two
> "notes merge" acting on different ref in refs/notes/*?  Or are there
> some other states in the shared part kept, which would be stomped on
> by simultaneously running "notes merge" instances in different
> $GIT_DIRs, that make this not to work?  Any other problems in the
> remainder of the current implementation of "notes merge"?

Still, I believe the answer is "No".

> If there are reasons/limitations that make simultaneous "notes
> merge" of different notes in different $GIT_DIRs impossible, then I
> agree we shouldn't bother with "[PATCH] notes: handle multiple worktrees" patch.  We should just
> declare "do not do it, it does not (yet) work".
>
> But if there isn't, "[PATCH] notes: handle multiple worktrees" is the absolute minimum thing we could
> do to make "notes merge" usable by making sure that the user does
> not attempt merging the same refs/notes/commits in two different
> places.

Sure. There's no point in delaying a patch that works well in practice
just because I have a delusion of a theoretically cleaner solution
that won't make any difference in practice.


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCH v3 2/6] notes: replace pseudorefs with real refs

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

>> Actually, the name "linked worktree" is probably a misnomer.
>> ...
> Makes sense, although currently, IINM, those multiple $GIT_DIRs must
> be associated with strictly different branches, which is completely
> unrelated to the desired notes-merge restriction (which applies to
> notes refs - not branches). But this has been discussed to death,
> already.

Yeah, when we enhance "linked worktree" to make it easier to create
"bare worktree", we'd need to make sure it is easy to make them in
the detached HEAD state, i.e. not tied to any particular branch.

>> The user could attempt to start different notes merges in her
>> multiple $GIT_DIRs.  The question is to what degree we want to
>> support her.
>>
>> Is it sufficient to have these per $GIT_DIR, when the user has two
>> $GIT_DIRs connected to the same repository and wants to do two
>> "notes merge" acting on different ref in refs/notes/*?  Or are there
>> some other states in the shared part kept, which would be stomped on
>> by simultaneously running "notes merge" instances in different
>> $GIT_DIRs, that make this not to work?  Any other problems in the
>> remainder of the current implementation of "notes merge"?
>
> Still, I believe the answer is "No".

Good.  So the following would be a viable way forward.

>> But if there isn't, "[PATCH] notes: handle multiple worktrees" is
>> the absolute minimum thing we could
>> do to make "notes merge" usable by making sure that the user does
>> not attempt merging the same refs/notes/commits in two different
>> places.
>
> Sure. There's no point in delaying a patch that works well in practice
> just because I have a delusion of a theoretically cleaner solution
> that won't make any difference in practice.

I think the part of that patch that refactors the mechanism to
notice and disallow symrefs pointing at the same thing (with various
tweaks suggested during the review) is a good idea in the longer
term.  If we were to switch to a "you can do multiple notes merges
inside a single repository without any additional linked worktree"
paradigm in the future, we would not want (or need) to use that
"avoid pointing this symref to the same thing" code for notes merge
codepath, but that is not something we need to decide now.

Re: [PATCH v3 2/6] notes: replace pseudorefs with real refs

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> In fact, you can easily do a notes merge in a _bare_ repo...

You keep repeating that but I do not think it is relevant at all.

If you have a bare repository, you either

 (1) do not have any worktree associated with it; or

 (2) have worktrees associated with it elsewhere, but its parent
     directory is *not* the root of any worktree.

If (1), what are found outside GIT_COMMON_DIR (e.g. HEAD) is found
inside GIT_DIR, so if we leave NOTES_MERGE_REF and friends outside
GIT_COMMON_DIR and create the NOTES_MERGE_WORKTREE in GIT_DIR, that
would work just as fine as it currently does.

If (2), what are found outside GIT_COMMON_DIR (e.g. HEAD) is still
found inside GIT_DIR (that is now per worktree, but for your bare
repository, that is the repository directory itself).  Again, if we
leave NOTES_MERGE_REF and friends outside GIT_COMMON_DIR and create
the NOTES_MERGE_WORKTREE in GIT_DIR, that would work just as fine as
it currently does.

And as long as NOTES_MERGE_REF is made per $GIT_DIR ("per worktree"
is the phrase I am refraining deliberately here, because all the
worktrees have their own private area, and in addition, your bare
repository has one, too) that is protected against "multiple
checkout", all worktrees and your bare repository can perform
independent notes-merges.

Perhaps you meant by "per repo" to mean "per $GIT_DIR" in this
message, and if that is the case, then I think we are in agreement.

Re: [PATCH v3 2/6] notes: replace pseudorefs with real refs

[Johan Herland]

On Wed, Jul 29, 2015 at 4:17 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Perhaps you meant by "per repo" to mean "per $GIT_DIR" in this
> message, and if that is the case, then I think we are in agreement.

No, by per repo I mean per $GIT_COMMON_DIR (I haven't followed the
linked worktree effort, so this language is new to me. My apologies).

...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

[PATCH v3 3/6] refs: add ref_type function

[David Turner]

Add a function ref_type, which categorizes refs as per-worktree,
pseudoref, or normal ref.

Later, we will use this in refs.c to treat pseudorefs specially.
Alternate ref backends may use it to treat both pseudorefs and
per-worktree refs differently.

Signed-off-by: David Turner <dturner@twopensource.com>
---
 refs.c | 29 +++++++++++++++++++++++++++++
 refs.h |  8 ++++++++
 2 files changed, 37 insertions(+)

diff --git a/refs.c b/refs.c
index 0b96ece..553ae8b 100644
--- a/refs.c
+++ b/refs.c
@@ -2848,6 +2848,35 @@ static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf *err)
 	return 0;
 }
 
+static int is_per_worktree_ref(const char *refname)
+{
+	return !strcmp(refname, "HEAD");
+}
+
+static int is_pseudoref_syntax(const char *refname)
+{
+	const char *c;
+
+	if (strchr(refname, '/'))
+		return 0;
+
+	for (c = refname; *c; ++c) {
+		if (!isupper(*c) && *c != '-' && *c != '_')
+			return 0;
+	}
+
+	return 1;
+}
+
+enum ref_type ref_type(const char *refname)
+{
+	if (is_per_worktree_ref(refname))
+		return REF_TYPE_PER_WORKTREE;
+	if (is_pseudoref_syntax(refname))
+		return REF_TYPE_PSEUDOREF;
+       return REF_TYPE_NORMAL;
+}
+
 int delete_ref(const char *refname, const unsigned char *old_sha1,
 	       unsigned int flags)
 {
diff --git a/refs.h b/refs.h
index e4e46c3..dca4fb5 100644
--- a/refs.h
+++ b/refs.h
@@ -445,6 +445,14 @@ extern int parse_hide_refs_config(const char *var, const char *value, const char
 
 extern int ref_is_hidden(const char *);
 
+enum ref_type {
+	REF_TYPE_PER_WORKTREE,
+	REF_TYPE_PSEUDOREF,
+	REF_TYPE_NORMAL,
+};
+
+enum ref_type ref_type(const char *refname);
+
 enum expire_reflog_flags {
 	EXPIRE_REFLOGS_DRY_RUN = 1 << 0,
 	EXPIRE_REFLOGS_UPDATE_REF = 1 << 1,
-- 
2.0.4.315.gad8727a-twtrsrc

Re: [PATCH v3 3/6] refs: add ref_type function

[Eric Sunshine]

On Tue, Jul 28, 2015 at 2:12 PM, David Turner <dturner@twopensource.com> wrote:
> Add a function ref_type, which categorizes refs as per-worktree,
> pseudoref, or normal ref.
>
> Later, we will use this in refs.c to treat pseudorefs specially.
> Alternate ref backends may use it to treat both pseudorefs and
> per-worktree refs differently.
>
> Signed-off-by: David Turner <dturner@twopensource.com>
> ---
> diff --git a/refs.c b/refs.c
> index 0b96ece..553ae8b 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2848,6 +2848,35 @@ static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf *err)
>         return 0;
>  }
>
> +static int is_per_worktree_ref(const char *refname)
> +{
> +       return !strcmp(refname, "HEAD");
> +}
> +
> +static int is_pseudoref_syntax(const char *refname)
> +{
> +       const char *c;
> +
> +       if (strchr(refname, '/'))
> +               return 0;

Isn't this redundant? Won't the below for-loop already return 0 if it
encounters a slash?

> +       for (c = refname; *c; ++c) {

Style: c++

> +               if (!isupper(*c) && *c != '-' && *c != '_')
> +                       return 0;
> +       }
> +
> +       return 1;
> +}
> +
> +enum ref_type ref_type(const char *refname)
> +{
> +       if (is_per_worktree_ref(refname))
> +               return REF_TYPE_PER_WORKTREE;
> +       if (is_pseudoref_syntax(refname))
> +               return REF_TYPE_PSEUDOREF;
> +       return REF_TYPE_NORMAL;
> +}

[PATCH v3 4/6] pseudorefs: create and use pseudoref update and delete functions

[David Turner]

Pseudorefs should not be updated through the ref transaction
API, because alternate ref backends still need to store pseudorefs
in GIT_DIR (instead of wherever they store refs).  Instead,
change update_ref and delete_ref to call pseudoref-specific
functions.

Signed-off-by: David Turner <dturner@twopensource.com>
---
 refs.c | 100 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 92 insertions(+), 8 deletions(-)

diff --git a/refs.c b/refs.c
index 553ae8b..2bd6aa6 100644
--- a/refs.c
+++ b/refs.c
@@ -2877,12 +2877,87 @@ enum ref_type ref_type(const char *refname)
        return REF_TYPE_NORMAL;
 }
 
+static int write_pseudoref(const char *pseudoref, const unsigned char *sha1,
+			   const unsigned char *old_sha1, struct strbuf *err)
+{
+	const char *filename;
+	int fd;
+	static struct lock_file lock;
+	struct strbuf buf = STRBUF_INIT;
+	int ret = -1;
+
+	strbuf_addf(&buf, "%s\n", sha1_to_hex(sha1));
+
+	filename = git_path("%s", pseudoref);
+	fd = hold_lock_file_for_update(&lock, filename, LOCK_DIE_ON_ERROR);
+	if (fd < 0) {
+		strbuf_addf(err, "Could not open '%s' for writing: %s",
+			    filename, strerror(errno));
+		return -1;
+	}
+
+	if (old_sha1) {
+		unsigned char actual_old_sha1[20];
+		read_ref(pseudoref, actual_old_sha1);
+		if (hashcmp(actual_old_sha1, old_sha1)) {
+			strbuf_addf(err, "Unexpected sha1 when writing %s", pseudoref);
+			rollback_lock_file(&lock);
+			goto done;
+		}
+	}
+
+	if (write_in_full(fd, buf.buf, buf.len) != buf.len) {
+		strbuf_addf(err, "Could not write to '%s'", filename);
+		rollback_lock_file(&lock);
+		goto done;
+	}
+
+	commit_lock_file(&lock);
+	ret = 0;
+done:
+	strbuf_release(&buf);
+	return ret;
+}
+
+static int delete_pseudoref(const char *pseudoref, const unsigned char *old_sha1)
+{
+	static struct lock_file lock;
+	const char *filename;
+
+	filename = git_path("%s", pseudoref);
+
+	if (old_sha1 && !is_null_sha1(old_sha1)) {
+		int fd;
+		unsigned char actual_old_sha1[20];
+
+		fd = hold_lock_file_for_update(&lock, filename,
+					       LOCK_DIE_ON_ERROR);
+		if (fd < 0)
+			die_errno(_("Could not open '%s' for writing"), filename);
+		read_ref(pseudoref, actual_old_sha1);
+		if (hashcmp(actual_old_sha1, old_sha1)) {
+			warning("Unexpected sha1 when deleting %s", pseudoref);
+			return -1;
+		}
+
+		unlink(filename);
+		rollback_lock_file(&lock);
+	} else {
+		unlink(filename);
+	}
+
+	return 0;
+}
+
 int delete_ref(const char *refname, const unsigned char *old_sha1,
 	       unsigned int flags)
 {
 	struct ref_transaction *transaction;
 	struct strbuf err = STRBUF_INIT;
 
+	if (ref_type(refname) == REF_TYPE_PSEUDOREF)
+		return delete_pseudoref(refname, old_sha1);
+
 	transaction = ref_transaction_begin(&err);
 	if (!transaction ||
 	    ref_transaction_delete(transaction, refname, old_sha1,
@@ -3976,17 +4051,25 @@ int update_ref(const char *msg, const char *refname,
 	       const unsigned char *new_sha1, const unsigned char *old_sha1,
 	       unsigned int flags, enum action_on_err onerr)
 {
-	struct ref_transaction *t;
+	struct ref_transaction *t = NULL;
 	struct strbuf err = STRBUF_INIT;
+	int ret = 0;
 
-	t = ref_transaction_begin(&err);
-	if (!t ||
-	    ref_transaction_update(t, refname, new_sha1, old_sha1,
-				   flags, msg, &err) ||
-	    ref_transaction_commit(t, &err)) {
+	if (ref_type(refname) == REF_TYPE_PSEUDOREF) {
+		ret = write_pseudoref(refname, new_sha1, old_sha1, &err);
+	} else {
+		t = ref_transaction_begin(&err);
+		if (!t ||
+		    ref_transaction_update(t, refname, new_sha1, old_sha1,
+					   flags, msg, &err) ||
+		    ref_transaction_commit(t, &err)) {
+			ret = 1;
+			ref_transaction_free(t);
+		}
+	}
+	if (ret) {
 		const char *str = "update_ref failed for ref '%s': %s";
 
-		ref_transaction_free(t);
 		switch (onerr) {
 		case UPDATE_REFS_MSG_ON_ERR:
 			error(str, refname, err.buf);
@@ -4001,7 +4084,8 @@ int update_ref(const char *msg, const char *refname,
 		return 1;
 	}
 	strbuf_release(&err);
-	ref_transaction_free(t);
+	if (t)
+		ref_transaction_free(t);
 	return 0;
 }
 
-- 
2.0.4.315.gad8727a-twtrsrc

Re: [PATCH v3 4/6] pseudorefs: create and use pseudoref update and delete functions

[Eric Sunshine]

On Tue, Jul 28, 2015 at 2:12 PM, David Turner <dturner@twopensource.com> wrote:
> Pseudorefs should not be updated through the ref transaction
> API, because alternate ref backends still need to store pseudorefs
> in GIT_DIR (instead of wherever they store refs).  Instead,
> change update_ref and delete_ref to call pseudoref-specific
> functions.
>
> Signed-off-by: David Turner <dturner@twopensource.com>
> ---
> diff --git a/refs.c b/refs.c
> index 553ae8b..2bd6aa6 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2877,12 +2877,87 @@ enum ref_type ref_type(const char *refname)
> +static int delete_pseudoref(const char *pseudoref, const unsigned char *old_sha1)
> +{
> +       static struct lock_file lock;
> +       const char *filename;
> +
> +       filename = git_path("%s", pseudoref);
> +
> +       if (old_sha1 && !is_null_sha1(old_sha1)) {
> +               int fd;
> +               unsigned char actual_old_sha1[20];
> +
> +               fd = hold_lock_file_for_update(&lock, filename,
> +                                              LOCK_DIE_ON_ERROR);
> +               if (fd < 0)
> +                       die_errno(_("Could not open '%s' for writing"), filename);
> +               read_ref(pseudoref, actual_old_sha1);
> +               if (hashcmp(actual_old_sha1, old_sha1)) {
> +                       warning("Unexpected sha1 when deleting %s", pseudoref);
> +                       return -1;

Does this need to release the lock file before returning?

> +               }
> +
> +               unlink(filename);
> +               rollback_lock_file(&lock);
> +       } else {
> +               unlink(filename);
> +       }
> +
> +       return 0;
> +}

[PATCH v3 5/6] rebase: use update_ref

[David Turner]

Instead of manually writing a pseudoref (in one case) and shelling out
to git update-ref (in another), use the update_ref function.  This
is much simpler.

Signed-off-by: David Turner <dturner@twopensource.com>
---
 bisect.c | 37 ++++++++-----------------------------
 1 file changed, 8 insertions(+), 29 deletions(-)

diff --git a/bisect.c b/bisect.c
index 857cf59..33ac88d 100644
--- a/bisect.c
+++ b/bisect.c
@@ -19,7 +19,6 @@ static struct object_id *current_bad_oid;
 
 static const char *argv_checkout[] = {"checkout", "-q", NULL, "--", NULL};
 static const char *argv_show_branch[] = {"show-branch", NULL, NULL};
-static const char *argv_update_ref[] = {"update-ref", "--no-deref", "BISECT_HEAD", NULL, NULL};
 
 static const char *term_bad;
 static const char *term_good;
@@ -675,34 +674,16 @@ static int is_expected_rev(const struct object_id *oid)
 	return res;
 }
 
-static void mark_expected_rev(char *bisect_rev_hex)
-{
-	int len = strlen(bisect_rev_hex);
-	const char *filename = git_path("BISECT_EXPECTED_REV");
-	int fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600);
-
-	if (fd < 0)
-		die_errno("could not create file '%s'", filename);
-
-	bisect_rev_hex[len] = '\n';
-	write_or_die(fd, bisect_rev_hex, len + 1);
-	bisect_rev_hex[len] = '\0';
-
-	if (close(fd) < 0)
-		die("closing file %s: %s", filename, strerror(errno));
-}
-
-static int bisect_checkout(char *bisect_rev_hex, int no_checkout)
+static int bisect_checkout(const unsigned char *bisect_rev, int no_checkout)
 {
+	char bisect_rev_hex[GIT_SHA1_HEXSZ + 1];
 
-	mark_expected_rev(bisect_rev_hex);
+	memcpy(bisect_rev_hex, sha1_to_hex(bisect_rev), GIT_SHA1_HEXSZ + 1);
+	update_ref(NULL, "BISECT_EXPECTED_REV", bisect_rev, NULL, 0, UPDATE_REFS_DIE_ON_ERR);
 
 	argv_checkout[2] = bisect_rev_hex;
 	if (no_checkout) {
-		argv_update_ref[3] = bisect_rev_hex;
-		if (run_command_v_opt(argv_update_ref, RUN_GIT_CMD))
-			die("update-ref --no-deref HEAD failed on %s",
-			    bisect_rev_hex);
+		update_ref(NULL, "BISECT_HEAD", bisect_rev, NULL, 0, UPDATE_REFS_DIE_ON_ERR);
 	} else {
 		int res;
 		res = run_command_v_opt(argv_checkout, RUN_GIT_CMD);
@@ -804,7 +785,7 @@ static void check_merge_bases(int no_checkout)
 			handle_skipped_merge_base(mb);
 		} else {
 			printf("Bisecting: a merge base must be tested\n");
-			exit(bisect_checkout(sha1_to_hex(mb), no_checkout));
+			exit(bisect_checkout(mb, no_checkout));
 		}
 	}
 
@@ -948,7 +929,6 @@ int bisect_next_all(const char *prefix, int no_checkout)
 	struct commit_list *tried;
 	int reaches = 0, all = 0, nr, steps;
 	const unsigned char *bisect_rev;
-	char bisect_rev_hex[GIT_SHA1_HEXSZ + 1];
 
 	read_bisect_terms(&term_bad, &term_good);
 	if (read_bisect_refs())
@@ -986,11 +966,10 @@ int bisect_next_all(const char *prefix, int no_checkout)
 	}
 
 	bisect_rev = revs.commits->item->object.sha1;
-	memcpy(bisect_rev_hex, sha1_to_hex(bisect_rev), GIT_SHA1_HEXSZ + 1);
 
 	if (!hashcmp(bisect_rev, current_bad_oid->hash)) {
 		exit_if_skipped_commits(tried, current_bad_oid);
-		printf("%s is the first %s commit\n", bisect_rev_hex,
+		printf("%s is the first %s commit\n", sha1_to_hex(bisect_rev),
 			term_bad);
 		show_diff_tree(prefix, revs.commits->item);
 		/* This means the bisection process succeeded. */
@@ -1003,7 +982,7 @@ int bisect_next_all(const char *prefix, int no_checkout)
 	       "(roughly %d step%s)\n", nr, (nr == 1 ? "" : "s"),
 	       steps, (steps == 1 ? "" : "s"));
 
-	return bisect_checkout(bisect_rev_hex, no_checkout);
+	return bisect_checkout(bisect_rev, no_checkout);
 }
 
 static inline int log2i(int n)
-- 
2.0.4.315.gad8727a-twtrsrc

[PATCH v3 6/6] sequencer: replace write_cherry_pick_head with update_ref

[David Turner]

Now update_ref (via write_pseudoref) does almost exactly what
write_cherry_pick_head did, so we can remove write_cherry_pick_head
and just use update_ref.

Signed-off-by: David Turner <dturner@twopensource.com>
---
 sequencer.c | 23 ++++-------------------
 1 file changed, 4 insertions(+), 19 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index c4f4b7d..554a704 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -158,23 +158,6 @@ static void free_message(struct commit *commit, struct commit_message *msg)
 	unuse_commit_buffer(commit, msg->message);
 }
 
-static void write_cherry_pick_head(struct commit *commit, const char *pseudoref)
-{
-	const char *filename;
-	int fd;
-	struct strbuf buf = STRBUF_INIT;
-
-	strbuf_addf(&buf, "%s\n", sha1_to_hex(commit->object.sha1));
-
-	filename = git_path("%s", pseudoref);
-	fd = open(filename, O_WRONLY | O_CREAT, 0666);
-	if (fd < 0)
-		die_errno(_("Could not open '%s' for writing"), filename);
-	if (write_in_full(fd, buf.buf, buf.len) != buf.len || close(fd))
-		die_errno(_("Could not write to '%s'"), filename);
-	strbuf_release(&buf);
-}
-
 static void print_advice(int show_hint, struct replay_opts *opts)
 {
 	char *msg = getenv("GIT_CHERRY_PICK_HELP");
@@ -607,9 +590,11 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 	 * write it at all.
 	 */
 	if (opts->action == REPLAY_PICK && !opts->no_commit && (res == 0 || res == 1))
-		write_cherry_pick_head(commit, "CHERRY_PICK_HEAD");
+		update_ref(NULL, "CHERRY_PICK_HEAD", commit->object.sha1, NULL,
+			   REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);
 	if (opts->action == REPLAY_REVERT && ((opts->no_commit && res == 0) || res == 1))
-		write_cherry_pick_head(commit, "REVERT_HEAD");
+		update_ref(NULL, "REVERT_HEAD", commit->object.sha1, NULL,
+			   REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);
 
 	if (res) {
 		error(opts->action == REPLAY_REVERT
-- 
2.0.4.315.gad8727a-twtrsrc

Re: [PATCH v3 0/6] pseudorefs

[Junio C Hamano]

On top of what work is this series expected to be applied?

Re: [PATCH v3 0/6] pseudorefs

[David Turner]

On Tue, 2015-07-28 at 12:01 -0700, Junio C Hamano wrote:
> On top of what work is this series expected to be applied?

I think I started from 'next' as of a few days ago:

commit df7aaa5e3454bbcbb1f142dd6b95b214d0b8efad
Author: Zoë Blade <zoe@bytenoise.co.uk>
Date:   Tue Jul 21 14:22:46 2015 +0100

    userdiff: add support for Fountain documents

Let me know if you need me to rebase on something else.