On Wed, May 4, 2016 at 10:28 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> On Wed, May 4, 2016 at 3:36 AM, Pranit Bauva <pranit.bauva@xxxxxxxxx> wrote:
>> On Wed, May 4, 2016 at 12:22 PM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
>>> On Wed, May 4, 2016 at 1:07 AM, Pranit Bauva <pranit.bauva@xxxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html