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

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

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.

Sounds good.

quoted

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.

Just to clarify this isn't a comment describing the suggested is_merge()
changes I had elsewhere, or that we should get rid of "enum
rebase_type".

But rather that if we were to split up the "we haven't decided on the
type yet" from "we know the rebase type, let's act on it" which only
happens around the option/config/directory introspection etc. from the
rest of the code, we could stick on the "real" rebase types in the enum.

We could then have just two enum arms for switches (less verbosity) and
no "default: BUG(...)" case.

Such a change should make it easier for contributors to add new
backends, as they'd get the compiler helping them to see where they'd
need to add their code. Now you need to hunt around for various
is_merge, "opts.type == " etc, switches with "default" etc. comparisons.

Of course such a change might not be worth it, i.e. maybe it's too
painful to untangle REBASE_UNSPECIFIED from the existing enum. I just
wanted to clarify what it was that I was suggesting here.