From c67318ecb6ddef6b9ecf59a422c4a282594e602d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 2 Aug 2018 00:31:33 +0200 Subject: [PATCH 1/7] push: use PARSE_OPT_LITERAL_ARGHELP instead of unbalanced brackets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The option help text for the force-with-lease option to "git push" reads like this: $ git push -h 2>&1 | grep -e force-with-lease --force-with-lease[=:] which comes from having N_("refname>:" at both ends. It turns out that parse-options machinery takes the whole string and encloses it inside a pair of "<>", to make it easier for majority cases that uses a single token placeholder. The help string was written in a funnily unbalanced way knowing that the end result would balance out, by somebody who forgot the presence of PARSE_OPT_LITERAL_ARGHELP, which is the escape hatch mechanism designed to help such a case. We just should use the official escape hatch instead. Because ":" part can be omitted to ask Git to guess, it may be more correct to spell it as "[:]", but that is not the focus of this topic. Signed-off-by: Ævar Arnfjörð Bjarmason Helped-by: René Scharfe Signed-off-by: Junio C Hamano --- builtin/push.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/builtin/push.c b/builtin/push.c index 1c28427d82..ef4032a9ef 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -542,9 +542,9 @@ int cmd_push(int argc, const char **argv, const char *prefix) OPT_BIT( 0, "porcelain", &flags, N_("machine-readable output"), TRANSPORT_PUSH_PORCELAIN), OPT_BIT('f', "force", &flags, N_("force updates"), TRANSPORT_PUSH_FORCE), { OPTION_CALLBACK, - 0, CAS_OPT_NAME, &cas, N_("refname>::"), N_("require old value of ref to be at this value"), - PARSE_OPT_OPTARG, parseopt_push_cas_option }, + PARSE_OPT_OPTARG | PARSE_OPT_LITERAL_ARGHELP, parseopt_push_cas_option }, { OPTION_CALLBACK, 0, "recurse-submodules", &recurse_submodules, "check|on-demand|no", N_("control recursive pushing of submodules"), PARSE_OPT_OPTARG, option_parse_recurse_submodules }, From 8b5ebbed0e793f6fcf92100e600b7a4bdd6452e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Scharfe?= Date: Thu, 2 Aug 2018 21:17:35 +0200 Subject: [PATCH 2/7] add, update-index: fix --chmod argument help Don't translate the argument specification for --chmod; "+x" and "-x" are the literal strings that the commands accept. Separate alternatives using a pipe character instead of a slash, for consistency. Use the flag PARSE_OPT_LITERAL_ARGHELP to prevent parseopt from adding a pair of angular brackets around the argument help string, as that would wrongly indicate that users need to replace the literal strings with some kind of value. Signed-off-by: Rene Scharfe Signed-off-by: Junio C Hamano --- builtin/add.c | 4 +++- builtin/update-index.c | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/builtin/add.c b/builtin/add.c index bf01d89e28..110f2ae0dc 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -306,7 +306,9 @@ static struct option builtin_add_options[] = { OPT_BOOL( 0 , "refresh", &refresh_only, N_("don't add, only refresh the index")), OPT_BOOL( 0 , "ignore-errors", &ignore_add_errors, N_("just skip files which cannot be added because of errors")), OPT_BOOL( 0 , "ignore-missing", &ignore_missing, N_("check if - even missing - files are ignored in dry run")), - OPT_STRING( 0 , "chmod", &chmod_arg, N_("(+/-)x"), N_("override the executable bit of the listed files")), + { OPTION_STRING, 0, "chmod", &chmod_arg, "(+|-)x", + N_("override the executable bit of the listed files"), + PARSE_OPT_LITERAL_ARGHELP }, OPT_HIDDEN_BOOL(0, "warn-embedded-repo", &warn_on_embedded_repo, N_("warn when adding an embedded repository")), OPT_END(), diff --git a/builtin/update-index.c b/builtin/update-index.c index 52eaebae91..a2aee925a0 100644 --- a/builtin/update-index.c +++ b/builtin/update-index.c @@ -970,7 +970,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix) PARSE_OPT_NOARG | /* disallow --cacheinfo= form */ PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP, (parse_opt_cb *) cacheinfo_callback}, - {OPTION_CALLBACK, 0, "chmod", &set_executable_bit, N_("(+/-)x"), + {OPTION_CALLBACK, 0, "chmod", &set_executable_bit, "(+|-)x", N_("override the executable bit of the listed files"), PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP, chmod_callback}, From 9f6013a88d71860433a0c11571dc87aef3c9c809 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Scharfe?= Date: Thu, 2 Aug 2018 21:17:43 +0200 Subject: [PATCH 3/7] difftool: remove angular brackets from argument help Parseopt wraps arguments in a pair of angular brackets by default, signifying that the user needs to replace it with a value of the documented type. Remove the pairs from the option definitions to duplication and confusion. Signed-off-by: Rene Scharfe Signed-off-by: Junio C Hamano --- builtin/difftool.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/builtin/difftool.c b/builtin/difftool.c index bcc79d1888..91dbef8f49 100644 --- a/builtin/difftool.c +++ b/builtin/difftool.c @@ -703,7 +703,7 @@ int cmd_difftool(int argc, const char **argv, const char *prefix) NULL, 1 }, OPT_BOOL(0, "symlinks", &symlinks, N_("use symlinks in dir-diff mode")), - OPT_STRING('t', "tool", &difftool_cmd, N_(""), + OPT_STRING('t', "tool", &difftool_cmd, N_("tool"), N_("use the specified diff tool")), OPT_BOOL(0, "tool-help", &tool_help, N_("print a list of diff tools that may be used with " @@ -711,7 +711,7 @@ int cmd_difftool(int argc, const char **argv, const char *prefix) OPT_BOOL(0, "trust-exit-code", &trust_exit_code, N_("make 'git-difftool' exit when an invoked diff " "tool returns a non - zero exit code")), - OPT_STRING('x', "extcmd", &extcmd, N_(""), + OPT_STRING('x', "extcmd", &extcmd, N_("command"), N_("specify a custom command for viewing diffs")), OPT_END() }; From cbd23de8bbf2e98d629f8e242901dd08f723106e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Scharfe?= Date: Thu, 2 Aug 2018 21:17:50 +0200 Subject: [PATCH 4/7] pack-objects: specify --index-version argument help explicitly Wrap both placeholders in the argument help string in angular brackets to signal that users needs replace them with some actual value. Use the flag PARSE_OPT_LITERAL_ARGHELP to prevent parseopt from adding another pair. Signed-off-by: Rene Scharfe Signed-off-by: Junio C Hamano --- builtin/pack-objects.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index 6b9cfc289d..5d7566a40b 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -2941,9 +2941,9 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix) OPT_BOOL(0, "all-progress-implied", &all_progress_implied, N_("similar to --all-progress when progress meter is shown")), - { OPTION_CALLBACK, 0, "index-version", NULL, N_("version[,offset]"), + { OPTION_CALLBACK, 0, "index-version", NULL, N_("[,]"), N_("write the pack index file in the specified idx format version"), - 0, option_parse_index_version }, + PARSE_OPT_LITERAL_ARGHELP, option_parse_index_version }, OPT_MAGNITUDE(0, "max-pack-size", &pack_size_limit, N_("maximum size of each output pack file")), OPT_BOOL(0, "local", &local, From 1758abed1a54568eab720177fe60178203887e80 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Scharfe?= Date: Thu, 2 Aug 2018 21:17:58 +0200 Subject: [PATCH 5/7] send-pack: specify --force-with-lease argument help explicitly Wrap each part of the argument help string in angular brackets to show that users need to replace them with actual values. Do that explicitly to balance the pairs nicely in the code and avoid confusing casual readers. Add the flag PARSE_OPT_LITERAL_ARGHELP to keep parseopt from adding another pair. Signed-off-by: Rene Scharfe Signed-off-by: Junio C Hamano --- builtin/send-pack.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/builtin/send-pack.c b/builtin/send-pack.c index fc4f0bb5fb..c8af374dee 100644 --- a/builtin/send-pack.c +++ b/builtin/send-pack.c @@ -177,9 +177,10 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix) OPT_BOOL(0, "stdin", &from_stdin, N_("read refs from stdin")), OPT_BOOL(0, "helper-status", &helper_status, N_("print status from remote helper")), { OPTION_CALLBACK, - 0, CAS_OPT_NAME, &cas, N_("refname>::"), N_("require old value of ref to be at this value"), - PARSE_OPT_OPTARG, parseopt_push_cas_option }, + PARSE_OPT_OPTARG | PARSE_OPT_LITERAL_ARGHELP, + parseopt_push_cas_option }, OPT_END() }; From b8ade4c5766774207e05c8f92e262f8f6835bd3d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Scharfe?= Date: Thu, 2 Aug 2018 21:18:06 +0200 Subject: [PATCH 6/7] shortlog: correct option help for -w Wrap the placeholders in the option help string for -w in pairs of angular brackets to document that users need to replace them with actual numbers. Use the flag PARSE_OPT_LITERAL_ARGHELP to prevent parseopt from adding another pair. Signed-off-by: Rene Scharfe Signed-off-by: Junio C Hamano --- builtin/shortlog.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/builtin/shortlog.c b/builtin/shortlog.c index e29875b843..98508fc556 100644 --- a/builtin/shortlog.c +++ b/builtin/shortlog.c @@ -267,8 +267,10 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix) N_("Suppress commit descriptions, only provides commit count")), OPT_BOOL('e', "email", &log.email, N_("Show the email address of each author")), - { OPTION_CALLBACK, 'w', NULL, &log, N_("w[,i1[,i2]]"), - N_("Linewrap output"), PARSE_OPT_OPTARG, &parse_wrap_args }, + { OPTION_CALLBACK, 'w', NULL, &log, N_("[,[,]]"), + N_("Linewrap output"), + PARSE_OPT_OPTARG | PARSE_OPT_LITERAL_ARGHELP, + &parse_wrap_args }, OPT_END(), }; From 5f0df44cd79a953c287ccb598896ea7aaa2cc9e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Scharfe?= Date: Thu, 2 Aug 2018 21:18:14 +0200 Subject: [PATCH 7/7] parse-options: automatically infer PARSE_OPT_LITERAL_ARGHELP Parseopt wraps argument help strings in a pair of angular brackets by default, to tell users that they need to replace it with an actual value. This is useful in most cases, because most option arguments are indeed single values of a certain type. The option PARSE_OPT_LITERAL_ARGHELP needs to be used in option definitions with arguments that have multiple parts or are literal strings. Stop adding these angular brackets if special characters are present, as they indicate that we don't deal with a simple placeholder. This simplifies the code a bit and makes defining special options slightly easier. Remove the flag PARSE_OPT_LITERAL_ARGHELP in the cases where the new and more cautious handling suffices. Signed-off-by: Rene Scharfe Signed-off-by: Junio C Hamano --- builtin/add.c | 5 ++--- builtin/pack-objects.c | 2 +- builtin/read-tree.c | 2 +- builtin/send-pack.c | 3 +-- builtin/shortlog.c | 3 +-- builtin/show-branch.c | 2 +- builtin/update-index.c | 2 +- builtin/write-tree.c | 5 ++--- parse-options.c | 3 ++- 9 files changed, 12 insertions(+), 15 deletions(-) diff --git a/builtin/add.c b/builtin/add.c index 110f2ae0dc..8352bdf71f 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -306,9 +306,8 @@ static struct option builtin_add_options[] = { OPT_BOOL( 0 , "refresh", &refresh_only, N_("don't add, only refresh the index")), OPT_BOOL( 0 , "ignore-errors", &ignore_add_errors, N_("just skip files which cannot be added because of errors")), OPT_BOOL( 0 , "ignore-missing", &ignore_missing, N_("check if - even missing - files are ignored in dry run")), - { OPTION_STRING, 0, "chmod", &chmod_arg, "(+|-)x", - N_("override the executable bit of the listed files"), - PARSE_OPT_LITERAL_ARGHELP }, + OPT_STRING(0, "chmod", &chmod_arg, "(+|-)x", + N_("override the executable bit of the listed files")), OPT_HIDDEN_BOOL(0, "warn-embedded-repo", &warn_on_embedded_repo, N_("warn when adding an embedded repository")), OPT_END(), diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index 5d7566a40b..1529a75e03 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -2943,7 +2943,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix) N_("similar to --all-progress when progress meter is shown")), { OPTION_CALLBACK, 0, "index-version", NULL, N_("[,]"), N_("write the pack index file in the specified idx format version"), - PARSE_OPT_LITERAL_ARGHELP, option_parse_index_version }, + 0, option_parse_index_version }, OPT_MAGNITUDE(0, "max-pack-size", &pack_size_limit, N_("maximum size of each output pack file")), OPT_BOOL(0, "local", &local, diff --git a/builtin/read-tree.c b/builtin/read-tree.c index bf87a2710b..3a7fd6394e 100644 --- a/builtin/read-tree.c +++ b/builtin/read-tree.c @@ -134,7 +134,7 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix) N_("same as -m, but discard unmerged entries")), { OPTION_STRING, 0, "prefix", &opts.prefix, N_("/"), N_("read the tree into the index under /"), - PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP }, + PARSE_OPT_NONEG }, OPT_BOOL('u', NULL, &opts.update, N_("update working tree with merge result")), { OPTION_CALLBACK, 0, "exclude-per-directory", &opts, diff --git a/builtin/send-pack.c b/builtin/send-pack.c index c8af374dee..a4f7dea115 100644 --- a/builtin/send-pack.c +++ b/builtin/send-pack.c @@ -179,8 +179,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix) { OPTION_CALLBACK, 0, CAS_OPT_NAME, &cas, N_(":"), N_("require old value of ref to be at this value"), - PARSE_OPT_OPTARG | PARSE_OPT_LITERAL_ARGHELP, - parseopt_push_cas_option }, + PARSE_OPT_OPTARG, parseopt_push_cas_option }, OPT_END() }; diff --git a/builtin/shortlog.c b/builtin/shortlog.c index 98508fc556..581293ee7b 100644 --- a/builtin/shortlog.c +++ b/builtin/shortlog.c @@ -268,8 +268,7 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix) OPT_BOOL('e', "email", &log.email, N_("Show the email address of each author")), { OPTION_CALLBACK, 'w', NULL, &log, N_("[,[,]]"), - N_("Linewrap output"), - PARSE_OPT_OPTARG | PARSE_OPT_LITERAL_ARGHELP, + N_("Linewrap output"), PARSE_OPT_OPTARG, &parse_wrap_args }, OPT_END(), }; diff --git a/builtin/show-branch.c b/builtin/show-branch.c index e8a4aa40cb..371b8c8b84 100644 --- a/builtin/show-branch.c +++ b/builtin/show-branch.c @@ -655,7 +655,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix) { OPTION_CALLBACK, 'g', "reflog", &reflog_base, N_("[,]"), N_("show most recent ref-log entries starting at " "base"), - PARSE_OPT_OPTARG | PARSE_OPT_LITERAL_ARGHELP, + PARSE_OPT_OPTARG, parse_reflog_param }, OPT_END() }; diff --git a/builtin/update-index.c b/builtin/update-index.c index a2aee925a0..6700a632be 100644 --- a/builtin/update-index.c +++ b/builtin/update-index.c @@ -972,7 +972,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix) (parse_opt_cb *) cacheinfo_callback}, {OPTION_CALLBACK, 0, "chmod", &set_executable_bit, "(+|-)x", N_("override the executable bit of the listed files"), - PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP, + PARSE_OPT_NONEG, chmod_callback}, {OPTION_SET_INT, 0, "assume-unchanged", &mark_valid_only, NULL, N_("mark files as \"not changing\""), diff --git a/builtin/write-tree.c b/builtin/write-tree.c index bd0a78aa3c..e636419122 100644 --- a/builtin/write-tree.c +++ b/builtin/write-tree.c @@ -24,9 +24,8 @@ int cmd_write_tree(int argc, const char **argv, const char *unused_prefix) struct option write_tree_options[] = { OPT_BIT(0, "missing-ok", &flags, N_("allow missing objects"), WRITE_TREE_MISSING_OK), - { OPTION_STRING, 0, "prefix", &prefix, N_("/"), - N_("write tree object for a subdirectory ") , - PARSE_OPT_LITERAL_ARGHELP }, + OPT_STRING(0, "prefix", &prefix, N_("/"), + N_("write tree object for a subdirectory ")), { OPTION_BIT, 0, "ignore-cache-tree", &flags, NULL, N_("only useful for debugging"), PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, NULL, diff --git a/parse-options.c b/parse-options.c index fca7159646..2a2b97aec9 100644 --- a/parse-options.c +++ b/parse-options.c @@ -562,7 +562,8 @@ int parse_options(int argc, const char **argv, const char *prefix, static int usage_argh(const struct option *opts, FILE *outfile) { const char *s; - int literal = (opts->flags & PARSE_OPT_LITERAL_ARGHELP) || !opts->argh; + int literal = (opts->flags & PARSE_OPT_LITERAL_ARGHELP) || + !opts->argh || !!strpbrk(opts->argh, "()<>[]|"); if (opts->flags & PARSE_OPT_OPTARG) if (opts->long_name) s = literal ? "[=%s]" : "[=<%s>]";