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

From: Alban Gruin <alban.gruin@gmail.com>
To: git@vger.kernel.org
Cc: Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	Phillip Wood <phillip.wood@dunelm.org.uk>,
	Junio C Hamano <gitster@pobox.com>,
	Alban Gruin <alban.gruin@gmail.com>
Subject: [PATCH v5 0/2] rebase -i: extend rebase.missingCommitsCheck
Date: Sat, 25 Jan 2020 18:54:34 +0100	[thread overview]
Message-ID: <20200125175436.29786-1-alban.gruin@gmail.com> (raw)
In-Reply-To: <20200111173917.15690-1-alban.gruin@gmail.com>

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
drop a commit, no warnings or errors would be issued.

This adds the ability to check if commits were dropped when editing the
todo list with `--edit-todo', and when resuming a rebase.

The first patch moves moves check_todo_list_from_file() and
`edit_todo_list_advice' from sequencer.c to rebase-interactive.c so the
latter can be used by edit_todo_list() and todo_list_check().  The
second patch adds the check to `--edit-todo' and `--continue' and tests.

This is based on 393adf7a6f ("sequencer: directly call pick_commits()
from complete_action()", 2019-11-24).

The tip of this series is tagged as "edit-todo-drop-v5" at
https://github.com/agrn/git.

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().

 - 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