[PATCH/WIP 0/6] Detached HEADs in new worktrees considered harmful

[PATCH/WIP 0/6] Detached HEADs in new worktrees considered harmful

[Michael J Gruber]

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

[PATCH 1/6] Documentation/git-worktree: spell --detach correctly

[Michael J Gruber]

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 Documentation/git-worktree.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 62c76c1..51a2ea0 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -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

[PATCH 2/6] t6014: test prune with detached HEADs in separate worktrees

[Michael J Gruber]

"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.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 t/t6014-rev-list-all.sh | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/t/t6014-rev-list-all.sh b/t/t6014-rev-list-all.sh
index c9bedd2..99bf8ae 100755
--- a/t/t6014-rev-list-all.sh
+++ b/t/t6014-rev-list-all.sh
@@ -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

[PATCH/RFD 3/6] rev-list: list all heads with --all

[Michael J Gruber]

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.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---

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(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 81f68f8..5bdb568 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -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

[PATCH 4/6] WIP: mess only with mark_reachable

[Michael J Gruber]

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.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 reachable.c          |  2 +-
 refs.h               |  1 +
 refs/files-backend.c | 23 +++++++++++++++++++++++
 3 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/reachable.c b/reachable.c
index ed35201..15a06a7 100644
--- a/reachable.c
+++ b/reachable.c
@@ -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)
diff --git a/refs.h b/refs.h
index 2f3decb..c9621b4 100644
--- a/refs.h
+++ b/refs.h
@@ -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);
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 5bdb568..f020c52 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -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

[PATCH 5/6] WIP: fix unborn branch case

[Michael J Gruber]

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 refs/files-backend.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index f020c52..1614854 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -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

[PATCH 6/6] revisions: list all worktree HEADs with --all

[Michael J Gruber]

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---

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(-)

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 4f009d4..fe37956 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -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
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 1614854..e717db0 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -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

Re: [PATCH/WIP 0/6] Detached HEADs in new worktrees considered harmful

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

> 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.

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.

Re: [PATCH 1/6] Documentation/git-worktree: spell --detach correctly

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
> ---
>  Documentation/git-worktree.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> index 62c76c1..51a2ea0 100644
> --- a/Documentation/git-worktree.txt
> +++ b/Documentation/git-worktree.txt
> @@ -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,

That is correct, but I wonder if there is an " is" missing there...?

>  then, as a convenience, a new branch based at HEAD is created automatically,
>  as if `-b $(basename <path>)` was specified.

Re: [PATCH 2/6] t6014: test prune with detached HEADs in separate worktrees

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

> "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.

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.

Re: [PATCH/RFD 3/6] rev-list: list all heads with --all

[Duy Nguyen]

On Fri, Feb 26, 2016 at 11:39 PM, Michael J Gruber
<git@drmicha.warpmail.net> wrote:
> 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.
>
> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
> ---
>
> 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.]

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