git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Thomas Gummerer <t.gummerer@gmail.com>
To: Eric Sunshine <sunshine@sunshineco.com>
Cc: "Git List" <git@vger.kernel.org>,
	"Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>,
	"Junio C Hamano" <gitster@pobox.com>
Subject: Re: [PATCH v3 2/3] worktree: make add <path> <branch> dwim
Date: Sun, 19 Nov 2017 17:43:45 +0000	[thread overview]
Message-ID: <20171119174345.GA24891@hank> (raw)
In-Reply-To: <CAPig+cS-eQGXU8YKsOAP_RE55GreqLJXPooddpzewUYUZeP6LQ@mail.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
> > +       )
> > +'

  reply	other threads:[~2017-11-19 17:42 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-11-12 13:43 [PATCH v1 1/2] checkout: factor out functions to new lib file Thomas Gummerer
2017-11-12 13:43 ` [PATCH v1 2/2] worktree: make add dwim Thomas Gummerer
2017-11-13  3:04   ` Junio C Hamano
2017-11-14  8:45     ` Thomas Gummerer
2017-11-14 20:14       ` Eric Sunshine
2017-11-14 20:29         ` Eric Sunshine
2017-11-15  8:52           ` Thomas Gummerer
2017-11-18 18:13             ` Thomas Gummerer
2017-11-15  8:50         ` Thomas Gummerer
2017-11-15  9:12           ` Eric Sunshine
2017-11-13  2:41 ` [PATCH v1 1/2] checkout: factor out functions to new lib file Junio C Hamano
2017-11-14  8:46   ` Thomas Gummerer
2017-11-18 18:11 ` [PATCH v2 1/3] " Thomas Gummerer
2017-11-18 18:11   ` [PATCH v2 2/3] worktree: make add <path> <branch> dwim Thomas Gummerer
2017-11-18 22:18     ` Thomas Gummerer
2017-11-18 18:11   ` [PATCH v2 3/3] worktree: make add <path> dwim Thomas Gummerer
2017-11-18 22:47   ` [PATCH v3 0/3] make git worktree add dwim more Thomas Gummerer
2017-11-18 22:47     ` [PATCH v3 1/3] checkout: factor out functions to new lib file Thomas Gummerer
2017-11-18 22:47     ` [PATCH v3 2/3] worktree: make add <path> <branch> dwim Thomas Gummerer
2017-11-19  8:31       ` Eric Sunshine
2017-11-19 17:43         ` Thomas Gummerer [this message]
2017-11-18 22:47     ` [PATCH v3 3/3] worktree: make add <path> dwim Thomas Gummerer
2017-11-19 19:04       ` Eric Sunshine
2017-11-19 20:20         ` Eric Sunshine
2017-11-20  0:39           ` Junio C Hamano
2017-11-21 22:13           ` Thomas Gummerer
2017-11-22  1:20             ` Junio C Hamano
2017-11-22 19:49               ` Thomas Gummerer

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20171119174345.GA24891@hank \
    --to=t.gummerer@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=pclouds@gmail.com \
    --cc=sunshine@sunshineco.com \
    --subject='Re: [PATCH v3 2/3] worktree: make add <path> <branch> dwim' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).