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. 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).
>
> Ciao,
> Dscho
[1]: http://article.gmane.org/gmane.comp.version-control.git/289653
Regards,
Pranit Bauva
--
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