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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]