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