[PATCH v2 0/2] Show suggested refs when ref unknown

[PATCH v2 0/2] Show suggested refs when ref unknown

[Vikrant Varma]

If origin/foo exists, but foo doesn't:

   $ git merge foo
   fatal: foo - not something we can merge

This patch series improves the error message. If a remote branch exists with the
same name, it now says:

     $ git merge foo
     fatal: foo - not something we can merge

     Did you mean this?
     	 origin/foo

It does this by adding a new help function, help_unknown_ref, that takes care of
printing the more friendly error message, and modifies builtin/merge.c to use it.

This function can easily be used by other operations involving refs that don't
exist instead of providing blanket failure error messages (eg. git checkout foo).

Vikrant Varma (2):
  help: add help_unknown_ref
  merge: use help_unknown_ref

 builtin/merge.c |    3 ++-
 help.c          |   50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 help.h          |    5 +++++
 3 files changed, 57 insertions(+), 1 deletion(-)

-- 
1.7.10.4

[PATCH v2 1/2] help: add help_unknown_ref

[Vikrant Varma]

When a ref is not known, currently functions call die() with an error message.
Add helper function help_unknown_ref to take care of displaying an error
message along with a list of suggested refs the user might have meant.

Example:
	$ git merge foo
	merge: foo - not something we can merge

	Did you mean one of these?
	    origin/foo
	    upstream/foo

Signed-off-by: Vikrant Varma <vikrant.varma94@gmail.com>
---
 help.c |   50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 help.h |    5 +++++
 2 files changed, 55 insertions(+)

diff --git a/help.c b/help.c
index 02ba043..fe5fe67 100644
--- a/help.c
+++ b/help.c
@@ -7,6 +7,7 @@
 #include "string-list.h"
 #include "column.h"
 #include "version.h"
+#include "refs.h"
 
 void add_cmdname(struct cmdnames *cmds, const char *name, int len)
 {
@@ -404,3 +405,52 @@ int cmd_version(int argc, const char **argv, const char *prefix)
 	printf("git version %s\n", git_version_string);
 	return 0;
 }
+
+struct similar_ref_cb {
+	const char *base_ref;
+	struct string_list *similar_refs;
+};
+
+static int append_similar_ref(const char *refname, const unsigned char *sha1,
+			      int flags, void *cb_data)
+{
+	struct similar_ref_cb *cb = (struct similar_ref_cb *)(cb_data);
+	char *branch = strrchr(refname, '/') + 1;
+	/* A remote branch of the same name is deemed similar */
+	if (!prefixcmp(refname, "refs/remotes/") &&
+	    !strcmp(branch, cb->base_ref))
+		string_list_append(cb->similar_refs,
+				   refname + strlen("refs/remotes/"));
+	return 0;
+}
+
+struct string_list guess_refs(const char *ref)
+{
+	struct similar_ref_cb ref_cb;
+	struct string_list similar_refs = STRING_LIST_INIT_NODUP;
+
+	ref_cb.base_ref = ref;
+	ref_cb.similar_refs = &similar_refs;
+	for_each_ref(append_similar_ref, &ref_cb);
+	return similar_refs;
+}
+
+void help_unknown_ref(const char *ref, const char *cmd, const char *error)
+{
+	int i;
+	struct string_list suggested_refs = guess_refs(ref);
+
+	fprintf_ln(stderr, _("%s: %s - %s"), cmd, ref, error);
+
+	if (suggested_refs.nr > 0) {
+		fprintf_ln(stderr,
+			   Q_("\nDid you mean this?",
+			      "\nDid you mean one of these?",
+			      suggested_refs.nr));
+		for (i = 0; i < suggested_refs.nr; i++)
+			fprintf(stderr, "\t%s\n", suggested_refs.items[i].string);
+	}
+
+	string_list_clear(&suggested_refs, 0);
+	exit(1);
+}
diff --git a/help.h b/help.h
index 0ae5a12..5003423 100644
--- a/help.h
+++ b/help.h
@@ -27,4 +27,9 @@ extern void exclude_cmds(struct cmdnames *cmds, struct cmdnames *excludes);
 extern int is_in_cmdlist(struct cmdnames *cmds, const char *name);
 extern void list_commands(unsigned int colopts, struct cmdnames *main_cmds, struct cmdnames *other_cmds);
 
