[PATCH 0/2] Better advice on merge

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. In the future, this could also be extended to other operations
involving branches that don't exist locally, instead of providing blanket
failure error messages (eg. git checkout foo).

Vikrant Varma (2):
  help: add help_unknown_ref
  merge: use help_unknown_ref instead of die

 builtin/merge.c |  4 ++--
 help.c          | 44 ++++++++++++++++++++++++++++++++++++++++++++
 help.h          |  6 ++++++
 3 files changed, 52 insertions(+), 2 deletions(-)

-- 
1.8.3-rc0

Give better advice when trying to merge a branch that doesn't exist. If
the branch exists in any remotes, display a list of suggestions.

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

	Did you mean this?
	    bar/foo

Signed-off-by: Vikrant Varma <redacted>
---
 help.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 help.h |  6 ++++++
 2 files changed, 50 insertions(+)

diff --git a/help.c b/help.c
index 02ba043..faf18b9 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,46 @@ 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)
+{
+        int i;
+        struct similar_ref_cb *cb = (struct similar_ref_cb *)(cb_data);
+        for (i = strlen(refname); refname[i] != '/'; i--)
+	        ;
+        /* A remote branch of the same name is deemed similar */
+        if (!prefixcmp(refname, "refs/remotes/") && !strcmp(refname + i + 1, cb->base_ref))
+	       	string_list_append(&(cb->similar_refs), refname + 13);
+        return 0;
+}
+
+void help_unknown_ref(const char* ref) {
+        int i;
+        struct similar_ref_cb ref_cb;
+        ref_cb.similar_refs = (struct string_list)STRING_LIST_INIT_NODUP;
+        ref_cb.base_ref = ref;
+
+        for_each_ref(append_similar_ref, &ref_cb);
+
+        fprintf_ln(stderr, _("fatal: %s - not something we can merge"), ref);
+
+        if (ref_cb.similar_refs.nr > 0) {
+	        fprintf_ln(stderr,
+		           Q_("\nDid you mean this?",
+		              "\nDid you mean one of these?",
+		              ref_cb.similar_refs.nr));
+
+	        for (i = 0; i < ref_cb.similar_refs.nr; i++)
+		        fprintf(stderr, "\t%s\n", ref_cb.similar_refs.items[i].string);
+        }
+        exit(1);
+}
+	
+			
+
+

diff --git a/help.h b/help.h
index 0ae5a12..613cb45 100644
--- a/help.h
+++ b/help.h

@@ -27,4 +27,10 @@ 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);
 
+/* 
+ * ref is not something that can be merged. Print a list of remote branches of the
+ * same name that the user might have meant. 
+ */
+extern void help_unknown_ref(const char* ref);
+
 #endif /* HELP_H */

-- 
1.8.3-rc0

The previous patch added help_unknown_ref to print a more helpful error 
message when trying to merge a branch that doesn't exist, by printing a 
list of remote branches the user might have meant. Use it.

Signed-off-by: Vikrant Varma <redacted>
---
 builtin/merge.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 3e2daa3..0f1f39b 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c

@@ -1053,8 +1053,8 @@ static struct commit_list *collect_parents(struct commit *head_commit,
 		remotes = &commit_list_insert(head_commit, remotes)->next;
 	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]);
+		if (!commit) 
+		        help_unknown_ref(argv[i]);		        
 		remotes = &commit_list_insert(commit, remotes)->next;
 	}
 	*remotes = NULL;

-- 
1.8.3-rc0

Vikrant Varma wrote:

Give better advice when trying to merge a branch that doesn't exist. If
the branch exists in any remotes, display a list of suggestions.

Interesting.  Thanks for working on this.

You say advice, but you're not invoking advise() or guarding the
advice with an advice.* -- the advice is undoubtedly helpful, but not
everyone wants to see it.

I see that there are other structs in our codebase suffixing _cb, to
indicate "callback data".  I normally reserve _cb for callback
functions.

