Re: [RFC PATCH 0/9] rebase -i: extend rebase.missingCommitsCheck to `--edit-todo' and co.

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Alban

On 25/07/2019 21:26, Alban Gruin wrote:
> Hi Phillip,
> 
> Le 24/07/2019 à 15:29, Phillip Wood a écrit :
>> Hi Alban
>>
>> Thanks for working on this, it's great to see you back on the list and I
>> think it would be a useful addition to rebase. Unfortunately I'm not
>> sure about this implementation though (although the early bug fix
>> patches are useful in their own right)
>>
>> On 17/07/2019 15:39, Alban Gruin wrote:
>>> To prevent mistakes when editing a branch, rebase features a knob,
>>> rebase.missingCommitsCheck, to warn the user if a commit was dropped.
>>> Unfortunately, this check is only effective for the initial edit, which
>>> means that if you edit the todo list at a later point of the rebase and
>>> dropped a commit, no warnings or errors would be issued.
>>>
>>> This adds the ability to check if commits were dropped when resuming a
>>> rebase (with `rebase --continue'), when editing the todo list (with
>>> `rebase --edit-todo'), or when reloading the todo list after an `exec'
>>> command.
>>
>> I'm not sure if we really need to check the todo list when continuing or
>> after an exec command.
> 
> In which case I don’t really understand why there is an `error' mode if
> one can completely bypass it with `--continue'.

That's an interesting point about `--continue`. Perhaps if `--edit-todo` 
detects deleted lines in error mode it should write a file to stop 
`--continue` continuing rather than having to validate the entire list 
each time we continue a rebase. Alternatively we could annotate the todo 
list with a message the dropped commits commented out and reopen the 
editor for the user to fix the problem, but that would cause scripted 
editors to enter a infinite loop as they're unlikely to fix the problem 
the second time round. A third possibility is to keep your code 
validating the list each time we run continue, but update the backup 
file with each edit so it detects added commits that are deleted in a 
later edit. This would also provide some protection for users who edit 
git-rebase-todo directly, though if they are using a script that deletes 
lines in git-rebase-todo directly it will suddenly stop working with 
this change if they have rebase.missingCommitsCheck set to error.

Having said all that we could decide that the existing error message is 
enough and allow the user to skip re-editing the list if they really did 
mean to remove those lines. It would be annoying to have to re-edit the 
list when one had intended to delete those lines.

>> The official way to edit the todo list is to run
>> 'git rebase --edit-todo' and I'm not sure if we support scripts writing
>> to .git/rebase-merge/git-rebase-todo directly. If we only support the
>> check after --edit-todo then I think the implementation can be
>> simplified as we can just write a copy of the file before it is edited
>> and don't need to check .git/rebase-merge/done. Additionally that would
>> catch commits that are added by the user and then deleted in a later
>> edit. They wont be in the original list so I don't think this series
>> will detect their deletion.
>>
> 
> True -- but with this solution, if a bad command is introduced, there
> will be false negatives.  Given the pitfall of my solution, this should
> be an acceptable trade-off.

We could detect a bad commit by checking the oid and not complaining if 
it is not valid. That's slightly complicated by labels, but we could 
fairly easily keep of list of the labels defined so far as we scan the 
list. That would also open the possibility of detecting errors where the 
user references an undefined label in `merge` or `reset` commands but 
that's a separate problem.

Best Wishes

Phillip

>> At the extreme I have a script around rebase that runs 'rebase -i HEAD'
>> and then fills in the todo list with a fake editor that adds 'reset ...'
>> as the first line to set the starting point of the rebase. I think
>> dscho's garden-shears script does something similar. Under the proposed
>> scheme if I subsequently edit the todo list it will not catch any
>> deleted commits as the original list is empty.
>>
>> Best Wishes
>>
>> Phillip
>>
> 
> Cheers,
> Alban
>