From f221861e490d2c57b960cf86c5bc37279a3ebc2b Mon Sep 17 00:00:00 2001 From: Brandon Casey Date: Sun, 17 Sep 2017 15:28:14 -0700 Subject: t1502: demonstrate rev-parse --parseopt option mis-parsing Since commit 2d893df rev-parse will scan forward from the beginning of the option string looking for a flag character. If there are no flag characters then the scan will spill over into the help text and will interpret the characters preceding the "flag" as part of the option-spec i.e. the long option name. For example, the following option spec: exclame this does something! will produce this 'set' expression when --exclame is specified: set -- --exclame this does something -- which will be interpreted as four separate parameters by the shell. And will produce a help string that looks like: --exclame this does something this does something! git-rebase.sh has such an option (--autosquash), and so will add extra parameters to the 'set' expression when --autosquash is used. git-rebase continues to work correctly though because when it parses the arguments, it ignores ones that it does not recognize. Also, rev-parse --parseopt does not currently interpret a tab character as a delimiter between the option spec and the help text. If a tab is used at the end of the option spec, before the help text, and before a space has been specified, then rev-parse will interpret the tab as part of the preceding component (either the long name or the arg hint). For example, the following option spec (note: there is a between "frotz" and "enable"): frotz enable frotzing will produce this 'set' expression when --frotz is specified: set -- --frotz enable -- which will be interpreted as 2 separate arguments by the shell. git-rebase.sh has one of these too (--keep-empty). In this case the tab is immediately followed by spaces so there are no additional parameters produced on the command line. The only side-effect is misalignment in the help text. Signed-off-by: Brandon Casey Signed-off-by: Junio C Hamano diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh index 310f93f..910fc56 100755 --- a/t/t1502-rev-parse-parseopt.sh +++ b/t/t1502-rev-parse-parseopt.sh @@ -28,6 +28,9 @@ test_expect_success 'setup optionspec' ' |g,fluf?path short and long option optional argument |longest=very-long-argument-hint a very long argument hint |pair=key=value with an equals sign in the hint +|aswitch help te=t contains? fl*g characters!` +|bswitch=hint hint has trailing tab character +|cswitch switch has trailing tab character |short-hint=a with a one symbol hint | |Extras @@ -35,7 +38,7 @@ test_expect_success 'setup optionspec' ' EOF ' -test_expect_success 'test --parseopt help output' ' +test_expect_failure 'test --parseopt help output' ' sed -e "s/^|//" >expect <<\END_EXPECT && |cat <<\EOF |usage: some-command [options] ... @@ -62,6 +65,9 @@ test_expect_success 'test --parseopt help output' ' | --longest | a very long argument hint | --pair with an equals sign in the hint +| --aswitch help te=t contains? fl*g characters!` +| --bswitch hint has trailing tab character +| --cswitch switch has trailing tab character | --short-hint with a one symbol hint | |Extras @@ -75,17 +81,17 @@ END_EXPECT test_expect_success 'setup expect.1' " cat > expect < output && +test_expect_failure 'test --parseopt' ' + git rev-parse --parseopt -- --foo --bar=ham --baz --aswitch arg < optionspec > output && test_cmp expect output ' -test_expect_success 'test --parseopt with mixed options and arguments' ' - git rev-parse --parseopt -- --foo arg --bar=ham --baz < optionspec > output && +test_expect_failure 'test --parseopt with mixed options and arguments' ' + git rev-parse --parseopt -- --foo arg --bar=ham --baz --aswitch < optionspec > output && test_cmp expect output ' -- cgit v0.10.2-6-g49f6 From 28a8d0f77a27646dcbd87f0718b79501113cb82c Mon Sep 17 00:00:00 2001 From: Brandon Casey Date: Sun, 17 Sep 2017 15:28:15 -0700 Subject: rev-parse parseopt: do not search help text for flag chars When searching for flag characters in the option spec, we should ensure the search stays within the bounds of the option spec and does not enter the help text portion of the spec. So when we find the boundary white space marking the start of the help text, let's mark it with a nul character. Then when we search for flag characters starting from the beginning of the string we'll stop at the nul and won't enter the help text. Now, the following option spec: exclame this does something! will produce this 'set' expression when --exclame is specified: set -- --exclame -- instead of this one: set -- --exclame this does something -- Mark t1502.4 and t1502.5 as fixed. Signed-off-by: Brandon Casey Signed-off-by: Junio C Hamano diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c index c78b7b3..1cd55a2 100644 --- a/builtin/rev-parse.c +++ b/builtin/rev-parse.c @@ -434,7 +434,7 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix) /* parse: (|,|)[*=?!]*? SP+ */ while (strbuf_getline(&sb, stdin) != EOF) { const char *s; - const char *help; + char *help; struct option *o; if (!sb.len) @@ -451,8 +451,10 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix) continue; } + *help = '\0'; + o->type = OPTION_CALLBACK; - o->help = xstrdup(skipspaces(help)); + o->help = xstrdup(skipspaces(help+1)); o->value = &parsed; o->flags = PARSE_OPT_NOARG; o->callback = &parseopt_dump; diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh index 910fc56..3d895e0 100755 --- a/t/t1502-rev-parse-parseopt.sh +++ b/t/t1502-rev-parse-parseopt.sh @@ -85,12 +85,12 @@ set -- --foo --bar 'ham' -b --aswitch -- 'arg' EOF " -test_expect_failure 'test --parseopt' ' +test_expect_success 'test --parseopt' ' git rev-parse --parseopt -- --foo --bar=ham --baz --aswitch arg < optionspec > output && test_cmp expect output ' -test_expect_failure 'test --parseopt with mixed options and arguments' ' +test_expect_success 'test --parseopt with mixed options and arguments' ' git rev-parse --parseopt -- --foo arg --bar=ham --baz --aswitch < optionspec > output && test_cmp expect output ' -- cgit v0.10.2-6-g49f6 From 33e75122f483f887d4db7ccec01f42dea7ee79fb Mon Sep 17 00:00:00 2001 From: Brandon Casey Date: Sun, 17 Sep 2017 15:28:16 -0700 Subject: rev-parse parseopt: interpret any whitespace as start of help text Currently, rev-parse only interprets a space ' ' character as the delimiter between the option spec and the help text. So if a tab character is placed between the option spec and the help text, it will be interpreted as part of the long option name or as part of the arg hint. If it is interpreted as part of the long option name, then rev-parse will produce what will be interpreted as multiple arguments on the command line. For example, the following option spec (note: there is a between "frotz" and "enable"): frotz enable frotzing will produce the following set expression when --frotz is used: set -- --frotz -- instead of this: set -- --frotz enable -- Mark t1502.2 as fixed. Signed-off-by: Brandon Casey Signed-off-by: Junio C Hamano diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c index 1cd55a2..b3f634d 100644 --- a/builtin/rev-parse.c +++ b/builtin/rev-parse.c @@ -387,6 +387,14 @@ static const char *skipspaces(const char *s) return s; } +static char *findspace(const char *s) +{ + for (; *s; s++) + if (isspace(*s)) + return (char*)s; + return NULL; +} + static int cmd_parseopt(int argc, const char **argv, const char *prefix) { static int keep_dashdash = 0, stop_at_non_option = 0; @@ -444,8 +452,8 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix) memset(opts + onb, 0, sizeof(opts[onb])); o = &opts[onb++]; - help = strchr(sb.buf, ' '); - if (!help || *sb.buf == ' ') { + help = findspace(sb.buf); + if (!help || sb.buf == help) { o->type = OPTION_GROUP; o->help = xstrdup(skipspaces(sb.buf)); continue; diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh index 3d895e0..6e1b45f 100755 --- a/t/t1502-rev-parse-parseopt.sh +++ b/t/t1502-rev-parse-parseopt.sh @@ -38,7 +38,7 @@ test_expect_success 'setup optionspec' ' EOF ' -test_expect_failure 'test --parseopt help output' ' +test_expect_success 'test --parseopt help output' ' sed -e "s/^|//" >expect <<\END_EXPECT && |cat <<\EOF |usage: some-command [options] ... -- cgit v0.10.2-6-g49f6 From 697bc8858114ddda705be6f6eb3f997b64efa659 Mon Sep 17 00:00:00 2001 From: Brandon Casey Date: Sun, 17 Sep 2017 15:28:17 -0700 Subject: git-rebase: don't ignore unexpected command line arguments Currently, git-rebase will silently ignore any unexpected command-line switches and arguments (the command-line produced by git rev-parse). This allowed the rev-parse bug, fixed in the preceding commits, to go unnoticed. Let's make sure that doesn't happen again. We shouldn't be ignoring unexpected arguments. Let's not. Signed-off-by: Brandon Casey Signed-off-by: Junio C Hamano diff --git a/git-rebase.sh b/git-rebase.sh index 2cf73b8..45f187b 100755 --- a/git-rebase.sh +++ b/git-rebase.sh @@ -348,6 +348,9 @@ do shift break ;; + *) + usage + ;; esac shift done -- cgit v0.10.2-6-g49f6 From c97ee171a6b5a7e41234d46341b496146fa08bf1 Mon Sep 17 00:00:00 2001 From: Brandon Casey Date: Sun, 24 Sep 2017 21:08:03 -0700 Subject: t0040,t1502: Demonstrate parse_options bugs When the option spec contains no switches or only hidden switches, parse_options will emit an extra blank line at the end of help output so that the help text will end in two blank lines instead of one. When parse_options produces internal help output after an error has occurred it will emit blank lines within the usage string to stdout instead of stderr. Update t/helper/test-parse-options.c to have a description body in the usage string to exercise this second bug and mark tests as failing in t0040. Add tests to t1502 to demonstrate both of these problems. Signed-off-by: Brandon Casey Signed-off-by: Junio C Hamano diff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c index 75fe883..630c76d 100644 --- a/t/helper/test-parse-options.c +++ b/t/helper/test-parse-options.c @@ -99,6 +99,8 @@ int cmd_main(int argc, const char **argv) const char *prefix = "prefix/"; const char *usage[] = { "test-parse-options ", + "", + "A helper function for the parse-options API.", NULL }; struct string_list expect = STRING_LIST_INIT_NODUP; diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh index 74d2cd7..a36434b 100755 --- a/t/t0040-parse-options.sh +++ b/t/t0040-parse-options.sh @@ -10,6 +10,8 @@ test_description='our own option parser' cat >expect <<\EOF usage: test-parse-options + A helper function for the parse-options API. + --yes get a boolean -D, --no-doubt begins with 'no-' -B, --no-fear be brave @@ -90,8 +92,8 @@ test_expect_success 'OPT_BOOL() is idempotent #2' 'check boolean: 1 -DB' test_expect_success 'OPT_BOOL() negation #1' 'check boolean: 0 -D --no-yes' test_expect_success 'OPT_BOOL() negation #2' 'check boolean: 0 -D --no-no-doubt' -test_expect_success 'OPT_BOOL() no negation #1' 'check_unknown_i18n --fear' -test_expect_success 'OPT_BOOL() no negation #2' 'check_unknown_i18n --no-no-fear' +test_expect_failure 'OPT_BOOL() no negation #1' 'check_unknown_i18n --fear' +test_expect_failure 'OPT_BOOL() no negation #2' 'check_unknown_i18n --no-no-fear' test_expect_success 'OPT_BOOL() positivation' 'check boolean: 0 -D --doubt' @@ -286,7 +288,7 @@ test_expect_success 'OPT_CALLBACK() and OPT_BIT() work' ' >expect -test_expect_success 'OPT_CALLBACK() and callback errors work' ' +test_expect_failure 'OPT_CALLBACK() and callback errors work' ' test_must_fail test-parse-options --no-length >output 2>output.err && test_i18ncmp expect output && test_i18ncmp expect.err output.err diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh index 6e1b45f..1bfa80f 100755 --- a/t/t1502-rev-parse-parseopt.sh +++ b/t/t1502-rev-parse-parseopt.sh @@ -38,6 +38,25 @@ test_expect_success 'setup optionspec' ' EOF ' +test_expect_success 'setup optionspec-no-switches' ' + sed -e "s/^|//" >optionspec_no_switches <<\EOF +|some-command [options] ... +| +|some-command does foo and bar! +|-- +EOF +' + +test_expect_success 'setup optionspec-only-hidden-switches' ' + sed -e "s/^|//" >optionspec_only_hidden_switches <<\EOF +|some-command [options] ... +| +|some-command does foo and bar! +|-- +|hidden1* A hidden switch +EOF +' + test_expect_success 'test --parseopt help output' ' sed -e "s/^|//" >expect <<\END_EXPECT && |cat <<\EOF @@ -79,6 +98,87 @@ END_EXPECT test_i18ncmp expect output ' +test_expect_failure 'test --parseopt help output no switches' ' + sed -e "s/^|//" >expect <<\END_EXPECT && +|cat <<\EOF +|usage: some-command [options] ... +| +| some-command does foo and bar! +| +|EOF +END_EXPECT + test_expect_code 129 git rev-parse --parseopt -- -h > output < optionspec_no_switches && + test_i18ncmp expect output +' + +test_expect_failure 'test --parseopt help output hidden switches' ' + sed -e "s/^|//" >expect <<\END_EXPECT && +|cat <<\EOF +|usage: some-command [options] ... +| +| some-command does foo and bar! +| +|EOF +END_EXPECT + test_expect_code 129 git rev-parse --parseopt -- -h > output < optionspec_only_hidden_switches && + test_i18ncmp expect output +' + +test_expect_success 'test --parseopt help-all output hidden switches' ' + sed -e "s/^|//" >expect <<\END_EXPECT && +|cat <<\EOF +|usage: some-command [options] ... +| +| some-command does foo and bar! +| +| --hidden1 A hidden switch +| +|EOF +END_EXPECT + test_expect_code 129 git rev-parse --parseopt -- --help-all > output < optionspec_only_hidden_switches && + test_i18ncmp expect output +' + +test_expect_failure 'test --parseopt invalid switch help output' ' + sed -e "s/^|//" >expect <<\END_EXPECT && +|error: unknown option `does-not-exist'\'' +|usage: some-command [options] ... +| +| some-command does foo and bar! +| +| -h, --help show the help +| --foo some nifty option --foo +| --bar ... some cool option --bar with an argument +| -b, --baz a short and long option +| +|An option group Header +| -C[...] option C with an optional argument +| -d, --data[=...] short and long option with an optional argument +| +|Argument hints +| -B short option required argument +| --bar2 long option required argument +| -e, --fuz +| short and long option required argument +| -s[] short option optional argument +| --long[=] long option optional argument +| -g, --fluf[=] short and long option optional argument +| --longest +| a very long argument hint +| --pair with an equals sign in the hint +| --aswitch help te=t contains? fl*g characters!` +| --bswitch hint has trailing tab character +| --cswitch switch has trailing tab character +| --short-hint with a one symbol hint +| +|Extras +| --extra1 line above used to cause a segfault but no longer does +| +END_EXPECT + test_expect_code 129 git rev-parse --parseopt -- --does-not-exist 1>/dev/null 2>output < optionspec && + test_i18ncmp expect output +' + test_expect_success 'setup expect.1' " cat > expect < Date: Sun, 24 Sep 2017 21:08:04 -0700 Subject: parse-options: write blank line to correct output stream When commit 54e6dc7 added translation support to parse-options, an fprintf was mistakenly replaced by a call to putchar(). Let's use fputc instead. Fixes t0040.11, t0040.12, t0040.33, and t1502.8. Signed-off-by: Brandon Casey Signed-off-by: Junio C Hamano diff --git a/parse-options.c b/parse-options.c index 0dd9fc6..6a03a52 100644 --- a/parse-options.c +++ b/parse-options.c @@ -599,7 +599,7 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx, if (**usagestr) fprintf_ln(outfile, _(" %s"), _(*usagestr)); else - putchar('\n'); + fputc('\n', outfile); usagestr++; } diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh index a36434b..0c2fc81 100755 --- a/t/t0040-parse-options.sh +++ b/t/t0040-parse-options.sh @@ -92,8 +92,8 @@ test_expect_success 'OPT_BOOL() is idempotent #2' 'check boolean: 1 -DB' test_expect_success 'OPT_BOOL() negation #1' 'check boolean: 0 -D --no-yes' test_expect_success 'OPT_BOOL() negation #2' 'check boolean: 0 -D --no-no-doubt' -test_expect_failure 'OPT_BOOL() no negation #1' 'check_unknown_i18n --fear' -test_expect_failure 'OPT_BOOL() no negation #2' 'check_unknown_i18n --no-no-fear' +test_expect_success 'OPT_BOOL() no negation #1' 'check_unknown_i18n --fear' +test_expect_success 'OPT_BOOL() no negation #2' 'check_unknown_i18n --no-no-fear' test_expect_success 'OPT_BOOL() positivation' 'check boolean: 0 -D --doubt' @@ -288,7 +288,7 @@ test_expect_success 'OPT_CALLBACK() and OPT_BIT() work' ' >expect -test_expect_failure 'OPT_CALLBACK() and callback errors work' ' +test_expect_success 'OPT_CALLBACK() and callback errors work' ' test_must_fail test-parse-options --no-length >output 2>output.err && test_i18ncmp expect output && test_i18ncmp expect.err output.err diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh index 1bfa80f..ce7dda1 100755 --- a/t/t1502-rev-parse-parseopt.sh +++ b/t/t1502-rev-parse-parseopt.sh @@ -139,7 +139,7 @@ END_EXPECT test_i18ncmp expect output ' -test_expect_failure 'test --parseopt invalid switch help output' ' +test_expect_success 'test --parseopt invalid switch help output' ' sed -e "s/^|//" >expect <<\END_EXPECT && |error: unknown option `does-not-exist'\'' |usage: some-command [options] ... -- cgit v0.10.2-6-g49f6 From a6304fa4c2f57b08ec0acea9f91c188f284f8374 Mon Sep 17 00:00:00 2001 From: Brandon Casey Date: Sun, 24 Sep 2017 21:08:05 -0700 Subject: parse-options: only insert newline in help text if needed Currently, when parse_options() produces a help message it always emits a blank line after the usage text to separate it from the options text. If the option spec does not define any switches, or only defines hidden switches that will not be displayed, then the help text will end up with two trailing blank lines instead of one. Let's defer emitting the blank line between the usage text and the options text until it is clear that the options section will not be empty. Fixes t1502.5, t1502.6. Signed-off-by: Brandon Casey Signed-off-by: Junio C Hamano diff --git a/parse-options.c b/parse-options.c index 6a03a52..fca7159 100644 --- a/parse-options.c +++ b/parse-options.c @@ -581,6 +581,7 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx, const struct option *opts, int full, int err) { FILE *outfile = err ? stderr : stdout; + int need_newline; if (!usagestr) return PARSE_OPT_HELP; @@ -603,8 +604,7 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx, usagestr++; } - if (opts->type != OPTION_GROUP) - fputc('\n', outfile); + need_newline = 1; for (; opts->type != OPTION_END; opts++) { size_t pos; @@ -612,6 +612,7 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx, if (opts->type == OPTION_GROUP) { fputc('\n', outfile); + need_newline = 0; if (*opts->help) fprintf(outfile, "%s\n", _(opts->help)); continue; @@ -619,6 +620,11 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx, if (!full && (opts->flags & PARSE_OPT_HIDDEN)) continue; + if (need_newline) { + fputc('\n', outfile); + need_newline = 0; + } + pos = fprintf(outfile, " "); if (opts->short_name) { if (opts->flags & PARSE_OPT_NODASH) diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh index ce7dda1..a859abe 100755 --- a/t/t1502-rev-parse-parseopt.sh +++ b/t/t1502-rev-parse-parseopt.sh @@ -98,7 +98,7 @@ END_EXPECT test_i18ncmp expect output ' -test_expect_failure 'test --parseopt help output no switches' ' +test_expect_success 'test --parseopt help output no switches' ' sed -e "s/^|//" >expect <<\END_EXPECT && |cat <<\EOF |usage: some-command [options] ... @@ -111,7 +111,7 @@ END_EXPECT test_i18ncmp expect output ' -test_expect_failure 'test --parseopt help output hidden switches' ' +test_expect_success 'test --parseopt help output hidden switches' ' sed -e "s/^|//" >expect <<\END_EXPECT && |cat <<\EOF |usage: some-command [options] ... -- cgit v0.10.2-6-g49f6