Re: [PATCH v3 5/7] rebase: add coverage of other incompatible options

[Elijah Newren <newren@gmail.com>]

On Sat, Jan 21, 2023 at 11:25 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> On 21/01/2023 15:20, Phillip Wood wrote:
> >> @@ -1238,6 +1238,17 @@ int cmd_rebase(int argc, const char **argv,
> >> const char *prefix)
> >>           if (options.fork_point < 0)
> >>               options.fork_point = 0;
> >>       }
> >> +    /*
> >> +     * reapply_cherry_picks is slightly weird.  It starts out with a
> >> +     * value of -1.  It will be assigned a value of keep_base below and
> >> +     * then handled specially.  The apply backend is hardcoded to
> >> +     * behave like reapply_cherry_picks==1,
> >
> > I think it is hard coded to 0 not 1. We generate the patches with
> >
> >      git format-patch --right-only $upstream...$head
>
> Sorry I somhow managed to omit --cherry-pick from that, it should have been
>
>         git format-patch --right-only --cherry-pick $upstream...$head
>
> Phillip

Oh, indeed, I was reading wrong.  Thanks for the correction.  I still
think there's something to fix up here, which I'll include in my
re-roll.

> > so cherry-picks will not be reapplied. I'm hardly an impartial observer
> > but I think the current check is correct. We support
> >
> >      --whitespace=fix --no-reapply-cherry-picks
> >      --whitespace=fix --keep-base --reapply-cherry-picks
> >
> > but not
> >
> >      --whitespace=fix --reapply-cherry-picks

Right, nor do we support
      --whitespace=fix --keep-base --no-reapply-cherry-picks
(If you try it, you'll notice that the code accepts those flags and
starts the rebase pretending everything is fine, but it silently
ignores the --no-reapply-cherry-picks flag.)

Stepping back a bit, though, the apply backend doesn't support
toggling --[no-]reapply-cherry-picks at all.  It hard codes the
behavior (in a way dependent upon --keep-base).  So, if the user
passes --[no-]reapply-cherry-picks and we don't error out, we are just
going to ignore whatever they passed and do whatever hardcoded thing
is baked into the algorithm.

While the user can pass the flag in a way that happens to match what
is baked into the apply backend, I'd still say it's wrong for them to
specify it since we will just ignore it.  Doing so is likely to result
in the user later toggling the flag and being surprised that they get
an error when it used to be accepted, or seeing that it only sometimes
gets accepted.

Anyway, I'll update this patch to document this a bit more clearly in
a code comment and add an improved check.