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

From: Pranit Bauva <pranit.bauva@gmail.com>
Date: May 8, 2016, 07:17 UTC
Message-ID: <CAFZEwPOy5avk=3WKj0AdP-j7Ump4ie90+CeFTj6K83EMj-pQQA@mail.gmail.com>
In-Reply-To: <alpine.DEB.2.20.1605080858190.4092@virtualbox>

On Sun, May 8, 2016 at 12:34 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 18 quoted lines

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

Show 16 quoted lines

>>  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).
>
> Ciao,
> Dscho

[1]: http://article.gmane.org/gmane.comp.version-control.git/289653

Regards, Pranit Bauva