+/*
+ * This function has been called because ref is not known.
+ * Print a list of refs that the user might have meant, and exit.
+ */
+extern void help_unknown_ref(const char *ref, const char *cmd, const char *error);
 #endif /* HELP_H */
-- 
1.7.10.4

[PATCH v2 2/2] merge: use help_unknown_ref

[Vikrant Varma]

Use help.c:help_unknown_ref instead of die to provide a friendlier error
message before exiting, when one of the refs specified in a merge is unknown.

Signed-off-by: Vikrant Varma <vikrant.varma94@gmail.com>
---
 builtin/merge.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 3e2daa3..2ebe732 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1054,7 +1054,8 @@ static struct commit_list *collect_parents(struct commit *head_commit,
 	for (i = 0; i < argc; i++) {
 		struct commit *commit = get_merge_parent(argv[i]);
 		if (!commit)
-			die(_("%s - not something we can merge"), argv[i]);
+			help_unknown_ref(argv[i], "merge",
+					 "not something we can merge");
 		remotes = &commit_list_insert(commit, remotes)->next;
 	}
 	*remotes = NULL;
-- 
1.7.10.4

Re: [PATCH v2 1/2] help: add help_unknown_ref

[Junio C Hamano]

Vikrant Varma <vikrant.varma94@gmail.com> writes:

> When a ref is not known, currently functions call die() with an
> error message.

The first part read somewhat awkward, so I started rewriting the
above like so:

    When the user gives an unknown string to a command that expects
    to see a ref, we could be more helpful than just saying "that's
    not a ref" and die.

which in turn made me realize that some commands may not even know
if the user mistyped a ref.  It is not an objection to this patch
per-se, but a useful future enhancement may be to allow the callers
call guess_mistyped_ref() directly and let them decide what to do
when they suspect the string they did not understand is not a
mistyped ref but something else, i.e. not let help_unknown_ref() die
unconditionally but allow it to return.  Then the caller can do:

        commit = get_commit_from_string(argv[i]);
        if (!commit) {
            ... I do not understand argv[i], but ...
            ... it may be a mistyped ref ...
            help_unknown_ref(argv[i], "expected a revision");
            ... it is not likely to be a typo ...
            ... perhaps it was meant to be a filename? ...
            if (file_exists(argv[i])) {
                ... yes! ...
                ... do the "file" thing instead ...
            }
        }

> Add helper function help_unknown_ref to take care of displaying an
> error message along with a list of suggested refs the user might
> have meant.

OK.

> Example:
> 	$ git merge foo
> 	merge: foo - not something we can merge

That leading "merge: " looks somewhat strange, especially when it
immediately follows the command line to invoke "merge", making it
appear to waste space by stating the obvious.

Our messages are generally marked with "error:", "fatal:",
"warning:", etc. at the beginning.

>
> 	Did you mean one of these?
> 	    origin/foo
> 	    upstream/foo
>
> Signed-off-by: Vikrant Varma <vikrant.varma94@gmail.com>

> ...
> +struct string_list guess_refs(const char *ref)
> +{
> +	struct similar_ref_cb ref_cb;
> +	struct string_list similar_refs = STRING_LIST_INIT_NODUP;
> +
> +	ref_cb.base_ref = ref;
> +	ref_cb.similar_refs = &similar_refs;
> +	for_each_ref(append_similar_ref, &ref_cb);
> +	return similar_refs;
> +}
> +
> +void help_unknown_ref(const char *ref, const char *cmd, const char *error)
> +{
> +	int i;
> +	struct string_list suggested_refs = guess_refs(ref);
> +
> +	fprintf_ln(stderr, _("%s: %s - %s"), cmd, ref, error);
> +
> +	if (suggested_refs.nr > 0) {
> +		fprintf_ln(stderr,
> +			   Q_("\nDid you mean this?",
> +			      "\nDid you mean one of these?",
> +			      suggested_refs.nr));
> +		for (i = 0; i < suggested_refs.nr; i++)
> +			fprintf(stderr, "\t%s\n", suggested_refs.items[i].string);
> +	}
> +
> +	string_list_clear(&suggested_refs, 0);
> +	exit(1);
> +}

Looks sensible.

> diff --git a/help.h b/help.h
> index 0ae5a12..5003423 100644
> --- a/help.h
> +++ b/help.h
> @@ -27,4 +27,9 @@ extern void exclude_cmds(struct cmdnames *cmds, struct cmdnames *excludes);
>  extern int is_in_cmdlist(struct cmdnames *cmds, const char *name);
>  extern void list_commands(unsigned int colopts, struct cmdnames *main_cmds, struct cmdnames *other_cmds);
>  
> +/*
> + * This function has been called because ref is not known.
> + * Print a list of refs that the user might have meant, and exit.
> + */

The wording is a bit funny; I'll amend it somehow before queuing.

> +extern void help_unknown_ref(const char *ref, const char *cmd, const char *error);
>  #endif /* HELP_H */

Re: [PATCH v2 1/2] help: add help_unknown_ref

[Vikrant Varma]

On Thursday 09 May 2013 04:19 AM, Junio C Hamano wrote:
 > [...] which in turn made me realize that some commands may not even know
 > if the user mistyped a ref.  It is not an objection to this patch
 > per-se, but a useful future enhancement may be to allow the callers
 > call guess_mistyped_ref() directly and let them decide what to do
 > when they suspect the string they did not understand is not a
 > mistyped ref but something else, i.e. not let help_unknown_ref() die
 > unconditionally but allow it to return.  Then the caller can do:
 >
 >          commit = get_commit_from_string(argv[i]);
 >          if (!commit) {
 >              ... I do not understand argv[i], but ...
 >              ... it may be a mistyped ref ...
 >              help_unknown_ref(argv[i], "expected a revision");
 >              ... it is not likely to be a typo ...
 >              ... perhaps it was meant to be a filename? ...
 >              if (file_exists(argv[i])) {
 >                  ... yes! ...
 >                  ... do the "file" thing instead ...
 >              }
 >          }
 >

I'm apprehensive about calling guess_mistyped_ref() (or it's equivalent, 
which happens to be guess_refs()) directly, because it doesn't seem like 
a clean enough separation. When the caller thinks it's got a bad 
refname, it should just hand it over to help_unknown_ref, for further 
processing.

