Re: [PATCH] rebase -x: sanity check command

[Phillip Wood <phillip.wood@talktalk.net>]

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
>