Re: [PATCH v2] rebase: write script before initializing state
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Jul 24, 2025, 14:22 UTC
- Message-ID
- <9dc4331e-7c0d-4cc5-980e-a151853725b6@gmail.com>
- In-Reply-To
- <xmqqple6zaga.fsf@gitster.g>
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")"