Re: [PATCH v6 3/3] worktree add: improve message for ambiguous remote branch name
Yoichi Nakayama <[email protected]>
| Newsgroups | org.kernel.vger.git |
|---|---|
| Message-ID | <CAF5D8-vDzp9xhu96Tu0ScmWRHvVyi74MD0NhPMnQ9ayoy6h+wQ@mail.gmail.com> |
On Fri, Aug 21, 2026 at 12:54 PM Junio C Hamano <[email protected]> wrote: > > "Yoichi NAKAYAMA via GitGitGadget" <[email protected]> writes: > > > From: Yoichi NAKAYAMA <[email protected]> > > > > diff --git a/builtin/worktree.c b/builtin/worktree.c > > index 22c8e5e131..8286c283e0 100644 > > --- a/builtin/worktree.c > > +++ b/builtin/worktree.c > > @@ -788,6 +788,25 @@ static char *dwim_branch(const char *path, char **new_branch) > > return NULL; > > } > > > > +static void advise_disambiguating_remotes(const char *path, const char *branch, > > + const struct string_list *matched_remote_names) > > +{ > > + struct string_list_item *item; > > + > > + advise(_("Branches with the same name appears in multiple remotes:")); > > The subject "Branches" calls for plural verb "appear" (not > "appears"). The same issue appears in [PATCH 2/3]. I overlooked that. Thank you. Rather than simply matching the verb to the subject, I want to clarify what (as specified by the user) exists on multiple remotes: advise(_("Branch name '%s' appears in multiple remotes:"), branch); > > if (!commit) { > > - remote = unique_tracking_name(branch, &oid, NULL, NULL); > > + char *remote; > > + int num_matches = 0; > > + struct string_list matched_remote_names = STRING_LIST_INIT_DUP; > > + > > + remote = unique_tracking_name(branch, &oid, &num_matches, > > + &matched_remote_names); > > if (remote) { > > new_branch = branch; > > branch = new_branch_to_free = remote; > > + } else if (num_matches > 1) { > > + if (!opts.quiet && > > + advice_enabled(ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME)) > > + advise_disambiguating_remotes(path, branch, > > + &matched_remote_names); > > + die(_("'%s' matched multiple (%d) remote tracking branches"), > > + branch, num_matches); > > } > > + string_list_clear(&matched_remote_names, 0); > > } > > This appears inside "} else if (ac == 2) {" to catch an invocation > like > > git worktree add ../over-there topic-branch > > where the origin of topic-branch is ambiguous (in other words, > appears in multiple remotes). But don't we have the same issue for > 1 argument case that appears just above this (ac == 2) case that > handles > > git worktree add ../topic-branch > > invocation? The code reads like: > > } else if (ac < 2) { > /* DWIM: Guess branch name from path. */ > char *s = dwim_branch(path, &new_branch_to_free); > if (s) > branch = branch_to_free = s; > new_branch = new_branch_to_free; > > /* DWIM: Infer --orphan when repo has no refs. */ > opts.orphan = (!s) && dwim_orphan(&opts, !!opt_track, 1); > } else if (ac == 2) { > > where the branch name "topic-branch" is guessed from the path by > calling dwim_branch(), and we would get NULL in s. branch is left > as-is, so it becomes "HEAD" that was assigned much earlier in the > same function. > > branch = ac < 2 ? "HEAD" : av[1]; > > We would create a new directory in ../topic-branch next door, and > then which branch would we check out? Would dwim_orphan() kick in? > > Perhaps we want to update that code path to disambiguate the same way? In the case of git worktree add ../topic-branch invocation, multiple match can occur in dwim_branch() if there is a 'worktree.guessremote=true' config or one specifies '--guess-remote' option.Then it creates a branch named 'topic-branch' from HEAD, and the command exits with success. My initial patch included a warning and advice here, but now I don't think they are necessary. Even if multiple remotes match here, the command completes successfully. This could well be the intended behavior (just as when there is no match). In that case, a warning or advice might be superfluous. From the perspective of offering advice that actually helps the user, since the branch and worktree have already been created, the appropriate guidance would be to suggest deleting them and starting over. That, however, would likely make the message even longer. If there were an option (which currently doesn't exist) to make the command fail when remote inference fails, then I think it would be appropriate to issue the same advice and error message as in "ac == 2" case. Thanks, -- Yoichi NAKAYAMA