worktree add: improve message for ambiguous remote branch name - #2197
Open
yoichi wants to merge 3 commits into
Open
worktree add: improve message for ambiguous remote branch name#2197yoichi wants to merge 3 commits into
yoichi wants to merge 3 commits into
Conversation
yoichi
force-pushed
the
improve-worktree-add-error-message
branch
2 times, most recently
from
August 8, 2026 06:34
63d9b6b to
00b814f
Compare
Author
|
/submit |
|
Submitted as pull.2197.git.1786177301832.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
Junio C Hamano wrote on the Git mailing list (how to reply to this email): "Yoichi NAKAYAMA via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Yoichi NAKAYAMA <yoichi.nakayama@gmail.com>
>
> Display a descriptive message when DWIM fails.
>
> Add advice on how to work around this by specifying the fully
> qualified name or by setting checkout.defaultRemote.
>
> Signed-off-by: Yoichi NAKAYAMA <yoichi.nakayama@gmail.com>
> ---
> worktree add: improve message for ambiguous remote branch name
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2197%2Fyoichi%2Fimprove-worktree-add-error-message-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2197/yoichi/improve-worktree-add-error-message-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/2197
>
> builtin/worktree.c | 30 ++++++++++++++++++++++++++----
> t/t2400-worktree-add.sh | 21 +++++++++++++++++++--
> 2 files changed, 45 insertions(+), 6 deletions(-)
>
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 654d27c3e1..46bc305116 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -116,6 +116,16 @@ static const char * const git_worktree_unlock_usage[] = {
> NULL
> };
>
> +static const char message_advice_ambiguous_remote_tracking_branch[] =
> + N_("If you meant to create a worktree from a remote tracking branch on,\n"
> + "e.g. 'origin', you can do so by fully qualifying the name:\n"
> + "\n"
> + " git worktree add <path> origin/<name>\n"
> + "\n"
This is shown in two places, but what did the user exactly type in
these two situations? Can their intent be different, in which case
different suggestions might be more appropriate to each of them?
Let's see.
> @@ -781,8 +791,14 @@ static char *dwim_branch(const char *path, char **new_branch)
>
> *new_branch = branchname;
> if (guess_remote) {
> + int num_matches = 0;
> struct object_id oid;
> - char *remote = unique_tracking_name(*new_branch, &oid, NULL);
> + char *remote = unique_tracking_name(*new_branch, &oid, &num_matches);
> + if (!opts->quiet && !remote && num_matches > 1) {
> + if (advice_enabled(ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME))
> + advise(_(message_advice_ambiguous_remote_tracking_branch));
> + warning(_("'%s' matched multiple (%d) remote tracking branches\n"), branchname, num_matches);
> + }
> return remote;
> }
The worktree.guessremote configuration is set. dwim_branch() is
called when "git worktree add A/B/X" is run with a single argument
"A/B/X", which comes here as "path", and that is munged into the
branchname "X".
We used to pass NULL as the second parameter to unique_tracking_name(),
so we were only interested in the case where we have exactly one
matching remote, and if there is 0 or multiple remotes with the
named branch, we returned NULL from here.
The patch does not change that, but using the branch name, we try to
see if there are multiple matches, in that case, we give the advice
message to say "hey, don't be so lazy, as X appears in more than one
remote, so tell me which one you mean".
> @@ -890,7 +906,7 @@ static int add(int ac, const char **av, const char *prefix,
> opts.orphan = dwim_orphan(&opts, !!opt_track, 0);
> } else if (ac < 2) {
> /* DWIM: Guess branch name from path. */
> - char *s = dwim_branch(path, &new_branch_to_free);
> + char *s = dwim_branch(&opts, path, &new_branch_to_free);
> if (s)
> branch = branch_to_free = s;
> new_branch = new_branch_to_free;
But shouldn't we do a bit better than
git worktree add <path> origin/<name>
The above makes the user think that just like 'git', 'worktree' and
'add', 'origin/' is a fixed part, and they would need to substitute
<path> and <name>, but that is not really what we want to tell them.
The most crucial part to correct is 'origin/', as that is what we
could not guess from the given information.
We know that the user gave us "A/B/X" (path) and probably they want
to create local "X" from it. Or not. We also should know, in
caller's opt_track and used_new_branch_options, that the user gave
us "-t -b Y" from the command line.
> @@ -904,10 +920,16 @@ static int add(int ac, const char **av, const char *prefix,
>
> commit = lookup_commit_reference_by_name(branch);
> if (!commit) {
> - remote = unique_tracking_name(branch, &oid, NULL);
> + int num_matches = 0;
> + remote = unique_tracking_name(branch, &oid, &num_matches);
> 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(_(message_advice_ambiguous_remote_tracking_branch));
> + }
> + die(_("'%s' matched multiple (%d) remote tracking branches"), branch, num_matches);
Style: overly long line, with {braces} around a single statement block.
What does this case handle? Can you make a similar analysis to come
up with the list of things we know the user gave us, to give a bit
better command line to suggest here?
> }
> }
Thanks. |
|
Junio C Hamano wrote on the Git mailing list (how to reply to this email): Junio C Hamano <gitster@pobox.com> writes:
>> +static const char message_advice_ambiguous_remote_tracking_branch[] =
>> + N_("If you meant to create a worktree from a remote tracking branch on,\n"
>> + "e.g. 'origin', you can do so by fully qualifying the name:\n"
>> + "\n"
>> + " git worktree add <path> origin/<name>\n"
>> + "\n"
>> ...
>> + char *remote = unique_tracking_name(*new_branch, &oid, &num_matches);
>> + if (!opts->quiet && !remote && num_matches > 1) {
>> + if (advice_enabled(ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME))
>> + advise(_(message_advice_ambiguous_remote_tracking_branch));
>> + warning(_("'%s' matched multiple (%d) remote tracking branches\n"), branchname, num_matches);
>> + }
>> return remote;
>> }
>
> The worktree.guessremote configuration is set. dwim_branch() is
> called when "git worktree add A/B/X" is run with a single argument
> "A/B/X", which comes here as "path", and that is munged into the
> branchname "X".
>
> We used to pass NULL as the second parameter to unique_tracking_name(),
> so we were only interested in the case where we have exactly one
> matching remote, and if there is 0 or multiple remotes with the
> named branch, we returned NULL from here.
>
> The patch does not change that, but using the branch name, we try to
> see if there are multiple matches, in that case, we give the advice
> message to say "hey, don't be so lazy, as X appears in more than one
> remote, so tell me which one you mean".
Stepping back a bit, I think what I find lacking in the proposed
warning message is not that we lose what the user gave us, such as
'-b <branch>' or '-t'. While this loss makes it impossible to
simply copy and paste to reproduce what the user may have intended,
it is not the end of the world.
What disturbs me more is that the code holds back information only
it possesses, which would immediately help the user if we shared it.
The reason we got this error may not be that the user did not know
exactly how to spell out the necessary information (such as which
branch to use from which remote) on the command line. It may be
that the user did not remember some of the necessary details (such
as which remotes have the branch they have in mind). Displaying
the command line and advising them to use the fully qualified name
might not be the best approach in that case. Telling them that
they may have meant 'origin', 'upstream', or 'home' (all of which
are remotes with the named branch, though we could not guess which
one of the three to choose) may be much more helpful. |
|
Harald Nordgren wrote on the Git mailing list (how to reply to this email): This is an interesting idea!
Harald |
|
User |
|
Junio C Hamano wrote on the Git mailing list (how to reply to this email): Junio C Hamano <gitster@pobox.com> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>>> +static const char message_advice_ambiguous_remote_tracking_branch[] =
>>> + N_("If you meant to create a worktree from a remote tracking branch on,\n"
>>> + "e.g. 'origin', you can do so by fully qualifying the name:\n"
>>> + "\n"
>>> + " git worktree add <path> origin/<name>\n"
>>> + "\n"
>>> ...
>>> + char *remote = unique_tracking_name(*new_branch, &oid, &num_matches);
>>> + if (!opts->quiet && !remote && num_matches > 1) {
>>> + if (advice_enabled(ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME))
>>> + advise(_(message_advice_ambiguous_remote_tracking_branch));
>>> + warning(_("'%s' matched multiple (%d) remote tracking branches\n"), branchname, num_matches);
>>> + }
Sorry for piecemeal reviews, but I just noticed that you have a
terminating LF at the end of a single-liner warning message. As
die/error/warning ffamily of helpers give the terminating newline
themselves, you must not. Unless you want to leave a blank line
after your message, that is.
Thanks.
|
|
Junio C Hamano wrote on the Git mailing list (how to reply to this email): Harald Nordgren <haraldnordgren@gmail.com> writes:
> This is an interesting idea!
>
>
> Harald
When expressing your opinion on what another said, quote a bit from
the message you are responding to so that people know what you are
referring to. I cannot easily tell which part of what I said you
found interesting.
Thanks. |
|
Harald Nordgren wrote on the Git mailing list (how to reply to this email): > When expressing your opinion on what another said, quote a bit from
> the message you are responding to so that people know what you are
> referring to. I cannot easily tell which part of what I said you
> found interesting.
Sorry, yes this was about:
> Telling them that
> they may have meant 'origin', 'upstream', or 'home' (all of which
> are remotes with the named branch, though we could not guess which
> one of the three to choose) may be much more helpful.
Harald |
yoichi
force-pushed
the
improve-worktree-add-error-message
branch
from
August 10, 2026 13:38
00b814f to
1bc57ce
Compare
|
Yoichi Nakayama wrote on the Git mailing list (how to reply to this email): On Sun, Aug 9, 2026 at 2:00 AM Junio C Hamano <gitster@pobox.com> wrote:
> > @@ -781,8 +791,14 @@ static char *dwim_branch(const char *path, char **new_branch)
> >
> > *new_branch = branchname;
> > if (guess_remote) {
> > + int num_matches = 0;
> > struct object_id oid;
> > - char *remote = unique_tracking_name(*new_branch, &oid, NULL);
> > + char *remote = unique_tracking_name(*new_branch, &oid, &num_matches);
> > + if (!opts->quiet && !remote && num_matches > 1) {
> > + if (advice_enabled(ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME))
> > + advise(_(message_advice_ambiguous_remote_tracking_branch));
> > + warning(_("'%s' matched multiple (%d) remote tracking branches\n"), branchname, num_matches);
> > + }
> > return remote;
> > }
>
> The worktree.guessremote configuration is set. dwim_branch() is
> called when "git worktree add A/B/X" is run with a single argument
> "A/B/X", which comes here as "path", and that is munged into the
> branchname "X".
>
> We used to pass NULL as the second parameter to unique_tracking_name(),
> so we were only interested in the case where we have exactly one
> matching remote, and if there is 0 or multiple remotes with the
> named branch, we returned NULL from here.
>
> The patch does not change that, but using the branch name, we try to
> see if there are multiple matches, in that case, we give the advice
> message to say "hey, don't be so lazy, as X appears in more than one
> remote, so tell me which one you mean".
I thought the problem here was that it was impossible to distinguish whether
the guess was successful, but it was not true. We can distinguish by
the message:
branch 'name' set up to track 'remote/name'.
I will not make changes to this part.
Thanks,
--
Yoichi NAKAYAMA |
|
User |
|
Yoichi Nakayama wrote on the Git mailing list (how to reply to this email): On Sun, Aug 9, 2026 at 6:57 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> >> +static const char message_advice_ambiguous_remote_tracking_branch[] =
> >> + N_("If you meant to create a worktree from a remote tracking branch on,\n"
> >> + "e.g. 'origin', you can do so by fully qualifying the name:\n"
> >> + "\n"
> >> + " git worktree add <path> origin/<name>\n"
> >> + "\n"
> >> ...
> >> + char *remote = unique_tracking_name(*new_branch, &oid, &num_matches);
> >> + if (!opts->quiet && !remote && num_matches > 1) {
> >> + if (advice_enabled(ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME))
> >> + advise(_(message_advice_ambiguous_remote_tracking_branch));
> >> + warning(_("'%s' matched multiple (%d) remote tracking branches\n"), branchname, num_matches);
> >> + }
> >> return remote;
> >> }
> >
> > The worktree.guessremote configuration is set. dwim_branch() is
> > called when "git worktree add A/B/X" is run with a single argument
> > "A/B/X", which comes here as "path", and that is munged into the
> > branchname "X".
> >
> > We used to pass NULL as the second parameter to unique_tracking_name(),
> > so we were only interested in the case where we have exactly one
> > matching remote, and if there is 0 or multiple remotes with the
> > named branch, we returned NULL from here.
> >
> > The patch does not change that, but using the branch name, we try to
> > see if there are multiple matches, in that case, we give the advice
> > message to say "hey, don't be so lazy, as X appears in more than one
> > remote, so tell me which one you mean".
>
> Stepping back a bit, I think what I find lacking in the proposed
> warning message is not that we lose what the user gave us, such as
> '-b <branch>' or '-t'. While this loss makes it impossible to
> simply copy and paste to reproduce what the user may have intended,
> it is not the end of the world.
>
> What disturbs me more is that the code holds back information only
> it possesses, which would immediately help the user if we shared it.
>
> The reason we got this error may not be that the user did not know
> exactly how to spell out the necessary information (such as which
> branch to use from which remote) on the command line. It may be
> that the user did not remember some of the necessary details (such
> as which remotes have the branch they have in mind). Displaying
> the command line and advising them to use the fully qualified name
> might not be the best approach in that case. Telling them that
> they may have meant 'origin', 'upstream', or 'home' (all of which
> are remotes with the named branch, though we could not guess which
> one of the three to choose) may be much more helpful.
I realized that instead of placing a burden on the user, we should
present a solution.
When a multiple match occurs, the only decision the user needs to make
is which remote to select.
For everything else, the hint should give a specific command with
arguments that achieve the same
behavior as when exactly one remote matches.
Rather than presenting a list of candidates, I think it is preferable
to explain how to generate that list.
This allows users to process the list e.g. by piping it into a command.
I'll submit an updated patch.
Thanks,
--
Yoichi NAKAYAMA |
|
"D. Ben Knoble" wrote on the Git mailing list (how to reply to this email): Hi Yoichi,
On Sat, Aug 8, 2026 at 4:21 AM Yoichi NAKAYAMA via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Yoichi NAKAYAMA <yoichi.nakayama@gmail.com>
>
> Display a descriptive message when DWIM fails.
>
> Add advice on how to work around this by specifying the fully
> qualified name or by setting checkout.defaultRemote.
>
> Signed-off-by: Yoichi NAKAYAMA <yoichi.nakayama@gmail.com>
> ---
[snip]
> -static char *dwim_branch(const char *path, char **new_branch)
> +static char *dwim_branch(const struct add_opts *opts, const char *path, char **new_branch)
> {
> int n;
> int branch_exists;
> @@ -781,8 +791,14 @@ static char *dwim_branch(const char *path, char **new_branch)
>
> *new_branch = branchname;
> if (guess_remote) {
> + int num_matches = 0;
> struct object_id oid;
> - char *remote = unique_tracking_name(*new_branch, &oid, NULL);
> + char *remote = unique_tracking_name(*new_branch, &oid, &num_matches);
> + if (!opts->quiet && !remote && num_matches > 1) {
> + if (advice_enabled(ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME))
> + advise(_(message_advice_ambiguous_remote_tracking_branch));
> + warning(_("'%s' matched multiple (%d) remote tracking branches\n"), branchname, num_matches);
> + }
> return remote;
> }
> return NULL;
I suppose the extra warning won't hurt anyone's workflow :) so that's good.
[snip]
> @@ -904,10 +920,16 @@ static int add(int ac, const char **av, const char *prefix,
>
> commit = lookup_commit_reference_by_name(branch);
> if (!commit) {
> - remote = unique_tracking_name(branch, &oid, NULL);
> + int num_matches = 0;
> + remote = unique_tracking_name(branch, &oid, &num_matches);
> 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(_(message_advice_ambiguous_remote_tracking_branch));
> + }
> + die(_("'%s' matched multiple (%d) remote tracking branches"), branch, num_matches);
> }
> }
We would now die() here where we didn't before. I'm not suggesting
that is wrong (I haven't given it much thought), but I was surprised
to see it in the code without mention in the message, which I've left
quoted above. In particular, the proposed log message talks about
giving new advice, so I wasn't expecting us to abort.
Now, it may be that this case already causes an error later on (I
haven't analyzed that), in which case dying early with a better
diagnostic is definitely helpful. If that's the case, it would be nice
to spell that out for the rest of us :)
If not, I would want to know why we can die() here without bothering
anyone's workflow that is expecting us to carry on.
Thanks!
--
D. Ben Knoble |
|
User |
|
Yoichi Nakayama wrote on the Git mailing list (how to reply to this email): On Mon, Aug 10, 2026 at 10:08 PM D. Ben Knoble <ben.knoble@gmail.com> wrote:
> > @@ -904,10 +920,16 @@ static int add(int ac, const char **av, const char *prefix,
> >
> > commit = lookup_commit_reference_by_name(branch);
> > if (!commit) {
> > - remote = unique_tracking_name(branch, &oid, NULL);
> > + int num_matches = 0;
> > + remote = unique_tracking_name(branch, &oid, &num_matches);
> > 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(_(message_advice_ambiguous_remote_tracking_branch));
> > + }
> > + die(_("'%s' matched multiple (%d) remote tracking branches"), branch, num_matches);
> > }
> > }
>
> We would now die() here where we didn't before. I'm not suggesting
> that is wrong (I haven't given it much thought), but I was surprised
> to see it in the code without mention in the message, which I've left
> quoted above. In particular, the proposed log message talks about
> giving new advice, so I wasn't expecting us to abort.
>
> Now, it may be that this case already causes an error later on (I
> haven't analyzed that), in which case dying early with a better
> diagnostic is definitely helpful. If that's the case, it would be nice
> to spell that out for the rest of us :)
>
> If not, I would want to know why we can die() here without bothering
> anyone's workflow that is expecting us to carry on.
Before the change, it calles lookup_commit_reference_by_name() again
in the if condition and die() at:
if (!opts.orphan && !lookup_commit_reference_by_name(branch)) {
/* snip */
die(_("invalid reference: %s"), branch);
}
The motivation for the fix was that this error message did not
accurately reflect the situation.
Thanks,
--
Yoichi NAKAYAMA |
Author
|
/submit |
|
Submitted as pull.2197.v2.git.1786374470383.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
Junio C Hamano wrote on the Git mailing list (how to reply to this email): Yoichi Nakayama <yoichi.nakayama@gmail.com> writes:
> Before the change, it calles lookup_commit_reference_by_name() again
> in the if condition and die() at:
>
> if (!opts.orphan && !lookup_commit_reference_by_name(branch)) {
> /* snip */
> die(_("invalid reference: %s"), branch);
> }
>
> The motivation for the fix was that this error message did not
> accurately reflect the situation.
The location of this die() is a tad away from the places that the
patch touched. The proposed log message could be made a bit more
helpful by mentioning it. What was posted reads:
Display a descriptive message when DWIM fails.
Add advice on how to work around this by specifying the fully
qualified name or by setting checkout.defaultRemote.
but telling the readers what they will see instead of a descriptive
message and how that happens would be very helpful to understand why
it is a good idea to die early. Perhaps
When the user runs 'git worktree add x y z' command that does
not exactly say which remote they want to work with, we try to
guess which remote by passing y. If there are multiple remotes
that have branch named y, we silently gave up, leaving remote
still NULL. This later causes A and B not happen, and we end up
with passing an non-existing branch to
lookup_commit_reference_by_name(), triggering "invalid
reference" error and die.
or something like that that describes the issue to a similar degree
as above mock-up message.
Thanks. |
yoichi
force-pushed
the
improve-worktree-add-error-message
branch
2 times, most recently
from
August 10, 2026 20:48
5ad82c4 to
1b9364d
Compare
Author
|
/submit |
|
Submitted as pull.2197.v3.git.1786395305884.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
Yoichi Nakayama wrote on the Git mailing list (how to reply to this email): On Mon, Aug 10, 2026 at 10:08 PM D. Ben Knoble <ben.knoble@gmail.com> wrote:
> > -static char *dwim_branch(const char *path, char **new_branch)
> > +static char *dwim_branch(const struct add_opts *opts, const char *path, char **new_branch)
> > {
> > int n;
> > int branch_exists;
> > @@ -781,8 +791,14 @@ static char *dwim_branch(const char *path, char **new_branch)
> >
> > *new_branch = branchname;
> > if (guess_remote) {
> > + int num_matches = 0;
> > struct object_id oid;
> > - char *remote = unique_tracking_name(*new_branch, &oid, NULL);
> > + char *remote = unique_tracking_name(*new_branch, &oid, &num_matches);
> > + if (!opts->quiet && !remote && num_matches > 1) {
> > + if (advice_enabled(ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME))
> > + advise(_(message_advice_ambiguous_remote_tracking_branch));
> > + warning(_("'%s' matched multiple (%d) remote tracking branches\n"), branchname, num_matches);
> > + }
> > return remote;
> > }
> > return NULL;
>
> I suppose the extra warning won't hurt anyone's workflow :) so that's good.
I removed the change (advise and warn) here in the latest patch. But I am still
wondering what I should do. I think a warning would be excessive if
there is no match,
but the user might want to know if there are multiple matches.
Thanks,
--
Yoichi NAKAYAMA |
|
Junio C Hamano wrote on the Git mailing list (how to reply to this email): "Yoichi NAKAYAMA via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Yoichi NAKAYAMA <yoichi.nakayama@gmail.com>
>
> When the user runs 'git worktree add x y' command that does not
> exactly say which remote they want to work with, and there is no local
> branch named y, we try to guess which remote by passing y then create
> a new branch named y which tracks the remote branch.
I used x and y as placeholders. The readers would be helped if you
used a more plausible sounding names, e.g., naming directory as
something like foo-dir (the point being 'dir' somewhere in its name)
and naming a branch as something like bar-topic. If this were 'git
worktree add', it is probably more than likely that the destination
directory would begin with ../ to have the new worktree next to the
primary repository we are running in, no?
> If there are multiple remotes that have branch named y, we silently
> gave up, leaving the variable branch intact. This later causes
> creating local branch and worktree not happen, and we end up with
> passing an non-existing branch to lookup_commit_reference_by_name(),
> triggering "invalid reference" error and die.
"This later causes" part still seems a bit too sketchy to help a
totally new reader, even though I've stared at this code long enough
so it would be sufficient for me personally. But these logs are not
about helping me, but helping other developers, so...
> +#define WORKTREE_ADD_AMBIGUOUS_REMOTE_BRANCH_NAME_HINT_TEXT \
> + _("Matched multiple remote tracking branches, you can list them by:\n" \
> + "\n" \
> + " git branch -r --list \"*/%s\"\n" \
> + "\n" \
> + "If you meant to create a worktree from a remote tracking branch on,\n" \
> + "e.g. 'origin', you can do so by:\n" \
> + "\n" \
> + " git worktree add -b %s %s origin/%s\n" \
> + "\n" \
> + "If you'd like to always prefer some remote, e.g. 'origin',\n" \
> + "consider setting checkout.defaultRemote=origin in your config.")
Instead of throwing the problem back to the user with four extra
lines of message telling them how to run 'git branch', I would have
expected this patch to teach unique_tracking_name() to optionally
return the list of remotes with that branch name, and to use that
result in this message. However, if the goal is simply to provide
something better than 'invalid reference', we do not even need to
go that far. Just stating that branch 'y' appears on multiple
remotes and asking them to clarify which one they mean might be a
sufficient improvement.
Could the original request be aiming to create a new worktree with
the HEAD detached at the commit pointed at by the remote-tracking
branch, instead of creating a local branch forked from it? I am
just wondering if "-b %s" is too specific to one possible
interpretation that may contradict to what the user actually wanted
to do.
Thanks.
|
yoichi
force-pushed
the
improve-worktree-add-error-message
branch
from
August 11, 2026 06:07
1b9364d to
f7c413b
Compare
Author
|
/submit |
|
Submitted as pull.2197.v4.git.1786430155244.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
This patch series was integrated into seen via git@4ceff92. |
|
Junio C Hamano wrote on the Git mailing list (how to reply to this email): Yoichi Nakayama <yoichi.nakayama@gmail.com> writes:
>> Instead of throwing the problem back to the user with four extra
>> lines of message telling them how to run 'git branch', I would have
>> expected this patch to teach unique_tracking_name() to optionally
>> return the list of remotes with that branch name, and to use that
>> result in this message. However, if the goal is simply to provide
>> something better than 'invalid reference', we do not even need to
>> go that far. Just stating that branch 'y' appears on multiple
>> remotes and asking them to clarify which one they mean might be a
>> sufficient improvement.
>
> Extending `unique_tracking_name()` would also affect the implementation
> in `checkout.c`, and since the goal here is to improve the messages
> (making them as helpful as those in `checkout`), I will hold off on doing
> that for now.
Sorry but I do not quite understand this logic.
Giving unique_tracking_name() the optional ability to report which
remotes have a branch with the given name does not have to affect
other callers of the function at all; that is the definition of a
new feature being "optional."
Furthermore, the goal of improving these messages falls short if we
withhold the list of remotes the user could have meant, which we are
already computing internally to decide that the original request is
ambiguous.
>> Could the original request be aiming to create a new worktree with
>> the HEAD detached at the commit pointed at by the remote-tracking
>> branch, instead of creating a local branch forked from it? I am
>> just wondering if "-b %s" is too specific to one possible
>> interpretation that may contradict to what the user actually wanted
>> to do.
>
> If the user is aiming to create a new worktree with the HEAD detached,
> one would specify a fully qualified branch name like origin/bar-topic,
> starting with a remote name.
I am not sure about this part, either. After all, we are trying to
help a user who made a mistake composing their command-line
arguments. If they specify a fully qualified branch name like
'origin/bar-topic' (regardless of whether they want to create a
detached 'HEAD' or not), they would not hit the 'saying bar-topic
alone is ambiguous' error path, would they?
Here is a patch to show what I mean. To 'improve the message for an
ambiguous remote branch name' for the 'checkout' command, we can add
this optional feature to the unique_tracking_name() function and we
can do so without affecting the 'worktree add' command.
Regardless of whether we modify unique_tracking_name(), I think
refactoring be_explicit() is worth doing, as the original code in
parse_remote_branch() was badly misindented.
builtin/checkout.c | 74 ++++++++++++++++++++++++++++++++----------------------
builtin/worktree.c | 4 +--
checkout.c | 14 +++++++++--
checkout.h | 5 +++-
4 files changed, 62 insertions(+), 35 deletions(-)
diff --git c/builtin/checkout.c w/builtin/checkout.c
index 55e3a89a85..8d61973a11 100644
--- c/builtin/checkout.c
+++ w/builtin/checkout.c
@@ -1343,13 +1343,50 @@ enum checkout_command {
CHECKOUT_RESTORE = 3,
};
+static void be_explicit(enum checkout_command which_command,
+ const struct string_list *matched_remote_names)
+{
+ const char *cmdname;
+ struct string_list_item *item;
+
+ switch (which_command) {
+ case CHECKOUT_CHECKOUT:
+ cmdname = "checkout";
+ break;
+ case CHECKOUT_SWITCH:
+ cmdname = "switch";
+ break;
+ default:
+ BUG("command <%d> should not reach parse_remote_branch",
+ which_command);
+ break;
+ }
+
+ advise(_("Branches with the same name appears in multiple remotes:"));
+ for_each_string_list_item(item, matched_remote_names) {
+ advise(_(" %s"), item->string);
+ }
+ advise(_("If you meant to check out a remote tracking branch on, e.g. 'origin',\n"
+ "you can do so by fully qualifying the name with the --track option:\n"
+ "\n"
+ " git %s --track origin/<name>\n"
+ "\n"
+ "If you'd like to always have checkouts of an ambiguous <name> prefer\n"
+ "one remote, e.g. the 'origin' remote, consider setting\n"
+ "checkout.defaultRemote=origin in your config."),
+ cmdname);
+}
+
static char *parse_remote_branch(const char *arg,
struct object_id *rev,
int could_be_checkout_paths,
enum checkout_command which_command)
{
int num_matches = 0;
- char *remote = unique_tracking_name(arg, rev, &num_matches);
+ struct string_list matched_remote_names = STRING_LIST_INIT_DUP;
+
+ char *remote = unique_tracking_name(arg, rev, &num_matches,
+ &matched_remote_names);
if (remote && could_be_checkout_paths) {
die(_("'%s' could be both a local file and a tracking branch.\n"
@@ -1358,37 +1395,14 @@ static char *parse_remote_branch(const char *arg,
}
if (!remote && num_matches > 1) {
- if (advice_enabled(ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME)) {
- const char *cmdname;
-
- switch (which_command) {
- case CHECKOUT_CHECKOUT:
- cmdname = "checkout";
- break;
- case CHECKOUT_SWITCH:
- cmdname = "switch";
- break;
- default:
- BUG("command <%d> should not reach parse_remote_branch",
- which_command);
- break;
- }
-
- advise(_("If you meant to check out a remote tracking branch on, e.g. 'origin',\n"
- "you can do so by fully qualifying the name with the --track option:\n"
- "\n"
- " git %s --track origin/<name>\n"
- "\n"
- "If you'd like to always have checkouts of an ambiguous <name> prefer\n"
- "one remote, e.g. the 'origin' remote, consider setting\n"
- "checkout.defaultRemote=origin in your config."),
- cmdname);
- }
-
- die(_("'%s' matched multiple (%d) remote tracking branches"),
- arg, num_matches);
+ if (advice_enabled(ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME))
+ be_explicit(which_command, &matched_remote_names);
+ die(_("'%s' matched multiple (%d) remote tracking branches"),
+ arg, num_matches);
}
+ string_list_clear(&matched_remote_names, 0);
+
return remote;
}
diff --git c/builtin/worktree.c w/builtin/worktree.c
index 654d27c3e1..22c8e5e131 100644
--- c/builtin/worktree.c
+++ w/builtin/worktree.c
@@ -782,7 +782,7 @@ static char *dwim_branch(const char *path, char **new_branch)
*new_branch = branchname;
if (guess_remote) {
struct object_id oid;
- char *remote = unique_tracking_name(*new_branch, &oid, NULL);
+ char *remote = unique_tracking_name(*new_branch, &oid, NULL, NULL);
return remote;
}
return NULL;
@@ -904,7 +904,7 @@ static int add(int ac, const char **av, const char *prefix,
commit = lookup_commit_reference_by_name(branch);
if (!commit) {
- remote = unique_tracking_name(branch, &oid, NULL);
+ remote = unique_tracking_name(branch, &oid, NULL, NULL);
if (remote) {
new_branch = branch;
branch = new_branch_to_free = remote;
diff --git c/checkout.c w/checkout.c
index 1588b116ee..2806b783ec 100644
--- c/checkout.c
+++ w/checkout.c
@@ -8,6 +8,7 @@
#include "checkout.h"
#include "config.h"
#include "strbuf.h"
+#include "string-list.h"
struct tracking_name_data {
/* const */ char *src_ref;
@@ -17,6 +18,7 @@ struct tracking_name_data {
const char *default_remote;
char *default_dst_ref;
struct object_id *default_dst_oid;
+ struct string_list **remote_names;
};
#define TRACKING_NAME_DATA_INIT { 0 }
@@ -39,6 +41,8 @@ static int check_tracking_name(struct remote *remote, void *cb_data)
oidcpy(dst, cb->dst_oid);
cb->default_dst_oid = dst;
}
+ if (cb->remote_names)
+ string_list_append(*cb->remote_names, remote->name);
if (cb->dst_ref) {
free(query.dst);
return 0;
@@ -48,14 +52,20 @@ static int check_tracking_name(struct remote *remote, void *cb_data)
}
char *unique_tracking_name(const char *name, struct object_id *oid,
- int *dwim_remotes_matched)
+ int *dwim_remotes_matched,
+ struct string_list *dwim_remote_names)
{
struct tracking_name_data cb_data = TRACKING_NAME_DATA_INIT;
const char *default_remote = NULL;
- if (!repo_config_get_string_tmp(the_repository, "checkout.defaultremote", &default_remote))
+
+ if (!repo_config_get_string_tmp(the_repository,
+ "checkout.defaultremote",
+ &default_remote))
cb_data.default_remote = default_remote;
cb_data.src_ref = xstrfmt("refs/heads/%s", name);
cb_data.dst_oid = oid;
+ if (dwim_remote_names)
+ cb_data.remote_names = &dwim_remote_names;
for_each_remote(check_tracking_name, &cb_data);
if (dwim_remotes_matched)
*dwim_remotes_matched = cb_data.num_matches;
diff --git c/checkout.h w/checkout.h
index 55920e7aeb..0b185a0fc9 100644
--- c/checkout.h
+++ w/checkout.h
@@ -3,6 +3,8 @@
#include "hash.h"
+struct string_list;
+
/*
* Check if the branch name uniquely matches a branch name on a remote
* tracking branch. Return the name of the remote if such a branch
@@ -10,6 +12,7 @@
*/
char *unique_tracking_name(const char *name,
struct object_id *oid,
- int *dwim_remotes_matched);
+ int *dwim_remotes_matched,
+ struct string_list *dwim_remote_names);
#endif /* CHECKOUT_H */
|
|
There was a status update in the "Cooking" section about the branch 'git worktree add' did not prevent DWIM behavior when '-b' or '-B' was specified, which has been corrected. Waiting for response. cf. <xmqqjypvw3cg.fsf@gitster.g> source: <pull.2197.v4.git.1786430155244.gitgitgadget@gmail.com> |
|
There was a status update in the "Cooking" section about the branch 'git worktree add' did not prevent DWIM behavior when '-b' or '-B' was specified, which has been corrected. Waiting for response. cf. <xmqqjypvw3cg.fsf@gitster.g> source: <pull.2197.v4.git.1786430155244.gitgitgadget@gmail.com> |
|
Yoichi Nakayama wrote on the Git mailing list (how to reply to this email): On Thu, Aug 13, 2026 at 4:22 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Yoichi Nakayama <yoichi.nakayama@gmail.com> writes:
>
> >> Instead of throwing the problem back to the user with four extra
> >> lines of message telling them how to run 'git branch', I would have
> >> expected this patch to teach unique_tracking_name() to optionally
> >> return the list of remotes with that branch name, and to use that
> >> result in this message. However, if the goal is simply to provide
> >> something better than 'invalid reference', we do not even need to
> >> go that far. Just stating that branch 'y' appears on multiple
> >> remotes and asking them to clarify which one they mean might be a
> >> sufficient improvement.
> >
> > Extending `unique_tracking_name()` would also affect the implementation
> > in `checkout.c`, and since the goal here is to improve the messages
> > (making them as helpful as those in `checkout`), I will hold off on doing
> > that for now.
>
> Sorry but I do not quite understand this logic.
>
> Giving unique_tracking_name() the optional ability to report which
> remotes have a branch with the given name does not have to affect
> other callers of the function at all; that is the definition of a
> new feature being "optional."
>
> Furthermore, the goal of improving these messages falls short if we
> withhold the list of remotes the user could have meant, which we are
> already computing internally to decide that the original request is
> ambiguous.
That is certainly true.
I think the modification to `unique_tracking_name()` that you shared is good.
> >> Could the original request be aiming to create a new worktree with
> >> the HEAD detached at the commit pointed at by the remote-tracking
> >> branch, instead of creating a local branch forked from it? I am
> >> just wondering if "-b %s" is too specific to one possible
> >> interpretation that may contradict to what the user actually wanted
> >> to do.
> >
> > If the user is aiming to create a new worktree with the HEAD detached,
> > one would specify a fully qualified branch name like origin/bar-topic,
> > starting with a remote name.
>
> I am not sure about this part, either. After all, we are trying to
> help a user who made a mistake composing their command-line
> arguments. If they specify a fully qualified branch name like
> 'origin/bar-topic' (regardless of whether they want to create a
> detached 'HEAD' or not), they would not hit the 'saying bar-topic
> alone is ambiguous' error path, would they?
If the user specifies a fully qualified branch name like
git worktree add ../foo-dir origin/bar-topic
the 2nd argument 'origin/bar-topic' will be resolved by
commit = lookup_commit_reference_by_name(branch);
before calling unique_tracking_name(), so the ambiguous warning
won't occur.
In a worst-case scenario, a user might make a typo in the 2nd argument (e.g.
origin/bar-topik) that fails to match a local ref (e.g.
refs/remotes/origin/bar-topic)
but accidentally matches multiple remote branch names (e.g. branches related to
refs/remotes/remote1/origin/bar-topik and
refs/remotes/remote2/origin/bar-topik),
then it could cause an issue. However, I think this is an edge case that doesn't
need to be considered.
Thanks,
--
Yoichi NAKAYAMA |
|
There was a status update in the "Cooking" section about the branch 'git worktree add' did not prevent DWIM behavior when '-b' or '-B' was specified, which has been corrected. Expecting a reroll. cf. <CAF5D8-tfAfjNONh8dbqxmF+kzzaQ4rSQL94DK2=TLd9_HV9oaA@mail.gmail.com> source: <pull.2197.v4.git.1786430155244.gitgitgadget@gmail.com> |
|
There was a status update in the "Cooking" section about the branch 'git worktree add' did not prevent DWIM behavior when '-b' or '-B' was specified, which has been corrected. Expecting a reroll. cf. <CAF5D8-tfAfjNONh8dbqxmF+kzzaQ4rSQL94DK2=TLd9_HV9oaA@mail.gmail.com> source: <pull.2197.v4.git.1786430155244.gitgitgadget@gmail.com> |
yoichi
force-pushed
the
improve-worktree-add-error-message
branch
from
August 19, 2026 11:35
f7c413b to
7778622
Compare
Author
|
/preview |
|
Preview email sent as pull.2197.v5.git.1787143681.gitgitgadget@gmail.com |
Author
|
/submit |
|
Submitted as pull.2197.v5.git.1787143859.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -1343,13 +1343,51 @@ enum checkout_command { | |||
| CHECKOUT_RESTORE = 3, | |||
There was a problem hiding this comment.
"D. Ben Knoble" wrote on the Git mailing list (how to reply to this email):
On Wed, Aug 19, 2026 at 8:51 AM Yoichi NAKAYAMA via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Yoichi NAKAYAMA <yoichi.nakayama@gmail.com>
>
> When the user runs 'git checkout bar-topic' command that does not
> exactly say which remote they want to work with, and there is no local
> branch named bar-topic, we try to guess which remote by passing
> bar-topic then create a new branch named bar-topic which tracks the
> remote branch.
>
> If multiple remotes have a branch named bar-topic, we cannot determine
> a single specific remote. Therefore, we provide information that the
> user can utilize to resolve the issue.
>
> To make the advice more feasible, we will provide matched remote names
> for the specified branch name.
>
> To achive that, we add an optional feature to the
> `unique_tracking_name()` function that allows the matched remote name
> to be exposed to the caller.
>
> Signed-off-by: Yoichi NAKAYAMA <yoichi.nakayama@gmail.com>
> ---
> builtin/checkout.c | 75 +++++++++++++++++++++++++++-------------------
> builtin/worktree.c | 4 +--
> checkout.c | 14 +++++++--
> checkout.h | 5 +++-
> 4 files changed, 63 insertions(+), 35 deletions(-)
>
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 55e3a89a85..a2749352e6 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -1343,13 +1343,51 @@ enum checkout_command {
> CHECKOUT_RESTORE = 3,
> };
>
> +static void be_explicit(const char *branch,
Be explicit about what? Reading below, a better name might be
"advise_ambiguous_remote_branch_name" or something, idk.
> + enum checkout_command which_command,
> + const struct string_list *matched_remote_names)
> +{
> + const char *cmdname;
> + struct string_list_item *item;
> +
> + switch (which_command) {
> + case CHECKOUT_CHECKOUT:
> + cmdname = "checkout";
> + break;
> + case CHECKOUT_SWITCH:
> + cmdname = "switch";
> + break;
> + default:
> + BUG("command <%d> should not reach parse_remote_branch",
> + which_command);
> + break;
> + }
> +
> + advise(_("Branches with the same name appears in multiple remotes:"));
> + for_each_string_list_item(item, matched_remote_names) {
> + advise(_(" %s"), item->string);
> + }
> + advise(_("If you meant to check out a remote tracking branch on <remote>,\n"
> + "you can do so by fully qualifying the name with the --track option:\n"
> + "\n"
> + " git %s --track <remote>/%s\n"
> + "\n"
> + "If you'd like to always have checkouts of an ambiguous name prefer\n"
> + "one remote, e.g. the 'origin' remote, consider setting\n"
> + "checkout.defaultRemote=origin in your config."),
> + cmdname, branch);
> +}
> +
I think it's possible this refactor is a bit distracting from the
overall goal of the patch, though I don't think extracting the
function is a bad thing. Maybe split the steps up into
- mechanical refactoring (no behavior change)
- changes to improve the message (easier to see the diff)
? Just my 2 cents.
> static char *parse_remote_branch(const char *arg,
> struct object_id *rev,
> int could_be_checkout_paths,
> enum checkout_command which_command)
> {
> int num_matches = 0;
> - char *remote = unique_tracking_name(arg, rev, &num_matches);
> + struct string_list matched_remote_names = STRING_LIST_INIT_DUP;
> +
> + char *remote = unique_tracking_name(arg, rev, &num_matches,
> + &matched_remote_names);
>
> if (remote && could_be_checkout_paths) {
> die(_("'%s' could be both a local file and a tracking branch.\n"
> @@ -1358,37 +1396,14 @@ static char *parse_remote_branch(const char *arg,
> }
>
> if (!remote && num_matches > 1) {
> - if (advice_enabled(ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME)) {
> - const char *cmdname;
> -
> - switch (which_command) {
> - case CHECKOUT_CHECKOUT:
> - cmdname = "checkout";
> - break;
> - case CHECKOUT_SWITCH:
> - cmdname = "switch";
> - break;
> - default:
> - BUG("command <%d> should not reach parse_remote_branch",
> - which_command);
> - break;
> - }
> -
> - advise(_("If you meant to check out a remote tracking branch on, e.g. 'origin',\n"
> - "you can do so by fully qualifying the name with the --track option:\n"
> - "\n"
> - " git %s --track origin/<name>\n"
> - "\n"
> - "If you'd like to always have checkouts of an ambiguous <name> prefer\n"
> - "one remote, e.g. the 'origin' remote, consider setting\n"
> - "checkout.defaultRemote=origin in your config."),
> - cmdname);
> - }
> -
> - die(_("'%s' matched multiple (%d) remote tracking branches"),
> - arg, num_matches);
> + if (advice_enabled(ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME))
> + be_explicit(arg, which_command, &matched_remote_names);
> + die(_("'%s' matched multiple (%d) remote tracking branches"),
> + arg, num_matches);
> }
>
> + string_list_clear(&matched_remote_names, 0);
> +
> return remote;
> }
[rest of diff snipped]There was a problem hiding this comment.
Junio C Hamano wrote on the Git mailing list (how to reply to this email):
"D. Ben Knoble" <ben.knoble@gmail.com> writes:
>> +static void be_explicit(const char *branch,
>
> Be explicit about what? Reading below, a better name might be
> "advise_ambiguous_remote_branch_name" or something, idk.
It stands for "tell the user to be more explicit". I agree with you
that the refactoring should be done as a separate step, on top of
which we should add the new feature, i.e., "give list of possible
candidates", as a separate step.There was a problem hiding this comment.
Yoichi Nakayama wrote on the Git mailing list (how to reply to this email):
On Thu, Aug 20, 2026 at 11:18 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "D. Ben Knoble" <ben.knoble@gmail.com> writes:
>
> >> +static void be_explicit(const char *branch,
> >
> > Be explicit about what? Reading below, a better name might be
> > "advise_ambiguous_remote_branch_name" or something, idk.
>
> It stands for "tell the user to be more explicit". I agree with you
> that the refactoring should be done as a separate step, on top of
> which we should add the new feature, i.e., "give list of possible
> candidates", as a separate step.
I think it's a good idea to separate the steps, so I'll split the commits.
I am considering "advise_disambiguating_remotes" as a better function name.
Thanks,
--
Yoichi NAKAYAMA
Signed-off-by: Yoichi NAKAYAMA <yoichi.nakayama@gmail.com>
yoichi
force-pushed
the
improve-worktree-add-error-message
branch
2 times, most recently
from
August 20, 2026 15:33
93ad277 to
dcb84a6
Compare
Author
|
/submit |
|
Submitted as pull.2197.v6.git.1787259838.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
This patch series is no longer integrated into seen. |
|
There was a status update in the "Cooking" section about the branch The DWIM logic in 'git worktree add' sometimes tried to infer a remote-tracking branch when an explicit '-b' or '-B' option was given to create a new branch, causing the explicit branch name to be ignored, which has been corrected. Will merge to 'next'? cf. <xmqqecfssnk0.fsf@gitster.g> source: <pull.2192.v4.git.1787221888406.gitgitgadget@gmail.com> |
| char *remote = unique_tracking_name(*new_branch, &oid, NULL, NULL); | ||
| return remote; | ||
| } | ||
| return NULL; |
There was a problem hiding this comment.
Junio C Hamano wrote on the Git mailing list (how to reply to this email):
"Yoichi NAKAYAMA via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Yoichi NAKAYAMA <yoichi.nakayama@gmail.com>
>
> When the user runs 'git worktree add ../foo-dir bar-topic' command
> that does not exactly say which remote they want to work with, and
> there is no local branch named bar-topic, we try to guess which remote
> by passing bar-topic then create a new branch named bar-topic which
> tracks the remote branch.
>
> If there are multiple remotes that have branch named bar-topic, we
> silently gave up, leaving the variable 'branch' intact. Then we
> entered the conditional clause 'if (!opts.orphan &&
> !lookup_commit_reference_by_name(branch))' and triggered "invalid
> reference" error. This error message did not contain enough
> information to resolve the issue where the remote could not be
> guessed.
>
> To improve the situation, we display a hint and a descriptive error
> message and die immediately when multiple matching branches are found.
>
> Signed-off-by: Yoichi NAKAYAMA <yoichi.nakayama@gmail.com>
> ---
> builtin/worktree.c | 35 +++++++++++++++++++++++++++++++++--
> t/t2400-worktree-add.sh | 4 ++--
> 2 files changed, 35 insertions(+), 4 deletions(-)
>
> 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].
> 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?
> diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
> index 87b926728a..5c105cf252 100755
> --- a/t/t2400-worktree-add.sh
> +++ b/t/t2400-worktree-add.sh
> @@ -624,12 +624,12 @@ test_expect_success '"add" <path> <branch> dwims' '
> test_expect_success '"add" <path> <branch> dwims with checkout.defaultRemote' '
> test_when_finished rm -rf repo_upstream repo_dwim foo &&
> setup_remote_repo repo_upstream repo_dwim &&
> - git init repo_dwim &&
> (
> cd repo_dwim &&
> git remote add repo_upstream2 ../repo_upstream &&
> git fetch repo_upstream2 &&
> - test_must_fail git worktree add ../foo foo &&
> + test_must_fail git worktree add ../foo foo 2>error.actual &&
> + test_grep "matched multiple (2) remote tracking branches" error.actual &&
> git -c checkout.defaultRemote=repo_upstream worktree add ../foo foo &&
> git status -uno --porcelain >status.actual &&
> test_must_be_empty status.actualThere was a problem hiding this comment.
Yoichi Nakayama wrote on the Git mailing list (how to reply to this email):
On Fri, Aug 21, 2026 at 12:54 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Yoichi NAKAYAMA via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Yoichi NAKAYAMA <yoichi.nakayama@gmail.com>
> >
> > 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 NAKAYAMAThere was a problem hiding this comment.
Junio C Hamano wrote on the Git mailing list (how to reply to this email):
Yoichi Nakayama <yoichi.nakayama@gmail.com> writes:
> 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.
In other words, there is no point in calling dwim_branch() from that
code path, as the end result is exactly the same whether no remotes
match, exactly one remote matches, or two or more remotes match?
Would it then make sense to leave a note there to consider later if
the dwim_branch() call can be removed?
It is a bit hard to believe that is the intended behavior, but OK.
It does not regress the current behavior in any way.
Thanks.
There was a problem hiding this comment.
Yoichi Nakayama wrote on the Git mailing list (how to reply to this email):
On Sat, Aug 22, 2026 at 8:49 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Yoichi Nakayama <yoichi.nakayama@gmail.com> writes:
>
> > 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.
>
> In other words, there is no point in calling dwim_branch() from that
> code path, as the end result is exactly the same whether no remotes
> match, exactly one remote matches, or two or more remotes match?
> Would it then make sense to leave a note there to consider later if
> the dwim_branch() call can be removed?
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.
Thanks,
--
Yoichi NAKAYAMAThere was a problem hiding this comment.
Junio C Hamano wrote on the Git mailing list (how to reply to this email):
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/
yoichi
force-pushed
the
improve-worktree-add-error-message
branch
from
August 21, 2026 23:44
dcb84a6 to
55bafe3
Compare
When the user runs 'git checkout bar-topic' without specifying a remote, and there is no local branch named bar-topic, we try to guess which remote branch bar-topic refers to, then create a new branch named bar-topic that tracks the remote branch. If multiple remotes have a branch named bar-topic, we cannot determine a single remote. To make it easier to resolve the ambiguity, provide the names of the matching remotes for the specified branch name. To achieve that, add an optional feature to the `unique_tracking_name()` function that allows the matching remote names to be exposed to the caller. Signed-off-by: Yoichi NAKAYAMA <yoichi.nakayama@gmail.com>
When the user runs 'git worktree add ../foo-dir bar-topic' without specifying a remote, and there is no local branch named bar-topic, we try to guess which remote branch bar-topic refers to, then create a new branch named bar-topic that tracks the remote branch. If multiple remotes have a branch named bar-topic, we silently gave up, leaving the variable 'branch' intact. We then entered the conditional clause 'if (!opts.orphan && !lookup_commit_reference_by_name(branch))' and triggered an "invalid reference" error. This error message did not provide enough information to resolve the ambiguity. When multiple matching branches are found, display a hint and a descriptive error message and die immediately. Signed-off-by: Yoichi NAKAYAMA <yoichi.nakayama@gmail.com>
yoichi
force-pushed
the
improve-worktree-add-error-message
branch
from
August 22, 2026 01:53
55bafe3 to
095a579
Compare
Author
|
/submit |
|
Submitted as pull.2197.v7.git.1787368962.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
Junio C Hamano wrote on the Git mailing list (how to reply to this email): "Yoichi NAKAYAMA via GitGitGadget" <gitgitgadget@gmail.com> writes:
> * fix grammatical errors in hints
> * narrow the scope of local variable oid
Both changes look sensible. I very much like the new advise()
message that is much more concise.
> -+ advise(_("Branches with the same name appears in multiple remotes:"));
> ++ advise(_("Branch name '%s' appears in multiple remotes:"), branch);
> -+ advise(_("Branches with the same name appears in multiple remotes:"));
> ++ advise(_("Branch name '%s' appears in multiple remotes:"), branch);
Will queue. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
'git worktree add ../foo-dir bar-topic' fails to dwim when there are multiple remote branches with name `bar-topic'. But it doesn't display meaningful message as 'git checkout bar-topic' does under the same situation.
We improve this by adding advice and modify the error message for worktree add.
By Junio's suggestion, we include matched remote names in the
advice. It is applied to checkout, too.
Changes from the previous patch:
cc: Harald Nordgren haraldnordgren@gmail.com
cc: Yoichi Nakayama yoichi.nakayama@gmail.com
cc: "D. Ben Knoble" ben.knoble@gmail.com