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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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:
>>
>> > 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.

>> 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.