Re: [PATCH v1 2/2] worktree: make add dwim

From: Thomas Gummerer <t.gummerer@gmail.com>
Date: Nov 14, 2017, 08:45 UTC
Message-ID: <20171114084517.GA12097@hank>
In-Reply-To: <xmqq1sl2c21g.fsf@gitster.mtv.corp.google.com>

On 11/13, Junio C Hamano wrote:

Show 33 quoted lines

> Thomas Gummerer <t.gummerer@gmail.com> writes:
> 
> > I'm a bit torn about hiding his behind an additional flag in git
> > worktree add or not.  I would like to have the feature without the
> > additional flag, but it might break some peoples expectations, so
> > dunno.
> 
> Yeah, I agree with the sentiment.  But what expectation would we be
> breaking, I wonder (see below)?
> 
> At the conceptual level, it is even more unfortunate that "git
> worktree --help" says this for "git worktree add <path> [<branch>]":
> 
>     If `<branch>` is omitted and neither `-b` nor `-B` nor
>     `--detach` used, then, as a convenience, a new branch based at
>     HEAD is created automatically, as if `-b $(basename <path>)` was
>     specified.
> 
> which means that it already does a DWIM, namely "since you didn't
> say what branch to create to track what other branch, we'll fork one
> for you from the HEAD, and give a name to it".  That may be a useful
> DWIM for some existing users sometimes, and you may even find it
> useful some of the time but not always.  Different people mean
> different things in different situations, and there is no single
> definition for DWIMming that would fit all of them.
> 
> Which in turn means that the new variable name DWIM_NEW_BRANCH and
> the new option name GUESS are way underspecified.  You'd need to
> name them after the "kind" of dwim; otherwise, not only the future
> additions for new (third) kind of dwim would become cumbersome, but
> readers of the code would be confused if they are about the existing
> dwim (fork a new branch from HEAD and give it a name guessed by the
> pathname) or your new dwim.

Yeah I definitely agree with that. I was mainly thinking about my personal use case with --guess, and didn't fully think through that it might mean different things to other people.

> This also needs a documentation update.  Unlike the existing DWIM,
> it is totally unclear when you are dwimming in absence of which
> option.

Sorry about that, I totally forgot about the documentation for this. Will add in the next round.

Show 14 quoted lines

> I am guessing that, when you do not have a branch "topic" but your
> upstream does, saying
> 
>     $ git worktree add ../a-new-worktree topic
> 
> would create "refs/heads/topic" to build on top of
> "refs/remotes/origin/topic" just like "git checkout topic" would.
> 
> IOW, when fully spelled out:
> 
>     $ git branch -t -b topic origin/topic
>     $ git worktree add ../a-new-worktree topic
> 
> is what your DWIM does?  Am I following you correctly?

It's not quite what my DWIM does/what I originally intended my DWIM to do. What it does is when

    $ git worktree add ../topic

and omitting the branch argument, it would behave the way you spelled out, i.e.

    $ git branch -t -b topic origin/topic
    $ git worktree add ../topic topic

instead of just checking it out from HEAD. I must confess I missed the branch argument, so that still behaves the old way with my code, while what you have above would make a lot more sense.

Show 7 quoted lines

> If so, as long as the new DWIM kicks in ONLY when "topic" does not
> exist, I suspect that there is no backward compatibility worries.
> The command line
> 
>     $ git worktree add ../a-new-worktree topic
> 
> would just have failed because three was no 'topic' branch yet, no?

Indeed, with this there would not be any backwards compatility worries.

Ideally I'd still like to make

    $ git worktree add ../topic

work as described above, to avoid having to type 'topic' twice (the directory name matches the branch name for the vast majority of my worktrees) but that should then come behind a flag/config option? Following your reasoning above it should probably be called something other than --guess.

Maybe --guess-remote and worktree.guessRemote would do? I'm quite bad at naming though, so other suggestions are very welcome.