Re: [PATCH v2 09/12] worktree.c: test if branch being rebased in another worktree

Nguyễn Thái Ngọc Duy  [off-list ref] writes:

Subject: Re: [PATCH v2 09/12] worktree.c: test if branch being rebased in another worktree

Lacks the verb?  Perhaps s/being/is/ is sufficient.

This function find_shared_symref() is used in a couple places:

1) in builtin/branch.c: it's used to detect if a branch is checked out
   elsewhere and refuse to delete the branch.

2) in builtin/notes.c: it's used to detect if a note is being merged in
   another worktree

3) in branch.c, the function die_if_checked_out() is actually used by
   "git checkout" and "git worktree add" to see if a branch is already
   checked out elsewhere and refuse the operation.

In cases 1 and 3, if a rebase is ongoing, "HEAD" will be in detached
mode, find_shared_symref() fails to detect it and declares "no branch is
checked out here", which is incorrect.

which is technically correct but is not what we want to check.

This patch tightens the test. If the given symref is "HEAD", we try to
detect if rebase is ongoing. If so return the branch being rebased. This
makes checkout and branch delete operations safer because you can't
checkout a branch being rebased in another place, or delete it.

Is rebase the only thing that tentatively detach before working on
the real branch?  It may be currently the case, but I would imagine
that we want to makefind-shared-symref to be responsible for
detecting new cases other than rebase that we may introduce in the
future, in which case we may want to leave come comment near the
function to describe that expectation.

Special case for checkout. If the current branch is being rebased,
git-rebase.sh may use "git checkout" to abort and return back to the
original branch. The updated test in find_shared_symref() will prevent
that and "git rebase --abort" will fail as a result.
find_shared_symref() and die_if_checked_out() have to learn a new
option ignore_current_worktree to loose the test a bit.

s/loose/&n/

So the idea is "if the branch is checked out (or "being worked on"
even if technically the HEAD is detached, like with 'rebase')
anywhere, callers of die-if-checked-out in general want to die; but
for this caller, it is OK if the place the branch is checked out or
being worked on is in this repository"?

I understand die_if_checked_out() taking that "ignore this one" bit
may be sensible, but I do not understand why find_shared_symref()
needs to be told about it.  The change makes the meaning of the
find_shared_symref() function unclear.  It used to mean "This
symbolic ref cannot point at the same ref in different worktrees, so
for a given pair of a symbolic ref and a concrete ref, there can be
at most one worktree in which the symbolic ref points at that ref".
That is already a mouthful.  As the worktree structure already have
"Am I the current worktree?" bit, "ignore" logic can easily be done
inside die_if_checked_out() and that would help find_shared_symref()
stay simpler and more focused function, no?

On Thu, Apr 21, 2016 at 1:04 AM, Junio C Hamano [off-list ref] wrote:

quoted

diff --git a/builtin/checkout.c b/builtin/checkout.c
index efcbd8f..6041718 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c

@@ -1111,7 +1111,7 @@ static int checkout_branch(struct checkout_opts *opts,
              char *head_ref = resolve_refdup("HEAD", 0, sha1, &flag);
              if (head_ref &&
                  (!(flag & REF_ISSYMREF) || strcmp(head_ref, new->path)))
-                     die_if_checked_out(new->path);
+                     die_if_checked_out(new->path, 1);
              free(head_ref);
      }

So the idea is "if the branch is checked out (or "being worked on"
even if technically the HEAD is detached, like with 'rebase')
anywhere, callers of die-if-checked-out in general want to die; but
for this caller, it is OK if the place the branch is checked out or
being worked on is in this repository"?

I understand die_if_checked_out() taking that "ignore this one" bit
may be sensible, but I do not understand why find_shared_symref()
needs to be told about it.  The change makes the meaning of the
find_shared_symref() function unclear.  It used to mean "This
symbolic ref cannot point at the same ref in different worktrees, so
for a given pair of a symbolic ref and a concrete ref, there can be
at most one worktree in which the symbolic ref points at that ref".
That is already a mouthful.  As the worktree structure already have
"Am I the current worktree?" bit, "ignore" logic can easily be done
inside die_if_checked_out() and that would help find_shared_symref()
stay simpler and more focused function, no?

That was the intention when I made find_shared_symref() return struct
worktree * instead of char *. I forget why I changed my mind and not
do so. The only case when find_shared_symref should do this is when a
ref is shared twice, then we need to ignore current worktree from
inside the loop. But that can't happen. Will move this
ignore-current-worktree test to die_if_checked_out().
-- 
Duy