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
