Re: [PATCH v5 2/2] bisect: rewrite `check_term_format` shell function in C

[Eric Sunshine <sunshine@sunshineco.com>]

On Fri, May 6, 2016 at 10:49 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> Reimplement the `check_term_format` shell function in C and add
> a `--check-term-format` subcommand to `git bisect--helper` to call it
> from git-bisect.sh
>
> Using `--check-term-format` subcommand is a temporary measure to port
> shell function to C so as to use the existing test suite. As more
> functions are ported, this subcommand will loose its existence and will

s/loose/lose/

Though, maybe it would be better to say:

    ...this subcommand will be retired...

> be called by some other method/subcommand. For eg. In conversion of
> write_terms() of git-bisect.sh, the subcommand will be removed and
> instead check_term_format() will be called in its C implementation while
> a new subcommand will be introduced for write_terms().
>
> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> ---
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> @@ -2,16 +2,65 @@
> +/*
> + * To check whether the string `term` belongs to the set of strings
> + * included in the variable arguments.

This is still a sentence fragment as pointed out last time[1]. You can
fix it with: s/To check/Check/

> + */
> +static int one_of(const char *term, ...)
> +{
> +       int res = 0;
> +       va_list matches;
> +       const char *match;
> +
> +       va_start(matches, term);
> +       while (!res && (match=va_arg(matches, const char *)))

Style: add spaces around '='

> +               res = !strcmp(term, match);
> +       va_end(matches);
> +
> +       return res;
> +}

The rest of the patch seems to address the issues raised by my
previous review[1] (aside from my comments[1,2] about this bottom-up
approach repeatedly adding and removing unnecessary complexity as each
little function is ported from shell to C, but that's a separate
philosophical discussion).

[1]: http://thread.gmane.org/gmane.comp.version-control.git/289476/focus=293501
[2]: http://thread.gmane.org/gmane.comp.version-control.git/289476/focus=293517