Re: [PATCH v4 5/9] rebase: add coverage of other incompatible options

[Elijah Newren <newren@gmail.com>]

Hi Phillip,

On Mon, Jan 23, 2023 at 12:08 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> Hi Elijah
>
> On 22/01/2023 06:12, Elijah Newren via GitGitGadget wrote:
> > From: Elijah Newren <newren@gmail.com>
> >
> > The git-rebase manual noted several sets of incompatible options, but
> > we were missing tests for a few of these.  Further, we were missing
> > code checks for some of these, which could result in command line
> > options being silently ignored.
> >
> > Also, note that adding a check for autosquash means that using
> > --whitespace=fix together with the config setting rebase.autosquash=true
> > will trigger an error.  A subsequent commit will improve the error
> > message.
>
> Thanks for updating the commit message and for the new commits at the
> end of the series.
>
> > Signed-off-by: Elijah Newren <newren@gmail.com>
> > --- a/builtin/rebase.c
> > +++ b/builtin/rebase.c
> > @@ -1224,6 +1224,26 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> >               if (options.fork_point < 0)
> >                       options.fork_point = 0;
> >       }
> > +     /*
> > +      * The apply backend does not support --[no-]reapply-cherry-picks.
> > +      * The behavior it implements by default is equivalent to
> > +      * --no-reapply-cherry-picks (due to passing --cherry-picks to
> > +      * format-patch), but --keep-base alters the upstream such that no
> > +      * cherry-picks can be found (effectively making it act like
> > +      * --reapply-cherry-picks).
> > +      *
> > +      * Now, if the user does specify --[no-]reapply-cherry-picks, but
> > +      * does so in such a way that options.reapply_cherry_picks ==
> > +      * keep_base, then the behavior they get will match what they
> > +      * expect despite options.reapply_cherry_picks being ignored.  We
> > +      * could just allow the flag in that case, but it seems better to
> > +      * just alert the user that they've specified a flag that the
> > +      * backend ignores.
> > +      */
>
> I'm a bit confused by this. --keep-base works with either
> --reapply-cherry-picks (which is the default if --keep-base is given) or
> --no-reapply-cherry-picks. Just below this hunk we have
>
>         if (options.reapply_cherry_picks < 0)
>                 options.reapply_cherry_picks = keep_base;
>
> So we only set options.reapply_cherry_picks to match keep_base if the
> user did not specify -[-no]-reapply-cherry-picks on the commandline.

options.reapply_cherry_picks is totally ignored by the apply backend,
regardless of whether it's set by the user or the setup code in
builtin/rebase.c.  And if we have an option which is ignored, isn't it
nicer to provide an error message to the user if they tried to set it?

Said another way, while users could start with these command lines:

    (Y) git rebase --whitespace=fix
    (Z) git rebase --whitespace=fix --keep-base

and modify them to include flags that would be ignored, we could allow:

    (A) git rebase --whitespace=fix --no-reapply-cherry-picks
    (B) git rebase --whitespace=fix --keep-base --reapply-cherry-picks

But we could not allow commands like

    (C) git rebase --whitespace=fix --reapply-cherry-picks
    (D) git rebase --whitespace=fix --keep-base --no-reapply-cherry-picks

For all four cases (A)-(D), the apply backend will ignore whatever
--[no-]reapply-cherry-picks flag is provided.  For (A) and (B), the
behavior the apply backend provides happens to match what the user
is requesting, while for (C) and (D) the behavior does not match.
So we should at least reject (C) and (D).  But, although we could
technically allow (A) and (B), what advantage would it provide?  I
think the results of allowing those two commands would be:

    1) Confusion by end users -- why should (C) & (D) throw errors if
       (A) and (B) are accepted?  That's not an easy rule to understand.

    2) More confusion by end users -- the documentation for years has
       stated that --reapply-cherry-picks is incompatible with the apply
       backend, suggesting users would be surprised if at least (B) and
       probably (A) didn't throw error messages.

    3) Confusing documentation -- If we don't want to throw errors for
       (A) and (B), how do we modify the "INCOMPATIBLE OPTIONS" section
       of Documentation/git-rebase.txt to explain the relevant details
       of when these flags are (or are not) incompatible with the apply
       backend?   I think it'd end up with a very verbose explanation
       that likely confuses more than it helps.

    4) Excessively complicated code -- The previous attempts to
       implement this got it wrong.  Prior to ce5238a690 ("rebase
       --keep-base: imply --reapply-cherry-picks", 2022-10-17), the code
       would error out on (B) and (C).  After that commit, it would only
       error out on (C).  Both solutions are incorrect since they miss
       (D), and I think the code just becomes hard to hard to follow in
       order to only error out on both (C) and (D) without (A) and (B).

(#2 and #3 might just be a repeat of the same issue, documentation,
but it seemed easier to write separately.)

I think it's simpler for the code, for the documentation, and for end
users to just error out on all of (A), (B), (C), and (D).
 --[no-]reapply-cherry-picks is not supported by the apply backend.

But, given this lengthy email, perhaps I should split out the handling
of --[no-]reapply-cherry-picks into its own commit and copy some or
all of the description above into the commit message?