| Message ID | 20190717143918.7406-1-alban.gruin@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | rebase -i: extend rebase.missingCommitsCheck to `--edit-todo' and co. | expand |
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(-) >
Hi, On Wed, 24 Jul 2019, Phillip Wood wrote: > 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. Indeed, and in the meantime there is also an ever-green.sh script that not only uses that trick to custom-populate the todo list, but also _extends_ it via an `exec` command at the end of the first rebase run, essentially allowing for a nested rebase. Ciao, Dscho
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'. > 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. > 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
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 >
Hi, This has been a long time since I have not given an update on this -- sorry about that. I think it’s time to do it. Le 29/07/2019 à 11:38, Phillip Wood a écrit : > Hi Alban > > -%<- > > 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. > If we do not check the todo list after `exec' commands, patches 3 to 6 should be useless in this series and could be sent separately (I’m still interested in reducing useless round trips to the disk). I found a cleaner way to do that, without touching to sequencer_continue(). For the main feature of this series, I need to write tests for it, and then I’ll send it as a WIP series, once again. Cheers, Alban
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. 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(-)