Re: [PATCH 2/2] Make "git branch -d" prune missing worktrees automatically.

From: Eric Sunshine <sunshine@sunshineco.com>
To: Peter Jones <pjones@redhat.com>
Cc: Git List <git@vger.kernel.org>
Subject: Re: [PATCH 2/2] Make "git branch -d" prune missing worktrees automatically.
Date: Thu, 17 Oct 2019 13:28:09 -0400	[thread overview]
Message-ID: <CAPig+cS6SzLdgmzffNkg72YSiDQ9eQRqTK12NsraKpGbkJFY_w@mail.gmail.com> (raw)
In-Reply-To: <20191017162826.1064257-2-pjones@redhat.com>

On Thu, Oct 17, 2019 at 12:28 PM Peter Jones <pjones@redhat.com> wrote:
> Currently, if you do:
>
> $ git branch zonk origin/master
> $ git worktree add zonk zonk
> $ rm -rf zonk
> $ git branch -d zonk
>
> You get the following error:
>
> $ git branch -d zonk
> error: Cannot delete branch 'zonk' checked out at '/home/pjones/devel/kernel.org/git/zonk'
>
> It isn't meaningfully checked out, the repo's data is just stale and no
> longer reflects reality.

Echoing SEZDER's comment on patch 1/2, this behavior is an intentional
design choice and safety feature of the worktree implementation since
worktrees may exist on removable media or remote filesystems which
might not always be mounted; hence, the presence of commands "git
worktree prune" and "git worktree remove".

A couple comment regarding this patch...

> Signed-off-by: Peter Jones <pjones@redhat.com>
> ---
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -133,6 +133,20 @@ static int prune_worktree(const char *id, struct strbuf *reason)
> +int prune_worktree_if_missing(const struct worktree *wt)
> +{
> +       struct strbuf reason = STRBUF_INIT;
> +
> +       if (access(wt->path, F_OK) >= 0 ||
> +           (errno != ENOENT && errno == ENOTDIR)) {
> +               errno = EEXIST;
> +               return -1;
> +       }
> +
> +       strbuf_addf(&reason, _("Removing worktrees/%s: worktree directory is not present"), wt->id);
> +       return prune_worktree(wt->id, &reason);
> +}

"git worktree" tries to clean up after itself as much as possible. For
instance, it is careful to remove the .git/worktrees directory when
the last worktree itself is removed (or pruned). So, the caller of
this function would also want to call delete_worktrees_dir_if_empty()
to follow suit.

> diff --git a/worktree.h b/worktree.h
> @@ -132,4 +132,10 @@ void strbuf_worktree_ref(const struct worktree *wt,
> +/*
> + * Prune a worktree if it is no longer present at the checked out location.
> + * Returns < 0 if the checkout is there or if pruning fails.
> + */
> +int prune_worktree_if_missing(const struct worktree *wt);

It's rather ugly that this function is declared in top-level
worktree.h whereas the actual implementation is in builtin/worktree.c.
I'd expect to see a preparatory patch which moves prune_worktree()
(and probably delete_worktrees_dir_if_empty()) to top-level
worktree.c.

These minor implementation comments aside, before considering this
patch series, it would be nice to see a compelling argument as to why
this change of behavior, which undercuts a deliberate design decision,
is really desirable.