Re: [PATCH 5/8] rebase: preserve interactive todo file on checkout failure

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 23, 2023, 20:16 UTC
Message-ID: <xmqq8rfn8bps.fsf@gitster.g>
In-Reply-To: <20230323162235.995574-6-oswald.buddenhagen@gmx.de>

Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:

> Creating a suitable todo file is a potentially labor-intensive process,
> so be less cavalier about discarding it when something goes wrong (e.g.,
> the user messed with the repo while editing the todo).

Is there a reason why we do not always keep it? Why is the file sometimes precious but not precious at all in other times?

Tying the previous bit to "-i was explicitly given" feels a bit unintuitive---when the sequencer machinery was implicitly chosen, and gives the control back to the user, should a user be forbidden to muck with the todo list?

Show 23 quoted lines

> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index a309addd50..728c869db4 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -153,6 +153,7 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)
>  	replay.keep_redundant_commits = (opts->empty == EMPTY_KEEP);
>  	replay.quiet = !(opts->flags & REBASE_NO_QUIET);
>  	replay.verbose = opts->flags & REBASE_VERBOSE;
> +	replay.precious_todo = opts->flags & REBASE_INTERACTIVE_EXPLICIT;
>  	replay.reschedule_failed_exec = opts->reschedule_failed_exec;
>  	replay.committer_date_is_author_date =
>  					opts->committer_date_is_author_date;
> diff --git a/sequencer.c b/sequencer.c
> index b1c29c8802..f8a7f4e721 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -4570,6 +4570,10 @@ static int checkout_onto(struct repository *r, struct replay_opts *opts,
>  		.default_reflog_action = sequencer_reflog_action(opts)
>  	};
>  	if (reset_head(r, &ropts)) {
> +		// Editing the todo may have been costly; don't just discard it.
> +		if (opts->precious_todo)
> +			exit(1);  // Error was already printed

No // comments, please.

Show 20 quoted lines

> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index ff0afad63e..c625aad10a 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -288,13 +288,14 @@ test_expect_success 'abort' '
>  '
>  
>  test_expect_success 'abort with error when new base cannot be checked out' '
> +	test_when_finished "git rebase --abort ||:" &&
>  	git rm --cached file1 &&
>  	git commit -m "remove file in base" &&
>  	test_must_fail git rebase -i primary > output 2>&1 &&
>  	test_i18ngrep "The following untracked working tree files would be overwritten by checkout:" \
>  		output &&
>  	test_i18ngrep "file1" output &&
> -	test_path_is_missing .git/rebase-merge &&
> +	test_path_is_dir .git/rebase-merge &&
>  	rm file1 &&
>  	git reset --hard HEAD^
>  '

Are we happy to just see that the directory still exists? I thought the original motivation explained in the proposed log message was to keep the todo list file, so shouldn't you be checking if the file is there (and if you can reliably ensure that the file has contents that are expected, that would be even better)?

Also, as the keeping of the todo list is now conditional, we should have another test that checks that the file is gone when that condition ("INTERACTIVE_EXPLICIT"?) does not trigger, I think.

Other than that, nicely written.  Thanks.