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 1:58 PM, 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.

I will modify the commit message in order to reflect this.

>>>> +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.)

I am sorry for my misunderstanding. I had very little idea about
variable arguments. I have searched on this now. I will update by
according to Johannes which seems nice to me.

>>>> +       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.
>
> Sorry, again this does not compute. Certainly, you must pass *some*
> flags argument to check_refname_format() as 'flags' is part of its
> signature, but that doesn't explain why check_term_format() accepts a
> 'flag' argument. Moreover, check_term_format() is not a general
> purpose function like check_refname_format(), so this sort of
> *apparent* flexibility adds complexity with no obvious benefit.

I check the future functions and it does not require the flag argument
so I will remove it.

>>>> +       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.
>
> The name of the command is "check-term-format" and that is precisely
> its purpose so, from the perspective of the caller, *all* problems
> with the term are fatal. It's black-and-white, there is no grey:
> either a term is acceptable, or it isn't; that's all the caller wants
> to know. Consequently, all problems detected by this function should
> be reported the same way (preferably via 'return error()').

Sure. I will use 'return error()'. Any particular reason why this
instead of die() ?

>>>> +       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().
>>
>>>> +       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.
>
> Whether this was a 'switch' statement in the shell script is
> immaterial. The body of each of these 'if' statements exits the
> function, so no following code will be executed anyhow when the
> condition is true. This makes the 'else' pure noise which is why
> 's/else //' is suggested and good style. The less the reader's brain
> has to process, the easier the code is to comprehend.

Okay. I get it. Will drop off the else.

>>>> +               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).
>
> Sorry, does not compute. To what HEAD or other ref are you referring?
> This command is about checking the name of a bisection term. Where
> does 'ref' come into it (other than as an implementation detail)?

I guess it would be more appropriate to use term.

Thanks,
Pranit Bauva