patch, 8 partsrebase: improve resumption from incorrect initial todo list

> When the user butchers the todo file during rebase -i setup, the
> --continue which would follow --edit-todo would have skipped the last
> steps of the setup. Notably, this would bypass the fast-forward over
> untouched picks (though the actual picking loop would still fast-forward
> the commits, one by one).
> 
> Fix this by splitting off the tail of complete_action() to a new
> start_rebase() function and call that from sequencer_continue() when no
> commands have been executed yet.
> 
> More or less as a side effect, we no longer checkout `onto` before exiting
> when the todo file is bad. 

> diff --git a/builtin/revert.c b/builtin/revert.c
> index 62986a7b1b..00d3e19c62 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -231,7 +231,8 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
>   		return ret;
>   	}
>   	if (cmd == 'c')
> -		return sequencer_continue(the_repository, opts);
> +		return sequencer_continue(the_repository, opts,
> +					  0, NULL, NULL, NULL);

> @@ -6142,49 +6154,52 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
>   
>   		return error(_("nothing to do"));
>   	} else if (res == EDIT_TODO_INCORRECT) {
> -		checkout_onto(r, opts, onto_name, onto, orig_head);
>   		todo_list_release(&new_todo);
>   
>   		return -1;
>   	}
>   
> -	/* Expand the commit IDs */
> -	todo_list_to_strbuf(r, &new_todo, &buf2, -1, 0);
> -	strbuf_swap(&new_todo.buf, &buf2);
> -	strbuf_release(&buf2);
> -	new_todo.total_nr -= new_todo.nr;
> -	if (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo) < 0)
> -		BUG("invalid todo list after expanding IDs:\n%s",
> -		    new_todo.buf.buf);

> -	if (opts->allow_ff && skip_unnecessary_picks(r, &new_todo, &onto)) {
> -		todo_list_release(&new_todo);
> -		return error(_("could not skip unnecessary pick commands"));
> -	}
> -
> -	if (todo_list_write_to_file(r, &new_todo, todo_file, NULL, NULL, -1,
> -				    flags & ~(TODO_LIST_SHORTEN_IDS), action)) {
> -		todo_list_release(&new_todo);
> -		return error_errno(_("could not write '%s'"), todo_file);
> -	}
> -
> -	res = -1;
> -
> -	if (checkout_onto(r, opts, onto_name, onto, orig_head))
> -		goto cleanup;
> -
> -	if (require_clean_work_tree(r, "rebase", NULL, 1, 1))
> -		goto cleanup;
> -
> -	todo_list_write_total_nr(&new_todo);
> -	res = pick_commits(r, &new_todo, opts);
> -
> -cleanup:
> +	res = start_rebase(r, opts, flags, onto_name, onto, orig_head, &new_todo);
>   	todo_list_release(&new_todo);
>   
>   	return res;
>   }
>   

> +	(
> +		set_fake_editor &&
> +		test_must_fail env FAKE_LINES="bad 1 pick 1 pick 2 reword 3" \
> +			git rebase -i HEAD~3 &&
> +		test_cmp_rev HEAD primary &&
> +		FAKE_LINES="pick 2 pick 3 reword 4" git rebase --edit-todo &&
> +		FAKE_COMMIT_MESSAGE="E_reworded" git rebase --continue
> +	) &&
> +	git reflog > reflog &&
> +	test $(grep -c fast-forward reflog) = 1 &&

> +'
> +
> +test_expect_success 'abort after bad first command' '
> +	test_when_finished "git rebase --abort ||:" &&
> +	git checkout primary^0 &&
> +	(
> +		set_fake_editor &&
> +		test_must_fail env FAKE_LINES="bad 1 pick 1 pick 2 reword 3" \
> +			git rebase -i HEAD~3
> +	) &&
> +	git rebase --abort &&
> +	test_cmp_rev HEAD primary
> +'
> +
>   test_expect_success 'tabs and spaces are accepted in the todolist' '
>   	rebase_setup_and_clean indented-comment &&
>   	write_script add-indent.sh <<-\EOF &&

>On 23/03/2023 16:22, Oswald Buddenhagen wrote:
>> When the user butchers the todo file during rebase -i setup, the
>> --continue which would follow --edit-todo would have skipped the last
>> steps of the setup. Notably, this would bypass the fast-forward over
>> untouched picks (though the actual picking loop would still fast-forward
>> the commits, one by one).
>> 
>> Fix this by splitting off the tail of complete_action() to a new
>> start_rebase() function and call that from sequencer_continue() when no
>> commands have been executed yet.
>> 
>> More or less as a side effect, we no longer checkout `onto` before exiting
>> when the todo file is bad. 
>
>I think the implications of this change deserve to be discussed in the 
>commit message. Three things spring to mind but there may be others I 
>haven't thought of
>
>  - Previously when rebase stopped and handed control back to the user
>    HEAD would have already been detached. This patch changes that
>    meaning we can have an active rebase of a branch while that branch is
>    checked out. What does "git status" show in this case? What does the
>    shell prompt show? Will it confuse users?
>

> > This makes aborting cheaper and will simplify
> > things in a later change.
>
>Given that we're stopping so the user can fix the problem and continue 
>the rebase I don't think optimizing for aborting is a convincing reason 
>for this change on its own.
>

>> diff --git a/builtin/revert.c b/builtin/revert.c
>> index 62986a7b1b..00d3e19c62 100644
>> --- a/builtin/revert.c
>> +++ b/builtin/revert.c
>> @@ -231,7 +231,8 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
>>   		return ret;
>>   	}
>>   	if (cmd == 'c')
>> -		return sequencer_continue(the_repository, opts);
>> +		return sequencer_continue(the_repository, opts,
>> +					  0, NULL, NULL, NULL);
>
>It's a bit unfortunate that we have to start passing all these extra 
>parameters, could the sequencer read them itself in read_populate_opts()?
>

> On Sun, Mar 26, 2023 at 03:28:01PM +0100, Phillip Wood wrote:
>> On 23/03/2023 16:22, Oswald Buddenhagen wrote:
>>> When the user butchers the todo file during rebase -i setup, the
>>> --continue which would follow --edit-todo would have skipped the last
>>> steps of the setup. Notably, this would bypass the fast-forward over
>>> untouched picks (though the actual picking loop would still fast-forward
>>> the commits, one by one).
>>>
>>> Fix this by splitting off the tail of complete_action() to a new
>>> start_rebase() function and call that from sequencer_continue() when no
>>> commands have been executed yet.
>>>
>>> More or less as a side effect, we no longer checkout `onto` before 
>>> exiting
>>> when the todo file is bad. 
>>
>> I think the implications of this change deserve to be discussed in the 
>> commit message. Three things spring to mind but there may be others I 
>> haven't thought of
>>
>>  - Previously when rebase stopped and handed control back to the user
>>    HEAD would have already been detached. This patch changes that
>>    meaning we can have an active rebase of a branch while that branch is
>>    checked out. What does "git status" show in this case? What does the
>>    shell prompt show? Will it confuse users?
>>
> the failed state is identical to the "still editing the initial todo" 
> state as far as "git status" and the shell prompt are concerned. this 
> seems reasonable. i'll add it to the commit message.

>>  - Previously if the user created a commit before running "rebase
>>    --continue" we'd rebase on to that commit. Now that commit will be
>>    silently dropped.
>>
> this is arguably a problem, but not much different from the pre-existing 
> behavior of changes to HEAD done during the initial todo edit being lost.

