{"thread":{"id":"46776","subject":"[PATCH 1/4] t1502: demonstrate rev-parse --parseopt option mis-parsing","startedAt":"2017-09-17T22:28:28Z","lastAt":"2017-09-17T22:28:36Z","messageCount":4,"participants":["Brandon Casey"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"328273","messageId":"1505687297-31183-1-git-send-email-drafnel@gmail.com","threadId":"46776","inReplyTo":null,"subject":"[PATCH 1/4] t1502: demonstrate rev-parse --parseopt option mis-parsing","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2017-09-17T22:28:14Z","receivedAt":"2017-09-17T22:28:28Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"Since commit 2d893df rev-parse will scan forward from the beginning of\nthe option string looking for a flag character.  If there are no flag\ncharacters then the scan will spill over into the help text and will\ninterpret the characters preceding the \"flag\" as part of the option-spec\ni.e. the long option name.\n\nFor example, the following option spec:\n\n    exclame this does something!\n\nwill produce this 'set' expression when --exclame is specified:\n\n    set -- --exclame this does something --\n\nwhich will be interpreted as four separate parameters by the shell.  And\nwill produce a help string that looks like:\n\n    --exclame this does something\n                          this does something!\n\ngit-rebase.sh has such an option (--autosquash), and so will add extra\nparameters to the 'set' expression when --autosquash is used.\ngit-rebase continues to work correctly though because when it parses the\narguments, it ignores ones that it does not recognize.\n\nAlso, rev-parse --parseopt does not currently interpret a tab character\nas a delimiter between the option spec and the help text.  If a tab is\nused at the end of the option spec, before the help text, and before a\nspace has been specified, then rev-parse will interpret the tab as part\nof the preceding component (either the long name or the arg hint).\n\nFor example, the following option spec (note: there is a <tab> between\n\"frotz\" and \"enable\"):\n\n    frotz\tenable frotzing\n\nwill produce this 'set' expression when --frotz is specified:\n\n    set -- --frotz  enable --\n\nwhich will be interpreted as 2 separate arguments by the shell.\n\ngit-rebase.sh has one of these too (--keep-empty).  In this case the tab\nis immediately followed by spaces so there are no additional parameters\nproduced on the command line.  The only side-effect is misalignment in\nthe help text.\n\nSigned-off-by: Brandon Casey <drafnel@gmail.com>\n---\n t/t1502-rev-parse-parseopt.sh | 18 ++++++++++++------\n 1 file changed, 12 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh\nindex 310f93f..910fc56 100755\n--- a/t/t1502-rev-parse-parseopt.sh\n+++ b/t/t1502-rev-parse-parseopt.sh\n@@ -28,6 +28,9 @@ test_expect_success 'setup optionspec' '\n |g,fluf?path     short and long option optional argument\n |longest=very-long-argument-hint  a very long argument hint\n |pair=key=value  with an equals sign in the hint\n+|aswitch help te=t contains? fl*g characters!`\n+|bswitch=hint\t hint has trailing tab character\n+|cswitch\t switch has trailing tab character\n |short-hint=a    with a one symbol hint\n |\n |Extras\n@@ -35,7 +38,7 @@ test_expect_success 'setup optionspec' '\n EOF\n '\n \n-test_expect_success 'test --parseopt help output' '\n+test_expect_failure 'test --parseopt help output' '\n \tsed -e \"s/^|//\" >expect <<\\END_EXPECT &&\n |cat <<\\EOF\n |usage: some-command [options] <args>...\n@@ -62,6 +65,9 @@ test_expect_success 'test --parseopt help output' '\n |    --longest <very-long-argument-hint>\n |                          a very long argument hint\n |    --pair <key=value>    with an equals sign in the hint\n+|    --aswitch             help te=t contains? fl*g characters!`\n+|    --bswitch <hint>      hint has trailing tab character\n+|    --cswitch             switch has trailing tab character\n |    --short-hint <a>      with a one symbol hint\n |\n |Extras\n@@ -75,17 +81,17 @@ END_EXPECT\n \n test_expect_success 'setup expect.1' \"\n \tcat > expect <<EOF\n-set -- --foo --bar 'ham' -b -- 'arg'\n+set -- --foo --bar 'ham' -b --aswitch -- 'arg'\n EOF\n \"\n \n-test_expect_success 'test --parseopt' '\n-\tgit rev-parse --parseopt -- --foo --bar=ham --baz arg < optionspec > output &&\n+test_expect_failure 'test --parseopt' '\n+\tgit rev-parse --parseopt -- --foo --bar=ham --baz --aswitch arg < optionspec > output &&\n \ttest_cmp expect output\n '\n \n-test_expect_success 'test --parseopt with mixed options and arguments' '\n-\tgit rev-parse --parseopt -- --foo arg --bar=ham --baz < optionspec > output &&\n+test_expect_failure 'test --parseopt with mixed options and arguments' '\n+\tgit rev-parse --parseopt -- --foo arg --bar=ham --baz --aswitch < optionspec > output &&\n \ttest_cmp expect output\n '\n \n-- \n2.2.0.rc3\n\n"},{"id":"328274","messageId":"1505687297-31183-2-git-send-email-drafnel@gmail.com","threadId":"46776","inReplyTo":"1505687297-31183-1-git-send-email-drafnel@gmail.com","subject":"[PATCH 2/4] rev-parse parseopt: do not search help text for flag chars","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2017-09-17T22:28:15Z","receivedAt":"2017-09-17T22:28:33Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"When searching for flag characters in the option spec, we should ensure\nthe search stays within the bounds of the option spec and does not enter\nthe help text portion of the spec.  So when we find the boundary white\nspace marking the start of the help text, let's mark it with a nul\ncharacter.  Then when we search for flag characters starting from the\nbeginning of the string we'll stop at the nul and won't enter the help\ntext.\n\nNow, the following option spec:\n\n    exclame this does something!\n\nwill produce this 'set' expression when --exclame is specified:\n\n    set -- --exclame --\n\ninstead of this one:\n\n    set -- --exclame this does something --\n\nMark t1502.4 and t1502.5 as fixed.\n\nSigned-off-by: Brandon Casey <drafnel@gmail.com>\n---\n builtin/rev-parse.c           | 6 ++++--\n t/t1502-rev-parse-parseopt.sh | 4 ++--\n 2 files changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex 2bd28d3..b19f677 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -434,7 +434,7 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)\n \t/* parse: (<short>|<short>,<long>|<long>)[*=?!]*<arghint>? SP+ <help> */\n \twhile (strbuf_getline(&sb, stdin) != EOF) {\n \t\tconst char *s;\n-\t\tconst char *help;\n+\t\tchar *help;\n \t\tstruct option *o;\n \n \t\tif (!sb.len)\n@@ -451,8 +451,10 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)\n \t\t\tcontinue;\n \t\t}\n \n+\t\t*help = '\\0';\n+\n \t\to->type = OPTION_CALLBACK;\n-\t\to->help = xstrdup(skipspaces(help));\n+\t\to->help = xstrdup(skipspaces(help+1));\n \t\to->value = &parsed;\n \t\to->flags = PARSE_OPT_NOARG;\n \t\to->callback = &parseopt_dump;\ndiff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh\nindex 910fc56..3d895e0 100755\n--- a/t/t1502-rev-parse-parseopt.sh\n+++ b/t/t1502-rev-parse-parseopt.sh\n@@ -85,12 +85,12 @@ set -- --foo --bar 'ham' -b --aswitch -- 'arg'\n EOF\n \"\n \n-test_expect_failure 'test --parseopt' '\n+test_expect_success 'test --parseopt' '\n \tgit rev-parse --parseopt -- --foo --bar=ham --baz --aswitch arg < optionspec > output &&\n \ttest_cmp expect output\n '\n \n-test_expect_failure 'test --parseopt with mixed options and arguments' '\n+test_expect_success 'test --parseopt with mixed options and arguments' '\n \tgit rev-parse --parseopt -- --foo arg --bar=ham --baz --aswitch < optionspec > output &&\n \ttest_cmp expect output\n '\n-- \n2.2.0.rc3\n\n"},{"id":"328275","messageId":"1505687297-31183-3-git-send-email-drafnel@gmail.com","threadId":"46776","inReplyTo":"1505687297-31183-1-git-send-email-drafnel@gmail.com","subject":"[PATCH 3/4] rev-parse parseopt: interpret any whitespace as start of help text","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2017-09-17T22:28:16Z","receivedAt":"2017-09-17T22:28:35Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"Currently, rev-parse only interprets a space ' ' character as the\ndelimiter between the option spec and the help text.  So if a tab\ncharacter is placed between the option spec and the help text, it will\nbe interpreted as part of the long option name or as part of the arg\nhint.  If it is interpreted as part of the long option name, then\nrev-parse will produce what will be interpreted as multiple arguments\non the command line.\n\nFor example, the following option spec (note: there is a <tab> between\n\"frotz\" and \"enable\"):\n\n    frotz\tenable frotzing\n\nwill produce the following set expression when --frotz is used:\n\n    set -- --frotz --\n\ninstead of this:\n\n    set -- --frotz  enable --\n\nMark t1502.2 as fixed.\n\nSigned-off-by: Brandon Casey <drafnel@gmail.com>\n---\n builtin/rev-parse.c           | 12 ++++++++++--\n t/t1502-rev-parse-parseopt.sh |  2 +-\n 2 files changed, 11 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex b19f677..351b1a3 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -387,6 +387,14 @@ static const char *skipspaces(const char *s)\n \treturn s;\n }\n \n+static char *findspace(const char *s)\n+{\n+\tfor (; *s; s++)\n+\t\tif (isspace(*s))\n+\t\t\treturn (char*)s;\n+\treturn NULL;\n+}\n+\n static int cmd_parseopt(int argc, const char **argv, const char *prefix)\n {\n \tstatic int keep_dashdash = 0, stop_at_non_option = 0;\n@@ -444,8 +452,8 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)\n \t\tmemset(opts + onb, 0, sizeof(opts[onb]));\n \n \t\to = &opts[onb++];\n-\t\thelp = strchr(sb.buf, ' ');\n-\t\tif (!help || *sb.buf == ' ') {\n+\t\thelp = findspace(sb.buf);\n+\t\tif (!help || sb.buf == help) {\n \t\t\to->type = OPTION_GROUP;\n \t\t\to->help = xstrdup(skipspaces(sb.buf));\n \t\t\tcontinue;\ndiff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh\nindex 3d895e0..6e1b45f 100755\n--- a/t/t1502-rev-parse-parseopt.sh\n+++ b/t/t1502-rev-parse-parseopt.sh\n@@ -38,7 +38,7 @@ test_expect_success 'setup optionspec' '\n EOF\n '\n \n-test_expect_failure 'test --parseopt help output' '\n+test_expect_success 'test --parseopt help output' '\n \tsed -e \"s/^|//\" >expect <<\\END_EXPECT &&\n |cat <<\\EOF\n |usage: some-command [options] <args>...\n-- \n2.2.0.rc3\n\n"},{"id":"328276","messageId":"1505687297-31183-4-git-send-email-drafnel@gmail.com","threadId":"46776","inReplyTo":"1505687297-31183-1-git-send-email-drafnel@gmail.com","subject":"[PATCH 4/4] git-rebase: don't ignore unexpected command line arguments","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2017-09-17T22:28:17Z","receivedAt":"2017-09-17T22:28:36Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"Currently, git-rebase will silently ignore any unexpected command-line\nswitches and arguments (the command-line produced by git rev-parse).\nThis allowed the rev-parse bug, fixed in the preceding commits, to go\nunnoticed.  Let's make sure that doesn't happen again.  We shouldn't be\nignoring unexpected arguments.  Let's not.\n\nSigned-off-by: Brandon Casey <drafnel@gmail.com>\n---\n git-rebase.sh | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/git-rebase.sh b/git-rebase.sh\nindex ad8415e..6344e8d 100755\n--- a/git-rebase.sh\n+++ b/git-rebase.sh\n@@ -350,6 +350,9 @@ do\n \t\tshift\n \t\tbreak\n \t\t;;\n+\t*)\n+\t\tusage\n+\t\t;;\n \tesac\n \tshift\n done\n-- \n2.2.0.rc3\n\n"}]}