Re: [PATCH] bisect--helper: convert a function in shell to C

From: Stefan Beller <sbeller@google.com>
Date: Mar 22, 2016, 00:28 UTC
Message-ID: <CAGZ79kZveu07h_vERFpJekp4ayJwytwoNVG0LbhiaCnt4u-jRw@mail.gmail.com>
In-Reply-To: <010201539a8d2b8a-9f168d7a-d4c6-4c23-a61f-1ef6ee22f774-000000@eu-west-1.amazonses.com>

On Mon, Mar 21, 2016 at 12:00 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:

> Convert the code literally without changing its design even though it
> seems that its obscure as to the use of comparing revision to different bisect
> arguments which seems like a problem in shell because of the way
> function arguments are handled.

How would I use the C version instead of the shell version now?
I'd imagine you'd want to change calls in git-bisect.sh from
    check_term_format <term> <bad/new>
to be:
    git bisect--helper check_term_format <term> <bad/new>
and "git bisect--helper" would then call the new static method?
Once you have the C version working (do we need additional tests
or can we rely on the test suite being enough for now?),
you can also delete the shell version.

Thanks, Stefan

Show 31 quoted lines

>
> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> ---
>  builtin/bisect--helper.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 3324229..61abe68 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -2,12 +2,35 @@
>  #include "cache.h"
>  #include "parse-options.h"
>  #include "bisect.h"
> +#include "refs.h"
>
>  static const char * const git_bisect_helper_usage[] = {
>         N_("git bisect--helper --next-all [--no-checkout]"),
>         NULL
>  };
>
> +static int check_term_format(const char *term, const char *revision, int flag) {
> +       if (check_refname_format(term, flag))
> +               die("'%s' is not a valid term", term);
> +
> +       if (!strcmp(term, "help") || !strcmp(term, "start") ||
> +               !strcmp(term, "skip") || !strcmp(term, "next") ||
> +               !strcmp(term, "reset") || !strcmp(term, "visualize") ||
> +               !strcmp(term, "replay") || !strcmp(term, "log") ||
> +               !strcmp(term, "run"))
> +               die("can't use the builtin command '%s' as a term", term);

"terms" is missing?

eval_gettext would translate into C as
    die (_("translatable message"));

Show 10 quoted lines

> +
> +       if (!strcmp(term, "bad") || !strcmp(term, "new"))
> +               if(strcmp(revision, "bad"))
> +                       die("can't change the meaning of term '%s'", term);
> +
> +       if (!strcmp(term, "good") || !strcmp(term, "old"))
> +               if (strcmp(revision, "good"))
> +                       die("can't change the meaning of term '%s'", term);
> +
> +       return 1;

Why 1? Usually we use 0 for success in C. die(...) returns with non zero, so having 0 here would help us in the shell code to see if the C version died (or not).

Show 12 quoted lines

> +}
> +
>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  {
>         int next_all = 0;
>
> --
> https://github.com/git/git/pull/216
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html