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

[Stefan Beller <sbeller@google.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

>
> 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"));


> +
> +       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).

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