Oswald Buddenhagen <oswald.buddenhagen@xxxxxx> 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?
> 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.
> 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.
