threads / patch / 41536

patch, 6 partsWIP: fix unborn branch case

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

## tl;dr

11 messages between Feb 26, 2016 and Feb 27, 2016. Diffs are folded; open one to read it.

replies: 10people: 3as markdown or json

Michael J Gruber· Feb 26, 2016, 16:39 UTC · lore

[PATCH/WIP 0/6] 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
Michael J Gruber· Feb 26, 2016, 16:39 UTC · re: Michael J Gruber · lore

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

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 Documentation/git-worktree.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to Documentation/git-worktree.txt +1 −1
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
Junio C Hamano· Feb 26, 2016, 17:52 UTC · re: Michael J Gruber · lore

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

Michael J Gruber <git@drmicha.warpmail.net> writes:
Show 15 quoted lines
> 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.
Michael J Gruber· Feb 26, 2016, 16:39 UTC · re: Michael J Gruber · lore

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

"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(+)
Show changes to t/t6014-rev-list-all.sh +25 −0
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
Junio C Hamano· Feb 26, 2016, 18:03 UTC · re: Michael J Gruber · lore

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

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.

Michael J Gruber· Feb 26, 2016, 16:39 UTC · re: Michael J Gruber · lore

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

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(-)
Show changes to refs/files-backend.c +11 −4
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
Duy Nguyen· Feb 27, 2016, 02:15 UTC · re: Michael J Gruber · lore

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

On Fri, Feb 26, 2016 at 11:39 PM, Michael J Gruber <git@drmicha.warpmail.net> wrote:

Show 27 quoted lines
> 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
Michael J Gruber· Feb 26, 2016, 16:39 UTC · re: Michael J Gruber · lore

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

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(-)
Show changes to 3 files +25 −1

reachable.c, refs.h, refs/files-backend.c

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
Michael J Gruber· Feb 26, 2016, 16:39 UTC · re: Michael J Gruber · lore
Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 refs/files-backend.c | 2 ++
 1 file changed, 2 insertions(+)
Show changes to refs/files-backend.c +2 −0
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
Michael J Gruber· Feb 26, 2016, 16:39 UTC · re: Michael J Gruber · lore

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

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(-)
Show changes to 2 files +2 −2

Documentation/rev-list-options.txt, refs/files-backend.c

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
Junio C Hamano· Feb 26, 2016, 17:37 UTC · re: Michael J Gruber · lore

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

Michael J Gruber <git@drmicha.warpmail.net> writes:
Show 6 quoted lines
> 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.

← back to recent threads