Detached HEADs in new worktrees considered harmful

Before someone loses their HEAD I should put this series out (again).

1/6 is a tiny documentation fix.

2/6 demonstrates that "git prune" in the main worktree loses HEADs in other worktrees, and vice versa.

In fact, one should rethink the meaning of "git rev-list --all" in the context of per-worktree refs such as HEAD.

3/6 is a possible fix (making "--all" mean "all refs in all worktrees") but may be too intrusive.

4/6, 5/6, 6/6 are some WIP patches about making 3/6 less intrusive and fixing some cases where do_head_ref_worktrees() has problems that do_head_ref() doesn't have.

Michael J Gruber (6):
Documentation/git-worktree: spell --detach correctly
t6014: test prune with detached HEADs in separate worktrees
rev-list: list all heads with --all
WIP: mess only with mark_reachable
fix unborn branch case
revisions: list all worktree HEADs with --all

 Documentation/git-worktree.txt     |  2 +-
 Documentation/rev-list-options.txt |  2 +-
 reachable.c                        |  2 +-
 refs.h                             |  1 +
 refs/files-backend.c               | 34 +++++++++++++++++++++++++++++++++-
 t/t6014-rev-list-all.sh            | 25 +++++++++++++++++++++++++
 6 files changed, 62 insertions(+), 4 deletions(-)

2.7.2.618.g7a61b68

---
 Documentation/git-worktree.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 62c76c1..51a2ea0 100644
@@ -50,7 +50,7 @@ Create `<path>` and checkout `<branch>` into it. The new working directory
 is linked to the current repository, sharing everything except working
 directory specific files such as HEAD, index, etc.
 +
-If `<branch>` is omitted and neither `-b` nor `-B` nor `--detached` used,
+If `<branch>` is omitted and neither `-b` nor `-B` nor `--detach` used,
 then, as a convenience, a new branch based at HEAD is created automatically,
 as if `-b $(basename <path>)` was specified.
 

2.7.2.618.g7a61b68

"git prune" relies on "git rev-list --all" to list all reachable, non-prunable commits. In the presence of per-worktree refs such as HEAD this becomes more complicated.

Add a test that makes sure that per-worktree refs from other worktrees are not omitted.

This is currently broken, possibly resulting in data loss.

---
 t/t6014-rev-list-all.sh | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

index c9bedd2..99bf8ae 100755
@@ -39,4 +39,29 @@ test_expect_success 'rev-list --graph --no-walk is forbidden' '
 	test_must_fail git rev-list --graph --no-walk HEAD
 '
 
+test_expect_success 'setup worktree tests' '
+	mkdir newtree &&
+	git worktree add --detach newtree master^ &&
+	(
+		cd newtree &&
+		commit detached2
+	)
+'
+
+test_expect_failure 'prune in main worktree does not lose detached HEAD in new worktree' '
+	git prune --expire=now &&
+	(
+		cd newtree &&
+		git show HEAD
+	)
+'
+
+test_expect_failure 'prune in new worktree does not lose detached HEAD in main worktree' '
+	(
+		cd newtree &&
+		git prune --expire=now
+	) &&
+	git show HEAD
+'
+
 test_done

2.7.2.618.g7a61b68

Not "all", perhaps "majority of". The index is another anchor point and rev-list is not involved in it at all.

