{"thread":{"id":"40812","subject":"[PATCH 0/5] parse-options: allow -h as a short option","startedAt":"2015-11-17T10:19:30Z","lastAt":"2015-12-04T18:40:07Z","messageCount":7,"participants":["René Scharfe","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"273419","messageId":"564AFF32.1030406@web.de","threadId":"40812","inReplyTo":null,"subject":"[PATCH 0/5] parse-options: allow -h as a short option","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2015-11-17T10:19:30Z","receivedAt":"2015-11-17T10:19:30Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"The short option -h is hard-wired in parseopt to list the options of\na command together with a short explanation.  If -h is to be given a\ndifferent meaning then the flag PARSE_OPT_NO_INTERNAL_HELP has to be\nspecified, which turns off handling for -h, --help and --help-all\n(except that --help handling is effectively overridden by git.c, but\nthat's a different story).\n\nMost commands that do that, grep and show-ref in particular, still show\nthe usage when -h is specified as the only parameter and with --help-all\nby  implementing explicit handlers for them.  This series makes it\neasier for them by letting them override -h handling without any flag.\n\nRene Scharfe (5):\n  parse-options: deduplicate parse_options_usage() calls\n  parse-options: inline parse_options_usage() at its only remaining caller\n  parse-options: allow -h as a short option\n  grep: stop using PARSE_OPT_NO_INTERNAL_HELP\n  show-ref: stop using PARSE_OPT_NO_INTERNAL_HELP\n\n builtin/grep.c     | 17 +----------------\n builtin/show-ref.c | 12 +-----------\n parse-options.c    | 40 ++++++++++++++++++++--------------------\n parse-options.h    |  2 +-\n 4 files changed, 23 insertions(+), 48 deletions(-)\n\n-- \n2.6.3\n"},{"id":"273420","messageId":"564B008A.9000102@web.de","threadId":"40812","inReplyTo":"564AFF32.1030406@web.de","subject":"[PATCH 1/5] parse-options: deduplicate parse_options_usage() calls","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2015-11-17T10:25:14Z","receivedAt":"2015-11-17T10:25:14Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Avoid long lines and repeating parse_options_usage() calls with their\nduplicate parameters by providing labels with speaking names to jump to.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n parse-options.c | 18 ++++++++++++------\n 1 file changed, 12 insertions(+), 6 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 3eceba4..f0b6d9b 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -435,6 +435,7 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,\n \t\t       const char * const usagestr[])\n {\n \tint internal_help = !(ctx->flags & PARSE_OPT_NO_INTERNAL_HELP);\n+\tint err = 0;\n \n \t/* we must reset ->opt, unknown short option leave it dangling */\n \tctx->opt = NULL;\n@@ -454,10 +455,10 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,\n \t\tif (arg[1] != '-') {\n \t\t\tctx->opt = arg + 1;\n \t\t\tif (internal_help && *ctx->opt == 'h')\n-\t\t\t\treturn parse_options_usage(ctx, usagestr, options, 0);\n+\t\t\t\tgoto show_usage;\n \t\t\tswitch (parse_short_opt(ctx, options)) {\n \t\t\tcase -1:\n-\t\t\t\treturn parse_options_usage(ctx, usagestr, options, 1);\n+\t\t\t\tgoto show_usage_error;\n \t\t\tcase -2:\n \t\t\t\tif (ctx->opt)\n \t\t\t\t\tcheck_typos(arg + 1, options);\n@@ -467,10 +468,10 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,\n \t\t\t\tcheck_typos(arg + 1, options);\n \t\t\twhile (ctx->opt) {\n \t\t\t\tif (internal_help && *ctx->opt == 'h')\n-\t\t\t\t\treturn parse_options_usage(ctx, usagestr, options, 0);\n+\t\t\t\t\tgoto show_usage;\n \t\t\t\tswitch (parse_short_opt(ctx, options)) {\n \t\t\t\tcase -1:\n-\t\t\t\t\treturn parse_options_usage(ctx, usagestr, options, 1);\n+\t\t\t\t\tgoto show_usage_error;\n \t\t\t\tcase -2:\n \t\t\t\t\t/* fake a short option thing to hide the fact that we may have\n \t\t\t\t\t * started to parse aggregated stuff\n@@ -496,10 +497,10 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,\n \t\tif (internal_help && !strcmp(arg + 2, \"help-all\"))\n \t\t\treturn usage_with_options_internal(ctx, usagestr, options, 1, 0);\n \t\tif (internal_help && !strcmp(arg + 2, \"help\"))\n-\t\t\treturn parse_options_usage(ctx, usagestr, options, 0);\n+\t\t\tgoto show_usage;\n \t\tswitch (parse_long_opt(ctx, arg + 2, options)) {\n \t\tcase -1:\n-\t\t\treturn parse_options_usage(ctx, usagestr, options, 1);\n+\t\t\tgoto show_usage_error;\n \t\tcase -2:\n \t\t\tgoto unknown;\n \t\t}\n@@ -511,6 +512,11 @@ unknown:\n \t\tctx->opt = NULL;\n \t}\n \treturn PARSE_OPT_DONE;\n+\n+ show_usage_error:\n+\terr = 1;\n+ show_usage:\n+\treturn parse_options_usage(ctx, usagestr, options, err);\n }\n \n int parse_options_end(struct parse_opt_ctx_t *ctx)\n-- \n2.6.3\n"},{"id":"273421","messageId":"564B0097.9050206@web.de","threadId":"40812","inReplyTo":"564AFF32.1030406@web.de","subject":"[PATCH 2/5] parse-options: inline parse_options_usage() at its only remaining caller","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2015-11-17T10:25:27Z","receivedAt":"2015-11-17T10:25:27Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Signed-off-by: Rene Scharfe <l.s.r@web.de>\n---\n parse-options.c | 13 +------------\n 1 file changed, 1 insertion(+), 12 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex f0b6d9b..4863966 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -5,10 +5,6 @@\n #include \"color.h\"\n #include \"utf8.h\"\n \n-static int parse_options_usage(struct parse_opt_ctx_t *ctx,\n-\t\t\t       const char * const *usagestr,\n-\t\t\t       const struct option *opts, int err);\n-\n #define OPT_SHORT 1\n #define OPT_UNSET 2\n \n@@ -516,7 +512,7 @@ unknown:\n  show_usage_error:\n \terr = 1;\n  show_usage:\n-\treturn parse_options_usage(ctx, usagestr, options, err);\n+\treturn usage_with_options_internal(ctx, usagestr, options, 0, err);\n }\n \n int parse_options_end(struct parse_opt_ctx_t *ctx)\n@@ -662,13 +658,6 @@ void NORETURN usage_msg_opt(const char *msg,\n \tusage_with_options(usagestr, options);\n }\n \n-static int parse_options_usage(struct parse_opt_ctx_t *ctx,\n-\t\t\t       const char * const *usagestr,\n-\t\t\t       const struct option *opts, int err)\n-{\n-\treturn usage_with_options_internal(ctx, usagestr, opts, 0, err);\n-}\n-\n #undef opterror\n int opterror(const struct option *opt, const char *reason, int flags)\n {\n-- \n2.6.3\n"},{"id":"273422","messageId":"564B00A2.5040801@web.de","threadId":"40812","inReplyTo":"564AFF32.1030406@web.de","subject":"[PATCH 3/5] parse-options: allow -h as a short option","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2015-11-17T10:25:38Z","receivedAt":"2015-11-17T10:25:38Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Let callers provide their own handler for the short option -h even\nwithout the flag PARSE_OPT_NO_INTERNAL_HELP, but call the internal\nhandler (showing usage information) if that is the only parameter.\nImplement the first part by checking for -h only if parse_short_opt()\ncan't find it and returns -2.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n parse-options.c | 15 ++++++++++-----\n parse-options.h |  2 +-\n 2 files changed, 11 insertions(+), 6 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 4863966..47a9192 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -410,7 +410,7 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,\n \t\t\t const struct option *options, int flags)\n {\n \tmemset(ctx, 0, sizeof(*ctx));\n-\tctx->argc = argc - 1;\n+\tctx->argc = ctx->total = argc - 1;\n \tctx->argv = argv + 1;\n \tctx->out  = argv;\n \tctx->prefix = prefix;\n@@ -448,27 +448,32 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,\n \t\t\tcontinue;\n \t\t}\n \n+\t\t/* lone -h asks for help */\n+\t\tif (internal_help && ctx->total == 1 && !strcmp(arg + 1, \"h\"))\n+\t\t\tgoto show_usage;\n+\n \t\tif (arg[1] != '-') {\n \t\t\tctx->opt = arg + 1;\n-\t\t\tif (internal_help && *ctx->opt == 'h')\n-\t\t\t\tgoto show_usage;\n \t\t\tswitch (parse_short_opt(ctx, options)) {\n \t\t\tcase -1:\n \t\t\t\tgoto show_usage_error;\n \t\t\tcase -2:\n \t\t\t\tif (ctx->opt)\n \t\t\t\t\tcheck_typos(arg + 1, options);\n+\t\t\t\tif (internal_help && *ctx->opt == 'h')\n+\t\t\t\t\tgoto show_usage;\n \t\t\t\tgoto unknown;\n \t\t\t}\n \t\t\tif (ctx->opt)\n \t\t\t\tcheck_typos(arg + 1, options);\n \t\t\twhile (ctx->opt) {\n-\t\t\t\tif (internal_help && *ctx->opt == 'h')\n-\t\t\t\t\tgoto show_usage;\n \t\t\t\tswitch (parse_short_opt(ctx, options)) {\n \t\t\t\tcase -1:\n \t\t\t\t\tgoto show_usage_error;\n \t\t\t\tcase -2:\n+\t\t\t\t\tif (internal_help && *ctx->opt == 'h')\n+\t\t\t\t\t\tgoto show_usage;\n+\n \t\t\t\t\t/* fake a short option thing to hide the fact that we may have\n \t\t\t\t\t * started to parse aggregated stuff\n \t\t\t\t\t *\ndiff --git a/parse-options.h b/parse-options.h\nindex e8b55ea..ea4af92 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -199,7 +199,7 @@ enum {\n struct parse_opt_ctx_t {\n \tconst char **argv;\n \tconst char **out;\n-\tint argc, cpidx;\n+\tint argc, cpidx, total;\n \tconst char *opt;\n \tint flags;\n \tconst char *prefix;\n-- \n2.6.3\n"},{"id":"273423","messageId":"564B00B1.1070709@web.de","threadId":"40812","inReplyTo":"564AFF32.1030406@web.de","subject":"[PATCH 4/5] grep: stop using PARSE_OPT_NO_INTERNAL_HELP","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2015-11-17T10:25:53Z","receivedAt":"2015-11-17T10:25:53Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"The flag PARSE_OPT_NO_INTERNAL_HELP is set to allow overriding the\noption -h, except when it's the only one given.  This is the default\nbehavior now, so remove the flag and the hand-rolled --help-all\nhandling.  The internal --help-all handler now actually shows hidden\noptions, i.e. --debug in this case.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n builtin/grep.c | 17 +----------------\n 1 file changed, 1 insertion(+), 16 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex d04f440..749e346 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -612,11 +612,6 @@ static int pattern_callback(const struct option *opt, const char *arg,\n \treturn 0;\n }\n \n-static int help_callback(const struct option *opt, const char *arg, int unset)\n-{\n-\treturn -1;\n-}\n-\n int cmd_grep(int argc, const char **argv, const char *prefix)\n {\n \tint hit = 0;\n@@ -738,18 +733,9 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\t\tPARSE_OPT_OPTARG, NULL, (intptr_t)default_pager },\n \t\tOPT_BOOL(0, \"ext-grep\", &external_grep_allowed__ignored,\n \t\t\t N_(\"allow calling of grep(1) (ignored by this build)\")),\n-\t\t{ OPTION_CALLBACK, 0, \"help-all\", NULL, NULL, N_(\"show usage\"),\n-\t\t  PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, help_callback },\n \t\tOPT_END()\n \t};\n \n-\t/*\n-\t * 'git grep -h', unlike 'git grep -h <pattern>', is a request\n-\t * to show usage information and exit.\n-\t */\n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(grep_usage, options);\n-\n \tinit_grep_defaults();\n \tgit_config(grep_cmd_config, NULL);\n \tgrep_init(&opt, prefix);\n@@ -766,8 +752,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t */\n \targc = parse_options(argc, argv, prefix, options, grep_usage,\n \t\t\t     PARSE_OPT_KEEP_DASHDASH |\n-\t\t\t     PARSE_OPT_STOP_AT_NON_OPTION |\n-\t\t\t     PARSE_OPT_NO_INTERNAL_HELP);\n+\t\t\t     PARSE_OPT_STOP_AT_NON_OPTION);\n \tgrep_commit_pattern_type(pattern_type_arg, &opt);\n \n \tif (use_index && !startup_info->have_repository)\n-- \n2.6.3\n"},{"id":"273424","messageId":"564B00BD.7010709@web.de","threadId":"40812","inReplyTo":"564AFF32.1030406@web.de","subject":"[PATCH 5/5] show-ref: stop using PARSE_OPT_NO_INTERNAL_HELP","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2015-11-17T10:26:05Z","receivedAt":"2015-11-17T10:26:05Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"The flag PARSE_OPT_NO_INTERNAL_HELP is set to allow overriding the\noption -h, except when it's the only one given.  This is the default\nbehavior now, so remove the flag and the hand-rolled --help-all\nhandling.  The internal --help-all handler now actually shows hidden\noptions, i.e. -h in this case.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n builtin/show-ref.c | 12 +-----------\n 1 file changed, 1 insertion(+), 11 deletions(-)\n\ndiff --git a/builtin/show-ref.c b/builtin/show-ref.c\nindex 264c392..6d4e669 100644\n--- a/builtin/show-ref.c\n+++ b/builtin/show-ref.c\n@@ -161,11 +161,6 @@ static int exclude_existing_callback(const struct option *opt, const char *arg,\n \treturn 0;\n }\n \n-static int help_callback(const struct option *opt, const char *arg, int unset)\n-{\n-\treturn -1;\n-}\n-\n static const struct option show_ref_options[] = {\n \tOPT_BOOL(0, \"tags\", &tags_only, N_(\"only show tags (can be combined with heads)\")),\n \tOPT_BOOL(0, \"heads\", &heads_only, N_(\"only show heads (can be combined with tags)\")),\n@@ -186,18 +181,13 @@ static const struct option show_ref_options[] = {\n \t{ OPTION_CALLBACK, 0, \"exclude-existing\", &exclude_existing_arg,\n \t  N_(\"pattern\"), N_(\"show refs from stdin that aren't in local repository\"),\n \t  PARSE_OPT_OPTARG | PARSE_OPT_NONEG, exclude_existing_callback },\n-\t{ OPTION_CALLBACK, 0, \"help-all\", NULL, NULL, N_(\"show usage\"),\n-\t  PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, help_callback },\n \tOPT_END()\n };\n \n int cmd_show_ref(int argc, const char **argv, const char *prefix)\n {\n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(show_ref_usage, show_ref_options);\n-\n \targc = parse_options(argc, argv, prefix, show_ref_options,\n-\t\t\t     show_ref_usage, PARSE_OPT_NO_INTERNAL_HELP);\n+\t\t\t     show_ref_usage, 0);\n \n \tif (exclude_arg)\n \t\treturn exclude_existing(exclude_existing_arg);\n-- \n2.6.3\n"},{"id":"274014","messageId":"xmqqtwnyrraw.fsf@gitster.mtv.corp.google.com","threadId":"40812","inReplyTo":"564B00BD.7010709@web.de","subject":"Re: [PATCH 5/5] show-ref: stop using PARSE_OPT_NO_INTERNAL_HELP","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-04T18:40:07Z","receivedAt":"2015-12-04T18:40:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> The flag PARSE_OPT_NO_INTERNAL_HELP is set to allow overriding the\n> option -h, except when it's the only one given.\n> This is the default behavior now,...\n\nOK, so in the old world order, \"-h\" used to trigger the internal\nhelp even before consulting parse_short_opt() to see if there is a\nhandler supplied by the caller, and NO_INTERNAL_HELP was invented as\na kludge to to disable this behaviour.  The existing two users\n(dealt with 4/5 and 5/5) both used this mechanism to let their own\nhandler kick in, but they had to make \"-h\" without anything else on\nthe command line behave just like the internal one, while handling\n\"-h\" with something else on the command line do a custom thing.\n\nIn the new world order, internal \"-h\" handler is called only after\nseeing that parse_short_opt() decides there is no handler for \"-h\"\nas a fallback.\n\n\tSide note.  Not really.  Among the three uses of\n\tintenal_help in parse_options_step(), the first one (\"lone\n\t-h asks for help\") is used before we ask parse_short_opt().\n\nI wonder if we can/want to further tweak this in a follow-up\nseries.  If that is done, I think NO_INTERNAL_HELP can go away, as\nits only effect would be to make us say \"unknown option\" when \"-h\"\nalone was given from the command line for an options[] array that\ndoes not have a handler for \"-h\".\n"}]}