Re: [PATCH v4 8/9] checkout: add advice for ambiguous "checkout <branch>"
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Jun 1, 2018, 09:54 UTC
- Message-ID
- <871sdqal2g.fsf@evledraar.gmail.com>
- In-Reply-To
- <xmqqefhr850j.fsf@gitster-ct.c.googlers.com>
On Fri, Jun 01 2018, Junio C Hamano wrote:
Show 37 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>>
>>> @@ -1269,6 +1270,16 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
>>> if (opts.patch_mode || opts.pathspec.nr) {
>>> int ret = checkout_paths(&opts, new_branch_info.name,
>>> &dwim_remotes_matched);
>>> + if (ret && dwim_remotes_matched > 1 &&
>>> + advice_checkout_ambiguous_remote_branch_name)
>>> + advise(_("The argument '%s' matched more than one remote tracking branch.\n"
>>> + "We found %d remotes with a reference that matched. So we fell back\n"
>>> + "on trying to resolve the argument as a path, but failed there too!\n"
>>> + "\n"
>>> + "Perhaps you meant fully qualify the branch name? E.g. origin/<name>\n"
>>> + "instead of <name>?"),
>>> + argv[0],
>>> + dwim_remotes_matched);
>>> return ret;
>>
>> Do we give "checkout -p no-such-file" the above wall of text?
>>
>> Somehow checkout_paths(), which is "we were given a tree-ish and
>> pathspec and told to grab the matching paths out of it and stuff
>> them to the index and the working tree", is a wrong place to be
>> doing the "oh, what the caller thought was pathspec may turn out to
>> be a rev, so check that too for such a confused caller". Shouldn't
>> the caller be doing all that (which would mean we wan't need to pass
>> "remotes-matched" to the function, as the helper has nothing to do
>> with deciding which arg is the tree-ish).
>
> Well, upon closer inspection adding *dwim_remotes_matched parameter
> to checkout_paths() done in an earlier step seems to be totally
> bogus and only serves the purpose of confusing reviewers. The
> function does not touch the pointer in any way---it does not use the
> pointer to return its findings, and it does not use an earlier
> findings to affect its behaviour by dereferencing it.Yes, sorry. I'll fix that. This was a relic from an earlier version of this that never escaped into the wild where I first tried printing out this error printing it out near the report_path_error() codepath called from checkout_paths().
I.e. I was trying to avoid printing out the "error: pathspec 'master' did not match any file(s) known to git." error altogether. That's still arguably a good direction, since we *know* "master" would have otherwise matched a remote branch, so that's probably a more informative message than falling back to checking out pathspecs and failing, and complaining about there being no such pathspec.
But it was a pain to handle the various edge cases, e.g.:
$ ./git --exec-path=$PWD checkout x y z
error: pathspec 'x' did not match any file(s) known to git.
error: pathspec 'y' did not match any file(s) known to git.
error: pathspec 'z' did not match any file(s) known to git.So I decided just to let checkout_paths() to its thing and then print out an error about dwim branches if applicable if it failed.
Show 5 quoted lines
> The dwim_remotes_matched is set by an earlier call to > parse_branchname_arg(), which does gain an int* parameter in this > series. And that addition _does_ make sense. That codepath is > where the "do we have many remotes that could match, or none, or > unique?" determination is made.
*nod*