| Message ID | 20190129184327.30391-1-phillip.wood@talktalk.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] rebase -x: sanity check command | expand |
Phillip Wood <phillip.wood@talktalk.net> writes: > Thanks for the comments. I've updated the code as suggested by Dscho > and tried to make it clearer in the commit message that this is fixing > git creating a todo-list that it cannot parse. Will queue. Thanks.
Hi Phillip, On Tue, 29 Jan 2019, Phillip Wood wrote: > Range-diff against v1: > 1: 9626549aec ! 1: 726b84c300 rebase -x: sanity check command > @@ -2,8 +2,9 @@ > > rebase -x: sanity check command > > - If the user gives an empty argument to --exec then the rebase starts to > - run before erroring out with > + If the user gives an empty argument to --exec then git creates a todo > + list that it cannot parse. The rebase starts to run before erroring out > + with > > error: missing arguments for exec > error: invalid line 2: exec > @@ -33,21 +34,16 @@ > > +static int check_exec_cmd(const char *cmd) > +{ > -+ int non_blank = 0; > ++ if (strchr(cmd, '\n')) > ++ return error(_("exec commands cannot contain newlines")); > + > -+ while (*cmd) { > -+ if (*cmd == '\n') > -+ return error(_("exec commands cannot contain newlines")); > -+ if (!isspace(*cmd)) > -+ non_blank = 1; > -+ cmd++; > -+ } > ++ /* Does the command consist purely of whitespace? */ > ++ if (!cmd[strspn(cmd, " \t\r\f\v")]) > ++ return error(_("empty exec command")); > + > -+ if (non_blank) > -+ return 0; > -+ > -+ return error(_("empty exec command")); > ++ return 0; > +} > ++ > + > int cmd_rebase(int argc, const char **argv, const char *prefix) > { Looks great! Thanks, Dscho
On Tue, Jan 29 2019, Phillip Wood wrote: > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > If the user gives an empty argument to --exec then git creates a todo > list that it cannot parse. The rebase starts to run before erroring out > with > > error: missing arguments for exec > error: invalid line 2: exec > You can fix this with 'git rebase --edit-todo' and then run 'git rebase --continue'. > Or you can abort the rebase with 'git rebase --abort'. > > Instead check for empty commands before starting the rebase. > > Also check that the command does not contain any newlines as the > todo-list format is unable to cope with multiline commands. Note that > this changes the behavior, before this change one could do > > git rebase --exec='echo one > exec echo two' > > and it would insert two exec lines in the todo list, now it will error > out. > > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> > --- > > Apologies for the resend but I messed up the from address on the > previous send and I don't think it made it to the mailing list. > > Thanks for the comments. I've updated the code as suggested by Dscho > and tried to make it clearer in the commit message that this is fixing > git creating a todo-list that it cannot parse. > > Range-diff against v1: > 1: 9626549aec ! 1: 726b84c300 rebase -x: sanity check command > @@ -2,8 +2,9 @@ > > rebase -x: sanity check command > > - If the user gives an empty argument to --exec then the rebase starts to > - run before erroring out with > + If the user gives an empty argument to --exec then git creates a todo > + list that it cannot parse. The rebase starts to run before erroring out > + with > > error: missing arguments for exec > error: invalid line 2: exec > @@ -33,21 +34,16 @@ > > +static int check_exec_cmd(const char *cmd) > +{ > -+ int non_blank = 0; > ++ if (strchr(cmd, '\n')) > ++ return error(_("exec commands cannot contain newlines")); > + > -+ while (*cmd) { > -+ if (*cmd == '\n') > -+ return error(_("exec commands cannot contain newlines")); > -+ if (!isspace(*cmd)) > -+ non_blank = 1; > -+ cmd++; > -+ } > ++ /* Does the command consist purely of whitespace? */ > ++ if (!cmd[strspn(cmd, " \t\r\f\v")]) > ++ return error(_("empty exec command")); > + > -+ if (non_blank) > -+ return 0; > -+ > -+ return error(_("empty exec command")); > ++ return 0; > +} > ++ > + > int cmd_rebase(int argc, const char **argv, const char *prefix) > { > > builtin/rebase.c | 17 +++++++++++++++++ > t/t3404-rebase-interactive.sh | 19 +++++++++++++++++++ > 2 files changed, 36 insertions(+) > > diff --git a/builtin/rebase.c b/builtin/rebase.c > index 00de70365e..f75bd79b45 100644 > --- a/builtin/rebase.c > +++ b/builtin/rebase.c > @@ -793,6 +793,19 @@ static void set_reflog_action(struct rebase_options *options) > strbuf_release(&buf); > } > > +static int check_exec_cmd(const char *cmd) > +{ > + if (strchr(cmd, '\n')) > + return error(_("exec commands cannot contain newlines")); > + > + /* Does the command consist purely of whitespace? */ > + if (!cmd[strspn(cmd, " \t\r\f\v")]) > + return error(_("empty exec command")); > + > + return 0; > +} > + > + > int cmd_rebase(int argc, const char **argv, const char *prefix) > { > struct rebase_options options = { > @@ -1130,6 +1143,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) > } > } > > + for (i = 0; i < exec.nr; i++) > + if (check_exec_cmd(exec.items[i].string)) > + exit(1); > + > if (!(options.flags & REBASE_NO_QUIET)) > argv_array_push(&options.git_am_opts, "-q"); > > diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh > index 7a440e08d8..c98f64eb2d 100755 > --- a/t/t3404-rebase-interactive.sh > +++ b/t/t3404-rebase-interactive.sh > @@ -147,6 +147,25 @@ test_expect_success 'rebase -i with the exec command checks tree cleanness' ' > git rebase --continue > ' > > +test_expect_success 'rebase -x with empty command fails' ' > + test_when_finished "git rebase --abort ||:" && > + test_must_fail git rebase -x "" @ 2>actual && > + test_write_lines "error: empty exec command" >expected && > + test_i18ncmp expected actual && > + test_must_fail git rebase -x " " @ 2>actual && > + test_i18ncmp expected actual > +' > + > +LF=' > +' > +test_expect_success 'rebase -x with newline in command fails' ' > + test_when_finished "git rebase --abort ||:" && > + test_must_fail git rebase -x "a${LF}b" @ 2>actual && > + test_write_lines "error: exec commands cannot contain newlines" \ > + >expected && > + test_i18ncmp expected actual > +' > + > test_expect_success 'rebase -i with exec of inexistent command' ' > git checkout master && > test_when_finished "git rebase --abort" && This patch breaks the test suite (with these two new tests) under GIT_TEST_REBASE_USE_BUILTIN=false. So a 2.21.0-rc0 regression. It would have been better to raise this before the rc period, but I just noticed this now, but we can now: 1. Skip the test under that mode 2. Fix the shell code to do the same thing 3. Just remove the shell code & GIT_TEST_REBASE_USE_BUILTIN=false mode Maybe we should just do #3. The reason for the escape hatch was in case we had bugs, and now we've had a full release cycle, but maybe that's too early...
diff --git a/builtin/rebase.c b/builtin/rebase.c index 00de70365e..f75bd79b45 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -793,6 +793,19 @@ static void set_reflog_action(struct rebase_options *options) strbuf_release(&buf); } +static int check_exec_cmd(const char *cmd) +{ + if (strchr(cmd, '\n')) + return error(_("exec commands cannot contain newlines")); + + /* Does the command consist purely of whitespace? */ + if (!cmd[strspn(cmd, " \t\r\f\v")]) + return error(_("empty exec command")); + + return 0; +} + + int cmd_rebase(int argc, const char **argv, const char *prefix) { struct rebase_options options = { @@ -1130,6 +1143,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) } } + for (i = 0; i < exec.nr; i++) + if (check_exec_cmd(exec.items[i].string)) + exit(1); + if (!(options.flags & REBASE_NO_QUIET)) argv_array_push(&options.git_am_opts, "-q"); diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index 7a440e08d8..c98f64eb2d 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -147,6 +147,25 @@ test_expect_success 'rebase -i with the exec command checks tree cleanness' ' git rebase --continue ' +test_expect_success 'rebase -x with empty command fails' ' + test_when_finished "git rebase --abort ||:" && + test_must_fail git rebase -x "" @ 2>actual && + test_write_lines "error: empty exec command" >expected && + test_i18ncmp expected actual && + test_must_fail git rebase -x " " @ 2>actual && + test_i18ncmp expected actual +' + +LF=' +' +test_expect_success 'rebase -x with newline in command fails' ' + test_when_finished "git rebase --abort ||:" && + test_must_fail git rebase -x "a${LF}b" @ 2>actual && + test_write_lines "error: exec commands cannot contain newlines" \ + >expected && + test_i18ncmp expected actual +' + test_expect_success 'rebase -i with exec of inexistent command' ' git checkout master && test_when_finished "git rebase --abort" &&