On Thu, Sep 02 2021, Johannes Schindelin wrote:
> 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@xxxxxx>
>> >
>> > 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"));
