Re: [PATCH v2] rebase: write script before initializing state

[Junio C Hamano <gitster@pobox.com>]

Ø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.  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.

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