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