Re: [PATCH v2 5/7] rebase: drop support for `--preserve-merges`
From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2021-09-02 14:26:28
Subsystem:
the rest · Maintainer:
Linus Torvalds
On Thu, Sep 02 2021, Johannes Schindelin wrote:
Hi Ævar, On Wed, 1 Sep 2021, Ævar Arnfjörð Bjarmason wrote:quoted
On Wed, Sep 01 2021, Johannes Schindelin via GitGitGadget wrote:quoted
From: Johannes Schindelin <redacted> This option was deprecated in favor of `--rebase-merges` some time ago, and now we retire it.quoted
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"));