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

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Elijah

On 24/01/2023 02:36, Elijah Newren wrote:
> 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

(C) is already an error
(D) is currently allowed and I think works as expected (--keep-base only 
implies --reapply-cherry-picks, the user is free to override that with 
--no-reapply-cherry-picks) so I don't see why we'd want to make it an error.

> For all four cases (A)-(D), the apply backend will ignore whatever
> --[no-]reapply-cherry-picks flag is provided.

For (D) the flag is respected, (C) errors out, the other cases 
correspond to the default so it's like saying

	git rebase --merge --no-reapply-cherry-picks

ignores the flag. Arguably it is confusing that the apply backend only 
supports -[-no]-reapply-cherry-picks if --keep-base is given but I'm not 
sure that is a good reason to reject a combination that currently works 
as expected.

Best Wishes

Phillip

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