If autocorrect is enabled, it can get back a single corrected refname 
(that is what my next patch will include - is it okay to base it on pu?).

If the need ever did arise to get that kind of information from 
help_unknown_ref, it could always be done using callback data?

	commit = get_commit_from_string(argv[i]);
	if (!commit) {
		... maybe mistyped ref, maybe something else ...
		struct unknown_ref_cb data;
		help_unknown_ref(argv[i], "expected something else",
				 &data);
		if (data.autocorrect)
			commit = get_commit_from_string(
					data.corrected_ref);
		else if (data.is_file)
			... do the file thing instead ...
    	}

I didn't see the need for this right away.

 >> Example:
 >> 	$ git merge foo
 >> 	merge: foo - not something we can merge
 >
 > That leading "merge: " looks somewhat strange, especially when it
 > immediately follows the command line to invoke "merge", making it
 > appear to waste space by stating the obvious.
 >
 > Our messages are generally marked with "error:", "fatal:",
 > "warning:", etc. at the beginning.

I agree, it looks strange. However the alternatives seem to be:

1) hard code 'fatal' into the error message
2) print the corrections before using die()
3) create and store the corrections string beforehand, and then call die()

1 and 3 are not elegant, and 2's output seems harder to read. I haven't 
been able to figure out a way to do this well.