Re: [PATCH v3 2/2] rebase-interactive: warn if commit is dropped with `rebase --edit-todo'

> Alban Gruin <alban.gruin@gmail.com> writes:
> 
>> When set to "warn" or "error", `rebase.missingCommitsCheck' would make
>> `rebase -i' warn if the user removed commits from the todo list to
>> prevent mistakes.  Unfortunately, `rebase --edit-todo' and `rebase
>> --continue' don't take it into account.
>>
>> This adds the ability for `rebase --edit-todo' and `rebase --continue'
>> to check if commits were dropped by the user.  As both edit_todo_list()
>> and complete_action() parse the todo list and check for dropped commits,
>> the code doing so in the latter is removed to reduce duplication.
>> `edit_todo_list_advice' is removed from sequencer.c as it is no longer
>> used there.
>>
>> This changes when a backup of the todo list is made.  Until now, it was
>> saved only before the initial edit.  Now, it is always performed before
>> the todo list is edited.  Without this, sequencer_continue() (`rebase
>> --continue') could only compare the current todo list against the
>> original, unedited list.  Before this change, this file was only used by
>> edit_todo_list() and `rebase -p' to create the backup before the initial
>> edit, and check_todo_list_from_file(), only used by `rebase -p' to check
>> for dropped commits after its own initial edit.
>>
>> Three tests are added to t3404.  The tests for
>> `rebase.missingCommitsCheck = warn' and `rebase.missingCommitsCheck =
>> error' have a similar structure.  First, we start a rebase with an
>> incorrect command on the first line.  Then, we edit the todo list,
>> removing the first and the last lines.  This demonstrates that
>> `--edit-todo' notices dropped commits, but not when the command is
>> incorrect.  Then, we restore the original todo list, and edit it to
>> remove the last line.  This demonstrates that if we add a commit after
>> the initial edit, then remove it, `--edit-todo' will notice that it has
>> been dropped.  Then, the actual rebase takes place.  In the third test,
>> it is also checked that `--continue' will refuse to resume the rebase if
>> commits were dropped.
>>
>> Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
>> ---
>>   rebase-interactive.c          | 22 ++++++----
>>   sequencer.c                   | 24 +++++-----
>>   t/t3404-rebase-interactive.sh | 83 +++++++++++++++++++++++++++++++++++
>>   3 files changed, 110 insertions(+), 19 deletions(-)
> 
> Let me see if I understand the primary idea behind this change by
> trying to paraphrase the log (read: this is not to suggest a better
> phrasing of the log message proposed in your message).
> 
>   * rebase-interactive.c::edit_todo_list() does not perform "did the
>     user delete a pick, instead of turning pick into drop?" but after
>     the end-user edits the file is the most logical place to do so.
>     Let's do that there.
> 
>   * The sequencer used to perform "did the user delete a pick,
>     instead of turning pick into drop?" check in complete_action().
>     We drop that call but for this particular codepath it does not
>     make any behaviour difference due to the next item.
> 
>   * New code does the check in sequencer_continue(), which is called
>     at the end of complete_action(), as well as many other places,
>     like builtin/rebase.c, builtin/revert.c, and sequencer_skip().
>     Because the check is only done when we are running "rebase-i",
>     this is safe---it only affects complete_action().
> 
> I hope I got it more-or-less correctly ;-)
> 
>> diff --git a/rebase-interactive.c b/rebase-interactive.c
>> index ad5dd49c31..80b6a2e7a6 100644
>> --- a/rebase-interactive.c
>> +++ b/rebase-interactive.c
>> @@ -97,7 +97,8 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list,
>>   		   struct todo_list *new_todo, const char *shortrevisions,
>>   		   const char *shortonto, unsigned flags)
>>   {
>> -	const char *todo_file = rebase_path_todo();
>> +	const char *todo_file = rebase_path_todo(),
>> +		*todo_backup = rebase_path_todo_backup();
>>   	/* If the user is editing the todo list, we first try to parse
>> @@ -110,9 +111,9 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list,
>>   				    -1, flags | TODO_LIST_SHORTEN_IDS | TODO_LIST_APPEND_TODO_HELP))
>>   		return error_errno(_("could not write '%s'"), todo_file);
>>   
>> -	if (initial && copy_file(rebase_path_todo_backup(), todo_file, 0666))
>> -		return error(_("could not copy '%s' to '%s'."), todo_file,
>> -			     rebase_path_todo_backup());
>> +	unlink(todo_backup);
>> +	if (copy_file(todo_backup, todo_file, 0666))
>> +		return error(_("could not copy '%s' to '%s'."), todo_file, todo_backup);
> 
> We used to copy ONLY when initial is set and we left old todo_backup
> intact when !initial.  That is no longer true after this change, but
> it is intended---we create an exact copy of what we would hand out
> to the end-user, so that we can compare it with the edited result
> to figure out what got changed.