{"thread":{"id":"63613","subject":"[PATCH] rebase: write script before initializing state","startedAt":"2025-06-09T22:11:56Z","lastAt":"2025-07-24T14:22:18Z","messageCount":8,"participants":["Øystein Walle","Junio C Hamano","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"520021","messageId":"20250609221055.136074-1-oystwa@gmail.com","threadId":"63613","inReplyTo":null,"subject":"[PATCH] rebase: write script before initializing state","fromName":"Øystein Walle","fromEmail":"oystwa@gmail.com","sentAt":"2025-06-09T22:10:55Z","receivedAt":"2025-06-09T22:11:56Z","isPatch":true,"sender":{"key":"oystwa@gmail.com","avatar":"https://avatars.githubusercontent.com/u/794585?v=4"},"body":"If rebase.instructionFormat is invalid the repository is left in a\nstrange state when the interactive rebase fails. `git status` outputs\nboths the same as it would in the normal case *and* something related to\ninteractive rebase:\n\n    $ git -c rebase.instructionFormat=blah rebase -i\n    fatal: invalid --pretty format: blah\n    $ git status\n    On branch master\n    Your branch is ahead of 'upstream/master' by 1 commit.\n      (use \"git push\" to publish your local commits)\n\n    git-rebase-todo is missing.\n    No commands done.\n    No commands remaining.\n    You are currently editing a commit while rebasing branch 'master' on '8db3019401'.\n      (use \"git commit --amend\" to amend the current commit)\n      (use \"git rebase --continue\" once you are satisfied with your changes)\n\nBy attempting to write the rebase script before initializing the state\nthis potential scenario is avoided.\n---\nThe diff looks perhaps more messy than required. The only required\nchange is the filling in of make_script_args and the call to\nsequencer_make_script() above the call to init_basic_state(). But then\nthe `if (ret)` looks out of place, and moving that up means adding `goto\ncleanup` which means the code that was previously the else case can be\ndedented.\n\nget_commit_format() calls die() in this case, so cleaning up the\nsequencer state isn't an option. Maybe it shouldn't call die in the\nfirst place, but that looks to be much larger change.\n\n builtin/rebase.c             | 42 ++++++++++++++++++------------------\n t/t3415-rebase-autosquash.sh | 10 +++++++++\n 2 files changed, 31 insertions(+), 21 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 2e8c4ee678..8139816417 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -293,15 +293,6 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)\n \t\t\t\t&revisions, &shortrevisions))\n \t\tgoto cleanup;\n \n-\tif (init_basic_state(&replay,\n-\t\t\t     opts->head_name ? opts->head_name : \"detached HEAD\",\n-\t\t\t     opts->onto, &opts->orig_head->object.oid))\n-\t\tgoto cleanup;\n-\n-\tif (!opts->upstream && opts->squash_onto)\n-\t\twrite_file(path_squash_onto(), \"%s\\n\",\n-\t\t\t   oid_to_hex(opts->squash_onto));\n-\n \tstrvec_pushl(&make_script_args, \"\", revisions, NULL);\n \tif (opts->restrict_revision)\n \t\tstrvec_pushf(&make_script_args, \"^%s\",\n@@ -310,21 +301,30 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)\n \tret = sequencer_make_script(the_repository, &todo_list.buf,\n \t\t\t\t    make_script_args.nr, make_script_args.v,\n \t\t\t\t    flags);\n-\n-\tif (ret)\n+\tif (ret) {\n \t\terror(_(\"could not generate todo list\"));\n-\telse {\n-\t\tdiscard_index(the_repository->index);\n-\t\tif (todo_list_parse_insn_buffer(the_repository, &replay,\n-\t\t\t\t\t\ttodo_list.buf.buf, &todo_list))\n-\t\t\tBUG(\"unusable todo list\");\n-\n-\t\tret = complete_action(the_repository, &replay, flags,\n-\t\t\tshortrevisions, opts->onto_name, opts->onto,\n-\t\t\t&opts->orig_head->object.oid, &opts->exec,\n-\t\t\topts->autosquash, opts->update_refs, &todo_list);\n+\t\tgoto cleanup;\n \t}\n \n+\tif (init_basic_state(&replay,\n+\t\t\t     opts->head_name ? opts->head_name : \"detached HEAD\",\n+\t\t\t     opts->onto, &opts->orig_head->object.oid))\n+\t\tgoto cleanup;\n+\n+\tif (!opts->upstream && opts->squash_onto)\n+\t\twrite_file(path_squash_onto(), \"%s\\n\",\n+\t\t\t   oid_to_hex(opts->squash_onto));\n+\n+\tdiscard_index(the_repository->index);\n+\tif (todo_list_parse_insn_buffer(the_repository, &replay,\n+\t\t\t\t\ttodo_list.buf.buf, &todo_list))\n+\t\tBUG(\"unusable todo list\");\n+\n+\tret = complete_action(the_repository, &replay, flags,\n+\t\tshortrevisions, opts->onto_name, opts->onto,\n+\t\t&opts->orig_head->object.oid, &opts->exec,\n+\t\topts->autosquash, opts->update_refs, &todo_list);\n+\n cleanup:\n \treplay_opts_release(&replay);\n \tfree(revisions);\ndiff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh\nindex 26b42a526a..5d093e3a7a 100755\n--- a/t/t3415-rebase-autosquash.sh\n+++ b/t/t3415-rebase-autosquash.sh\n@@ -394,6 +394,16 @@ test_expect_success 'autosquash with empty custom instructionFormat' '\n \t)\n '\n \n+test_expect_success 'autosquash with invalid custom instructionFormat' '\n+\tgit reset --hard base &&\n+\ttest_commit invalid-instructionFormat-test &&\n+\t(\n+\t\ttest_must_fail git -c rebase.instructionFormat=blah \\\n+\t\t\trebase --autosquash  --force-rebase -i HEAD^ &&\n+\t\ttest_path_is_missing .git/rebase-merge\n+\t)\n+'\n+\n set_backup_editor () {\n \twrite_script backup-editor.sh <<-\\EOF\n \tcp \"$1\" .git/backup-\"$(basename \"$1\")\"\n-- \n2.43.0\n\n"},{"id":"520022","messageId":"xmqqa56gplfy.fsf@gitster.g","threadId":"63613","inReplyTo":"20250609221055.136074-1-oystwa@gmail.com","subject":"Re: [PATCH] rebase: write script before initializing state","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-09T23:03:13Z","receivedAt":"2025-06-09T23:03:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Øystein Walle <oystwa@gmail.com> writes:\n\n> ...\n> By attempting to write the rebase script before initializing the state\n> this potential scenario is avoided.\n> ---\n\nMissing sign-off.\n\n> The diff looks perhaps more messy than required. The only required\n> change is the filling in of make_script_args and the call to\n> sequencer_make_script() above the call to init_basic_state(). But then\n> the `if (ret)` looks out of place, and moving that up means adding `goto\n> cleanup` which means the code that was previously the else case can be\n> dedented.\n\n\"git show --histogram\" may do a lot better job than the default\nalgorithm used by \"git show\" for this patch.\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex d4715ed35d..06da8e4f36 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -283,53 +283,53 @@ static int init_basic_state(struct replay_opts *opts, const char *head_name,\n static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)\n {\n \tint ret = -1;\n \tchar *revisions = NULL, *shortrevisions = NULL;\n \tstruct strvec make_script_args = STRVEC_INIT;\n \tstruct todo_list todo_list = TODO_LIST_INIT;\n \tstruct replay_opts replay = get_replay_opts(opts);\n \n \tif (get_revision_ranges(opts->upstream, opts->onto, &opts->orig_head->object.oid,\n \t\t\t\t&revisions, &shortrevisions))\n \t\tgoto cleanup;\n \n+\tstrvec_pushl(&make_script_args, \"\", revisions, NULL);\n+\tif (opts->restrict_revision)\n+\t\tstrvec_pushf(&make_script_args, \"^%s\",\n+\t\t\t     oid_to_hex(&opts->restrict_revision->object.oid));\n+\n+\tret = sequencer_make_script(the_repository, &todo_list.buf,\n+\t\t\t\t    make_script_args.nr, make_script_args.v,\n+\t\t\t\t    flags);\n+\tif (ret) {\n+\t\terror(_(\"could not generate todo list\"));\n+\t\tgoto cleanup;\n+\t}\n+\n \tif (init_basic_state(&replay,\n \t\t\t     opts->head_name ? opts->head_name : \"detached HEAD\",\n \t\t\t     opts->onto, &opts->orig_head->object.oid))\n \t\tgoto cleanup;\n \n \tif (!opts->upstream && opts->squash_onto)\n \t\twrite_file(path_squash_onto(), \"%s\\n\",\n \t\t\t   oid_to_hex(opts->squash_onto));\n \n-\tstrvec_pushl(&make_script_args, \"\", revisions, NULL);\n-\tif (opts->restrict_revision)\n-\t\tstrvec_pushf(&make_script_args, \"^%s\",\n-\t\t\t     oid_to_hex(&opts->restrict_revision->object.oid));\n+\tdiscard_index(the_repository->index);\n+\tif (todo_list_parse_insn_buffer(the_repository, &replay,\n+\t\t\t\t\ttodo_list.buf.buf, &todo_list))\n+\t\tBUG(\"unusable todo list\");\n \n-\tret = sequencer_make_script(the_repository, &todo_list.buf,\n-\t\t\t\t    make_script_args.nr, make_script_args.v,\n-\t\t\t\t    flags);\n-\n-\tif (ret)\n-\t\terror(_(\"could not generate todo list\"));\n-\telse {\n-\t\tdiscard_index(the_repository->index);\n-\t\tif (todo_list_parse_insn_buffer(the_repository, &replay,\n-\t\t\t\t\t\ttodo_list.buf.buf, &todo_list))\n-\t\t\tBUG(\"unusable todo list\");\n-\n-\t\tret = complete_action(the_repository, &replay, flags,\n-\t\t\tshortrevisions, opts->onto_name, opts->onto,\n-\t\t\t&opts->orig_head->object.oid, &opts->exec,\n-\t\t\topts->autosquash, opts->update_refs, &todo_list);\n-\t}\n+\tret = complete_action(the_repository, &replay, flags,\n+\t\tshortrevisions, opts->onto_name, opts->onto,\n+\t\t&opts->orig_head->object.oid, &opts->exec,\n+\t\topts->autosquash, opts->update_refs, &todo_list);\n \n cleanup:\n \treplay_opts_release(&replay);\n \tfree(revisions);\n \tfree(shortrevisions);\n \ttodo_list_release(&todo_list);\n \tstrvec_clear(&make_script_args);\n \n \treturn ret;\n }\ndiff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh\nindex fcc40d6fe1..d4c624b841 100755\n--- a/t/t3415-rebase-autosquash.sh\n+++ b/t/t3415-rebase-autosquash.sh\n@@ -394,6 +394,16 @@ test_expect_success 'autosquash with empty custom instructionFormat' '\n \t)\n '\n \n+test_expect_success 'autosquash with invalid custom instructionFormat' '\n+\tgit reset --hard base &&\n+\ttest_commit invalid-instructionFormat-test &&\n+\t(\n+\t\ttest_must_fail git -c rebase.instructionFormat=blah \\\n+\t\t\trebase --autosquash  --force-rebase -i HEAD^ &&\n+\t\ttest_path_is_missing .git/rebase-merge\n+\t)\n+'\n+\n set_backup_editor () {\n \twrite_script backup-editor.sh <<-\\EOF\n \tcp \"$1\" .git/backup-\"$(basename \"$1\")\"\n"},{"id":"520033","messageId":"7e796844-97e2-4b45-a76e-4c1fcb1da3ae@gmail.com","threadId":"63613","inReplyTo":"20250609221055.136074-1-oystwa@gmail.com","subject":"Re: [PATCH] rebase: write script before initializing state","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-06-10T10:13:07Z","receivedAt":"2025-06-10T10:11:54Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Øystein\n\nOn 09/06/2025 23:10, Øystein Walle wrote:\n> If rebase.instructionFormat is invalid the repository is left in a\n> strange state when the interactive rebase fails. `git status` outputs\n> boths the same as it would in the normal case *and* something related to\n> interactive rebase:\n> \n>      $ git -c rebase.instructionFormat=blah rebase -i\n>      fatal: invalid --pretty format: blah\n>      $ git status\n>      On branch master\n>      Your branch is ahead of 'upstream/master' by 1 commit.\n>        (use \"git push\" to publish your local commits)\n> \n>      git-rebase-todo is missing.\n>      No commands done.\n>      No commands remaining.\n>      You are currently editing a commit while rebasing branch 'master' on '8db3019401'.\n>        (use \"git commit --amend\" to amend the current commit)\n>        (use \"git rebase --continue\" once you are satisfied with your changes)\n\nThanks for working on this.\n\n> By attempting to write the rebase script before initializing the state\n> this potential scenario is avoided.\n> ---\n> The diff looks perhaps more messy than required. The only required\n> change is the filling in of make_script_args and the call to\n> sequencer_make_script() above the call to init_basic_state(). But then\n> the `if (ret)` looks out of place, and moving that up means adding `goto\n> cleanup` which means the code that was previously the else case can be\n> dedented.\n> \n> get_commit_format() calls die() in this case, so cleaning up the\n> sequencer state isn't an option. Maybe it shouldn't call die in the\n> first place, but that looks to be much larger change.\n\nI don't think that should be too difficult and it is the only way to fix \nthis that ensures we restore the stashed changes when the commit format \nis invalid. The test added in this patch should be updated to check that \nthe changes stashed with '--autostash' are restored when the commit \nformat is invalid.\n\nLooking at the callers of get_commit_format() there are three in \nrevision.c:handle_revision_opt() only one of which can fail. That can be \nconverted to\n\n\tif (get_commit_format(...))\n\t\tdie(NULL);\n\nI think we can do the same for the caller in \nbuiltin/log.c:cmd_log_init_defaults() and should be straight forward to \nupdate the example in Documentation/MyFirstObjectWalk.adoc in a similar \nway. The caller in sequencer.c:sequencer_make_script() should propagate \nthe error.\n\nNote that to use \"die(NULL)\" you need to base you patch on the branch \n'ps/maintenance-ref-lock' which is currently in next.\n\nI'm about to go off the list for a couple of weeks but I'm sure someone \nelse will be happy to answer any questions that you have.\n\nBest Wishes\n\nPhillip\n\n> \n>   builtin/rebase.c             | 42 ++++++++++++++++++------------------\n>   t/t3415-rebase-autosquash.sh | 10 +++++++++\n>   2 files changed, 31 insertions(+), 21 deletions(-)\n> \n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index 2e8c4ee678..8139816417 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -293,15 +293,6 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)\n>   \t\t\t\t&revisions, &shortrevisions))\n>   \t\tgoto cleanup;\n>   \n> -\tif (init_basic_state(&replay,\n> -\t\t\t     opts->head_name ? opts->head_name : \"detached HEAD\",\n> -\t\t\t     opts->onto, &opts->orig_head->object.oid))\n> -\t\tgoto cleanup;\n> -\n> -\tif (!opts->upstream && opts->squash_onto)\n> -\t\twrite_file(path_squash_onto(), \"%s\\n\",\n> -\t\t\t   oid_to_hex(opts->squash_onto));\n> -\n>   \tstrvec_pushl(&make_script_args, \"\", revisions, NULL);\n>   \tif (opts->restrict_revision)\n>   \t\tstrvec_pushf(&make_script_args, \"^%s\",\n> @@ -310,21 +301,30 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)\n>   \tret = sequencer_make_script(the_repository, &todo_list.buf,\n>   \t\t\t\t    make_script_args.nr, make_script_args.v,\n>   \t\t\t\t    flags);\n> -\n> -\tif (ret)\n> +\tif (ret) {\n>   \t\terror(_(\"could not generate todo list\"));\n> -\telse {\n> -\t\tdiscard_index(the_repository->index);\n> -\t\tif (todo_list_parse_insn_buffer(the_repository, &replay,\n> -\t\t\t\t\t\ttodo_list.buf.buf, &todo_list))\n> -\t\t\tBUG(\"unusable todo list\");\n> -\n> -\t\tret = complete_action(the_repository, &replay, flags,\n> -\t\t\tshortrevisions, opts->onto_name, opts->onto,\n> -\t\t\t&opts->orig_head->object.oid, &opts->exec,\n> -\t\t\topts->autosquash, opts->update_refs, &todo_list);\n> +\t\tgoto cleanup;\n>   \t}\n>   \n> +\tif (init_basic_state(&replay,\n> +\t\t\t     opts->head_name ? opts->head_name : \"detached HEAD\",\n> +\t\t\t     opts->onto, &opts->orig_head->object.oid))\n> +\t\tgoto cleanup;\n> +\n> +\tif (!opts->upstream && opts->squash_onto)\n> +\t\twrite_file(path_squash_onto(), \"%s\\n\",\n> +\t\t\t   oid_to_hex(opts->squash_onto));\n> +\n> +\tdiscard_index(the_repository->index);\n> +\tif (todo_list_parse_insn_buffer(the_repository, &replay,\n> +\t\t\t\t\ttodo_list.buf.buf, &todo_list))\n> +\t\tBUG(\"unusable todo list\");\n> +\n> +\tret = complete_action(the_repository, &replay, flags,\n> +\t\tshortrevisions, opts->onto_name, opts->onto,\n> +\t\t&opts->orig_head->object.oid, &opts->exec,\n> +\t\topts->autosquash, opts->update_refs, &todo_list);\n> +\n>   cleanup:\n>   \treplay_opts_release(&replay);\n>   \tfree(revisions);\n> diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh\n> index 26b42a526a..5d093e3a7a 100755\n> --- a/t/t3415-rebase-autosquash.sh\n> +++ b/t/t3415-rebase-autosquash.sh\n> @@ -394,6 +394,16 @@ test_expect_success 'autosquash with empty custom instructionFormat' '\n>   \t)\n>   '\n>   \n> +test_expect_success 'autosquash with invalid custom instructionFormat' '\n> +\tgit reset --hard base &&\n> +\ttest_commit invalid-instructionFormat-test &&\n> +\t(\n> +\t\ttest_must_fail git -c rebase.instructionFormat=blah \\\n> +\t\t\trebase --autosquash  --force-rebase -i HEAD^ &&\n> +\t\ttest_path_is_missing .git/rebase-merge\n> +\t)\n> +'\n> +\n>   set_backup_editor () {\n>   \twrite_script backup-editor.sh <<-\\EOF\n>   \tcp \"$1\" .git/backup-\"$(basename \"$1\")\"\n\n"},{"id":"521595","messageId":"xmqqfrf6qkyy.fsf@gitster.g","threadId":"63613","inReplyTo":"20250609221055.136074-1-oystwa@gmail.com","subject":"Re: [PATCH] rebase: write script before initializing state","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-09T00:14:13Z","receivedAt":"2025-07-09T00:14:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Øystein Walle <oystwa@gmail.com> writes:\n\n> If rebase.instructionFormat is invalid the repository is left in a\n> strange state when the interactive rebase fails. `git status` outputs\n> boths the same as it would in the normal case *and* something related to\n> interactive rebase:\n>\n>     $ git -c rebase.instructionFormat=blah rebase -i\n>     fatal: invalid --pretty format: blah\n>     $ git status\n>     On branch master\n>     Your branch is ahead of 'upstream/master' by 1 commit.\n>       (use \"git push\" to publish your local commits)\n>\n>     git-rebase-todo is missing.\n>     No commands done.\n>     No commands remaining.\n>     You are currently editing a commit while rebasing branch 'master' on '8db3019401'.\n>       (use \"git commit --amend\" to amend the current commit)\n>       (use \"git rebase --continue\" once you are satisfied with your changes)\n>\n> By attempting to write the rebase script before initializing the state\n> this potential scenario is avoided.\n> ---\n> The diff looks perhaps more messy than required. The only required\n> change is the filling in of make_script_args and the call to\n> sequencer_make_script() above the call to init_basic_state(). But then\n> the `if (ret)` looks out of place, and moving that up means adding `goto\n> cleanup` which means the code that was previously the else case can be\n> dedented.\n>\n> get_commit_format() calls die() in this case, so cleaning up the\n> sequencer state isn't an option. Maybe it shouldn't call die in the\n> first place, but that looks to be much larger change.\n\nThe patch has been stalled for a few weeks since Phillip's review\ncomments.  What's the status of this?  Will we see a response and/or\nan updated patch sometime soon?\n\nThanks.\n"},{"id":"521846","messageId":"20250711203615.9982-1-oystwa@gmail.com","threadId":"63613","inReplyTo":"xmqqfrf6qkyy.fsf@gitster.g","subject":"[PATCH v2] rebase: write script before initializing state","fromName":"Øystein Walle","fromEmail":"oystwa@gmail.com","sentAt":"2025-07-11T20:36:15Z","receivedAt":"2025-07-11T20:36:45Z","isPatch":true,"sender":{"key":"oystwa@gmail.com","avatar":"https://avatars.githubusercontent.com/u/794585?v=4"},"body":"If rebase.instructionFormat is invalid the repository is left in a\nstrange state when the interactive rebase fails. `git status` outputs\nboth the same as it would have in the normal case *and* something\nrelated to the interactive rebase:\n\n    $ git -c rebase.instructionFormat=blah rebase -i\n    fatal: invalid --pretty format: blah\n    $ git status\n    On branch master\n    Your branch is ahead of 'upstream/master' by 1 commit.\n      (use \"git push\" to publish your local commits)\n\n    git-rebase-todo is missing.\n    No commands done.\n    No commands remaining.\n    You are currently editing a commit while rebasing branch 'master' on '8db3019401'.\n      (use \"git commit --amend\" to amend the current commit)\n      (use \"git rebase --continue\" once you are satisfied with your changes)\n\nget_commit_format() calls die() on failure so we cannot handle the error\ngracefully. By attempting to write the rebase script before initializing\nthe state this bad state can be avoided.\n\nSigned-off-by: Øystein Walle <oystwa@gmail.com>\n---\nSo sorry for the delay. I saw that the signoff was missing, then saw\nPhillip's review, decided to think about it and then life happened in\nthe mean time...\n\nThis patch is identical to the first one except it has the missing\nsignoff and a few typos in the commit message corrected. Phillip's\nsuggestions are noted and appreciated but unfortunately I am unable to\nwork on the at the moment. And I do think my patch is at least an\nimprovement albeit perhaps less thorough than it could have been.\n\nØsse\n\n builtin/rebase.c             | 42 ++++++++++++++++++------------------\n t/t3415-rebase-autosquash.sh | 10 +++++++++\n 2 files changed, 31 insertions(+), 21 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 2e8c4ee678..8139816417 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -293,15 +293,6 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)\n \t\t\t\t&revisions, &shortrevisions))\n \t\tgoto cleanup;\n \n-\tif (init_basic_state(&replay,\n-\t\t\t     opts->head_name ? opts->head_name : \"detached HEAD\",\n-\t\t\t     opts->onto, &opts->orig_head->object.oid))\n-\t\tgoto cleanup;\n-\n-\tif (!opts->upstream && opts->squash_onto)\n-\t\twrite_file(path_squash_onto(), \"%s\\n\",\n-\t\t\t   oid_to_hex(opts->squash_onto));\n-\n \tstrvec_pushl(&make_script_args, \"\", revisions, NULL);\n \tif (opts->restrict_revision)\n \t\tstrvec_pushf(&make_script_args, \"^%s\",\n@@ -310,21 +301,30 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)\n \tret = sequencer_make_script(the_repository, &todo_list.buf,\n \t\t\t\t    make_script_args.nr, make_script_args.v,\n \t\t\t\t    flags);\n-\n-\tif (ret)\n+\tif (ret) {\n \t\terror(_(\"could not generate todo list\"));\n-\telse {\n-\t\tdiscard_index(the_repository->index);\n-\t\tif (todo_list_parse_insn_buffer(the_repository, &replay,\n-\t\t\t\t\t\ttodo_list.buf.buf, &todo_list))\n-\t\t\tBUG(\"unusable todo list\");\n-\n-\t\tret = complete_action(the_repository, &replay, flags,\n-\t\t\tshortrevisions, opts->onto_name, opts->onto,\n-\t\t\t&opts->orig_head->object.oid, &opts->exec,\n-\t\t\topts->autosquash, opts->update_refs, &todo_list);\n+\t\tgoto cleanup;\n \t}\n \n+\tif (init_basic_state(&replay,\n+\t\t\t     opts->head_name ? opts->head_name : \"detached HEAD\",\n+\t\t\t     opts->onto, &opts->orig_head->object.oid))\n+\t\tgoto cleanup;\n+\n+\tif (!opts->upstream && opts->squash_onto)\n+\t\twrite_file(path_squash_onto(), \"%s\\n\",\n+\t\t\t   oid_to_hex(opts->squash_onto));\n+\n+\tdiscard_index(the_repository->index);\n+\tif (todo_list_parse_insn_buffer(the_repository, &replay,\n+\t\t\t\t\ttodo_list.buf.buf, &todo_list))\n+\t\tBUG(\"unusable todo list\");\n+\n+\tret = complete_action(the_repository, &replay, flags,\n+\t\tshortrevisions, opts->onto_name, opts->onto,\n+\t\t&opts->orig_head->object.oid, &opts->exec,\n+\t\topts->autosquash, opts->update_refs, &todo_list);\n+\n cleanup:\n \treplay_opts_release(&replay);\n \tfree(revisions);\ndiff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh\nindex 26b42a526a..5d093e3a7a 100755\n--- a/t/t3415-rebase-autosquash.sh\n+++ b/t/t3415-rebase-autosquash.sh\n@@ -394,6 +394,16 @@ test_expect_success 'autosquash with empty custom instructionFormat' '\n \t)\n '\n \n+test_expect_success 'autosquash with invalid custom instructionFormat' '\n+\tgit reset --hard base &&\n+\ttest_commit invalid-instructionFormat-test &&\n+\t(\n+\t\ttest_must_fail git -c rebase.instructionFormat=blah \\\n+\t\t\trebase --autosquash  --force-rebase -i HEAD^ &&\n+\t\ttest_path_is_missing .git/rebase-merge\n+\t)\n+'\n+\n set_backup_editor () {\n \twrite_script backup-editor.sh <<-\\EOF\n \tcp \"$1\" .git/backup-\"$(basename \"$1\")\"\n-- \n2.43.0\n\n"},{"id":"521849","messageId":"xmqqple6zaga.fsf@gitster.g","threadId":"63613","inReplyTo":"20250711203615.9982-1-oystwa@gmail.com","subject":"Re: [PATCH v2] rebase: write script before initializing state","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-11T21:25:41Z","receivedAt":"2025-07-11T21:25:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Øystein Walle <oystwa@gmail.com> writes:\n\n> If rebase.instructionFormat is invalid the repository is left in a\n> strange state when the interactive rebase fails. `git status` outputs\n> both the same as it would have in the normal case *and* something\n> related to the interactive rebase:\n>\n>     $ git -c rebase.instructionFormat=blah rebase -i\n>     fatal: invalid --pretty format: blah\n>     $ git status\n>     On branch master\n>     Your branch is ahead of 'upstream/master' by 1 commit.\n>       (use \"git push\" to publish your local commits)\n>\n>     git-rebase-todo is missing.\n>     No commands done.\n>     No commands remaining.\n>     You are currently editing a commit while rebasing branch 'master' on '8db3019401'.\n>       (use \"git commit --amend\" to amend the current commit)\n>       (use \"git rebase --continue\" once you are satisfied with your changes)\n>\n> get_commit_format() calls die() on failure so we cannot handle the error\n> gracefully. By attempting to write the rebase script before initializing\n> the state this bad state can be avoided.\n>\n> Signed-off-by: Øystein Walle <oystwa@gmail.com>\n> ---\n> So sorry for the delay. I saw that the signoff was missing, then saw\n> Phillip's review, decided to think about it and then life happened in\n> the mean time...\n\nNo need to be sorry.  Life happens, indeed.\n\n> This patch is identical to the first one except it has the missing\n> signoff and a few typos in the commit message corrected. Phillip's\n> suggestions are noted and appreciated but unfortunately I am unable to\n> work on the at the moment. And I do think my patch is at least an\n> improvement albeit perhaps less thorough than it could have been.\n\nWell, as long as we are making a step in the right direction, such a\npartial improvement gives us a better foundation for somebody else\nto further build on.  It does not look like that this patch would\nmake it harder to later give us a more thorough solution.\n\nThanks for working on the topic.\n\n>  builtin/rebase.c             | 42 ++++++++++++++++++------------------\n>  t/t3415-rebase-autosquash.sh | 10 +++++++++\n>  2 files changed, 31 insertions(+), 21 deletions(-)\n>\n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index 2e8c4ee678..8139816417 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -293,15 +293,6 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)\n>  \t\t\t\t&revisions, &shortrevisions))\n>  \t\tgoto cleanup;\n>  \n> -\tif (init_basic_state(&replay,\n> -\t\t\t     opts->head_name ? opts->head_name : \"detached HEAD\",\n> -\t\t\t     opts->onto, &opts->orig_head->object.oid))\n> -\t\tgoto cleanup;\n> -\n> -\tif (!opts->upstream && opts->squash_onto)\n> -\t\twrite_file(path_squash_onto(), \"%s\\n\",\n> -\t\t\t   oid_to_hex(opts->squash_onto));\n> -\n>  \tstrvec_pushl(&make_script_args, \"\", revisions, NULL);\n>  \tif (opts->restrict_revision)\n>  \t\tstrvec_pushf(&make_script_args, \"^%s\",\n> @@ -310,21 +301,30 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)\n>  \tret = sequencer_make_script(the_repository, &todo_list.buf,\n>  \t\t\t\t    make_script_args.nr, make_script_args.v,\n>  \t\t\t\t    flags);\n> -\n> -\tif (ret)\n> +\tif (ret) {\n>  \t\terror(_(\"could not generate todo list\"));\n> -\telse {\n> -\t\tdiscard_index(the_repository->index);\n> -\t\tif (todo_list_parse_insn_buffer(the_repository, &replay,\n> -\t\t\t\t\t\ttodo_list.buf.buf, &todo_list))\n> -\t\t\tBUG(\"unusable todo list\");\n> -\n> -\t\tret = complete_action(the_repository, &replay, flags,\n> -\t\t\tshortrevisions, opts->onto_name, opts->onto,\n> -\t\t\t&opts->orig_head->object.oid, &opts->exec,\n> -\t\t\topts->autosquash, opts->update_refs, &todo_list);\n> +\t\tgoto cleanup;\n>  \t}\n>  \n> +\tif (init_basic_state(&replay,\n> +\t\t\t     opts->head_name ? opts->head_name : \"detached HEAD\",\n> +\t\t\t     opts->onto, &opts->orig_head->object.oid))\n> +\t\tgoto cleanup;\n> +\n> +\tif (!opts->upstream && opts->squash_onto)\n> +\t\twrite_file(path_squash_onto(), \"%s\\n\",\n> +\t\t\t   oid_to_hex(opts->squash_onto));\n> +\n> +\tdiscard_index(the_repository->index);\n> +\tif (todo_list_parse_insn_buffer(the_repository, &replay,\n> +\t\t\t\t\ttodo_list.buf.buf, &todo_list))\n> +\t\tBUG(\"unusable todo list\");\n> +\n> +\tret = complete_action(the_repository, &replay, flags,\n> +\t\tshortrevisions, opts->onto_name, opts->onto,\n> +\t\t&opts->orig_head->object.oid, &opts->exec,\n> +\t\topts->autosquash, opts->update_refs, &todo_list);\n> +\n>  cleanup:\n>  \treplay_opts_release(&replay);\n>  \tfree(revisions);\n> diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh\n> index 26b42a526a..5d093e3a7a 100755\n> --- a/t/t3415-rebase-autosquash.sh\n> +++ b/t/t3415-rebase-autosquash.sh\n> @@ -394,6 +394,16 @@ test_expect_success 'autosquash with empty custom instructionFormat' '\n>  \t)\n>  '\n>  \n> +test_expect_success 'autosquash with invalid custom instructionFormat' '\n> +\tgit reset --hard base &&\n> +\ttest_commit invalid-instructionFormat-test &&\n> +\t(\n> +\t\ttest_must_fail git -c rebase.instructionFormat=blah \\\n> +\t\t\trebase --autosquash  --force-rebase -i HEAD^ &&\n> +\t\ttest_path_is_missing .git/rebase-merge\n> +\t)\n> +'\n> +\n>  set_backup_editor () {\n>  \twrite_script backup-editor.sh <<-\\EOF\n>  \tcp \"$1\" .git/backup-\"$(basename \"$1\")\"\n"},{"id":"522607","messageId":"xmqq1pq6mw0j.fsf@gitster.g","threadId":"63613","inReplyTo":"20250711203615.9982-1-oystwa@gmail.com","subject":"Re: [PATCH v2] rebase: write script before initializing state","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-23T21:34:36Z","receivedAt":"2025-07-23T21:34:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Øystein Walle <oystwa@gmail.com> writes:\n\n> If rebase.instructionFormat is invalid the repository is left in a\n> strange state when the interactive rebase fails. `git status` outputs\n> both the same as it would have in the normal case *and* something\n> related to the interactive rebase:\n>\n>     $ git -c rebase.instructionFormat=blah rebase -i\n>     fatal: invalid --pretty format: blah\n>     $ git status\n>     On branch master\n>     Your branch is ahead of 'upstream/master' by 1 commit.\n>       (use \"git push\" to publish your local commits)\n>\n>     git-rebase-todo is missing.\n>     No commands done.\n>     No commands remaining.\n>     You are currently editing a commit while rebasing branch 'master' on '8db3019401'.\n>       (use \"git commit --amend\" to amend the current commit)\n>       (use \"git rebase --continue\" once you are satisfied with your changes)\n>\n> get_commit_format() calls die() on failure so we cannot handle the error\n> gracefully. By attempting to write the rebase script before initializing\n> the state this bad state can be avoided.\n>\n> Signed-off-by: Øystein Walle <oystwa@gmail.com>\n> ---\n> So sorry for the delay. I saw that the signoff was missing, then saw\n> Phillip's review, decided to think about it and then life happened in\n> the mean time...\n>\n> This patch is identical to the first one except it has the missing\n> signoff and a few typos in the commit message corrected. Phillip's\n> suggestions are noted and appreciated but unfortunately I am unable to\n> work on the at the moment. And I do think my patch is at least an\n> improvement albeit perhaps less thorough than it could have been.\n\nI am sweeping my backlog and noticed that nobody chimed in to help\nimproving this topic.  As I already said, this would not least be\nmoving a step in the right direction, so I am planning to mark it\nfor 'next', but thought that I should check first before doing so,\nin case you are back on the topic and cooking a new iteration.\n\nThanks.\n"},{"id":"522666","messageId":"9dc4331e-7c0d-4cc5-980e-a151853725b6@gmail.com","threadId":"63613","inReplyTo":"xmqqple6zaga.fsf@gitster.g","subject":"Re: [PATCH v2] rebase: write script before initializing state","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-07-24T14:22:15Z","receivedAt":"2025-07-24T14:22:18Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 11/07/2025 22:25, Junio C Hamano wrote:\n> Øystein Walle <oystwa@gmail.com> writes:\n> \n>> If rebase.instructionFormat is invalid the repository is left in a\n>> strange state when the interactive rebase fails. `git status` outputs\n>> both the same as it would have in the normal case *and* something\n>> related to the interactive rebase:\n>>\n>>      $ git -c rebase.instructionFormat=blah rebase -i\n>>      fatal: invalid --pretty format: blah\n>>      $ git status\n>>      On branch master\n>>      Your branch is ahead of 'upstream/master' by 1 commit.\n>>        (use \"git push\" to publish your local commits)\n>>\n>>      git-rebase-todo is missing.\n>>      No commands done.\n>>      No commands remaining.\n>>      You are currently editing a commit while rebasing branch 'master' on '8db3019401'.\n>>        (use \"git commit --amend\" to amend the current commit)\n>>        (use \"git rebase --continue\" once you are satisfied with your changes)\n>>\n>> get_commit_format() calls die() on failure so we cannot handle the error\n>> gracefully. By attempting to write the rebase script before initializing\n>> the state this bad state can be avoided.\n>>\n>> Signed-off-by: Øystein Walle <oystwa@gmail.com>\n>> ---\n>> So sorry for the delay. I saw that the signoff was missing, then saw\n>> Phillip's review, decided to think about it and then life happened in\n>> the mean time...\n> \n> No need to be sorry.  Life happens, indeed.\n> \n>> This patch is identical to the first one except it has the missing\n>> signoff and a few typos in the commit message corrected. Phillip's\n>> suggestions are noted and appreciated but unfortunately I am unable to\n>> work on the at the moment. And I do think my patch is at least an\n>> improvement albeit perhaps less thorough than it could have been.\n> \n> Well, as long as we are making a step in the right direction, such a\n> partial improvement gives us a better foundation for somebody else\n> to further build on.\n\nI don't think this is a better (or worse) foundation for future \nimprovement as solving this when the user passes '--autostash' needs a \ndifferent approach. When that option is given we create the state \ndirectory earlier so moving when we call init_basic_state() has no effect.\n\n>  It does not look like that this patch would\n> make it harder to later give us a more thorough solution.\n\nI agree with this. I don't think this change makes fixing the \n'--autostash' case harder, but fixing that case makes this change \nredundant.\n> Thanks for working on the topic.\n\nYes, thank you Øystein\n\nPhillip\n\n>>   builtin/rebase.c             | 42 ++++++++++++++++++------------------\n>>   t/t3415-rebase-autosquash.sh | 10 +++++++++\n>>   2 files changed, 31 insertions(+), 21 deletions(-)\n>>\n>> diff --git a/builtin/rebase.c b/builtin/rebase.c\n>> index 2e8c4ee678..8139816417 100644\n>> --- a/builtin/rebase.c\n>> +++ b/builtin/rebase.c\n>> @@ -293,15 +293,6 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)\n>>   \t\t\t\t&revisions, &shortrevisions))\n>>   \t\tgoto cleanup;\n>>   \n>> -\tif (init_basic_state(&replay,\n>> -\t\t\t     opts->head_name ? opts->head_name : \"detached HEAD\",\n>> -\t\t\t     opts->onto, &opts->orig_head->object.oid))\n>> -\t\tgoto cleanup;\n>> -\n>> -\tif (!opts->upstream && opts->squash_onto)\n>> -\t\twrite_file(path_squash_onto(), \"%s\\n\",\n>> -\t\t\t   oid_to_hex(opts->squash_onto));\n>> -\n>>   \tstrvec_pushl(&make_script_args, \"\", revisions, NULL);\n>>   \tif (opts->restrict_revision)\n>>   \t\tstrvec_pushf(&make_script_args, \"^%s\",\n>> @@ -310,21 +301,30 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)\n>>   \tret = sequencer_make_script(the_repository, &todo_list.buf,\n>>   \t\t\t\t    make_script_args.nr, make_script_args.v,\n>>   \t\t\t\t    flags);\n>> -\n>> -\tif (ret)\n>> +\tif (ret) {\n>>   \t\terror(_(\"could not generate todo list\"));\n>> -\telse {\n>> -\t\tdiscard_index(the_repository->index);\n>> -\t\tif (todo_list_parse_insn_buffer(the_repository, &replay,\n>> -\t\t\t\t\t\ttodo_list.buf.buf, &todo_list))\n>> -\t\t\tBUG(\"unusable todo list\");\n>> -\n>> -\t\tret = complete_action(the_repository, &replay, flags,\n>> -\t\t\tshortrevisions, opts->onto_name, opts->onto,\n>> -\t\t\t&opts->orig_head->object.oid, &opts->exec,\n>> -\t\t\topts->autosquash, opts->update_refs, &todo_list);\n>> +\t\tgoto cleanup;\n>>   \t}\n>>   \n>> +\tif (init_basic_state(&replay,\n>> +\t\t\t     opts->head_name ? opts->head_name : \"detached HEAD\",\n>> +\t\t\t     opts->onto, &opts->orig_head->object.oid))\n>> +\t\tgoto cleanup;\n>> +\n>> +\tif (!opts->upstream && opts->squash_onto)\n>> +\t\twrite_file(path_squash_onto(), \"%s\\n\",\n>> +\t\t\t   oid_to_hex(opts->squash_onto));\n>> +\n>> +\tdiscard_index(the_repository->index);\n>> +\tif (todo_list_parse_insn_buffer(the_repository, &replay,\n>> +\t\t\t\t\ttodo_list.buf.buf, &todo_list))\n>> +\t\tBUG(\"unusable todo list\");\n>> +\n>> +\tret = complete_action(the_repository, &replay, flags,\n>> +\t\tshortrevisions, opts->onto_name, opts->onto,\n>> +\t\t&opts->orig_head->object.oid, &opts->exec,\n>> +\t\topts->autosquash, opts->update_refs, &todo_list);\n>> +\n>>   cleanup:\n>>   \treplay_opts_release(&replay);\n>>   \tfree(revisions);\n>> diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh\n>> index 26b42a526a..5d093e3a7a 100755\n>> --- a/t/t3415-rebase-autosquash.sh\n>> +++ b/t/t3415-rebase-autosquash.sh\n>> @@ -394,6 +394,16 @@ test_expect_success 'autosquash with empty custom instructionFormat' '\n>>   \t)\n>>   '\n>>   \n>> +test_expect_success 'autosquash with invalid custom instructionFormat' '\n>> +\tgit reset --hard base &&\n>> +\ttest_commit invalid-instructionFormat-test &&\n>> +\t(\n>> +\t\ttest_must_fail git -c rebase.instructionFormat=blah \\\n>> +\t\t\trebase --autosquash  --force-rebase -i HEAD^ &&\n>> +\t\ttest_path_is_missing .git/rebase-merge\n>> +\t)\n>> +'\n>> +\n>>   set_backup_editor () {\n>>   \twrite_script backup-editor.sh <<-\\EOF\n>>   \tcp \"$1\" .git/backup-\"$(basename \"$1\")\"\n\n"}]}