On Sun, May 8, 2016 at 9:17 AM, Pranit Bauva <pranit.bauva@xxxxxxxxx> wrote:
> On Sun, May 8, 2016 at 12:34 PM, Johannes Schindelin
> <Johannes.Schindelin@xxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html