+        const char *base_ref;
+        struct string_list similar_refs;

Okay, so you might have more than one matching candidate.

+static int append_similar_ref(const char* refname, const unsigned char *sha1, int flags, void *cb_data)
+{
+        int i;
+        struct similar_ref_cb *cb = (struct similar_ref_cb *)(cb_data);
+        for (i = strlen(refname); refname[i] != '/'; i--)
+               ;

Er, what is this?  A re-implementation of strrchr()?

+        /* A remote branch of the same name is deemed similar */
+        if (!prefixcmp(refname, "refs/remotes/") && !strcmp(refname + i + 1, cb->base_ref))
+               string_list_append(&(cb->similar_refs), refname + 13);

What is 13?  Please use strlen("refs/remotes/") for readability.

I don't like the + i + 1 thing, but you should be able to get rid of
it with strrchr().

+void help_unknown_ref(const char* ref) {
+        int i;
+        struct similar_ref_cb ref_cb;
+        ref_cb.similar_refs = (struct string_list)STRING_LIST_INIT_NODUP;

Why are you casting STRING_LIST_INIT_NODUP?

+        ref_cb.base_ref = ref;
+
+        for_each_ref(append_similar_ref, &ref_cb);
+
+        fprintf_ln(stderr, _("fatal: %s - not something we can merge"), ref);

Hm, "fatal: " was emitted by die() earlier.  I wonder if something
like error() will be nicer than hard-coding "fatal: ".

+        if (ref_cb.similar_refs.nr > 0) {
+               fprintf_ln(stderr,
+                          Q_("\nDid you mean this?",
+                             "\nDid you mean one of these?",
+                             ref_cb.similar_refs.nr));

Hm, why did you use Q_?

+               for (i = 0; i < ref_cb.similar_refs.nr; i++)
+                       fprintf(stderr, "\t%s\n", ref_cb.similar_refs.items[i].string);
+        }
+        exit(1);

die() exits with 128, no?  Why are you exiting with 1 now?

Vikrant Varma wrote:

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

Fun. :)

I haven't looked at the patches closely.  My only immediate thoughts
are:

 - It would be nice to add a test under t/, so we can be sure without
   manually testing it that this new feature doesn't break in the
   future.  See t/README if interested.  I guess it would be t7613.

 - Since the first patch isn't useful without or logically separate
   from the second, this would probably be easier to read as a single
   patch.

Thanks for your work, and hope that helps.

Sincerely,
Jonathan

Vikrant Varma [off-list ref] writes:

Give better advice when trying to merge a branch that doesn't exist. If
the branch exists in any remotes, display a list of suggestions.

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

	Did you mean this?
	    bar/foo

Signed-off-by: Vikrant Varma <redacted>
---

Nicely explained.

If you step back a bit, you would notice two things.

 (1) Saying 'foo' when the user means 'origin/foo' is hardly the
     only (or even the most common) kind of mistake that the code
     you need to add to 'git merge' would encounter and could help
     the user with.  "git merge origin/mastre" and "orign/master"
     may benefit from a typofix as well, and the mechanism to come
     up with the suggestion is likely to hook to the same codepath
     you are modifying with this patch, even though the logic to
     come up with the suggested alternatives may be different.

 (2) "merge" is not the single command that user may make this kind
     of mistake the command could help and use the same helper.
     "git branch myfoo foo" may want to suggest "origin/foo", for
     example.  I just typed "git checkout mater", which could have
     been easily corrected to "git checkout master" with a mechanism
     like this.

An asterisk sticks to the parameter name, not type, like this:

	..._ref(const char *refname, ...

There are other places with the same style problem in this patch.

+{
+        int i;
+        struct similar_ref_cb *cb = (struct similar_ref_cb *)(cb_data);
+        for (i = strlen(refname); refname[i] != '/'; i--)
+	        ;

Indent with two HT, not HT followed by a run of SPs.

+        /* A remote branch of the same name is deemed similar */
+        if (!prefixcmp(refname, "refs/remotes/") && !strcmp(refname + i + 1, cb->base_ref))

An overlong line can and should be split, perhaps like this:

	if (!prefixcmp(... very long parameter list ...) &&
            !strcmp(... another very long parameter list ...))

+	       	string_list_append(&(cb->similar_refs), refname + 13);

To suggest "orign/foo" => "origin/foo", "foz" => "origin/foo", and
"mastre" => "master", using levenshtein.c would help here.

You would special case the distance between "foo" and "origin/foo"
as "very low", e.g. 0, and compute levenshtein distance with refname
and cb->base_ref, store the result in the .util field of the string
list, and sort it at the end after you finish iterating using the
computed distance to come up with the list of suggestions.

+        return 0;
+}
+
+void help_unknown_ref(const char* ref) {
+        int i;
+        struct similar_ref_cb ref_cb;
+        ref_cb.similar_refs = (struct string_list)STRING_LIST_INIT_NODUP;
+        ref_cb.base_ref = ref;
+
+        for_each_ref(append_similar_ref, &ref_cb);
+
+        fprintf_ln(stderr, _("fatal: %s - not something we can merge"), ref);

When you consider the point (2) above, it becomes clear why this
message does not belong to a helper function with a bland and
generic name "help unknown ref".

This API is misdesigned.  A possible alternative that may be better
reusable would be to have a helper that is used to come up with a
list of suggestions and make the caller responsible for emitting the
error message.

+        if (ref_cb.similar_refs.nr > 0) {
+	        fprintf_ln(stderr,
+		           Q_("\nDid you mean this?",
+		              "\nDid you mean one of these?",
+		              ref_cb.similar_refs.nr));
+
+	        for (i = 0; i < ref_cb.similar_refs.nr; i++)
+		        fprintf(stderr, "\t%s\n", ref_cb.similar_refs.items[i].string);
+        }
+        exit(1);
+}
+	
+			
+
+

Do not add trailing blank lines.

Vikrant Varma [off-list ref] writes:

This calling site may become something like:

	if (!commit) {
        	char *suggestion;
                suggestion = guess_misspelled_ref(argv[i]);
                die(suggestion == NULL
                    ? _("%s - not something we can merge")
                    : _("%s - not something we can merge\n"
                        "Perhaps you meant one of these?\n"
                        "%s"), argv[i], suggestion);
	}

if you really want to keep "not something we can merge" at the top.

I however suspect that this might be easier for the reader.

	if (!commit) {
        	struct string_list *suggestion;
                suggestion = guess_misspelled_ref(argv[i]);
                if (suggestion)
			print_string_list(suggestion,
                                      	_("Perhaps you meant one of these?"));
               	die(_("%s - not something we can merge"), argv[i]);
	}

Note that print_string_list() needs to be enhanced so that the
caller can tell it not to show the .util field if you go in this
direction.

 		remotes = &commit_list_insert(commit, remotes)->next;
 	}
 	*remotes = NULL;

Thanks.

On 01-05-2013 17:53, Ramkumar Ramachandra wrote:

Vikrant Varma wrote:

quoted

Give better advice when trying to merge a branch that doesn't exist. If
the branch exists in any remotes, display a list of suggestions.

Interesting.  Thanks for working on this.

You say advice, but you're not invoking advise() or guarding the
advice with an advice.* -- the advice is undoubtedly helpful, but not
everyone wants to see it.

I agree with Matthieu, the people who don't want to see this advice 
never will, because they won't make that mistake. Maybe advice is the 
wrong word, corrections might be more appropriate.

quoted

diff --git a/help.c b/help.c
index 02ba043..faf18b9 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,46 @@ 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 {

I see that there are other structs in our codebase suffixing _cb, to
indicate "callback data".  I normally reserve _cb for callback
functions.

I'm following the convention (builtin/merge.c: struct append_ref_cb). If 
there's a better way to name it, I'll use that.

quoted

+static int append_similar_ref(const char* refname, const unsigned char *sha1, int flags, void *cb_data)
+{
+        int i;
+        struct similar_ref_cb *cb = (struct similar_ref_cb *)(cb_data);
+        for (i = strlen(refname); refname[i] != '/'; i--)
+               ;

Er, what is this?  A re-implementation of strrchr()?

Oh so that's what it's called. Apologies, will fix this.

quoted

+        /* A remote branch of the same name is deemed similar */
+        if (!prefixcmp(refname, "refs/remotes/") && !strcmp(refname + i + 1, cb->base_ref))
+               string_list_append(&(cb->similar_refs), refname + 13);

What is 13?  Please use strlen("refs/remotes/") for readability.

Yes, will fix this too.

quoted

+void help_unknown_ref(const char* ref) {
+        int i;
+        struct similar_ref_cb ref_cb;
+        ref_cb.similar_refs = (struct string_list)STRING_LIST_INIT_NODUP;

Why are you casting STRING_LIST_INIT_NODUP?

quoted

+        ref_cb.base_ref = ref;

ref_cb.similar_refs has already been defined. The compiler won't let me 
assign to it unless I cast first. However, I think compound literals are 
a C99/gcc feature. Is this better?

	struct similar_ref_cb ref_cb = {ref, STRING_LIST_INIT_NODUP};

quoted

+        if (ref_cb.similar_refs.nr > 0) {
+               fprintf_ln(stderr,
+                          Q_("\nDid you mean this?",
+                             "\nDid you mean one of these?",
+                             ref_cb.similar_refs.nr));

Hm, why did you use Q_?

Q_ is a pluralization helper that picks one of the two strings based on 
ref_cb.similar_refs.nr. It's used in help.c:help_unknown_cmd for the 
same reason.

quoted

+               for (i = 0; i < ref_cb.similar_refs.nr; i++)
+                       fprintf(stderr, "\t%s\n", ref_cb.similar_refs.items[i].string);
+        }
+        exit(1);

die() exits with 128, no?  Why are you exiting with 1 now?

Again, because help_unknown_cmd exited with 1. I've tried to follow the 
convention as laid down there. What's the significance of the error code 
for die()? When is it correct to use die(), and when to use error() 
followed by exit(128)?

Am 01.05.2013 21:55, schrieb Vikrant Varma:

On 01-05-2013 17:53, Ramkumar Ramachandra wrote:

quoted

Vikrant Varma wrote:

quoted

+void help_unknown_ref(const char* ref) {
+        int i;
+        struct similar_ref_cb ref_cb;
+        ref_cb.similar_refs = (struct string_list)STRING_LIST_INIT_NODUP;

Why are you casting STRING_LIST_INIT_NODUP?

quoted

+        ref_cb.base_ref = ref;

ref_cb.similar_refs has already been defined. The compiler won't let me
assign to it unless I cast first. However, I think compound literals are
a C99/gcc feature. Is this better?

    struct similar_ref_cb ref_cb = {ref, STRING_LIST_INIT_NODUP};

No. There are compilers that can initialize a struct only with constant
data, but ref is not a constant.

-- Hannes

Vikrant Varma wrote:

I agree with Matthieu, the people who don't want to see this advice never
will, because they won't make that mistake. Maybe advice is the wrong word,
corrections might be more appropriate.

Makes sense.

Perhaps it would make sense to hook into help.autocorrect.  I would
definitely like that.

quoted

I see that there are other structs in our codebase suffixing _cb, to
indicate "callback data".  I normally reserve _cb for callback
functions.

I'm following the convention (builtin/merge.c: struct append_ref_cb). If
there's a better way to name it, I'll use that.

Fine leaving it as it is.

ref_cb.similar_refs has already been defined. The compiler won't let me
assign to it unless I cast first. However, I think compound literals are a
C99/gcc feature. Is this better?

        struct similar_ref_cb ref_cb = {ref, STRING_LIST_INIT_NODUP};

As Johannes pointed out, ref is a variable and that is problematic.
Leave the cast on: I didn't notice the compiler warning in my head.

Q_ is a pluralization helper that picks one of the two strings based on
ref_cb.similar_refs.nr. It's used in help.c:help_unknown_cmd for the same
reason.

Thanks.

Again, because help_unknown_cmd exited with 1. I've tried to follow the
convention as laid down there.

Ah, I didn't notice that.

What's the significance of the error code for
die()?

When something _really_ bad happens, exit() with 128, without
bothering to go up the callstack.  Return error() is a common idiom to
print an error message immediately, and propagate the return status -1
up the callstack.

When is it correct to use die(), and when to use error() followed by
exit(128)?

exit(128) is a bit rare [grep for it yourself].  It's for when you
want to die(), but exit() only after doing an important task in the
caller.

On 02-05-2013 02:02, Ramkumar Ramachandra wrote:

quoted

ref_cb.similar_refs has already been defined. The compiler won't let me
assign to it unless I cast first. However, I think compound literals are a
C99/gcc feature. Is this better?

         struct similar_ref_cb ref_cb = {ref, STRING_LIST_INIT_NODUP};

As Johannes pointed out, ref is a variable and that is problematic.
Leave the cast on: I didn't notice the compiler warning in my head.

Is it okay to use a compound literal? It's not supported in C89.

On 02-05-2013 00:05, Junio C Hamano wrote:

If you step back a bit, you would notice two things.

  (1) Saying 'foo' when the user means 'origin/foo' is hardly the
      only (or even the most common) kind of mistake that the code
      you need to add to 'git merge' would encounter and could help
      the user with.

Yes. I like your suggestion of using levenshtein.c, similar to what's 
been done in help.c:help_unknown_cmd. However, where do you draw the 
line? Do you also suggest 'remotes/origin/foo' for 'remotes/foo'? Also, 
which would you then prioritize for 'foo': 'fob' (this is local) or 
'origin/foo'? In other words, what kind of mistakes are you looking to 
correct -  typos, or forgetful omissions, or both and something more?

  (2) "merge" is not the single command that user may make this kind
      of mistake the command could help and use the same helper.
      "git branch myfoo foo" may want to suggest "origin/foo", for
      example.  I just typed "git checkout mater", which could have
      been easily corrected to "git checkout master" with a mechanism
      like this.

Of course, once the suggestion mechanism is in place, it can be used to 
replace unfriendly die()s in every command that takes a ref.

An asterisk sticks to the parameter name, not type

Indent with two HT, not HT followed by a run of SPs.

An overlong line can and should be split

 >
 > Do not add trailing blank lines.
 >

Thanks, will fix this.

When you consider the point (2) above, it becomes clear why this
message does not belong to a helper function with a bland and
generic name "help unknown ref".

This API is misdesigned.  A possible alternative that may be better
reusable would be to have a helper that is used to come up with a
list of suggestions and make the caller responsible for emitting the
error message.

Yes, I think a better name is needed, I was trying to follow along the 
lines of help_unknown_cmd.

However, making the caller responsible for printing the suggestions may 
not be the best alternative. Borrowing from the way help_unknown_cmd 
works, in help_unknown_ref we could:
1) check if autocorrect is on, returning the corrected refname to the 
calling function, otherwise
2) print an error message, a list of suggestions, and exit()

I think this makes for a clean and reusable API, and requires changing 
one line of code in every function that currently calls die().

On 01-05-2013 23:57, Jonathan Nieder wrote:

  - It would be nice to add a test under t/

Thanks, I'll do that.

  - Since the first patch isn't useful without or logically separate
    from the second, this would probably be easier to read as a single
    patch.

They are logically separate, even if the first isn't useful without the 
second. I wanted to segregate the task of defining a helper function 
that corrects the ref name, and changing the parts of the code that 
should use it. The reason the second is separate is that once the first 
is in place, even commands like 'checkout' can use the helper function.