Re: [PATCH 1/2] help: add help_unknown_ref
- From
Vikrant Varma <vikrant.varma94@gmail.com>
- Date
- May 1, 2013, 19:55 UTC
- Message-ID
- <51817319.6060201@gmail.com>
- In-Reply-To
- <CALkWK0nMMi-nmAMUGXCaJDCV29G3dOzYTosKqSw+bFzc0osiaA@mail.gmail.com>
On 01-05-2013 17:53, Ramkumar Ramachandra wrote:
Show 10 quoted lines
> 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.
Show 22 quoted lines
>> 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.
Show 9 quoted lines
>> +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.
Show 6 quoted lines
>> + /* 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.
Show 8 quoted lines
>> +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};Show 8 quoted lines
>> + 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.
Show 7 quoted lines
>> + 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)?