* [PATCH 1/4] t1502: demonstrate rev-parse --parseopt option mis-parsing
@ 2017-09-17 22:28 Brandon Casey
2017-09-17 22:28 ` [PATCH 2/4] rev-parse parseopt: do not search help text for flag chars Brandon Casey
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Brandon Casey @ 2017-09-17 22:28 UTC (permalink / raw)
To: git; +Cc: ilya.bobyr, Brandon Casey
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 <tab> 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 <drafnel@gmail.com>
---
t/t1502-rev-parse-parseopt.sh | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
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] <args>...
@@ -62,6 +65,9 @@ test_expect_success 'test --parseopt help output' '
| --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
@@ -75,17 +81,17 @@ END_EXPECT
test_expect_success 'setup expect.1' "
cat > expect <<EOF
-set -- --foo --bar 'ham' -b -- 'arg'
+set -- --foo --bar 'ham' -b --aswitch -- 'arg'
EOF
"
-test_expect_success 'test --parseopt' '
- git rev-parse --parseopt -- --foo --bar=ham --baz arg < optionspec > 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
'
--
2.2.0.rc3
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH 2/4] rev-parse parseopt: do not search help text for flag chars
2017-09-17 22:28 [PATCH 1/4] t1502: demonstrate rev-parse --parseopt option mis-parsing Brandon Casey
@ 2017-09-17 22:28 ` Brandon Casey
2017-09-17 22:28 ` [PATCH 3/4] rev-parse parseopt: interpret any whitespace as start of help text Brandon Casey
2017-09-17 22:28 ` [PATCH 4/4] git-rebase: don't ignore unexpected command line arguments Brandon Casey
2 siblings, 0 replies; 4+ messages in thread
From: Brandon Casey @ 2017-09-17 22:28 UTC (permalink / raw)
To: git; +Cc: ilya.bobyr, Brandon Casey
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 <drafnel@gmail.com>
---
builtin/rev-parse.c | 6 ++++--
t/t1502-rev-parse-parseopt.sh | 4 ++--
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 2bd28d3..b19f677 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: (<short>|<short>,<long>|<long>)[*=?!]*<arghint>? SP+ <help> */
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
'
--
2.2.0.rc3
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH 3/4] rev-parse parseopt: interpret any whitespace as start of help text
2017-09-17 22:28 [PATCH 1/4] t1502: demonstrate rev-parse --parseopt option mis-parsing Brandon Casey
2017-09-17 22:28 ` [PATCH 2/4] rev-parse parseopt: do not search help text for flag chars Brandon Casey
@ 2017-09-17 22:28 ` Brandon Casey
2017-09-17 22:28 ` [PATCH 4/4] git-rebase: don't ignore unexpected command line arguments Brandon Casey
2 siblings, 0 replies; 4+ messages in thread
From: Brandon Casey @ 2017-09-17 22:28 UTC (permalink / raw)
To: git; +Cc: ilya.bobyr, Brandon Casey
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 <tab> 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 <drafnel@gmail.com>
---
builtin/rev-parse.c | 12 ++++++++++--
t/t1502-rev-parse-parseopt.sh | 2 +-
2 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index b19f677..351b1a3 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] <args>...
--
2.2.0.rc3
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH 4/4] git-rebase: don't ignore unexpected command line arguments
2017-09-17 22:28 [PATCH 1/4] t1502: demonstrate rev-parse --parseopt option mis-parsing Brandon Casey
2017-09-17 22:28 ` [PATCH 2/4] rev-parse parseopt: do not search help text for flag chars Brandon Casey
2017-09-17 22:28 ` [PATCH 3/4] rev-parse parseopt: interpret any whitespace as start of help text Brandon Casey
@ 2017-09-17 22:28 ` Brandon Casey
2 siblings, 0 replies; 4+ messages in thread
From: Brandon Casey @ 2017-09-17 22:28 UTC (permalink / raw)
To: git; +Cc: ilya.bobyr, Brandon Casey
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 <drafnel@gmail.com>
---
git-rebase.sh | 3 +++
1 file changed, 3 insertions(+)
diff --git a/git-rebase.sh b/git-rebase.sh
index ad8415e..6344e8d 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -350,6 +350,9 @@ do
shift
break
;;
+ *)
+ usage
+ ;;
esac
shift
done
--
2.2.0.rc3
^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-09-17 22:28 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-17 22:28 [PATCH 1/4] t1502: demonstrate rev-parse --parseopt option mis-parsing Brandon Casey
2017-09-17 22:28 ` [PATCH 2/4] rev-parse parseopt: do not search help text for flag chars Brandon Casey
2017-09-17 22:28 ` [PATCH 3/4] rev-parse parseopt: interpret any whitespace as start of help text Brandon Casey
2017-09-17 22:28 ` [PATCH 4/4] git-rebase: don't ignore unexpected command line arguments Brandon Casey
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).