[RFC PATCH 0/9] rebase -i: extend rebase.missingCommitsCheck to `--edit-todo' and co.

[RFC PATCH 0/9] rebase -i: extend rebase.missingCommitsCheck to `--edit-todo' and co.

[Alban Gruin]

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

This adds the ability to check if commits were dropped when resuming a
rebase (with `rebase --continue'), when editing the todo list (with
`rebase --edit-todo'), or when reloading the todo list after an `exec'
command.

The idea to extend this feature was suggested to me more than a year ago
by Phillip Wood, if I'm not mistaken.  I postponed this until four month
ago, when ag/sequencer-reduce-rewriting-todo finally hit master, but I
had to stop because of other obligations.  I could go back to work one
month ago, when I did the bulk of this series, but I lacked time to
polish it, so it waited a bit more.  Now, I think it is in a good shape
to be sent, although it is still RFC-quality to me.  The advertised
functionality should work well, but perhaps there is some flaws I
missed.

The first two patches are new tests, demonstrating that after the
initial edit, the check is not done.  The next four are what could be
qualified as omissions from ag/sequencer-reduce-rewriting-todo, but they
are quite important (IMHO) for the rest of the series.  The last three
actually extend rebase.missingCommitsCheck.

This is based on master (9d418600f4, "The fifth batch").

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

Alban Gruin (9):
  t3404: demonstrate that --edit-todo does not check for dropped commits
  t3429: demonstrate that rebase exec does not check for dropped commits
  sequencer: update `total_nr' when adding an item to a todo list
  sequencer: update `done_nr' when skipping commands in a todo list
  sequencer: move the code writing total_nr on the disk to a new
    function
  sequencer: add a parameter to sequencer_continue() to accept a todo
    list
  rebase-interactive: todo_list_check() also uses the done list
  rebase-interactive: warn if commit is dropped with --edit-todo
  sequencer: have read_populate_todo() check for dropped commits

 builtin/rebase.c              |  2 +-
 builtin/revert.c              |  2 +-
 rebase-interactive.c          | 67 +++++++++++++++++++++++-----
 rebase-interactive.h          |  6 ++-
 sequencer.c                   | 53 ++++++++++++++--------
 sequencer.h                   |  3 +-
 t/t3404-rebase-interactive.sh | 82 +++++++++++++++++++++++++++++++++++
 t/t3429-rebase-edit-todo.sh   | 44 ++++++++++++++++++-
 8 files changed, 224 insertions(+), 35 deletions(-)

-- 
2.22.0

[RFC PATCH 1/9] t3404: demonstrate that --edit-todo does not check for dropped commits

[Alban Gruin]

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

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 t/t3404-rebase-interactive.sh | 82 +++++++++++++++++++++++++++++++++++
 1 file changed, 82 insertions(+)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 461dd539ff..f5c0a8d2bb 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1345,6 +1345,88 @@ 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 &&
+	test_must_fail env FAKE_LINES="1 2 bad 3 4" \
+		git rebase -i --root >/dev/null 2>stderr &&
+	FAKE_LINES="1 2 4" git rebase --edit-todo &&
+	git rebase --continue 2>actual &&
+	test D = $(git cat-file commit HEAD | sed -ne \$p) &&
+	test_i18ngrep \
+		"Successfully rebased and updated refs/heads/missing-commit" \
+		actual
+'
+
+cat >expect <<EOF
+error: invalid line 5: badcmd $(git rev-list --pretty=oneline --abbrev-commit -1 master)
+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.
+
+EOF
+
+tail -n 8 <expect >expect.2
+
+test_expect_failure 'rebase --edit-todo respects rebase.missingCommitsCheck = warn' '
+	test_config rebase.missingCommitsCheck warn &&
+	rebase_setup_and_clean missing-commit &&
+	set_fake_editor &&
+	test_must_fail env FAKE_LINES="1 2 3 4 bad 5" \
+		git rebase -i --root >/dev/null 2>stderr &&
+	FAKE_LINES="1 2 3 4" git rebase --edit-todo 2>actual &&
+	test_i18ncmp expect actual &&
+	git rebase --continue 2>actual.2 &&
+	head -n 8 <actual.2 >actual &&
+	test_i18ncmp expect.2 actual &&
+	test D = $(git cat-file commit HEAD | sed -ne \$p) &&
+	test_i18ngrep \
+		"Successfully rebased and updated refs/heads/missing-commit" \
+		actual.2
+'
+
+cat >expect <<EOF
+error: invalid line 3: badcmd $(git rev-list --pretty=oneline --abbrev-commit -1 master~2)
+Warning: some commits may have been dropped accidentally.
+Dropped commits (newer to older):
+ - $(git rev-list --pretty=oneline --abbrev-commit -1 master)
+ - $(git rev-list --pretty=oneline --abbrev-commit -1 master~2)
+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.
+
+EOF
+
+tail -n 9 <expect >expect.2
+
+test_expect_failure 'rebase --edit-todo respects rebase.missingCommitsCheck = error' '
+	test_config rebase.missingCommitsCheck error &&
+	rebase_setup_and_clean missing-commit &&
+	set_fake_editor &&
+	test_must_fail env FAKE_LINES="1 2 bad 3 4" \
+		git rebase -i --root >/dev/null 2>stderr &&
+	test_must_fail env FAKE_LINES="1 2 4" \
+		git rebase --edit-todo 2>actual &&
+	test_i18ncmp expect actual &&
+	test_must_fail git rebase --continue 2>actual &&
+	test_i18ncmp expect.2 actual &&
+	cp .git/rebase-merge/git-rebase-todo.backup \
+		.git/rebase-merge/git-rebase-todo &&
+	FAKE_LINES="1 2 drop 3 4 drop 5" \
+		git rebase --edit-todo &&
+	git rebase --continue 2>actual &&
+	test D = $(git cat-file commit HEAD | sed -ne \$p) &&
+	test_i18ngrep \
+		"Successfully rebased and updated refs/heads/missing-commit" \
+		actual
+'
+
 test_expect_success 'respects rebase.abbreviateCommands with fixup, squash and exec' '
 	rebase_setup_and_clean abbrevcmd &&
 	test_commit "first" file1.txt "first line" first &&
-- 
2.22.0

[RFC PATCH 2/9] t3429: demonstrate that rebase exec does not check for dropped commits

[Alban Gruin]

After executing a command, rebase reloads the todo list from the disk,
in case the script has modified it, but does not honour
rebase.missingCommitsCheck.

This adds three tests to t3429 to demonstrate this.  The first one is
not broken, as when `rebase.missingCommitsCheck' is not set, nothing
should be done for dropped commits.  The last two are, demonstrating the
problem.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 t/t3429-rebase-edit-todo.sh | 44 +++++++++++++++++++++++++++++++++++--
 1 file changed, 42 insertions(+), 2 deletions(-)

diff --git a/t/t3429-rebase-edit-todo.sh b/t/t3429-rebase-edit-todo.sh
index 76f6d306ea..2bb9fb65fa 100755
--- a/t/t3429-rebase-edit-todo.sh
+++ b/t/t3429-rebase-edit-todo.sh
@@ -4,15 +4,16 @@ test_description='rebase should reread the todo file if an exec modifies it'
 
 . ./test-lib.sh
 
+todo=.git/rebase-merge/git-rebase-todo
+
 test_expect_success 'rebase exec modifies rebase-todo' '
 	test_commit initial &&
-	todo=.git/rebase-merge/git-rebase-todo &&
 	git rebase HEAD -x "echo exec touch F >>$todo" &&
 	test -e F
 '
 
 test_expect_success SHA1 'loose object cache vs re-reading todo list' '
-	GIT_REBASE_TODO=.git/rebase-merge/git-rebase-todo &&
+	GIT_REBASE_TODO=$todo &&
 	export GIT_REBASE_TODO &&
 	write_script append-todo.sh <<-\EOS &&
 	# For values 5 and 6, this yields SHA-1s with the same first two digits
@@ -33,4 +34,43 @@ test_expect_success SHA1 'loose object cache vs re-reading todo list' '
 	git rebase HEAD -x "./append-todo.sh 5 6"
 '
 
+test_expect_success 'rebase exec respects rebase.missingCommitsCheck = ignore' '
+	test_config rebase.missingCommitsCheck ignore &&
+	git rebase HEAD~2 --keep-empty -x "echo >$todo" &&
+	test 5 = $(git cat-file commit HEAD | sed -ne \$p)
+'
+
+cat >expect <<EOF
+Warning: some commits may have been dropped accidentally.
+Dropped commits (newer to older):
+ - $(git rev-list --pretty=oneline --abbrev-commit -1 HEAD@{2})
+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.
+EOF
+
+test_expect_failure 'rebase exec respects rebase.missingCommitsCheck = warn' '
+	test_config rebase.missingCommitsCheck warn &&
+	git reset --hard HEAD@{2} &&
+	git rebase HEAD~2 --keep-empty -x "echo >$todo" 2>actual.2 &&
+	head -n8 actual.2 | tail -n7 >actual &&
+	test_i18ncmp expect actual &&
+	test 5 = $(git cat-file commit HEAD | sed -ne \$p)
+'
+
+test_expect_failure 'rebase exec respects rebase.missingCommitsCheck = error' '
+	test_config rebase.missingCommitsCheck error &&
+	git reset --hard HEAD@{2} &&
+	test_must_fail git rebase HEAD~2 --keep-empty -x "echo >$todo" 2>actual.2 &&
+	head -n8 actual.2 | tail -n7 >actual &&
+	test_i18ncmp expect actual &&
+	echo drop $(git rev-list --pretty=oneline -1 HEAD@{1}) >$todo &&
+	git rebase --continue 2>actual &&
+	test 5 = $(git cat-file commit HEAD | sed -ne \$p) &&
+	test_i18ngrep \
+		"Successfully rebased and updated refs/heads/master" \
+		actual
+'
+
 test_done
-- 
2.22.0

[RFC PATCH 3/9] sequencer: update `total_nr' when adding an item to a todo list

[Alban Gruin]

`total_nr' is the total amount of items, done and toto, that are in a
todo list.  But unlike `nr', it was not updated when an item was
appended to the list.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 sequencer.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sequencer.c b/sequencer.c
index cf262701e8..e61ae75451 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2070,6 +2070,7 @@ void todo_list_release(struct todo_list *todo_list)
 static struct todo_item *append_new_todo(struct todo_list *todo_list)
 {
 	ALLOC_GROW(todo_list->items, todo_list->nr + 1, todo_list->alloc);
+	++todo_list->total_nr;
 	return todo_list->items + todo_list->nr++;
 }
 
-- 
2.22.0

[RFC PATCH 4/9] sequencer: update `done_nr' when skipping commands in a todo list

[Alban Gruin]

In a todo list, `done_nr' is the amount of commands that were executed
or skipped, but skip_unnecessary_picks() did not update it.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 sequencer.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sequencer.c b/sequencer.c
index e61ae75451..ec9c3d4dc5 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4939,6 +4939,7 @@ static int skip_unnecessary_picks(struct repository *r,
 		MOVE_ARRAY(todo_list->items, todo_list->items + i, todo_list->nr - i);
 		todo_list->nr -= i;
 		todo_list->current = 0;
+		todo_list->done_nr += i;
 
 		if (is_fixup(peek_command(todo_list, 0)))
 			record_in_rewritten(base_oid, peek_command(todo_list, 0));
-- 
2.22.0

[RFC PATCH 5/9] sequencer: move the code writing total_nr on the disk to a new function

[Alban Gruin]

The total amount of commands can be used to show the progression of the
rebasing in a shell.  This number is written to the disk by
read_populate_todo() when the todo list is loaded from
sequencer_continue() or pick_commits(), but not by complete_action(),
which releases its todo list before calling sequencer_continue(), which
reloads it immediatly afterwards.

To avoid to reload the todo list from the disk, sequencer_continue()
will be modified to accept a todo list, and if it is not null,
read_populate_todo() will not be called.

This moves the part writing total_nr to a new function so it can be
called by complete_action().

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 sequencer.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index ec9c3d4dc5..d66b80d49f 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2343,6 +2343,16 @@ void sequencer_post_commit_cleanup(struct repository *r, int verbose)
 	sequencer_remove_state(&opts);
 }
 
+static void todo_list_write_total_nr(struct todo_list *todo_list)
+{
+	FILE *f = fopen_or_warn(rebase_path_msgtotal(), "w");
+
+	if (f) {
+		fprintf(f, "%d\n", todo_list->total_nr);
+		fclose(f);
+	}
+}
+
 static int read_populate_todo(struct repository *r,
 			      struct todo_list *todo_list,
 			      struct replay_opts *opts)
@@ -2388,7 +2398,6 @@ static int read_populate_todo(struct repository *r,
 
 	if (is_rebase_i(opts)) {
 		struct todo_list done = TODO_LIST_INIT;
-		FILE *f = fopen_or_warn(rebase_path_msgtotal(), "w");
 
 		if (strbuf_read_file(&done.buf, rebase_path_done(), 0) > 0 &&
 		    !todo_list_parse_insn_buffer(r, done.buf.buf, &done))
@@ -2400,10 +2409,7 @@ static int read_populate_todo(struct repository *r,
 			+ count_commands(todo_list);
 		todo_list_release(&done);
 
-		if (f) {
-			fprintf(f, "%d\n", todo_list->total_nr);
-			fclose(f);
-		}
+		todo_list_write_total_nr(todo_list);
 	}
 
 	return 0;
-- 
2.22.0

[RFC PATCH 6/9] sequencer: add a parameter to sequencer_continue() to accept a todo list

[Alban Gruin]

As it is called by sequencer_continue() or after an exec command,
read_populate_todo() is a great place to check for dropped commits in
the todo list, but complete_action() (a caller of sequencer_continue())
already does.  Double-checking would show the message twice.

This adds a parameter to sequencer_continue() to accept a todo list.  If
a valid list is provided, read_populate_todo() won't be called.
complete_action() is modified to pass its todo list to
sequencer_continue().

This also avoids reloading the todo list from the disk just after
releasing it.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 builtin/rebase.c |  2 +-
 builtin/revert.c |  2 +-
 sequencer.c      | 26 +++++++++++++++-----------
 sequencer.h      |  3 ++-
 4 files changed, 19 insertions(+), 14 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 89fc4b8153..205916ca11 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -393,7 +393,7 @@ static int run_rebase_interactive(struct rebase_options *opts,
 	case ACTION_CONTINUE: {
 		struct replay_opts replay_opts = get_replay_opts(opts);
 
-		ret = sequencer_continue(the_repository, &replay_opts);
+		ret = sequencer_continue(the_repository, &replay_opts, NULL);
 		break;
 	}
 	case ACTION_EDIT_TODO:
diff --git a/builtin/revert.c b/builtin/revert.c
index 4e71b2f2aa..45a5c6217d 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -207,7 +207,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
 		return ret;
 	}
 	if (cmd == 'c')
-		return sequencer_continue(the_repository, opts);
+		return sequencer_continue(the_repository, opts, NULL);
 	if (cmd == 'a')
 		return sequencer_rollback(the_repository, opts);
 	return sequencer_pick_revisions(the_repository, opts);
diff --git a/sequencer.c b/sequencer.c
index d66b80d49f..3fb15ff8d9 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4147,9 +4147,11 @@ static int commit_staged_changes(struct repository *r,
 	return 0;
 }
 
-int sequencer_continue(struct repository *r, struct replay_opts *opts)
+int sequencer_continue(struct repository *r, struct replay_opts *opts,
+		       struct todo_list *p_todo_list)
 {
-	struct todo_list todo_list = TODO_LIST_INIT;
+	struct todo_list todo_list = TODO_LIST_INIT,
+		*ptr_todo = (p_todo_list) ? p_todo_list : &todo_list;
 	int res;
 
 	if (read_and_refresh_cache(r, opts))
@@ -4158,13 +4160,13 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)
 	if (read_populate_opts(opts))
 		return -1;
 	if (is_rebase_i(opts)) {
-		if ((res = read_populate_todo(r, &todo_list, opts)))
+		if (!p_todo_list && (res = read_populate_todo(r, &todo_list, opts)))
 			goto release_todo_list;
-		if (commit_staged_changes(r, opts, &todo_list))
+		if (commit_staged_changes(r, opts, ptr_todo))
 			return -1;
 	} else if (!file_exists(get_todo_path(opts)))
 		return continue_single_pick(r);
-	else if ((res = read_populate_todo(r, &todo_list, opts)))
+	else if (!p_todo_list && (res = read_populate_todo(r, &todo_list, opts)))
 		goto release_todo_list;
 
 	if (!is_rebase_i(opts)) {
@@ -4179,18 +4181,18 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)
 			res = error_dirty_index(r, opts);
 			goto release_todo_list;
 		}
-		todo_list.current++;
+		ptr_todo->current++;
 	} else if (file_exists(rebase_path_stopped_sha())) {
 		struct strbuf buf = STRBUF_INIT;
 		struct object_id oid;
 
 		if (read_oneliner(&buf, rebase_path_stopped_sha(), 1) &&
 		    !get_oid_committish(buf.buf, &oid))
-			record_in_rewritten(&oid, peek_command(&todo_list, 0));
+			record_in_rewritten(&oid, peek_command(ptr_todo, 0));
 		strbuf_release(&buf);
 	}
 
-	res = pick_commits(r, &todo_list, opts);
+	res = pick_commits(r, ptr_todo, opts);
 release_todo_list:
 	todo_list_release(&todo_list);
 	return res;
@@ -5025,15 +5027,17 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
 		return error_errno(_("could not write '%s'"), todo_file);
 	}
 
-	todo_list_release(&new_todo);
-
 	if (checkout_onto(r, opts, onto_name, &oid, orig_head))
 		return -1;
 
 	if (require_clean_work_tree(r, "rebase", "", 1, 1))
 		return -1;
 
