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

From: Phillip Wood <phillip.wood123@gmail.com>
To: Junio C Hamano <gitster@pobox.com>, Alban Gruin <alban.gruin@gmail.com>
Cc: git@vger.kernel.org,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	Phillip Wood <phillip.wood@dunelm.org.uk>
Subject: Re: [PATCH v3 2/2] rebase-interactive: warn if commit is dropped with `rebase --edit-todo'
Date: Mon, 9 Dec 2019 16:00:14 +0000	[thread overview]
Message-ID: <64aa4049-ee35-df4c-1e6c-80707f4f9070@gmail.com> (raw)
In-Reply-To: <xmqqk17b5263.fsf@gitster-ct.c.googlers.com>

Hi Alban and Junio

On 04/12/2019 19:19, Junio C Hamano wrote:
> 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.

I think it would be better to only create a new copy if the last edit 
was successful. As it stands if I edit the todo list and accidentally 
delete some lines and then edit the todo list again to try and fix it 
the second edit will succeed whether or not I reinserted the deleted lines.

We could add this to the tests to check that a subsequent edit that does 
not fix the problem fails

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 969e12d281..8544d8ab2c 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh

@@ -1416,6 +1416,7 @@ test_expect_success 'rebase --edit-todo respects 
rebase.missingCommitsCheck = er
                 test_i18ncmp expect actual &&
                 test_must_fail git rebase --continue 2>actual &&
                 test_i18ncmp expect actual &&
+               test_must_fail git rebase --edit-todo &&
                 cp orig .git/rebase-merge/git-rebase-todo &&
                 test_must_fail env FAKE_LINES="1 2 3 4" \
                         git rebase --edit-todo 2>actual &&


> 
> We unlink(2) unconditionally because the only effect we want to see
> here is that todo_backup does not exist before we call copy_file()
> that wants to do O_CREAT|O_EXCL.  I wonder if we want to avoid
> unlink() when initial, and also if we want to do unlink_or_warn()
> when !initial (read: this is just "wondering" without thinking long
> enough to suggest that doing so would be better)
> 
>> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
>> index 29a35840ed..9051c1e11d 100755
>> --- a/t/t3404-rebase-interactive.sh
>> +++ b/t/t3404-rebase-interactive.sh
>> @@ -1343,6 +1343,89 @@ test_expect_success 'rebase -i respects rebase.missingCommitsCheck = error' '
>>   	test B = $(git cat-file commit HEAD^ | sed -ne \$p)
>>   '
>>   
>> +test_expect_success 'rebase --edit-todo respects rebase.missingCommitsCheck = ignore' '
>> +	test_config rebase.missingCommitsCheck ignore &&
>> +	rebase_setup_and_clean missing-commit &&
>> +	set_fake_editor &&
>> +	FAKE_LINES="break 1 2 3 4 5" git rebase -i --root &&
>> +	FAKE_LINES="1 2 3 4" git rebase --edit-todo 2>actual &&
> 
> OK, so we lost "pick 5" but with missing-check disabled, that should
> not trigger any annoying warning or error.
> 
>> +	git rebase --continue 2>actual &&

This clobbers actual which hasn't been used yet

>> +	test D = $(git cat-file commit HEAD | sed -ne \$p) &&
> 
>> +	test_i18ngrep \
>> +		"Successfully rebased and updated refs/heads/missing-commit" \
>> +		actual
>> +'
>> +
>> +test_expect_success 'rebase --edit-todo respects rebase.missingCommitsCheck = warn' '
>> +	cat >expect <<-EOF &&
>> +	error: invalid line 1: badcmd $(git rev-list --pretty=oneline --abbrev-commit -1 master~4)
>> +	Warning: some commits may have been dropped accidentally.
>> +	Dropped commits (newer to older):
>> +	 - $(git rev-list --pretty=oneline --abbrev-commit -1 master)
>> +	To avoid this message, use "drop" to explicitly remove a commit.
>> +	EOF
>> +	tail -n4 expect >expect.2 &&
>> +	test_config rebase.missingCommitsCheck warn &&
>> +	rebase_setup_and_clean missing-commit &&
>> +	set_fake_editor &&
>> +	test_must_fail env FAKE_LINES="bad 1 2 3 4 5" \
>> +		git rebase -i --root &&
>> +	cp .git/rebase-merge/git-rebase-todo.backup orig &&
>> +	FAKE_LINES="2 3 4" git rebase --edit-todo 2>actual.2 &&
>> +	head -n5 actual.2 >actual &&
>> +	test_i18ncmp expect actual &&
> 
> OK, so we lost "pick 1" while discarding "bad", and we should notice
> the lossage?  I see "head -n5" there, which means we are still
> getting "invalid line 1: badcmd", even though FAKE_LINES now got rid
> of "bad"?  Puzzled...

Is the bad there to stop the rebase so we can edit the todo list? If so 
it would be better to use 'break' instead.

Best Wishes

Phillip