For those that are anchored by refs and ref-like things, I think the right thing to do is not to change the semantics of "--all" like [3/6] does, but to invent a new option to "for-each-ref" that shows only the refs and ref-like things that are private to the worktree. That set may include HEAD and refs/bisect/*, perhaps.

With that, the scripted equivalent of "git repack", "git prune" and "git fsck" can ask "git worktree list" the list of worktrees to consult, and in each of them run "git for-each-ref --private" and "git ls-files -s" to grab the objects to be used as the starting point of reachability traversal.

HEAD is a worktree specific sysmref, so that a repository with multiple worktrees can have multiple HEADs, or HEADs in a worktree different from the current worktree.

So far, "rev-parse --all" adds only the HEAD from the current worktree to the list of refs (besides branches etc.). So, a detached HEAD from a different checkout would be missed unless a shared ref (or current HEAD) points to it (or descents from it). As a consequence, "git prune" can prune detached HEADs from worktrees and leave the repo in an inconsistent state.

Make "rev-parse --all" add the HEADs from all worktrees. This results in a non-worktree-specific ref list and solves the pruning problem.

---

Notes:
This patch solves the pruning problem with worktrees, but I feel quite uneasy about substituting the ref solving at the very heart of refs.c. So please consider this a mere poc and a request for discussion/input about how to do this the right way.

In essence, I feel the worktree interface still has to evolve a bit: I'd rather for_each_worktree than loop myself, and if many call sites need to be aware of multiple heads or worktrees than get_worktrees() should be part of our init stuff, not here. [I may be out of sync of newer progress.]

 refs/files-backend.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

index 81f68f8..5bdb568 100644
@@ -4,6 +4,7 @@
 #include "../lockfile.h"
 #include "../object.h"
 #include "../dir.h"
+#include "../worktree.h"
 
 struct ref_lock {
 	char *ref_name;

@@ -1748,7 +1749,8 @@ static int do_for_each_ref(struct ref_cache *refs, const char *base,
 static int do_head_ref(const char *submodule, each_ref_fn fn, void *cb_data)
 {
 	struct object_id oid;
-	int flag;
+	struct worktree **worktrees;
+	int i, retval;
 
 	if (submodule) {
 		if (resolve_gitlink_ref(submodule, "HEAD", oid.hash) == 0)

@@ -1757,10 +1759,15 @@ static int do_head_ref(const char *submodule, each_ref_fn fn, void *cb_data)
 		return 0;
 	}
 
-	if (!read_ref_full("HEAD", RESOLVE_REF_READING, oid.hash, &flag))
-		return fn("HEAD", &oid, flag, cb_data);
+	worktrees = get_worktrees();
+	retval = 0;
+	for (i=0; worktrees[i]; i++) {
+		hashcpy(oid.hash, worktrees[i]->head_sha1);
+		retval = retval || fn("HEAD", &oid, worktrees[i]->is_detached ? 0 : REF_ISSYMREF, cb_data);
+	}
 
-	return 0;
+	free_worktrees(worktrees);
+	return retval;
 }
 
 int head_ref(each_ref_fn fn, void *cb_data)

2.7.2.618.g7a61b68

How about adding for_each_head_ref_submodule() and make handle_revision_pseudo_opt() use it with a new option, .e.g --all-work-trees?

I looked over head_ref and head_ref_submodule call sites. I think we're ok. Most of them only concern about "our head". for-each-ref may need some improvement though.

Duy

This still messes up gc/prune on an empty repo where the worktree code can't parse HEAD properly. Bummer. That code sucks for the non-worktree case.

---
 reachable.c          |  2 +-
 refs.h               |  1 +
 refs/files-backend.c | 23 +++++++++++++++++++++++
 3 files changed, 25 insertions(+), 1 deletion(-)

index ed35201..15a06a7 100644
@@ -177,7 +177,7 @@ void mark_reachable_objects(struct rev_info *revs, int mark_reflog,
 	for_each_ref(add_one_ref, revs);
 
 	/* detached HEAD is not included in the list above */
-	head_ref(add_one_ref, revs);
+	head_ref_worktrees(add_one_ref, revs);
 
 	/* Add all reflog info */
 	if (mark_reflog)

index 2f3decb..c9621b4 100644
@@ -183,6 +183,7 @@ typedef int each_ref_fn(const char *refname,
  * stop the iteration.
  */
 extern int head_ref(each_ref_fn fn, void *cb_data);
+extern int head_ref_worktrees(each_ref_fn fn, void *cb_data);
 extern int for_each_ref(each_ref_fn fn, void *cb_data);
 extern int for_each_ref_in(const char *prefix, each_ref_fn fn, void *cb_data);
 extern int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data, unsigned int broken);

