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. 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.
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
The idea to extend this feature was suggested to me more than a year ago
by Phillip Wood, if I'm not mistaken. I postponed this until four month
ago, when ag/sequencer-reduce-rewriting-todo finally hit master, but I
had to stop because of other obligations. I could go back to work one
month ago, when I did the bulk of this series, but I lacked time to
polish it, so it waited a bit more. Now, I think it is in a good shape
to be sent, although it is still RFC-quality to me. The advertised
functionality should work well, but perhaps there is some flaws I
missed.
The first two patches are new tests, demonstrating that after the
initial edit, the check is not done. The next four are what could be
qualified as omissions from ag/sequencer-reduce-rewriting-todo, but they
are quite important (IMHO) for the rest of the series. The last three
actually extend rebase.missingCommitsCheck.
This is based on master (9d418600f4, "The fifth batch").
The tip of this series is tagged as "edit-todo-drop-rfc" in
https://github.com/agrn/git.
Alban Gruin (9):
t3404: demonstrate that --edit-todo does not check for dropped commits
t3429: demonstrate that rebase exec does not check for dropped commits
sequencer: update `total_nr' when adding an item to a todo list
sequencer: update `done_nr' when skipping commands in a todo list
sequencer: move the code writing total_nr on the disk to a new
function
sequencer: add a parameter to sequencer_continue() to accept a todo
list
rebase-interactive: todo_list_check() also uses the done list
rebase-interactive: warn if commit is dropped with --edit-todo
sequencer: have read_populate_todo() check for dropped commits
builtin/rebase.c | 2 +-
builtin/revert.c | 2 +-
rebase-interactive.c | 67 +++++++++++++++++++++++-----
rebase-interactive.h | 6 ++-
sequencer.c | 53 ++++++++++++++--------
sequencer.h | 3 +-
t/t3404-rebase-interactive.sh | 82 +++++++++++++++++++++++++++++++++++
t/t3429-rebase-edit-todo.sh | 44 ++++++++++++++++++-
8 files changed, 224 insertions(+), 35 deletions(-)
