threads / patch / 63613

patchrebase: write script before initializing state

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

## tl;dr

8 messages between Jun 9, 2025 and Jul 24, 2025. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

Øystein Walle· Jun 9, 2025, 22:10 UTC · lore

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(-)
Show changes to 2 files +31 −21

builtin/rebase.c, t/t3415-rebase-autosquash.sh

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
Junio C Hamano· Jun 9, 2025, 23:03 UTC · re: Øystein Walle · lore

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

Ø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.
Show 6 quoted lines
> 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.

Show changes to 2 files +31 −21

builtin/rebase.c, t/t3415-rebase-autosquash.sh

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")"
Phillip Wood· Jun 10, 2025, 10:13 UTC · re: Øystein Walle · lore

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

Hi Øystein
On 09/06/2025 23:10, Øystein Walle wrote:
Show 18 quoted lines
> 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.
Show 13 quoted lines
> 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
Show 89 quoted lines
> 
>   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")"
Junio C Hamano· Jul 9, 2025, 00:14 UTC · re: Øystein Walle · lore

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

Øystein Walle <oystwa@gmail.com> writes:
Show 32 quoted lines
> 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.
Øystein Walle· Jul 11, 2025, 20:36 UTC · re: Junio C Hamano · lore

[PATCH v2] rebase: write script before initializing state

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(-)
Show changes to 2 files +31 −21

builtin/rebase.c, t/t3415-rebase-autosquash.sh

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
Junio C Hamano· Jul 11, 2025, 21:25 UTC · re: Øystein Walle · lore

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

Øystein Walle <oystwa@gmail.com> writes:
Show 28 quoted lines
> 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.
Show 5 quoted lines
> 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.
Show 88 quoted lines
>  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")"
Phillip Wood· Jul 24, 2025, 14:22 UTC · re: Junio C Hamano · lore

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

On 11/07/2025 22:25, Junio C Hamano wrote:
Show 42 quoted lines
> Ø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
Show 88 quoted lines
>>   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")"
Junio C Hamano· Jul 23, 2025, 21:34 UTC · re: Øystein Walle · lore

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

Øystein Walle <oystwa@gmail.com> writes:
Show 34 quoted lines
> 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.

← back to recent threads