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

Commit Message

Elijah Newren Jan. 22, 2023, 6:12 a.m. UTC

From: Elijah Newren <[e-mail]>

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.

Signed-off-by: Elijah Newren <[e-mail]>
---
 Documentation/git-rebase.txt           |  2 +-
 builtin/rebase.c                       | 30 ++++++++++++++++++++------
 t/t3422-rebase-incompatible-options.sh | 25 +++++++++++++++++++++
 3 files changed, 49 insertions(+), 8 deletions(-)

Comments

Phillip Wood Jan. 23, 2023, 8:08 p.m. UTC | #1

Hi Elijah

On 22/01/2023 06:12, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <[e-mail]>
> 
> 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 <[e-mail]>
> --- 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.

Best Wishes

Phillip

> +	if (options.reapply_cherry_picks >= 0)
> +		imply_merge(&options, options.reapply_cherry_picks ? "--reapply-cherry-picks" :
> +								     "--no-reapply-cherry-picks");
> +
>   	/*
>   	 * --keep-base defaults to --reapply-cherry-picks to avoid losing
>   	 * commits when using this option.
> @@ -1406,13 +1426,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   	if (options.empty != EMPTY_UNSPECIFIED)
>   		imply_merge(&options, "--empty");
>   
> -	/*
> -	 * --keep-base implements --reapply-cherry-picks by altering upstream so
> -	 * it works with both backends.
> -	 */
> -	if (options.reapply_cherry_picks && !keep_base)
> -		imply_merge(&options, "--reapply-cherry-picks");
> -
>   	if (gpg_sign)
>   		options.gpg_sign_opt = xstrfmt("-S%s", gpg_sign);
>   
> @@ -1503,6 +1516,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   	if (options.update_refs)
>   		imply_merge(&options, "--update-refs");
>   
> +	if (options.autosquash)
> +		imply_merge(&options, "--autosquash");
> +
>   	if (options.type == REBASE_UNSPECIFIED) {
>   		if (!strcmp(options.default_backend, "merge"))
>   			imply_merge(&options, "--merge");
> diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
> index f86274990b0..6a17b571ec7 100755
> --- a/t/t3422-rebase-incompatible-options.sh
> +++ b/t/t3422-rebase-incompatible-options.sh
> @@ -50,6 +50,11 @@ test_rebase_am_only () {
>   		test_must_fail git rebase $opt --strategy-option=ours A
>   	"
>   
> +	test_expect_success "$opt incompatible with --autosquash" "
> +		git checkout B^0 &&
> +		test_must_fail git rebase $opt --autosquash A
> +	"
> +
>   	test_expect_success "$opt incompatible with --interactive" "
>   		git checkout B^0 &&
>   		test_must_fail git rebase $opt --interactive A
> @@ -60,6 +65,26 @@ test_rebase_am_only () {
>   		test_must_fail git rebase $opt --exec 'true' A
>   	"
>   
> +	test_expect_success "$opt incompatible with --keep-empty" "
> +		git checkout B^0 &&
> +		test_must_fail git rebase $opt --keep-empty A
> +	"
> +
> +	test_expect_success "$opt incompatible with --empty=..." "
> +		git checkout B^0 &&
> +		test_must_fail git rebase $opt --empty=ask A
> +	"
> +
> +	test_expect_success "$opt incompatible with --no-reapply-cherry-picks" "
> +		git checkout B^0 &&
> +		test_must_fail git rebase $opt --no-reapply-cherry-picks A
> +	"
> +
> +	test_expect_success "$opt incompatible with --reapply-cherry-picks" "
> +		git checkout B^0 &&
> +		test_must_fail git rebase $opt --reapply-cherry-picks A
> +	"
> +
>   	test_expect_success "$opt incompatible with --update-refs" "
>   		git checkout B^0 &&
>   		test_must_fail git rebase $opt --update-refs A

Elijah Newren Jan. 24, 2023, 2:36 a.m. UTC | #2

Hi Phillip,

On Mon, Jan 23, 2023 at 12:08 PM Phillip Wood <[e-mail]> wrote:
>
> Hi Elijah
>
> On 22/01/2023 06:12, Elijah Newren via GitGitGadget wrote:
> > From: Elijah Newren <[e-mail]>
> >
> > 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 <[e-mail]>
> > --- 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?

Phillip Wood Jan. 24, 2023, 10:27 a.m. UTC | #3

Hi Elijah

On 24/01/2023 02:36, Elijah Newren wrote:
> Hi Phillip,
> 
> On Mon, Jan 23, 2023 at 12:08 PM Phillip Wood <[e-mail]> wrote:
>>
>> Hi Elijah
>>
>> On 22/01/2023 06:12, Elijah Newren via GitGitGadget wrote:
>>> From: Elijah Newren <[e-mail]>
>>>
>>> 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 <[e-mail]>
>>> --- 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?

Phillip Wood Jan. 24, 2023, 1:16 p.m. UTC | #4

>>>> Signed-off-by: Elijah Newren <[e-mail]>
>>>> --- 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.

On reflection that is only true for (B). I agree that we should error 
out on (A) which we don't at the moment.

I'd support a change that errors out on (A) and (C) but continues to 
allow (B) and (D). I think we can do that with the diff below

Best Wishes

Phillip

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 1481c5b6a5..66aef356b8 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1230,12 +1230,6 @@ int cmd_rebase(int argc, const char **argv, const 
char *prefix)
                  if (options.fork_point < 0)
                          options.fork_point = 0;
          }
-        /*
-         * --keep-base defaults to --reapply-cherry-picks to avoid losing
-         * commits when using this option.
-         */
-        if (options.reapply_cherry_picks < 0)
-                options.reapply_cherry_picks = keep_base;

          if (options.root && options.fork_point > 0)
                  die(_("options '%s' and '%s' cannot be used 
together"), "--root", "--fork-point");
@@ -1412,11 +1406,17 @@ int cmd_rebase(int argc, const char **argv, 
const char *prefix)
          if (options.empty != EMPTY_UNSPECIFIED)
                  imply_merge(&options, "--empty");

-        /*
-         * --keep-base implements --reapply-cherry-picks by altering 
upstream so
-         * it works with both backends.
-         */
-        if (options.reapply_cherry_picks && !keep_base)
+        if (options.reapply_cherry_picks < 0)
+                /*
+                 * --keep-base defaults to --reapply-cherry-picks to
+                 * avoid losing commits when using this option.
+                 */
+                options.reapply_cherry_picks = keep_base;
+        else if (!keep_base)
+                /*
+                 * --keep-base implements --reapply-cherry-picks by
+                 * altering upstream so it works with both backends.
+                 */
                  imply_merge(&options, "--reapply-cherry-picks");

          if (gpg_sign)

Junio C Hamano Jan. 24, 2023, 2:48 p.m. UTC | #5

Phillip Wood <[e-mail]> writes:

> On reflection that is only true for (B). I agree that we should error
> out on (A) which we don't at the moment.
>
> I'd support a change that errors out on (A) and (C) but continues to
> allow (B) and (D). I think we can do that with the diff below

Thanks, both of you, for well reasoned design work and respectful
communication.

Elijah Newren Jan. 24, 2023, 3:41 p.m. UTC | #6

Hi Phillip,

On Tue, Jan 24, 2023 at 5:16 AM Phillip Wood <[e-mail]> wrote:
>
> >>>> Signed-off-by: Elijah Newren <[e-mail]>
> >>>> --- 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.

Ah, despite looking over the code multiple times to check my
statements, I somehow kept missing this:

    if (keep_base && options.reapply_cherry_picks)
        options.upstream = options.onto;

which is how --[no-]reapply-cherry-picks is supported in conjunction
with --keep-base.  Thanks.

> >> 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.
>
> On reflection that is only true for (B). I agree that we should error
> out on (A) which we don't at the moment.
>
> I'd support a change that errors out on (A) and (C) but continues to
> allow (B) and (D). I think we can do that with the diff below
>
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 1481c5b6a5..66aef356b8 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1230,12 +1230,6 @@ int cmd_rebase(int argc, const char **argv, const
> char *prefix)
>                   if (options.fork_point < 0)
>                           options.fork_point = 0;
>           }
> -        /*
> -         * --keep-base defaults to --reapply-cherry-picks to avoid losing
> -         * commits when using this option.
> -         */
> -        if (options.reapply_cherry_picks < 0)
> -                options.reapply_cherry_picks = keep_base;
>
>           if (options.root && options.fork_point > 0)
>                   die(_("options '%s' and '%s' cannot be used
> together"), "--root", "--fork-point");
> @@ -1412,11 +1406,17 @@ int cmd_rebase(int argc, const char **argv,
> const char *prefix)
>           if (options.empty != EMPTY_UNSPECIFIED)
>                   imply_merge(&options, "--empty");
>
> -        /*
> -         * --keep-base implements --reapply-cherry-picks by altering
> upstream so
> -         * it works with both backends.
> -         */
> -        if (options.reapply_cherry_picks && !keep_base)
> +        if (options.reapply_cherry_picks < 0)
> +                /*
> +                 * --keep-base defaults to --reapply-cherry-picks to
> +                 * avoid losing commits when using this option.
> +                 */

I know you were copying the previous comment, but this comment is
really confusing to me.  Shouldn't it read "--reapply-cherry-picks
defaults to --keep-base" instead of vice-versa?

> +                options.reapply_cherry_picks = keep_base;
> +        else if (!keep_base)
> +                /*
> +                 * --keep-base implements --reapply-cherry-picks by

Should this be --[no-]reapply-cherry-picks, to clarify that it handles
both cases?  Especially given how many times I missed it?

> +                 * altering upstream so it works with both backends.
> +                 */
>                   imply_merge(&options, "--reapply-cherry-picks");

And perhaps this should be

    imply_merge(&options, options.reapply_cherry_picks ?
"--reapply-cherry-picks" :
         "--no-reapply-cherry-picks");

Also, the comment in git-rebase.txt about incompatibilities would become

     * --[no-]reapply-cherry-picks, when --keep-base is not provided

Phillip Wood Jan. 24, 2023, 4:48 p.m. UTC | #7

Hi Elijah

On 24/01/2023 15:41, Elijah Newren wrote:
> Ah, despite looking over the code multiple times to check my
> statements, I somehow kept missing this:
> 
>      if (keep_base && options.reapply_cherry_picks)
>          options.upstream = options.onto;
> 
> which is how --[no-]reapply-cherry-picks is supported in conjunction
> with --keep-base.  Thanks.
> 
>>>> 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.
>>
>> On reflection that is only true for (B). I agree that we should error
>> out on (A) which we don't at the moment.
>>
>> I'd support a change that errors out on (A) and (C) but continues to
>> allow (B) and (D). I think we can do that with the diff below
>>
>>
>> diff --git a/builtin/rebase.c b/builtin/rebase.c
>> index 1481c5b6a5..66aef356b8 100644
>> --- a/builtin/rebase.c
>> +++ b/builtin/rebase.c
>> @@ -1230,12 +1230,6 @@ int cmd_rebase(int argc, const char **argv, const
>> char *prefix)
>>                    if (options.fork_point < 0)
>>                            options.fork_point = 0;
>>            }
>> -        /*
>> -         * --keep-base defaults to --reapply-cherry-picks to avoid losing
>> -         * commits when using this option.
>> -         */
>> -        if (options.reapply_cherry_picks < 0)
>> -                options.reapply_cherry_picks = keep_base;
>>
>>            if (options.root && options.fork_point > 0)
>>                    die(_("options '%s' and '%s' cannot be used
>> together"), "--root", "--fork-point");
>> @@ -1412,11 +1406,17 @@ int cmd_rebase(int argc, const char **argv,
>> const char *prefix)
>>            if (options.empty != EMPTY_UNSPECIFIED)
>>                    imply_merge(&options, "--empty");
>>
>> -        /*
>> -         * --keep-base implements --reapply-cherry-picks by altering
>> upstream so
>> -         * it works with both backends.
>> -         */
>> -        if (options.reapply_cherry_picks && !keep_base)
>> +        if (options.reapply_cherry_picks < 0)
>> +                /*
>> +                 * --keep-base defaults to --reapply-cherry-picks to
>> +                 * avoid losing commits when using this option.
>> +                 */
> 
> I know you were copying the previous comment, but this comment is
> really confusing to me.  Shouldn't it read "--reapply-cherry-picks
> defaults to --keep-base" instead of vice-versa?

Clearly this comment has not been as helpful as I indented it to be. I 
think maybe we should spell out the defaults with and without 
--keep-base. Perhaps something like

default to --no-reapply-cherry-picks unless --keep-base is given in 
which case default to --reapply-cherry-picks


>> +                options.reapply_cherry_picks = keep_base;
>> +        else if (!keep_base)
>> +                /*
>> +                 * --keep-base implements --reapply-cherry-picks by
> 
> Should this be --[no-]reapply-cherry-picks, to clarify that it handles
> both cases?  Especially given how many times I missed it?

This has obviously proved to be confusing. The aim was to explain that 
in order to work with the apply backend "[--reapply-cherry-picks] 
--keep-base" was doing something unusual with `upstream` to reapply 
cherry picks. "--no-reapply-cherry-picks --keep-base" does not do 
anything unusual with `upstream`. I don't think changing it to 
--[no-]reapply-cherry-picks quite captures that. I came up with

To support --reapply-cherry-picks (which is not supported by the apply 
backend) --keep-base alters upstream to prevent cherry picked commits 
from being dropped.

but it really needs to mention that --keep-base also supports 
--no-reapply-cherry-picks in the usual way

>> +                 * altering upstream so it works with both backends.
>> +                 */
>>                    imply_merge(&options, "--reapply-cherry-picks");
> 
> And perhaps this should be
> 
>      imply_merge(&options, options.reapply_cherry_picks ?
> "--reapply-cherry-picks" :
>           "--no-reapply-cherry-picks");

That's a good idea

> Also, the comment in git-rebase.txt about incompatibilities would become
> 
>       * --[no-]reapply-cherry-picks, when --keep-base is not provided

Yes, that's good

Thanks again for working on this

Phillip

Elijah Newren Jan. 24, 2023, 5:12 p.m. UTC | #8

On Tue, Jan 24, 2023 at 8:48 AM Phillip Wood <[e-mail]> wrote:
>
> Hi Elijah
>
> On 24/01/2023 15:41, Elijah Newren wrote:
[...]
> >> -        /*
> >> -         * --keep-base implements --reapply-cherry-picks by altering
> >> upstream so
> >> -         * it works with both backends.
> >> -         */
> >> -        if (options.reapply_cherry_picks && !keep_base)
> >> +        if (options.reapply_cherry_picks < 0)
> >> +                /*
> >> +                 * --keep-base defaults to --reapply-cherry-picks to
> >> +                 * avoid losing commits when using this option.
> >> +                 */
> >
> > I know you were copying the previous comment, but this comment is
> > really confusing to me.  Shouldn't it read "--reapply-cherry-picks
> > defaults to --keep-base" instead of vice-versa?
>
> Clearly this comment has not been as helpful as I indented it to be. I
> think maybe we should spell out the defaults with and without
> --keep-base. Perhaps something like
>
> default to --no-reapply-cherry-picks unless --keep-base is given in
> which case default to --reapply-cherry-picks

I like that; sounds good to me.

> >> +                options.reapply_cherry_picks = keep_base;
> >> +        else if (!keep_base)
> >> +                /*
> >> +                 * --keep-base implements --reapply-cherry-picks by
> >
> > Should this be --[no-]reapply-cherry-picks, to clarify that it handles
> > both cases?  Especially given how many times I missed it?
>
> This has obviously proved to be confusing. The aim was to explain that
> in order to work with the apply backend "[--reapply-cherry-picks]
> --keep-base" was doing something unusual with `upstream` to reapply
> cherry picks. "--no-reapply-cherry-picks --keep-base" does not do
> anything unusual with `upstream`. I don't think changing it to
> --[no-]reapply-cherry-picks quite captures that. I came up with
>
> To support --reapply-cherry-picks (which is not supported by the apply
> backend) --keep-base alters upstream to prevent cherry picked commits
> from being dropped.
>
> but it really needs to mention that --keep-base also supports
> --no-reapply-cherry-picks in the usual way

Somewhat wordy, but perhaps:

    /*
     * The apply backend always searches for and drops cherry
     * picks.  This is often not wanted with --keep-base, so
     * --keep-base allows --reapply-cherry-picks to be
     * simulated by altering the upstream such that
     * cherry-picks cannot be detected and thus all commits are
     * reapplied.  Thus, --[no-]reapply-cherry-picks is
     * supported when --keep-base is specified, but not when
     * --keep-base is left out.
     */

?

Phillip Wood Jan. 24, 2023, 7:21 p.m. UTC | #9

On 24/01/2023 17:12, Elijah Newren wrote:
> On Tue, Jan 24, 2023 at 8:48 AM Phillip Wood <[e-mail]> wrote:
>>>> +                options.reapply_cherry_picks = keep_base;
>>>> +        else if (!keep_base)
>>>> +                /*
>>>> +                 * --keep-base implements --reapply-cherry-picks by
>>>
>>> Should this be --[no-]reapply-cherry-picks, to clarify that it handles
>>> both cases?  Especially given how many times I missed it?
>>
>> This has obviously proved to be confusing. The aim was to explain that
>> in order to work with the apply backend "[--reapply-cherry-picks]
>> --keep-base" was doing something unusual with `upstream` to reapply
>> cherry picks. "--no-reapply-cherry-picks --keep-base" does not do
>> anything unusual with `upstream`. I don't think changing it to
>> --[no-]reapply-cherry-picks quite captures that. I came up with
>>
>> To support --reapply-cherry-picks (which is not supported by the apply
>> backend) --keep-base alters upstream to prevent cherry picked commits
>> from being dropped.
>>
>> but it really needs to mention that --keep-base also supports
>> --no-reapply-cherry-picks in the usual way
> 
> Somewhat wordy, but perhaps:
> 
>      /*
>       * The apply backend always searches for and drops cherry
>       * picks.  This is often not wanted with --keep-base, so
>       * --keep-base allows --reapply-cherry-picks to be
>       * simulated by altering the upstream such that
>       * cherry-picks cannot be detected and thus all commits are
>       * reapplied.  Thus, --[no-]reapply-cherry-picks is
>       * supported when --keep-base is specified, but not when
>       * --keep-base is left out.
>       */

That sounds good to me, it is definitely an improvement on the current 
comment which I think is too terse.

Best Wishes

Phillip

Patch

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 846aeed1b69..8cb075b2bdb 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -650,7 +650,7 @@  are incompatible with the following options:
  * --exec
  * --no-keep-empty
  * --empty=
- * --reapply-cherry-picks
+ * --[no-]reapply-cherry-picks
  * --edit-todo
  * --update-refs
  * --root when used without --onto
diff --git a/builtin/rebase.c b/builtin/rebase.c
index b742cc8fb5c..ed7475804cb 100644
--- 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.
+	 */
+	if (options.reapply_cherry_picks >= 0)
+		imply_merge(&options, options.reapply_cherry_picks ? "--reapply-cherry-picks" :
+								     "--no-reapply-cherry-picks");
+
 	/*
 	 * --keep-base defaults to --reapply-cherry-picks to avoid losing
 	 * commits when using this option.
@@ -1406,13 +1426,6 @@  int cmd_rebase(int argc, const char **argv, const char *prefix)
 	if (options.empty != EMPTY_UNSPECIFIED)
 		imply_merge(&options, "--empty");
 
-	/*
-	 * --keep-base implements --reapply-cherry-picks by altering upstream so
-	 * it works with both backends.
-	 */
-	if (options.reapply_cherry_picks && !keep_base)
-		imply_merge(&options, "--reapply-cherry-picks");
-
 	if (gpg_sign)
 		options.gpg_sign_opt = xstrfmt("-S%s", gpg_sign);
 
@@ -1503,6 +1516,9 @@  int cmd_rebase(int argc, const char **argv, const char *prefix)
 	if (options.update_refs)
 		imply_merge(&options, "--update-refs");
 
+	if (options.autosquash)
+		imply_merge(&options, "--autosquash");
+
 	if (options.type == REBASE_UNSPECIFIED) {
 		if (!strcmp(options.default_backend, "merge"))
 			imply_merge(&options, "--merge");
diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
index f86274990b0..6a17b571ec7 100755
--- a/t/t3422-rebase-incompatible-options.sh
+++ b/t/t3422-rebase-incompatible-options.sh
@@ -50,6 +50,11 @@  test_rebase_am_only () {
 		test_must_fail git rebase $opt --strategy-option=ours A
 	"
 
+	test_expect_success "$opt incompatible with --autosquash" "
+		git checkout B^0 &&
+		test_must_fail git rebase $opt --autosquash A
+	"
+
 	test_expect_success "$opt incompatible with --interactive" "
 		git checkout B^0 &&
 		test_must_fail git rebase $opt --interactive A
@@ -60,6 +65,26 @@  test_rebase_am_only () {
 		test_must_fail git rebase $opt --exec 'true' A
 	"
 
+	test_expect_success "$opt incompatible with --keep-empty" "
+		git checkout B^0 &&
+		test_must_fail git rebase $opt --keep-empty A
+	"
+
+	test_expect_success "$opt incompatible with --empty=..." "
+		git checkout B^0 &&
+		test_must_fail git rebase $opt --empty=ask A
+	"
+
+	test_expect_success "$opt incompatible with --no-reapply-cherry-picks" "
+		git checkout B^0 &&
+		test_must_fail git rebase $opt --no-reapply-cherry-picks A
+	"
+
+	test_expect_success "$opt incompatible with --reapply-cherry-picks" "
+		git checkout B^0 &&
+		test_must_fail git rebase $opt --reapply-cherry-picks A
+	"
+
 	test_expect_success "$opt incompatible with --update-refs" "
 		git checkout B^0 &&
 		test_must_fail git rebase $opt --update-refs A