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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

[-- Attachment #1: Type: text/plain, Size: 3024 bytes --]

Hi Ævar,

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

>
> On Wed, Sep 01 2021, Johannes Schindelin via GitGitGadget wrote:
>
> > 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