Re: [PATCH v3 2/3] worktree: make add <path> <branch> dwim

[Thomas Gummerer <t.gummerer@gmail.com>]

On 11/19, Eric Sunshine wrote:
> On Sat, Nov 18, 2017 at 5:47 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> > Currently 'git worktree add <path> <branch>', errors out when 'branch'
> > is not a local branch.   It has no additional dwim'ing features that one
> > might expect.
> >
> > Make it behave more like 'git checkout <branch>' when the branch doesn't
> > exist locally, but a remote tracking branch uniquely matches the desired
> > branch name, i.e. create a new branch from the remote tracking branch
> > and set the upstream to the remote tracking branch.
> >
> > As 'git worktree add' currently just dies in this situation, there are
> > no backwards compatibility worries when introducing this feature.
> >
> > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> > ---
> > diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> > @@ -52,6 +52,13 @@ is linked to the current repository, sharing everything except working
> > +If <branch> is not found, and neither `-b` nor `-B` nor `--detach` are
> > +used, but there does exist a tracking branch in exactly one remote
> > +(call it <remote>) with a matching name, treat as equivalent to
> > +------------
> > +$ git worktree add -b <branch> <path> <remote>/<branch>
> > +------------
> 
> The example from which this was copied in git-checkout.txt shows the
> --track option being used, which makes it clear that the new local
> branch will track the remote-tracking branch. git-worktree, of course,
> does not have a --track option, but would it make sense to mention
> explicitly in the prose that the newly-created local branch tracks the
> remote one? (Or are readers sufficiently savvy to intuit it?)

It is how 'git worktree add -b <branch> <path> <remote>/<branch>'
currently works, which is also not documented anywhere.  However I'm
not sure it's super intuitive, from just reading the command, so I'll
add an explicit mention about it.

> > diff --git a/builtin/worktree.c b/builtin/worktree.c
> > index 7b9307aa58..05fc902bcc 100644
> > --- a/builtin/worktree.c
> > +++ b/builtin/worktree.c
> > @@ -1,4 +1,5 @@
> >  #include "cache.h"
> > +#include "checkout.h"
> >  #include "config.h"
> >  #include "builtin.h"
> >  #include "dir.h"
> > @@ -386,6 +387,20 @@ static int add(int ac, const char **av, const char *prefix)
> >                 opts.new_branch = xstrndup(s, n);
> >         }
> >
> > +       if (ac == 2 && !opts.new_branch && !opts.detach) {
> > +               struct object_id oid;
> > +               struct commit *commit;
> > +               const char *remote;
> > +
> > +               commit = lookup_commit_reference_by_name(branch);
> > +               if (!commit)
> > +                       remote = unique_tracking_name(branch, &oid);
> > +               if (!commit && remote) {
> > +                       opts.new_branch = branch;
> > +                       branch = remote;
> > +               }
> > +       }
> 
> You can simplify the above conditionals to:
> 
>     commit = ...
>     if (!commit) {
>         remote = ...
>         if (remote) {
>             ...
>         }
>     }

Will change, thanks!

> So, although you're not passing --track explicitly to the "git branch"
> invocation just below, you get --track for free since it's the default
> behavior when creating a new local branch from a remote one, correct?
> (Just checking my understanding.)

Yes that's correct.  The default behaviour of git branch does exactly
what we want here, so we're relying on it, instead of gouing through
the trouble of explicitly specifying --track.

We have a test checking the expected behaviour of setting up the
upstream, so in the unlikely event that the behaviour in 'git branch'
changes, we'd still guard against it there.  Therefore I don't think
there's a need to be extra defensive here.

> >         if (opts.new_branch) {
> >                 struct child_process cp = CHILD_PROCESS_INIT;
> >                 cp.git_cmd = 1;
> > diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
> > @@ -6,6 +6,16 @@ test_description='test git worktree add'
> > +# Is branch "refs/heads/$1" set to pull from "$2/$3"?
> > +test_branch_upstream () {
> > +       printf "%s\n" "$2" "refs/heads/$3" >expect.upstream &&
> > +       {
> > +               git config "branch.$1.remote" &&
> > +               git config "branch.$1.merge"
> > +       } >actual.upstream &&
> > +       test_cmp expect.upstream actual.upstream
> > +}
> 
> Not a big deal, but it wouldn't hurt to move this function down to the
> point where it is first needed...

Will do.

> >  test_expect_success 'setup' '
> >         test_commit init
> >  '
> > @@ -314,4 +324,40 @@ test_expect_success 'rename a branch under bisect not allowed' '
> > +test_expect_success '"add" <path> <non-existent-branch> fails' '
> > +       test_must_fail git worktree add foo non-existent
> > +'
> > +
> > +test_expect_success '"add" <path> <branch> dwims' '
> > +       test_when_finished rm -rf repo_upstream &&
> > +       test_when_finished rm -rf repo_dwim &&
> > +       test_when_finished rm -rf foo &&
> 
> Also not a big deal, but could all be combined into a single invocation:
> 
>     test_when_finished rm -rf repo_upstream repo_dwim foo &&

Thanks, will change!

Thanks a lot for the review!

> > +       git init repo_upstream &&
> > +       (
> > +               cd repo_upstream &&
> > +               test_commit upstream_master &&
> > +               git checkout -b foo &&
> > +               test_commit a_foo
> > +       ) &&
> > +       git init repo_dwim &&
> > +       (
> > +               cd repo_dwim &&
> > +               test_commit dwim_master &&
> > +               git remote add repo_upstream ../repo_upstream &&
> > +               git config remote.repo_upstream.fetch \
> > +                         "refs/heads/*:refs/remotes/repo_upstream/*" &&
> > +               git fetch --all &&
> > +               test_must_fail git worktree add -b foo ../foo foo &&
> > +               test_must_fail git worktree add --detach ../foo foo &&
> > +               git worktree add ../foo foo
> > +       ) &&
> > +       (
> > +               cd foo &&
> > +               test_branch_upstream foo repo_upstream foo &&
> > +               git rev-parse repo_upstream/foo >expect &&
> > +               git rev-parse foo >actual &&
> > +               test_cmp expect actual
> > +       )
> > +'