-	return sequencer_continue(r, opts);
+	todo_list_write_total_nr(&new_todo);
+	res = sequencer_continue(r, opts, &new_todo);
+	todo_list_release(&new_todo);
+
+	return res;
 }
 
 struct subject2item_entry {
diff --git a/sequencer.h b/sequencer.h
index 3d0b68c34c..858816f94a 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -127,7 +127,8 @@ const char *todo_item_get_arg(struct todo_list *todo_list,
 void sequencer_init_config(struct replay_opts *opts);
 int sequencer_pick_revisions(struct repository *repo,
 			     struct replay_opts *opts);
-int sequencer_continue(struct repository *repo, struct replay_opts *opts);
+int sequencer_continue(struct repository *repo, struct replay_opts *opts,
+		       struct todo_list *p_todo_list);
 int sequencer_rollback(struct repository *repo, struct replay_opts *opts);
 int sequencer_remove_state(struct replay_opts *opts);
 
-- 
2.22.0

[RFC PATCH 7/9] rebase-interactive: todo_list_check() also uses the done list

[Alban Gruin]

todo_list_check() works by checking if every commit in old_todo (the
backup list) is also present in new_todo (the todo list to check).

This works only when no commits have been picked (ie. right after the
initial edit).  In other cases, the backup list will contain one or
several commits that have been applied and are no longer in the todo
list.  Let's take this todo list, for instance:

	pick 123abc An interesting change
	break
	pick cba321 A boring change

After executing the `break' command, the backup list will contain the
original todo list, while the todo list will only contain the last
`pick' insn.  Even if commit 123abc has been picked up, in this case,
todo_list_check() will fail because it is not part of the todo list.

There is another list containing every command that have been executed:
the "done" list.  In our example, after the `break' command, the "done"
list will contain the first commit (123abc).  Comparing the backup list
against the todo list and the done list will fix this problem.

This changes todo_list_check() to compare old_todo against new_todo
_and_ the done list.  This should be useful to check the todo list when
resuming a rebase or reloading the todo list after an `exec' command,
but not for `--edit-todo' (the todo list is copied before edition).
But, for the sake of safety, it is still done in the next commit.

This also adds a function, todo_list_check_against_backup(), to load the
done list and the backup list, and call todo_list_check().

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 rebase-interactive.c | 57 ++++++++++++++++++++++++++++++++++++++------
 rebase-interactive.h |  6 ++++-
 sequencer.c          |  4 ++--
 3 files changed, 57 insertions(+), 10 deletions(-)

diff --git a/rebase-interactive.c b/rebase-interactive.c
index aa18ae82b7..c7dea85553 100644
--- a/rebase-interactive.c
+++ b/rebase-interactive.c
@@ -6,6 +6,8 @@
 #include "commit-slab.h"
 #include "config.h"
 
+static GIT_PATH_FUNC(rebase_path_done, "rebase-merge/done")
+
 enum missing_commit_check_level {
 	MISSING_COMMIT_CHECK_IGNORE = 0,
 	MISSING_COMMIT_CHECK_WARN,
@@ -124,13 +126,27 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list,
 }
 
 define_commit_slab(commit_seen, unsigned char);
+
+static void mark_commits_as_seen(struct commit_seen *commit_seen,
+				 struct todo_list *list)
+{
+	int i;
+
+	for (i = 0; i < list->nr; i++) {
+		struct commit *commit = list->items[i].commit;
+		if (commit)
+			*commit_seen_at(commit_seen, commit) = 1;
+	}
+}
+
 /*
  * Check if the user dropped some commits by mistake
  * Behaviour determined by rebase.missingCommitsCheck.
  * Check if there is an unrecognized command or a
  * bad SHA-1 in a command.
  */
-int todo_list_check(struct todo_list *old_todo, struct todo_list *new_todo)
+int todo_list_check(struct todo_list *old_todo, struct todo_list *new_todo,
+		    struct todo_list *done)
 {
 	enum missing_commit_check_level check_level = get_missing_commit_check_level();
 	struct strbuf missing = STRBUF_INIT;
@@ -142,12 +158,11 @@ int todo_list_check(struct todo_list *old_todo, struct todo_list *new_todo)
 	if (check_level == MISSING_COMMIT_CHECK_IGNORE)
 		goto leave_check;
 
-	/* Mark the commits in git-rebase-todo as seen */
-	for (i = 0; i < new_todo->nr; i++) {
-		struct commit *commit = new_todo->items[i].commit;
-		if (commit)
-			*commit_seen_at(&commit_seen, commit) = 1;
-	}
+	/* Mark the commits in git-rebase-todo and git-rebase-done as
+	   seen */
+	mark_commits_as_seen(&commit_seen, new_todo);
+	if (done)
+		mark_commits_as_seen(&commit_seen, done);
 
 	/* Find commits in git-rebase-todo.backup yet unseen */
 	for (i = old_todo->nr - 1; i >= 0; i--) {
@@ -187,3 +202,31 @@ int todo_list_check(struct todo_list *old_todo, struct todo_list *new_todo)
 	clear_commit_seen(&commit_seen);
 	return res;
 }
+
+int todo_list_check_against_backup(struct repository *r,
+				   struct todo_list *todo_list)
+{
+	struct todo_list done = TODO_LIST_INIT, initial = TODO_LIST_INIT;
+	int res;
+
+	if (strbuf_read_file(&done.buf, rebase_path_done(), 0) < 0 && errno != ENOENT)
+		return error(_("could not read '%s'"), rebase_path_done());
+
+	strbuf_read_file(&done.buf, rebase_path_done(), 0);
+	todo_list_parse_insn_buffer(r, done.buf.buf, &done);
+
+	if (strbuf_read_file(&initial.buf, rebase_path_todo_backup(), 0) < 0 &&
+	    errno != ENOENT) {
+		todo_list_release(&done);
+		return error(_("could not read '%s'"), rebase_path_done());
+	}
+
+	todo_list_parse_insn_buffer(r, initial.buf.buf, &initial);
+
+	res = todo_list_check(&initial, todo_list, &done);
+
+	todo_list_release(&done);
+	todo_list_release(&initial);
+
+	return res;
+}
diff --git a/rebase-interactive.h b/rebase-interactive.h
index 44dbb06311..ac401cda25 100644
--- a/rebase-interactive.h
+++ b/rebase-interactive.h
@@ -11,6 +11,10 @@ void append_todo_help(unsigned keep_empty, int command_count,
 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);
-int todo_list_check(struct todo_list *old_todo, struct todo_list *new_todo);
+
+int todo_list_check(struct todo_list *old_todo, struct todo_list *new_todo,
+		    struct todo_list *done);
+int todo_list_check_against_backup(struct repository *r,
+				   struct todo_list *todo_list);
 
 #endif
diff --git a/sequencer.c b/sequencer.c
index 3fb15ff8d9..0638c92f12 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4898,7 +4898,7 @@ int check_todo_list_from_file(struct repository *r)
 	if (!res)
 		res = todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo);
 	if (!res)
-		res = todo_list_check(&old_todo, &new_todo);
+		res = todo_list_check(&old_todo, &new_todo, NULL);
 	if (res)
 		fprintf(stderr, _(edit_todo_list_advice));
 out:
@@ -5008,7 +5008,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
 	}
 
 	if (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo) ||
-	    todo_list_check(todo_list, &new_todo)) {
+	    todo_list_check(todo_list, &new_todo, NULL)) {
 		fprintf(stderr, _(edit_todo_list_advice));
 		checkout_onto(r, opts, onto_name, &onto->object.oid, orig_head);
 		todo_list_release(&new_todo);
-- 
2.22.0

[RFC PATCH 8/9] rebase-interactive: warn if commit is dropped with --edit-todo

[Alban Gruin]

This adds the ability for --edit-todo to check if commits were dropped
by the user using todo_list_check_against_backup().

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 rebase-interactive.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/rebase-interactive.c b/rebase-interactive.c
index c7dea85553..c346d4ced4 100644
--- a/rebase-interactive.c
+++ b/rebase-interactive.c
@@ -95,6 +95,7 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list,
 {
 	const char *todo_file = rebase_path_todo();
 	unsigned initial = shortrevisions && shortonto;
+	int res = 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
@@ -119,10 +120,13 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list,
 
 	/* 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 (!initial) {
+		res = todo_list_parse_insn_buffer(r, new_todo->buf.buf, new_todo);
+		if (!res)
+			res = todo_list_check_against_backup(r, new_todo);
+	}
 
-	return 0;
+	return res;
 }
 
 define_commit_slab(commit_seen, unsigned char);
-- 
2.22.0

[RFC PATCH 9/9] sequencer: have read_populate_todo() check for dropped commits

[Alban Gruin]

This adds the ability to check if commits were dropped when resuming a
rebase (with `--continue') or when reloading the todo list after an
`exec' command.

Tests added previously should work now.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 sequencer.c                   | 5 ++++-
 t/t3404-rebase-interactive.sh | 4 ++--
 t/t3429-rebase-edit-todo.sh   | 4 ++--
 3 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 0638c92f12..d2c4459e7c 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2360,7 +2360,6 @@ static int read_populate_todo(struct repository *r,
 	struct stat st;
 	const char *todo_file = get_todo_path(opts);
 	int res;
-
 	strbuf_reset(&todo_list->buf);
 	if (strbuf_read_file_or_whine(&todo_list->buf, todo_file) < 0)
 		return -1;
@@ -2378,6 +2377,10 @@ static int read_populate_todo(struct repository *r,
 		return error(_("unusable instruction sheet: '%s'"), todo_file);
 	}
 
+	res = todo_list_check_against_backup(r, todo_list);
+	if (res)
+		return -1;
+
 	if (!todo_list->nr &&
 	    (!is_rebase_i(opts) || !file_exists(rebase_path_done())))
 		return error(_("no commits parsed."));
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index f5c0a8d2bb..090a496bcc 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1373,7 +1373,7 @@ EOF
 
 tail -n 8 <expect >expect.2
 
-test_expect_failure 'rebase --edit-todo respects rebase.missingCommitsCheck = warn' '
+test_expect_success 'rebase --edit-todo respects rebase.missingCommitsCheck = warn' '
 	test_config rebase.missingCommitsCheck warn &&
 	rebase_setup_and_clean missing-commit &&
 	set_fake_editor &&
@@ -1405,7 +1405,7 @@ EOF
 
 tail -n 9 <expect >expect.2
 
-test_expect_failure 'rebase --edit-todo respects rebase.missingCommitsCheck = error' '
+test_expect_success 'rebase --edit-todo respects rebase.missingCommitsCheck = error' '
 	test_config rebase.missingCommitsCheck error &&
 	rebase_setup_and_clean missing-commit &&
 	set_fake_editor &&
diff --git a/t/t3429-rebase-edit-todo.sh b/t/t3429-rebase-edit-todo.sh
index 2bb9fb65fa..79cd5657b3 100755
--- a/t/t3429-rebase-edit-todo.sh
+++ b/t/t3429-rebase-edit-todo.sh
@@ -50,7 +50,7 @@ Use 'git config rebase.missingCommitsCheck' to change the level of warnings.
 The possible behaviours are: ignore, warn, error.
 EOF
 
-test_expect_failure 'rebase exec respects rebase.missingCommitsCheck = warn' '
+test_expect_success 'rebase exec respects rebase.missingCommitsCheck = warn' '
 	test_config rebase.missingCommitsCheck warn &&
 	git reset --hard HEAD@{2} &&
 	git rebase HEAD~2 --keep-empty -x "echo >$todo" 2>actual.2 &&
@@ -59,7 +59,7 @@ test_expect_failure 'rebase exec respects rebase.missingCommitsCheck = warn' '
 	test 5 = $(git cat-file commit HEAD | sed -ne \$p)
 '
 
-test_expect_failure 'rebase exec respects rebase.missingCommitsCheck = error' '
+test_expect_success 'rebase exec respects rebase.missingCommitsCheck = error' '
 	test_config rebase.missingCommitsCheck error &&
 	git reset --hard HEAD@{2} &&
 	test_must_fail git rebase HEAD~2 --keep-empty -x "echo >$todo" 2>actual.2 &&
-- 
2.22.0

Re: [RFC PATCH 1/9] t3404: demonstrate that --edit-todo does not check for dropped commits

[Junio C Hamano]

Alban Gruin <alban.gruin@gmail.com> writes:

> 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 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.
>
> Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
> ---
>  t/t3404-rebase-interactive.sh | 82 +++++++++++++++++++++++++++++++++++
>  1 file changed, 82 insertions(+)
>
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 461dd539ff..f5c0a8d2bb 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -1345,6 +1345,88 @@ 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 &&
> +	test_must_fail env FAKE_LINES="1 2 bad 3 4" \
> +		git rebase -i --root >/dev/null 2>stderr &&

Do you need to capture into stderr?  Nobody seems to use it.

> +	FAKE_LINES="1 2 4" git rebase --edit-todo &&
> +	git rebase --continue 2>actual &&
> +	test D = $(git cat-file commit HEAD | sed -ne \$p) &&
> +	test_i18ngrep \
> +		"Successfully rebased and updated refs/heads/missing-commit" \
> +		actual
> +'
> +
> +cat >expect <<EOF
> +error: invalid line 5: badcmd $(git rev-list --pretty=oneline --abbrev-commit -1 master)
> +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.
> +
> +EOF
> +
> +tail -n 8 <expect >expect.2

Having this outside the test_expect_success block that uses the file
is bad.  You may have mimicked other tests in the same script, but
that is not a good excuse to make things worse.  Just make sure
these new stuff follow the best-current-practice pattern without
touching the existing bad examples (and later fix them up after the
dust settles, but don't let it distract you from the theme these
patches are addressing).

> +
> +test_expect_failure 'rebase --edit-todo respects rebase.missingCommitsCheck = warn' '
> +	test_config rebase.missingCommitsCheck warn &&
> +	rebase_setup_and_clean missing-commit &&
> +	set_fake_editor &&
> +	test_must_fail env FAKE_LINES="1 2 3 4 bad 5" \
> +		git rebase -i --root >/dev/null 2>stderr &&

Ditto.

> +	FAKE_LINES="1 2 3 4" git rebase --edit-todo 2>actual &&
> +	test_i18ncmp expect actual &&

So, after "--edit-todo", you are supposed to get an error and a warning,
but ...

> +	git rebase --continue 2>actual.2 &&
> +	head -n 8 <actual.2 >actual &&
> +	test_i18ncmp expect.2 actual &&

... after "--continue", you do not get any error, as you removed
'bad' from the input, but you still get a warning, followed by a
report of the fact that a commit has been dropped.  OK.

> +	test D = $(git cat-file commit HEAD | sed -ne \$p) &&
> +	test_i18ngrep \
> +		"Successfully rebased and updated refs/heads/missing-commit" \
> +		actual.2
> +'
> +
> +cat >expect <<EOF
> +error: invalid line 3: badcmd $(git rev-list --pretty=oneline --abbrev-commit -1 master~2)
> +Warning: some commits may have been dropped accidentally.
> +Dropped commits (newer to older):
> + - $(git rev-list --pretty=oneline --abbrev-commit -1 master)
> + - $(git rev-list --pretty=oneline --abbrev-commit -1 master~2)
> +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.
> +
> +EOF
> +
> +tail -n 9 <expect >expect.2
> +
> +test_expect_failure 'rebase --edit-todo respects rebase.missingCommitsCheck = error' '
> +	test_config rebase.missingCommitsCheck error &&
> +	rebase_setup_and_clean missing-commit &&
> +	set_fake_editor &&
> +	test_must_fail env FAKE_LINES="1 2 bad 3 4" \
> +		git rebase -i --root >/dev/null 2>stderr &&
> +	test_must_fail env FAKE_LINES="1 2 4" \
> +		git rebase --edit-todo 2>actual &&
> +	test_i18ncmp expect actual &&
> +	test_must_fail git rebase --continue 2>actual &&

OK, and this one fails as the configuration is set to 'error'.

> +	test_i18ncmp expect.2 actual &&
> +	cp .git/rebase-merge/git-rebase-todo.backup \
> +		.git/rebase-merge/git-rebase-todo &&

Why?  Who uses this copy?

> +	FAKE_LINES="1 2 drop 3 4 drop 5" \
> +		git rebase --edit-todo &&
> +	git rebase --continue 2>actual &&
> +	test D = $(git cat-file commit HEAD | sed -ne \$p) &&
> +	test_i18ngrep \
> +		"Successfully rebased and updated refs/heads/missing-commit" \
> +		actual
> +'
> +
>  test_expect_success 'respects rebase.abbreviateCommands with fixup, squash and exec' '
>  	rebase_setup_and_clean abbrevcmd &&
>  	test_commit "first" file1.txt "first line" first &&

Re: [RFC PATCH 3/9] sequencer: update `total_nr' when adding an item to a todo list

[Junio C Hamano]

Alban Gruin <alban.gruin@gmail.com> writes:

> `total_nr' is the total amount of items, done and toto, that are in a

"amount" -> "number" perhaps.  Also s/toto/todo/ perhaps but I am
not sure what you wanted to say here, so...

> todo list.  But unlike `nr', it was not updated when an item was
> appended to the list.

Good finding.

>
> Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
> ---
>  sequencer.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/sequencer.c b/sequencer.c
> index cf262701e8..e61ae75451 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2070,6 +2070,7 @@ void todo_list_release(struct todo_list *todo_list)
>  static struct todo_item *append_new_todo(struct todo_list *todo_list)
>  {
>  	ALLOC_GROW(todo_list->items, todo_list->nr + 1, todo_list->alloc);
> +	++todo_list->total_nr;

When we do not use the value in an expression, we prefer post
increment, not pre increment.

>  	return todo_list->items + todo_list->nr++;
>  }

Re: [RFC PATCH 4/9] sequencer: update `done_nr' when skipping commands in a todo list

[Junio C Hamano]

Alban Gruin <alban.gruin@gmail.com> writes:

> In a todo list, `done_nr' is the amount of commands that were executed
> or skipped, but skip_unnecessary_picks() did not update it.

OK.  Together with 3/9 and this one, any increment of total_nr and
done_nr in the existing code is not removed; does it mean that
nobody actually cares what these fields contain?  IOW, there is no
code that says "if (list->total_nr <= i) { we are done; }" etc.?

Or are these fields used later, but somehow the lack of increment in
the places touched by 3/9 and 4/9 is compensated?

> Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
> ---
>  sequencer.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/sequencer.c b/sequencer.c
> index e61ae75451..ec9c3d4dc5 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -4939,6 +4939,7 @@ static int skip_unnecessary_picks(struct repository *r,
>  		MOVE_ARRAY(todo_list->items, todo_list->items + i, todo_list->nr - i);
>  		todo_list->nr -= i;
>  		todo_list->current = 0;
> +		todo_list->done_nr += i;
>  
>  		if (is_fixup(peek_command(todo_list, 0)))
>  			record_in_rewritten(base_oid, peek_command(todo_list, 0));

Re: [RFC PATCH 5/9] sequencer: move the code writing total_nr on the disk to a new function

[Junio C Hamano]

Alban Gruin <alban.gruin@gmail.com> writes:

> The total amount of commands can be used to show the progression of the
> rebasing in a shell.  This number is written to the disk by
> read_populate_todo() when the todo list is loaded from
> sequencer_continue() or pick_commits(), but not by complete_action(),
> which releases its todo list before calling sequencer_continue(), which
> reloads it immediatly afterwards.
>
> To avoid to reload the todo list from the disk, sequencer_continue()
> will be modified to accept a todo list, and if it is not null,
> read_populate_todo() will not be called.

That may be good as a plan, but readers who are reading this step
are left puzzled as the changes so far do not seem to have much to
do with that change.  Perhaps this paragraph belongs to the step
that actually makes that modification?

> This moves the part writing total_nr to a new function so it can be
> called by complete_action().

So without 3/9 and 4/9, we have been simply writing out wrong
number?  Good.

>
> Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
> ---
>  sequencer.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index ec9c3d4dc5..d66b80d49f 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2343,6 +2343,16 @@ void sequencer_post_commit_cleanup(struct repository *r, int verbose)
>  	sequencer_remove_state(&opts);
>  }
>  
> +static void todo_list_write_total_nr(struct todo_list *todo_list)
> +{
> +	FILE *f = fopen_or_warn(rebase_path_msgtotal(), "w");
> +
> +	if (f) {
> +		fprintf(f, "%d\n", todo_list->total_nr);
> +		fclose(f);
> +	}
> +}
> +
>  static int read_populate_todo(struct repository *r,
>  			      struct todo_list *todo_list,
>  			      struct replay_opts *opts)
> @@ -2388,7 +2398,6 @@ static int read_populate_todo(struct repository *r,
>  
>  	if (is_rebase_i(opts)) {
>  		struct todo_list done = TODO_LIST_INIT;
> -		FILE *f = fopen_or_warn(rebase_path_msgtotal(), "w");
>  
>  		if (strbuf_read_file(&done.buf, rebase_path_done(), 0) > 0 &&
>  		    !todo_list_parse_insn_buffer(r, done.buf.buf, &done))
> @@ -2400,10 +2409,7 @@ static int read_populate_todo(struct repository *r,
>  			+ count_commands(todo_list);
>  		todo_list_release(&done);
>  
> -		if (f) {
> -			fprintf(f, "%d\n", todo_list->total_nr);
> -			fclose(f);
> -		}
> +		todo_list_write_total_nr(todo_list);
>  	}
>  
>  	return 0;

Re: [RFC PATCH 1/9] t3404: demonstrate that --edit-todo does not check for dropped commits

[Alban Gruin]

Hi Junio,

Le 18/07/2019 à 20:31, Junio C Hamano a écrit :
> Alban Gruin <alban.gruin@gmail.com> writes:
> > 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 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.
> > 
> > Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
> > ---
> > 
> >  t/t3404-rebase-interactive.sh | 82 +++++++++++++++++++++++++++++++++++
> >  1 file changed, 82 insertions(+)
> > 
> > diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> > index 461dd539ff..f5c0a8d2bb 100755
> > --- a/t/t3404-rebase-interactive.sh
> > +++ b/t/t3404-rebase-interactive.sh
> > @@ -1345,6 +1345,88 @@ 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 &&
> > +	test_must_fail env FAKE_LINES="1 2 bad 3 4" \
> > +		git rebase -i --root >/dev/null 2>stderr &&
> 
> Do you need to capture into stderr?  Nobody seems to use it.
> 

No.  I’m changing this.

> > +	FAKE_LINES="1 2 4" git rebase --edit-todo &&
> > +	git rebase --continue 2>actual &&
> > +	test D = $(git cat-file commit HEAD | sed -ne \$p) &&
> > +	test_i18ngrep \
> > +		"Successfully rebased and updated refs/heads/missing-
commit" \
> > +		actual
> > +'
> > +
> > +cat >expect <<EOF
> > +error: invalid line 5: badcmd $(git rev-list --pretty=oneline
> > --abbrev-commit -1 master) +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.
> > +
> > +EOF
> > +
> > +tail -n 8 <expect >expect.2
> 
> Having this outside the test_expect_success block that uses the file
> is bad.  You may have mimicked other tests in the same script, but
> that is not a good excuse to make things worse.  Just make sure
> these new stuff follow the best-current-practice pattern without
> touching the existing bad examples (and later fix them up after the
> dust settles, but don't let it distract you from the theme these
> patches are addressing).
> 

Okay.

> > +
> > +test_expect_failure 'rebase --edit-todo respects
> > rebase.missingCommitsCheck = warn' ' +	test_config
> > rebase.missingCommitsCheck warn &&
> > +	rebase_setup_and_clean missing-commit &&
> > +	set_fake_editor &&
> > +	test_must_fail env FAKE_LINES="1 2 3 4 bad 5" \
> > +		git rebase -i --root >/dev/null 2>stderr &&
> 
> Ditto.
> 
> > +	FAKE_LINES="1 2 3 4" git rebase --edit-todo 2>actual &&
> > +	test_i18ncmp expect actual &&
> 
> So, after "--edit-todo", you are supposed to get an error and a warning,
> but ...
> 
> > +	git rebase --continue 2>actual.2 &&
> > +	head -n 8 <actual.2 >actual &&
> > +	test_i18ncmp expect.2 actual &&
> 
> ... after "--continue", you do not get any error, as you removed
> 'bad' from the input, but you still get a warning, followed by a
> report of the fact that a commit has been dropped.  OK.
> 
> > +	test D = $(git cat-file commit HEAD | sed -ne \$p) &&
> > +	test_i18ngrep \
> > +		"Successfully rebased and updated refs/heads/missing-
commit" \
> > +		actual.2
> > +'
> > +
> > +cat >expect <<EOF
> > +error: invalid line 3: badcmd $(git rev-list --pretty=oneline
> > --abbrev-commit -1 master~2) +Warning: some commits may have been dropped
> > accidentally.
> > +Dropped commits (newer to older):
> > + - $(git rev-list --pretty=oneline --abbrev-commit -1 master)
> > + - $(git rev-list --pretty=oneline --abbrev-commit -1 master~2)
> > +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.
> > +
> > +EOF
> > +
> > +tail -n 9 <expect >expect.2
> > +
> > +test_expect_failure 'rebase --edit-todo respects
> > rebase.missingCommitsCheck = error' ' +	test_config
> > rebase.missingCommitsCheck error &&
> > +	rebase_setup_and_clean missing-commit &&
> > +	set_fake_editor &&
> > +	test_must_fail env FAKE_LINES="1 2 bad 3 4" \
> > +		git rebase -i --root >/dev/null 2>stderr &&
> > +	test_must_fail env FAKE_LINES="1 2 4" \
> > +		git rebase --edit-todo 2>actual &&
> > +	test_i18ncmp expect actual &&
> > +	test_must_fail git rebase --continue 2>actual &&
> 
> OK, and this one fails as the configuration is set to 'error'.
> 
> > +	test_i18ncmp expect.2 actual &&
> > +	cp .git/rebase-merge/git-rebase-todo.backup \
> > +		.git/rebase-merge/git-rebase-todo &&
> 
> Why?  Who uses this copy?
> 

The same technique is used in "rebase -i respects rebase.missingCommitsCheck = 
error".

> > +	FAKE_LINES="1 2 drop 3 4 drop 5" \
> > +		git rebase --edit-todo &&
> > +	git rebase --continue 2>actual &&
> > +	test D = $(git cat-file commit HEAD | sed -ne \$p) &&
> > +	test_i18ngrep \
> > +		"Successfully rebased and updated refs/heads/missing-
commit" \
> > +		actual
> > +'
> > +
> > 
> >  test_expect_success 'respects rebase.abbreviateCommands with fixup,
> >  squash and exec' '>  
> >  	rebase_setup_and_clean abbrevcmd &&
> >  	test_commit "first" file1.txt "first line" first &&

Re: [RFC PATCH 3/9] sequencer: update `total_nr' when adding an item to a todo list

[Alban Gruin]

Hi,

Le 18/07/2019 à 21:52, Junio C Hamano a écrit :
> Alban Gruin <alban.gruin@gmail.com> writes:
> > `total_nr' is the total amount of items, done and toto, that are in a
> 
> "amount" -> "number" perhaps.  Also s/toto/todo/ perhaps but I am
> not sure what you wanted to say here, so...
> 

`total_nr' is the number of commands, whether they have been executed or not.  
It’s used to show the progression of the rebase.

> > todo list.  But unlike `nr', it was not updated when an item was
> > appended to the list.
> 
> Good finding.
> 
> > Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
> > ---
> > 
> >  sequencer.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/sequencer.c b/sequencer.c
> > index cf262701e8..e61ae75451 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -2070,6 +2070,7 @@ void todo_list_release(struct todo_list *todo_list)
> > 
> >  static struct todo_item *append_new_todo(struct todo_list *todo_list)
> >  {
> >  
> >  	ALLOC_GROW(todo_list->items, todo_list->nr + 1, todo_list->alloc);
> > 
> > +	++todo_list->total_nr;
> 
> When we do not use the value in an expression, we prefer post
> increment, not pre increment.
> 
> >  	return todo_list->items + todo_list->nr++;
> >  
> >  }

Re: [RFC PATCH 4/9] sequencer: update `done_nr' when skipping commands in a todo list

[Alban Gruin]

Hi,

Le 18/07/2019 à 21:55, Junio C Hamano a écrit :
> Alban Gruin <alban.gruin@gmail.com> writes:
> > In a todo list, `done_nr' is the amount of commands that were executed
> > or skipped, but skip_unnecessary_picks() did not update it.
> 
> OK.  Together with 3/9 and this one, any increment of total_nr and
> done_nr in the existing code is not removed; does it mean that
> nobody actually cares what these fields contain?  IOW, there is no
> code that says "if (list->total_nr <= i) { we are done; }" etc.?
> 
> Or are these fields used later, but somehow the lack of increment in
> the places touched by 3/9 and 4/9 is compensated?
> 

`total_nr' is not used for this, because it’s not necessarily the number of 
items in the todo list.  That’s the role of `nr'.  So the comparison is more 
like "if (list->nr <= i) { we are done; }".

Same think for `done_nr'.  Each time a command is executed, git prints 
"Rebasing ($done_nr/$total_nr)".  These two variables are written to the disk, 
and might be used by a shell prompt (eg. git-prompt.sh, oh my zsh…)

And this is actually how I found this.  Originally, I wrote what became 5/9 
and 6/9, without touching to `done_nr' and `total_nr'.  All rebase tests 
(t34??*) passed, but t9903.15 ("prompt - rebase merge") failed, because the 
value was incorrect.

The reason is that, before I changed sequencer_continue() in 6/9, it called 
another function, read_populate_todo(), which would recompute `done_nr' and 
`total_nr', then write them to the disk.  With my changes, these values would 
not have been updated after adding `exec' commands or skipping picks in 
complete_action(), so the numbers written to the disk were incorrect.

tl;dr: it does not impact how the rebase works, but it might impact the 
messages printed while rebasing or shell prompts.

> > Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
> > ---
> > 
> >  sequencer.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/sequencer.c b/sequencer.c
> > index e61ae75451..ec9c3d4dc5 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -4939,6 +4939,7 @@ static int skip_unnecessary_picks(struct repository
> > *r,> 
> >  		MOVE_ARRAY(todo_list->items, todo_list->items + i, 
todo_list->nr - i);
> >  		todo_list->nr -= i;
> >  		todo_list->current = 0;
> > 
> > +		todo_list->done_nr += i;
> > 
> >  		if (is_fixup(peek_command(todo_list, 0)))
> >  		
> >  			record_in_rewritten(base_oid, 
peek_command(todo_list, 0));

Re: [RFC PATCH 5/9] sequencer: move the code writing total_nr on the disk to a new function

[Alban Gruin]

Hi,

Le 18/07/2019 à 22:04, Junio C Hamano a écrit :
> Alban Gruin <alban.gruin@gmail.com> writes:
> > The total amount of commands can be used to show the progression of the
> > rebasing in a shell.  This number is written to the disk by
> > read_populate_todo() when the todo list is loaded from
> > sequencer_continue() or pick_commits(), but not by complete_action(),
> > which releases its todo list before calling sequencer_continue(), which
> > reloads it immediatly afterwards.
> > 
> > To avoid to reload the todo list from the disk, sequencer_continue()
> > will be modified to accept a todo list, and if it is not null,
> > read_populate_todo() will not be called.
> 
> That may be good as a plan, but readers who are reading this step
> are left puzzled as the changes so far do not seem to have much to
> do with that change.  Perhaps this paragraph belongs to the step
> that actually makes that modification?
> 
> > This moves the part writing total_nr to a new function so it can be
> > called by complete_action().
> 
> So without 3/9 and 4/9, we have been simply writing out wrong
> number?  Good.
> 

Not here, the numbers computed in read_populate_todo() are correct.  But they 
may be incorrect in complete_action(), before calling sequencer_continue().  
This was not a big deal as the todo list was released before and 
sequencer_continue() re-read it, but it would have became a problem with 6/9.

> > Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
> > ---
> > 
> >  sequencer.c | 16 +++++++++++-----
> >  1 file changed, 11 insertions(+), 5 deletions(-)
> > 
> > diff --git a/sequencer.c b/sequencer.c
> > index ec9c3d4dc5..d66b80d49f 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -2343,6 +2343,16 @@ void sequencer_post_commit_cleanup(struct
> > repository *r, int verbose)> 
> >  	sequencer_remove_state(&opts);
> >  
> >  }
> > 
> > +static void todo_list_write_total_nr(struct todo_list *todo_list)
> > +{
> > +	FILE *f = fopen_or_warn(rebase_path_msgtotal(), "w");
> > +
> > +	if (f) {
> > +		fprintf(f, "%d\n", todo_list->total_nr);
> > +		fclose(f);
> > +	}
> > +}
> > +
> > 
> >  static int read_populate_todo(struct repository *r,
> >  
> >  			      struct todo_list *todo_list,
> >  			      struct replay_opts *opts)
> > 
> > @@ -2388,7 +2398,6 @@ static int read_populate_todo(struct repository *r,
> > 
> >  	if (is_rebase_i(opts)) {
> >  	
> >  		struct todo_list done = TODO_LIST_INIT;
> > 
> > -		FILE *f = fopen_or_warn(rebase_path_msgtotal(), "w");
> > 
> >  		if (strbuf_read_file(&done.buf, rebase_path_done(), 0) 
> 0 &&
> >  		
> >  		    !todo_list_parse_insn_buffer(r, done.buf.buf, 
&done))
> > 
> > @@ -2400,10 +2409,7 @@ static int read_populate_todo(struct repository *r,
> > 
> >  			+ count_commands(todo_list);
> >  		
> >  		todo_list_release(&done);
> > 
> > -		if (f) {
> > -			fprintf(f, "%d\n", todo_list->total_nr);
> > -			fclose(f);
> > -		}
> > +		todo_list_write_total_nr(todo_list);
> > 
> >  	}
> >  	
> >  	return 0;

Re: [RFC PATCH 1/9] t3404: demonstrate that --edit-todo does not check for dropped commits

[Junio C Hamano]

Alban Gruin <alban.gruin@gmail.com> writes:

>> > +	test_i18ncmp expect.2 actual &&
>> > +	cp .git/rebase-merge/git-rebase-todo.backup \
>> > +		.git/rebase-merge/git-rebase-todo &&
>> 
>> Why?  Who uses this copy?
>> 
>
> The same technique is used in "rebase -i respects rebase.missingCommitsCheck = 
> error".

Ah, I misread the code.  I thought you were making a copy in .backup
that nobody looks at after the copy is made.

Re: [RFC PATCH 0/9] rebase -i: extend rebase.missingCommitsCheck to `--edit-todo' and co.

[Phillip Wood]

Hi Alban

Thanks for working on this, it's great to see you back on the list and I 
think it would be a useful addition to rebase. Unfortunately I'm not 
sure about this implementation though (although the early bug fix 
patches are useful in their own right)

On 17/07/2019 15:39, Alban Gruin wrote:
> 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
> dropped a commit, no warnings or errors would be issued.
> 
> This adds the ability to check if commits were dropped when resuming a
> rebase (with `rebase --continue'), when editing the todo list (with
> `rebase --edit-todo'), or when reloading the todo list after an `exec'
> command.

I'm not sure if we really need to check the todo list when continuing or 
after an exec command. The official way to edit the todo list is to run 
'git rebase --edit-todo' and I'm not sure if we support scripts writing 
to .git/rebase-merge/git-rebase-todo directly. If we only support the 
check after --edit-todo then I think the implementation can be 
simplified as we can just write a copy of the file before it is edited 
and don't need to check .git/rebase-merge/done. Additionally that would 
catch commits that are added by the user and then deleted in a later 
edit. They wont be in the original list so I don't think this series 
will detect their deletion.

At the extreme I have a script around rebase that runs 'rebase -i HEAD' 
and then fills in the todo list with a fake editor that adds 'reset ...' 
as the first line to set the starting point of the rebase. I think 
dscho's garden-shears script does something similar. Under the proposed 
scheme if I subsequently edit the todo list it will not catch any 
deleted commits as the original list is empty.

Best Wishes

Phillip

> The idea to extend this feature was suggested to me more than a year ago
> by Phillip Wood, if I'm not mistaken.  I postponed this until four month
> ago, when ag/sequencer-reduce-rewriting-todo finally hit master, but I
> had to stop because of other obligations.  I could go back to work one
> month ago, when I did the bulk of this series, but I lacked time to
> polish it, so it waited a bit more.  Now, I think it is in a good shape
> to be sent, although it is still RFC-quality to me.  The advertised
> functionality should work well, but perhaps there is some flaws I
> missed.
> 
> The first two patches are new tests, demonstrating that after the
> initial edit, the check is not done.  The next four are what could be
> qualified as omissions from ag/sequencer-reduce-rewriting-todo, but they
> are quite important (IMHO) for the rest of the series.  The last three
> actually extend rebase.missingCommitsCheck.
> 
> This is based on master (9d418600f4, "The fifth batch").
> 
> The tip of this series is tagged as "edit-todo-drop-rfc" in
> https://github.com/agrn/git.
> 
> Alban Gruin (9):
>    t3404: demonstrate that --edit-todo does not check for dropped commits
>    t3429: demonstrate that rebase exec does not check for dropped commits
>    sequencer: update `total_nr' when adding an item to a todo list
>    sequencer: update `done_nr' when skipping commands in a todo list
>    sequencer: move the code writing total_nr on the disk to a new
>      function
>    sequencer: add a parameter to sequencer_continue() to accept a todo
>      list
>    rebase-interactive: todo_list_check() also uses the done list
>    rebase-interactive: warn if commit is dropped with --edit-todo
>    sequencer: have read_populate_todo() check for dropped commits
> 
>   builtin/rebase.c              |  2 +-
>   builtin/revert.c              |  2 +-
>   rebase-interactive.c          | 67 +++++++++++++++++++++++-----
>   rebase-interactive.h          |  6 ++-
>   sequencer.c                   | 53 ++++++++++++++--------
>   sequencer.h                   |  3 +-
>   t/t3404-rebase-interactive.sh | 82 +++++++++++++++++++++++++++++++++++
>   t/t3429-rebase-edit-todo.sh   | 44 ++++++++++++++++++-
>   8 files changed, 224 insertions(+), 35 deletions(-)
> 

Re: [RFC PATCH 0/9] rebase -i: extend rebase.missingCommitsCheck to `--edit-todo' and co.

[Johannes Schindelin]

Hi,

On Wed, 24 Jul 2019, Phillip Wood wrote:

> On 17/07/2019 15:39, Alban Gruin wrote:
>
> > 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 dropped a commit, no warnings or errors
> > would be issued.
> >
> > This adds the ability to check if commits were dropped when resuming
> > a rebase (with `rebase --continue'), when editing the todo list
> > (with `rebase --edit-todo'), or when reloading the todo list after
> > an `exec' command.
>
> I'm not sure if we really need to check the todo list when continuing or after
> an exec command. The official way to edit the todo list is to run 'git rebase
> --edit-todo' and I'm not sure if we support scripts writing to
> .git/rebase-merge/git-rebase-todo directly. If we only support the check after
> --edit-todo then I think the implementation can be simplified as we can just
> write a copy of the file before it is edited and don't need to check
> .git/rebase-merge/done. Additionally that would catch commits that are added
> by the user and then deleted in a later edit. They wont be in the original
> list so I don't think this series will detect their deletion.
>
> At the extreme I have a script around rebase that runs 'rebase -i HEAD' and
> then fills in the todo list with a fake editor that adds 'reset ...' as the
> first line to set the starting point of the rebase. I think dscho's
> garden-shears script does something similar. Under the proposed scheme if I
> subsequently edit the todo list it will not catch any deleted commits as the
> original list is empty.

Indeed, and in the meantime there is also an ever-green.sh script that
not only uses that trick to custom-populate the todo list, but also
_extends_ it via an `exec` command at the end of the first rebase run,
essentially allowing for a nested rebase.

Ciao,
Dscho

Re: [RFC PATCH 0/9] rebase -i: extend rebase.missingCommitsCheck to `--edit-todo' and co.

[Alban Gruin]

Hi Phillip,

Le 24/07/2019 à 15:29, Phillip Wood a écrit :
> Hi Alban
> 
> Thanks for working on this, it's great to see you back on the list and I
> think it would be a useful addition to rebase. Unfortunately I'm not
> sure about this implementation though (although the early bug fix
> patches are useful in their own right)
> 
> On 17/07/2019 15:39, Alban Gruin wrote:
>> 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
>> dropped a commit, no warnings or errors would be issued.
>>
>> This adds the ability to check if commits were dropped when resuming a
>> rebase (with `rebase --continue'), when editing the todo list (with
>> `rebase --edit-todo'), or when reloading the todo list after an `exec'
>> command.
> 
> I'm not sure if we really need to check the todo list when continuing or
> after an exec command.

In which case I don’t really understand why there is an `error' mode if
one can completely bypass it with `--continue'.

> The official way to edit the todo list is to run
> 'git rebase --edit-todo' and I'm not sure if we support scripts writing
> to .git/rebase-merge/git-rebase-todo directly. If we only support the
> check after --edit-todo then I think the implementation can be
> simplified as we can just write a copy of the file before it is edited
> and don't need to check .git/rebase-merge/done. Additionally that would
> catch commits that are added by the user and then deleted in a later
> edit. They wont be in the original list so I don't think this series
> will detect their deletion.
> 

True -- but with this solution, if a bad command is introduced, there
will be false negatives.  Given the pitfall of my solution, this should
be an acceptable trade-off.

> At the extreme I have a script around rebase that runs 'rebase -i HEAD'
> and then fills in the todo list with a fake editor that adds 'reset ...'
> as the first line to set the starting point of the rebase. I think
> dscho's garden-shears script does something similar. Under the proposed
> scheme if I subsequently edit the todo list it will not catch any
> deleted commits as the original list is empty.
> 
> Best Wishes
> 
> Phillip
> 

Cheers,
Alban

Re: [RFC PATCH 0/9] rebase -i: extend rebase.missingCommitsCheck to `--edit-todo' and co.

[Phillip Wood]

Hi Alban

On 25/07/2019 21:26, Alban Gruin wrote:
> Hi Phillip,
> 
> Le 24/07/2019 à 15:29, Phillip Wood a écrit :
>> Hi Alban
>>
>> Thanks for working on this, it's great to see you back on the list and I
>> think it would be a useful addition to rebase. Unfortunately I'm not
>> sure about this implementation though (although the early bug fix
>> patches are useful in their own right)
>>
>> On 17/07/2019 15:39, Alban Gruin wrote:
>>> 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
>>> dropped a commit, no warnings or errors would be issued.
>>>
>>> This adds the ability to check if commits were dropped when resuming a
>>> rebase (with `rebase --continue'), when editing the todo list (with
>>> `rebase --edit-todo'), or when reloading the todo list after an `exec'
>>> command.
>>
>> I'm not sure if we really need to check the todo list when continuing or
>> after an exec command.
> 
> In which case I don’t really understand why there is an `error' mode if
> one can completely bypass it with `--continue'.

That's an interesting point about `--continue`. Perhaps if `--edit-todo` 
detects deleted lines in error mode it should write a file to stop 
`--continue` continuing rather than having to validate the entire list 
each time we continue a rebase. Alternatively we could annotate the todo 
list with a message the dropped commits commented out and reopen the 
editor for the user to fix the problem, but that would cause scripted 
editors to enter a infinite loop as they're unlikely to fix the problem 
the second time round. A third possibility is to keep your code 
validating the list each time we run continue, but update the backup 
file with each edit so it detects added commits that are deleted in a 
later edit. This would also provide some protection for users who edit 
git-rebase-todo directly, though if they are using a script that deletes 
lines in git-rebase-todo directly it will suddenly stop working with 
this change if they have rebase.missingCommitsCheck set to error.

Having said all that we could decide that the existing error message is 
enough and allow the user to skip re-editing the list if they really did 
mean to remove those lines. It would be annoying to have to re-edit the 
list when one had intended to delete those lines.

>> The official way to edit the todo list is to run
>> 'git rebase --edit-todo' and I'm not sure if we support scripts writing
>> to .git/rebase-merge/git-rebase-todo directly. If we only support the
>> check after --edit-todo then I think the implementation can be
>> simplified as we can just write a copy of the file before it is edited
>> and don't need to check .git/rebase-merge/done. Additionally that would
>> catch commits that are added by the user and then deleted in a later
>> edit. They wont be in the original list so I don't think this series
>> will detect their deletion.
>>
> 
> True -- but with this solution, if a bad command is introduced, there
> will be false negatives.  Given the pitfall of my solution, this should
> be an acceptable trade-off.

We could detect a bad commit by checking the oid and not complaining if 
it is not valid. That's slightly complicated by labels, but we could 
fairly easily keep of list of the labels defined so far as we scan the 
list. That would also open the possibility of detecting errors where the 
user references an undefined label in `merge` or `reset` commands but 
that's a separate problem.

Best Wishes

Phillip

>> At the extreme I have a script around rebase that runs 'rebase -i HEAD'
>> and then fills in the todo list with a fake editor that adds 'reset ...'
>> as the first line to set the starting point of the rebase. I think
>> dscho's garden-shears script does something similar. Under the proposed
>> scheme if I subsequently edit the todo list it will not catch any
>> deleted commits as the original list is empty.
>>
>> Best Wishes
>>
>> Phillip
>>
> 
> Cheers,
> Alban
> 

Re: [RFC PATCH 0/9] rebase -i: extend rebase.missingCommitsCheck to `--edit-todo' and co.

[Alban Gruin]

Hi,

This has been a long time since I have not given an update on this --
sorry about that.  I think it’s time to do it.

Le 29/07/2019 à 11:38, Phillip Wood a écrit :
> Hi Alban
> 
> -%<-
> 
> That's an interesting point about `--continue`. Perhaps if `--edit-todo`
> detects deleted lines in error mode it should write a file to stop
> `--continue` continuing rather than having to validate the entire list
> each time we continue a rebase. Alternatively we could annotate the todo
> list with a message the dropped commits commented out and reopen the
> editor for the user to fix the problem, but that would cause scripted
> editors to enter a infinite loop as they're unlikely to fix the problem
> the second time round. A third possibility is to keep your code
> validating the list each time we run continue, but update the backup
> file with each edit so it detects added commits that are deleted in a
> later edit. This would also provide some protection for users who edit
> git-rebase-todo directly, though if they are using a script that deletes
> lines in git-rebase-todo directly it will suddenly stop working with
> this change if they have rebase.missingCommitsCheck set to error.
> 
> Having said all that we could decide that the existing error message is
> enough and allow the user to skip re-editing the list if they really did
> mean to remove those lines. It would be annoying to have to re-edit the
> list when one had intended to delete those lines.
> 

If we do not check the todo list after `exec' commands, patches 3 to 6
should be useless in this series and could be sent separately (I’m still
interested in reducing useless round trips to the disk).  I found a
cleaner way to do that, without touching to sequencer_continue().

For the main feature of this series, I need to write tests for it, and
then I’ll send it as a WIP series, once again.

Cheers,
Alban

[RFC PATCH v2 0/2] rebase -i: extend rebase.missingCommitsCheck to `--edit-todo'

[Alban Gruin]

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
dropped 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'.

The first patch adds new tests demonstrating that the todo list is not
checked with `--edit-todo'.  The second patch implements the check.

This series is pretty much a revamp of the first version.  Some patches
from the original series were rerolled separately and live now in
ag/sequencer-todo-updates.  This version is pretty much brand new, as
you can see in the range diff below.

Changes since v1 are about what the new todo list is compared to, and
when.  The original series compared the todo list against the original
edit, so if a commit was added then deleted, it would go unnoticed.
Now, it is compared against the previous version to avoid that.  This is
highlighted in the new tests.  The original series checked the todo list
after `--edit-todo', but also before continuing a rebase or after having
executed a command.  Now, it only checks it after `--edit-todo', as
suggested by Phillip Wood.

This is based on master (da72936f54, "Git 2.24").

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

Alban Gruin (2):
  t3404: demonstrate that --edit-todo does not check for dropped commits
  rebase-interactive: warn if commit is dropped with --edit-todo

 rebase-interactive.c          |  7 ++--
 sequencer.c                   |  5 +--
 t/t3404-rebase-interactive.sh | 75 +++++++++++++++++++++++++++++++++++
 3 files changed, 79 insertions(+), 8 deletions(-)

Diff-intervalle contre v1 :
 1:  b35f920318 !  1:  6974b6c8f2 t3404: demonstrate that --edit-todo does not check for dropped commits
    @@ Commit message
     
         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 and rebase --continue don't
    -    take it into account.
    +    mistakes.  Unfortunately, rebase --edit-todo 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.
     
    +    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.
    +
         Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
     
      ## t/t3404-rebase-interactive.sh ##
    @@ t/t3404-rebase-interactive.sh: test_expect_success 'rebase -i respects rebase.mi
     +	test_config rebase.missingCommitsCheck ignore &&
     +	rebase_setup_and_clean missing-commit &&
     +	set_fake_editor &&
    -+	test_must_fail env FAKE_LINES="1 2 bad 3 4" \
    -+		git rebase -i --root >/dev/null 2>stderr &&
    -+	FAKE_LINES="1 2 4" git rebase --edit-todo &&
    ++	FAKE_LINES="break 1 2 3 4 5" git rebase -i --root &&
    ++	FAKE_LINES="1 2 3 4" git rebase --edit-todo 2>actual &&
     +	git rebase --continue 2>actual &&
     +	test D = $(git cat-file commit HEAD | sed -ne \$p) &&
     +	test_i18ngrep \
    @@ t/t3404-rebase-interactive.sh: test_expect_success 'rebase -i respects rebase.mi
     +		actual
     +'
     +
    -+cat >expect <<EOF
    -+error: invalid line 5: badcmd $(git rev-list --pretty=oneline --abbrev-commit -1 master)
    -+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.
    -+
    -+EOF
    -+
    -+tail -n 8 <expect >expect.2
    -+
     +test_expect_failure '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="1 2 3 4 bad 5" \
    -+		git rebase -i --root >/dev/null 2>stderr &&
    -+	FAKE_LINES="1 2 3 4" git rebase --edit-todo 2>actual &&
    ++	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 &&
    -+	git rebase --continue 2>actual.2 &&
    -+	head -n 8 <actual.2 >actual &&
    ++	cp orig .git/rebase-merge/git-rebase-todo &&
    ++	FAKE_LINES="1 2 3 4" git rebase --edit-todo 2>actual.2 &&
    ++	head -n4 actual.2 >actual &&
     +	test_i18ncmp expect.2 actual &&
    ++	git rebase --continue 2>actual &&
     +	test D = $(git cat-file commit HEAD | sed -ne \$p) &&
     +	test_i18ngrep \
     +		"Successfully rebased and updated refs/heads/missing-commit" \
    -+		actual.2
    ++		actual
     +'
     +
    -+cat >expect <<EOF
    -+error: invalid line 3: badcmd $(git rev-list --pretty=oneline --abbrev-commit -1 master~2)
    -+Warning: some commits may have been dropped accidentally.
    -+Dropped commits (newer to older):
    -+ - $(git rev-list --pretty=oneline --abbrev-commit -1 master)
    -+ - $(git rev-list --pretty=oneline --abbrev-commit -1 master~2)
    -+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.
    -+
    -+EOF
    -+
    -+tail -n 9 <expect >expect.2
    -+
     +test_expect_failure '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.
    ++	EOF
    ++	tail -n4 expect >expect.2 &&
     +	test_config rebase.missingCommitsCheck error &&
     +	rebase_setup_and_clean missing-commit &&
     +	set_fake_editor &&
    -+	test_must_fail env FAKE_LINES="1 2 bad 3 4" \
    -+		git rebase -i --root >/dev/null 2>stderr &&
    -+	test_must_fail env FAKE_LINES="1 2 4" \
    -+		git rebase --edit-todo 2>actual &&
    ++	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 &&
    ++	test_must_fail env FAKE_LINES="2 3 4" \
    ++		git rebase --edit-todo 2>actual.2 &&
    ++	head -n5 actual.2 >actual &&
     +	test_i18ncmp expect actual &&
    -+	test_must_fail git rebase --continue 2>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 &&
     +	test_i18ncmp expect.2 actual &&
    -+	cp .git/rebase-merge/git-rebase-todo.backup \
    -+		.git/rebase-merge/git-rebase-todo &&
    -+	FAKE_LINES="1 2 drop 3 4 drop 5" \
    -+		git rebase --edit-todo &&
    ++	cp orig .git/rebase-merge/git-rebase-todo &&
    ++	FAKE_LINES="1 2 3 4 drop 5" git rebase --edit-todo &&
     +	git rebase --continue 2>actual &&
     +	test D = $(git cat-file commit HEAD | sed -ne \$p) &&
     +	test_i18ngrep \
 2:  7410a1bc4b <  -:  ---------- t3429: demonstrate that rebase exec does not check for dropped commits
 3:  f9ef6d5569 <  -:  ---------- sequencer: update `total_nr' when adding an item to a todo list
 4:  049a92dec0 <  -:  ---------- sequencer: update `done_nr' when skipping commands in a todo list
 5:  f7aae0c763 <  -:  ---------- sequencer: move the code writing total_nr on the disk to a new function
 6:  0e41c4c85e <  -:  ---------- sequencer: add a parameter to sequencer_continue() to accept a todo list
 7:  e6f5589f18 <  -:  ---------- rebase-interactive: todo_list_check() also uses the done list
 8:  69a562b0ab <  -:  ---------- rebase-interactive: warn if commit is dropped with --edit-todo
 9:  4656ab11ae <  -:  ---------- sequencer: have read_populate_todo() check for dropped commits
 -:  ---------- >  2:  a4a700ce8b rebase-interactive: warn if commit is dropped with --edit-todo
-- 
2.24.0.2.ga4a700ce8b

[RFC PATCH v2 1/2] t3404: demonstrate that --edit-todo does not check for dropped commits

[Alban Gruin]

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.

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.

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.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 t/t3404-rebase-interactive.sh | 75 +++++++++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index d2dfbe46b9..be8badf7b3 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1353,6 +1353,81 @@ 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 &&
+	git rebase --continue 2>actual &&
+	test D = $(git cat-file commit HEAD | sed -ne \$p) &&
+	test_i18ngrep \
+		"Successfully rebased and updated refs/heads/missing-commit" \
+		actual
+'
+
+test_expect_failure '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 &&
+	cp orig .git/rebase-merge/git-rebase-todo &&
+	FAKE_LINES="1 2 3 4" git rebase --edit-todo 2>actual.2 &&
+	head -n4 actual.2 >actual &&
+	test_i18ncmp expect.2 actual &&
+	git rebase --continue 2>actual &&
+	test D = $(git cat-file commit HEAD | sed -ne \$p) &&
+	test_i18ngrep \
+		"Successfully rebased and updated refs/heads/missing-commit" \
+		actual
+'
+
+test_expect_failure '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.
+	EOF
+	tail -n4 expect >expect.2 &&
+	test_config rebase.missingCommitsCheck error &&
+	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 &&
+	test_must_fail env FAKE_LINES="2 3 4" \
+		git rebase --edit-todo 2>actual.2 &&
+	head -n5 actual.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 &&
+	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 &&
+	git rebase --continue 2>actual &&
+	test D = $(git cat-file commit HEAD | sed -ne \$p) &&
+	test_i18ngrep \
+		"Successfully rebased and updated refs/heads/missing-commit" \
+		actual
+'
+
 test_expect_success 'respects rebase.abbreviateCommands with fixup, squash and exec' '
 	rebase_setup_and_clean abbrevcmd &&
 	test_commit "first" file1.txt "first line" first &&
-- 
2.24.0.2.ga4a700ce8b

[RFC PATCH v2 2/2] rebase-interactive: warn if commit is dropped with --edit-todo

[Alban Gruin]

This adds the ability for --edit-todo 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.

Tests added previously should work now.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 rebase-interactive.c          | 7 +++----
 sequencer.c                   | 5 +----
 t/t3404-rebase-interactive.sh | 4 ++--
 3 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/rebase-interactive.c b/rebase-interactive.c
index aa18ae82b7..1b8d7f25bf 100644
--- a/rebase-interactive.c
+++ b/rebase-interactive.c
@@ -115,10 +115,9 @@ 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) ||
+	    todo_list_check(todo_list, new_todo))
+		return -4;
 
 	return 0;
 }
diff --git a/sequencer.c b/sequencer.c
index 9d5964fd81..92f2e6f6a0 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -5062,10 +5062,7 @@ 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)) {
+	} else if (res == -4) {
 		fprintf(stderr, _(edit_todo_list_advice));
 		checkout_onto(r, opts, onto_name, &onto->object.oid, orig_head);
 		todo_list_release(&new_todo);
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index be8badf7b3..040ec616f3 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1366,7 +1366,7 @@ test_expect_success 'rebase --edit-todo respects rebase.missingCommitsCheck = ig
 		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.
@@ -1395,7 +1395,7 @@ test_expect_failure 'rebase --edit-todo respects rebase.missingCommitsCheck = wa
 		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.
-- 
2.24.0.2.ga4a700ce8b

Re: [RFC PATCH v2 2/2] rebase-interactive: warn if commit is dropped with --edit-todo

[Phillip Wood]

Hi Alban

Thanks for working on this. I've got a couple of specific comments 
below. On a more general level I'd be tempted to squash the two patches 
together rather than adding the tests as a separate commit. Also I think 
before we discussed having `rebase --continue` refuse to continue if 
`rebase --edit-todo` had dropped commits when rebase.missingCommitsCheck 
was set to error. I think that would be useful but could always come 
later. --edit-todo could write a file to .git/rebase-merge to prevent 
--continue from continuing in case of errors.

On 04/11/2019 09:54, Alban Gruin wrote:
> This adds the ability for --edit-todo 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.
> 
> Tests added previously should work now.
> 
> Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
> ---
>   rebase-interactive.c          | 7 +++----
>   sequencer.c                   | 5 +----
>   t/t3404-rebase-interactive.sh | 4 ++--
>   3 files changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/rebase-interactive.c b/rebase-interactive.c
> index aa18ae82b7..1b8d7f25bf 100644
> --- a/rebase-interactive.c
> +++ b/rebase-interactive.c
> @@ -115,10 +115,9 @@ 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) ||
> +	    todo_list_check(todo_list, new_todo))
> +		return -4;
>   
>   	return 0;
>   }
> diff --git a/sequencer.c b/sequencer.c
> index 9d5964fd81..92f2e6f6a0 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -5062,10 +5062,7 @@ 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)) {
> +	} else if (res == -4) {
>   		fprintf(stderr, _(edit_todo_list_advice));

Do we want to print this advice when `rebase --edit-todo` detects 
dropped commits as well? I'm not clear what error messages are printed 
it that case. If we always want this printed then it could be moved into 
edit_todo_list()

Thanks again for working on this, it's a really useful addition to rebase

Best Wishes

Phillip

>   		checkout_onto(r, opts, onto_name, &onto->object.oid, orig_head);
>   		todo_list_release(&new_todo);
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index be8badf7b3..040ec616f3 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -1366,7 +1366,7 @@ test_expect_success 'rebase --edit-todo respects rebase.missingCommitsCheck = ig
>   		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.
> @@ -1395,7 +1395,7 @@ test_expect_failure 'rebase --edit-todo respects rebase.missingCommitsCheck = wa
>   		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.
> 

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

[Alban Gruin]

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

[PATCH v3 1/2] sequencer: move check_todo_list_from_file() to rebase-interactive.c

[Alban Gruin]

The message contained in `edit_todo_list_advice' (sequencer.c) is
printed after the initial edit of the todo list if it can't be parsed or
if commits were dropped.  This is done either in complete_action() for
`rebase -i', or in check_todo_list_from_file() for `rebase -p'.

Since we want to add this check when editing the list, we also want to
use this message from edit_todo_list() (rebase-interactive.c).  To this
end, check_todo_list_from_file() is moved to rebase-interactive.c, and
`edit_todo_list_advice' is copied there.  In the next commit,
complete_action() will stop using it, and `edit_todo_list_advice' will
be removed from sequencer.c.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 rebase-interactive.c | 35 +++++++++++++++++++++++++++++++++++
 rebase-interactive.h |  2 ++
 sequencer.c          | 29 -----------------------------
 sequencer.h          |  1 -
 4 files changed, 37 insertions(+), 30 deletions(-)

diff --git a/rebase-interactive.c b/rebase-interactive.c
index aa18ae82b7..ad5dd49c31 100644
--- a/rebase-interactive.c
+++ b/rebase-interactive.c
@@ -6,6 +6,12 @@
 #include "commit-slab.h"
 #include "config.h"
 
+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");
+
 enum missing_commit_check_level {
 	MISSING_COMMIT_CHECK_IGNORE = 0,
 	MISSING_COMMIT_CHECK_WARN,
@@ -187,3 +193,32 @@ int todo_list_check(struct todo_list *old_todo, struct todo_list *new_todo)
 	clear_commit_seen(&commit_seen);
 	return res;
 }
+
+int check_todo_list_from_file(struct repository *r)
+{
+	struct todo_list old_todo = TODO_LIST_INIT, new_todo = TODO_LIST_INIT;
+	int res = 0;
+
+	if (strbuf_read_file(&new_todo.buf, rebase_path_todo(), 0) < 0) {
+		res = error(_("could not read '%s'."), rebase_path_todo());
+		goto out;
+	}
+
+	if (strbuf_read_file(&old_todo.buf, rebase_path_todo_backup(), 0) < 0) {
+		res = error(_("could not read '%s'."), rebase_path_todo_backup());
+		goto out;
+	}
+
+	res = todo_list_parse_insn_buffer(r, old_todo.buf.buf, &old_todo);
+	if (!res)
+		res = todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo);
+	if (!res)
+		res = todo_list_check(&old_todo, &new_todo);
+	if (res)
+		fprintf(stderr, _(edit_todo_list_advice));
+out:
+	todo_list_release(&old_todo);
+	todo_list_release(&new_todo);
+
+	return res;
+}
diff --git a/rebase-interactive.h b/rebase-interactive.h
index 44dbb06311..5f41bf5a28 100644
--- a/rebase-interactive.h
+++ b/rebase-interactive.h
@@ -13,4 +13,6 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list,
 		   const char *shortonto, unsigned flags);
 int todo_list_check(struct todo_list *old_todo, struct todo_list *new_todo);
 
