RFC patch, 9 partsrebase -i: extend rebase.missingCommitsCheck to `--edit-todo' and co.

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

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

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

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

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

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

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

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

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

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

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

> 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->current = 0;
> > 
> > +		todo_list->done_nr += i;
> > 
> >  		if (is_fixup(peek_command(todo_list, 0)))
> >  		
> >  			record_in_rewritten(base_oid, 

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

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

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

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

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

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

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

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

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

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

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

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

> 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();
>  	/* 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);

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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