[v2,3/4] Make die_if_checked_out() prune missing checkouts of unlocked worktrees.
diff mbox series

Message ID 20191018194542.1316981-3-pjones@redhat.com
State New
Headers show
Series
  • [v2,1/4] libgit: Add a read-only helper to test the worktree lock
Related show

Commit Message

Peter Jones Oct. 18, 2019, 7:45 p.m. UTC
Currently if you do, for example:

$ git worktree add path foo

And "foo" has already been checked out at some other path, but the user
has removed it without pruning, and the worktree is not locked, you'll
get an error that the branch is already checked out.  It isn't
meaningfully checked out, the repo's data is just stale and no longer
reflects reality.

This makes it so that if nothing is present where a worktree is
supposedly checked out, and it is not locked, we ignore that the
worktree exists, then it should be cleaned up.

Signed-off-by: Peter Jones <pjones@redhat.com>
---
 branch.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Junio C Hamano Oct. 21, 2019, 2:09 a.m. UTC | #1
Peter Jones <pjones@redhat.com> writes:

[jc: won't repeat comments on the title]

> @@ -360,6 +360,12 @@ void die_if_checked_out(const char *branch, int ignore_current_worktree)
>  	wt = find_shared_symref("HEAD", branch);
>  	if (!wt || (ignore_current_worktree && wt->is_current))
>  		return;

die-if-checked-out is called from callers that expect to be stopped
before they do any harm, so it feels dirty to make a side effect
like this.

If the user tries to check out a branch that used to be checked out
in an already removed worktree, doesn't that indicate that an
earlier worktree removal was done incorrectly, which is something
worth reporting to the user and give the user a chance to think and
choose what corrective action(s) need to be taken?

For that, instead of automatically losing information like this
patch does, it may make more sense to fail the checkout and stop at
giving diagnosis (e.g. "our record shows that the branch is checked
out in that worktree, but you seem to have lost it.  if you forgot
to prune it, then here is the command you can give to do so.")
without actually touching the filesystem.

Thanks.


> +
> +	if (prune_worktree_if_missing(wt) >= 0) {
> +		delete_worktrees_dir_if_empty();
> +		return;
> +	}
> +
>  	skip_prefix(branch, "refs/heads/", &branch);
>  	die(_("'%s' is already checked out at '%s'"),
>  	    branch, wt->path);

Patch
diff mbox series

diff --git a/branch.c b/branch.c
index 579494738a7..760ef387144 100644
--- a/branch.c
+++ b/branch.c
@@ -360,6 +360,12 @@  void die_if_checked_out(const char *branch, int ignore_current_worktree)
 	wt = find_shared_symref("HEAD", branch);
 	if (!wt || (ignore_current_worktree && wt->is_current))
 		return;
+
+	if (prune_worktree_if_missing(wt) >= 0) {
+		delete_worktrees_dir_if_empty();
+		return;
+	}
+
 	skip_prefix(branch, "refs/heads/", &branch);
 	die(_("'%s' is already checked out at '%s'"),
 	    branch, wt->path);