Re: [PATCH v5 1/2] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL

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

On Sun, May 8, 2016 at 9:17 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> On Sun, May 8, 2016 at 12:34 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
>> Hi Pranit,
>>
>> On Fri, 6 May 2016, Pranit Bauva wrote:
>>
>>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>>> index 3324229..d8de651 100644
>>> --- a/builtin/bisect--helper.c
>>> +++ b/builtin/bisect--helper.c
>>> @@ -8,13 +8,17 @@ static const char * const git_bisect_helper_usage[] = {
>>>       NULL
>>>  };
>>>
>>> +enum subcommand {
>>> +     NEXT_ALL = 1
>>> +};
>>
>> I still do not think that this enum needs to have file scope. Function
>> scope is enough.
>
> In the very initial patch I made it in function scope. To which you
> pointed out[1] that in all other examples but for one have file scope
> so then I thought maybe that exception was a wrong example and I
> should stick to the convention of putting it in file scope.

In the message Dscho wrote:

"Interesting. I did not think that Git's source code declares enums inside
functions, but builtin/remote.c's config_read_branches() does, so this
code is fine." So you didn't need to put it back in file scope.

Please don't change things when you are told they are fine unless
there is a good reason like a bug and in this case explain the reason.

> But now I
> also realize that builtin/replace.c uses "cmdmode" instead of
> "subcommand" so I am still wondering what would be the most
> appropriate?
>
>>>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>>>  {
>>> -     int next_all = 0;
>>> +     int subcommand = 0;
>>
>> Since subcommand is not simply an integer, but wants to take on the values
>> defined in the enum above, the type should be changed accordingly. You
>> could do it this way (short and sweet, with the appropriate scope):
>>
>>         enum { NEXT_ALL = 1 } subcommand = 0;
>>
>> See https://github.com/git/git/blob/v2.8.2/builtin/replace.c#L423-L430 for
>> an example (which uses "cmdmode" instead of "subcommand", too).

Yeah, please use "cmdmode" as Junio already suggested and do it like
in the example that Dscho points to.

Thanks,
Christian.