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

From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: May 1, 2013, 12:23 UTC
Message-ID: <CALkWK0nMMi-nmAMUGXCaJDCV29G3dOzYTosKqSw+bFzc0osiaA@mail.gmail.com>
In-Reply-To: <1367407327-5216-2-git-send-email-vikrant.varma94@gmail.com>

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.

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

> +        const char *base_ref;
> +        struct string_list similar_refs;

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

Show 6 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()?

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

Show 5 quoted lines

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

Show 5 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_?

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