Re: [PATCH v5 2/2] bisect: rewrite `check_term_format` shell function in C
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- May 7, 2016, 00:05 UTC
- Message-ID
- <CAPig+cSc-k-+41=mspJ_XS+cZXn-rM+h_tSSGVobU+gH6fxxEg@mail.gmail.com>
- In-Reply-To
- <1462546167-1125-3-git-send-email-pranit.bauva@gmail.com>
On Fri, May 6, 2016 at 10:49 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
Show 7 quoted lines
> 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...
Show 12 quoted lines
> 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/
Show 9 quoted lines
> + */
> +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 '='
Show 5 quoted lines
> + 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