From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Cc: Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com>,
git@vger.kernel.org, Eric Wong <e@80x24.org>
Subject: Re: [PATCH v2 5/7] rebase: drop support for `--preserve-merges`
Date: Thu, 02 Sep 2021 16:16:22 +0200 [thread overview]
Message-ID: <87tuj39hqd.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <nycvar.QRO.7.76.6.2109021555050.55@tvgsbejvaqbjf.bet>
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@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"));
next prev parent reply other threads:[~2021-09-02 14:26 UTC|newest]
Thread overview: 78+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-11-23 20:50 [PATCH 0/8] Drop support for git rebase --preserve-merges Johannes Schindelin via GitGitGadget
2019-11-23 20:50 ` [PATCH 1/8] t5520: do not use `pull.rebase=preserve` Johannes Schindelin via GitGitGadget
2019-11-23 20:50 ` [PATCH 2/8] remote: warn about unhandled branch.<name>.rebase values Johannes Schindelin via GitGitGadget
2019-11-23 20:50 ` [PATCH 3/8] tests: stop testing `git rebase --preserve-merges` Johannes Schindelin via GitGitGadget
2019-11-23 20:50 ` [PATCH 4/8] pull: remove support for `--rebase=preserve` Johannes Schindelin via GitGitGadget
2019-11-23 20:50 ` [PATCH 5/8] rebase: drop support for `--preserve-merges` Johannes Schindelin via GitGitGadget
2019-11-23 20:50 ` [PATCH 6/8] git-svn: " Johannes Schindelin via GitGitGadget
2019-11-23 22:08 ` Eric Wong
2019-11-24 21:29 ` Johannes Schindelin
2019-11-25 3:15 ` Eric Wong
2019-11-23 20:50 ` [PATCH 7/8] rebase: drop the internal `rebase--interactive` command Johannes Schindelin via GitGitGadget
2019-11-23 20:50 ` [PATCH 8/8] remote: no longer claim that branch.*.rebase=preserve is a thing Johannes Schindelin via GitGitGadget
2021-09-01 11:57 ` [PATCH v2 0/7] Drop support for git rebase --preserve-merges Johannes Schindelin via GitGitGadget
2021-09-01 11:57 ` [PATCH v2 1/7] t5520: do not use `pull.rebase=preserve` Johannes Schindelin via GitGitGadget
2021-09-01 11:57 ` [PATCH v2 2/7] remote: warn about unhandled branch.<name>.rebase values Johannes Schindelin via GitGitGadget
2021-09-01 11:57 ` [PATCH v2 3/7] tests: stop testing `git rebase --preserve-merges` Johannes Schindelin via GitGitGadget
2021-09-01 13:26 ` Ævar Arnfjörð Bjarmason
2021-09-01 11:57 ` [PATCH v2 4/7] pull: remove support for `--rebase=preserve` Johannes Schindelin via GitGitGadget
2021-09-01 11:57 ` [PATCH v2 5/7] rebase: drop support for `--preserve-merges` Johannes Schindelin via GitGitGadget
2021-09-01 12:21 ` Ævar Arnfjörð Bjarmason
2021-09-02 13:54 ` Johannes Schindelin
2021-09-02 14:11 ` Ævar Arnfjörð Bjarmason
2021-09-01 13:33 ` Ævar Arnfjörð Bjarmason
2021-09-02 13:59 ` Johannes Schindelin
2021-09-02 14:16 ` Ævar Arnfjörð Bjarmason [this message]
2021-09-02 14:28 ` Ævar Arnfjörð Bjarmason
2021-09-02 14:34 ` Ævar Arnfjörð Bjarmason
2021-09-02 14:56 ` Ævar Arnfjörð Bjarmason
2021-09-02 15:34 ` Ævar Arnfjörð Bjarmason
2021-09-04 19:41 ` Johannes Schindelin
2021-09-05 7:32 ` Ævar Arnfjörð Bjarmason
2021-09-05 22:36 ` Junio C Hamano
2021-09-06 10:15 ` Phillip Wood
2021-09-07 12:32 ` Johannes Schindelin
2021-09-07 15:31 ` Phillip Wood
2021-09-07 19:44 ` Johannes Schindelin
2021-09-01 11:57 ` [PATCH v2 6/7] git-svn: " Johannes Schindelin via GitGitGadget
2021-09-01 13:25 ` Ævar Arnfjörð Bjarmason
2021-09-02 14:00 ` Johannes Schindelin
2021-09-02 14:08 ` Johannes Schindelin
2021-09-01 11:57 ` [PATCH v2 7/7] rebase: drop the internal `rebase--interactive` command Johannes Schindelin via GitGitGadget
2021-09-06 10:10 ` Phillip Wood
2021-09-07 12:39 ` Johannes Schindelin
2021-09-01 13:37 ` [PATCH v2 0/7] Drop support for git rebase --preserve-merges Ævar Arnfjörð Bjarmason
2021-09-02 14:16 ` Johannes Schindelin
2021-09-02 14:51 ` Ævar Arnfjörð Bjarmason
2021-09-01 22:25 ` Junio C Hamano
2021-09-02 14:18 ` Johannes Schindelin
2021-09-02 20:06 ` Johannes Sixt
2021-09-07 17:33 ` Johannes Schindelin
2021-09-07 22:48 ` Elijah Newren
2021-09-10 12:08 ` Johannes Schindelin
2021-09-10 17:16 ` Elijah Newren
2021-09-13 11:24 ` merge-ort and --rebase-merges, was " Johannes Schindelin
2021-09-13 15:53 ` Elijah Newren
2021-09-06 6:58 ` Ævar Arnfjörð Bjarmason
2021-09-07 18:27 ` Junio C Hamano
2021-09-07 19:52 ` Ævar Arnfjörð Bjarmason
2021-09-04 22:30 ` Alban Gruin
2021-09-06 10:17 ` Phillip Wood
2021-09-07 12:48 ` Johannes Schindelin
2021-09-07 21:05 ` [PATCH v3 00/11] " Johannes Schindelin via GitGitGadget
2021-09-07 21:05 ` [PATCH v3 01/11] t5520: do not use `pull.rebase=preserve` Johannes Schindelin via GitGitGadget
2021-09-07 21:05 ` [PATCH v3 02/11] remote: warn about unhandled branch.<name>.rebase values Johannes Schindelin via GitGitGadget
2021-09-07 21:05 ` [PATCH v3 03/11] tests: stop testing `git rebase --preserve-merges` Johannes Schindelin via GitGitGadget
2021-09-07 21:05 ` [PATCH v3 04/11] pull: remove support for `--rebase=preserve` Johannes Schindelin via GitGitGadget
2021-09-07 21:05 ` [PATCH v3 05/11] rebase: drop support for `--preserve-merges` Johannes Schindelin via GitGitGadget
2021-09-10 14:53 ` Ævar Arnfjörð Bjarmason
2022-07-21 19:02 ` re-mentioning --preserve-merges in the docs (was: [PATCH v3 05/11] rebase: drop support for `--preserve-merges`) Ævar Arnfjörð Bjarmason
2022-07-21 20:15 ` re-mentioning --preserve-merges in the docs Junio C Hamano
2022-07-29 13:24 ` Johannes Schindelin
2021-09-07 21:05 ` [PATCH v3 06/11] git-svn: drop support for `--preserve-merges` Johannes Schindelin via GitGitGadget
2021-09-07 21:05 ` [PATCH v3 07/11] rebase: drop the internal `rebase--interactive` command Johannes Schindelin via GitGitGadget
2021-09-07 21:05 ` [PATCH v3 08/11] rebase: remove obsolete code comment Johannes Schindelin via GitGitGadget
2021-09-07 21:05 ` [PATCH v3 09/11] rebase: stop mentioning the -p option in comments Johannes Schindelin via GitGitGadget
2021-09-07 21:05 ` [PATCH v3 10/11] rebase: remove a no-longer-used function Johannes Schindelin via GitGitGadget
2021-09-07 21:05 ` [PATCH v3 11/11] sequencer: restrict scope of a formerly public function Johannes Schindelin via GitGitGadget
2021-09-08 1:30 ` [PATCH v3 00/11] Drop support for git rebase --preserve-merges Ævar Arnfjörð Bjarmason
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87tuj39hqd.fsf@evledraar.gmail.com \
--to=avarab@gmail.com \
--cc=Johannes.Schindelin@gmx.de \
--cc=e@80x24.org \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).