+int check_todo_list_from_file(struct repository *r);
+
 #endif
diff --git a/sequencer.c b/sequencer.c
index ec0b793fc5..181bb35f5f 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4992,35 +4992,6 @@ N_("You can fix this with 'git rebase --edit-todo' "
 "Or you can abort the rebase with 'git rebase"
 " --abort'.\n");
 
-int check_todo_list_from_file(struct repository *r)
-{
-	struct todo_list old_todo = TODO_LIST_INIT, new_todo = TODO_LIST_INIT;
-	int res = 0;
-
-	if (strbuf_read_file_or_whine(&new_todo.buf, rebase_path_todo()) < 0) {
-		res = -1;
-		goto out;
-	}
-
-	if (strbuf_read_file_or_whine(&old_todo.buf, rebase_path_todo_backup()) < 0) {
-		res = -1;
-		goto out;
-	}
-
-	res = todo_list_parse_insn_buffer(r, old_todo.buf.buf, &old_todo);
-	if (!res)
-		res = todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo);
-	if (!res)
-		res = todo_list_check(&old_todo, &new_todo);
-	if (res)
-		fprintf(stderr, _(edit_todo_list_advice));
-out:
-	todo_list_release(&old_todo);
-	todo_list_release(&new_todo);
-
-	return res;
-}
-
 /* skip picking commits whose parents are unchanged */
 static int skip_unnecessary_picks(struct repository *r,
 				  struct todo_list *todo_list,
diff --git a/sequencer.h b/sequencer.h
index 574260f621..75ddc5db3a 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -155,7 +155,6 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
 
 void todo_list_add_exec_commands(struct todo_list *todo_list,
 				 struct string_list *commands);
-int check_todo_list_from_file(struct repository *r);
 int complete_action(struct repository *r, struct replay_opts *opts, unsigned flags,
 		    const char *shortrevisions, const char *onto_name,
 		    struct commit *onto, const char *orig_head, struct string_list *commands,
-- 
2.24.0

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

[Alban Gruin]

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

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();
 	unsigned initial = shortrevisions && shortonto;
 
 	/* 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);
 
 	if (launch_sequence_editor(todo_file, &new_todo->buf, NULL))
 		return -2;
@@ -121,10 +122,13 @@ 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;
 }
@@ -189,6 +193,8 @@ 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;
diff --git a/sequencer.c b/sequencer.c
index 181bb35f5f..75d5ad0496 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4271,8 +4271,20 @@ 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)))
@@ -4986,12 +4998,6 @@ 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,
@@ -5089,11 +5095,7 @@ 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);
 
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 &&
+	git rebase --continue 2>actual &&
+	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 &&
+	cp orig .git/rebase-merge/git-rebase-todo &&
+	FAKE_LINES="1 2 3 4" git rebase --edit-todo 2>actual.2 &&
+	head -n4 actual.2 >actual &&
+	test_i18ncmp expect.2 actual &&
+	git rebase --continue 2>actual &&
+	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 = 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 -n10 expect >expect.2 &&
+	test_config rebase.missingCommitsCheck error &&
+	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 &&
+	test_must_fail env FAKE_LINES="2 3 4" \
+		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 &&
+	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 &&
+	git rebase --continue 2>actual &&
+	test D = $(git cat-file commit HEAD | sed -ne \$p) &&
+	test_i18ngrep \
+		"Successfully rebased and updated refs/heads/missing-commit" \
+		actual
+'
+
 test_expect_success 'respects rebase.abbreviateCommands with fixup, squash and exec' '
 	rebase_setup_and_clean abbrevcmd &&
 	test_commit "first" file1.txt "first line" first &&
-- 
2.24.0

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

[Junio C Hamano]

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.

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 &&
> +	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...

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

[Junio C Hamano]

Alban Gruin <alban.gruin@gmail.com> writes:

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

This passes the self-test when tested by itself, but when merged
near the tip of 'pu', it breaks t3404.116, it seems.

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

[Alban Gruin]

Hi Junio,

Le 04/12/2019 à 22:51, Junio C Hamano a écrit :
> Alban Gruin <alban.gruin@gmail.com> writes:
> 
>> 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.
>> ...
>>  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(-)
> 
> This passes the self-test when tested by itself, but when merged
> near the tip of 'pu', it breaks t3404.116, it seems.
> 

After a quick investigation, it comes from
pw/post-commit-from-sequencer.  Since then, tests are expected to setup
the editor and run the commands using it in a subshell.  So the fix is
straightforward.

Perhaps I should take ag/sequencer-todo-updates, merge
pw/post-commit-from-sequencer, rebase this series onto the result, fix
the issue, and reroll the series?

Cheers,
Alban

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

[Phillip Wood]

Hi Alban

On 05/12/2019 23:15, Alban Gruin wrote:
> Hi Junio,
> 
> Le 04/12/2019 à 22:51, Junio C Hamano a écrit :
>> Alban Gruin <alban.gruin@gmail.com> writes:
>>
>>> 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.
>>> ...
>>>   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(-)
>>
>> This passes the self-test when tested by itself, but when merged
>> near the tip of 'pu', it breaks t3404.116, it seems.
>>
> 
> After a quick investigation, it comes from
> pw/post-commit-from-sequencer.  Since then, tests are expected to setup
> the editor and run the commands using it in a subshell.  So the fix is
> straightforward.
> 
> Perhaps I should take ag/sequencer-todo-updates, merge
> pw/post-commit-from-sequencer, rebase this series onto the result, fix
> the issue, and reroll the series?

If the issue is just using a subshell to set the editor then (assuming 
you're only adding new tests) I don't think you need to rebase - just 
change your tests and it should be fine when Junio merges it into pu. 
I'm sorry I've not looked at the latest version yet, I'll try and get 
round to it next week.

Best Wishes

Phillip
> Cheers,
> Alban
> 

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 6607 bytes --]

Hi Alban,

On Fri, 6 Dec 2019, Phillip Wood wrote:

> On 05/12/2019 23:15, Alban Gruin wrote:
> >
> > Le 04/12/2019 à 22:51, Junio C Hamano a écrit :
> > > Alban Gruin <alban.gruin@gmail.com> writes:
> > >
> > > > 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.
> > > > ...
> > > >   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(-)
> > >
> > > This passes the self-test when tested by itself, but when merged
> > > near the tip of 'pu', it breaks t3404.116, it seems.
> > >
> >
> > After a quick investigation, it comes from
> > pw/post-commit-from-sequencer.  Since then, tests are expected to setup
> > the editor and run the commands using it in a subshell.  So the fix is
> > straightforward.
> >
> > Perhaps I should take ag/sequencer-todo-updates, merge
> > pw/post-commit-from-sequencer, rebase this series onto the result, fix
> > the issue, and reroll the series?
>
> If the issue is just using a subshell to set the editor then (assuming you're
> only adding new tests) I don't think you need to rebase - just change your
> tests and it should be fine when Junio merges it into pu. I'm sorry I've not
> looked at the latest version yet, I'll try and get round to it next week.

Just squash this in:

-- snipsnap --
Subject: [PATCH] fixup??? rebase-interactive: warn if commit is dropped with
 `rebase --edit-todo'

This is required to appease the interaction with
`pw/post-commit-from-sequencer`.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t3404-rebase-interactive.sh | 74 +++++++++++++++++++----------------
 1 file changed, 40 insertions(+), 34 deletions(-)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index e3f64bc2a59..72718d0d839 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1453,10 +1453,12 @@ test_expect_success 'rebase -i respects rebase.missingCommitsCheck = error' '
 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 &&
-	git rebase --continue 2>actual &&
+	(
+		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 &&
+		git rebase --continue 2>actual
+	) &&
 	test D = $(git cat-file commit HEAD | sed -ne \$p) &&
 	test_i18ngrep \
 		"Successfully rebased and updated refs/heads/missing-commit" \
@@ -1474,18 +1476,20 @@ test_expect_success 'rebase --edit-todo respects rebase.missingCommitsCheck = wa
 	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 &&
-	cp orig .git/rebase-merge/git-rebase-todo &&
-	FAKE_LINES="1 2 3 4" git rebase --edit-todo 2>actual.2 &&
-	head -n4 actual.2 >actual &&
-	test_i18ncmp expect.2 actual &&
-	git rebase --continue 2>actual &&
+	(
+		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 &&
+		cp orig .git/rebase-merge/git-rebase-todo &&
+		FAKE_LINES="1 2 3 4" git rebase --edit-todo 2>actual.2 &&
+		head -n4 actual.2 >actual &&
+		test_i18ncmp expect.2 actual &&
+		git rebase --continue 2>actual
+	) &&
 	test D = $(git cat-file commit HEAD | sed -ne \$p) &&
 	test_i18ngrep \
 		"Successfully rebased and updated refs/heads/missing-commit" \
@@ -1509,24 +1513,26 @@ test_expect_success 'rebase --edit-todo respects rebase.missingCommitsCheck = er
 	tail -n10 expect >expect.2 &&
 	test_config rebase.missingCommitsCheck error &&
 	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 &&
-	test_must_fail env FAKE_LINES="2 3 4" \
-		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 &&
-	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 &&
-	git rebase --continue 2>actual &&
+	(
+		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 &&
+		test_must_fail env FAKE_LINES="2 3 4" \
+			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 &&
+		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 &&
+		git rebase --continue 2>actual
+	) &&
 	test D = $(git cat-file commit HEAD | sed -ne \$p) &&
 	test_i18ngrep \
 		"Successfully rebased and updated refs/heads/missing-commit" \
--
2.24.0.windows.2.611.ge9aced84530

Re: [PATCH v3 1/2] sequencer: move check_todo_list_from_file() to rebase-interactive.c

[Johannes Schindelin]

Hi Alban,

On Tue, 3 Dec 2019, Alban Gruin wrote:

> The message contained in `edit_todo_list_advice' (sequencer.c) is
> printed after the initial edit of the todo list if it can't be parsed or
> if commits were dropped.  This is done either in complete_action() for
> `rebase -i', or in check_todo_list_from_file() for `rebase -p'.
>
> Since we want to add this check when editing the list, we also want to
> use this message from edit_todo_list() (rebase-interactive.c).  To this
> end, check_todo_list_from_file() is moved to rebase-interactive.c, and
> `edit_todo_list_advice' is copied there.  In the next commit,
> complete_action() will stop using it, and `edit_todo_list_advice' will
> be removed from sequencer.c.

Makes sense to me.

> diff --git a/rebase-interactive.c b/rebase-interactive.c
> index aa18ae82b7..ad5dd49c31 100644
> --- a/rebase-interactive.c
> +++ b/rebase-interactive.c
> @@ -187,3 +193,32 @@ int todo_list_check(struct todo_list *old_todo, struct todo_list *new_todo)
>  	clear_commit_seen(&commit_seen);
>  	return res;
>  }
> +
> +int check_todo_list_from_file(struct repository *r)
> +{
> +	struct todo_list old_todo = TODO_LIST_INIT, new_todo = TODO_LIST_INIT;
> +	int res = 0;
> +
> +	if (strbuf_read_file(&new_todo.buf, rebase_path_todo(), 0) < 0) {
> +		res = error(_("could not read '%s'."), rebase_path_todo());
> +		goto out;
> +	}
> +
> +	if (strbuf_read_file(&old_todo.buf, rebase_path_todo_backup(), 0) < 0) {
> +		res = error(_("could not read '%s'."), rebase_path_todo_backup());
> +		goto out;
> +	}
> +
> +	res = todo_list_parse_insn_buffer(r, old_todo.buf.buf, &old_todo);
> +	if (!res)
> +		res = todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo);
> +	if (!res)
> +		res = todo_list_check(&old_todo, &new_todo);
> +	if (res)
> +		fprintf(stderr, _(edit_todo_list_advice));
> +out:
> +	todo_list_release(&old_todo);
> +	todo_list_release(&new_todo);
> +
> +	return res;
> +}

No need to address the following concern in this patch series, but I do
think that a #leftoverbits project could be to simplify this to

	if (strbuf_read_file(&new_todo.buf, rebase_path_todo(), 0) < 0)
		res = error(_("could not read '%s'."), rebase_path_todo());
	else if (strbuf_read_file(&old_todo.buf, rebase_path_todo_backup(), 0) < 0)
		res = error(_("could not read '%s'."), rebase_path_todo_backup());
	else if ((res = todo_list_parse_insn_buffer(r, old_todo.buf.buf, &old_todo)) ||
		 (res = todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo)) ||
		 (res = todo_list_check(&old_todo, &new_todo)))
		fprintf(stderr, _(edit_todo_list_advice));

Ciao,
Dscho

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

[Phillip Wood]

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

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

[Phillip Wood]

Hi Alban

On 02/12/2019 23:47, Alban Gruin wrote:
 > [...]
> diff --git a/sequencer.c b/sequencer.c
> index 181bb35f5f..75d5ad0496 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -4271,8 +4271,20 @@ 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);

This causes problems if the user edits the todo list and then later runs 
'git rebase --continue' after resolving conflicts as the backup todo 
list has a bunch of commands that have already been processed but they 
are seen as missing

This test fails

test_expect_success 'rebase.missingCommitsCheck = error after resolving 
conflicts' '
	test_config rebase.missingCommitsCheck error &&
	(
		set_fake_editor &&
		FAKE_LINES="break 2 3 4" git rebase -i A E
	) &&
	git rebase --edit-todo &&
	test_must_fail git rebase --continue &&
	echo x >file1 &&
	git add file1 &&
	git rebase --continue
'

I think it would be better to write a file if the check fails when 
editing the todo list and check for the presence of that file when 
continuing and error out if it exists. This would also allow --edit-todo 
to only remove the backup file if there are no outstanding errors from 
the last edit and so check that those errors are fixed by the second 
edit. I think we'd only want to do this if rebase.missingCommitsCheck is 
set to error.

Best Wishes

Phillip

> +
> +			if (res)
> +				goto release_todo_list;
> +		}
> +
>   		if (commit_staged_changes(r, opts, &todo_list))
>   			return -1;
>   	} else if (!file_exists(get_todo_path(opts)))
> @@ -4986,12 +4998,6 @@ 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,
> @@ -5089,11 +5095,7 @@ 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);
>   
> 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 &&
> +	git rebase --continue 2>actual &&
> +	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 &&
> +	cp orig .git/rebase-merge/git-rebase-todo &&
> +	FAKE_LINES="1 2 3 4" git rebase --edit-todo 2>actual.2 &&
> +	head -n4 actual.2 >actual &&
> +	test_i18ncmp expect.2 actual &&
> +	git rebase --continue 2>actual &&
> +	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 = 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 -n10 expect >expect.2 &&
> +	test_config rebase.missingCommitsCheck error &&
> +	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 &&
> +	test_must_fail env FAKE_LINES="2 3 4" \
> +		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 &&
> +	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 &&
> +	git rebase --continue 2>actual &&
> +	test D = $(git cat-file commit HEAD | sed -ne \$p) &&
> +	test_i18ngrep \
> +		"Successfully rebased and updated refs/heads/missing-commit" \
> +		actual
> +'
> +
>   test_expect_success 'respects rebase.abbreviateCommands with fixup, squash and exec' '
>   	rebase_setup_and_clean abbrevcmd &&
>   	test_commit "first" file1.txt "first line" first &&
> 

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

[Alban Gruin]

Hi Phillip,

Le 09/12/2019 à 17:00, Phillip Wood a écrit :
>>> 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 &&
> 
> 

In which case, if the check did not pass at the previous edit, the new
todo list should be compared to the backup.  As sequencer_continue()
already does this, extract this to its own function in
rebase-interactive.c.  To keep track of this, a file is created on the
disk (as you suggested in your other email.)  At the next edit, if this
file exists and no errors were found, it is deleted.  The backup is only
created if there is no errors in `todo_list' and in `new_todo'.

This would guarantee that there is no errors in the backup, and that the
edited list is always compared to a list exempt of errors.

This approach also has the benefit to detect if a commit part of a
badcmd was dropped.

After some tweaks (ie. `expect' now lists 2 commits instead of one),
this passes the test with the change you suggested, and the one you sent
in your other email.


>>
>> 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
> 

Good catch.

>>> +    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.
> 

No, it was here to show that we can detect dropped commits, even if the
todo list has an error.

> Best Wishes
> 
> Phillip

Cheers,
Alban

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

[Phillip Wood]

Hi Alban

On 09/01/2020 21:13, Alban Gruin wrote:
> Hi Phillip,
> 
> Le 09/12/2019 à 17:00, Phillip Wood a écrit :
>>>> 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 &&
>>
>>
> 
> In which case, if the check did not pass at the previous edit, the new
> todo list should be compared to the backup.  As sequencer_continue()
> already does this, extract this to its own function in
> rebase-interactive.c.  To keep track of this, a file is created on the
> disk (as you suggested in your other email.)  At the next edit, if this
> file exists and no errors were found, it is deleted.  The backup is only
> created if there is no errors in `todo_list' and in `new_todo'.
> 
> This would guarantee that there is no errors in the backup, and that the
> edited list is always compared to a list exempt of errors.
> 
> This approach also has the benefit to detect if a commit part of a
> badcmd was dropped.
> 
> After some tweaks (ie. `expect' now lists 2 commits instead of one),
> this passes the test with the change you suggested, and the one you sent
> in your other email.

That sounds good. I'm not sure how it passes the test in my other email 
though, if sequencer_continue() compares the todo list to the backup 
wont it still fail when continuing after conflicts as the backup is out 
of date?

Best Wishes

Phillip


>>>
>>> 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
>>
> 
> Good catch.
> 
>>>> +    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.
>>
> 
> No, it was here to show that we can detect dropped commits, even if the
> todo list has an error.
> 
>> Best Wishes
>>
>> Phillip
> 
> Cheers,
> Alban
> 

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

[Alban Gruin]

Hi Phillip,

Le 10/01/2020 à 18:13, Phillip Wood a écrit :
> Hi Alban
> 
> On 09/01/2020 21:13, Alban Gruin wrote:
>> Hi Phillip,
>>
>> Le 09/12/2019 à 17:00, Phillip Wood a écrit :
>>>>> 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 &&
>>>
>>>
>>
>> In which case, if the check did not pass at the previous edit, the new
>> todo list should be compared to the backup.  As sequencer_continue()
>> already does this, extract this to its own function in
>> rebase-interactive.c.  To keep track of this, a file is created on the
>> disk (as you suggested in your other email.)  At the next edit, if this
>> file exists and no errors were found, it is deleted.  The backup is only
>> created if there is no errors in `todo_list' and in `new_todo'.
>>
>> This would guarantee that there is no errors in the backup, and that the
>> edited list is always compared to a list exempt of errors.
>>
>> This approach also has the benefit to detect if a commit part of a
>> badcmd was dropped.
>>
>> After some tweaks (ie. `expect' now lists 2 commits instead of one),
>> this passes the test with the change you suggested, and the one you sent
>> in your other email.
> 
> That sounds good. I'm not sure how it passes the test in my other email
> though, if sequencer_continue() compares the todo list to the backup
> wont it still fail when continuing after conflicts as the backup is out
> of date?
> 

I changed sequencer_continue() to check the todo list only if the file
indicating an error exists.

I still have to rewrite the commit message, then I’ll re-send this series.

Cheers,
Alban

> Best Wishes
> 
> Phillip
> 

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

[Phillip Wood]

Hi Alban

On 10/01/2020 21:31, Alban Gruin wrote:
> Hi Phillip,
> 
> Le 10/01/2020 à 18:13, Phillip Wood a écrit :
>> Hi Alban
>>
>> On 09/01/2020 21:13, Alban Gruin wrote:
>>> Hi Phillip,
>>> [...]
>>>
>>> In which case, if the check did not pass at the previous edit, the new
>>> todo list should be compared to the backup.  As sequencer_continue()
>>> already does this, extract this to its own function in
>>> rebase-interactive.c.  To keep track of this, a file is created on the
>>> disk (as you suggested in your other email.)  At the next edit, if this
>>> file exists and no errors were found, it is deleted.  The backup is only
>>> created if there is no errors in `todo_list' and in `new_todo'.
>>>
>>> This would guarantee that there is no errors in the backup, and that the
>>> edited list is always compared to a list exempt of errors.
>>>
>>> This approach also has the benefit to detect if a commit part of a
>>> badcmd was dropped.
>>>
>>> After some tweaks (ie. `expect' now lists 2 commits instead of one),
>>> this passes the test with the change you suggested, and the one you sent
>>> in your other email.
>>
>> That sounds good. I'm not sure how it passes the test in my other email
>> though, if sequencer_continue() compares the todo list to the backup
>> wont it still fail when continuing after conflicts as the backup is out
>> of date?
>>
> 
> I changed sequencer_continue() to check the todo list only if the file
> indicating an error exists.

That makes sense

> I still have to rewrite the commit message, then I’ll re-send this series.

Excellent, I look forward to reading them

Best Wishes

Phillip

> Cheers,
> Alban
> 
>> Best Wishes
>>
>> Phillip
>>

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

[Alban Gruin]

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-v4" at
https://github.com/agrn/git.

Changes since v3:

 - Set the fake editor in a subshell in the tests to avoid conflicts with
   pw/post-commit-from-sequencer.

 - Added the `dropped' file to indicate that the todo list is incorrect
   (ie. no invalid commands or dropped commits.)

 - The todo list is copied only if it is correct.

 - sequencer_continue() checks the todo list for dropped commits only
   when `dropped' exists.

 - Added a new test to check that `rebase --continue' does not warn for
   dropped commits after solving a conflict.  This was provided by
   Phillip Wood.

 - check_todo_list_from_file() no longer prints `edit_todo_list_advice'
   if there is dropped commits since todo_list_check() do it now.

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          |  89 ++++++++++++++++++++++++---
 rebase-interactive.h          |   5 ++
 sequencer.c                   |  51 ++++------------
 sequencer.h                   |   2 +-
 t/t3404-rebase-interactive.sh | 109 ++++++++++++++++++++++++++++++++++
 5 files changed, 206 insertions(+), 50 deletions(-)

Diff-intervalle contre v3:
1:  996045a300 = 1:  996045a300 sequencer: move check_todo_list_from_file() to rebase-interactive.c
2:  ba6d27e5b4 < -:  ---------- rebase-interactive: warn if commit is dropped with `rebase --edit-todo'
-:  ---------- > 2:  11b0e1e78c rebase-interactive: warn if commit is dropped with `rebase --edit-todo'
-- 
2.24.1

[PATCH v4 1/2] sequencer: move check_todo_list_from_file() to rebase-interactive.c

[Alban Gruin]

The message contained in `edit_todo_list_advice' (sequencer.c) is
printed after the initial edit of the todo list if it can't be parsed or
if commits were dropped.  This is done either in complete_action() for
`rebase -i', or in check_todo_list_from_file() for `rebase -p'.

Since we want to add this check when editing the list, we also want to
use this message from edit_todo_list() (rebase-interactive.c).  To this
end, check_todo_list_from_file() is moved to rebase-interactive.c, and
`edit_todo_list_advice' is copied there.  In the next commit,
complete_action() will stop using it, and `edit_todo_list_advice' will
be removed from sequencer.c.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 rebase-interactive.c | 35 +++++++++++++++++++++++++++++++++++
 rebase-interactive.h |  2 ++
 sequencer.c          | 29 -----------------------------
 sequencer.h          |  1 -
 4 files changed, 37 insertions(+), 30 deletions(-)

diff --git a/rebase-interactive.c b/rebase-interactive.c
index aa18ae82b7..ad5dd49c31 100644
--- a/rebase-interactive.c
+++ b/rebase-interactive.c
@@ -6,6 +6,12 @@
 #include "commit-slab.h"
 #include "config.h"
 
+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");
+
 enum missing_commit_check_level {
 	MISSING_COMMIT_CHECK_IGNORE = 0,
 	MISSING_COMMIT_CHECK_WARN,
@@ -187,3 +193,32 @@ int todo_list_check(struct todo_list *old_todo, struct todo_list *new_todo)
 	clear_commit_seen(&commit_seen);
 	return res;
 }
+
+int check_todo_list_from_file(struct repository *r)
+{
+	struct todo_list old_todo = TODO_LIST_INIT, new_todo = TODO_LIST_INIT;
+	int res = 0;
+
+	if (strbuf_read_file(&new_todo.buf, rebase_path_todo(), 0) < 0) {
+		res = error(_("could not read '%s'."), rebase_path_todo());
+		goto out;
+	}
+
+	if (strbuf_read_file(&old_todo.buf, rebase_path_todo_backup(), 0) < 0) {
+		res = error(_("could not read '%s'."), rebase_path_todo_backup());
+		goto out;
+	}
+
+	res = todo_list_parse_insn_buffer(r, old_todo.buf.buf, &old_todo);
+	if (!res)
+		res = todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo);
+	if (!res)
+		res = todo_list_check(&old_todo, &new_todo);
+	if (res)
+		fprintf(stderr, _(edit_todo_list_advice));
+out:
+	todo_list_release(&old_todo);
+	todo_list_release(&new_todo);
+
+	return res;
+}
diff --git a/rebase-interactive.h b/rebase-interactive.h
index 44dbb06311..5f41bf5a28 100644
--- a/rebase-interactive.h
+++ b/rebase-interactive.h
@@ -13,4 +13,6 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list,
 		   const char *shortonto, unsigned flags);
 int todo_list_check(struct todo_list *old_todo, struct todo_list *new_todo);
 
+int check_todo_list_from_file(struct repository *r);
+
 #endif
diff --git a/sequencer.c b/sequencer.c
index ec0b793fc5..181bb35f5f 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4992,35 +4992,6 @@ N_("You can fix this with 'git rebase --edit-todo' "
 "Or you can abort the rebase with 'git rebase"
 " --abort'.\n");
 
-int check_todo_list_from_file(struct repository *r)
-{
-	struct todo_list old_todo = TODO_LIST_INIT, new_todo = TODO_LIST_INIT;
-	int res = 0;
-
-	if (strbuf_read_file_or_whine(&new_todo.buf, rebase_path_todo()) < 0) {
-		res = -1;
-		goto out;
-	}
-
-	if (strbuf_read_file_or_whine(&old_todo.buf, rebase_path_todo_backup()) < 0) {
-		res = -1;
-		goto out;
-	}
-
-	res = todo_list_parse_insn_buffer(r, old_todo.buf.buf, &old_todo);
-	if (!res)
-		res = todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo);
-	if (!res)
-		res = todo_list_check(&old_todo, &new_todo);
-	if (res)
-		fprintf(stderr, _(edit_todo_list_advice));
-out:
-	todo_list_release(&old_todo);
-	todo_list_release(&new_todo);
-
-	return res;
-}
-
 /* skip picking commits whose parents are unchanged */
 static int skip_unnecessary_picks(struct repository *r,
 				  struct todo_list *todo_list,
diff --git a/sequencer.h b/sequencer.h
index 574260f621..75ddc5db3a 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -155,7 +155,6 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
 
 void todo_list_add_exec_commands(struct todo_list *todo_list,
 				 struct string_list *commands);
-int check_todo_list_from_file(struct repository *r);
 int complete_action(struct repository *r, struct replay_opts *opts, unsigned flags,
 		    const char *shortrevisions, const char *onto_name,
 		    struct commit *onto, const char *orig_head, struct string_list *commands,
-- 
2.24.1

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

[Alban Gruin]

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

If the edited list has an error, a file, `dropped', is created to
report the issue.  Otherwise, it is deleted.  Usually, the edited list
is compared against the list before editing, but if this file exists,
it will be compared to the backup.  Also, if the file exists,
sequencer_continue() checks the list for dropped commits.  If the
check was performed every time, it would fail when resuming a rebase
after resolving a conflict, as the backup will contain commits that
were picked, but they will not be in the new list.  It's safe to
ignore this check if `dropped' does not exist, because that means that
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
`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.  The fourth test checks that no errors
are raised when resuming a rebase after resolving a conflict.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---

Notes:
    I don't think the way I create `expect.3' files in "rebase --edit-todo
    respects rebase.missingCommitsCheck = warning" & "... = error" is the
    best practice.  Perhaps I should create a new file from scratch instead
    of calling `head' and `tail' successively?

 rebase-interactive.c          |  58 ++++++++++++++----
 rebase-interactive.h          |   3 +
 sequencer.c                   |  22 +++----
 sequencer.h                   |   1 +
 t/t3404-rebase-interactive.sh | 109 ++++++++++++++++++++++++++++++++++
 5 files changed, 171 insertions(+), 22 deletions(-)

diff --git a/rebase-interactive.c b/rebase-interactive.c
index ad5dd49c31..36b08a55ef 100644
--- a/rebase-interactive.c
+++ b/rebase-interactive.c
@@ -5,6 +5,7 @@
 #include "strbuf.h"
 #include "commit-slab.h"
 #include "config.h"
+#include "dir.h"
 
 static const char edit_todo_list_advice[] =
 N_("You can fix this with 'git rebase --edit-todo' "
@@ -97,22 +98,25 @@ 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;
+	int incorrect = 1;
 
 	/* 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());
 
 	if (todo_list_write_to_file(r, todo_list, todo_file, shortrevisions, shortonto,
 				    -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());
+	if (initial && 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;
@@ -121,10 +125,26 @@ 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 (incorrect) {
+		if (todo_list_check_against_backup(r, new_todo)) {
+			write_file(rebase_path_dropped(), "");
+			return -4;
+		}
+
+		if (incorrect > 0)
+			unlink(rebase_path_dropped());
+	} 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;
 }
@@ -189,11 +209,27 @@ 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;
 }
 
+int todo_list_check_against_backup(struct repository *r, struct todo_list *todo_list)
+{
+	struct todo_list backup = TODO_LIST_INIT;
+	int res = 0;
+
+	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);
+	return res;
+}
+
 int check_todo_list_from_file(struct repository *r)
 {
 	struct todo_list old_todo = TODO_LIST_INIT, new_todo = TODO_LIST_INIT;
@@ -212,10 +248,10 @@ int check_todo_list_from_file(struct repository *r)
 	res = todo_list_parse_insn_buffer(r, old_todo.buf.buf, &old_todo);
 	if (!res)
 		res = todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo);
-	if (!res)
-		res = todo_list_check(&old_todo, &new_todo);
 	if (res)
 		fprintf(stderr, _(edit_todo_list_advice));
+	if (!res)
+		res = todo_list_check(&old_todo, &new_todo);
 out:
 	todo_list_release(&old_todo);
 	todo_list_release(&new_todo);
diff --git a/rebase-interactive.h b/rebase-interactive.h
index 5f41bf5a28..4af0c1fcc7 100644
--- a/rebase-interactive.h
+++ b/rebase-interactive.h
@@ -11,7 +11,10 @@ void append_todo_help(unsigned keep_empty, int command_count,
 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);
+
 int todo_list_check(struct todo_list *old_todo, struct todo_list *new_todo);
+int todo_list_check_against_backup(struct repository *r,
+				   struct todo_list *todo_list);
 
 int check_todo_list_from_file(struct repository *r);
 
diff --git a/sequencer.c b/sequencer.c
index 181bb35f5f..2ff18943fb 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -57,6 +57,8 @@ static GIT_PATH_FUNC(rebase_path, "rebase-merge")
 GIT_PATH_FUNC(rebase_path_todo, "rebase-merge/git-rebase-todo")
 GIT_PATH_FUNC(rebase_path_todo_backup, "rebase-merge/git-rebase-todo.backup")
 
+GIT_PATH_FUNC(rebase_path_dropped, "rebase-merge/dropped")
+
 /*
  * The rebase command lines that have already been processed. A line
  * is moved here when it is first handled, before any associated user
@@ -4273,6 +4275,14 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)
 	if (is_rebase_i(opts)) {
 		if ((res = read_populate_todo(r, &todo_list, opts)))
 			goto release_todo_list;
+
+		if (file_exists(rebase_path_dropped())) {
+			if ((res = todo_list_check_against_backup(r, &todo_list)))
+				goto release_todo_list;
+
+			unlink(rebase_path_dropped());
+		}
+
 		if (commit_staged_changes(r, opts, &todo_list))
 			return -1;
 	} else if (!file_exists(get_todo_path(opts)))
@@ -4986,12 +4996,6 @@ 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,
@@ -5089,11 +5093,7 @@ 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);
 
diff --git a/sequencer.h b/sequencer.h
index 75ddc5db3a..00debf5107 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -11,6 +11,7 @@ const char *git_path_commit_editmsg(void);
 const char *git_path_seq_dir(void);
 const char *rebase_path_todo(void);
 const char *rebase_path_todo_backup(void);
+const char *rebase_path_dropped(void);
 
 #define APPEND_SIGNOFF_DEDUP (1u << 0)
 
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 29a35840ed..f5c3da33bf 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1343,6 +1343,115 @@ 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 &&
+		git rebase --continue 2>actual
+	) &&
+	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)
+	 - $(git rev-list --pretty=oneline --abbrev-commit -1 master~4)
+	To avoid this message, use "drop" to explicitly remove a commit.
+	EOF
+	head -n4 expect >expect.2 &&
+	tail -n1 expect >>expect.2 &&
+	tail -n4 expect.2 >expect.3 &&
+	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 -n6 actual.2 >actual &&
+		test_i18ncmp expect actual &&
+		cp orig .git/rebase-merge/git-rebase-todo &&
+		FAKE_LINES="1 2 3 4" git rebase --edit-todo 2>actual.2 &&
+		head -n4 actual.2 >actual &&
+		test_i18ncmp expect.3 actual &&
+		git rebase --continue 2>actual
+	) &&
+	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 = 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)
+	 - $(git rev-list --pretty=oneline --abbrev-commit -1 master~4)
+	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 -n11 expect >expect.2 &&
+	head -n3 expect.2 >expect.3 &&
+	tail -n7 expect.2 >>expect.3 &&
+	test_config rebase.missingCommitsCheck error &&
+	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 &&
+		test_must_fail env FAKE_LINES="2 3 4" \
+			git rebase --edit-todo 2>actual &&
+		test_i18ncmp expect actual &&
+		test_must_fail git rebase --continue 2>actual &&
+		test_i18ncmp expect.2 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 &&
+		test_i18ncmp expect.3 actual &&
+		test_must_fail git rebase --continue 2>actual &&
+		test_i18ncmp expect.3 actual &&
+		cp orig .git/rebase-merge/git-rebase-todo &&
+		FAKE_LINES="1 2 3 4 drop 5" git rebase --edit-todo &&
+		git rebase --continue 2>actual
+	) &&
+	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.missingCommitsCheck = error after resolving conflicts' '
+	test_config rebase.missingCommitsCheck error &&
+	(
+		set_fake_editor &&
+		FAKE_LINES="drop 1 break 2 3 4" git rebase -i A E
+	) &&
+	git rebase --edit-todo &&
+	test_must_fail git rebase --continue &&
+	echo x >file1 &&
+	git add file1 &&
+	git rebase --continue
+'
+
 test_expect_success 'respects rebase.abbreviateCommands with fixup, squash and exec' '
 	rebase_setup_and_clean abbrevcmd &&
 	test_commit "first" file1.txt "first line" first &&
-- 
2.24.1

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

[Phillip Wood]

Hi Alban

On 11/01/2020 17:39, Alban Gruin wrote:
> 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 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
> 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
> 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.
> 
> If the edited list has an error, a file, `dropped', is created to
> report the issue.  Otherwise, it is deleted.  Usually, the edited list
> is compared against the list before editing, but if this file exists,
> it will be compared to the backup.  Also, if the file exists,
> sequencer_continue() checks the list for dropped commits.  If the
> check was performed every time, it would fail when resuming a rebase
> after resolving a conflict, as the backup will contain commits that
> were picked, but they will not be in the new list.  It's safe to
> ignore this check if `dropped' does not exist, because that means that
> 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
> `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.  The fourth test checks that no errors
> are raised when resuming a rebase after resolving a conflict.
> 
> Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
> ---
> 
> Notes:
>      I don't think the way I create `expect.3' files in "rebase --edit-todo
>      respects rebase.missingCommitsCheck = warning" & "... = error" is the
>      best practice.  Perhaps I should create a new file from scratch instead
>      of calling `head' and `tail' successively?
> 
>   rebase-interactive.c          |  58 ++++++++++++++----
>   rebase-interactive.h          |   3 +
>   sequencer.c                   |  22 +++----
>   sequencer.h                   |   1 +
>   t/t3404-rebase-interactive.sh | 109 ++++++++++++++++++++++++++++++++++
>   5 files changed, 171 insertions(+), 22 deletions(-)
> 
> diff --git a/rebase-interactive.c b/rebase-interactive.c
> index ad5dd49c31..36b08a55ef 100644
> --- a/rebase-interactive.c
> +++ b/rebase-interactive.c
> @@ -5,6 +5,7 @@
>   #include "strbuf.h"
>   #include "commit-slab.h"
>   #include "config.h"
> +#include "dir.h"
>   
>   static const char edit_todo_list_advice[] =
>   N_("You can fix this with 'git rebase --edit-todo' "
> @@ -97,22 +98,25 @@ 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;
> +	int incorrect = 1;
>   
>   	/* 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());
>   
>   	if (todo_list_write_to_file(r, todo_list, todo_file, shortrevisions, shortonto,
>   				    -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());
> +	if (initial && 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;
> @@ -121,10 +125,26 @@ 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 (incorrect) {
> +		if (todo_list_check_against_backup(r, new_todo)) {
> +			write_file(rebase_path_dropped(), "");
> +			return -4;
> +		}
> +
> +		if (incorrect > 0)
> +			unlink(rebase_path_dropped());
> +	} else if (todo_list_check(todo_list, new_todo)) {
> +		write_file(rebase_path_dropped(), "");

We also need to create a backup from todo_list otherwise we'll be using
a stale backup file for the in the clause above when the user edits the
todo list to fix it. This test fails

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
	)
'
The second edit complains that commit B is missing even though we have
already pick it. This is because we're using a stale backup file.

Sorry to be the bearer of bad tidings

Phillip

> +		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;
>   }
> @@ -189,11 +209,27 @@ 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;
>   }
>   
> +int todo_list_check_against_backup(struct repository *r, struct todo_list *todo_list)
> +{
> +	struct todo_list backup = TODO_LIST_INIT;
> +	int res = 0;
> +
> +	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);
> +	return res;
> +}
> +
>   int check_todo_list_from_file(struct repository *r)
>   {
>   	struct todo_list old_todo = TODO_LIST_INIT, new_todo = TODO_LIST_INIT;
> @@ -212,10 +248,10 @@ int check_todo_list_from_file(struct repository *r)
>   	res = todo_list_parse_insn_buffer(r, old_todo.buf.buf, &old_todo);
>   	if (!res)
>   		res = todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo);
> -	if (!res)
> -		res = todo_list_check(&old_todo, &new_todo);
>   	if (res)
>   		fprintf(stderr, _(edit_todo_list_advice));
> +	if (!res)
> +		res = todo_list_check(&old_todo, &new_todo);
>   out:
>   	todo_list_release(&old_todo);
>   	todo_list_release(&new_todo);
> diff --git a/rebase-interactive.h b/rebase-interactive.h
> index 5f41bf5a28..4af0c1fcc7 100644
> --- a/rebase-interactive.h
> +++ b/rebase-interactive.h
> @@ -11,7 +11,10 @@ void append_todo_help(unsigned keep_empty, int command_count,
>   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);
> +
>   int todo_list_check(struct todo_list *old_todo, struct todo_list *new_todo);
> +int todo_list_check_against_backup(struct repository *r,
> +				   struct todo_list *todo_list);
>   
>   int check_todo_list_from_file(struct repository *r);
>   
> diff --git a/sequencer.c b/sequencer.c
> index 181bb35f5f..2ff18943fb 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -57,6 +57,8 @@ static GIT_PATH_FUNC(rebase_path, "rebase-merge")
>   GIT_PATH_FUNC(rebase_path_todo, "rebase-merge/git-rebase-todo")
>   GIT_PATH_FUNC(rebase_path_todo_backup, "rebase-merge/git-rebase-todo.backup")
>   
> +GIT_PATH_FUNC(rebase_path_dropped, "rebase-merge/dropped")
> +
>   /*
>    * The rebase command lines that have already been processed. A line
>    * is moved here when it is first handled, before any associated user
> @@ -4273,6 +4275,14 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)
>   	if (is_rebase_i(opts)) {
>   		if ((res = read_populate_todo(r, &todo_list, opts)))
>   			goto release_todo_list;
> +
> +		if (file_exists(rebase_path_dropped())) {
> +			if ((res = todo_list_check_against_backup(r, &todo_list)))
> +				goto release_todo_list;
> +
> +			unlink(rebase_path_dropped());
> +		}
> +
>   		if (commit_staged_changes(r, opts, &todo_list))
>   			return -1;
>   	} else if (!file_exists(get_todo_path(opts)))
> @@ -4986,12 +4996,6 @@ 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,
> @@ -5089,11 +5093,7 @@ 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);
>   
> diff --git a/sequencer.h b/sequencer.h
> index 75ddc5db3a..00debf5107 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -11,6 +11,7 @@ const char *git_path_commit_editmsg(void);
>   const char *git_path_seq_dir(void);
>   const char *rebase_path_todo(void);
>   const char *rebase_path_todo_backup(void);
> +const char *rebase_path_dropped(void);
>   
>   #define APPEND_SIGNOFF_DEDUP (1u << 0)
>   
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 29a35840ed..f5c3da33bf 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -1343,6 +1343,115 @@ 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 &&
> +		git rebase --continue 2>actual
> +	) &&
> +	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)
> +	 - $(git rev-list --pretty=oneline --abbrev-commit -1 master~4)
> +	To avoid this message, use "drop" to explicitly remove a commit.
> +	EOF
> +	head -n4 expect >expect.2 &&
> +	tail -n1 expect >>expect.2 &&
> +	tail -n4 expect.2 >expect.3 &&
> +	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 -n6 actual.2 >actual &&
> +		test_i18ncmp expect actual &&
> +		cp orig .git/rebase-merge/git-rebase-todo &&
> +		FAKE_LINES="1 2 3 4" git rebase --edit-todo 2>actual.2 &&
> +		head -n4 actual.2 >actual &&
> +		test_i18ncmp expect.3 actual &&
> +		git rebase --continue 2>actual
> +	) &&
> +	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 = 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)
> +	 - $(git rev-list --pretty=oneline --abbrev-commit -1 master~4)
> +	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 -n11 expect >expect.2 &&
> +	head -n3 expect.2 >expect.3 &&
> +	tail -n7 expect.2 >>expect.3 &&
> +	test_config rebase.missingCommitsCheck error &&
> +	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 &&
> +		test_must_fail env FAKE_LINES="2 3 4" \
> +			git rebase --edit-todo 2>actual &&
> +		test_i18ncmp expect actual &&
> +		test_must_fail git rebase --continue 2>actual &&
> +		test_i18ncmp expect.2 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 &&
> +		test_i18ncmp expect.3 actual &&
> +		test_must_fail git rebase --continue 2>actual &&
> +		test_i18ncmp expect.3 actual &&
> +		cp orig .git/rebase-merge/git-rebase-todo &&
> +		FAKE_LINES="1 2 3 4 drop 5" git rebase --edit-todo &&
> +		git rebase --continue 2>actual
> +	) &&
> +	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.missingCommitsCheck = error after resolving conflicts' '
> +	test_config rebase.missingCommitsCheck error &&
> +	(
> +		set_fake_editor &&
> +		FAKE_LINES="drop 1 break 2 3 4" git rebase -i A E
> +	) &&
> +	git rebase --edit-todo &&
> +	test_must_fail git rebase --continue &&
> +	echo x >file1 &&
> +	git add file1 &&
> +	git rebase --continue
> +'
> +
>   test_expect_success 'respects rebase.abbreviateCommands with fixup, squash and exec' '
>   	rebase_setup_and_clean abbrevcmd &&
>   	test_commit "first" file1.txt "first line" first &&
> 

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

[Alban Gruin]

Hi Phillip,

Le 19/01/2020 à 17:28, Phillip Wood a écrit :
> Hi Alban
> 
> On 11/01/2020 17:39, Alban Gruin wrote:

>> @@ -121,10 +125,26 @@ 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 (incorrect) {
>> +		if (todo_list_check_against_backup(r, new_todo)) {
>> +			write_file(rebase_path_dropped(), "");
>> +			return -4;
>> +		}
>> +
>> +		if (incorrect > 0)
>> +			unlink(rebase_path_dropped());
>> +	} else if (todo_list_check(todo_list, new_todo)) {
>> +		write_file(rebase_path_dropped(), "");
> 
> We also need to create a backup from todo_list otherwise we'll be using
> a stale backup file for the in the clause above when the user edits the
> todo list to fix it. This test fails
> 
> 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
> 	)
> '
> The second edit complains that commit B is missing even though we have
> already pick it. This is because we're using a stale backup file.
> 

Good catch.  Fortunately this time, the fix is trivial, and the reroll
will come very soon.

> Sorry to be the bearer of bad tidings
> 

No worries, this series wouldn't get very far without your feedback.

> Phillip
> 
>> +		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;
>>   }

Thank you,
Alban

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

[Alban Gruin]

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

[PATCH v5 1/2] sequencer: move check_todo_list_from_file() to rebase-interactive.c

[Alban Gruin]

The message contained in `edit_todo_list_advice' (sequencer.c) is
printed after the initial edit of the todo list if it can't be parsed or
if commits were dropped.  This is done either in complete_action() for
`rebase -i', or in check_todo_list_from_file() for `rebase -p'.

Since we want to add this check when editing the list, we also want to
use this message from edit_todo_list() (rebase-interactive.c).  To this
end, check_todo_list_from_file() is moved to rebase-interactive.c, and
`edit_todo_list_advice' is copied there.  In the next commit,
complete_action() will stop using it, and `edit_todo_list_advice' will
be removed from sequencer.c.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 rebase-interactive.c | 35 +++++++++++++++++++++++++++++++++++
 rebase-interactive.h |  2 ++
 sequencer.c          | 29 -----------------------------
 sequencer.h          |  1 -
 4 files changed, 37 insertions(+), 30 deletions(-)

diff --git a/rebase-interactive.c b/rebase-interactive.c
index aa18ae82b7..ad5dd49c31 100644
--- a/rebase-interactive.c
+++ b/rebase-interactive.c
@@ -6,6 +6,12 @@
 #include "commit-slab.h"
 #include "config.h"
 
+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");
+
 enum missing_commit_check_level {
 	MISSING_COMMIT_CHECK_IGNORE = 0,
 	MISSING_COMMIT_CHECK_WARN,
@@ -187,3 +193,32 @@ int todo_list_check(struct todo_list *old_todo, struct todo_list *new_todo)
 	clear_commit_seen(&commit_seen);
 	return res;
 }
+
+int check_todo_list_from_file(struct repository *r)
+{
+	struct todo_list old_todo = TODO_LIST_INIT, new_todo = TODO_LIST_INIT;
+	int res = 0;
+
+	if (strbuf_read_file(&new_todo.buf, rebase_path_todo(), 0) < 0) {
+		res = error(_("could not read '%s'."), rebase_path_todo());
+		goto out;
+	}
+
+	if (strbuf_read_file(&old_todo.buf, rebase_path_todo_backup(), 0) < 0) {
+		res = error(_("could not read '%s'."), rebase_path_todo_backup());
+		goto out;
+	}
+
+	res = todo_list_parse_insn_buffer(r, old_todo.buf.buf, &old_todo);
+	if (!res)
+		res = todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo);
+	if (!res)
+		res = todo_list_check(&old_todo, &new_todo);
+	if (res)
+		fprintf(stderr, _(edit_todo_list_advice));
+out:
+	todo_list_release(&old_todo);
+	todo_list_release(&new_todo);
+
+	return res;
+}
diff --git a/rebase-interactive.h b/rebase-interactive.h
index 44dbb06311..5f41bf5a28 100644
--- a/rebase-interactive.h
+++ b/rebase-interactive.h
@@ -13,4 +13,6 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list,
 		   const char *shortonto, unsigned flags);
 int todo_list_check(struct todo_list *old_todo, struct todo_list *new_todo);
 
+int check_todo_list_from_file(struct repository *r);
+
 #endif
diff --git a/sequencer.c b/sequencer.c
index ec0b793fc5..181bb35f5f 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4992,35 +4992,6 @@ N_("You can fix this with 'git rebase --edit-todo' "
 "Or you can abort the rebase with 'git rebase"
 " --abort'.\n");
 
-int check_todo_list_from_file(struct repository *r)
-{
-	struct todo_list old_todo = TODO_LIST_INIT, new_todo = TODO_LIST_INIT;
-	int res = 0;
-
-	if (strbuf_read_file_or_whine(&new_todo.buf, rebase_path_todo()) < 0) {
-		res = -1;
-		goto out;
-	}
-
-	if (strbuf_read_file_or_whine(&old_todo.buf, rebase_path_todo_backup()) < 0) {
-		res = -1;
-		goto out;
-	}
-
-	res = todo_list_parse_insn_buffer(r, old_todo.buf.buf, &old_todo);
-	if (!res)
-		res = todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo);
-	if (!res)
-		res = todo_list_check(&old_todo, &new_todo);
-	if (res)
-		fprintf(stderr, _(edit_todo_list_advice));
-out:
-	todo_list_release(&old_todo);
-	todo_list_release(&new_todo);
-
-	return res;
-}
-
 /* skip picking commits whose parents are unchanged */
 static int skip_unnecessary_picks(struct repository *r,
 				  struct todo_list *todo_list,
diff --git a/sequencer.h b/sequencer.h
index 574260f621..75ddc5db3a 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -155,7 +155,6 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
 
 void todo_list_add_exec_commands(struct todo_list *todo_list,
 				 struct string_list *commands);
-int check_todo_list_from_file(struct repository *r);
 int complete_action(struct repository *r, struct replay_opts *opts, unsigned flags,
 		    const char *shortrevisions, const char *onto_name,
 		    struct commit *onto, const char *orig_head, struct string_list *commands,
-- 
2.24.1

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

[Alban Gruin]

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

If the edited list has an error, a file, `dropped', is created to
report the issue.  Otherwise, it is deleted.  Usually, the edited list
is compared against the list before editing, but if this file exists,
it will be compared to the backup.  Also, if the file exists,
sequencer_continue() checks the list for dropped commits.  If the
check was performed every time, it would fail when resuming a rebase
after resolving a conflict, as the backup will contain commits that
were picked, but they will not be in the new list.  It's safe to
ignore this check if `dropped' does not exist, because that means that
no errors were found at the last edition, so any missing commits here
have already been picked.

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,
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.  The fourth test checks that no errors
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>
---

Notes:
    I don't think the way I create `expect.3' files in "rebase --edit-todo
    respects rebase.missingCommitsCheck = warning" & "... = error" is the
    best practice.  Perhaps I should create a new file from scratch instead
    of calling `head' and `tail' successively?

 rebase-interactive.c          |  59 +++++++++++++----
 rebase-interactive.h          |   3 +
 sequencer.c                   |  22 +++----
 sequencer.h                   |   1 +
 t/t3404-rebase-interactive.sh | 121 ++++++++++++++++++++++++++++++++++
 5 files changed, 184 insertions(+), 22 deletions(-)

diff --git a/rebase-interactive.c b/rebase-interactive.c
index ad5dd49c31..97f609f3ee 100644
--- a/rebase-interactive.c
+++ b/rebase-interactive.c
@@ -5,6 +5,7 @@
 #include "strbuf.h"
 #include "commit-slab.h"
 #include "config.h"
+#include "dir.h"
 
 static const char edit_todo_list_advice[] =
 N_("You can fix this with 'git rebase --edit-todo' "
@@ -97,22 +98,29 @@ 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;
+	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) |
+			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))
 		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());
+	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;
@@ -121,10 +129,23 @@ 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 (incorrect) {
+		if (todo_list_check_against_backup(r, new_todo)) {
+			write_file(rebase_path_dropped(), "");
+			return -4;
+		}
+
+		if (incorrect > 0)
+			unlink(rebase_path_dropped());
+	} else if (todo_list_check(todo_list, new_todo)) {
+		write_file(rebase_path_dropped(), "");
+		return -4;
+	}
 
 	return 0;
 }
@@ -189,11 +210,27 @@ 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;
 }
 
+int todo_list_check_against_backup(struct repository *r, struct todo_list *todo_list)
+{
+	struct todo_list backup = TODO_LIST_INIT;
+	int res = 0;
+
+	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);
+	return res;
+}
+
 int check_todo_list_from_file(struct repository *r)
 {
 	struct todo_list old_todo = TODO_LIST_INIT, new_todo = TODO_LIST_INIT;
@@ -212,10 +249,10 @@ int check_todo_list_from_file(struct repository *r)
 	res = todo_list_parse_insn_buffer(r, old_todo.buf.buf, &old_todo);
 	if (!res)
 		res = todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo);
-	if (!res)
-		res = todo_list_check(&old_todo, &new_todo);
 	if (res)
 		fprintf(stderr, _(edit_todo_list_advice));
+	if (!res)
+		res = todo_list_check(&old_todo, &new_todo);
 out:
 	todo_list_release(&old_todo);
 	todo_list_release(&new_todo);
diff --git a/rebase-interactive.h b/rebase-interactive.h
index 5f41bf5a28..4af0c1fcc7 100644
--- a/rebase-interactive.h
+++ b/rebase-interactive.h
@@ -11,7 +11,10 @@ void append_todo_help(unsigned keep_empty, int command_count,
 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);
+
 int todo_list_check(struct todo_list *old_todo, struct todo_list *new_todo);
+int todo_list_check_against_backup(struct repository *r,
+				   struct todo_list *todo_list);
 
 int check_todo_list_from_file(struct repository *r);
 
diff --git a/sequencer.c b/sequencer.c
index 181bb35f5f..2ff18943fb 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -57,6 +57,8 @@ static GIT_PATH_FUNC(rebase_path, "rebase-merge")
 GIT_PATH_FUNC(rebase_path_todo, "rebase-merge/git-rebase-todo")
 GIT_PATH_FUNC(rebase_path_todo_backup, "rebase-merge/git-rebase-todo.backup")
 
+GIT_PATH_FUNC(rebase_path_dropped, "rebase-merge/dropped")
+
 /*
  * The rebase command lines that have already been processed. A line
  * is moved here when it is first handled, before any associated user
@@ -4273,6 +4275,14 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)
 	if (is_rebase_i(opts)) {
 		if ((res = read_populate_todo(r, &todo_list, opts)))
 			goto release_todo_list;
+
+		if (file_exists(rebase_path_dropped())) {
+			if ((res = todo_list_check_against_backup(r, &todo_list)))
+				goto release_todo_list;
+
+			unlink(rebase_path_dropped());
+		}
+
 		if (commit_staged_changes(r, opts, &todo_list))
 			return -1;
 	} else if (!file_exists(get_todo_path(opts)))
@@ -4986,12 +4996,6 @@ 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,
@@ -5089,11 +5093,7 @@ 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);
 
diff --git a/sequencer.h b/sequencer.h
index 75ddc5db3a..00debf5107 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -11,6 +11,7 @@ const char *git_path_commit_editmsg(void);
 const char *git_path_seq_dir(void);
 const char *rebase_path_todo(void);
 const char *rebase_path_todo_backup(void);
+const char *rebase_path_dropped(void);
 
 #define APPEND_SIGNOFF_DEDUP (1u << 0)
 
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 29a35840ed..14b8c3ee4f 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1343,6 +1343,127 @@ 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 &&
+		git rebase --continue 2>actual
+	) &&
+	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)
+	 - $(git rev-list --pretty=oneline --abbrev-commit -1 master~4)
+	To avoid this message, use "drop" to explicitly remove a commit.
+	EOF
+	head -n4 expect >expect.2 &&
+	tail -n1 expect >>expect.2 &&
+	tail -n4 expect.2 >expect.3 &&
+	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 -n6 actual.2 >actual &&
+		test_i18ncmp expect actual &&
+		cp orig .git/rebase-merge/git-rebase-todo &&
+		FAKE_LINES="1 2 3 4" git rebase --edit-todo 2>actual.2 &&
+		head -n4 actual.2 >actual &&
+		test_i18ncmp expect.3 actual &&
+		git rebase --continue 2>actual
+	) &&
+	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 = 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)
+	 - $(git rev-list --pretty=oneline --abbrev-commit -1 master~4)
+	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 -n11 expect >expect.2 &&
+	head -n3 expect.2 >expect.3 &&
+	tail -n7 expect.2 >>expect.3 &&
+	test_config rebase.missingCommitsCheck error &&
+	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 &&
+		test_must_fail env FAKE_LINES="2 3 4" \
+			git rebase --edit-todo 2>actual &&
+		test_i18ncmp expect actual &&
+		test_must_fail git rebase --continue 2>actual &&
+		test_i18ncmp expect.2 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 &&
+		test_i18ncmp expect.3 actual &&
+		test_must_fail git rebase --continue 2>actual &&
+		test_i18ncmp expect.3 actual &&
+		cp orig .git/rebase-merge/git-rebase-todo &&
+		FAKE_LINES="1 2 3 4 drop 5" git rebase --edit-todo &&
+		git rebase --continue 2>actual
+	) &&
+	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.missingCommitsCheck = error after resolving conflicts' '
+	test_config rebase.missingCommitsCheck error &&
+	(
+		set_fake_editor &&
+		FAKE_LINES="drop 1 break 2 3 4" git rebase -i A E
+	) &&
+	git rebase --edit-todo &&
+	test_must_fail git rebase --continue &&
+	echo x >file1 &&
+	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 &&
 	test_commit "first" file1.txt "first line" first &&
-- 
2.24.1

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

[Johannes Schindelin]

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
>
>

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

[Alban Gruin]

Hi Johannes,

Le 26/01/2020 à 11:04, Johannes Schindelin a écrit :
> 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?
> 

Let's do this.  This will also solve the conflict this series already
has with master.

> Thanks,
> Dscho
> 

Cheers,
Alban

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

[Alban Gruin]

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 26027625dd ("rebase -i: also avoid SHA-1 collisions
with missingCommitsCheck", 2020-01-23).

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

Changes since v5:

 - Rebased onto js/rebase-i-with-colliding-hash.  This means the backup
   is once again created with todo_list_write_to_file(), but without the
   flag `TODO_LIST_SHORTEN_IDS' this time.

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          |  84 ++++++++++++++++++++---
 rebase-interactive.h          |   5 ++
 sequencer.c                   |  51 ++++----------
 sequencer.h                   |   2 +-
 t/t3404-rebase-interactive.sh | 121 ++++++++++++++++++++++++++++++++++
 5 files changed, 214 insertions(+), 49 deletions(-)

Range-diff against v5:
1:  996045a300 = 1:  4f07ce94a8 sequencer: move check_todo_list_from_file() to rebase-interactive.c
2:  6dbaa8cbe6 ! 2:  68481d89b0 rebase-interactive: warn if commit is dropped with `rebase --edit-todo'
    @@ rebase-interactive.c: int edit_todo_list(struct repository *r, struct 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());
    -+	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;
    +-	if (initial &&
    +-	    todo_list_write_to_file(r, todo_list, rebase_path_todo_backup(),
    ++	if (!incorrect &&
    ++	    todo_list_write_to_file(r, todo_list, todo_backup,
    + 				    shortrevisions, shortonto, -1,
    + 				    (flags | TODO_LIST_APPEND_TODO_HELP) & ~TODO_LIST_SHORTEN_IDS) < 0)
    + 		return error(_("could not write '%s'."), rebase_path_todo_backup());
     @@ rebase-interactive.c: int edit_todo_list(struct repository *r, struct todo_list *todo_list,
      	if (initial && new_todo->buf.len == 0)
      		return -3;
    @@ sequencer.c: int sequencer_continue(struct repository *r, struct replay_opts *op
     +			unlink(rebase_path_dropped());
     +		}
     +
    - 		if (commit_staged_changes(r, opts, &todo_list))
    - 			return -1;
    - 	} else if (!file_exists(get_todo_path(opts)))
    + 		if (commit_staged_changes(r, opts, &todo_list)) {
    + 			res = -1;
    + 			goto release_todo_list;
     @@ sequencer.c: int todo_list_write_to_file(struct repository *r, struct todo_list *todo_list,
      	return res;
      }
-- 
2.24.1

[PATCH v6 1/2] sequencer: move check_todo_list_from_file() to rebase-interactive.c

[Alban Gruin]

The message contained in `edit_todo_list_advice' (sequencer.c) is
printed after the initial edit of the todo list if it can't be parsed or
if commits were dropped.  This is done either in complete_action() for
`rebase -i', or in check_todo_list_from_file() for `rebase -p'.

Since we want to add this check when editing the list, we also want to
use this message from edit_todo_list() (rebase-interactive.c).  To this
end, check_todo_list_from_file() is moved to rebase-interactive.c, and
`edit_todo_list_advice' is copied there.  In the next commit,
complete_action() will stop using it, and `edit_todo_list_advice' will
be removed from sequencer.c.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 rebase-interactive.c | 35 +++++++++++++++++++++++++++++++++++
 rebase-interactive.h |  2 ++
 sequencer.c          | 29 -----------------------------
 sequencer.h          |  1 -
 4 files changed, 37 insertions(+), 30 deletions(-)

diff --git a/rebase-interactive.c b/rebase-interactive.c
index 1259adc8ea..45f29c28a8 100644
--- a/rebase-interactive.c
+++ b/rebase-interactive.c
@@ -6,6 +6,12 @@
 #include "commit-slab.h"
 #include "config.h"
 
+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");
+
 enum missing_commit_check_level {
 	MISSING_COMMIT_CHECK_IGNORE = 0,
 	MISSING_COMMIT_CHECK_WARN,
@@ -189,3 +195,32 @@ int todo_list_check(struct todo_list *old_todo, struct todo_list *new_todo)
 	clear_commit_seen(&commit_seen);
 	return res;
 }
+
+int check_todo_list_from_file(struct repository *r)
+{
+	struct todo_list old_todo = TODO_LIST_INIT, new_todo = TODO_LIST_INIT;
+	int res = 0;
+
+	if (strbuf_read_file(&new_todo.buf, rebase_path_todo(), 0) < 0) {
+		res = error(_("could not read '%s'."), rebase_path_todo());
+		goto out;
+	}
+
+	if (strbuf_read_file(&old_todo.buf, rebase_path_todo_backup(), 0) < 0) {
+		res = error(_("could not read '%s'."), rebase_path_todo_backup());
+		goto out;
+	}
+
+	res = todo_list_parse_insn_buffer(r, old_todo.buf.buf, &old_todo);
+	if (!res)
+		res = todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo);
+	if (!res)
+		res = todo_list_check(&old_todo, &new_todo);
+	if (res)
+		fprintf(stderr, _(edit_todo_list_advice));
+out:
+	todo_list_release(&old_todo);
+	todo_list_release(&new_todo);
+
+	return res;
+}
diff --git a/rebase-interactive.h b/rebase-interactive.h
index 44dbb06311..5f41bf5a28 100644
--- a/rebase-interactive.h
+++ b/rebase-interactive.h
@@ -13,4 +13,6 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list,
 		   const char *shortonto, unsigned flags);
 int todo_list_check(struct todo_list *old_todo, struct todo_list *new_todo);
 
+int check_todo_list_from_file(struct repository *r);
+
 #endif
diff --git a/sequencer.c b/sequencer.c
index 5f69b47e32..9365fc3e5a 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4991,35 +4991,6 @@ N_("You can fix this with 'git rebase --edit-todo' "
 "Or you can abort the rebase with 'git rebase"
 " --abort'.\n");
 
-int check_todo_list_from_file(struct repository *r)
-{
-	struct todo_list old_todo = TODO_LIST_INIT, new_todo = TODO_LIST_INIT;
-	int res = 0;
-
-	if (strbuf_read_file_or_whine(&new_todo.buf, rebase_path_todo()) < 0) {
-		res = -1;
-		goto out;
-	}
-
-	if (strbuf_read_file_or_whine(&old_todo.buf, rebase_path_todo_backup()) < 0) {
-		res = -1;
-		goto out;
-	}
-
-	res = todo_list_parse_insn_buffer(r, old_todo.buf.buf, &old_todo);
-	if (!res)
-		res = todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo);
-	if (!res)
-		res = todo_list_check(&old_todo, &new_todo);
-	if (res)
-		fprintf(stderr, _(edit_todo_list_advice));
-out:
-	todo_list_release(&old_todo);
-	todo_list_release(&new_todo);
-
-	return res;
-}
-
 /* skip picking commits whose parents are unchanged */
 static int skip_unnecessary_picks(struct repository *r,
 				  struct todo_list *todo_list,
diff --git a/sequencer.h b/sequencer.h
index 9f9ae291e3..5d4df0807c 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -155,7 +155,6 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
 
 void todo_list_add_exec_commands(struct todo_list *todo_list,
 				 struct string_list *commands);
-int check_todo_list_from_file(struct repository *r);
 int complete_action(struct repository *r, struct replay_opts *opts, unsigned flags,
 		    const char *shortrevisions, const char *onto_name,
 		    struct commit *onto, const char *orig_head, struct string_list *commands,
-- 
2.24.1

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

[Alban Gruin]

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

If the edited list has an error, a file, `dropped', is created to
report the issue.  Otherwise, it is deleted.  Usually, the edited list
is compared against the list before editing, but if this file exists,
it will be compared to the backup.  Also, if the file exists,
sequencer_continue() checks the list for dropped commits.  If the
check was performed every time, it would fail when resuming a rebase
after resolving a conflict, as the backup will contain commits that
were picked, but they will not be in the new list.  It's safe to
ignore this check if `dropped' does not exist, because that means that
no errors were found at the last edition, so any missing commits here
have already been picked.

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,
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.  The fourth test checks that no errors
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>
---

Notes:
    I don't think the way I create `expect.3' files in "rebase --edit-todo
    respects rebase.missingCommitsCheck = warning" & "... = error" is the
    best practice.  Perhaps I should create a new file from scratch instead
    of calling `head' and `tail' successively?

 rebase-interactive.c          |  53 ++++++++++++---
 rebase-interactive.h          |   3 +
 sequencer.c                   |  22 +++----
 sequencer.h                   |   1 +
 t/t3404-rebase-interactive.sh | 121 ++++++++++++++++++++++++++++++++++
 5 files changed, 179 insertions(+), 21 deletions(-)

diff --git a/rebase-interactive.c b/rebase-interactive.c
index 45f29c28a8..ac001dea58 100644
--- a/rebase-interactive.c
+++ b/rebase-interactive.c
@@ -5,6 +5,7 @@
 #include "strbuf.h"
 #include "commit-slab.h"
 #include "config.h"
+#include "dir.h"
 
 static const char edit_todo_list_advice[] =
 N_("You can fix this with 'git rebase --edit-todo' "
@@ -97,21 +98,24 @@ 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;
+	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) |
+			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))
 		return error_errno(_("could not write '%s'"), todo_file);
 
-	if (initial &&
-	    todo_list_write_to_file(r, todo_list, rebase_path_todo_backup(),
+	if (!incorrect &&
+	    todo_list_write_to_file(r, todo_list, todo_backup,
 				    shortrevisions, shortonto, -1,
 				    (flags | TODO_LIST_APPEND_TODO_HELP) & ~TODO_LIST_SHORTEN_IDS) < 0)
 		return error(_("could not write '%s'."), rebase_path_todo_backup());
@@ -123,10 +127,23 @@ 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 (incorrect) {
+		if (todo_list_check_against_backup(r, new_todo)) {
+			write_file(rebase_path_dropped(), "");
+			return -4;
+		}
+
+		if (incorrect > 0)
+			unlink(rebase_path_dropped());
+	} else if (todo_list_check(todo_list, new_todo)) {
+		write_file(rebase_path_dropped(), "");
+		return -4;
+	}
 
 	return 0;
 }
@@ -191,11 +208,27 @@ 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;
 }
 
+int todo_list_check_against_backup(struct repository *r, struct todo_list *todo_list)
+{
+	struct todo_list backup = TODO_LIST_INIT;
+	int res = 0;
+
+	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);
+	return res;
+}
+
 int check_todo_list_from_file(struct repository *r)
 {
 	struct todo_list old_todo = TODO_LIST_INIT, new_todo = TODO_LIST_INIT;
@@ -214,10 +247,10 @@ int check_todo_list_from_file(struct repository *r)
 	res = todo_list_parse_insn_buffer(r, old_todo.buf.buf, &old_todo);
 	if (!res)
 		res = todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo);