index 5bdb568..f020c52 100644
@@ -1749,6 +1749,24 @@ static int do_for_each_ref(struct ref_cache *refs, const char *base,
 static int do_head_ref(const char *submodule, each_ref_fn fn, void *cb_data)
 {
 	struct object_id oid;
+	int flag;
+
+	if (submodule) {
+		if (resolve_gitlink_ref(submodule, "HEAD", oid.hash) == 0)
+			return fn("HEAD", &oid, 0, cb_data);
+
+		return 0;
+	}
+
+	if (!read_ref_full("HEAD", RESOLVE_REF_READING, oid.hash, &flag))
+		return fn("HEAD", &oid, flag, cb_data);
+
+	return 0;
+}
+
+static int do_head_ref_worktrees(const char *submodule, each_ref_fn fn, void *cb_data)
+{
+	struct object_id oid;
 	struct worktree **worktrees;
 	int i, retval;
 

@@ -1775,6 +1793,11 @@ int head_ref(each_ref_fn fn, void *cb_data)
 	return do_head_ref(NULL, fn, cb_data);
 }
 
+int head_ref_worktrees(each_ref_fn fn, void *cb_data)
+{
+	return do_head_ref_worktrees(NULL, fn, cb_data);
+}
+
 int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)
 {
 	return do_head_ref(submodule, fn, cb_data);

2.7.2.618.g7a61b68

---
 refs/files-backend.c | 2 ++
 1 file changed, 2 insertions(+)

index f020c52..1614854 100644
@@ -1780,6 +1780,8 @@ static int do_head_ref_worktrees(const char *submodule, each_ref_fn fn, void *cb
 	worktrees = get_worktrees();
 	retval = 0;
 	for (i=0; worktrees[i]; i++) {
+		if (is_null_sha1(worktrees[i]->head_sha1))
+			continue;
 		hashcpy(oid.hash, worktrees[i]->head_sha1);
 		retval = retval || fn("HEAD", &oid, worktrees[i]->is_detached ? 0 : REF_ISSYMREF, cb_data);
 	}

2.7.2.618.g7a61b68

---

Notes:
Note that ever since
f0298cf (revision walker: include a detached HEAD in --all, 2009-01-16) the documentation failed to mention the inclusion of HEAD with "--all".

 Documentation/rev-list-options.txt | 2 +-
 refs/files-backend.c               | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

index 4f009d4..fe37956 100644
@@ -134,7 +134,7 @@ parents) and `--max-parents=-1` (negative numbers denote no upper limit).
 
 --all::
 	Pretend as if all the refs in `refs/` are listed on the
-	command line as '<commit>'.
+	command line as '<commit>', as well as the HEADs of all worktrees.
 
 --branches[=<pattern>]::
 	Pretend as if all the refs in `refs/heads` are listed

index 1614854..e717db0 100644
@@ -1802,7 +1802,7 @@ int head_ref_worktrees(each_ref_fn fn, void *cb_data)
 
 int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)
 {
-	return do_head_ref(submodule, fn, cb_data);
+	return do_head_ref_worktrees(submodule, fn, cb_data);
 }
 
 int for_each_ref(each_ref_fn fn, void *cb_data)

2.7.2.618.g7a61b68

You do need to teach the reachability code to consider points that are outside refs/ (HEAD and index, at least; I also recall there were talks about per-worktree hierarchies in refs/?) as anchoring objects to the non-prunable part of the history.

The prune/fsck reachability code already knows to consider objects that are reachable only from the index as reachable, and I do not think "rev-list --all" is used to enumerate them for safekeeping (even when the worktree feature is not used). The reachability code that enumerates objects in the index and cache-tree needs to learn to peek into sibling worktrees' private objects, I would think, and I do not think the addition is a good match for rev-list.