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

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

On Tue, Mar 22, 2016 at 8:39 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Tue, 22 Mar 2016, Pranit Bauva 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
>
> s/its/it is/

Sure! A typo.

>> bisect arguments which seems like a problem in shell because of the way
>> function arguments are handled.
>
> I agree that it is obscure. That is why I would suggest to fix it during
> the conversion. Using 'new_term' and 'orig_term' (or something similar)
> would make much more sense.
>
> Another good idea would be to include the shell code, or at least to
> provide a link such as:
>
>         https://github.com/git/git/blob/v2.8.0-rc4/git-bisect.sh#L572-L597
>

I will take care about this henceforth.

>> The argument handling is kind of hard coded right now because it is not
>> really be meant to be used like this and this is just for testing
>> purposes whether this new method is as functional as its counter part.
>> The shell counter part of the method has been retained for historical
>> purposes.
>
> Still, it would make more sense (both in terms of readability and in terms
> of code safety) to introduce and use a function like
>
> static int one_of(const char *term, ...)
> {
>         va_list matches;
>         const char *match;
>
>         va_start(matches, term);
>         while ((match = va_arg(matches, const char *)))
>                 if (!strcmp(term, match))
>                         return 1;
>         va_end(matches);
>
>         return 0;
> }
>

>> +static int check_term_format(const char *term, const char *revision, int flags);
>> +
>> +static int check_term_format(const char *term, const char *revision, int flag) {
>
> Since you define the check_term_format() function here, the declaration
> above is unnecessary. Let's just delete it.

Yes. We could just add functions below this so that it would not
create a problem.

>
>> +     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);
>
> This would look so much nicer using the one_of() function above.

one_of() will definitely make it clean.

> Please also note that our coding convention (as can be seen from the
> existing code in builtin/*.c) is to indent the condition differently than
> the block, either using an extra tab, or by using 4 spaces instead of the
> tab.
>

I
>> +     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);
>
> These two can be combined. Actually, these *four* can easily be combined:
>
>         if ((one_of(term, "bad", "new", NULL) && strcmp(orig, "bad")) ||
>             (one_of(term, "good", "old", NULL) && strcmp(orig, "good")))
>                 die("can't change the meaning of term '%s'", term);
>
>>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>>  {
>>       int next_all = 0;
>>       int no_checkout = 0;
>> +     const char *term;
>
> Better use the existing convention:
>
>         int check_term_format = 0;
>
>>       struct option options[] = {
>>               OPT_BOOL(0, "next-all", &next_all,
>>                        N_("perform 'git bisect next'")),
>>               OPT_BOOL(0, "no-checkout", &no_checkout,
>>                        N_("update BISECT_HEAD instead of checking out the current commit")),
>> +             OPT_STRING(0, "check-term-format", &term, N_("term"),
>> +                      N_("check the format of the ref")),
>
> Hmm. The existing code suggests to use OPT_BOOL instead.
>
>>               OPT_END()
>>       };
>>
>>       argc = parse_options(argc, argv, prefix, options,
>>                            git_bisect_helper_usage, 0);
>>
>> +
>> +     if (term != NULL) {
>> +             if (argc > 0)
>
> Here you need to test for a precise argc, not for a range.

True.

>> +                     return check_term_format(term, argv[0], 0);
>> +             else
>> +                     die("no revision provided with check_for_term");
>> +     }
>> +
>>       if (!next_all)
>>               usage_with_options(git_bisect_helper_usage, options);
>>
>> diff --git a/git-bisect.sh b/git-bisect.sh
>> index 5d1cb00..ea237be 100755
>> --- a/git-bisect.sh
>> +++ b/git-bisect.sh
>> @@ -564,8 +564,8 @@ write_terms () {
>>       then
>>               die "$(gettext "please use two different terms")"
>>       fi
>> -     check_term_format "$TERM_BAD" bad
>> -     check_term_format "$TERM_GOOD" good
>> +     git bisect--helper --check-term-format="$TERM_BAD" bad
>> +     git bisect--helper --check-term-format="$TERM_GOOD" good
>
> The existing convention is to make the first argument *not* a value of the
> "option", i.e. `--check-term-format "$TERM_BAD"` without an equal sign.

Will change this.

> Did you also run the test suite after compiling, to verify that the
> documented expectations are still met after the conversion?

Yes I did run the tests. They produce the same results as they did before.
To ease review I will next time include these the output of the tests
in the commented section.

t6002-rev-list-bisect.sh : http://paste.ubuntu.com/15473728/
t6030-bisect-porcelain.sh : http://paste.ubuntu.com/15473734/
t6041-bisect-submodule.sh : http://paste.ubuntu.com/15473743/

Is there any other test I would need to run?

Regards,
Pranit Bauva