>>  - Previously if the user checkout out another commit before running
>>    "rebase --continue" we'd rebase on to that commit. Now we we rebase
>>    on to the original "onto" commit.
>>
> this can be subsumed into the above case.

>> > This makes aborting cheaper and will simplify
>> > things in a later change.
>>
>> Given that we're stopping so the user can fix the problem and continue 
>> the rebase I don't think optimizing for aborting is a convincing 
>> reason for this change on its own.
>>
> this is all part of the "More or less as a side effect" paragraph, so 
> this isn't a relevant objection.

>>> diff --git a/builtin/revert.c b/builtin/revert.c
>>> index 62986a7b1b..00d3e19c62 100644
>>> --- a/builtin/revert.c
>>> +++ b/builtin/revert.c
>>> @@ -231,7 +231,8 @@ static int run_sequencer(int argc, const char 
>>> **argv, struct replay_opts *opts)
>>>           return ret;
>>>       }
>>>       if (cmd == 'c')
>>> -        return sequencer_continue(the_repository, opts);
>>> +        return sequencer_continue(the_repository, opts,
>>> +                      0, NULL, NULL, NULL);
>>
>> It's a bit unfortunate that we have to start passing all these extra 
>> parameters, could the sequencer read them itself in read_populate_opts()?
>>
> that wouldn't help in this case, as these are dummy values which aren't 
> going to be used.

>>> -int sequencer_continue(struct repository *r, struct replay_opts *opts)
>>> +static int start_rebase(struct repository *r, struct replay_opts 
>>> *opts, unsigned flags,
>>> +            const char *onto_name, const struct object_id *onto,
>>> +            const struct object_id *orig_head, struct todo_list 
>>> *todo_list);
>>
>> It would be nice to avoid this forward declaration. I think you could 
>> do that by adding a preparatory patch that moves either 
>> checkout_onto() or sequencer_continue()
>>
> i went for the "minimal churn" approach.

>>> +    git reflog > reflog &&
>>> +    test $(grep -c fast-forward reflog) = 1 &&
>>
>> Using test_line_count would make test failures easier to debug.
>>
> that's calling for a new test_filtered_line_count function which would 
> have quite some users.
> for the time being, both grep + test_line_count and grep -c are rather 
> prevalent, in this file the latter in particular.

>>> +    test_cmp_rev HEAD~1 primary~1 &&
>>> +    test "$(git log -1 --format=%B)" = "E_reworded"
>>
>> It is slightly more work, but please use test_cmp for things like this 
>> as it makes it so much easier to debug test failures.
>>
> fair enough, but the precedents again speak a different language.

>On 26/04/2023 16:34, Oswald Buddenhagen wrote:
>> the failed state is identical to the "still editing the initial todo" 
>> state as far as "git status" and the shell prompt are concerned. this 
>> seems reasonable. i'll add it to the commit message.
>
>When you do that please mention what "git status" and the shell prompt 
>actually print in this case.
>

>>>  - Previously if the user created a commit before running "rebase
>>>    --continue" we'd rebase on to that commit. Now that commit will be
>>>    silently dropped.
>>>
>> this is arguably a problem, but not much different from the pre-existing 
>> behavior of changes to HEAD done during the initial todo edit being lost.
>
>I think there is a significant difference in that we're moving from a 
>situation where we lose commits that are created while rebase is running 
>to one where we're losing commits created while rebase is stopped. If a 
>user tries to create a commit while rebase is running then they're 
>asking for trouble. I don't think creating commits when rebase is 
>stopped is unreasonable in the same way.
>

>> to avoid that, we'd need to lock HEAD while editing the todo. is that 
>> realistic at all?
>
>I don't think it is practical to lock HEAD while git is not running.
>

>> on top of that, i should verify HEAD against orig-head in 
>> start_rebase(). though the only way for the user to get out of that 
>> situation is saving the todo contents and --abort'ing (and we must 
>> take care not the touch HEAD).
>
>I think in that case it wouldn't be terrible to lose the edited todo 
>list as it is a bit of a corner case.
>

>> this is somewhat similar to the abysmal situation of the final 
>> update-ref failing if the target ref has been modified while being 
>> rebased. we'd need to lock that ref for the entire duration of the 
>> rebase to avoid that.
>
>"abysmal" is rather harsh - it would also be bad to overwrite the ref in 
>that case. I think it in relatively hard to get into that situation 
>though as "git checkout" wont checkout a branch that is being updated by 
>a rebase.
>

>>>  - Previously if the user checkout out another commit before running
>>>    "rebase --continue" we'd rebase on to that commit. Now we we rebase
>>>    on to the original "onto" commit.
>>>
>> this can be subsumed into the above case.
>
>Meaning check and error out if HEAD has changed?
>

>>> > This makes aborting cheaper and will simplify
>>> > things in a later change.
>>>
>>> Given that we're stopping so the user can fix the problem and continue 
>>> the rebase I don't think optimizing for aborting is a convincing 
>>> reason for this change on its own.
>>>
>> this is all part of the "More or less as a side effect" paragraph, so 
>> this isn't a relevant objection.
>
>I'm simply saying that we should not be optimizing for "rebase --abort" 
>in this case. Do you think we should?
>

> The code's evolution left in some bits surrounding enum rebase_type that
> don't really make sense any more. In particular, it makes no sense to
> invoke imply_merge() if the type is already known not to be
> REBASE_APPLY, and it makes no sense to assign the type after calling
> imply_merge().

> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
> ---
>   builtin/rebase.c | 6 +-----
>   1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 5b7b908b66..8ffea0f0d8 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -372,7 +372,6 @@ static int parse_opt_keep_empty(const struct option *opt, const char *arg,
>   
>   	imply_merge(opts, unset ? "--no-keep-empty" : "--keep-empty");
>   	opts->keep_empty = !unset;
> -	opts->type = REBASE_MERGE;
>   	return 0;
>   }
>   
> @@ -1494,9 +1493,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   		}
>   	}
>   
> -	if (options.type == REBASE_MERGE)
> -		imply_merge(&options, "--merge");
> -
>   	if (options.root && !options.onto_name)
>   		imply_merge(&options, "--root without --onto");
>   
> @@ -1534,7 +1530,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   
>   	if (options.type == REBASE_UNSPECIFIED) {
>   		if (!strcmp(options.default_backend, "merge"))
> -			imply_merge(&options, "--merge");
> +			options.type = REBASE_MERGE;
>   		else if (!strcmp(options.default_backend, "apply"))
>   			options.type = REBASE_APPLY;
>   		else

> On 23/03/2023 16:22, Oswald Buddenhagen wrote:
>> The code's evolution left in some bits surrounding enum rebase_type that
>> don't really make sense any more. In particular, it makes no sense to
>> invoke imply_merge() if the type is already known not to be
>> REBASE_APPLY, and it makes no sense to assign the type after calling
>> imply_merge().
>
> These look sensible, did imply_merges() use to do something more which
> made these calls useful?

>>   @@ -1494,9 +1493,6 @@ int cmd_rebase(int argc, const char **argv,
>> const char *prefix)
>>   		}
>>   	}
>>   -	if (options.type == REBASE_MERGE)
>> -		imply_merge(&options, "--merge");

