Re: [PATCH v2 0/7] Drop support for git rebase --preserve-merges

Hi Ævar,

On Wed, 1 Sep 2021, Ævar Arnfjörð Bjarmason wrote:

On Wed, Sep 01 2021, Johannes Schindelin via GitGitGadget wrote:

quoted

In 427c3bd28ab (rebase: deprecate --preserve-merges, 2019-03-11) (which was
included in v2.22.0), we officially deprecated the --preserve-merges
backend. Over two years later, it is time to drop that backend, and here is
a patch series that does just that.

Changes since v1:

 * Rebased onto v2.33.0

I'm very much in favor if this series.

I independently came up with pretty much the same at the beginning of
last month before finding your v1 in the list archive. The comments I
left inline are based on the diff between our two versions, i.e. I found
some spots you missed (and your version has spots I missed).

You're also leaving behind a comment in builtin/rebase.c referring to
PRESERVE_MERGES. Perhaps we should just turn that into an "else if"
while we're at it (the "ignore-space-change" argument won't need
wrapping anymore then):

builtin/rebase.c-       } else {
builtin/rebase.c:               /* REBASE_MERGE and PRESERVE_MERGES */
builtin/rebase.c-               if (ignore_whitespace) {
builtin/rebase.c-                       string_list_append(&strategy_options,
builtin/rebase.c-                                          "ignore-space-change");
builtin/rebase.c-               }
builtin/rebase.c-       }

While it would be technically correct to turn this into an `else if`, by
all other standards it would be incorrect. As I commented on your earlier
comment: just because it uses less lines does not make the intention
clearer. In this instance, I am actually quite certain that it dilutes the
intention. The `if (options.type == REBASE_APPLY)` clearly indicates a
top-level decision on the rebase backend, and an `else if
(ignore_whitespace)` would disrupt that idea to be about distinguishing
between completely unrelated concerns.

In other words: while I accept that your taste would prefer the suggested
change, my taste prefers the opposite, and since I am the owner of this
patch series contribution, I go with my taste.

I do find the left-behind "enum rebase_type" rather unpleasant, i.e. we
have a REBASE_UNSPECIFIED during the initial parse_options() phase, and
after that just REBASE_{APPLY,MERGE}, but some of those codepaths use
switch(), some just check on or the other, and it's not immediately
obvious where we are in the control flow. Ideally we'd take care of
parsing out the option(s) early, and could just have an "int
rebase_apply" in the struct to clearly indicate the rarer cases where we
take the REBASE_APPLY path.

Thank you for offering your opinion.

This encourages me to offer my (differing) opinion to keep the `enum
rebase_type` to clarify that we have multiple rebase backends, and even
leave Git's source code open to future contributions of other rebase
backends.

Ciao,
Dscho