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

[Vikrant Varma <vikrant.varma94@gmail.com>]

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