Re: [PATCH v4 8/9] checkout: add advice for ambiguous "checkout <branch>"

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Fri, Jun 01 2018, Junio C Hamano wrote:

> 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.

> 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*