From: Phillip Wood Date: Tue, 10 Jun 2025 10:13:07 GMT Subject: Re: [PATCH] rebase: write script before initializing state Message-ID: <7e796844-97e2-4b45-a76e-4c1fcb1da3ae@gmail.com> In-Reply-To: <20250609221055.136074-1-oystwa@gmail.com> Hi Øystein On 09/06/2025 23:10, Øystein Walle wrote: > If rebase.instructionFormat is invalid the repository is left in a > strange state when the interactive rebase fails. `git status` outputs > boths the same as it would in the normal case *and* something related to > interactive rebase: > > $ git -c rebase.instructionFormat=blah rebase -i > fatal: invalid --pretty format: blah > $ git status > On branch master > Your branch is ahead of 'upstream/master' by 1 commit. > (use "git push" to publish your local commits) > > git-rebase-todo is missing. > No commands done. > No commands remaining. > You are currently editing a commit while rebasing branch 'master' on '8db3019401'. > (use "git commit --amend" to amend the current commit) > (use "git rebase --continue" once you are satisfied with your changes) Thanks for working on this. > By attempting to write the rebase script before initializing the state > this potential scenario is avoided. > --- > The diff looks perhaps more messy than required. The only required > change is the filling in of make_script_args and the call to > sequencer_make_script() above the call to init_basic_state(). But then > the `if (ret)` looks out of place, and moving that up means adding `goto > cleanup` which means the code that was previously the else case can be > dedented. > > get_commit_format() calls die() in this case, so cleaning up the > sequencer state isn't an option. Maybe it shouldn't call die in the > first place, but that looks to be much larger change. I don't think that should be too difficult and it is the only way to fix this that ensures we restore the stashed changes when the commit format is invalid. The test added in this patch should be updated to check that the changes stashed with '--autostash' are restored when the commit format is invalid. Looking at the callers of get_commit_format() there are three in revision.c:handle_revision_opt() only one of which can fail. That can be converted to if (get_commit_format(...)) die(NULL); I think we can do the same for the caller in builtin/log.c:cmd_log_init_defaults() and should be straight forward to update the example in Documentation/MyFirstObjectWalk.adoc in a similar way. The caller in sequencer.c:sequencer_make_script() should propagate the error. Note that to use "die(NULL)" you need to base you patch on the branch 'ps/maintenance-ref-lock' which is currently in next. I'm about to go off the list for a couple of weeks but I'm sure someone else will be happy to answer any questions that you have. Best Wishes Phillip > > builtin/rebase.c | 42 ++++++++++++++++++------------------ > t/t3415-rebase-autosquash.sh | 10 +++++++++ > 2 files changed, 31 insertions(+), 21 deletions(-) > > diff --git a/builtin/rebase.c b/builtin/rebase.c > index 2e8c4ee678..8139816417 100644 > --- a/builtin/rebase.c > +++ b/builtin/rebase.c > @@ -293,15 +293,6 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags) > &revisions, &shortrevisions)) > goto cleanup; > > - if (init_basic_state(&replay, > - opts->head_name ? opts->head_name : "detached HEAD", > - opts->onto, &opts->orig_head->object.oid)) > - goto cleanup; > - > - if (!opts->upstream && opts->squash_onto) > - write_file(path_squash_onto(), "%s\n", > - oid_to_hex(opts->squash_onto)); > - > strvec_pushl(&make_script_args, "", revisions, NULL); > if (opts->restrict_revision) > strvec_pushf(&make_script_args, "^%s", > @@ -310,21 +301,30 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags) > ret = sequencer_make_script(the_repository, &todo_list.buf, > make_script_args.nr, make_script_args.v, > flags); > - > - if (ret) > + if (ret) { > error(_("could not generate todo list")); > - else { > - discard_index(the_repository->index); > - if (todo_list_parse_insn_buffer(the_repository, &replay, > - todo_list.buf.buf, &todo_list)) > - BUG("unusable todo list"); > - > - ret = complete_action(the_repository, &replay, flags, > - shortrevisions, opts->onto_name, opts->onto, > - &opts->orig_head->object.oid, &opts->exec, > - opts->autosquash, opts->update_refs, &todo_list); > + goto cleanup; > } > > + if (init_basic_state(&replay, > + opts->head_name ? opts->head_name : "detached HEAD", > + opts->onto, &opts->orig_head->object.oid)) > + goto cleanup; > + > + if (!opts->upstream && opts->squash_onto) > + write_file(path_squash_onto(), "%s\n", > + oid_to_hex(opts->squash_onto)); > + > + discard_index(the_repository->index); > + if (todo_list_parse_insn_buffer(the_repository, &replay, > + todo_list.buf.buf, &todo_list)) > + BUG("unusable todo list"); > + > + ret = complete_action(the_repository, &replay, flags, > + shortrevisions, opts->onto_name, opts->onto, > + &opts->orig_head->object.oid, &opts->exec, > + opts->autosquash, opts->update_refs, &todo_list); > + > cleanup: > replay_opts_release(&replay); > free(revisions); > diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh > index 26b42a526a..5d093e3a7a 100755 > --- a/t/t3415-rebase-autosquash.sh > +++ b/t/t3415-rebase-autosquash.sh > @@ -394,6 +394,16 @@ test_expect_success 'autosquash with empty custom instructionFormat' ' > ) > ' > > +test_expect_success 'autosquash with invalid custom instructionFormat' ' > + git reset --hard base && > + test_commit invalid-instructionFormat-test && > + ( > + test_must_fail git -c rebase.instructionFormat=blah \ > + rebase --autosquash --force-rebase -i HEAD^ && > + test_path_is_missing .git/rebase-merge > + ) > +' > + > set_backup_editor () { > write_script backup-editor.sh <<-\EOF > cp "$1" .git/backup-"$(basename "$1")"