| Message ID | 20190128102620.18222-1-phillip.wood@talktalk.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rebase -x: sanity check command | expand |
Phillip Wood <phillip.wood@talktalk.net> writes: > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > If the user gives an empty argument to --exec then 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'. Hmph. I do agree that the above makes an unfortunate end-user experience, but I would sort-of imagine that it would even be nicer for such an empty exec to behave as if it were "exec false" but with less severe error message, i.e. a way for the user to say "I want to break the sequence here and get an interactive session". We may not even need to add the "break" insn if we go that way and there is one less thing for users to learn. I dunno, but I tend to prefer giving a useful and safe behaviour to interactive users other than erroring out, when there _is_ such a safe behaviour that is obvious from the situation, and I feel that an empty "exec" is such a case. > 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' It is very good to check the input, regardless of what an empty "exec" should do.
Hi Junio, On Mon, 28 Jan 2019, Junio C Hamano wrote: > Phillip Wood <phillip.wood@talktalk.net> writes: > > > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > > > If the user gives an empty argument to --exec then 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'. > > Hmph. I do agree that the above makes an unfortunate end-user > experience, but I would sort-of imagine that it would even be nicer > for such an empty exec to behave as if it were "exec false" but with > less severe error message, i.e. a way for the user to say "I want to > break the sequence here and get an interactive session". We may not > even need to add the "break" insn if we go that way and there is one > less thing for users to learn. I dunno, but I tend to prefer giving > a useful and safe behaviour to interactive users other than erroring > out, when there _is_ such a safe behaviour that is obvious from the > situation, and I feel that an empty "exec" is such a case. That would make things unnecessarily confusing. An empty command is not `false` with a gentler error message. An empty command is a missing command. I am, however, concerned that special-casing an empty command will not make things better: if the user called `git rebase --exec=fasle`, they will *still* have to clean up their edit script. Or just `git rebase --abort`, which I would do whether I had forgotten to specify a command or whether I had a typo in my command. > > 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' > > It is very good to check the input, regardless of what an empty > "exec" should do. Should we then also check for incorrect quoting, missing commands, other errors? I am not sure that this path leads to sanity. Ciao, Dscho
Hi Phillip, On Mon, 28 Jan 2019, Phillip Wood wrote: > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > If the user gives an empty argument to --exec then 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'. And that's the same if you specify an incorrect command. In both cases, I would probably heed the second line of the advice: 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. This, however, makes a ton of sense to me. > diff --git a/builtin/rebase.c b/builtin/rebase.c > index 00de70365e..b6c54b03c1 100644 > --- a/builtin/rebase.c > +++ b/builtin/rebase.c > @@ -793,6 +793,24 @@ static void set_reflog_action(struct rebase_options *options) > strbuf_release(&buf); > } > > +static int check_exec_cmd(const char *cmd) > +{ > + int non_blank = 0; > + > + while (*cmd) { > + if (*cmd == '\n') > + return error(_("exec commands cannot contain newlines")); > + if (!isspace(*cmd)) > + non_blank = 1; > + cmd++; > + } > + > + if (non_blank) > + return 0; We are not in a performance critical path here, so I would prefer the readability of this code: if (strchr(cmd, '\n')) return error(...); And if you *really* must, /* Does the command consist purely of whitespace? */ if (!cmd[strspn(cmd, " \t\r\n")]) return error(...); But as I suggested also in a reply to Junio's answer: where would we stop to validate the commands? Ciao, Dscho > + > + return error(_("empty exec command")); > +} > + > int cmd_rebase(int argc, const char **argv, const char *prefix) > { > struct rebase_options options = { > @@ -1130,6 +1148,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" && > -- > 2.20.1 > >
Hi Dscho On 28/01/2019 22:03, Johannes Schindelin wrote: > Hi Phillip, > > On Mon, 28 Jan 2019, Phillip Wood wrote: > >> From: Phillip Wood <phillip.wood@dunelm.org.uk> >> >> If the user gives an empty argument to --exec then 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'. > > And that's the same if you specify an incorrect command. Not quite, the issue is that the todo list is invalid, not that the command fails - it never gets that far. Those errors are coming from parse_insn_line() and parse_insn_buffer(). > > In both cases, I would probably heed the second line of the advice: 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. > > This, however, makes a ton of sense to me. > >> diff --git a/builtin/rebase.c b/builtin/rebase.c >> index 00de70365e..b6c54b03c1 100644 >> --- a/builtin/rebase.c >> +++ b/builtin/rebase.c >> @@ -793,6 +793,24 @@ static void set_reflog_action(struct rebase_options *options) >> strbuf_release(&buf); >> } >> >> +static int check_exec_cmd(const char *cmd) >> +{ >> + int non_blank = 0; >> + >> + while (*cmd) { >> + if (*cmd == '\n') >> + return error(_("exec commands cannot contain newlines")); >> + if (!isspace(*cmd)) >> + non_blank = 1; >> + cmd++; >> + } >> + >> + if (non_blank) >> + return 0; > > We are not in a performance critical path here, so I would prefer the > readability of this code: > > if (strchr(cmd, '\n')) > return error(...); > > And if you *really* must, > > /* Does the command consist purely of whitespace? */ > if (!cmd[strspn(cmd, " \t\r\n")]) > return error(...); > > But as I suggested also in a reply to Junio's answer: where would we stop > to validate the commands? I'm not trying to validate the command (and I don't think we can/should) - just generate a todo list that can be parsed. Best Wishes Phillip > > Ciao, > Dscho > >> + >> + return error(_("empty exec command")); >> +} >> + >> int cmd_rebase(int argc, const char **argv, const char *prefix) >> { >> struct rebase_options options = { >> @@ -1130,6 +1148,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" && >> -- >> 2.20.1 >> >>
Dear Junio & Dscho On 28/01/2019 21:56, Johannes Schindelin wrote: > Hi Junio, > > On Mon, 28 Jan 2019, Junio C Hamano wrote: > >> Phillip Wood <phillip.wood@talktalk.net> writes: >> >>> From: Phillip Wood <phillip.wood@dunelm.org.uk> >>> >>> If the user gives an empty argument to --exec then 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'. >> >> Hmph. I do agree that the above makes an unfortunate end-user >> experience, but I would sort-of imagine that it would even be nicer >> for such an empty exec to behave as if it were "exec false" but with >> less severe error message, i.e. a way for the user to say "I want to >> break the sequence here and get an interactive session". We may not >> even need to add the "break" insn if we go that way and there is one >> less thing for users to learn. I dunno, but I tend to prefer giving >> a useful and safe behaviour to interactive users other than erroring >> out, when there _is_ such a safe behaviour that is obvious from the >> situation, and I feel that an empty "exec" is such a case. > > That would make things unnecessarily confusing. An empty command is not > `false` with a gentler error message. An empty command is a missing > command. I agree that having a special meaning to the empty command would be confusing. Also giving it on the command line only helps if you want to stop after each pick and my impression is that people want to break after specific commits to amend a fixup or something like that. > I am, however, concerned that special-casing an empty command will not > make things better: if the user called `git rebase --exec=fasle`, they > will *still* have to clean up their edit script. The empty commands create an invalid todo list which git cannot parse, this patch is not a step down the path of checking that the command exists or is valid shell - I don't think that would be possible in the general case. Best Wishes Phillip > Or just `git rebase --abort`, which I would do whether I had forgotten to > specify a command or whether I had a typo in my command. > >>> 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' >> >> It is very good to check the input, regardless of what an empty >> "exec" should do. > > Should we then also check for incorrect quoting, missing commands, other > errors? I am not sure that this path leads to sanity. > > Ciao, > Dscho >
Hi Phillip, [sorry for the double-send, I dropped the Cc: list by mistake the first time I replied] On Tue, 29 Jan 2019, Phillip Wood wrote: > On 28/01/2019 22:03, Johannes Schindelin wrote: > > Hi Phillip, > > > > On Mon, 28 Jan 2019, Phillip Wood wrote: > > > >> From: Phillip Wood <phillip.wood@dunelm.org.uk> > >> > >> If the user gives an empty argument to --exec then 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'. > > > > And that's the same if you specify an incorrect command. > > Not quite, the issue is that the todo list is invalid, not that the > command fails - it never gets that far. Those errors are coming from > parse_insn_line() and parse_insn_buffer(). Ah! I misunderstood "then the rebase starts to run" part. So please let me withdraw my objections to catch this error. However, I still would highly recommend to use `strspn()` to simplify the code. Thanks, Dscho > > In both cases, I would probably heed the second line of the advice: 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. > > > > This, however, makes a ton of sense to me. > > > >> diff --git a/builtin/rebase.c b/builtin/rebase.c > >> index 00de70365e..b6c54b03c1 100644 > >> --- a/builtin/rebase.c > >> +++ b/builtin/rebase.c > >> @@ -793,6 +793,24 @@ static void set_reflog_action(struct rebase_options *options) > >> strbuf_release(&buf); > >> } > >> > >> +static int check_exec_cmd(const char *cmd) > >> +{ > >> + int non_blank = 0; > >> + > >> + while (*cmd) { > >> + if (*cmd == '\n') > >> + return error(_("exec commands cannot contain newlines")); > >> + if (!isspace(*cmd)) > >> + non_blank = 1; > >> + cmd++; > >> + } > >> + > >> + if (non_blank) > >> + return 0; > > > > We are not in a performance critical path here, so I would prefer the > > readability of this code: > > > > if (strchr(cmd, '\n')) > > return error(...); > > > > And if you *really* must, > > > > /* Does the command consist purely of whitespace? */ > > if (!cmd[strspn(cmd, " \t\r\n")]) > > return error(...); > > > > But as I suggested also in a reply to Junio's answer: where would we stop > > to validate the commands? > > I'm not trying to validate the command (and I don't think we can/should) > - just generate a todo list that can be parsed. > > Best Wishes > > Phillip > > > > > Ciao, > > Dscho > > > >> + > >> + return error(_("empty exec command")); > >> +} > >> + > >> int cmd_rebase(int argc, const char **argv, const char *prefix) > >> { > >> struct rebase_options options = { > >> @@ -1130,6 +1148,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" && > >> -- > >> 2.20.1 > >> > >> > >
Hi Phillip, On Tue, 29 Jan 2019, Phillip Wood wrote: > On 28/01/2019 21:56, Johannes Schindelin wrote: > > > > On Mon, 28 Jan 2019, Junio C Hamano wrote: > > > >> Phillip Wood <phillip.wood@talktalk.net> writes: > >> > >>> From: Phillip Wood <phillip.wood@dunelm.org.uk> > >>> > >>> If the user gives an empty argument to --exec then 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'. > >> > >> Hmph. I do agree that the above makes an unfortunate end-user > >> experience, but I would sort-of imagine that it would even be nicer > >> for such an empty exec to behave as if it were "exec false" but with > >> less severe error message, i.e. a way for the user to say "I want to > >> break the sequence here and get an interactive session". We may not > >> even need to add the "break" insn if we go that way and there is one > >> less thing for users to learn. I dunno, but I tend to prefer giving > >> a useful and safe behaviour to interactive users other than erroring > >> out, when there _is_ such a safe behaviour that is obvious from the > >> situation, and I feel that an empty "exec" is such a case. > > > > That would make things unnecessarily confusing. An empty command is not > > `false` with a gentler error message. An empty command is a missing > > command. > > I agree that having a special meaning to the empty command would be > confusing. Also giving it on the command line only helps if you want to > stop after each pick and my impression is that people want to break > after specific commits to amend a fixup or something like that. > > > I am, however, concerned that special-casing an empty command will not > > make things better: if the user called `git rebase --exec=fasle`, they > > will *still* have to clean up their edit script. > > The empty commands create an invalid todo list which git cannot parse, > this patch is not a step down the path of checking that the command > exists or is valid shell - I don't think that would be possible in the > general case. Well, at least the error message is helpful: it suggests --edit-todo. But as I said in another reply, I understand now that your patch validates the argument as necessary to produce a valid todo list. Thanks, Dscho > > Best Wishes > > Phillip > > > Or just `git rebase --abort`, which I would do whether I had forgotten to > > specify a command or whether I had a typo in my command. > > > >>> 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' > >> > >> It is very good to check the input, regardless of what an empty > >> "exec" should do. > > > > Should we then also check for incorrect quoting, missing commands, other > > errors? I am not sure that this path leads to sanity. > > > > Ciao, > > Dscho > > > >
diff --git a/builtin/rebase.c b/builtin/rebase.c index 00de70365e..b6c54b03c1 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -793,6 +793,24 @@ static void set_reflog_action(struct rebase_options *options) strbuf_release(&buf); } +static int check_exec_cmd(const char *cmd) +{ + int non_blank = 0; + + while (*cmd) { + if (*cmd == '\n') + return error(_("exec commands cannot contain newlines")); + if (!isspace(*cmd)) + non_blank = 1; + cmd++; + } + + if (non_blank) + return 0; + + return error(_("empty exec command")); +} + int cmd_rebase(int argc, const char **argv, const char *prefix) { struct rebase_options options = { @@ -1130,6 +1148,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" &&