git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
PWPhillip Wood <phillip.wood123@gmail.com>
Date
Jul 29, 2019, 09:38 UTC
Message-ID
<8214207e-fa35-44c4-4135-45bd90209e18@gmail.com>
In-Reply-To
<69ceed35-ced4-51e7-d724-1818ae45e9a6@gmail.com>
Hi Alban
On 25/07/2019 21:26, Alban Gruin wrote:
Show 27 quoted lines
> 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.

Show 14 quoted 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
Show 15 quoted lines
>> 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
>
Previous: Alban GruinNext: Alban Gruin
Message 23 of 57 in “rebase -i: extend rebase.missingCommitsCheck to `--edit-todo' and co.”
  1. 0/9 rebase -i: extend rebase.missingCommitsCheck to `--edit-todo' and co.Alban Gruin, Jul 17, 2019
  2. 1/9 t3404: demonstrate that --edit-todo does not check for dropped commitsAlban Gruin, Jul 17, 2019
  3. Junio C HamanoJul 18, 2019
  4. Alban GruinJul 19, 2019
  5. Junio C HamanoJul 19, 2019
  6. 2/9 t3429: demonstrate that rebase exec does not check for dropped commitsAlban Gruin, Jul 17, 2019
  7. 3/9 sequencer: update `total_nr' when adding an item to a todo listAlban Gruin, Jul 17, 2019
  8. Junio C HamanoJul 18, 2019
  9. Alban GruinJul 19, 2019
  10. 4/9 sequencer: update `done_nr' when skipping commands in a todo listAlban Gruin, Jul 17, 2019
  11. Junio C HamanoJul 18, 2019
  12. Alban GruinJul 19, 2019
  13. 5/9 sequencer: move the code writing total_nr on the disk to a new functionAlban Gruin, Jul 17, 2019
  14. Junio C HamanoJul 18, 2019
  15. Alban GruinJul 19, 2019
  16. 6/9 sequencer: add a parameter to sequencer_continue() to accept a todo listAlban Gruin, Jul 17, 2019
  17. 7/9 rebase-interactive: todo_list_check() also uses the done listAlban Gruin, Jul 17, 2019
  18. 8/9 rebase-interactive: warn if commit is dropped with --edit-todoAlban Gruin, Jul 17, 2019
  19. 9/9 sequencer: have read_populate_todo() check for dropped commitsAlban Gruin, Jul 17, 2019
  20. Phillip WoodJul 24, 2019
  21. Johannes SchindelinJul 25, 2019
  22. Alban GruinJul 25, 2019
  23. Phillip WoodJul 29, 2019
  24. Alban GruinSep 24, 2019
  25. 0/2 rebase -i: extend rebase.missingCommitsCheck to `--edit-todo'Alban Gruin, Nov 4, 2019
  26. 1/2 t3404: demonstrate that --edit-todo does not check for dropped commitsAlban Gruin, Nov 4, 2019
  27. 2/2 rebase-interactive: warn if commit is dropped with --edit-todoAlban Gruin, Nov 4, 2019
  28. Phillip WoodNov 5, 2019
  29. 0/2 rebase -i: extend rebase.missingCommitsCheckAlban Gruin, Dec 2, 2019
  30. 1/2 sequencer: move check_todo_list_from_file() to rebase-interactive.cAlban Gruin, Dec 2, 2019
  31. Johannes SchindelinDec 6, 2019
  32. 2/2 rebase-interactive: warn if commit is dropped with `rebase --edit-todo'Alban Gruin, Dec 2, 2019
  33. Junio C HamanoDec 4, 2019
  34. Phillip WoodDec 9, 2019
  35. Alban GruinJan 9, 2020
  36. Phillip WoodJan 10, 2020
  37. Alban GruinJan 10, 2020
  38. Phillip WoodJan 11, 2020
  39. Phillip WoodDec 9, 2019
  40. Junio C HamanoDec 4, 2019
  41. Alban GruinDec 5, 2019
  42. Phillip WoodDec 6, 2019
  43. Johannes SchindelinDec 6, 2019
  44. 0/2 rebase -i: extend rebase.missingCommitsCheckAlban Gruin, Jan 11, 2020
  45. 1/2 sequencer: move check_todo_list_from_file() to rebase-interactive.cAlban Gruin, Jan 11, 2020
  46. 2/2 rebase-interactive: warn if commit is dropped with `rebase --edit-todo'Alban Gruin, Jan 11, 2020
  47. Phillip WoodJan 19, 2020
  48. Alban GruinJan 25, 2020
  49. 0/2 rebase -i: extend rebase.missingCommitsCheckAlban Gruin, Jan 25, 2020
  50. 1/2 sequencer: move check_todo_list_from_file() to rebase-interactive.cAlban Gruin, Jan 25, 2020
  51. 2/2 rebase-interactive: warn if commit is dropped with `rebase --edit-todo'Alban Gruin, Jan 25, 2020
  52. Johannes SchindelinJan 26, 2020
  53. Alban GruinJan 27, 2020
  54. Johannes SchindelinJan 28, 2020
  55. 0/2 rebase -i: extend rebase.missingCommitsCheckAlban Gruin, Jan 28, 2020
  56. 1/2 sequencer: move check_todo_list_from_file() to rebase-interactive.cAlban Gruin, Jan 28, 2020
  57. 2/2 rebase-interactive: warn if commit is dropped with `rebase --edit-todo'Alban Gruin, Jan 28, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.