Re: [PATCH v2] rebase: write script before initializing state
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jul 11, 2025, 21:25 UTC
- Message-ID
- <xmqqple6zaga.fsf@gitster.g>
- In-Reply-To
- <20250711203615.9982-1-oystwa@gmail.com>
Ø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")"