[PATCH/RFC 0/2] bisect per-worktree

[PATCH/RFC 0/2] bisect per-worktree

[David Turner]

This is RFC because I'm not sure why show-ref only works on refs/ (and
whether it should learn to look in worktree-refs/).  I'm also not sure
whether there are other changes I should make to refs.c to handle
per-worktree refs; I basically did the simplest thing I could think of
to start with.

[PATCH 1/2] refs: workree-refs/* become per-worktree

[David Turner]

We need a place to stick refs for bisects in progress that is not
shared between worktrees.  So we use the worktree-refs/ hierarchy
instead of the refs/.

To do this, load loose refs from "worktree-refs/" as well as from "refs/".
The is_per_worktree_ref function and associated docs learn that
worktree-refs/ is per-worktree.

The ref-packing functions learn that refs beginning with
worktree-refs/ should not be packed (since packed-refs is common
rather than per-worktree).

Signed-off-by: David Turner <dturner@twopensource.com>
---
 Documentation/glossary-content.txt |  3 ++-
 refs.c                             | 13 ++++++++++---
 t/t3210-pack-refs.sh               |  7 +++++++
 3 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt
index 8c6478b..e2847a9 100644
--- a/Documentation/glossary-content.txt
+++ b/Documentation/glossary-content.txt
@@ -413,7 +413,8 @@ exclude;;
 
 [[def_per_worktree_ref]]per-worktree ref::
 	Refs that are per-<<def_working_tree,worktree>>, rather than
-	global.  This is presently only <<def_HEAD,HEAD>>, but might
+	global.  This is presently only <<def_HEAD,HEAD>> and any refs
+	that start with `worktree-refs/` (rather than `refs/`), but might
 	later include other unusual refs.
 
 [[def_pseudoref]]pseudoref::
diff --git a/refs.c b/refs.c
index e6fc3fe..c556b6f 100644
--- a/refs.c
+++ b/refs.c
@@ -1433,15 +1433,17 @@ static struct ref_dir *get_loose_refs(struct ref_cache *refs)
 	if (!refs->loose) {
 		/*
 		 * Mark the top-level directory complete because we
-		 * are about to read the only subdirectory that can
+		 * are about to read the only subdirectories that can
 		 * hold references:
 		 */
 		refs->loose = create_dir_entry(refs, "", 0, 0);
 		/*
-		 * Create an incomplete entry for "refs/":
+		 * Create incomplete entries for "refs/" and "worktree-refs":
 		 */
 		add_entry_to_dir(get_ref_dir(refs->loose),
 				 create_dir_entry(refs, "refs/", 5, 1));
+		add_entry_to_dir(get_ref_dir(refs->loose),
+				 create_dir_entry(refs, "worktree-refs/", 14, 1));
 	}
 	return get_ref_dir(refs->loose);
 }
@@ -2656,6 +2658,10 @@ static int pack_if_possible_fn(struct ref_entry *entry, void *cb_data)
 	struct ref_entry *packed_entry;
 	int is_tag_ref = starts_with(entry->name, "refs/tags/");
 
+	/* Do not pack per-worktree refs: */
+	if (starts_with(entry->name, "worktree-refs/"))
+		return 0;
+
 	/* ALWAYS pack tags */
 	if (!(cb->flags & PACK_REFS_ALL) && !is_tag_ref)
 		return 0;
@@ -2850,7 +2856,8 @@ static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf *err)
 
 static int is_per_worktree_ref(const char *refname)
 {
-	return !strcmp(refname, "HEAD");
+	return !strcmp(refname, "HEAD") ||
+		starts_with(refname, "worktree-refs/");
 }
 
 static int is_pseudoref_syntax(const char *refname)
diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh
index 8aae98d..0800a1c 100755
--- a/t/t3210-pack-refs.sh
+++ b/t/t3210-pack-refs.sh
@@ -160,6 +160,13 @@ test_expect_success 'pack ref directly below refs/' '
 	test_path_is_missing .git/refs/top
 '
 
+test_expect_success 'do not pack ref in worktree-refs' '
+	git update-ref worktree-refs/local HEAD &&
+	git pack-refs --all --prune &&
+	! grep worktree-refs/local .git/packed-refs >/dev/null &&
+	test_path_is_file .git/worktree-refs/local
+'
+
 test_expect_success 'disable reflogs' '
 	git config core.logallrefupdates false &&
 	rm -rf .git/logs
-- 
2.0.4.315.gad8727a-twtrsrc

[PATCH 2/2] bisect: make bisection refs per-worktree

[David Turner]

Using the new worktree-refs/ refs, make bisection per-worktree.

git show-ref presently only handles refs under refs/, so we change
git bisect to use git rev-parse instead.

Signed-off-by: David Turner <dturner@twopensource.com>
---
 Documentation/git-bisect.txt       |  4 ++--
 Documentation/rev-list-options.txt | 14 +++++++-------
 bisect.c                           |  2 +-
 builtin/rev-parse.c                |  4 ++--
 git-bisect.sh                      | 14 +++++++-------
 revision.c                         |  2 +-
 t/t6030-bisect-porcelain.sh        | 20 ++++++++++----------
 7 files changed, 30 insertions(+), 30 deletions(-)

diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt
index e97f2de..d8753d0 100644
--- a/Documentation/git-bisect.txt
+++ b/Documentation/git-bisect.txt
@@ -82,7 +82,7 @@ to ask for the next commit that needs testing.
 
 Eventually there will be no more revisions left to inspect, and the
 command will print out a description of the first bad commit. The
-reference `refs/bisect/bad` will be left pointing at that commit.
+reference `worktree-refs/bisect/bad` will be left pointing at that commit.
 
 
 Bisect reset
@@ -373,7 +373,7 @@ on a single line.
 ------------
 $ git bisect start HEAD <known-good-commit> [ <boundary-commit> ... ] --no-checkout
 $ git bisect run sh -c '
-	GOOD=$(git for-each-ref "--format=%(objectname)" refs/bisect/good-*) &&
+	GOOD=$(git for-each-ref "--format=%(objectname)" worktree-refs/bisect/good-*) &&
 	git rev-list --objects BISECT_HEAD --not $GOOD >tmp.$$ &&
 	git pack-objects --stdout >/dev/null <tmp.$$
 	rc=$?
diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index a9b808f..b00b2fa 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -183,9 +183,9 @@ explicitly.
 
 ifndef::git-rev-list[]
 --bisect::
-	Pretend as if the bad bisection ref `refs/bisect/bad`
+	Pretend as if the bad bisection ref `worktree-refs/bisect/bad`
 	was listed and as if it was followed by `--not` and the good
-	bisection refs `refs/bisect/good-*` on the command
+	bisection refs `worktree-refs/bisect/good-*` on the command
 	line. Cannot be combined with --first-parent.
 endif::git-rev-list[]
 
@@ -548,10 +548,10 @@ Bisection Helpers
 --bisect::
 	Limit output to the one commit object which is roughly halfway between
 	included and excluded commits. Note that the bad bisection ref
-	`refs/bisect/bad` is added to the included commits (if it
-	exists) and the good bisection refs `refs/bisect/good-*` are
+	`worktree-refs/bisect/bad` is added to the included commits (if it
+	exists) and the good bisection refs `worktree-refs/bisect/good-*` are
 	added to the excluded commits (if they exist). Thus, supposing there
-	are no refs in `refs/bisect/`, if
+	are no refs in `worktree-refs/bisect/`, if
 +
 -----------------------------------------------------------------------
 	$ git rev-list --bisect foo ^bar ^baz
@@ -571,7 +571,7 @@ one. Cannot be combined with --first-parent.
 
 --bisect-vars::
 	This calculates the same as `--bisect`, except that refs in
-	`refs/bisect/` are not used, and except that this outputs
+	`worktree-refs/bisect/` are not used, and except that this outputs
 	text ready to be eval'ed by the shell. These lines will assign the
 	name of the midpoint revision to the variable `bisect_rev`, and the
 	expected number of commits to be tested after `bisect_rev` is tested
@@ -584,7 +584,7 @@ one. Cannot be combined with --first-parent.
 --bisect-all::
 	This outputs all the commit objects between the included and excluded
 	commits, ordered by their distance to the included and excluded
-	commits. Refs in `refs/bisect/` are not used. The farthest
+	commits. Refs in `worktree-refs/bisect/` are not used. The farthest
 	from them is displayed first. (This is the only one displayed by
 	`--bisect`.)
 +
diff --git a/bisect.c b/bisect.c
index 33ac88d..63bd5cf 100644
--- a/bisect.c
+++ b/bisect.c
@@ -425,7 +425,7 @@ static int register_ref(const char *refname, const struct object_id *oid,
 
 static int read_bisect_refs(void)
 {
-	return for_each_ref_in("refs/bisect/", register_ref, NULL);
+	return for_each_ref_in("worktree-refs/bisect/", register_ref, NULL);
 }
 
 static void read_bisect_paths(struct argv_array *array)
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 02d747d..ad56154 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -663,8 +663,8 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 				continue;
 			}
 			if (!strcmp(arg, "--bisect")) {
-				for_each_ref_in("refs/bisect/bad", show_reference, NULL);
-				for_each_ref_in("refs/bisect/good", anti_reference, NULL);
+				for_each_ref_in("worktree-refs/bisect/bad", show_reference, NULL);
+				for_each_ref_in("worktree-refs/bisect/good", anti_reference, NULL);
 				continue;
 			}
 			if (starts_with(arg, "--branches=")) {
diff --git a/git-bisect.sh b/git-bisect.sh
index ea63223..cef4a94 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -210,7 +210,7 @@ bisect_write() {
 		*)
 			die "$(eval_gettext "Bad bisect_write argument: \$state")" ;;
 	esac
-	git update-ref "refs/bisect/$tag" "$rev" || exit
+	git update-ref "worktree-refs/bisect/$tag" "$rev" || exit
 	echo "# $state: $(git show-branch $rev)" >>"$GIT_DIR/BISECT_LOG"
 	test -n "$nolog" || echo "git bisect $state $rev" >>"$GIT_DIR/BISECT_LOG"
 }
