Re: [PATCH v2 3/3] worktree: teach "add" to check out existing branches

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

On 02/05, Duy Nguyen wrote:
> On Sun, Feb 04, 2018 at 10:13:05PM +0000, Thomas Gummerer wrote:
> > -	if (opts->new_branch)
> > +	if (opts->checkout_existing_branch)
> > +		fprintf(stderr, _(", checking out existing branch '%s'"),
> > +			refname);
> > +	else if (opts->new_branch)
> >  		fprintf(stderr, _(", creating new branch '%s'"), opts->new_branch);
> 
> I wonder if "creating branch" and "checkout out branch" are enough.

I thought printing the branch name might be a good idea just to show
more clearly what the dwim did.  Especially if we ever introduce new
heuristics, for example to cater for the case you mentioned in an
earlier email [1], I thought it might be helpful to also print the
branch name.  Especially because it makes it clear that the dwim the
reporter of the github issue hoped for didn't kick in at this point.

That said, I don't really think the branchname is of much use for my
usage of 'git worktree add', so I may be overestimating the need for
it.  I'm happy to remove it if that's preferred.

[1]: <CACsJy8D9LS7e=cVE3Fq2qOnxK5++nFg2vjuhkNtRO-Bx0X1j6w@mail.gmail.com>

> > @@ -423,14 +427,25 @@ static int add(int ac, const char **av, const char *prefix)
> >  	if (ac < 2 && !opts.new_branch && !opts.detach) {
> >  		int n;
> >  		const char *s = worktree_basename(path, &n);
> > -		opts.new_branch = xstrndup(s, n);
> > -		if (guess_remote) {
> > -			struct object_id oid;
> > -			const char *remote =
> > -				unique_tracking_name(opts.new_branch, &oid);
> > -			if (remote)
> > -				branch = remote;
> > +		const char *branchname = xstrndup(s, n);
> > +		struct strbuf ref = STRBUF_INIT;
> > +
> > +		if (!strbuf_check_branch_ref(&ref, branchname) &&
> > +		    ref_exists(ref.buf)) {
> > +			branch = branchname;
> > +			opts.checkout_existing_branch = 1;
> > +			UNLEAK(branch);
> > +		} else {
> > +			opts.new_branch = branchname;
> > +			if (guess_remote) {
> > +				struct object_id oid;
> > +				const char *remote =
> > +					unique_tracking_name(opts.new_branch, &oid);
> 
> Deep indentation may be a sign that it's time to move all this code to
> a separate function, maybe dwim_branch() or something.

Makes sense, I'll factor it out.  Thanks!

> > +				if (remote)
> > +					branch = remote;
> > +			}
> >  		}
> > +		strbuf_release(&ref);
> >  	}
> >  
> >  	if (ac == 2 && !opts.new_branch && !opts.detach) {