[PATCH v3 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 v3 0/2] rebase -i: extend rebase.missingCommitsCheck
Date: Tue,  3 Dec 2019 00:47:57 +0100	[thread overview]
Message-ID: <20191202234759.26201-1-alban.gruin@gmail.com> (raw)
In-Reply-To: <20191104095449.9964-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-v3" at
https://github.com/agrn/git.

Changes since v2:
 - Merged patches 1 and 2.
 - Reinstated support for `git rebase --continue'.
 - Print `edit_todo_list_advice' if a dropped commit was detected by
   `--edit-todo' or `--continue'.

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          | 57 ++++++++++++++++++++----
 rebase-interactive.h          |  2 +
 sequencer.c                   | 53 ++++++----------------
 sequencer.h                   |  1 -
 t/t3404-rebase-interactive.sh | 83 +++++++++++++++++++++++++++++++++++
 5 files changed, 147 insertions(+), 49 deletions(-)

Diff-intervalle contre v2:
-:  ---------- > 1:  996045a300 sequencer: move check_todo_list_from_file() to rebase-interactive.c
1:  6974b6c8f2 ! 2:  ba6d27e5b4 t3404: demonstrate that --edit-todo does not check for dropped commits
    @@ Metadata
     Author: Alban Gruin <alban.gruin@gmail.com>

      ## Commit message ##
    -    t3404: demonstrate that --edit-todo does not check for dropped commits
    +    rebase-interactive: warn if commit is dropped with `rebase --edit-todo'

    -    When set to "warn" or "error", `rebase.missingCommitCheck' would make
    -    rebase -i warn if the user removed commits from the todo list to prevent
    -    mistakes.  Unfortunately, rebase --edit-todo don't take it into account.
    +    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 three tests to t3404 to demonstrate this.  The first one is
    -    not broken, as when `rebase.missingCommitsCheck' is not set, nothing in
    -    particular must be done towards dropped commits.  The two others are
    -    broken, demonstrating the problem.
    +    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.

    -    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.
    +    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 ##
    +@@ rebase-interactive.c: 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();
    + 	unsigned initial = shortrevisions && shortonto;
    + 
    + 	/* If the user is editing the todo list, we first try to parse
    +@@ rebase-interactive.c: 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);
    + 
    + 	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 *todo_list,
    + 	if (initial && new_todo->buf.len == 0)
    + 		return -3;
    + 
    +-	/* For the initial edit, the todo list gets parsed in
    +-	 * complete_action(). */
    +-	if (!initial)
    +-		return todo_list_parse_insn_buffer(r, new_todo->buf.buf, new_todo);
    ++	if (todo_list_parse_insn_buffer(r, new_todo->buf.buf, new_todo)) {
    ++		fprintf(stderr, _(edit_todo_list_advice));
    ++		return -4;
    ++	}
    ++
    ++	if (todo_list_check(todo_list, new_todo))
    ++		return -4;
    + 
    + 	return 0;
    + }
    +@@ rebase-interactive.c: int todo_list_check(struct todo_list *old_todo, struct todo_list *new_todo)
    + 		"the level of warnings.\n"
    + 		"The possible behaviours are: ignore, warn, error.\n\n"));
    + 
    ++	fprintf(stderr, _(edit_todo_list_advice));
    ++
    + leave_check:
    + 	clear_commit_seen(&commit_seen);
    + 	return res;
    +
    + ## sequencer.c ##
    +@@ sequencer.c: int sequencer_continue(struct repository *r, struct replay_opts *opts)
    + 	if (read_populate_opts(opts))
    + 		return -1;
    + 	if (is_rebase_i(opts)) {
    ++		struct todo_list backup = TODO_LIST_INIT;
    ++
    + 		if ((res = read_populate_todo(r, &todo_list, opts)))
    + 			goto release_todo_list;
    ++
    ++		if (strbuf_read_file(&backup.buf, rebase_path_todo_backup(), 0) > 0) {
    ++			todo_list_parse_insn_buffer(r, backup.buf.buf, &backup);
    ++			res = todo_list_check(&backup, &todo_list);
    ++			todo_list_release(&backup);
    ++
    ++			if (res)
    ++				goto release_todo_list;
    ++		}
    ++
    + 		if (commit_staged_changes(r, opts, &todo_list))
    + 			return -1;
    + 	} else if (!file_exists(get_todo_path(opts)))
    +@@ sequencer.c: int todo_list_write_to_file(struct repository *r, struct todo_list *todo_list,
    + 	return res;
    + }
    + 
    +-static const char edit_todo_list_advice[] =
    +-N_("You can fix this with 'git rebase --edit-todo' "
    +-"and then run 'git rebase --continue'.\n"
    +-"Or you can abort the rebase with 'git rebase"
    +-" --abort'.\n");
    +-
    + /* skip picking commits whose parents are unchanged */
    + static int skip_unnecessary_picks(struct repository *r,
    + 				  struct todo_list *todo_list,
    +@@ sequencer.c: int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
    + 		todo_list_release(&new_todo);
    + 
    + 		return error(_("nothing to do"));
    +-	}
    +-
    +-	if (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo) ||
    +-	    todo_list_check(todo_list, &new_todo)) {
    +-		fprintf(stderr, _(edit_todo_list_advice));
    ++	} else if (res == -4) {
    + 		checkout_onto(r, opts, onto_name, &onto->object.oid, orig_head);
    + 		todo_list_release(&new_todo);
    + 
    +
      ## t/t3404-rebase-interactive.sh ##
     @@ t/t3404-rebase-interactive.sh: test_expect_success 'rebase -i respects rebase.missingCommitsCheck = error' '
      	test B = $(git cat-file commit HEAD^ | sed -ne \$p)
    @@ t/t3404-rebase-interactive.sh: test_expect_success 'rebase -i respects rebase.mi
     +		actual
     +'
     +
    -+test_expect_failure 'rebase --edit-todo respects rebase.missingCommitsCheck = warn' '
    ++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.
    @@ t/t3404-rebase-interactive.sh: test_expect_success 'rebase -i respects rebase.mi
     +		actual
     +'
     +
    -+test_expect_failure 'rebase --edit-todo respects rebase.missingCommitsCheck = error' '
    ++test_expect_success 'rebase --edit-todo respects rebase.missingCommitsCheck = error' '
     +	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.
    ++
    ++	Use '\''git config rebase.missingCommitsCheck'\'' to change the level of warnings.
    ++	The possible behaviours are: ignore, warn, error.
    ++
    ++	You can fix this with '\''git rebase --edit-todo'\'' and then run '\''git rebase --continue'\''.
    ++	Or you can abort the rebase with '\''git rebase --abort'\''.
     +	EOF
    -+	tail -n4 expect >expect.2 &&
    ++	tail -n10 expect >expect.2 &&
     +	test_config rebase.missingCommitsCheck error &&
     +	rebase_setup_and_clean missing-commit &&
     +	set_fake_editor &&
    @@ t/t3404-rebase-interactive.sh: test_expect_success 'rebase -i respects rebase.mi
     +		git rebase -i --root &&
     +	cp .git/rebase-merge/git-rebase-todo.backup orig &&
     +	test_must_fail env FAKE_LINES="2 3 4" \
    -+		git rebase --edit-todo 2>actual.2 &&
    -+	head -n5 actual.2 >actual &&
    ++		git rebase --edit-todo 2>actual &&
    ++	test_i18ncmp expect actual &&
    ++	test_must_fail git rebase --continue 2>actual &&
     +	test_i18ncmp expect actual &&
     +	cp orig .git/rebase-merge/git-rebase-todo &&
     +	test_must_fail env FAKE_LINES="1 2 3 4" \
    -+		git rebase --edit-todo 2>actual.2 &&
    -+	head -n4 actual.2 >actual &&
    ++		git rebase --edit-todo 2>actual &&
    ++	test_i18ncmp expect.2 actual &&
    ++	test_must_fail git rebase --continue 2>actual &&
     +	test_i18ncmp expect.2 actual &&
     +	cp orig .git/rebase-merge/git-rebase-todo &&
     +	FAKE_LINES="1 2 3 4 drop 5" git rebase --edit-todo &&
2:  a4a700ce8b < -:  ---------- rebase-interactive: warn if commit is dropped with --edit-todo
-- 
2.24.0