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

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