[PATCH 0/2] Better advice on merge

[PATCH 0/2] Better advice on merge

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

[PATCH 1/2] help: add help_unknown_ref

[Vikrant Varma]

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 <vikrant.varma94@gmail.com>
---
 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

[PATCH 2/2] merge: use help_unknown_ref instead of die

[Vikrant Varma]

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 <vikrant.varma94@gmail.com>
---
 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

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

[Ramkumar Ramachandra]

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.

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

> +        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?

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

[Matthieu Moy]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

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

Quite franckly, I don't see why anybody would need to disable it.
advice.* is very nice to disable messages that are shown in normal Git
usage (e.g. running "git status" or attempting a non-fast forward push),
but this is clearly a user-error, that an advanced user is not supposed
to see anyway.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH 0/2] Better advice on merge

[Jonathan Nieder]

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

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

[Junio C Hamano]

Vikrant Varma <vikrant.varma94@gmail.com> 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 <vikrant.varma94@gmail.com>
> ---

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.

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

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.

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

Re: [PATCH 2/2] merge: use help_unknown_ref instead of die

[Junio C Hamano]

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

> 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 <vikrant.varma94@gmail.com>
> ---
>  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]);		        

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.

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

[Vikrant Varma]

On 01-05-2013 17:53, Ramkumar Ramachandra wrote:
> 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 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.

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

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

>> +        /* 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.

>> +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;


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};


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

>> +               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)?

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

[Johannes Sixt]

Am 01.05.2013 21:55, schrieb Vikrant Varma:
> On 01-05-2013 17:53, Ramkumar Ramachandra wrote:
>> Vikrant Varma wrote:
>>> +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;
> 
> 
> 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

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

[Ramkumar Ramachandra]

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.

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

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

[Vikrant Varma]

On 02-05-2013 02:02, Ramkumar Ramachandra wrote:
>> 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.

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

[Junio C Hamano]

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

> On 02-05-2013 02:02, Ramkumar Ramachandra wrote:
>>> 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.

Building on top of what was suggested in the other message, the
helper could be made more reusable by doing something like this:

	int suggest_misspelt_ref(const char *ref, struct string_list *suggested);

and the caller can do

	if (!commit) {
		struct string_list suggested = STRING_LIST_INIT;
                if (suggest_misspelt_ref(argv[1], &suggested)) {
                	... Did you mean one of these??? ...
			string_list_clear(&suggested);
		}
                die(_("'%s' is not something we can merge'), argv[1]);
	}

So I think this point is moot.  Of course, similar_ref_cb needs to
be updated to keep a pointer to an existing string_list, not an
instance of its own string_list.

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

[Vikrant Varma]

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

Re: [PATCH 0/2] Better advice on merge

[Vikrant Varma]

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.

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

[Junio C Hamano]

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

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

This is your itch, so you get to choose what you would propose to
the list and have people tweak it ;-)

I personally think the first version should not use levenshtein _at
all_ (hence not solving my 'git checkout mastr'). I however want to
make sure the code is structured in such a way that allows other
people to build on top later.

> However, making the caller responsible for printing the suggestions
> may not be the best alternative.

That "responsibility" could be encapsulated in a separate helper
function the callers call instead of die().

The lower level machinery to come up with a suggested list of refs,
when the caller suspects that there is a typo in refs, is the same
across different commands, and it is insufficient to have a single
helper function that hardcodes "not something we can _MERGE_", and
copy the function to create many other helpers that do the same
thing but with different error messages.

You would need two layers, one that collects suggested list of refs,
and another that is directly called by the top-level caller that
takes the refname and the die message, and the latter would use the
former.  What I was showing was the lower level one, without the
higher level one that takes die message and a suspicious user input
(the higher level one would be trivial and do not need hand-holding
while designing).