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

[Phillip Wood <phillip.wood123@xxxxxxxxx>]

On 11/07/2025 22:25, Junio C Hamano wrote:
> Øystein Walle <oystwa@xxxxxxxxx> writes:
>
> > If rebase.instructionFormat is invalid the repository is left in a
> > strange state when the interactive rebase fails. `git status` outputs
> > both the same as it would have in the normal case *and* something
> > related to the 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)
> >
> > get_commit_format() calls die() on failure so we cannot handle the error
> > gracefully. By attempting to write the rebase script before initializing
> > the state this bad state can be avoided.
> >
> > Signed-off-by: Øystein Walle <oystwa@xxxxxxxxx>
> > ---
> > So sorry for the delay. I saw that the signoff was missing, then saw
> > Phillip's review, decided to think about it and then life happened in
> > the mean time...
>
> No need to be sorry.  Life happens, indeed.
>
> > This patch is identical to the first one except it has the missing
> > signoff and a few typos in the commit message corrected. Phillip's
> > suggestions are noted and appreciated but unfortunately I am unable to
> > work on the at the moment. And I do think my patch is at least an
> > improvement albeit perhaps less thorough than it could have been.
>
> Well, as long as we are making a step in the right direction, such a
> partial improvement gives us a better foundation for somebody else
> to further build on.

I don't think this is a better (or worse) foundation for future 
improvement as solving this when the user passes '--autostash' needs a 
different approach. When that option is given we create the state 
directory earlier so moving when we call init_basic_state() has no effect.

>  It does not look like that this patch would
> make it harder to later give us a more thorough solution.

I agree with this. I don't think this change makes fixing the 
'--autostash' case harder, but fixing that case makes this change 
redundant.
> Thanks for working on the topic.

Yes, thank you Øystein

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