Re: [PATCH v6 1/3] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL

[Pranit Bauva]

Hey Eric,

On Mon, May 16, 2016 at 12:37 PM, Eric Sunshine  wrote:
> On Thu, May 12, 2016 at 1:32 AM, Pranit Bauva  wrote:
>> `--next-all` is meant to be used as a subcommand to support multiple
>> "operation mode" though the current implementation does not contain any
>> other subcommand along side with `--next-all` but further commits will
>> include some more subcommands.
>>
>> Signed-off-by: Pranit Bauva 
>> ---
>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> @@ -23,9 +23,14 @@ int cmd_bisect__helper(int argc, const char **argv, const 
>> char *prefix)
>> -   if (!next_all)
>> +   if (!cmdmode)
>> usage_with_options(git_bisect_helper_usage, options);
>>
>> -   /* next-all */
>> -   return bisect_next_all(prefix, no_checkout);
>> +   switch (cmdmode) {
>> +   case NEXT_ALL:
>> +   return bisect_next_all(prefix, no_checkout);
>> +   default:
>> +   die("BUG: unknown subcommand '%d'", cmdmode);
>> +   }
>> +   return 0;
>
> What happens if you remove this useless 'return 0'? Does the (or some)
> compiler incorrectly complain about it falling off the end of the
> function without returning a value?

I tried removing it. It works fine with gcc and clang. You can see the
build on travis-CI[1]. I am not sure of other compilers and also don't
know a way to test it either. You could use my branch on github[2] if
you want to test it on other compilers. I think its better to keep the
return 0 if we aren't sure whether it would work on every compiler.

[1]: https://travis-ci.org/pranitbauva1997/git/builds/131622175
[2]: https://github.com/pranitbauva1997/git/tree/return-try

Regards,
Pranit Bauva
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 1/3] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL

[Eric Sunshine]

On Thu, May 12, 2016 at 1:32 AM, Pranit Bauva  wrote:
> `--next-all` is meant to be used as a subcommand to support multiple
> "operation mode" though the current implementation does not contain any
> other subcommand along side with `--next-all` but further commits will
> include some more subcommands.
>
> Signed-off-by: Pranit Bauva 
> ---
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> @@ -23,9 +23,14 @@ int cmd_bisect__helper(int argc, const char **argv, const 
> char *prefix)
> -   if (!next_all)
> +   if (!cmdmode)
> usage_with_options(git_bisect_helper_usage, options);
>
> -   /* next-all */
> -   return bisect_next_all(prefix, no_checkout);
> +   switch (cmdmode) {
> +   case NEXT_ALL:
> +   return bisect_next_all(prefix, no_checkout);
> +   default:
> +   die("BUG: unknown subcommand '%d'", cmdmode);
> +   }
> +   return 0;

What happens if you remove this useless 'return 0'? Does the (or some)
compiler incorrectly complain about it falling off the end of the
function without returning a value?

>  }
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 1/3] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL

[Pranit Bauva]

`--next-all` is meant to be used as a subcommand to support multiple
"operation mode" though the current implementation does not contain any
other subcommand along side with `--next-all` but further commits will
include some more subcommands.

Helped-by: Johannes Schindelin 
Mentored-by: Lars Schneider 
Mentored-by: Christian Couder 
Signed-off-by: Pranit Bauva 
---
 builtin/bisect--helper.c | 17 +++--
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 3324229..8111c91 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -10,11 +10,11 @@ static const char * const git_bisect_helper_usage[] = {
 
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
-   int next_all = 0;
+   enum { NEXT_ALL = 1 } cmdmode = 0;
int no_checkout = 0;
struct option options[] = {
-   OPT_BOOL(0, "next-all", _all,
-N_("perform 'git bisect next'")),
+   OPT_CMDMODE(0, "next-all", ,
+N_("perform 'git bisect next'"), NEXT_ALL),
OPT_BOOL(0, "no-checkout", _checkout,
 N_("update BISECT_HEAD instead of checking out the 
current commit")),
OPT_END()
@@ -23,9 +23,14 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
argc = parse_options(argc, argv, prefix, options,
 git_bisect_helper_usage, 0);
 
-   if (!next_all)
+   if (!cmdmode)
usage_with_options(git_bisect_helper_usage, options);
 
-   /* next-all */
-   return bisect_next_all(prefix, no_checkout);
+   switch (cmdmode) {
+   case NEXT_ALL:
+   return bisect_next_all(prefix, no_checkout);
+   default:
+   die("BUG: unknown subcommand '%d'", cmdmode);
+   }
+   return 0;
 }
-- 
2.8.2

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html