Re: [PATCH v5 0/2] rebase -i: extend rebase.missingCommitsCheck

From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Alban Gruin <alban.gruin@gmail.com>
Cc: git@vger.kernel.org, Phillip Wood <phillip.wood@dunelm.org.uk>,
	Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH v5 0/2] rebase -i: extend rebase.missingCommitsCheck
Date: Sun, 26 Jan 2020 11:04:11 +0100 (CET)	[thread overview]
Message-ID: <nycvar.QRO.7.76.6.2001261043540.46@tvgsbejvaqbjf.bet> (raw)
In-Reply-To: <20200125175436.29786-1-alban.gruin@gmail.com>

Hi Alban,

On Sat, 25 Jan 2020, Alban Gruin wrote:

> Changes since v4:
>
>  - Added a test to check that no errors are raised when editing the todo
>    list after pausing the rebase; it was provided by Phillip Wood.
>
>  - Fixed the issue pointed out by this test by creating a backup of the
>    todo list even if the edited list is incorrect.  This step was moved
>    before the user edits the list, so the backup can be created with
>    copy_file() instead of todo_list_write_to_file().

Please note that I changed this to always use `todo_list_write_to_file()`
in `js/rebase-i-with-colliding-hash` (see e.g.
https://github.com/gitgitgadget/git/commit/26027625).

The reason is that the copy would be made with _abbreviated_ commit IDs.
And during the rebase, those abbreviated IDs could become non-unique.

I have to admit that I did not follow the evolution of your patch series
terribly closely, but from your description I gather that we want to be
careful to ensure that the `.backup` file is written with non-abbreviated
commit IDs always.

The way I read your patches, they will conflict with
`js/rebase-i-with-colliding-hash`, so maybe it would be a good idea to
base your patches on that branch?

Thanks,
Dscho

>
>  - In edit_todo_list(), `incorrect' is set to 0 by default.  This only
>    concerns the initial edit case; instead of opening and parsing the
>    backup, we use directly `old_todo' as a reference.
>
>  - Don't check if the "dropped" file flag exists at the initial edit.
>
> Alban Gruin (2):
>   sequencer: move check_todo_list_from_file() to rebase-interactive.c
>   rebase-interactive: warn if commit is dropped with `rebase
>     --edit-todo'
>
>  rebase-interactive.c          |  90 ++++++++++++++++++++++---
>  rebase-interactive.h          |   5 ++
>  sequencer.c                   |  51 ++++----------
>  sequencer.h                   |   2 +-
>  t/t3404-rebase-interactive.sh | 121 ++++++++++++++++++++++++++++++++++
>  5 files changed, 219 insertions(+), 50 deletions(-)
>
> Range-diff against v4:
> 1:  996045a300 = 1:  996045a300 sequencer: move check_todo_list_from_file() to rebase-interactive.c
> 2:  11b0e1e78c ! 2:  6dbaa8cbe6 rebase-interactive: warn if commit is dropped with `rebase --edit-todo'
>     @@ Commit message
>          `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 once, before the initial edit.  Now, it is also made if
>     -    after the user edited the list, if it has no errors or if no commits
>     -    were dropped and `rebase.missingCommitsCheck' is set.  Thus, the
>     +    This changes when a backup of the todo list is made.  Until now, it was
>     +    saved only once, before the initial edit.  Now, it is also made if the
>     +    original todo list has no errors or no dropped commits.  Thus, the
>          backup should be error-free.  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
>     @@ Commit message
>          no errors were found at the last edition, so any missing commits here
>          have already been picked.
>
>     -    Four tests are added to t3404.  The tests for
>     +    Five 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,
>     @@ Commit message
>          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.  The fourth test checks that no errors
>     -    are raised when resuming a rebase after resolving a conflict.
>     +    are raised when resuming a rebase after resolving a conflict, the fifth
>     +    checks that no errors are raised when editing the todo list after
>     +    pausing the rebase.
>
>          Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
>          I don't think the way I create `expect.3' files in "rebase --edit-todo
>     @@ rebase-interactive.c: int edit_todo_list(struct repository *r, struct todo_list
>      +	const char *todo_file = rebase_path_todo(),
>      +		*todo_backup = rebase_path_todo_backup();
>       	unsigned initial = shortrevisions && shortonto;
>     -+	int incorrect = 1;
>     ++	int incorrect = 0;
>
>       	/* If the user is editing the todo list, we first try to parse
>       	 * it.  If there is an error, we do not return, because the user
>       	 * might want to fix it in the first place. */
>       	if (!initial)
>      -		todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list);
>     -+		incorrect = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list);
>     -+
>     -+	incorrect |= file_exists(rebase_path_dropped());
>     ++		incorrect = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list) |
>     ++			file_exists(rebase_path_dropped());
>
>       	if (todo_list_write_to_file(r, todo_list, todo_file, shortrevisions, shortonto,
>       				    -1, flags | TODO_LIST_SHORTEN_IDS | TODO_LIST_APPEND_TODO_HELP))
>     @@ rebase-interactive.c: int edit_todo_list(struct repository *r, struct todo_list
>      -	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());
>     -+	if (initial && copy_file(todo_backup, todo_file, 0666))
>     -+		return error(_("could not copy '%s' to '%s'."), todo_file, todo_backup);
>     ++	if (initial || !incorrect) {
>     ++		if (!initial)
>     ++			unlink(todo_backup);
>     ++
>     ++		if (copy_file(todo_backup, todo_file, 0666))
>     ++		    return error(_("could not copy '%s' to '%s'."), todo_file, todo_backup);
>     ++	}
>
>       	if (launch_sequence_editor(todo_file, &new_todo->buf, NULL))
>       		return -2;
>     @@ rebase-interactive.c: int edit_todo_list(struct repository *r, struct todo_list
>      +	} else if (todo_list_check(todo_list, new_todo)) {
>      +		write_file(rebase_path_dropped(), "");
>      +		return -4;
>     -+	} else {
>     -+		todo_list_write_to_file(r, todo_list, todo_backup, shortrevisions, shortonto,
>     -+					-1, flags | TODO_LIST_SHORTEN_IDS | TODO_LIST_APPEND_TODO_HELP);
>      +	}
>
>       	return 0;
>     @@ t/t3404-rebase-interactive.sh: test_expect_success 'rebase -i respects rebase.mi
>      +	git add file1 &&
>      +	git rebase --continue
>      +'
>     ++
>     ++test_expect_success 'rebase.missingCommitsCheck = error when editing for a second time' '
>     ++	test_config rebase.missingCommitsCheck error &&
>     ++	(
>     ++		set_fake_editor &&
>     ++		FAKE_LINES="1 break 2 3" git rebase -i A D &&
>     ++		cp .git/rebase-merge/git-rebase-todo todo &&
>     ++		test_must_fail env FAKE_LINES=2 git rebase --edit-todo &&
>     ++		GIT_SEQUENCE_EDITOR="cp todo" git rebase --edit-todo &&
>     ++		git rebase --continue
>     ++	)
>     ++'
>      +
>       test_expect_success 'respects rebase.abbreviateCommands with fixup, squash and exec' '
>       	rebase_setup_and_clean abbrevcmd &&
> --
> 2.24.1
>
>