Re: [RFC PATCH 08/11] rebase -i: use struct rebase_options to parse args

Phillip Wood [off-list ref] writes:

Hi Junio

On 21/03/2019 04:21, Junio C Hamano wrote:

quoted

Phillip Wood [off-list ref] writes:

quoted

+static struct replay_opts get_replay_opts(const struct rebase_options *opts)
+{
+	struct replay_opts replay = REPLAY_OPTS_INIT;
+
+	sequencer_init_config(&replay);
+
+	replay.action = REPLAY_INTERACTIVE_REBASE;
+	replay.signoff = opts->signoff;
+	replay.allow_ff = !(opts->flags & REBASE_FORCE);
+	if (opts->allow_rerere_autoupdate)
+		replay.allow_rerere_auto = opts->allow_rerere_autoupdate;
+	replay.allow_empty = 1;
+	replay.allow_empty_message = opts->allow_empty_message;
+	replay.verbose = opts->flags & REBASE_VERBOSE;
+	replay.reschedule_failed_exec = opts->reschedule_failed_exec;
+	replay.gpg_sign = xstrdup_or_null(opts->gpg_sign_opt);
+	replay.strategy = opts->strategy;
+
+	return replay;
+}

This calls init_config() and then sets .action; does it revert to
what dl/merge-cleanup-scissors-fix wants to do, which flipped the
order to fix some bug?  It is a bit hard to tell.

dl/merge-cleanup-scissors-fix changes sequencer_init_config() to
depend on the value of action so action should to be set first. What
is in pu at the moment in not quite right (though I'm not sure what
the practical implications are as it looks like the rebase tests are
passing[1]).

[1] https://travis-ci.org/git/git/jobs/509332448

Yes, it actually is the reason why I left the seemingly-wrong merge
in 'pu' on purpose to see if we have enough test coverage.

they'd both be in the same file. I think it would only need to share
the definitions of struct rebase_options, enum rebase_action and the
declaration of run_rebase_interactive(). Would you be happy with the
addition of builtin/rebase.h (there don't seem to be another headers
in that directory). We could leave rebase--interactive.c around until
rebase--preserve-merges.sh is finally removed.

I think dl/merge-cleanup-scissors-fix is getting solid enough, so a
better alternative may be to base this on top of

	reset e902e9bcae ;# The second batch
	merge ag/sequencer-reduce-rewriting-todo
	merge dl/merge-cleanup-scissors-fix

instead of basing it only on ag/sequencer-reduce-rewriting-todo.

Thanks.