-	if (!res)
-		res = todo_list_check(&old_todo, &new_todo);
 	if (res)
 		fprintf(stderr, _(edit_todo_list_advice));
+	if (!res)
+		res = todo_list_check(&old_todo, &new_todo);
 out:
 	todo_list_release(&old_todo);
 	todo_list_release(&new_todo);
diff --git a/rebase-interactive.h b/rebase-interactive.h
index 5f41bf5a28..4af0c1fcc7 100644
--- a/rebase-interactive.h
+++ b/rebase-interactive.h
@@ -11,7 +11,10 @@ void append_todo_help(unsigned keep_empty, int command_count,
 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);
+
 int todo_list_check(struct todo_list *old_todo, struct todo_list *new_todo);
+int todo_list_check_against_backup(struct repository *r,
+				   struct todo_list *todo_list);
 
 int check_todo_list_from_file(struct repository *r);
 
diff --git a/sequencer.c b/sequencer.c
index 9365fc3e5a..638098b936 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -57,6 +57,8 @@ static GIT_PATH_FUNC(rebase_path, "rebase-merge")
 GIT_PATH_FUNC(rebase_path_todo, "rebase-merge/git-rebase-todo")
 GIT_PATH_FUNC(rebase_path_todo_backup, "rebase-merge/git-rebase-todo.backup")
 