@@ -282,8 +282,8 @@ bisect_state() {
 
 bisect_next_check() {
 	missing_good= missing_bad=
-	git show-ref -q --verify refs/bisect/$TERM_BAD || missing_bad=t
-	test -n "$(git for-each-ref "refs/bisect/$TERM_GOOD-*")" || missing_good=t
+	git rev-parse -q --verify worktree-refs/bisect/$TERM_BAD >/dev/null || missing_bad=t
+	test -n "$(git for-each-ref "worktree-refs/bisect/$TERM_GOOD-*")" || missing_good=t
 
 	case "$missing_good,$missing_bad,$1" in
 	,,*)
@@ -341,15 +341,15 @@ bisect_next() {
 	# Check if we should exit because bisection is finished
 	if test $res -eq 10
 	then
-		bad_rev=$(git show-ref --hash --verify refs/bisect/$TERM_BAD)
+		bad_rev=$(git rev-parse -q --verify worktree-refs/bisect/$TERM_BAD)
 		bad_commit=$(git show-branch $bad_rev)
 		echo "# first $TERM_BAD commit: $bad_commit" >>"$GIT_DIR/BISECT_LOG"
 		exit 0
 	elif test $res -eq 2
 	then
 		echo "# only skipped commits left to test" >>"$GIT_DIR/BISECT_LOG"
-		good_revs=$(git for-each-ref --format="%(objectname)" "refs/bisect/$TERM_GOOD-*")
-		for skipped in $(git rev-list refs/bisect/$TERM_BAD --not $good_revs)
+		good_revs=$(git for-each-ref --format="%(objectname)" "worktree-refs/bisect/$TERM_GOOD-*")
+		for skipped in $(git rev-list worktree-refs/bisect/$TERM_BAD --not $good_revs)
 		do
 			skipped_commit=$(git show-branch $skipped)
 			echo "# possible first $TERM_BAD commit: $skipped_commit" >>"$GIT_DIR/BISECT_LOG"
@@ -412,7 +412,7 @@ Try 'git bisect reset <commit>'.")"
 
 bisect_clean_state() {
 	# There may be some refs packed during bisection.
-	git for-each-ref --format='%(refname) %(objectname)' refs/bisect/\* |
+	git for-each-ref --format='%(refname) %(objectname)' worktree-refs/bisect/\* |
 	while read ref hash
 	do
 		git update-ref -d $ref $hash || exit
diff --git a/revision.c b/revision.c
index b6b2cf7..90c9c36 100644
--- a/revision.c
+++ b/revision.c
@@ -2084,7 +2084,7 @@ extern void read_bisect_terms(const char **bad, const char **good);
 static int for_each_bisect_ref(const char *submodule, each_ref_fn fn, void *cb_data, const char *term) {
 	struct strbuf bisect_refs = STRBUF_INIT;
 	int status;
-	strbuf_addf(&bisect_refs, "refs/bisect/%s", term);
+	strbuf_addf(&bisect_refs, "worktree-refs/bisect/%s", term);
 	status = for_each_ref_in_submodule(submodule, bisect_refs.buf, fn, cb_data);
 	strbuf_release(&bisect_refs);
 	return status;
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 9e2c203..b5dffd6 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -68,7 +68,7 @@ test_expect_success 'bisect fails if given any junk instead of revs' '
 	git bisect reset &&
 	test_must_fail git bisect start foo $HASH1 -- &&
 	test_must_fail git bisect start $HASH4 $HASH1 bar -- &&
-	test -z "$(git for-each-ref "refs/bisect/*")" &&
+	test -z "$(git for-each-ref "worktree-refs/bisect/*")" &&
 	test -z "$(ls .git/BISECT_* 2>/dev/null)" &&
 	git bisect start &&
 	test_must_fail git bisect good foo $HASH1 &&
@@ -77,7 +77,7 @@ test_expect_success 'bisect fails if given any junk instead of revs' '
 	test_must_fail git bisect bad $HASH3 $HASH4 &&
 	test_must_fail git bisect skip bar $HASH3 &&
 	test_must_fail git bisect skip $HASH1 foo &&
-	test -z "$(git for-each-ref "refs/bisect/*")" &&
+	test -z "$(git for-each-ref "worktree-refs/bisect/*")" &&
 	git bisect good $HASH1 &&
 	git bisect bad $HASH4
 '
@@ -115,7 +115,7 @@ test_expect_success 'bisect reset removes packed refs' '
 	git pack-refs --all --prune &&
 	git bisect next &&
 	git bisect reset &&
-	test -z "$(git for-each-ref "refs/bisect/*")" &&
+	test -z "$(git for-each-ref "worktree-refs/bisect/*")" &&
 	test -z "$(git for-each-ref "refs/heads/bisect")"
 '
 
@@ -126,7 +126,7 @@ test_expect_success 'bisect reset removes bisect state after --no-checkout' '
 	git bisect bad $HASH3 &&
 	git bisect next &&
 	git bisect reset &&
-	test -z "$(git for-each-ref "refs/bisect/*")" &&
+	test -z "$(git for-each-ref "worktree-refs/bisect/*")" &&
 	test -z "$(git for-each-ref "refs/heads/bisect")" &&
 	test -z "$(git for-each-ref "BISECT_HEAD")"
 '
@@ -176,7 +176,7 @@ test_expect_success 'bisect start: no ".git/BISECT_START" if checkout error' '
 	git branch > branch.output &&
 	grep "* other" branch.output > /dev/null &&
 	test_must_fail test -e .git/BISECT_START &&
-	test -z "$(git for-each-ref "refs/bisect/*")" &&
+	test -z "$(git for-each-ref "worktree-refs/bisect/*")" &&
 	git checkout HEAD hello
 '
 
@@ -671,7 +671,7 @@ test_expect_success 'bisect: --no-checkout - target before breakage' '
 	git bisect bad BISECT_HEAD &&
 	check_same BROKEN_HASH5 BISECT_HEAD &&
 	git bisect bad BISECT_HEAD &&
-	check_same BROKEN_HASH5 bisect/bad &&
+	check_same BROKEN_HASH5 worktree-refs/bisect/bad &&
 	git bisect reset
 '
 
@@ -682,7 +682,7 @@ test_expect_success 'bisect: --no-checkout - target in breakage' '
 	git bisect bad BISECT_HEAD &&
 	check_same BROKEN_HASH5 BISECT_HEAD &&
 	git bisect good BISECT_HEAD &&
-	check_same BROKEN_HASH6 bisect/bad &&
+	check_same BROKEN_HASH6 worktree-refs/bisect/bad &&
 	git bisect reset
 '
 
@@ -693,7 +693,7 @@ test_expect_success 'bisect: --no-checkout - target after breakage' '
 	git bisect good BISECT_HEAD &&
 	check_same BROKEN_HASH8 BISECT_HEAD &&
 	git bisect good BISECT_HEAD &&
-	check_same BROKEN_HASH9 bisect/bad &&
+	check_same BROKEN_HASH9 worktree-refs/bisect/bad &&
 	git bisect reset
 '
 
@@ -702,13 +702,13 @@ test_expect_success 'bisect: demonstrate identification of damage boundary' "
 	git checkout broken &&
 	git bisect start broken master --no-checkout &&
 	git bisect run \"\$SHELL_PATH\" -c '
-		GOOD=\$(git for-each-ref \"--format=%(objectname)\" refs/bisect/good-*) &&
+		GOOD=\$(git for-each-ref \"--format=%(objectname)\" worktree-refs/bisect/good-*) &&
 		git rev-list --objects BISECT_HEAD --not \$GOOD >tmp.\$\$ &&
 		git pack-objects --stdout >/dev/null < tmp.\$\$
 		rc=\$?
 		rm -f tmp.\$\$
 		test \$rc = 0' &&
-	check_same BROKEN_HASH6 bisect/bad &&
+	check_same BROKEN_HASH6 worktree-refs/bisect/bad &&
 	git bisect reset
 "
 
-- 
2.0.4.315.gad8727a-twtrsrc

Re: [PATCH/RFC 0/2] bisect per-worktree

[Michael Haggerty]

On 08/01/2015 01:56 AM, David Turner wrote:
> This is RFC because I'm not sure why show-ref only works on refs/ (and
> whether it should learn to look in worktree-refs/).  I'm also not sure
> whether there are other changes I should make to refs.c to handle
> per-worktree refs; I basically did the simplest thing I could think of
> to start with.

It seems to me that adding a new top-level "worktree-refs" directory is
pretty traumatic. Lots of people and tools will have made the assumption
that all "normal" references live under "refs/".

Each worktree has a name, right? What if worktree-specific refs were
stored under "refs/worktree/<name>/" or something? This would require
the name to be a valid reference name component, but I think that is a
prudent idea anyway.

Of course that doesn't answer the question: where should the refs for
the main repository go? I suppose either in the current place, or maybe
the main repository should have a name too (e.g. "main") and its
references should be stored under "refs/worktree/main/".

These worktree-specific refs should presumably be deleted automatically
when the worktree is deleted.

Either way, there's also the question of who should know how to find the
worktree-specific references--the program that wants to access them, or
should there be a secret invisible mapping that is done on lookup, and
that knows the full list of references that want to be worktree-specific
(for example, that in a linked worktree, "refs/bisect/*" should be
silently redirected to "refs/worktree/<name>/bisect/*")?

It's all a bit frightening, frankly.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH/RFC 0/2] bisect per-worktree

[Junio C Hamano]

On Fri, Jul 31, 2015 at 8:59 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>
> It seems to me that adding a new top-level "worktree-refs" directory is
> pretty traumatic. Lots of people and tools will have made the assumption
> that all "normal" references live under "refs/".
> ...
> It's all a bit frightening, frankly.

I actually feel the prospect of pluggable ref backend more frightening,
frankly ;-). These bisect refs are just like FETCH_HEAD and MERGE_HEAD,
not about the primary purpose of the "repository" to grow the history of refs
(branches), but about ephemeral pointers into the history used to help keep
track of what is being done in the worktree upstairs. There is no need for
these to be visible across worktrees. If we use the real refs that are grobal
in the repository (as opposed to per-worktree ones), we would hit the backend
databas with transactions to update these ephemeral things, which somehow
makes me feel stupid.

I wish we could just make refs/bisect/* (or whatever the current bisect
code uses) automatically per worktree. I know David dismissed it saying
that the current code is not set up to allow it easily, but is it
really a fundamental
limitation, or is it just a matter of coding a few more dozens of lines?

If we can keep using the same location under refs/ and transparently make
them appear per-worktree, "what is the name of the main one?", and "do we
even need to call the one and the only one 'main'?" will automatically
disappear.
Of course, "git bisect" and "gitk --bisect" does not have to change if
we go that
route.

And there will not be any backward compatibility worries. If you are not
using multiple worktrees, you will see them as refs/bisect/*, just at the
same location as you are familiar with.

Re: [PATCH/RFC 0/2] bisect per-worktree

[David Turner]

On Fri, 2015-07-31 at 22:12 -0700, Junio C Hamano wrote:
> On Fri, Jul 31, 2015 at 8:59 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> >
> > It seems to me that adding a new top-level "worktree-refs" directory is
> > pretty traumatic. Lots of people and tools will have made the assumption
> > that all "normal" references live under "refs/".
> > ...
> > It's all a bit frightening, frankly.
> 
> I actually feel the prospect of pluggable ref backend more frightening,
> frankly ;-). These bisect refs are just like FETCH_HEAD and MERGE_HEAD,
> not about the primary purpose of the "repository" to grow the history of refs
> (branches), but about ephemeral pointers into the history used to help keep
> track of what is being done in the worktree upstairs. There is no need for
> these to be visible across worktrees. If we use the real refs that are grobal
> in the repository (as opposed to per-worktree ones), we would hit the backend
> databas with transactions to update these ephemeral things, which somehow
> makes me feel stupid.

Agreed, I think it's a mistake to complicate the global ref namespace
like that.

> I wish we could just make refs/bisect/* (or whatever the current bisect
> code uses) automatically per worktree. I know David dismissed it saying
> that the current code is not set up to allow it easily, but is it
> really a fundamental
> limitation, or is it just a matter of coding a few more dozens of lines?

We still need the packed-refs and is_per_worktree_ref change; we need a
different and more complicated loose-refs loading change, and we need
changes to path.c to treat refs/bisect different from refs/.  Probably a
few dozen lines, yeah.  And it's sort of ugly to make the refs/bisect
special case into a fundamental part of the repository structure.  

I'm worried that we'll discover a need for more of these.  But I can do
the rewrite and see how it looks (probably on Monday).

Do we need to worry about reflogs for these?  If so, a few more lines,
probably.

> If we can keep using the same location under refs/ and transparently make
> them appear per-worktree, "what is the name of the main one?", and "do we
> even need to call the one and the only one 'main'?" will automatically
> disappear.
> Of course, "git bisect" and "gitk --bisect" does not have to change if
> we go that
> route.
> 
> And there will not be any backward compatibility worries. If you are not
> using multiple worktrees, you will see them as refs/bisect/*, just at the
> same location as you are familiar with.

Yes, this is a good point.

Re: [PATCH/RFC 0/2] bisect per-worktree

[Michael Haggerty]

On 08/01/2015 07:12 AM, Junio C Hamano wrote:
> On Fri, Jul 31, 2015 at 8:59 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>>
>> It seems to me that adding a new top-level "worktree-refs" directory is
>> pretty traumatic. Lots of people and tools will have made the assumption
>> that all "normal" references live under "refs/".
>> ...
>> It's all a bit frightening, frankly.
> 
> I actually feel the prospect of pluggable ref backend more frightening,
> frankly ;-). These bisect refs are just like FETCH_HEAD and MERGE_HEAD,
> not about the primary purpose of the "repository" to grow the history of refs
> (branches), but about ephemeral pointers into the history used to help keep
> track of what is being done in the worktree upstairs. There is no need for
> these to be visible across worktrees. If we use the real refs that are grobal
> in the repository (as opposed to per-worktree ones), we would hit the backend
> databas with transactions to update these ephemeral things, which somehow
> makes me feel stupid.

Hmm, ok, so you are thinking of a remote database with high latency. I
was thinking more of something like LMDB, with latency comparable to
filesystem storage.

These worktree-specific references might be ephemeral, but they also
imply reachability, which means that they need to be visible at least
during object pruning. Moreover, if the references don't live in the
same database with the rest of the references, then we have to deal with
races due to updating references in different places without atomicity.

The refs+object store is the most important thing for maintaining the
integrity of a repo and avoiding races. To me it seems easier to do so
if there is a single refs+objects store than if we have some references
over here on the file system, some over there in a LMDB, etc. So my gut
feeling is for the primary reference storage to be in a single reference
namespace that (at least in principle) can be stored in a single ACID
database.

For each worktree, we could then create a different view of the
references by splicing parts of the full reference namespace together.
This could even be based on config settings so that we don't have to
hardcode information like "refs/bisect/* is worktree-specific" deep in
the references module. Suppose we could write

[worktree.refs]
	map = refs/worktrees/*:
	map = refs/bisect/*:refs/worktrees/[worktree]/refs/bisect/*

which would mean (a) hide the references under refs/worktrees", and (b)
make it look as if the references under
refs/worktrees/[worktree]/refs/bisect actually appear under refs/bisect
(where "[worktree]" is replaced with the current worktree's name). By
making these settings configurable, we allow other projects to define
their own worktree-specific reference namespaces too.

The corresponding main repo might hide "refs/worktrees/*" but leave its
refs/bisect namespace exposed in the usual place.

"git prune" would see the whole namespace as it really is so that it can
compute reachability correctly.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH/RFC 0/2] bisect per-worktree

[Junio C Hamano]

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

> Hmm, ok, so you are thinking of a remote database with high latency. I
> was thinking more of something like LMDB, with latency comparable to
> filesystem storage.

Not necessarily.  The comment was more from conceptual point: "Why
share what needs not to be shared?"

> These worktree-specific references might be ephemeral, but they also
> imply reachability, which means that they need to be visible at least
> during object pruning.

True, but for bisect in particular, that is moot.  You are in
sightseer mode, marking various (older) points in time of an
existing history that is already anchored by branches and tags.

> Moreover, if the references don't live in the
> same database with the rest of the references, then we have to deal with
> races due to updating references in different places without atomicity.

Does that still hold true in the context of bisection?  You do not
even want atomicity to get in the way when you found this old commit
is bad and are about to mark the next one for testing by checking it
out.  You still want to mark the "bad" one (as it being bad is already
an established fact after testing), even if the subsequent checkout
of another commit (i.e. update to "HEAD") fails.

But these two comments above do take advantage of the fact that we
are discussing "bisect" and nothing else.  As a set of points to
consider in a more general context, I do agree that there are refs
and ref-like things that are per-worktree but still has to be part
of reachability, which "HEAD" is the prime example ;-), and some of
them may want to be part of a transaction.

I however think what David has been calling pseudo-refs falls more
in the "ephemeral and unnecessary to participate in reachabliity or
transaction" category (e.g. CHERRY_PICK_HEAD).  And I think the refs
used during bisection falls into the same category.

> For each worktree, we could then create a different view of the
> references by splicing parts of the full reference namespace together.
> ...
> "git prune" would see the whole namespace as it really is so that it can
> compute reachability correctly.

I really like this as a mechanism to include refs/ hierarchy, some
part of which is shared and some part of which is private.

I do not think bisect or pseudorefs needs that, though.

Re: [PATCH/RFC 0/2] bisect per-worktree

[Duy Nguyen]

On Sat, Aug 1, 2015 at 1:51 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> For each worktree, we could then create a different view of the
> references by splicing parts of the full reference namespace together.
> This could even be based on config settings so that we don't have to
> hardcode information like "refs/bisect/* is worktree-specific" deep in
> the references module. Suppose we could write
>
> [worktree.refs]
>         map = refs/worktrees/*:
>         map = refs/bisect/*:refs/worktrees/[worktree]/refs/bisect/*

Perhaps the destination should be worktrees/[worktree]/refs/bisect/*.
Does it have to start with "refs/"?

> which would mean (a) hide the references under refs/worktrees", and (b)
> make it look as if the references under
> refs/worktrees/[worktree]/refs/bisect actually appear under refs/bisect
> (where "[worktree]" is replaced with the current worktree's name). By
> making these settings configurable, we allow other projects to define
> their own worktree-specific reference namespaces too.
>
> The corresponding main repo might hide "refs/worktrees/*" but leave its
> refs/bisect namespace exposed in the usual place.
>
> "git prune" would see the whole namespace as it really is so that it can
> compute reachability correctly.

For multiple wortrees, first we basically remap paths in .git,
relocate some to worktrees/[worktree]/ (already done), then we're
going to remap config keys (submodule support, just proof of concept
so far), remapping ref paths sounds like the logical next step if some
under refs/ needs to be worktree-specific. I wonder if current ref
namespace code could be extended to do this?
-- 
Duy

Re: [PATCH/RFC 0/2] bisect per-worktree

[David Turner]

On Sat, 2015-08-01 at 08:51 +0200, Michael Haggerty wrote:
> On 08/01/2015 07:12 AM, Junio C Hamano wrote:
> > On Fri, Jul 31, 2015 at 8:59 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> >>
> >> It seems to me that adding a new top-level "worktree-refs" directory is
> >> pretty traumatic. Lots of people and tools will have made the assumption
> >> that all "normal" references live under "refs/".
> >> ...
> >> It's all a bit frightening, frankly.
> > 
> > I actually feel the prospect of pluggable ref backend more frightening,
> > frankly ;-). These bisect refs are just like FETCH_HEAD and MERGE_HEAD,
> > not about the primary purpose of the "repository" to grow the history of refs
> > (branches), but about ephemeral pointers into the history used to help keep
> > track of what is being done in the worktree upstairs. There is no need for
> > these to be visible across worktrees. If we use the real refs that are grobal
> > in the repository (as opposed to per-worktree ones), we would hit the backend
> > databas with transactions to update these ephemeral things, which somehow
> > makes me feel stupid.
> 
> Hmm, ok, so you are thinking of a remote database with high latency. I
> was thinking more of something like LMDB, with latency comparable to
> filesystem storage.
> 
> These worktree-specific references might be ephemeral, but they also
> imply reachability, which means that they need to be visible at least
> during object pruning. Moreover, if the references don't live in the
> same database with the rest of the references, then we have to deal with
> races due to updating references in different places without atomicity.
> 
> The refs+object store is the most important thing for maintaining the
> integrity of a repo and avoiding races. To me it seems easier to do so
> if there is a single refs+objects store than if we have some references
> over here on the file system, some over there in a LMDB, etc. So my gut
> feeling is for the primary reference storage to be in a single reference
> namespace that (at least in principle) can be stored in a single ACID
> database.
>
> For each worktree, we could then create a different view of the
> references by splicing parts of the full reference namespace together.
> This could even be based on config settings so that we don't have to
> hardcode information like "refs/bisect/* is worktree-specific" deep in
> the references module. Suppose we could write
> 
> [worktree.refs]
> 	map = refs/worktrees/*:
> 	map = refs/bisect/*:refs/worktrees/[worktree]/refs/bisect/*
> 
> which would mean (a) hide the references under refs/worktrees", and (b)
> make it look as if the references under
> refs/worktrees/[worktree]/refs/bisect actually appear under refs/bisect
> (where "[worktree]" is replaced with the current worktree's name). By
> making these settings configurable, we allow other projects to define
> their own worktree-specific reference namespaces too.
> 
> The corresponding main repo might hide "refs/worktrees/*" but leave its
> refs/bisect namespace exposed in the usual place.
> 
> "git prune" would see the whole namespace as it really is so that it can
> compute reachability correctly.

I think making this configurable is (a) overkill and (b) dangerous.
It's dangerous because the semantics of which refs are per-worktree is
important to the correct operation of git, and allowing users to mess
with it seems like a big mistake.  Instead, we should figure out a
simple scheme and define it globally.

I think refs/worktree -> refs/worktrees/[worktree]/ would do fine as a
fixed scheme, if we go that route.

We would need two separate views of the refs hierarchy, though: one used
by prune (and pack-refs) that is non-mapped (that is, includes
per-worktree refs for each worktree), and one for general use that is
mapped.   Maybe this is just a flag to the ref traversal functions.

But I'm not sure that this is really the right way to go.

As I understand it, we don't presently do many transactions that include
both pseudorefs or per-worktree refs and other refs.  And we definitely
don't want to move pseudorefs into the database since there's so much
code that assumes they're files.  Also, the vast majority of refs are
common, rather than per-worktree.  In fact, the only per-worktree refs
I've seen mentioned so far are the bisect refs and NOTES_MERGE_REF and
HEAD.  Of these, only HEAD is needed for pruning. Are there more that I
haven't thought of?

So I'm not sure the gain from moving per-worktree refs into the database
is that great.

There are some downsides of moving per-worktree refs into the database:

1. More operations in one worktree can now contend with operations in
another worktree for the database.  LMDB only allows a single write
transaction at a time.  

2. The refs API would be more complicated: it would need to deal with
remapped vs raw ref paths.  Refs backends would need to have functions
to prune per-worktree data when a worktree is destroyed. 

4. We would still need to deal with pseudorefs, so there's still some
missing transactional safety, and still the complication of dealing with
files on the filesystem.

Simply treating refs/worktree as per-worktree, while the rest of refs/
is not, would be a few dozen lines of code.  The full remapping approach
is likely to be a lot more. I've already got the lmdb backend working
with something like this approach.  If we decide on a complicated
approach, I am likely to run out of time to work on pluggable backends.

Re: [PATCH/RFC 0/2] bisect per-worktree

[Junio C Hamano]

David Turner <dturner@twopensource.com> writes:

> I think making this configurable is (a) overkill and (b) dangerous.
> It's dangerous because the semantics of which refs are per-worktree is
> important to the correct operation of git, and allowing users to mess
> with it seems like a big mistake.  Instead, we should figure out a
> simple scheme and define it globally.
>
> I think refs/worktree -> refs/worktrees/[worktree]/ would do fine as a
> fixed scheme, if we go that route.

OK.

> We would need two separate views of the refs hierarchy, though: one used
> by prune (and pack-refs) that is non-mapped (that is, includes
> per-worktree refs for each worktree), and one for general use that is
> mapped.   Maybe this is just a flag to the ref traversal functions.

True.  Alternatively we could just view refs/worktree/* as if they
are symbolic refs that point into refs/worktrees/$my_worktree/*, but
that would imply making the latter always visible to all worktrees,
which would hurt when people use it to interact with outside world
(namely, refs in other people's private area should probably not be
advertised).

> As I understand it, we don't presently do many transactions that include
> both pseudorefs or per-worktree refs and other refs.  And we definitely
> don't want to move pseudorefs into the database since there's so much
> code that assumes they're files.  Also, the vast majority of refs are
> common, rather than per-worktree.  In fact, the only per-worktree refs
> I've seen mentioned so far are the bisect refs and NOTES_MERGE_REF and
> HEAD.  Of these, only HEAD is needed for pruning. Are there more that I
> haven't thought of?

I myself have come up with nothing other than the above.  Let's hear
from others.

> So I'm not sure the gain from moving per-worktree refs into the database
> is that great.

I am on the same wavelength as you are on this.

> There are some downsides of moving per-worktree refs into the database:
> ...

All good points except #3, which I cannot judge if it is good or bad.

> ...
> Simply treating refs/worktree as per-worktree, while the rest of refs/
> is not, would be a few dozen lines of code.  The full remapping approach
> is likely to be a lot more.

We may be over-engineering with Michael's and even with the more
simpler refs/worktree/* -> refs/worktrees/$mine/* fixed mapping;
I tend to agree with you.

Re: [PATCH/RFC 0/2] bisect per-worktree

[Duy Nguyen]

On Tue, Aug 4, 2015 at 2:49 AM, David Turner <dturner@twopensource.com> wrote:
> Simply treating refs/worktree as per-worktree, while the rest of refs/
> is not, would be a few dozen lines of code.  The full remapping approach
> is likely to be a lot more. I've already got the lmdb backend working
> with something like this approach.  If we decide on a complicated
> approach, I am likely to run out of time to work on pluggable backends.

I think you still have another option: decide that lmdb backend does
not (yet) support multiple worktrees (which means make "git worktree
add" reject when lmdb backend is used). That would simplify some for
you and we can continue on at a later time.
-- 
Duy

Re: [PATCH/RFC 0/2] bisect per-worktree

[David Turner]

On Tue, 2015-08-04 at 06:09 +0700, Duy Nguyen wrote:
> On Tue, Aug 4, 2015 at 2:49 AM, David Turner <dturner@twopensource.com> wrote:
> > Simply treating refs/worktree as per-worktree, while the rest of refs/
> > is not, would be a few dozen lines of code.  The full remapping approach
> > is likely to be a lot more. I've already got the lmdb backend working
> > with something like this approach.  If we decide on a complicated
> > approach, I am likely to run out of time to work on pluggable backends.
> 
> I think you still have another option: decide that lmdb backend does
> not (yet) support multiple worktrees (which means make "git worktree
> add" reject when lmdb backend is used). That would simplify some for
> you and we can continue on at a later time.

Some of our developers are pretty excited about multiple worktrees, so I
don't really want to do that.  Also, it's easier to develop when more of
the tests pass under the LMDB backend (no need to investigate whether
worktrees are the reason for failures when there are no failures).

Re: [PATCH/RFC 0/2] bisect per-worktree

[Duy Nguyen]

On Sat, Aug 1, 2015 at 10:59 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> Either way, there's also the question of who should know how to find the
> worktree-specific references--the program that wants to access them, or
> should there be a secret invisible mapping that is done on lookup, and
> that knows the full list of references that want to be worktree-specific
> (for example, that in a linked worktree, "refs/bisect/*" should be
> silently redirected to "refs/worktree/<name>/bisect/*")?
>
> It's all a bit frightening, frankly.

I would think all work to make pluggable backends happen should allow
us to do this kind of transparent mapping. We can add a new file-based
backend with just this redirection, can't we? I haven't read the
updated refs.c though, need to do it now..
-- 
Duy

Re: [PATCH/RFC 0/2] bisect per-worktree

[Duy Nguyen]

On Mon, Aug 3, 2015 at 8:02 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Sat, Aug 1, 2015 at 10:59 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> Either way, there's also the question of who should know how to find the
>> worktree-specific references--the program that wants to access them, or
>> should there be a secret invisible mapping that is done on lookup, and

After re-reading some code, I think this invisible mapping on lookup
is already done for our file-based ref backend. Take
resolve_ref_unsafe_1 for example, the call strbuf_git_path() may
return ".git/worktrees/foo/HEAD", not ".git/HEAD", given the input ref
"HEAD". So ref names are already separated from where they are stored.

>> that knows the full list of references that want to be worktree-specific
>> (for example, that in a linked worktree, "refs/bisect/*" should be
>> silently redirected to "refs/worktree/<name>/bisect/*")?

My decision to hard code these paths may turn out a bad thing (it's
common_list[] in path.c). Perhaps I should let the user extend them,
but we will not allow to override a small subset of paths because that
may break stuff horribly.

>> It's all a bit frightening, frankly.
>
> I would think all work to make pluggable backends happen should allow
> us to do this kind of transparent mapping. We can add a new file-based
> backend with just this redirection, can't we? I haven't read the
> updated refs.c though, need to do it now..
-- 
Duy