Re: [PATCH v2 5/7] rebase: drop support for `--preserve-merges`

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Sep 2, 2021, 14:16 UTC
Message-ID: <87tuj39hqd.fsf@evledraar.gmail.com>
In-Reply-To: <nycvar.QRO.7.76.6.2109021555050.55@tvgsbejvaqbjf.bet>

On Thu, Sep 02 2021, Johannes Schindelin wrote:

Show 33 quoted lines

> Hi Ævar,
>
> On Wed, 1 Sep 2021, Ævar Arnfjörð Bjarmason wrote:
>
>>
>> On Wed, Sep 01 2021, Johannes Schindelin via GitGitGadget wrote:
>>
>> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
>> >
>> > This option was deprecated in favor of `--rebase-merges` some time ago,
>> > and now we retire it.
>>
>> >  static int is_merge(struct rebase_options *opts)
>> >  {
>> > -	return opts->type == REBASE_MERGE ||
>> > -		opts->type == REBASE_PRESERVE_MERGES;
>> > +	return opts->type == REBASE_MERGE;
>> >  }
>>
>> This leaves us with a rather pointless is_merge() function and
>> nonsensical control flow in parse_opt_merge().
>
> Thank you for offering your perspective.
>
> From a readability point of view, I disagree with your assessment. Just
> because it can be written shorter does not mean that it is clearer. Quite
> the contrary, if you ask me. And since _I_ am contributing this patch
> series, I will respectfully disagree and keep the version I find more
> intuitive.
>
> You could potentially talk me into adding a patch that renames that
> function to `is_merge_backend()`, but that's as far as I would go. And I
> am not really certain that that would improve things, either.

I should have phrased that "Let's squash this in" less forcefully, to be clear it was just a suggestion. I'm fine with you not taking that squash. This series is good either way.

But just to poke a bit at the rationale: I'd be totally on-board with keeping it because we'd like the smallest possible diff here, even if that ends up including this oddity.

But in terms of making the end-result more readable I don't really see that.

We're left with a mismatch of some things comparing the enum label directly, and some using this function, and then there's REBASE_APPLY. To make it consistent we could alternatively have the diff here at the end, but having some mixture of both seems like the worst end-result, shouldn't we pick one way of inspecting these flags and use that consistently?

(I did not convert the few REBASE_UNSPECIFIED, but those comparisons could similarly be is_unspecified(), and all three could be named is_*_backend() per your suggestion).

diff --git a/builtin/rebase.c b/builtin/rebase.c
index d707e3604e7..ef82a5f1668 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -388,6 +388,11 @@ static int is_merge(struct rebase_options *opts)
 	return opts->type == REBASE_MERGE;
 }
 
+static int is_apply(struct rebase_options *opts)
+{
+	return opts->type == REBASE_APPLY;
+}
+
 static void imply_merge(struct rebase_options *opts, const char *option)
 {
 	switch (opts->type) {
@@ -558,7 +563,7 @@ static int finish_rebase(struct rebase_options *opts)
 	 * user should see them.
 	 */
 	run_auto_maintenance(!(opts->flags & (REBASE_NO_QUIET|REBASE_VERBOSE)));
-	if (opts->type == REBASE_MERGE) {
+	if (is_merge(opts)) {
 		struct replay_opts replay = REPLAY_OPTS_INIT;
 
 		replay.action = REPLAY_INTERACTIVE_REBASE;
@@ -744,7 +749,7 @@ static int run_specific_rebase(struct rebase_options *opts, enum action action)
 	struct strbuf script_snippet = STRBUF_INIT;
 	int status;
 
-	if (opts->type == REBASE_MERGE) {
+	if (is_merge(opts)) {
 		/* Run sequencer-based rebase */
 		setenv("GIT_CHERRY_PICK_HELP", resolvemsg, 1);
 		if (!(opts->flags & REBASE_INTERACTIVE_EXPLICIT)) {
@@ -759,14 +764,14 @@ static int run_specific_rebase(struct rebase_options *opts, enum action action)
 		}
 
 		status = run_sequencer_rebase(opts, action);
-	} else if (opts->type == REBASE_APPLY)
+	} else if (is_apply(opts))
 		status = run_am(opts);
 	else
 		BUG("Unhandled rebase type %d", opts->type);
 
 	if (opts->dont_finish_rebase)
 		; /* do nothing */
-	else if (opts->type == REBASE_MERGE)
+	else if (is_merge(opts))
 		; /* merge backend cleans up after itself */
 	else if (status == 0) {
 		if (!file_exists(state_dir_path("stopped-sha", opts)))
@@ -1293,7 +1298,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	}
 	case ACTION_QUIT: {
 		save_autostash(state_dir_path("autostash", &options));
-		if (options.type == REBASE_MERGE) {
+		if (is_merge(&options)) {
 			struct replay_opts replay = REPLAY_OPTS_INIT;
 
 			replay.action = REPLAY_INTERACTIVE_REBASE;
@@ -1406,7 +1411,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		imply_merge(&options, "--rebase-merges");
 	}
 
-	if (options.type == REBASE_APPLY) {
+	if (is_apply(&options)) {
 		if (ignore_whitespace)
 			strvec_push(&options.git_am_opts,
 				    "--ignore-whitespace");
@@ -1452,7 +1457,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		}
 	}
 
-	if (options.type == REBASE_MERGE)
+	if (is_merge(&options))
 		imply_merge(&options, "--merge");
 
 	if (options.root && !options.onto_name)
@@ -1461,7 +1466,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	if (isatty(2) && options.flags & REBASE_NO_QUIET)
 		strbuf_addstr(&options.git_format_patch_opt, " --progress");
 
-	if (options.git_am_opts.nr || options.type == REBASE_APPLY) {
+	if (options.git_am_opts.nr || is_apply(&options)) {
 		/* all am options except -q are compatible only with --apply */
 		for (i = options.git_am_opts.nr - 1; i >= 0; i--)
 			if (strcmp(options.git_am_opts.v[i], "-q"))
@@ -1486,7 +1491,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			    options.default_backend);
 	}
 
-	if (options.type == REBASE_MERGE &&
+	if (is_merge(&options) &&
 	    !options.strategy &&
 	    getenv("GIT_TEST_MERGE_ALGORITHM"))
 		options.strategy = xstrdup(getenv("GIT_TEST_MERGE_ALGORITHM"));