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

[Christian Couder <christian.couder@gmail.com>]

On Wed, May 4, 2016 at 10:28 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Wed, May 4, 2016 at 3:36 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> 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:
>>> 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.
>
> The commit message seems to be lacking this information and any other rationale.

Yeah, I agree it could be improved in this regard.

>> 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.
>
> You're taking an inverted bottom-up approach which repeatedly adds and
> removes unnecessary complexity rather than a more straight-forward
> top-down approach. For instance, with a top-down approach, as a first
> step, you could instead add a skeleton, do-nothing "git-bisect--helper
> set-terms" and flesh it out in subsequent patches until fully
> implemented,

I am not sure this is the best approach. If for some reason the GSoC
project is not finished, we will end up with a badly named
"git-bisect--helper --set-terms" command and a badly named
write_term() shell functions. They will be badly named because they
will not do all what the name suggests.

> at which point drop all the "terms" code from
> git-bisect.sh and have it invoke the helper instead. You get the same
> benefit of being able to use the existing test suite without the
> unnecessary complexity.
>
> In fact, git-bisect.sh could start invoking "git-bisect--helper"
> before it's fully fleshed out. For instance, a partially fleshed out C
> write_terms(), might just verify that the two terms are not the same
> and then write them out to BISECT_TERMS, and the shell script would
> invoke its own check_term_format() before calling the helper.
>
>>>> +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].
>
> Sorry, your response does not compute. Each va_start() *must* be
> balanced with a va_end(). (While it's true that you may encounters
> platforms/compilers for which a missing va_end() does no harm, such
> code is not portable.)

Yeah, I agree with that and the rest of your comments. Thanks.