>
> Phillip Wood <phillip.wood123@gmail.com> writes:
>
> > On 23/03/2023 16:22, Oswald Buddenhagen wrote:
> >> The code's evolution left in some bits surrounding enum rebase_type that
> >> don't really make sense any more. In particular, it makes no sense to
> >> invoke imply_merge() if the type is already known not to be
> >> REBASE_APPLY, and it makes no sense to assign the type after calling
> >> imply_merge().
> >
> > These look sensible, did imply_merges() use to do something more which
> > made these calls useful?
>
> Good question.

> This moves it right next to parse_opt_empty(), which is a much more
> logical place. As a side effect, this removes the need for a forward
> declaration of imply_merge().
> 
> Acked-by: Phillip Wood <phillip.wood123@gmail.com>
> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
> 
> ---
> i'm not sure how to "translate" phillip's informal approval; the
> acked-by doesn't seem quite right. please adjust as necessary.

> Cc: Junio C Hamano <gitster@pobox.com>
> ---
>   builtin/rebase.c | 25 ++++++++++++-------------
>   1 file changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 4a093bb125..13ca5a644b 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -376,19 +376,6 @@ static int run_sequencer_rebase(struct rebase_options *opts)
>   	return ret;
>   }
>   
> -static void imply_merge(struct rebase_options *opts, const char *option);
> -static int parse_opt_keep_empty(const struct option *opt, const char *arg,
> -				int unset)
> -{
> -	struct rebase_options *opts = opt->value;
> -
> -	BUG_ON_OPT_ARG(arg);
> -
> -	imply_merge(opts, unset ? "--no-keep-empty" : "--keep-empty");
> -	opts->keep_empty = !unset;
> -	return 0;
> -}
> -
>   static int is_merge(struct rebase_options *opts)
>   {
>   	return opts->type == REBASE_MERGE;
> @@ -982,6 +969,18 @@ static enum empty_type parse_empty_value(const char *value)
>   	die(_("unrecognized empty type '%s'; valid values are \"drop\", \"keep\", and \"ask\"."), value);
>   }
>   
> +static int parse_opt_keep_empty(const struct option *opt, const char *arg,
> +				int unset)
> +{
> +	struct rebase_options *opts = opt->value;
> +
> +	BUG_ON_OPT_ARG(arg);
> +
> +	imply_merge(opts, unset ? "--no-keep-empty" : "--keep-empty");
> +	opts->keep_empty = !unset;
> +	return 0;
> +}
> +
>   static int parse_opt_empty(const struct option *opt, const char *arg, int unset)
>   {
>   	struct rebase_options *options = opt->value;

> At least after the successive trimming of enum rebase_type mentioned in
> the previous commit, this code did exactly what imply_merge() does, so
> just call it instead.
>
> Suggested-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>

>
> ---
> Cc: Phillip Wood <phillip.wood123@gmail.com>
> ---
>  builtin/rebase.c | 13 +------------
>  1 file changed, 1 insertion(+), 12 deletions(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 44cc1eed12..4a093bb125 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1490,18 +1490,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  
>  	if (options.strategy) {
>  		options.strategy = xstrdup(options.strategy);
> -		switch (options.type) {
> -		case REBASE_APPLY:
> -			die(_("--strategy requires --merge or --interactive"));
> -		case REBASE_MERGE:
> -			/* compatible */
> -			break;
> -		case REBASE_UNSPECIFIED:
> -			options.type = REBASE_MERGE;
> -			break;
> -		default:
> -			BUG("unhandled rebase type (%d)", options.type);
> -		}
> +		imply_merge(&options, "--strategy");
>  	}
>  
>  	if (options.root && !options.onto_name)

> broken out of the bigger series, as the aggregation just unnecessarily holds it
> up.
>
> v3: removed "stray" footer. so more of a RESEND than an actual new version.
>
> Oswald Buddenhagen (3):
>   rebase: simplify code related to imply_merge()
>   rebase: handle --strategy via imply_merge() as well
>   rebase: move parse_opt_keep_empty() down
>
>  builtin/rebase.c | 44 ++++++++++++++------------------------------
>  1 file changed, 14 insertions(+), 30 deletions(-)

> Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
> 
>> broken out of the bigger series, as the aggregation just unnecessarily holds it
>> up.
>>
>> v3: removed "stray" footer. so more of a RESEND than an actual new version.
>>
>> Oswald Buddenhagen (3):
>>    rebase: simplify code related to imply_merge()
>>    rebase: handle --strategy via imply_merge() as well
>>    rebase: move parse_opt_keep_empty() down
>>
>>   builtin/rebase.c | 44 ++++++++++++++------------------------------
>>   1 file changed, 14 insertions(+), 30 deletions(-)
> 
> Looking quite straight-forward and I didn't see anythihng
> potentially controversial.

> On 20/10/2023 23:07, Junio C Hamano wrote:
>> Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
>> 
>>> broken out of the bigger series, as the aggregation just unnecessarily holds it
>>> up.
>>>
>>> v3: removed "stray" footer. so more of a RESEND than an actual new version.
>>>
>>> Oswald Buddenhagen (3):
>>>    rebase: simplify code related to imply_merge()
>>>    rebase: handle --strategy via imply_merge() as well
>>>    rebase: move parse_opt_keep_empty() down
>>>
>>>   builtin/rebase.c | 44 ++++++++++++++------------------------------
>>>   1 file changed, 14 insertions(+), 30 deletions(-)
>> Looking quite straight-forward and I didn't see anythihng
>> potentially controversial.
>
> Yes they look good, thanks Oswald

> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
> ---
>   builtin/rebase.c | 25 ++++++++++++-------------
>   1 file changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 8ffea0f0d8..491759db19 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -362,19 +362,6 @@ static int run_sequencer_rebase(struct rebase_options *opts)
>   	return ret;
>   }
>   
> -static void imply_merge(struct rebase_options *opts, const char *option);
> -static int parse_opt_keep_empty(const struct option *opt, const char *arg,
> -				int unset)
> -{
> -	struct rebase_options *opts = opt->value;
> -
> -	BUG_ON_OPT_ARG(arg);
> -
> -	imply_merge(opts, unset ? "--no-keep-empty" : "--keep-empty");
> -	opts->keep_empty = !unset;
> -	return 0;
> -}
> -
>   static int is_merge(struct rebase_options *opts)
>   {
>   	return opts->type == REBASE_MERGE;
> @@ -969,6 +956,18 @@ static enum empty_type parse_empty_value(const char *value)
>   	die(_("unrecognized empty type '%s'; valid values are \"drop\", \"keep\", and \"ask\"."), value);
>   }
>   
> +static int parse_opt_keep_empty(const struct option *opt, const char *arg,
> +				int unset)
> +{
> +	struct rebase_options *opts = opt->value;
> +
> +	BUG_ON_OPT_ARG(arg);
> +
> +	imply_merge(opts, unset ? "--no-keep-empty" : "--keep-empty");
> +	opts->keep_empty = !unset;
> +	return 0;
> +}
> +
>   static int parse_opt_empty(const struct option *opt, const char *arg, int unset)
>   {
>   	struct rebase_options *options = opt->value;

> This is a lot cleaner than open-coding magic numbers.
> 
> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
> ---
>   rebase-interactive.c | 15 ++++++++-------
>   rebase-interactive.h | 11 ++++++++++-
>   sequencer.c          |  8 ++++----
>   3 files changed, 22 insertions(+), 12 deletions(-)
> 
> diff --git a/rebase-interactive.c b/rebase-interactive.c
> index 111a2071ae..a3d8925b06 100644
> --- a/rebase-interactive.c
> +++ b/rebase-interactive.c
> @@ -94,7 +94,8 @@ void append_todo_help(int command_count, enum rebase_action action,
>   	strbuf_add_commented_lines(buf, msg, strlen(msg));
>   }
>   
> -int edit_todo_list(struct repository *r, struct todo_list *todo_list,
> +enum edit_todo_result edit_todo_list(
> +		   struct repository *r, struct todo_list *todo_list,
>   		   struct todo_list *new_todo, const char *shortrevisions,
>   		   const char *shortonto, unsigned flags,
>   		   enum rebase_action action)
> @@ -123,37 +124,37 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list,
>   		return error(_("could not write '%s'."), rebase_path_todo_backup());
>   
>   	if (launch_sequence_editor(todo_file, &new_todo->buf, NULL))
> -		return -2;
> +		return EDIT_TODO_FAILED;
>   
>   	strbuf_stripspace(&new_todo->buf, 1);
>   	if (action != ACTION_EDIT_TODO && new_todo->buf.len == 0)
> -		return -3;
> +		return EDIT_TODO_ABORT;
>   
>   	if (todo_list_parse_insn_buffer(r, new_todo->buf.buf, new_todo)) {
>   		fprintf(stderr, _(edit_todo_list_advice));
> -		return -4;
> +		return EDIT_TODO_INCORRECT;
>   	}
>   
>   	if (incorrect) {
>   		if (todo_list_check_against_backup(r, new_todo)) {
>   			write_file(rebase_path_dropped(), "%s", "");
> -			return -4;
> +			return EDIT_TODO_INCORRECT;
>   		}
>   
>   		if (incorrect > 0)
>   			unlink(rebase_path_dropped());
>   	} else if (todo_list_check(todo_list, new_todo)) {
>   		write_file(rebase_path_dropped(), "%s", "");
> -		return -4;
> +		return EDIT_TODO_INCORRECT;
>   	}
>   
>   	/*
>   	 * See if branches need to be added or removed from the update-refs
>   	 * file based on the new todo list.
>   	 */
>   	todo_list_filter_update_refs(r, new_todo);
>   
> -	return 0;
> +	return EDIT_TODO_OK;
>   }
>   
>   define_commit_slab(commit_seen, unsigned char);
> diff --git a/rebase-interactive.h b/rebase-interactive.h
> index d9873d3497..5aa4111b4f 100644
> --- a/rebase-interactive.h
> +++ b/rebase-interactive.h
> @@ -16,10 +16,19 @@ enum rebase_action {
>   	ACTION_LAST
>   };
>   
> +enum edit_todo_result {
> +	EDIT_TODO_OK = 0,         // must be 0
> +	EDIT_TODO_IOERROR = -1,   // generic i/o error; must be -1
> +	EDIT_TODO_FAILED = -2,    // editing failed
> +	EDIT_TODO_ABORT = -3,     // user requested abort
> +	EDIT_TODO_INCORRECT = -4  // file violates syntax or constraints
> +};
> +
>   void append_todo_help(int command_count, enum rebase_action action,
>   		      const char *shortrevisions, const char *shortonto,
>   		      struct strbuf *buf);
> -int edit_todo_list(struct repository *r, struct todo_list *todo_list,
> +enum edit_todo_result edit_todo_list(
> +		   struct repository *r, struct todo_list *todo_list,
>   		   struct todo_list *new_todo, const char *shortrevisions,
>   		   const char *shortonto, unsigned flags,
>   		   enum rebase_action action);
> diff --git a/sequencer.c b/sequencer.c
> index f05174d151..b1c29c8802 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -6125,20 +6125,20 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
>   
>   	res = edit_todo_list(r, todo_list, &new_todo, shortrevisions,
>   			     shortonto, flags, action);
> -	if (res == -1)
> +	if (res == EDIT_TODO_IOERROR)
>   		return -1;
> -	else if (res == -2) {
> +	else if (res == EDIT_TODO_FAILED) {
>   		apply_autostash(rebase_path_autostash());
>   		sequencer_remove_state(opts);
>   
>   		return -1;
> -	} else if (res == -3) {
> +	} else if (res == EDIT_TODO_ABORT) {
>   		apply_autostash(rebase_path_autostash());
>   		sequencer_remove_state(opts);
>   		todo_list_release(&new_todo);
>   
>   		return error(_("nothing to do"));
> -	} else if (res == -4) {
> +	} else if (res == EDIT_TODO_INCORRECT) {
>   		checkout_onto(r, opts, onto_name, &onto->object.oid, orig_head);
>   		todo_list_release(&new_todo);
>   

> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
> ---
>   builtin/rebase.c              | 1 +
>   sequencer.c                   | 4 ++++
>   sequencer.h                   | 1 +
>   t/t3404-rebase-interactive.sh | 3 ++-
>   4 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index a309addd50..728c869db4 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -153,6 +153,7 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)
>   	replay.keep_redundant_commits = (opts->empty == EMPTY_KEEP);
>   	replay.quiet = !(opts->flags & REBASE_NO_QUIET);
>   	replay.verbose = opts->flags & REBASE_VERBOSE;
> +	replay.precious_todo = opts->flags & REBASE_INTERACTIVE_EXPLICIT;
>   	replay.reschedule_failed_exec = opts->reschedule_failed_exec;
>   	replay.committer_date_is_author_date =
>   					opts->committer_date_is_author_date;
> diff --git a/sequencer.c b/sequencer.c
> index b1c29c8802..f8a7f4e721 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -4570,6 +4570,10 @@ static int checkout_onto(struct repository *r, struct replay_opts *opts,
>   		.default_reflog_action = sequencer_reflog_action(opts)
>   	};
>   	if (reset_head(r, &ropts)) {
> +		// Editing the todo may have been costly; don't just discard it.
> +		if (opts->precious_todo)
> +			exit(1);  // Error was already printed
> +
>   		apply_autostash(rebase_path_autostash());
>   		sequencer_remove_state(opts);
>   		return error(_("could not detach HEAD"));
> diff --git a/sequencer.h b/sequencer.h
> index 1a3e616af2..a1b8ca6eb1 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -47,6 +47,7 @@ struct replay_opts {
>   	int keep_redundant_commits;
>   	int verbose;
>   	int quiet;
> +	int precious_todo;
>   	int reschedule_failed_exec;
>   	int committer_date_is_author_date;
>   	int ignore_date;
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index ff0afad63e..c625aad10a 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -288,13 +288,14 @@ test_expect_success 'abort' '
>   '
>   
>   test_expect_success 'abort with error when new base cannot be checked out' '
> +	test_when_finished "git rebase --abort ||:" &&
>   	git rm --cached file1 &&
>   	git commit -m "remove file in base" &&
>   	test_must_fail git rebase -i primary > output 2>&1 &&
>   	test_i18ngrep "The following untracked working tree files would be overwritten by checkout:" \
>   		output &&
>   	test_i18ngrep "file1" output &&
> -	test_path_is_missing .git/rebase-merge &&
> +	test_path_is_dir .git/rebase-merge &&
>   	rm file1 &&
>   	git reset --hard HEAD^
>   '

>On 23/03/2023 16:22, Oswald Buddenhagen wrote:
>> Creating a suitable todo file is a potentially labor-intensive process,
>> so be less cavalier about discarding it when something goes wrong (e.g.,
>> the user messed with the repo while editing the todo).
>
>I was thinking about this problem the other day in the context of 
>rescheduling commands when they cannot be executed because they would 
>overwrite an untracked file. My thought was that we should prepend a 
>"reset" command to the todo list so that the checkout happened when the 
>user continued the rebase.
>

> On Thu, Mar 23, 2023 at 07:31:04PM +0000, Phillip Wood wrote:
>> On 23/03/2023 16:22, Oswald Buddenhagen wrote:
>>> Creating a suitable todo file is a potentially labor-intensive process,
>>> so be less cavalier about discarding it when something goes wrong (e.g.,
>>> the user messed with the repo while editing the todo).
>>
>> I was thinking about this problem the other day in the context of 
>> rescheduling commands when they cannot be executed because they would 
>> overwrite an untracked file. My thought was that we should prepend a 
>> "reset" command to the todo list so that the checkout happened when 
>> the user continued the rebase.
>>
> so you basically want to convert the magic `onto` into an explicit todo 
> command? i'm not sure what the advantage would be, and i certainly can 
> think of disadvantages re. usability and backwards compat.

>> How does this patch ensure the checkout happens when the user 
>> continues the rebase?
>>
> the idea was never that the user --continue's. we're talking about a 
> fatal error,

>On 23/03/2023 22:38, Oswald Buddenhagen wrote:
>> so you basically want to convert the magic `onto` into an explicit 
>> todo command? i'm not sure what the advantage would be, and i 
>> certainly can think of disadvantages re. usability and backwards 
>> compat.
>
>If the initial checkout of "onto" fails I want the rebase to stop so the 
>user can try and fix the problem (usually remove an untracked file) and 
>then run "git rebase --continue" to continue the rebase including the 
>initial checkout. Adding a "reset" command to the beginning of the todo 
>list when the initial checkout fails is one way of achieving that.
>

>>> How does this patch ensure the checkout happens when the user 
>>> continues the rebase?
>>>
>> the idea was never that the user --continue's. we're talking about a 
>> fatal error,
>
>If it is a fatal error what is stopping the user from running "rebase 
>--continue" and wreaking havoc? [...]
>
>It would be much more user friendly to simply allow them to fix the 
>problem with the checkout and run "git rebase --continue"
>

> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index a309addd50..728c869db4 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -153,6 +153,7 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)
>  	replay.keep_redundant_commits = (opts->empty == EMPTY_KEEP);
>  	replay.quiet = !(opts->flags & REBASE_NO_QUIET);
>  	replay.verbose = opts->flags & REBASE_VERBOSE;
> +	replay.precious_todo = opts->flags & REBASE_INTERACTIVE_EXPLICIT;
>  	replay.reschedule_failed_exec = opts->reschedule_failed_exec;
>  	replay.committer_date_is_author_date =
>  					opts->committer_date_is_author_date;
> diff --git a/sequencer.c b/sequencer.c
> index b1c29c8802..f8a7f4e721 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -4570,6 +4570,10 @@ static int checkout_onto(struct repository *r, struct replay_opts *opts,
>  		.default_reflog_action = sequencer_reflog_action(opts)
>  	};
>  	if (reset_head(r, &ropts)) {
> +		// Editing the todo may have been costly; don't just discard it.
> +		if (opts->precious_todo)
> +			exit(1);  // Error was already printed

> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index ff0afad63e..c625aad10a 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -288,13 +288,14 @@ test_expect_success 'abort' '
>  '
>  
>  test_expect_success 'abort with error when new base cannot be checked out' '
> +	test_when_finished "git rebase --abort ||:" &&
>  	git rm --cached file1 &&
>  	git commit -m "remove file in base" &&
>  	test_must_fail git rebase -i primary > output 2>&1 &&
>  	test_i18ngrep "The following untracked working tree files would be overwritten by checkout:" \
>  		output &&
>  	test_i18ngrep "file1" output &&
> -	test_path_is_missing .git/rebase-merge &&
> +	test_path_is_dir .git/rebase-merge &&
>  	rm file1 &&
>  	git reset --hard HEAD^
>  '

>Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
>
>> Creating a suitable todo file is a potentially labor-intensive process,
>> so be less cavalier about discarding it when something goes wrong (e.g.,
>> the user messed with the repo while editing the todo).
>
>Is there a reason why we do not always keep it?  Why is the file
>sometimes precious but not precious at all in other times?
>

>Tying the previous bit to "-i was explicitly given" feels a bit
>unintuitive---when the sequencer machinery was implicitly chosen,
>and gives the control back to the user, should a user be forbidden
>to muck with the todo list?
>

>> -	test_path_is_missing .git/rebase-merge &&
>> +	test_path_is_dir .git/rebase-merge &&
>
>Are we happy to just see that the directory still exists?  I thought
>the original motivation explained in the proposed log message was to
>keep the todo list file, so shouldn't you be checking if the file is
>there
>

> On Thu, Mar 23, 2023 at 01:16:47PM -0700, Junio C Hamano wrote:
>>Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
>>
>>> Creating a suitable todo file is a potentially labor-intensive process,
>>> so be less cavalier about discarding it when something goes wrong (e.g.,
>>> the user messed with the repo while editing the todo).
>>
>>Is there a reason why we do not always keep it?  Why is the file
>>sometimes precious but not precious at all in other times?
>>
> the unedited initial todo just isn't precious. that implies that in a
> non-interactive rebase, it is always worthless at the time of the
> initial reset.

>>(and if you can reliably ensure that the file has contents
>>that are expected, that would be even better)?
>>
> i could grep for a shortened sha1 i would obtain from the branch. but
> given that the error scenario of a present but somehow corrupted todo
> seems implausible given the circumstances, that seems like overkill.

> ... instead of deriving it from other arguments. This is a lot cleaner
> and more extensible.
> 
> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
> ---
>   builtin/rebase.c     | 25 ++++++++++---------------
>   rebase-interactive.c | 21 +++++++++++----------
>   rebase-interactive.h | 16 ++++++++++++++--
>   sequencer.c          | 16 +++++++++-------
>   sequencer.h          |  8 +++++---
>   5 files changed, 49 insertions(+), 37 deletions(-)
> 
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 491759db19..a309addd50 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -58,16 +58,6 @@ enum empty_type {
>   	EMPTY_ASK
>   };
>   
> -enum action {
> -	ACTION_NONE = 0,
> -	ACTION_CONTINUE,
> -	ACTION_SKIP,
> -	ACTION_ABORT,
> -	ACTION_QUIT,
> -	ACTION_EDIT_TODO,
> -	ACTION_SHOW_CURRENT_PATCH
> -};
> -
>   static const char *action_names[] = {
>   	"undefined",
>   	"continue",
> @@ -104,7 +94,7 @@ struct rebase_options {
>   		REBASE_INTERACTIVE_EXPLICIT = 1<<4,
>   	} flags;
>   	struct strvec git_am_opts;
> -	enum action action;
> +	enum rebase_action action;
>   	char *reflog_action;
>   	int signoff;
>   	int allow_rerere_autoupdate;
> @@ -198,9 +188,11 @@ static int edit_todo_file(unsigned flags)
>   		return error_errno(_("could not read '%s'."), todo_file);
>   
>   	strbuf_stripspace(&todo_list.buf, 1);
> -	res = edit_todo_list(the_repository, &todo_list, &new_todo, NULL, NULL, flags);
> +	res = edit_todo_list(the_repository, &todo_list, &new_todo, NULL, NULL, flags,
> +			     ACTION_EDIT_TODO);
>   	if (!res && todo_list_write_to_file(the_repository, &new_todo, todo_file,
> -					    NULL, NULL, -1, flags & ~(TODO_LIST_SHORTEN_IDS)))
> +					    NULL, NULL, -1, flags & ~(TODO_LIST_SHORTEN_IDS),
> +					    ACTION_EDIT_TODO))
>   		res = error_errno(_("could not write '%s'"), todo_file);
>   
>   	todo_list_release(&todo_list);
> @@ -294,7 +286,8 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
>   		ret = complete_action(the_repository, &replay, flags,
>   			shortrevisions, opts->onto_name, opts->onto,
>   			&opts->orig_head->object.oid, &opts->exec,
> -			opts->autosquash, opts->update_refs, &todo_list);
> +			opts->autosquash, opts->update_refs, &todo_list,
> +			opts->action);
>   	}
>   
>   cleanup:
> @@ -1246,7 +1239,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   		else if (options.exec.nr)
>   			trace2_cmd_mode("interactive-exec");
>   		else
> -			trace2_cmd_mode(action_names[options.action]);
> +			trace2_cmd_mode(
> +				(BUILD_ASSERT_OR_ZERO(ARRAY_SIZE(action_names) == ACTION_LAST),
> +				 action_names[options.action]));
>   	}
>   
>   	options.reflog_action = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);
> diff --git a/rebase-interactive.c b/rebase-interactive.c
> index 7407c59319..111a2071ae 100644
> --- a/rebase-interactive.c
> +++ b/rebase-interactive.c
> @@ -35,7 +35,7 @@ static enum missing_commit_check_level get_missing_commit_check_level(void)
>   	return MISSING_COMMIT_CHECK_IGNORE;
>   }
>   
> -void append_todo_help(int command_count,
> +void append_todo_help(int command_count, enum rebase_action action,
>   		      const char *shortrevisions, const char *shortonto,
>   		      struct strbuf *buf)
>   {
> @@ -62,9 +62,8 @@ void append_todo_help(int command_count,
>   "                      updated at the end of the rebase\n"
>   "\n"
>   "These lines can be re-ordered; they are executed from top to bottom.\n");
> -	unsigned edit_todo = !(shortrevisions && shortonto);
>   
> -	if (!edit_todo) {
> +	if (action == ACTION_NONE) {
>   		strbuf_addch(buf, '\n');
>   		strbuf_commented_addf(buf, Q_("Rebase %s onto %s (%d command)",
>   					      "Rebase %s onto %s (%d commands)",
> @@ -83,7 +82,7 @@ void append_todo_help(int command_count,
>   
>   	strbuf_add_commented_lines(buf, msg, strlen(msg));
>   
> -	if (edit_todo)
> +	if (action == ACTION_EDIT_TODO)
>   		msg = _("\nYou are editing the todo file "
>   			"of an ongoing interactive rebase.\n"
>   			"To continue rebase after editing, run:\n"
> @@ -97,35 +96,37 @@ void append_todo_help(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)
> +		   const char *shortonto, unsigned flags,
> +		   enum rebase_action action)
>   {
>   	const char *todo_file = rebase_path_todo(),
>   		*todo_backup = rebase_path_todo_backup();
> -	unsigned initial = shortrevisions && shortonto;
>   	int incorrect = 0;
>   
>   	/* If the user is editing the todo list, we first try to parse
>   	 * it.  If there is an error, we do not return, because the user
>   	 * might want to fix it in the first place. */
> -	if (!initial)
> +	if (action != ACTION_NONE)
>   		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))
> +				    -1, flags | TODO_LIST_SHORTEN_IDS | TODO_LIST_APPEND_TODO_HELP,
> +				    action))
>   		return error_errno(_("could not write '%s'"), todo_file);
>   
>   	if (!incorrect &&
>   	    todo_list_write_to_file(r, todo_list, todo_backup,
>   				    shortrevisions, shortonto, -1,
> -				    (flags | TODO_LIST_APPEND_TODO_HELP) & ~TODO_LIST_SHORTEN_IDS) < 0)
> +				    (flags | TODO_LIST_APPEND_TODO_HELP) & ~TODO_LIST_SHORTEN_IDS,
> +				    action) < 0)
>   		return error(_("could not write '%s'."), rebase_path_todo_backup());
>   
>   	if (launch_sequence_editor(todo_file, &new_todo->buf, NULL))
>   		return -2;
>   
>   	strbuf_stripspace(&new_todo->buf, 1);
> -	if (initial && new_todo->buf.len == 0)
> +	if (action != ACTION_EDIT_TODO && new_todo->buf.len == 0)
>   		return -3;
>   
>   	if (todo_list_parse_insn_buffer(r, new_todo->buf.buf, new_todo)) {
> diff --git a/rebase-interactive.h b/rebase-interactive.h
> index 7239c60f79..d9873d3497 100644
> --- a/rebase-interactive.h
> +++ b/rebase-interactive.h
> @@ -5,12 +5,24 @@ struct strbuf;
>   struct repository;
>   struct todo_list;
>   
> -void append_todo_help(int command_count,
> +enum rebase_action {
> +	ACTION_NONE = 0,
> +	ACTION_CONTINUE,
> +	ACTION_SKIP,
> +	ACTION_ABORT,
> +	ACTION_QUIT,
> +	ACTION_EDIT_TODO,
> +	ACTION_SHOW_CURRENT_PATCH,
> +	ACTION_LAST
> +};
> +
> +void append_todo_help(int command_count, enum rebase_action action,
>   		      const char *shortrevisions, const char *shortonto,
>   		      struct strbuf *buf);
>   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 *shortonto, unsigned flags,
> +		   enum rebase_action action);
>   
>   int todo_list_check(struct todo_list *old_todo, struct todo_list *new_todo);
>   int todo_list_check_against_backup(struct repository *r,
> diff --git a/sequencer.c b/sequencer.c
> index 7c275c9a65..f05174d151 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -5894,14 +5894,15 @@ static void todo_list_to_strbuf(struct repository *r, struct todo_list *todo_lis
>   
>   int todo_list_write_to_file(struct repository *r, struct todo_list *todo_list,
>   			    const char *file, const char *shortrevisions,
> -			    const char *shortonto, int num, unsigned flags)
> +			    const char *shortonto, int num, unsigned flags,
> +			    enum rebase_action action)
>   {
>   	int res;
>   	struct strbuf buf = STRBUF_INIT;
>   
>   	todo_list_to_strbuf(r, todo_list, &buf, num, flags);
>   	if (flags & TODO_LIST_APPEND_TODO_HELP)
> -		append_todo_help(count_commands(todo_list),
> +		append_todo_help(count_commands(todo_list), action,
>   				 shortrevisions, shortonto, &buf);
>   
>   	res = write_message(buf.buf, buf.len, file, 0);
> @@ -5941,7 +5942,8 @@ static int skip_unnecessary_picks(struct repository *r,
>   	if (i > 0) {
>   		const char *done_path = rebase_path_done();
>   
> -		if (todo_list_write_to_file(r, todo_list, done_path, NULL, NULL, i, 0)) {
> +		if (todo_list_write_to_file(r, todo_list, done_path, NULL, NULL, i, 0,
> +					    ACTION_NONE)) {
>   			error_errno(_("could not write to '%s'"), done_path);
>   			return -1;
>   		}
> @@ -6086,8 +6088,8 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
>   		    const char *shortrevisions, const char *onto_name,
>   		    struct commit *onto, const struct object_id *orig_head,
>   		    struct string_list *commands, unsigned autosquash,
> -		    unsigned update_refs,
> -		    struct todo_list *todo_list)
> +		    unsigned update_refs, struct todo_list *todo_list,
> +		    enum rebase_action action)
>   {
>   	char shortonto[GIT_MAX_HEXSZ + 1];
>   	const char *todo_file = rebase_path_todo();
> @@ -6122,7 +6124,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
>   	}
>   
>   	res = edit_todo_list(r, todo_list, &new_todo, shortrevisions,
> -			     shortonto, flags);
> +			     shortonto, flags, action);
>   	if (res == -1)
>   		return -1;
>   	else if (res == -2) {
> @@ -6158,7 +6160,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
>   	}
>   
>   	if (todo_list_write_to_file(r, &new_todo, todo_file, NULL, NULL, -1,
> -				    flags & ~(TODO_LIST_SHORTEN_IDS))) {
> +				    flags & ~(TODO_LIST_SHORTEN_IDS), action)) {
>   		todo_list_release(&new_todo);
>   		return error_errno(_("could not write '%s'"), todo_file);
>   	}
> diff --git a/sequencer.h b/sequencer.h
> index 33dbaf5b66..1a3e616af2 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -4,6 +4,7 @@
>   #include "strbuf.h"
>   #include "wt-status.h"
>   
> +enum rebase_action;
>   struct commit;
>   struct index_state;
>   struct repository;
> @@ -134,7 +135,8 @@ int todo_list_parse_insn_buffer(struct repository *r, char *buf,
>   				struct todo_list *todo_list);
>   int todo_list_write_to_file(struct repository *r, struct todo_list *todo_list,
>   			    const char *file, const char *shortrevisions,
> -			    const char *shortonto, int num, unsigned flags);
> +			    const char *shortonto, int num, unsigned flags,
> +			    enum rebase_action action);
>   void todo_list_release(struct todo_list *todo_list);
>   const char *todo_item_get_arg(struct todo_list *todo_list,
>   			      struct todo_item *item);
> @@ -187,8 +189,8 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
>   		    const char *shortrevisions, const char *onto_name,
>   		    struct commit *onto, const struct object_id *orig_head,
>   		    struct string_list *commands, unsigned autosquash,
> -		    unsigned update_refs,
> -		    struct todo_list *todo_list);
> +		    unsigned update_refs, struct todo_list *todo_list,
> +		    enum rebase_action action);
>   int todo_list_rearrange_squash(struct todo_list *todo_list);
>   
>   /*

> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
> ---
>   sequencer.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index f8a7f4e721..fb224445fa 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -6225,7 +6225,7 @@ static int skip_fixupish(const char *subject, const char **p) {
>   int todo_list_rearrange_squash(struct todo_list *todo_list)
>   {
>   	struct hashmap subject2item;
> -	int rearranged = 0, *next, *tail, i, nr = 0, alloc = 0;
> +	int rearranged = 0, *next, *tail, i, nr = 0;
>   	char **subjects;
>   	struct commit_todo_item commit_todo;
>   	struct todo_item *items = NULL;
> @@ -6334,6 +6334,8 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)
>   	}
>   
>   	if (rearranged) {
> +		items = ALLOC_ARRAY(items, todo_list->nr);
> +
>   		for (i = 0; i < todo_list->nr; i++) {
>   			enum todo_command command = todo_list->items[i].command;
>   			int cur = i;
> @@ -6346,16 +6348,15 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)
>   				continue;
>   
>   			while (cur >= 0) {
> -				ALLOC_GROW(items, nr + 1, alloc);
>   				items[nr++] = todo_list->items[cur];
>   				cur = next[cur];
>   			}
>   		}
>   
> +		assert(nr == todo_list->nr);

>> +		assert(nr == todo_list->nr);
>
>If this assert fails we may have already had some out of bounds memory 
>accesses.
>

>> +		todo_list->alloc = nr;
>>   		FREE_AND_NULL(todo_list->items);
>
>I think it would be cleaner to keep the original ordering and free the 
>old list before assigning todo_list->alloc
>

>>   		todo_list->items = items;
>> -		todo_list->nr = nr;
>> -		todo_list->alloc = alloc;
>>   	}
>

>   int complete_action(struct repository *r, struct replay_opts *opts, unsigned flags,
>   		    const char *shortrevisions, const char *onto_name,
> -		    struct commit *onto, const struct object_id *orig_head,
> +		    const struct object_id *onto, const struct object_id *orig_head,
>   		    struct string_list *commands, unsigned autosquash,
>   		    unsigned update_refs, struct todo_list *todo_list,
>   		    enum rebase_action action)
>   {
>   	char shortonto[GIT_MAX_HEXSZ + 1];
>   	const char *todo_file = rebase_path_todo();
>   	struct todo_list new_todo = TODO_LIST_INIT;
>   	struct strbuf *buf = &todo_list->buf, buf2 = STRBUF_INIT;
> -	struct object_id oid = onto->object.oid;

> @@ -6158,7 +6157,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
>   		BUG("invalid todo list after expanding IDs:\n%s",
>   		    new_todo.buf.buf);
>   
> -	if (opts->allow_ff && skip_unnecessary_picks(r, &new_todo, &oid)) {

> +	if (opts->allow_ff && skip_unnecessary_picks(r, &new_todo, &onto)) {
>   		todo_list_release(&new_todo);
>   		return error(_("could not skip unnecessary pick commands"));
>   	}
> @@ -6171,7 +6170,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
>   
>   	res = -1;
>   
> -	if (checkout_onto(r, opts, onto_name, &oid, orig_head))
> +	if (checkout_onto(r, opts, onto_name, onto, orig_head))
>   		goto cleanup;
>   
>   	if (require_clean_work_tree(r, "rebase", NULL, 1, 1))
> diff --git a/sequencer.h b/sequencer.h
> index a1b8ca6eb1..24bf71d5db 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -188,7 +188,7 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
>   
>   int complete_action(struct repository *r, struct replay_opts *opts, unsigned flags,
>   		    const char *shortrevisions, const char *onto_name,
> -		    struct commit *onto, const struct object_id *orig_head,
> +		    const struct object_id *onto, const struct object_id *orig_head,
>   		    struct string_list *commands, unsigned autosquash,
>   		    unsigned update_refs, struct todo_list *todo_list,
>   		    enum rebase_action action);

>On 23/03/2023 16:22, Oswald Buddenhagen wrote:
>> ... instead of as a commit, which makes the purpose clearer and will
>> simplify things later.
>
>given that we want onto to be a commit I'm not sure how this makes 
>anything clearer.
>

>> As a side effect, this change revealed that skip_unnecessary_picks() was
>> butchering the commit object due to missing const-correctness. Slightly
>> adjust its API to rectify this.
>
>I don't think this is correct. If you look at the original code it makes 
>a copy of the oid and uses the copy when calling skip_unnecessary_picks()
>

> On Thu, Mar 23, 2023 at 07:34:57PM +0000, Phillip Wood wrote:
>> On 23/03/2023 16:22, Oswald Buddenhagen wrote:
>>> ... instead of as a commit, which makes the purpose clearer and will
>>> simplify things later.
>>
>> given that we want onto to be a commit I'm not sure how this makes 
>> anything clearer.
>>
> it makes it clearer that we need only the oid, not any other part of the 
> commit object. and pulling ahead the "extraction" reduces the visual 
> noise further down.
> 
>>> As a side effect, this change revealed that skip_unnecessary_picks() was
>>> butchering the commit object due to missing const-correctness. Slightly
>>> adjust its API to rectify this.
>>
>> I don't think this is correct. If you look at the original code it 
>> makes a copy of the oid and uses the copy when calling 
>> skip_unnecessary_picks()
>>
> oops, you're quite right. (facepalm)
> imo the change still makes sense, though, as it replaces the relatively 
> expensive deep copies with simple pointer updates. so just fix the 
> commit message?

> This is a preparatory series for the separately posted 'rebase --rewind' patch,
> but I think it has value in itself.
> 
> 
> Oswald Buddenhagen (8):
>    rebase: simplify code related to imply_merge()
>    rebase: move parse_opt_keep_empty() down
>    sequencer: pass around rebase action explicitly
>    sequencer: create enum for edit_todo_list() return value
>    rebase: preserve interactive todo file on checkout failure
>    sequencer: simplify allocation of result array in
>      todo_list_rearrange_squash()
>    sequencer: pass `onto` to complete_action() as object-id
>    rebase: improve resumption from incorrect initial todo list
> 
>   builtin/rebase.c              |  63 +++++++--------
>   builtin/revert.c              |   3 +-
>   rebase-interactive.c          |  36 ++++-----
>   rebase-interactive.h          |  27 ++++++-
>   sequencer.c                   | 139 +++++++++++++++++++---------------
>   sequencer.h                   |  15 ++--
>   t/t3404-rebase-interactive.sh |  34 ++++++++-
>   7 files changed, 196 insertions(+), 121 deletions(-)
> 

> 
> Oswald Buddenhagen (8):
>    rebase: simplify code related to imply_merge()
>    rebase: move parse_opt_keep_empty() down
>    sequencer: pass around rebase action explicitly
>    sequencer: create enum for edit_todo_list() return value
>    rebase: preserve interactive todo file on checkout failure
>    sequencer: simplify allocation of result array in
>      todo_list_rearrange_squash()
>    sequencer: pass `onto` to complete_action() as object-id
>    rebase: improve resumption from incorrect initial todo list
> 
>   builtin/rebase.c              |  63 +++++++--------
>   builtin/revert.c              |   3 +-
>   rebase-interactive.c          |  36 ++++-----
>   rebase-interactive.h          |  27 ++++++-
>   sequencer.c                   | 139 +++++++++++++++++++---------------
>   sequencer.h                   |  15 ++--
>   t/t3404-rebase-interactive.sh |  34 ++++++++-
>   7 files changed, 196 insertions(+), 121 deletions(-)
> 

> Hi Oswald
> 
> On 23/03/2023 16:22, Oswald Buddenhagen wrote:
>> This is a preparatory series for the separately posted 'rebase 
>> --rewind' patch,
>> but I think it has value in itself.
> 
> I had a hard time applying these patches. In the end I had success with 
> checking out next, applying 
> https://lore.kernel.org/git/20230323162234.995514-1-oswald.buddenhagen@gmx.de and then applying this series. It is very helpful to detail the base commit in the cover letter. A series like this should normally be based on master see Documentation/SubmittingPatches.
> 
> Having applied the patches I'm unable to compile them with DEVELOPER=1 
> (see Documentation/CodingGuidelines)
> 
> In file included from log-tree.c:20:
> sequencer.h:7:6: error: ISO C forbids forward references to ‘enum’ types 
> [-Werror=pedantic]
>      7 | enum rebase_action;
>        |      ^~~~~~~~~~~~~
> sequencer.h:140:34: error: ISO C forbids forward references to ‘enum’ 
> types [-Werror=pedantic]
>    140 |                             enum rebase_action action);
>        |                                  ^~~~~~~~~~~~~
> sequencer.h:196:26: error: ISO C forbids forward references to ‘enum’ 
> types [-Werror=pedantic]
>    196 |                     enum rebase_action action);
>        |                          ^~~~~~~~~~~~~
> 
> In file included from ./cache.h:12,
>                   from ./builtin.h:6,
>                   from builtin/rebase.c:8:
> builtin/rebase.c: In function ‘cmd_rebase’:
> builtin/rebase.c:1246:95: error: left-hand operand of comma expression 
> has no effect [-Werror=unused-value]
>   1246 | (BUILD_ASSERT_OR_ZERO(ARRAY_SIZE(action_names) == ACTION_LAST),
>        |                               ^
> ./trace2.h:158:69: note: in definition of macro ‘trace2_cmd_mode’
>    158 | #define trace2_cmd_mode(sv) trace2_cmd_mode_fl(__FILE__, 
> __LINE__, (sv))
>        |     ^~

> 
> Best Wishes
> 
> Phillip
> 
>>
>> Oswald Buddenhagen (8):
>>    rebase: simplify code related to imply_merge()
>>    rebase: move parse_opt_keep_empty() down
>>    sequencer: pass around rebase action explicitly
>>    sequencer: create enum for edit_todo_list() return value
>>    rebase: preserve interactive todo file on checkout failure
>>    sequencer: simplify allocation of result array in
>>      todo_list_rearrange_squash()
>>    sequencer: pass `onto` to complete_action() as object-id
>>    rebase: improve resumption from incorrect initial todo list
>>
>>   builtin/rebase.c              |  63 +++++++--------
>>   builtin/revert.c              |   3 +-
>>   rebase-interactive.c          |  36 ++++-----
>>   rebase-interactive.h          |  27 ++++++-
>>   sequencer.c                   | 139 +++++++++++++++++++---------------
>>   sequencer.h                   |  15 ++--
>>   t/t3404-rebase-interactive.sh |  34 ++++++++-
>>   7 files changed, 196 insertions(+), 121 deletions(-)
>>

> 
> Oswald Buddenhagen (8):
>    rebase: simplify code related to imply_merge()
>    rebase: move parse_opt_keep_empty() down
>    sequencer: pass around rebase action explicitly
>    sequencer: create enum for edit_todo_list() return value
>    rebase: preserve interactive todo file on checkout failure
>    sequencer: simplify allocation of result array in
>      todo_list_rearrange_squash()
>    sequencer: pass `onto` to complete_action() as object-id
>    rebase: improve resumption from incorrect initial todo list
> 
>   builtin/rebase.c              |  63 +++++++--------
>   builtin/revert.c              |   3 +-
>   rebase-interactive.c          |  36 ++++-----
>   rebase-interactive.h          |  27 ++++++-
>   sequencer.c                   | 139 +++++++++++++++++++---------------
>   sequencer.h                   |  15 ++--
>   t/t3404-rebase-interactive.sh |  34 ++++++++-
>   7 files changed, 196 insertions(+), 121 deletions(-)
>