+GIT_PATH_FUNC(rebase_path_dropped, "rebase-merge/dropped")
+
 /*
  * The rebase command lines that have already been processed. A line
  * is moved here when it is first handled, before any associated user
@@ -4239,6 +4241,14 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)
 	if (is_rebase_i(opts)) {
 		if ((res = read_populate_todo(r, &todo_list, opts)))
 			goto release_todo_list;
+
+		if (file_exists(rebase_path_dropped())) {
+			if ((res = todo_list_check_against_backup(r, &todo_list)))
+				goto release_todo_list;
+
+			unlink(rebase_path_dropped());
+		}
+
 		if (commit_staged_changes(r, opts, &todo_list)) {
 			res = -1;
 			goto release_todo_list;
@@ -4985,12 +4995,6 @@ 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,
@@ -5088,11 +5092,7 @@ 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);
 
diff --git a/sequencer.h b/sequencer.h
index 5d4df0807c..84dc4160d5 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -11,6 +11,7 @@ const char *git_path_commit_editmsg(void);
 const char *git_path_seq_dir(void);
 const char *rebase_path_todo(void);
 const char *rebase_path_todo_backup(void);
+const char *rebase_path_dropped(void);
 
 #define APPEND_SIGNOFF_DEDUP (1u << 0)
 
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index b90ea0fe44..d79a3ef505 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1463,6 +1463,127 @@ 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 &&
+		git rebase --continue 2>actual
+	) &&
+	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)
+	 - $(git rev-list --pretty=oneline --abbrev-commit -1 master~4)
+	To avoid this message, use "drop" to explicitly remove a commit.
+	EOF
+	head -n4 expect >expect.2 &&
+	tail -n1 expect >>expect.2 &&
+	tail -n4 expect.2 >expect.3 &&
+	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 -n6 actual.2 >actual &&
+		test_i18ncmp expect actual &&
+		cp orig .git/rebase-merge/git-rebase-todo &&
+		FAKE_LINES="1 2 3 4" git rebase --edit-todo 2>actual.2 &&
+		head -n4 actual.2 >actual &&
+		test_i18ncmp expect.3 actual &&
+		git rebase --continue 2>actual
+	) &&
+	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 = 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)
+	 - $(git rev-list --pretty=oneline --abbrev-commit -1 master~4)
+	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 -n11 expect >expect.2 &&
+	head -n3 expect.2 >expect.3 &&
+	tail -n7 expect.2 >>expect.3 &&
+	test_config rebase.missingCommitsCheck error &&
+	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 &&
+		test_must_fail env FAKE_LINES="2 3 4" \
+			git rebase --edit-todo 2>actual &&
+		test_i18ncmp expect actual &&
+		test_must_fail git rebase --continue 2>actual &&
+		test_i18ncmp expect.2 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 &&
+		test_i18ncmp expect.3 actual &&
+		test_must_fail git rebase --continue 2>actual &&
+		test_i18ncmp expect.3 actual &&
+		cp orig .git/rebase-merge/git-rebase-todo &&
+		FAKE_LINES="1 2 3 4 drop 5" git rebase --edit-todo &&
+		git rebase --continue 2>actual
+	) &&
+	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.missingCommitsCheck = error after resolving conflicts' '
+	test_config rebase.missingCommitsCheck error &&
+	(
+		set_fake_editor &&
+		FAKE_LINES="drop 1 break 2 3 4" git rebase -i A E
+	) &&
+	git rebase --edit-todo &&
+	test_must_fail git rebase --continue &&
+	echo x >file1 &&
+	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 &&
 	test_commit "first" file1.txt "first line" first &&
-- 
2.24.1

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 413 bytes --]

Hi Alban,

On Mon, 27 Jan 2020, Alban Gruin wrote:

> Le 26/01/2020 à 11:04, Johannes Schindelin a écrit :
>
> > 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?
> >
>
> Let's do this.  This will also solve the conflict this series already
> has with master.

Thank you!
Dscho