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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: May 9, 2016, 14:59 UTC
Message-ID: <alpine.DEB.2.20.1605091657290.4092@virtualbox>
In-Reply-To: <CAFZEwPOy5avk=3WKj0AdP-j7Ump4ie90+CeFTj6K83EMj-pQQA@mail.gmail.com>

Hi Pranit,

On Sun, 8 May 2016, Pranit Bauva wrote:

Show 24 quoted lines

> On Sun, May 8, 2016 at 12:34 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > 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.

Oh, sorry, I meant to imply that it is good as it is by saying "so this code is fine"...

I was just surprised because I thought I remembered that some old C standard does not allow enums to be function scoped. But I was wrong.

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

I think the replace.c code is really a good example. Function-scoped, using the "cmdmode" name that obviously corresponds to the OPT_CMDMODE name.

Ciao, Dscho