Re: [PATCH] rebase: write script before initializing state

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Øystein

On 09/06/2025 23:10, Øystein Walle wrote:
> If rebase.instructionFormat is invalid the repository is left in a
> strange state when the interactive rebase fails. `git status` outputs
> boths the same as it would in the normal case *and* something related to
> interactive rebase:
> 
>      $ git -c rebase.instructionFormat=blah rebase -i
>      fatal: invalid --pretty format: blah
>      $ git status
>      On branch master
>      Your branch is ahead of 'upstream/master' by 1 commit.
>        (use "git push" to publish your local commits)
> 
>      git-rebase-todo is missing.
>      No commands done.
>      No commands remaining.
>      You are currently editing a commit while rebasing branch 'master' on '8db3019401'.
>        (use "git commit --amend" to amend the current commit)
>        (use "git rebase --continue" once you are satisfied with your changes)

Thanks for working on this.

> By attempting to write the rebase script before initializing the state
> this potential scenario is avoided.
> ---
> The diff looks perhaps more messy than required. The only required
> change is the filling in of make_script_args and the call to
> sequencer_make_script() above the call to init_basic_state(). But then
> the `if (ret)` looks out of place, and moving that up means adding `goto
> cleanup` which means the code that was previously the else case can be
> dedented.
> 
> get_commit_format() calls die() in this case, so cleaning up the
> sequencer state isn't an option. Maybe it shouldn't call die in the
> first place, but that looks to be much larger change.

I don't think that should be too difficult and it is the only way to fix 
this that ensures we restore the stashed changes when the commit format 
is invalid. The test added in this patch should be updated to check that 
the changes stashed with '--autostash' are restored when the commit 
format is invalid.

Looking at the callers of get_commit_format() there are three in 
revision.c:handle_revision_opt() only one of which can fail. That can be 
converted to

	if (get_commit_format(...))
		die(NULL);

I think we can do the same for the caller in 
builtin/log.c:cmd_log_init_defaults() and should be straight forward to 
update the example in Documentation/MyFirstObjectWalk.adoc in a similar 
way. The caller in sequencer.c:sequencer_make_script() should propagate 
the error.

Note that to use "die(NULL)" you need to base you patch on the branch 
'ps/maintenance-ref-lock' which is currently in next.

I'm about to go off the list for a couple of weeks but I'm sure someone 
else will be happy to answer any questions that you have.

Best Wishes

Phillip

> 
>   builtin/rebase.c             | 42 ++++++++++++++++++------------------
>   t/t3415-rebase-autosquash.sh | 10 +++++++++
>   2 files changed, 31 insertions(+), 21 deletions(-)
> 
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 2e8c4ee678..8139816417 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -293,15 +293,6 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
>   				&revisions, &shortrevisions))
>   		goto cleanup;
>   
> -	if (init_basic_state(&replay,
> -			     opts->head_name ? opts->head_name : "detached HEAD",
> -			     opts->onto, &opts->orig_head->object.oid))
> -		goto cleanup;
> -
> -	if (!opts->upstream && opts->squash_onto)
> -		write_file(path_squash_onto(), "%s\n",
> -			   oid_to_hex(opts->squash_onto));
> -
>   	strvec_pushl(&make_script_args, "", revisions, NULL);
>   	if (opts->restrict_revision)
>   		strvec_pushf(&make_script_args, "^%s",
> @@ -310,21 +301,30 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
>   	ret = sequencer_make_script(the_repository, &todo_list.buf,
>   				    make_script_args.nr, make_script_args.v,
>   				    flags);
> -
> -	if (ret)
> +	if (ret) {
>   		error(_("could not generate todo list"));
> -	else {
> -		discard_index(the_repository->index);
> -		if (todo_list_parse_insn_buffer(the_repository, &replay,
> -						todo_list.buf.buf, &todo_list))
> -			BUG("unusable todo list");
> -
> -		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);
> +		goto cleanup;
>   	}
>   
> +	if (init_basic_state(&replay,
> +			     opts->head_name ? opts->head_name : "detached HEAD",
> +			     opts->onto, &opts->orig_head->object.oid))
> +		goto cleanup;
> +
> +	if (!opts->upstream && opts->squash_onto)
> +		write_file(path_squash_onto(), "%s\n",
> +			   oid_to_hex(opts->squash_onto));
> +
> +	discard_index(the_repository->index);
> +	if (todo_list_parse_insn_buffer(the_repository, &replay,
> +					todo_list.buf.buf, &todo_list))
> +		BUG("unusable todo list");
> +
> +	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);
> +
>   cleanup:
>   	replay_opts_release(&replay);
>   	free(revisions);
> diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
> index 26b42a526a..5d093e3a7a 100755
> --- a/t/t3415-rebase-autosquash.sh
> +++ b/t/t3415-rebase-autosquash.sh
> @@ -394,6 +394,16 @@ test_expect_success 'autosquash with empty custom instructionFormat' '
>   	)
>   '
>   
> +test_expect_success 'autosquash with invalid custom instructionFormat' '
> +	git reset --hard base &&
> +	test_commit invalid-instructionFormat-test &&
> +	(
> +		test_must_fail git -c rebase.instructionFormat=blah \
> +			rebase --autosquash  --force-rebase -i HEAD^ &&
> +		test_path_is_missing .git/rebase-merge
> +	)
> +'
> +
>   set_backup_editor () {
>   	write_script backup-editor.sh <<-\EOF
>   	cp "$1" .git/backup-"$(basename "$1")"