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- }
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.
But I wasn't able to make that work with some quick hacking, and in any
case that can be left as a cleanup for later.
Ideally with the code comments I left addressed, but either way the
correctness of the end result isn't affected in terms of what the
program ends up doing:
Reviewed-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>
