[PATCH] rebase: write script before initializing state

[PATCH] rebase: write script before initializing state

[Øystein Walle]

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)

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.

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

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

[Junio C Hamano]

Øystein Walle <oystwa@gmail.com> writes:

> ...
> By attempting to write the rebase script before initializing the state
> this potential scenario is avoided.
> ---

Missing sign-off.

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

"git show --histogram" may do a lot better job than the default
algorithm used by "git show" for this patch.

diff --git a/builtin/rebase.c b/builtin/rebase.c
index d4715ed35d..06da8e4f36 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -283,53 +283,53 @@ static int init_basic_state(struct replay_opts *opts, const char *head_name,
 static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
 {
 	int ret = -1;
 	char *revisions = NULL, *shortrevisions = NULL;
 	struct strvec make_script_args = STRVEC_INIT;
 	struct todo_list todo_list = TODO_LIST_INIT;
 	struct replay_opts replay = get_replay_opts(opts);
 
 	if (get_revision_ranges(opts->upstream, opts->onto, &opts->orig_head->object.oid,
 				&revisions, &shortrevisions))
 		goto cleanup;
 
+	strvec_pushl(&make_script_args, "", revisions, NULL);
+	if (opts->restrict_revision)
+		strvec_pushf(&make_script_args, "^%s",
+			     oid_to_hex(&opts->restrict_revision->object.oid));
+
+	ret = sequencer_make_script(the_repository, &todo_list.buf,
+				    make_script_args.nr, make_script_args.v,
+				    flags);
+	if (ret) {
+		error(_("could not generate 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));
 
-	strvec_pushl(&make_script_args, "", revisions, NULL);
-	if (opts->restrict_revision)
-		strvec_pushf(&make_script_args, "^%s",
-			     oid_to_hex(&opts->restrict_revision->object.oid));
+	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 = sequencer_make_script(the_repository, &todo_list.buf,
-				    make_script_args.nr, make_script_args.v,
-				    flags);
-
-	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);
-	}
+	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);
 	free(shortrevisions);
 	todo_list_release(&todo_list);
 	strvec_clear(&make_script_args);
 
 	return ret;
 }
diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
index fcc40d6fe1..d4c624b841 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")"

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

[Phillip Wood]

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

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

[Junio C Hamano]

Øystein Walle <oystwa@gmail.com> writes:

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

The patch has been stalled for a few weeks since Phillip's review
comments.  What's the status of this?  Will we see a response and/or
an updated patch sometime soon?

Thanks.

[PATCH v2] rebase: write script before initializing state

[Øystein Walle]

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

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.

Øsse

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

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

[Junio C Hamano]

Øystein Walle <oystwa@gmail.com> 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@gmail.com>
> ---
> 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.  It does not look like that this patch would
make it harder to later give us a more thorough solution.

Thanks for working on the topic.

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

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

[Phillip Wood]

On 11/07/2025 22:25, Junio C Hamano wrote:
> Øystein Walle <oystwa@gmail.com> 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@gmail.com>
>> ---
>> 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")"

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

[Junio C Hamano]

Øystein Walle <oystwa@gmail.com> 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@gmail.com>
> ---
> 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...
>
> 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.

I am sweeping my backlog and noticed that nobody chimed in to help
improving this topic.  As I already said, this would not least be
moving a step in the right direction, so I am planning to mark it
for 'next', but thought that I should check first before doing so,
in case you are back on the topic and cooking a new iteration.

Thanks.