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

[Pranit Bauva <pranit.bauva@gmail.com>]

On Wed, May 4, 2016 at 12:22 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Wed, May 4, 2016 at 1:07 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> This reimplements the `check_term_format` shell function in C and adds
>
> s/This reimplements/Reimplement/
> s/adds/add/
>
>> a `--check-term-format` subcommand to `git bisect--helper` to call it
>> from git-bisect.sh
>
> s/$/./

Sure.

> Okay, I'll bite: Why is this a good idea? What does it buy you?
>
> It's not as if the rewrite is especially faster or more easily
> expressed in C; quite the contrary, the shell code is more concise and
> probably about equally as fast (not that execution speed matters in
> this case).
>
> I could understand this functionality being ported to C in the form of
> a static function as a minor part of porting "git bisect terms" in its
> entirety to C, but I'm not imaginative enough to see why this
> functionality is useful as a standalone git-bisect--helper subcommand,
> and the commit message doesn't enlighten. Consequently, it seems like
> unnecessary complexity.

It is important to understand that the subcommand is just a
**temporary** measure.
Yes, I agree that making it a subcommand increases unnecessary
complexity. As a part of complete rewrite of git-bisect.sh, I am
converting one function individually to C. The functionality of
subcommand is useful so that I can use the existing test suite to
verify whether I have done the conversion properly. As more functions
get ported (which I intend to finish this summers), previously
existing subcommands will be removed. For eg. After this patch, I will
now convert the function write_terms(). So in that patch, I will
remove the subcommand for check-term-format and instead use the
check_term_format() method and then introduce a new subcommand for
write_terms(). Verifying the function conversion was suggested by
Stefan Beller[1] and Christian Couder[2] gave a hint of how to go
about with using the existing test suite. As for the current
situation, git-bisect.sh calls `--next-all` in a similar way which was
the hint for me of how to go about with this project.

>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
>> ---
>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> @@ -2,16 +2,66 @@
>>  static const char * const git_bisect_helper_usage[] = {
>>         N_("git bisect--helper --next-all [--no-checkout]"),
>> +       N_("git bisect--helper --check-term-format <term> <orig_term>"),
>
> Could this be shortened to --check-term or would that be undesirable?

I guess --check-term-format would be more appropriate and as this is
temporary, it wouldn't matter afterwards.

>>         NULL
>>  };
>>
>>  enum sub_commands {
>> -       NEXT_ALL = 1
>> +       NEXT_ALL = 1,
>> +       CHECK_TERM_FMT
>>  };
>>
>> +/*
>> + * To check whether the string `term` belongs to the set of strings
>> + * included in the variable arguments so as to make the code look
>> + * clean and avoid having long lines in the `if` statement.
>> + */
>
> Is this a (long) sentence fragment? Code cleanliness is an obviously
> desirable trait, thus talking about it in the comment adds no value;
> it's just noise.

I will keep the initial part of the comment.

>> +static int one_of(const char *term, ...)
>> +{
>> +       va_list matches;
>> +       const char *match;
>> +
>> +       va_start(matches, term);
>> +       while ((match = va_arg(matches, const char *)) != NULL)
>> +               if (!strcmp(term, match))
>> +                       return 1;
>
> Is it wise to return here without invoking va_end()?

I guess since it already checks for NULL, invoking va_end() will make
it redundant[3].

>> +       va_end(matches);
>> +
>> +       return 0;
>> +}
>> +
>> +static int check_term_format(const char *term, const char *orig_term,
>> +                            int flag)
>
> What is 'flag' for? The single caller only ever passes 0, so why is this needed?

Well, currently the subcommand does not use this flag but this flag is
present in the method check_refname_format() so it would be better to
use it. This flag might be useful in further parts of conversion since
as I previously mentioned check-term-format isn't a permanent
solution.

>> +{
>> +       struct strbuf new_term = STRBUF_INIT;
>
> 'new_term' is being leaked at every 'return' statement in this function.

I will have to free this memory.

>
>> +       strbuf_addf(&new_term, "refs/bisect/%s", term);
>> +
>> +       if (check_refname_format(new_term.buf, flag))
>> +               die(_("'%s' is not a valid term\n"), term);
>
> Why does this die() while the other "invalid" cases merely return
> error()? What makes this special?

This is because I felt that if check_refname_format() fails then its a
fatal error while in other cases, it is not as fatal.

> Also, drop "\n" from the error string.

Sure!

>> +       else if (one_of(term, "help", "start", "skip", "next", "reset",
>
> s/else //

Agree since it would be a part of the switch which is not included
with the check_refname_format().

>> +                       "visualize", "replay", "log", "run", NULL))
>> +               return error("can't use the builtin command '%s' as a term\n", term);
>
> This should be wrapped in _(...). Also, drop the "\n".
>
>> +       /*
>> +        * In theory, nothing prevents swapping
>> +        * completely good and bad, but this situation
>> +        * could be confusing and hasn't been tested
>> +        * enough. Forbid it for now.
>> +        */
>
> This would be a bit easier to read if re-wrapped to fit within 80
> columns rather than 53 or so.
>
>> +       else if ((one_of(term, "bad", "new", NULL) && strcmp(orig_term, "bad")) ||
>
> s/else //

In the shell script a switch was used, thus `else if` would be a more
appropriate choice over `if`. Also if the first if statement fails
then it is unnecessary to go further.

>
>> +                (one_of(term, "good", "old", NULL) && strcmp(orig_term, "good")))
>
> This can be more efficient by doing the strcmp() before the expensive one_of():
>
>     if ((strcmp(...) && one_of(...)) ||
>         strcmp(...) && one_of(...)))

Nice. Will include this.

>> +               return error("can't change the meaning of the term '%s'\n", term);
>
> This should be wrapped in _(...). Also, drop the "\n".

Sure

>
>> +       return 0;
>> +}
>> +
>>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>>  {
>>         int sub_command = 0;
>> @@ -19,6 +69,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>>         struct option options[] = {
>>                 OPT_CMDMODE(0, "next-all", &sub_command,
>>                          N_("perform 'git bisect next'"), NEXT_ALL),
>> +               OPT_CMDMODE(0, "check-term-format", &sub_command,
>> +                        N_("check format of the ref"), CHECK_TERM_FMT),
>
> What "ref"?

The ref here means that ref (like HEAD).


>>                 OPT_BOOL(0, "no-checkout", &no_checkout,
>>                          N_("update BISECT_HEAD instead of checking out the current commit")),
>>                 OPT_END()
>> @@ -33,6 +85,10 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>>         switch (sub_command) {
>>         case NEXT_ALL:
>>                 return bisect_next_all(prefix, no_checkout);
>> +       case CHECK_TERM_FMT:
>> +               if (argc != 2)
>> +                       die(_("--check-term-format should be followed by exactly 2 arguments."));
>
> Drop the period. Possible reword:
>
>     --check-term-format requires two arguments

Seems better

>> +               return check_term_format(argv[0], argv[1], 0);
>>         default:
>>                 die(_("bug: unknown subcommand '%d'"), sub_command);
>>         }

[1]: http://article.gmane.org/gmane.comp.version-control.git/293489

[2]: http://article.gmane.org/gmane.comp.version-control.git/293489

[3]: http://article.gmane.org/gmane.comp.version-control.git/293489