Re: [PATCH 10/16] worktree: make branch creation distinct from worktree population

[Eric Sunshine <sunshine@sunshineco.com>]

On Sun, Jul 12, 2015 at 04:54:02PM +0700, Duy Nguyen wrote:
> On Sun, Jul 12, 2015 at 10:27 AM, Eric Sunshine <sunshine@sunshineco.com> wrot> > In this case, it's easy enough to side-step the issue since there's no
> > need to call ref_exists() if the new branch was created successfully
> > (since we know it exists). The logic would effectively become this:
> >
> >     branch = ...
> >     if (create_new_branch) {
> >         exec "git branch newbranch branch"
> >         exec "git symbolic-ref HEAD newbranch"
> >     } else if (ref_exists(branch) && !detach)
> >         exec "git symbolic-ref HEAD branch"
> >     else
> >         exec "git update-ref HEAD $(git rev-parse branch)"
> >     exec "git reset --hard"
> 
> Yeah.. Another option we can take is deal with this at run-command.c
> level (and outside this series) because this could affect everywhere:
> by default, invalidate all cache after running any git commands. The
> caller can pass options to keep some cache intact if they know the
> command won't touch it.
> 
> If ref_exists() is the only thing we use, right now it does not use
> cache so we should be safe. If the new ref backend introduces a cache,
> they would need to examine all callers anyway, including this one. The
> cache in refs.c seems to be for for_each_ref.. only, which we don't
> call here.

In case I don't need to re-roll the series for some other reason, here's
a squash-in making the above adjustment to patch 12/16, which Junio can
pick up if he wants to:

---- 8< ----
From: Eric Sunshine <sunshine@sunshineco.com>
Subject: [PATCH] fixup! worktree: detect branch symref/detach and error conditions locally

Current logic is effectively:

    branch = ...
    if (create_new_branch) {
        run "git branch newbranch branch"
        branch = newbranch
    }
    if (!detach && ref_exists(branch)) {
        if (!force)
            die_if_already_checked_out(branch)
        run "git symbolic-ref HEAD branch"
    } else
        run "git update-ref HEAD $(git rev-parse branch)"
    run "git reset --hard"

The ref_exists() call following 'run "git branch newbranch branch"'
works fine since ref_exists() hits the filesystem directly to answer the
question rather than relying upon potentially stale cached information.
However, this is potentially fragile if someone some day implements
caching. Moreover, with pluggable backends on the horizon, we may not
be able to rely upon ref_exists() in this process accurately reflecting
changes made by a subprocess.

If new branch creation was successful, then we know it's a branch, and
don't need to ask ref_exists() about it, thus we don't need to worry
about ref_exists() possibly returning an answer based upon stale
information, nor do we have to check if the new branch is already
checked out elsewhere. Therefore, rework the logic slightly to become
effectively:

    branch = ...
    if (create_new_branch) {
        run "git branch newbranch branch"
        run "git symbolic-ref HEAD newbranch"
    } else if (!detach && ref_exists(branch)) {
        if (!force)
            die_if_already_checked_out(branch)
        run "git symbolic-ref HEAD branch"
    } else
        run "git update-ref HEAD $(git rev-parse branch)"
    run "git reset --hard"

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 builtin/worktree.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 51c57bc..39f87a4 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -195,8 +195,10 @@ static int add_worktree(const char *path, const char *refname,
 	if (file_exists(path) && !is_empty_dir(path))
 		die(_("'%s' already exists"), path);
 
-	if (!opts->detach && !strbuf_check_branch_ref(&symref, refname) &&
-	    ref_exists(symref.buf)) {
+	if (opts->force_new_branch)
+		;
+	else if (!opts->detach && !strbuf_check_branch_ref(&symref, refname) &&
+		 ref_exists(symref.buf)) {
 		if (!opts->force)
 			die_if_checked_out(symref.buf);
 	} else {
-- 
2.5.0.rc1.387.g8463c8d