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

Junio C Hamano [off-list ref] writes:

Ævar Arnfjörð Bjarmason  [off-list ref] writes:

quoted

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

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.

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

Junio C Hamano [off-list ref] writes:

quoted

Ævar Arnfjörð Bjarmason  [off-list ref] writes:

quoted

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