Re: [PATCH v6 3/3] worktree add: improve message for ambiguous remote branch name

[Junio C Hamano <gitster@pobox.com>]

Yoichi Nakayama <yoichi.nakayama@gmail.com> writes:

> No. The exit codes of the command 'git worktree add ../topic-branch'
> are the same (== 0). but the results are different.
>
> If there is a unique match found in dwim_branch(), it creates a local
> branch named topic-branch which tracks <remote>/topic-branch.
> In case of no match or multiple matches, it creates a local branch
> named topic-branch from HEAD.
>
> Since Git treats both cases as successful, either can be considered
> the intended behavior.
> (Although, if there are multiple matches, there is a fair chance the
> result might not be what was intended.)
>
> I am confident that it is appropriate to provide a hint when a command
> fails, but it is difficult to decide what to do when a command succeeds.

I actually think it falls into the same class of bug you are fixing
in this topic, which was caused by not considering the possibility
that there can be any case other than 0-match and 1-match, and not
thinking through the ramifications of treating 2-match and 0-match
the same way.

It is of course OK to fix one bug and leave the other one
unaddressed, to be fixed in a later follow-up effort.

The rest of this message is only for those who will tackle the
"later follow-up effort" part after the dust settles once the
current topic lands (aka #leftoverbits).

In the beginning, before Thomas Gummerer started his topic in
November 2017 [*1*], 'git worktree add <path> [<branch>]' created a
new branch from the checked-out HEAD, without looking at any
remote.

 - 'git worktree add <path> <branch>' before Thomas's effort errored
   out if <branch> did not exist.  It was safe to add DWIM from
   remote-tracking branches without requiring any option.

 - 'git worktree add <path>' used to create a new branch whose name
   is derived from basename(path) that points at the current HEAD,
   without erroring out.  Enabling DWIM from remote-tracking
   branches unconditionally would have meant a silent behavior
   change.  So DWIM was added to this case to require the
   '--guess-remote' option to enable [*2*].

Back then, unique_tracking_name() did not let the callers
distinguish between 0-match and multiple-match cases, so when you
had multiple matches, 'git worktree add <path> [<branch>]' triggered
the same code path as 0-matches.  When the DWIM feature was
designed, handling the multiple-match case correctly was on nobody's
radar.

Even when Ævar Arnfjörð Bjarmason updated unique_tracking_name() in
3c87aa946a (checkout: pass the "num_matches" up to callers,
2018-06-05), in a topic that ends at 8d7b558bae (checkout &
worktree: introduce checkout.defaultRemote, 2018-06-05), to allow
callers to distinguish between 0-match and ambiguous multi-match
cases, this work unfortunately concentrated on improving "git
checkout", and callers of unique_tracking_name() in "git worktree"
were updated to pass NULL, i.e., teaching them to count how many
matches they got was postponed.

We know that the update to unique_tracking_name() in this work back
then was not complete on the "git worktree" side.  After all, that
is how this topic arose to fix one of the two code paths that call
the function so that we react differently between 0-match and
multiple-match cases.

Now that we are aware of the issue, I think the code should error
out, instead of creating the new branch out of HEAD, when there are
multiple remotes with the name of the branch.  In other words, the
existing code that behaves the same way in 0-match and 2-match cases
is buggy, and we should eventually fix it.


[Footnotes]

 *1* https://lore.kernel.org/git/20171112134305.3949-1-t.gummerer@gmail.com/
 *2* https://lore.kernel.org/git/20171126194356.16187-1-t.gummerer@gmail.com/