v4, 3 partsrebase: add documentation and test for --no-rebase-merges

> The unusual syntax --rebase-merges="" (that is, --rebase-merges with an
> empty string argument) has been an undocumented synonym of
> --rebase-merges=no-rebase-cousins. Stop accepting that syntax to avoid
> confusion when a rebase.merges config option is introduced, where
> rebase.merges="" will be equivalent to not passing --rebase-merges.

> On Wed, 22 Feb 2023, Alex Henrie wrote:
>
>> The unusual syntax --rebase-merges="" (that is, --rebase-merges with an
>> empty string argument) has been an undocumented synonym of
>> --rebase-merges=no-rebase-cousins. Stop accepting that syntax to avoid
>> confusion when a rebase.merges config option is introduced, where
>> rebase.merges="" will be equivalent to not passing --rebase-merges.
>
> Being undocumented and obscure might be a good reason for some to consider
> this a bug; I do not. You could deprecate it, but there are probably
> better ideas than to remove it without prior warning.
>
> If all you want to do is to support `rebase.merges=`, you can do that
> without having to change the meaning of `--rebase-merges=`.

>
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > On Wed, 22 Feb 2023, Alex Henrie wrote:
> >
> >> The unusual syntax --rebase-merges="" (that is, --rebase-merges with an
> >> empty string argument) has been an undocumented synonym of
> >> --rebase-merges=no-rebase-cousins. Stop accepting that syntax to avoid
> >> confusion when a rebase.merges config option is introduced, where
> >> rebase.merges="" will be equivalent to not passing --rebase-merges.
> >
> > Being undocumented and obscure might be a good reason for some to consider
> > this a bug; I do not. You could deprecate it, but there are probably
> > better ideas than to remove it without prior warning.
> >
> > If all you want to do is to support `rebase.merges=`, you can do that
> > without having to change the meaning of `--rebase-merges=`.
>
> Thanks for a doze of sanity.  Let me mark the topic as on hold to
> wait for a resolution.

> As Johannes pointed out, it's going to be confusing if
> rebase.merges="" does not mean rebase.merges=false. It's also going to
> be confusing if rebase.merges="" does not mean --rebase-merges="". The
> only sane option I see is to drop --rebase-merges="", or to add a
> deprecation warning now and drop it later.

>
> Alex Henrie <alexhenrie24@gmail.com> writes:
>
> > As Johannes pointed out, it's going to be confusing if
> > rebase.merges="" does not mean rebase.merges=false. It's also going to
> > be confusing if rebase.merges="" does not mean --rebase-merges="". The
> > only sane option I see is to drop --rebase-merges="", or to add a
> > deprecation warning now and drop it later.
>
> If we were doing anything, then I think the only sensible way
> forward is to warn and then drop long after everybody forgets about
> it.
>
> But does it really need to be changed?  I am perfectly happy to
> declare that those who wants to set rebase.merges to say false
> should set it to false (or no or 0), not an empty string.
>
> Also, "[rebase] merges = " in the configuration files does not have
> to mean the same thing as "--rebase-merges=" from the command line.
> Can't we just reserve that strange "--rebase-merges=" given from the
> command line to those who are already using it, without even
> advertising it in the documentation, document and encourage the
> longhand "--rebase-merges=no-rebase-cousins" from the command line
> and take only the  form that corresponds to it in the configuration
> file, i.e. "[rebase] merges = no-rebase-cousins".  We could even
> error out "[rebase] merges = " in the configuration file, as nobody
> is using such a configuration variable today.

> The only way to truly make "[rebase] merges =" invalid is to print an
> error message and die with that configuration. I think that would be
> confusing too, especially since it's now looking like rebase.merges
> needs to be a pure boolean and an independent rebase.cousins boolean
> option is needed as well.

>
> Alex Henrie <alexhenrie24@gmail.com> writes:
>
> > The only way to truly make "[rebase] merges =" invalid is to print an
> > error message and die with that configuration. I think that would be
> > confusing too, especially since it's now looking like rebase.merges
> > needs to be a pure boolean and an independent rebase.cousins boolean
> > option is needed as well.
>
> Oh, I wasn't aware of that direction.
>
> I do not know why rebase.cousins, which would only be meaningful
> when rebase.merges is true, is a better design than rebase.merges
> that is an enum of "don't do the merges stuff" plus "do the merges
> stuff with cousins", "without cousins" (which may allow us to gain
> more different ways to do "merges stuff" later), but that is what
> gained consensus on the list, then "[rebase]merges=" would become a
> problem.
>
> But --rebase-merges from the command line is not a pure Boolean
> already, so what does "[rebase]merges" that is a pure Boolean aim to
> help?

>
> Alex Henrie <alexhenrie24@gmail.com> writes:
>
> > Phillip is concerned about people and scripts assuming that
> > --rebase-merges is equivalent to --rebase-merges=no-rebase-cousins,
> > see [1].
>
> Isn't that already broken when you introduce rebase.merges
> configuration?  People and scripts are already relying on the lack
> of rebase-merges to flatten, and script writers will be surprised to
> receive a "bug report" complaining that their script does not work
> when the users set rebase.merges to anything but no.

> > Tao and others are probably not going to like it if --rebase-merges
> > without an argument undoes a rebase.merges=rebase-cousins
> > configuration.
>
> That is why I suggested to keep --rebase-merges= (with no value or
> an empty string) only for those who came from the world where it
> defaults to no-rebase-cousins and there was no rebase.merges
> configuration.  If --rebase-merges= is given from the command line
> without value *and* rebase.merges configuration is there (which is
> Tao's concern?), the command line option can error out asking for an
> explicit value to countermand whatever value is configured.
>
> Wouldn't that work for folks from both camps?

> Alex Henrie <alexhenrie24@gmail.com> writes:
> 
>> Phillip is concerned about people and scripts assuming that
>> --rebase-merges is equivalent to --rebase-merges=no-rebase-cousins,
>> see [1].
> 
> Isn't that already broken when you introduce rebase.merges
> configuration?

>> Tao and others are probably not going to like it if --rebase-merges
>> without an argument undoes a rebase.merges=rebase-cousins
>> configuration.
> 
> That is why I suggested to keep --rebase-merges= (with no value or
> an empty string) only for those who came from the world where it
> defaults to no-rebase-cousins and there was no rebase.merges
> configuration.  If --rebase-merges= is given from the command line
> without value *and* rebase.merges configuration is there (which is
> Tao's concern?), the command line option can error out asking for an
> explicit value to countermand whatever value is configured.
> 
> Wouldn't that work for folks from both camps?

>
> On 24/02/2023 19:13, Junio C Hamano wrote:
> > Alex Henrie <alexhenrie24@gmail.com> writes:
> >
> >> Phillip is concerned about people and scripts assuming that
> >> --rebase-merges is equivalent to --rebase-merges=no-rebase-cousins,
> >> see [1].
> >
> > Isn't that already broken when you introduce rebase.merges
> > configuration?
>
> Scripts using --rebase-merges are not broken by the introduction of
> rebase.merges so long as we follow our usual convention of always
> allowing the commandline to override the config (i.e. --rebase-merges is
> always equivalent to --rebase-merges=no-rebase-cousins). I don't really
> understand why Alex is suggesting splitting the config into two based on
> my comments.

> > People and scripts are already relying on the lack
> > of rebase-merges to flatten, and script writers will be surprised to
> > receive a "bug report" complaining that their script does not work
> > when the users set rebase.merges to anything but no.
>
> That is true.

> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index b68fc2fbb7..45cf445d42 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -771,6 +771,20 @@ static int run_specific_rebase(struct rebase_options *opts)
>  	return status ? -1 : 0;
>  }
>
> +static void parse_merges_value(struct rebase_options *options, const char *value)
> +{
> +	if (value) {
> +		if (!strcmp("no-rebase-cousins", value))

> +			options->rebase_cousins = 0;
> +		else if (!strcmp("rebase-cousins", value))
> +			options->rebase_cousins = 1;
> +		else
> +			die(_("Unknown mode: %s"), value);
> +	}
> +
> +	options->rebase_merges = 1;

> +}
> +
>  static int rebase_config(const char *var, const char *value, void *data)
>  {
>  	struct rebase_options *opts = data;
> @@ -815,6 +829,13 @@ static int rebase_config(const char *var, const char *value, void *data)
>  		return 0;
>  	}
>
> +	if (!strcmp(var, "rebase.merges") && value && *value) {

> +		opts->rebase_merges = git_parse_maybe_bool(value);
> +		if (opts->rebase_merges < 0)
> +			parse_merges_value(opts, value);
> +		return 0;
> +	}
> +
>  	if (!strcmp(var, "rebase.backend")) {
>  		return git_config_string(&opts->default_backend, var, value);
>  	}
> @@ -980,6 +1001,18 @@ static int parse_opt_empty(const struct option *opt, const char *arg, int unset)
>  	return 0;
>  }
>
> +static int parse_opt_merges(const struct option *opt, const char *arg, int unset)
> +{
> +	struct rebase_options *options = opt->value;
> +
> +	if (unset)
> +		options->rebase_merges = 0;
> +	else
> +		parse_merges_value(options, arg);
> +
> +	return 0;
> +}
> +

>  static void NORETURN error_on_missing_default_upstream(void)
>  {
>  	struct branch *current_branch = branch_get(NULL);
> @@ -1035,7 +1068,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  	struct object_id branch_base;
>  	int ignore_whitespace = 0;
>  	const char *gpg_sign = NULL;
> -	const char *rebase_merges = NULL;
>  	struct string_list strategy_options = STRING_LIST_INIT_NODUP;
>  	struct object_id squash_onto;
>  	char *squash_onto_name = NULL;
> @@ -1137,10 +1169,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  			   &options.allow_empty_message,
>  			   N_("allow rebasing commits with empty messages"),
>  			   PARSE_OPT_HIDDEN),
> -		{OPTION_STRING, 'r', "rebase-merges", &rebase_merges,
> -			N_("mode"),
> +		OPT_CALLBACK_F('r', "rebase-merges", &options, N_("mode"),
>  			N_("try to rebase merges instead of skipping them"),
> -			PARSE_OPT_OPTARG, NULL, (intptr_t)"no-rebase-cousins"},
> +			PARSE_OPT_OPTARG, parse_opt_merges),
>  		OPT_BOOL(0, "fork-point", &options.fork_point,
>  			 N_("use 'merge-base --fork-point' to refine upstream")),
>  		OPT_STRING('s', "strategy", &options.strategy,
> @@ -1436,14 +1467,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  	if (options.exec.nr)
>  		imply_merge(&options, "--exec");
>
> -	if (rebase_merges) {
> -		if (!strcmp("rebase-cousins", rebase_merges))
> -			options.rebase_cousins = 1;
> -		else if (strcmp("no-rebase-cousins", rebase_merges))
> -			die(_("Unknown mode: %s"), rebase_merges);
> -		options.rebase_merges = 1;
> +	if (options.rebase_merges)
>  		imply_merge(&options, "--rebase-merges");
> -	}
>
>  	if (options.type == REBASE_APPLY) {
>  		if (ignore_whitespace)
> diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
> index c73949df18..d4b0e8fd49 100755
> --- a/t/t3430-rebase-merges.sh
> +++ b/t/t3430-rebase-merges.sh
> @@ -284,6 +284,102 @@ test_expect_success '--rebase-merges="" is invalid syntax' '
>  	test_cmp expect actual
>  '
>
> +test_expect_success 'rebase.merges="" is equivalent to not passing --rebase-merges' '
> +	test_config rebase.merges "" &&
> +	git checkout -b config-merges-blank E &&
> +	git rebase C &&
> +	test_cmp_graph C.. <<-\EOF
> +	* B
> +	* D
> +	o C
> +	EOF
> +'
> +
> +test_expect_success 'rebase.merges=rebase-cousins is equivalent to --rebase-merges=rebase-cousins' '
> +	test_config rebase.merges rebase-cousins &&
> +	git checkout -b config-rebase-cousins main &&
> +	git rebase HEAD^ &&
> +	test_cmp_graph HEAD^.. <<-\EOF
> +	*   Merge the topic branch '\''onebranch'\''
> +	|\
> +	| * D
> +	| * G
> +	|/
> +	o H
> +	EOF
> +'
> +
> +test_expect_success '--no-rebase-merges overrides rebase.merges=no-rebase-cousins' '
> +	test_config rebase.merges no-rebase-cousins &&
> +	git checkout -b override-config-no-rebase-cousins E &&
> +	git rebase --no-rebase-merges C &&
> +	test_cmp_graph C.. <<-\EOF
> +	* B
> +	* D
> +	o C
> +	EOF
> +'
> +
> +test_expect_success '--rebase-merges=no-rebase-cousins overrides rebase.merges=rebase-cousins' '
> +	test_config rebase.merges rebase-cousins &&
> +	git checkout -b override-config-rebase-cousins main &&
> +	git rebase --rebase-merges=no-rebase-cousins HEAD^ &&
> +	test_cmp_graph HEAD^.. <<-\EOF
> +	*   Merge the topic branch '\''onebranch'\''
> +	|\
> +	| * D
> +	| * G
> +	o | H
> +	|/
> +	o A
> +	EOF
> +'
> +
> +test_expect_success '--rebase-merges overrides rebase.merges=false' '
> +	test_config rebase.merges false &&
> +	git checkout -b override-config-merges-false E &&
> +	before="$(git rev-parse --verify HEAD)" &&
> +	test_tick &&
> +	git rebase --rebase-merges C &&
> +	test_cmp_rev HEAD $before
> +'
> +
> +test_expect_success '--rebase-merges does not override rebase.merges=rebase-cousins' '
> +	test_config rebase.merges rebase-cousins &&
> +	git checkout -b no-override-config-rebase-cousins main &&
> +	git rebase --rebase-merges HEAD^ &&
> +	test_cmp_graph HEAD^.. <<-\EOF
> +	*   Merge the topic branch '\''onebranch'\''
> +	|\
> +	| * D
> +	| * G
> +	|/
> +	o H
> +	EOF
> +'
> +
> +test_expect_success 'local rebase.merges=false overrides global rebase.merges=true' '
> +	test_config_global rebase.merges true &&
> +	test_config rebase.merges false &&
> +	git checkout -b override-global-config E &&
> +	git rebase C &&
> +	test_cmp_graph C.. <<-\EOF
> +	* B
> +	* D
> +	o C
> +	EOF
> +'
> +
> +test_expect_success 'local rebase.merges="" does not override global rebase.merges=true' '
> +	test_config_global rebase.merges no-rebase-cousins &&
> +	test_config rebase.merges "" &&
> +	git checkout -b no-override-global-config E &&
> +	before="$(git rev-parse --verify HEAD)" &&
> +	test_tick &&
> +	git rebase C &&
> +	test_cmp_rev HEAD $before
> +'
> +

>  test_expect_success 'refs/rewritten/* is worktree-local' '
>  	git worktree add wt &&
>  	cat >wt/script-from-scratch <<-\EOF &&
> --
> 2.39.2
>
>

> in config value parsing, a "maybe-bool" with a
> NULL value is considered to be equivalent to `true`! (See
> `git_parse_maybe_bool_text()` or
> https://git-scm.com/docs/git-config#Documentation/git-config.txt-true for
> details.).

> On Wed, 22 Feb 2023, Alex Henrie wrote:
>
> > +     if (!strcmp(var, "rebase.merges") && value && *value) {
>
> Why do we require a non-empty `value` here?
>
>         [rebase]
>                 merges
>
> should be equivalent to `true`,
>
>         [rebase]
>                 merges =
>
> should probably be equivalent to `false`, and both are handled correctly
> by `git_parse_maybe_bool()`.

> The purpose of the new option is to accommodate users who would like
> --rebase-merges to be on by default and to facilitate possibly turning
> on --rebase-merges by default without configuration in a future version
> of Git.
> 
> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
> ---
>   Documentation/config/rebase.txt | 10 ++++
>   Documentation/git-rebase.txt    |  3 +-
>   builtin/rebase.c                | 47 ++++++++++++----
>   t/t3430-rebase-merges.sh        | 96 +++++++++++++++++++++++++++++++++
>   4 files changed, 144 insertions(+), 12 deletions(-)
> 
> diff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt
> index f19bd0e040..308baa9dbb 100644
> --- a/Documentation/config/rebase.txt
> +++ b/Documentation/config/rebase.txt
> @@ -67,3 +67,13 @@ rebase.rescheduleFailedExec::
>   
>   rebase.forkPoint::
>   	If set to false set `--no-fork-point` option by default.
> +
> +rebase.merges::
> +	Whether and how to set the `--rebase-merges` option by default. Can
> +	be `rebase-cousins`, `no-rebase-cousins`, or a boolean. Setting to
> +	true is equivalent to `--rebase-merges` without an argument, setting to
> +	`rebase-cousins` or `no-rebase-cousins` is equivalent to
> +	`--rebase-merges` with that value as its argument, and setting to false
> +	is equivalent to `--no-rebase-merges`. Passing `--rebase-merges` on the
> +	command line without an argument overrides a `rebase.merges=false`
> +	configuration but does not override other values of `rebase.merge`.

> [...]   
> +static void parse_merges_value(struct rebase_options *options, const char *value)
> +{
> +	if (value) {
> +		if (!strcmp("no-rebase-cousins", value))
> +			options->rebase_cousins = 0;
> +		else if (!strcmp("rebase-cousins", value))
> +			options->rebase_cousins = 1;
> +		else
> +			die(_("Unknown mode: %s"), value);
> +	}
> +
> +	options->rebase_merges = 1;
> +}

> @@ -1436,14 +1467,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   	if (options.exec.nr)
>   		imply_merge(&options, "--exec");
>   
> -	if (rebase_merges) {
> -		if (!strcmp("rebase-cousins", rebase_merges))
> -			options.rebase_cousins = 1;
> -		else if (strcmp("no-rebase-cousins", rebase_merges))
> -			die(_("Unknown mode: %s"), rebase_merges);
> -		options.rebase_merges = 1;
> +	if (options.rebase_merges)
>   		imply_merge(&options, "--rebase-merges");
> -	}

> [...]
> +test_expect_success '--rebase-merges does not override rebase.merges=rebase-cousins' '
> +	test_config rebase.merges rebase-cousins &&
> +	git checkout -b no-override-config-rebase-cousins main &&
> +	git rebase --rebase-merges HEAD^ &&

> [...]
> +test_expect_success 'local rebase.merges=false overrides global rebase.merges=true' '
> +	test_config_global rebase.merges true &&
> +	test_config rebase.merges false &&
> +	git checkout -b override-global-config E &&
> +	git rebase C &&
> +	test_cmp_graph C.. <<-\EOF
> +	* B
> +	* D
> +	o C
> +	EOF
> +'
> +
> +test_expect_success 'local rebase.merges="" does not override global rebase.merges=true' '
> +	test_config_global rebase.merges no-rebase-cousins &&
> +	test_config rebase.merges "" &&
> +	git checkout -b no-override-global-config E &&
> +	before="$(git rev-parse --verify HEAD)" &&
> +	test_tick &&
> +	git rebase C &&
> +	test_cmp_rev HEAD $before
> +'

> Alex Henrie <alexhenrie24@gmail.com> writes:
>
> > Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
> > ---
>
> No cover letter to summarize the changes?

> On Thu, 23 Feb 2023, Junio C Hamano wrote:
>
>> Alex Henrie <alexhenrie24@gmail.com> writes:
>>
>> > Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
>> > ---
>>
>> No cover letter to summarize the changes?
>
> A range-diff would have been nice, too, as well as replying-to the
> previous iteration so that they're all within the same email thread.

> A range-diff would have been nice, too, as well as replying-to the
> previous iteration so that they're all within the same email thread.
>
> And if you want cover letters and range-diffs and correct In-Reply-To
> headers, I can think of a splendid way to encourage that: promote tools
> that do that. That's much better than to require contributors to know all
> the customs and conventions of the project and send mails in the exact
> desired format...

>  -r::
>  --rebase-merges[=(rebase-cousins|no-rebase-cousins)]::
> +--no-rebase-merges::
>  	By default, a rebase will simply drop merge commits from the todo
>  	list, and put the rebased commits into a single, linear branch.
>  	With `--rebase-merges`, the rebase will instead try to preserve
>  	the branching structure within the commits that are to be rebased,
>  	by recreating the merge commits. Any resolved merge conflicts or
>  	manual amendments in these merge commits will have to be
> -	resolved/re-applied manually.
> +	resolved/re-applied manually. `--no-rebase-merges` can be used to
> +	countermand a previous `--rebase-merges`.
>  +
>  By default, or when `no-rebase-cousins` was specified, commits which do not
>  have `<upstream>` as direct ancestor will keep their original branch point,
> diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh

>  	if (rebase_merges) {
>  		if (!*rebase_merges)
> -			; /* default mode; do nothing */
> +			warning(_("--rebase-merges with an empty string "
> +				  "argument is deprecated and will stop "
> +				  "working in a future version of Git. Use "
> +				  "--rebase-merges=no-rebase-cousins "
> +				  "instead."));
>  		else if (!strcmp("rebase-cousins", rebase_merges))
>  			options.rebase_cousins = 1;
>  		else if (strcmp("no-rebase-cousins", rebase_merges))

> The unusual syntax --rebase-merges="" (that is, --rebase-merges with an
> empty string argument) has been an undocumented synonym of
> --rebase-merges=no-rebase-cousins. Deprecate that syntax to avoid
> confusion when a rebase.merges config option is introduced, where
> rebase.merges="" will be equivalent to --no-rebase-merges.

> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
> ---
>   builtin/rebase.c         | 8 ++++++--
>   t/t3430-rebase-merges.sh | 5 +++++
>   2 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 6635f10d52..ccc13200b5 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1140,7 +1140,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   		{OPTION_STRING, 'r', "rebase-merges", &rebase_merges,
>   			N_("mode"),
>   			N_("try to rebase merges instead of skipping them"),
> -			PARSE_OPT_OPTARG, NULL, (intptr_t)""},
> +			PARSE_OPT_OPTARG, NULL, (intptr_t)"no-rebase-cousins"},
>   		OPT_BOOL(0, "fork-point", &options.fork_point,
>   			 N_("use 'merge-base --fork-point' to refine upstream")),
>   		OPT_STRING('s', "strategy", &options.strategy,
> @@ -1438,7 +1438,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   
>   	if (rebase_merges) {
>   		if (!*rebase_merges)
> -			; /* default mode; do nothing */
> +			warning(_("--rebase-merges with an empty string "
> +				  "argument is deprecated and will stop "
> +				  "working in a future version of Git. Use "
> +				  "--rebase-merges=no-rebase-cousins "
> +				  "instead."));
>   		else if (!strcmp("rebase-cousins", rebase_merges))
>   			options.rebase_cousins = 1;
>   		else if (strcmp("no-rebase-cousins", rebase_merges))
> diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
> index d46d9545f2..f50fbf1390 100755
> --- a/t/t3430-rebase-merges.sh
> +++ b/t/t3430-rebase-merges.sh
> @@ -278,6 +278,11 @@ test_expect_success 'do not rebase cousins unless asked for' '
>   	EOF
>   '
>   
> +test_expect_success '--rebase-merges="" is deprecated' '
> +	git rebase --rebase-merges="" HEAD^ 2>actual &&
> +	grep deprecated actual
> +'
> +
>   test_expect_success 'refs/rewritten/* is worktree-local' '
>   	git worktree add wt &&
>   	cat >wt/script-from-scratch <<-\EOF &&

> -			; /* default mode; do nothing */
> +			warning(_("--rebase-merges with an empty string "
> +				  "argument is deprecated and will stop "
> +				  "working in a future version of Git. Use "
> +				  "--rebase-merges=no-rebase-cousins "
> +				  "instead."));

> +	Whether and how to set the `--rebase-merges` option by default. Can
> +	be `rebase-cousins`, `no-rebase-cousins`, or a boolean. Setting to
> +	true is equivalent to `--rebase-merges` without an argument, setting to
> +	`rebase-cousins` or `no-rebase-cousins` is equivalent to
> +	`--rebase-merges` with that value as its argument, and setting to false
> +	is equivalent to `--no-rebase-merges`. Passing `--rebase-merges` on the
> +	command line without an argument overrides a `rebase.merges=false`
> +	configuration but does not override other values of `rebase.merge`.

> The purpose of the new option is to accommodate users who would like
> --rebase-merges to be on by default and to facilitate possibly turning
> on --rebase-merges by default without configuration in a future version
> of Git.
> 
> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
> ---
>   Documentation/config/rebase.txt        | 10 ++++
>   Documentation/git-rebase.txt           |  3 +-
>   builtin/rebase.c                       | 79 ++++++++++++++++++--------
>   t/t3422-rebase-incompatible-options.sh | 12 ++++
>   t/t3430-rebase-merges.sh               | 63 ++++++++++++++++++++
>   5 files changed, 143 insertions(+), 24 deletions(-)
> 
> diff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt
> index f19bd0e040..308baa9dbb 100644
> --- a/Documentation/config/rebase.txt
> +++ b/Documentation/config/rebase.txt
> @@ -67,3 +67,13 @@ rebase.rescheduleFailedExec::
>   
>   rebase.forkPoint::
>   	If set to false set `--no-fork-point` option by default.
> +
> +rebase.merges::
> +	Whether and how to set the `--rebase-merges` option by default. Can
> +	be `rebase-cousins`, `no-rebase-cousins`, or a boolean. Setting to
> +	true is equivalent to `--rebase-merges` without an argument, setting to
> +	`rebase-cousins` or `no-rebase-cousins` is equivalent to
> +	`--rebase-merges` with that value as its argument, and setting to false
> +	is equivalent to `--no-rebase-merges`. Passing `--rebase-merges` on the
> +	command line without an argument overrides a `rebase.merges=false`
> +	configuration but does not override other values of `rebase.merge`.

> @@ -1513,13 +1539,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   				break;
>   
>   		if (i >= 0 || options.type == REBASE_APPLY) {
> -			if (is_merge(&options))
> -				die(_("apply options and merge options "
> -					  "cannot be used together"));
> -			else if (options.autosquash == -1 && options.config_autosquash == 1)
> +			if (options.autosquash == -1 && options.config_autosquash == 1)
>   				die(_("apply options are incompatible with rebase.autosquash.  Consider adding --no-autosquash"));
> +			else if (options.rebase_merges == -1 && options.config_rebase_merges == 1)
> +				die(_("apply options are incompatible with rebase.merges.  Consider adding --no-rebase-merges"));

> diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
> index f50fbf1390..a825a77fb4 100755
> --- a/t/t3430-rebase-merges.sh
> +++ b/t/t3430-rebase-merges.sh
> @@ -283,6 +283,69 @@ test_expect_success '--rebase-merges="" is deprecated' '
>   	grep deprecated actual
>   '
>   
> +test_expect_success 'rebase.merges=rebase-cousins is equivalent to --rebase-merges=rebase-cousins' '
> +	test_config rebase.merges rebase-cousins &&
> +	git checkout -b config-rebase-cousins main &&
> +	git rebase HEAD^ &&
> +	test_cmp_graph HEAD^.. <<-\EOF
> +	*   Merge the topic branch '\''onebranch'\''
> +	|\
> +	| * D
> +	| * G
> +	|/
> +	o H
> +	EOF
> +'
> +
> +test_expect_success '--no-rebase-merges overrides rebase.merges=no-rebase-cousins' '
> +	test_config rebase.merges no-rebase-cousins &&
> +	git checkout -b override-config-no-rebase-cousins E &&
> +	git rebase --no-rebase-merges C &&
> +	test_cmp_graph C.. <<-\EOF
> +	* B
> +	* D
> +	o C
> +	EOF
> +'
> +
> +test_expect_success '--rebase-merges=no-rebase-cousins overrides rebase.merges=rebase-cousins' '
> +	test_config rebase.merges rebase-cousins &&
> +	git checkout -b override-config-rebase-cousins main &&
> +	git rebase --rebase-merges=no-rebase-cousins HEAD^ &&
> +	test_cmp_graph HEAD^.. <<-\EOF
> +	*   Merge the topic branch '\''onebranch'\''
> +	|\
> +	| * D
> +	| * G
> +	o | H
> +	|/
> +	o A
> +	EOF
> +'

> +test_expect_success '--rebase-merges overrides rebase.merges=false' '
> +	test_config rebase.merges false &&
> +	git checkout -b override-config-merges-false E &&
> +	before="$(git rev-parse --verify HEAD)" &&
> +	test_tick &&
> +	git rebase --rebase-merges C &&
> +	test_cmp_rev HEAD $before

> +'
> +
> +test_expect_success '--rebase-merges does not override rebase.merges=rebase-cousins' '
> +	test_config rebase.merges rebase-cousins &&
> +	git checkout -b no-override-config-rebase-cousins main &&
> +	git rebase --rebase-merges HEAD^ &&
> +	test_cmp_graph HEAD^.. <<-\EOF
> +	*   Merge the topic branch '\''onebranch'\''
> +	|\
> +	| * D
> +	| * G
> +	|/
> +	o H
> +	EOF
> +'
> +
>   test_expect_success 'refs/rewritten/* is worktree-local' '
>   	git worktree add wt &&
>   	cat >wt/script-from-scratch <<-\EOF &&

> > +rebase.merges::
> > +     Whether and how to set the `--rebase-merges` option by default. Can
> > +     be `rebase-cousins`, `no-rebase-cousins`, or a boolean. Setting to
> > +     true is equivalent to `--rebase-merges` without an argument, setting to
> > +     `rebase-cousins` or `no-rebase-cousins` is equivalent to
> > +     `--rebase-merges` with that value as its argument, and setting to false
> > +     is equivalent to `--no-rebase-merges`. Passing `--rebase-merges` on the
> > +     command line without an argument overrides a `rebase.merges=false`
> > +     configuration but does not override other values of `rebase.merge`.
>
> I'm still not clear why the commandline doesn't override the config in
> all cases as is our usual practice. After all if the user has set
> rebase.merges then they don't need to pass --rebase-merges unless they
> want to override the config.

> > +test_expect_success '--rebase-merges=no-rebase-cousins overrides rebase.merges=rebase-cousins' '
> > +     test_config rebase.merges rebase-cousins &&
> > +     git checkout -b override-config-rebase-cousins main &&
> > +     git rebase --rebase-merges=no-rebase-cousins HEAD^ &&
> > +     test_cmp_graph HEAD^.. <<-\EOF
> > +     *   Merge the topic branch '\''onebranch'\''
> > +     |\
> > +     | * D
> > +     | * G
> > +     o | H
> > +     |/
> > +     o A
> > +     EOF
> > +'
>
> I'm not sure this test adds much value, it is hard to see what kind of
> regression would allow the others to pass but not this one.

> > +test_expect_success '--rebase-merges overrides rebase.merges=false' '
> > +     test_config rebase.merges false &&
> > +     git checkout -b override-config-merges-false E &&
> > +     before="$(git rev-parse --verify HEAD)" &&
> > +     test_tick &&
> > +     git rebase --rebase-merges C &&
> > +     test_cmp_rev HEAD $before
>
> This test passes if the rebase does nothing, maybe pass --force and
> check the graph?

> On Thu, Mar 2, 2023 at 2:37 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> 
>> On 25/02/2023 18:03, Alex Henrie wrote:
> 
>>> +rebase.merges::
>>> +     Whether and how to set the `--rebase-merges` option by default. Can
>>> +     be `rebase-cousins`, `no-rebase-cousins`, or a boolean. Setting to
>>> +     true is equivalent to `--rebase-merges` without an argument, setting to
>>> +     `rebase-cousins` or `no-rebase-cousins` is equivalent to
>>> +     `--rebase-merges` with that value as its argument, and setting to false
>>> +     is equivalent to `--no-rebase-merges`. Passing `--rebase-merges` on the
>>> +     command line without an argument overrides a `rebase.merges=false`
>>> +     configuration but does not override other values of `rebase.merge`.
>>
>> I'm still not clear why the commandline doesn't override the config in
>> all cases as is our usual practice. After all if the user has set
>> rebase.merges then they don't need to pass --rebase-merges unless they
>> want to override the config.
> 
> Given the current push to turn rebase-merges on by default, it seems
> likely that rebase-cousins will also be turned on by default at some
> point after that.

> There will be a warning about the default changing,
> and we'll want to let users suppress that warning by setting
> rebase.rebaseMerges=rebase-cousins. It would then be very confusing if
> a --rebase-merges from some old alias continued to mean
> --rebase-merges=no-rebase-cousins when the user expects it to start
> behaving as though the default has already changed.

>>> +test_expect_success '--rebase-merges=no-rebase-cousins overrides rebase.merges=rebase-cousins' '
>>> +     test_config rebase.merges rebase-cousins &&
>>> +     git checkout -b override-config-rebase-cousins main &&
>>> +     git rebase --rebase-merges=no-rebase-cousins HEAD^ &&
>>> +     test_cmp_graph HEAD^.. <<-\EOF
>>> +     *   Merge the topic branch '\''onebranch'\''
>>> +     |\
>>> +     | * D
>>> +     | * G
>>> +     o | H
>>> +     |/
>>> +     o A
>>> +     EOF
>>> +'
>>
>> I'm not sure this test adds much value, it is hard to see what kind of
>> regression would allow the others to pass but not this one.
> 
> I was worried that I or someone else would forget to explicitly set
> rebase_cousins to 0 when no-rebase-cousins is given on the command
> line, assuming that it is already 0 because that is the default. The
> test makes me feel better, but I am happy to remove it if you still
> think it's overkill.

>>> +test_expect_success '--rebase-merges overrides rebase.merges=false' '
>>> +     test_config rebase.merges false &&
>>> +     git checkout -b override-config-merges-false E &&
>>> +     before="$(git rev-parse --verify HEAD)" &&
>>> +     test_tick &&
>>> +     git rebase --rebase-merges C &&
>>> +     test_cmp_rev HEAD $before
>>
>> This test passes if the rebase does nothing, maybe pass --force and
>> check the graph?
> 
> The rebase is supposed to do nothing here.

> On 04/03/2023 23:24, Alex Henrie wrote:
> > On Thu, Mar 2, 2023 at 2:37 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> >
> >> On 25/02/2023 18:03, Alex Henrie wrote:
> >
> >>> +rebase.merges::
> >>> +     Whether and how to set the `--rebase-merges` option by default. Can
> >>> +     be `rebase-cousins`, `no-rebase-cousins`, or a boolean. Setting to
> >>> +     true is equivalent to `--rebase-merges` without an argument, setting to
> >>> +     `rebase-cousins` or `no-rebase-cousins` is equivalent to
> >>> +     `--rebase-merges` with that value as its argument, and setting to false
> >>> +     is equivalent to `--no-rebase-merges`. Passing `--rebase-merges` on the
> >>> +     command line without an argument overrides a `rebase.merges=false`
> >>> +     configuration but does not override other values of `rebase.merge`.
> >>
> >> I'm still not clear why the commandline doesn't override the config in
> >> all cases as is our usual practice. After all if the user has set
> >> rebase.merges then they don't need to pass --rebase-merges unless they
> >> want to override the config.
> >
> > Given the current push to turn rebase-merges on by default, it seems
> > likely that rebase-cousins will also be turned on by default at some
> > point after that.
>
> It is good to try and future proof things but this seems rather
> hypothetical. I don't really see how the choice of whether
> --rebase-merges is turned on by default is related to the choice of
> whether or not to rebase cousins by default. It is worth noting that the
> default was changed to from rebase-cousins to no-rebase-cousins early in
> the development of --rebase-merges[1] as it was felt to be less surprising.

> > There will be a warning about the default changing,
> > and we'll want to let users suppress that warning by setting
> > rebase.rebaseMerges=rebase-cousins. It would then be very confusing if
> > a --rebase-merges from some old alias continued to mean
> > --rebase-merges=no-rebase-cousins when the user expects it to start
> > behaving as though the default has already changed.
>
> But aren't you breaking those aliases now when
> rebase.rebaseMerges=rebase-cousins? That's what I'm objecting to. It
> seems like we're breaking things now to avoid a hypothetical future
> change breaking them which does not seem like the right trade off to me.
>
> It also does not fit with the way other optional arguments interact with
> their associated config setting. For example "git branch/checkout/switch
> --track" and branch.autoSetupMerge. If the optional argument to --track
> is omitted it defaults to "direct" irrespective of the config.

> >>> +test_expect_success '--rebase-merges overrides rebase.merges=false' '
> >>> +     test_config rebase.merges false &&
> >>> +     git checkout -b override-config-merges-false E &&
> >>> +     before="$(git rev-parse --verify HEAD)" &&
> >>> +     test_tick &&
> >>> +     git rebase --rebase-merges C &&
> >>> +     test_cmp_rev HEAD $before
> >>
> >> This test passes if the rebase does nothing, maybe pass --force and
> >> check the graph?
> >
> > The rebase is supposed to do nothing here.
>
> It's not doing nothing though - it is rebasing the branch, it just
> happens that everything fast-forwards so HEAD ends up unchanged. My
> point is that this test should verify the branch has been rebased. Maybe
> you could check the reflog message for HEAD@{0} is
>
>         rebase (finish): returning to refs/heads/override-config-merges-false
>
> > Checking that the commit
> > hash is the same is just a quick way to check that the entire graph is
> > the same. What more would be checked by checking the graph instead of
> > the hash?
>
> By using --force and checking the graph you check that the rebase
> actually happened.

> On Tue, Mar 7, 2023 at 9:23 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> 
>> On 04/03/2023 23:24, Alex Henrie wrote:
>>> On Thu, Mar 2, 2023 at 2:37 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>>
>>>> On 25/02/2023 18:03, Alex Henrie wrote:
>>>
>>>>> +rebase.merges::
>>>>> +     Whether and how to set the `--rebase-merges` option by default. Can
>>>>> +     be `rebase-cousins`, `no-rebase-cousins`, or a boolean. Setting to
>>>>> +     true is equivalent to `--rebase-merges` without an argument, setting to
>>>>> +     `rebase-cousins` or `no-rebase-cousins` is equivalent to
>>>>> +     `--rebase-merges` with that value as its argument, and setting to false
>>>>> +     is equivalent to `--no-rebase-merges`. Passing `--rebase-merges` on the
>>>>> +     command line without an argument overrides a `rebase.merges=false`
>>>>> +     configuration but does not override other values of `rebase.merge`.
>>>>
>>>> I'm still not clear why the commandline doesn't override the config in
>>>> all cases as is our usual practice. After all if the user has set
>>>> rebase.merges then they don't need to pass --rebase-merges unless they
>>>> want to override the config.
>>>
>>> Given the current push to turn rebase-merges on by default, it seems
>>> likely that rebase-cousins will also be turned on by default at some
>>> point after that.
>>
>> It is good to try and future proof things but this seems rather
>> hypothetical. I don't really see how the choice of whether
>> --rebase-merges is turned on by default is related to the choice of
>> whether or not to rebase cousins by default. It is worth noting that the
>> default was changed to from rebase-cousins to no-rebase-cousins early in
>> the development of --rebase-merges[1] as it was felt to be less surprising.
> 
>> [1]
>> https://lore.kernel.org/git/nycvar.QRO.7.76.6.1801292251240.35@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz/
> 
> Thank you for sharing that link. Even though I got the tests right, I
> got confused and started thinking that rebase-cousins was a more
> thorough version of rebase-merges. In fact, they do opposite things:
> rebase-merges tries to preserve the graph and rebase-cousins
> intentionally restructures the graph. In my opinion using the word
> "rebase" in the names of both options was another unfortunate UI
> decision, but I understand the difference now.
> 
>>> There will be a warning about the default changing,
>>> and we'll want to let users suppress that warning by setting
>>> rebase.rebaseMerges=rebase-cousins. It would then be very confusing if
>>> a --rebase-merges from some old alias continued to mean
>>> --rebase-merges=no-rebase-cousins when the user expects it to start
>>> behaving as though the default has already changed.
>>
>> But aren't you breaking those aliases now when
>> rebase.rebaseMerges=rebase-cousins? That's what I'm objecting to. It
>> seems like we're breaking things now to avoid a hypothetical future
>> change breaking them which does not seem like the right trade off to me.
>>
>> It also does not fit with the way other optional arguments interact with
>> their associated config setting. For example "git branch/checkout/switch
>> --track" and branch.autoSetupMerge. If the optional argument to --track
>> is omitted it defaults to "direct" irrespective of the config.

> You're
> right that it's unlikely that the default will ever change from
> no-rebase-cousins to rebase-cousins; I was mistaken. However, Glen
> thinks that in the future we might have some kind of
> rebase-evil-merges mode as well, and that that might become the
> default. If we don't let the rebase.rebaseMerges config value control
> the default behavior of --rebase-merges without an argument on the
> command line, we would have to introduce a separate config option for
> the transition, which would be ugly.

>>>>> +test_expect_success '--rebase-merges overrides rebase.merges=false' '
>>>>> +     test_config rebase.merges false &&
>>>>> +     git checkout -b override-config-merges-false E &&
>>>>> +     before="$(git rev-parse --verify HEAD)" &&
>>>>> +     test_tick &&
>>>>> +     git rebase --rebase-merges C &&
>>>>> +     test_cmp_rev HEAD $before
>>>>
>>>> This test passes if the rebase does nothing, maybe pass --force and
>>>> check the graph?
>>>
>>> The rebase is supposed to do nothing here.
>>
>> It's not doing nothing though - it is rebasing the branch, it just
>> happens that everything fast-forwards so HEAD ends up unchanged. My
>> point is that this test should verify the branch has been rebased. Maybe
>> you could check the reflog message for HEAD@{0} is
>>
>>          rebase (finish): returning to refs/heads/override-config-merges-false
>>
>>> Checking that the commit
>>> hash is the same is just a quick way to check that the entire graph is
>>> the same. What more would be checked by checking the graph instead of
>>> the hash?
>>
>> By using --force and checking the graph you check that the rebase
>> actually happened.
> 
> I got the impression that people like that not checking the graph
> itself (or the reflog) makes the tests more concise, but I don't care
> much either way. For what it's worth, the way I did it matches the
> existing tests in the file. If you can find at least one other person
> who thinks that it ought to change for this patch series to be
> accepted, and no one else objects, I'll change it.
> 
>> Thanks for working on this
> 
> You're welcome; hopefully we can get the remaining details ironed out quickly.
> 
> -Alex

> > More voices would be helpful here. Does anyone else have an opinion on
> > how likely it is that the default rebase-merges mode will change in
> > the future? Or on whether rebase.rebaseMerges should be allowed to
> > affect --rebase-merges in order to facilitate such a change?
>
> Getting other's views would indeed be helpful

> What I really don't want is to paint ourselves into a corner. You're
> right that it's unlikely that the default will ever change from
> no-rebase-cousins to rebase-cousins; I was mistaken. However, Glen
> thinks that in the future we might have some kind of
> rebase-evil-merges mode as well, and that that might become the
> default. If we don't let the rebase.rebaseMerges config value control
> the default behavior of --rebase-merges without an argument on the
> command line, we would have to introduce a separate config option for
> the transition, which would be ugly.
>
> More voices would be helpful here. Does anyone else have an opinion on
> how likely it is that the default rebase-merges mode will change in
> the future? Or on whether rebase.rebaseMerges should be allowed to
> affect --rebase-merges in order to facilitate such a change?

> +
> +rebase.merges::
> +	Whether and how to set the `--rebase-merges` option by default. Can
> +	be `rebase-cousins`, `no-rebase-cousins`, or a boolean. Setting to
> +	true is equivalent to `--rebase-merges` without an argument, setting to
> +	`rebase-cousins` or `no-rebase-cousins` is equivalent to
> +	`--rebase-merges` with that value as its argument, and setting to false
> +	is equivalent to `--no-rebase-merges`. Passing `--rebase-merges` on the
> +	command line without an argument overrides a `rebase.merges=false`
> +	configuration but does not override other values of `rebase.merge`.

> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index c98784a0d2..b02f9cbb8c 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -537,7 +537,8 @@ See also INCOMPATIBLE OPTIONS below.
>  	by recreating the merge commits. Any resolved merge conflicts or
>  	manual amendments in these merge commits will have to be
>  	resolved/re-applied manually. `--no-rebase-merges` can be used to
> -	countermand a previous `--rebase-merges`.
> +	countermand both the `rebase.merges` config option and a previous
> +	`--rebase-merges`.
>  +
>  By default, or when `no-rebase-cousins` was specified, commits which do not
>  have `<upstream>` as direct ancestor will keep their original branch point,
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index ccc13200b5..ac096f27e1 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -123,6 +123,7 @@ struct rebase_options {
>  	int fork_point;
>  	int update_refs;
>  	int config_autosquash;
> +	int config_rebase_merges;
>  	int config_update_refs;
>  };
>  
> @@ -140,6 +141,8 @@ struct rebase_options {
>  		.allow_empty_message = 1,               \
>  		.autosquash = -1,                       \
>  		.config_autosquash = -1,                \
> +		.rebase_merges = -1,                    \
> +		.config_rebase_merges = -1,             \
>  		.update_refs = -1,                      \
>  		.config_update_refs = -1,               \
>  	}
> @@ -771,6 +774,16 @@ static int run_specific_rebase(struct rebase_options *opts)
>  	return status ? -1 : 0;
>  }
>  
> +static void parse_rebase_merges_value(struct rebase_options *options, const char *value)
> +{
> +	if (!strcmp("no-rebase-cousins", value))
> +		options->rebase_cousins = 0;
> +	else if (!strcmp("rebase-cousins", value))
> +		options->rebase_cousins = 1;
> +	else
> +		die(_("Unknown rebase-merges mode: %s"), value);
> +}
> +
>  static int rebase_config(const char *var, const char *value, void *data)
>  {
>  	struct rebase_options *opts = data;
> @@ -800,6 +813,15 @@ static int rebase_config(const char *var, const char *value, void *data)
>  		return 0;
>  	}
>  
> +	if (!strcmp(var, "rebase.merges")) {
> +		opts->config_rebase_merges = git_parse_maybe_bool(value);
> +		if (opts->config_rebase_merges < 0) {
> +			opts->config_rebase_merges = 1;
> +			parse_rebase_merges_value(opts, value);
> +		}
> +		return 0;
> +	}
> +
>  	if (!strcmp(var, "rebase.updaterefs")) {
>  		opts->config_update_refs = git_config_bool(var, value);
>  		return 0;
> @@ -980,6 +1002,27 @@ static int parse_opt_empty(const struct option *opt, const char *arg, int unset)
>  	return 0;
>  }
>  
> +static int parse_opt_rebase_merges(const struct option *opt, const char *arg, int unset)
> +{
> +	struct rebase_options *options = opt->value;
> +
> +	options->rebase_merges = !unset;
> +
> +	if (arg) {
> +		if (!*arg) {
> +			warning(_("--rebase-merges with an empty string "
> +				  "argument is deprecated and will stop "
> +				  "working in a future version of Git. Use "
> +				  "--rebase-merges=no-rebase-cousins "
> +				  "instead."));
> +			arg = "no-rebase-cousins";
> +		}
> +		parse_rebase_merges_value(options, arg);
> +	}
> +
> +	return 0;
> +}
> +
>  static void NORETURN error_on_missing_default_upstream(void)
>  {
>  	struct branch *current_branch = branch_get(NULL);
> @@ -1035,7 +1078,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  	struct object_id branch_base;
>  	int ignore_whitespace = 0;
>  	const char *gpg_sign = NULL;
> -	const char *rebase_merges = NULL;
>  	struct string_list strategy_options = STRING_LIST_INIT_NODUP;
>  	struct object_id squash_onto;
>  	char *squash_onto_name = NULL;
> @@ -1137,10 +1179,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  			   &options.allow_empty_message,
>  			   N_("allow rebasing commits with empty messages"),
>  			   PARSE_OPT_HIDDEN),
> -		{OPTION_STRING, 'r', "rebase-merges", &rebase_merges,
> -			N_("mode"),
> +		OPT_CALLBACK_F('r', "rebase-merges", &options, N_("mode"),
>  			N_("try to rebase merges instead of skipping them"),
> -			PARSE_OPT_OPTARG, NULL, (intptr_t)"no-rebase-cousins"},
> +			PARSE_OPT_OPTARG, parse_opt_rebase_merges),
>  		OPT_BOOL(0, "fork-point", &options.fork_point,
>  			 N_("use 'merge-base --fork-point' to refine upstream")),
>  		OPT_STRING('s', "strategy", &options.strategy,
> @@ -1436,21 +1477,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  	if (options.exec.nr)
>  		imply_merge(&options, "--exec");
>  
> -	if (rebase_merges) {
> -		if (!*rebase_merges)
> -			warning(_("--rebase-merges with an empty string "
> -				  "argument is deprecated and will stop "
> -				  "working in a future version of Git. Use "
> -				  "--rebase-merges=no-rebase-cousins "
> -				  "instead."));
> -		else if (!strcmp("rebase-cousins", rebase_merges))
> -			options.rebase_cousins = 1;
> -		else if (strcmp("no-rebase-cousins", rebase_merges))
> -			die(_("Unknown mode: %s"), rebase_merges);
> -		options.rebase_merges = 1;
> -		imply_merge(&options, "--rebase-merges");
> -	}
> -
>  	if (options.type == REBASE_APPLY) {
>  		if (ignore_whitespace)
>  			strvec_push(&options.git_am_opts,
> @@ -1513,13 +1539,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  				break;
>  
>  		if (i >= 0 || options.type == REBASE_APPLY) {
> -			if (is_merge(&options))
> -				die(_("apply options and merge options "
> -					  "cannot be used together"));
> -			else if (options.autosquash == -1 && options.config_autosquash == 1)
> +			if (options.autosquash == -1 && options.config_autosquash == 1)
>  				die(_("apply options are incompatible with rebase.autosquash.  Consider adding --no-autosquash"));
> +			else if (options.rebase_merges == -1 && options.config_rebase_merges == 1)
> +				die(_("apply options are incompatible with rebase.merges.  Consider adding --no-rebase-merges"));
>  			else if (options.update_refs == -1 && options.config_update_refs == 1)
>  				die(_("apply options are incompatible with rebase.updateRefs.  Consider adding --no-update-refs"));
> +			else if (is_merge(&options))
> +				die(_("apply options and merge options "
> +					  "cannot be used together"));
>  			else
>  				options.type = REBASE_APPLY;
>  		}
> @@ -1530,6 +1558,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  	options.update_refs = (options.update_refs >= 0) ? options.update_refs :
>  			     ((options.config_update_refs >= 0) ? options.config_update_refs : 0);
>  
> +	if (options.rebase_merges == 1)
> +		imply_merge(&options, "--rebase-merges");
> +	options.rebase_merges = (options.rebase_merges >= 0) ? options.rebase_merges :
> +				((options.config_rebase_merges >= 0) ? options.config_rebase_merges : 0);
> +
>  	if (options.autosquash == 1)
>  		imply_merge(&options, "--autosquash");
>  	options.autosquash = (options.autosquash >= 0) ? options.autosquash :
> diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
> index 4711b37a28..af93f13849 100755
> --- a/t/t3422-rebase-incompatible-options.sh
> +++ b/t/t3422-rebase-incompatible-options.sh
> @@ -101,6 +101,12 @@ test_rebase_am_only () {
>  		grep -e --no-autosquash err
>  	"
>  
> +	test_expect_success "$opt incompatible with rebase.merges" "
> +		git checkout B^0 &&
> +		test_must_fail git -c rebase.merges=true rebase $opt A 2>err &&
> +		grep -e --no-rebase-merges err
> +	"
> +

>  	test_expect_success "$opt incompatible with rebase.updateRefs" "
>  		git checkout B^0 &&
>  		test_must_fail git -c rebase.updateRefs=true rebase $opt A 2>err &&
> @@ -113,6 +119,12 @@ test_rebase_am_only () {
>  		git -c rebase.autosquash=true rebase --no-autosquash $opt A
>  	"
>  
> +	test_expect_success "$opt okay with overridden rebase.merges" "
> +		test_when_finished \"git reset --hard B^0\" &&
> +		git checkout B^0 &&
> +		git -c rebase.merges=true rebase --no-rebase-merges $opt A
> +	"
> +
>  	test_expect_success "$opt okay with overridden rebase.updateRefs" "
>  		test_when_finished \"git reset --hard B^0\" &&
>  		git checkout B^0 &&
> diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
> index f50fbf1390..a825a77fb4 100755
> --- a/t/t3430-rebase-merges.sh
> +++ b/t/t3430-rebase-merges.sh
> @@ -283,6 +283,69 @@ test_expect_success '--rebase-merges="" is deprecated' '
>  	grep deprecated actual
>  '
>  
> +test_expect_success 'rebase.merges=rebase-cousins is equivalent to --rebase-merges=rebase-cousins' '
> +	test_config rebase.merges rebase-cousins &&
> +	git checkout -b config-rebase-cousins main &&
> +	git rebase HEAD^ &&
> +	test_cmp_graph HEAD^.. <<-\EOF
> +	*   Merge the topic branch '\''onebranch'\''
> +	|\
> +	| * D
> +	| * G
> +	|/
> +	o H
> +	EOF
> +'
> +
> +test_expect_success '--no-rebase-merges overrides rebase.merges=no-rebase-cousins' '
> +	test_config rebase.merges no-rebase-cousins &&
> +	git checkout -b override-config-no-rebase-cousins E &&
> +	git rebase --no-rebase-merges C &&
> +	test_cmp_graph C.. <<-\EOF
> +	* B
> +	* D
> +	o C
> +	EOF
> +'
> +
> +test_expect_success '--rebase-merges=no-rebase-cousins overrides rebase.merges=rebase-cousins' '
> +	test_config rebase.merges rebase-cousins &&
> +	git checkout -b override-config-rebase-cousins main &&
> +	git rebase --rebase-merges=no-rebase-cousins HEAD^ &&
> +	test_cmp_graph HEAD^.. <<-\EOF
> +	*   Merge the topic branch '\''onebranch'\''
> +	|\
> +	| * D
> +	| * G
> +	o | H
> +	|/
> +	o A
> +	EOF
> +'
> +
> +test_expect_success '--rebase-merges overrides rebase.merges=false' '
> +	test_config rebase.merges false &&
> +	git checkout -b override-config-merges-false E &&
> +	before="$(git rev-parse --verify HEAD)" &&
> +	test_tick &&
> +	git rebase --rebase-merges C &&
> +	test_cmp_rev HEAD $before
> +'
> +
> +test_expect_success '--rebase-merges does not override rebase.merges=rebase-cousins' '
> +	test_config rebase.merges rebase-cousins &&
> +	git checkout -b no-override-config-rebase-cousins main &&
> +	git rebase --rebase-merges HEAD^ &&
> +	test_cmp_graph HEAD^.. <<-\EOF
> +	*   Merge the topic branch '\''onebranch'\''
> +	|\
> +	| * D
> +	| * G
> +	|/
> +	o H
> +	EOF
> +'
> +
>  test_expect_success 'refs/rewritten/* is worktree-local' '
>  	git worktree add wt &&
>  	cat >wt/script-from-scratch <<-\EOF &&
> -- 
> 2.39.2
> 
> 
> 

> > +rebase.merges::
> > +     Whether and how to set the `--rebase-merges` option by default. Can
> > +     be `rebase-cousins`, `no-rebase-cousins`, or a boolean. Setting to
> > +     true is equivalent to `--rebase-merges` without an argument, setting to
> > +     `rebase-cousins` or `no-rebase-cousins` is equivalent to
> > +     `--rebase-merges` with that value as its argument, and setting to false
> > +     is equivalent to `--no-rebase-merges`. Passing `--rebase-merges` on the
> > +     command line without an argument overrides a `rebase.merges=false`
> > +     configuration but does not override other values of `rebase.merge`.
>
> I originally believed that having the boolean values as an option here
> created unnecessary complexity but I was convinced during Review Club
> that the tradeoff for a user who just wants to turn this on is fine. I'm
> just wondering whether this option will possibly have additional values
> in the future. If so, what would a user expect from setting this config
> to true?

> Changes from v4:
>
> [...]
>
> Suggestions not incorporated:
>
> [...]
>

>
> Hi Alex! It was great to see you at Review Club today. If you'd like the
> notes, they are available at
>
> https://docs.google.com/document/d/14L8BAumGTpsXpjDY8VzZ4rRtpAjuGrFSRqn3stCuS_w
>
> though reviewers will send their feedback to the mailing list anyway, so
> don't feel compelled to address feedback that isn't on the mailing list
> :)

> The purpose of the new option is to accommodate users who would like
> --rebase-merges to be on by default and to facilitate turning on
> --rebase-merges by default without configuration in a future version of
> Git.
> 
> Name the new option rebase.rebaseMerges, even though it is a little
> redundant, for consistency with the name of the command line option and
> to be clear when scrolling through values in the [rebase] section of
> .gitconfig.
> 
> In the future, the default rebase-merges mode may change from
> no-rebase-cousins to rebase-cousins. Support setting rebase.rebaseMerges
> to the nonspecific value "true" for users who do not need or want to
> care about the default changing in the future. Similarly, for users who
> have --rebase-merges in an alias and want to get the future behavior
> now, use the specific rebase-merges mode from the config if a specific
> mode is not given on the command line.
> 
> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
> ---
>   Documentation/config/rebase.txt        | 11 ++++
>   Documentation/git-rebase.txt           | 17 +++---
>   builtin/rebase.c                       | 79 ++++++++++++++++++--------
>   t/t3422-rebase-incompatible-options.sh | 17 ++++++
>   t/t3430-rebase-merges.sh               | 68 ++++++++++++++++++++++
>   5 files changed, 161 insertions(+), 31 deletions(-)
> 
> diff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt
> index f19bd0e040..f7d3218b1d 100644
> --- a/Documentation/config/rebase.txt
> +++ b/Documentation/config/rebase.txt
> @@ -67,3 +67,14 @@ rebase.rescheduleFailedExec::
>   
>   rebase.forkPoint::
>   	If set to false set `--no-fork-point` option by default.
> +
> +rebase.rebaseMerges::
> +	Whether and how to set the `--rebase-merges` option by default. Can
> +	be `rebase-cousins`, `no-rebase-cousins`, or a boolean. Setting to
> +	true is equivalent to `--rebase-merges` without an argument,

> setting to
> +	`rebase-cousins` or `no-rebase-cousins` is equivalent to
> +	`--rebase-merges` with that value as its argument, and setting to false
> +	is equivalent to `--no-rebase-merges`. Passing `--rebase-merges` on the
> +	command line without an argument overrides a `rebase.rebaseMerges=false`
> +	configuration, but the absence of a specific rebase-merges mode on the
> +	command line does not counteract a specific mode set in the configuration.

> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 4e57a87624..6ec86c9c6e 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -537,16 +537,17 @@ See also INCOMPATIBLE OPTIONS below.
>   	by recreating the merge commits. Any resolved merge conflicts or
>   	manual amendments in these merge commits will have to be
>   	resolved/re-applied manually. `--no-rebase-merges` can be used to
> -	countermand a previous `--rebase-merges`.
> +	countermand both the `rebase.rebaseMerges` config option and a previous
> +	`--rebase-merges`.
>   +
>   When rebasing merges, there are two modes: `rebase-cousins` and
> -`no-rebase-cousins`. If the mode is not specified, it defaults to
> -`no-rebase-cousins`. In `no-rebase-cousins` mode, commits which do not have
> -`<upstream>` as direct ancestor will keep their original branch point, i.e.
> -commits that would be excluded by linkgit:git-log[1]'s `--ancestry-path`
> -option will keep their original ancestry by default. In `rebase-cousins` mode,
> -such commits are instead rebased onto `<upstream>` (or `<onto>`, if
> -specified).
> +`no-rebase-cousins`. If the mode is not specified on the command line or in
> +the `rebase.rebaseMerges` config option, it defaults to `no-rebase-cousins`.
> +In `no-rebase-cousins` mode, commits which do not have `<upstream>` as direct
> +ancestor will keep their original branch point, i.e. commits that would be
> +excluded by linkgit:git-log[1]'s `--ancestry-path` option will keep their
> +original ancestry by default. In `rebase-cousins` mode, such commits are
> +instead rebased onto `<upstream>` (or `<onto>`, if specified).

>   static int rebase_config(const char *var, const char *value, void *data)
>   {
>   	struct rebase_options *opts = data;
> @@ -800,6 +813,15 @@ static int rebase_config(const char *var, const char *value, void *data)
>   		return 0;
>   	}
>   
> +	if (!strcmp(var, "rebase.rebasemerges")) {
> +		opts->config_rebase_merges = git_parse_maybe_bool(value);
> +		if (opts->config_rebase_merges < 0) {
> +			opts->config_rebase_merges = 1;
> +			parse_rebase_merges_value(opts, value);
> +		}

> +		return 0;
> +	}
> +
>   	if (!strcmp(var, "rebase.updaterefs")) {
>   		opts->config_update_refs = git_config_bool(var, value);
>   		return 0;
> @@ -980,6 +1002,27 @@ static int parse_opt_empty(const struct option *opt, const char *arg, int unset)
>   	return 0;
>   }
>   
> +static int parse_opt_rebase_merges(const struct option *opt, const char *arg, int unset)
> +{
> +	struct rebase_options *options = opt->value;
> +
> +	options->rebase_merges = !unset;
> +
> +	if (arg) {
> +		if (!*arg) {
> +			warning(_("--rebase-merges with an empty string "
> +				  "argument is deprecated and will stop "
> +				  "working in a future version of Git. Use "
> +				  "--rebase-merges without an argument "
> +				  "instead, which does the same thing."));
> +			return 0;
> +		}
> +		parse_rebase_merges_value(options, arg);
> +	}
> +
> +	return 0;
> +}
> +
>   static void NORETURN error_on_missing_default_upstream(void)
>   {
>   	struct branch *current_branch = branch_get(NULL);
> @@ -1035,7 +1078,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   	struct object_id branch_base;
>   	int ignore_whitespace = 0;
>   	const char *gpg_sign = NULL;
> -	const char *rebase_merges = NULL;
>   	struct string_list strategy_options = STRING_LIST_INIT_NODUP;
>   	struct object_id squash_onto;
>   	char *squash_onto_name = NULL;
> @@ -1137,10 +1179,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   			   &options.allow_empty_message,
>   			   N_("allow rebasing commits with empty messages"),
>   			   PARSE_OPT_HIDDEN),
> -		{OPTION_STRING, 'r', "rebase-merges", &rebase_merges,
> -			N_("mode"),
> +		OPT_CALLBACK_F('r', "rebase-merges", &options, N_("mode"),
>   			N_("try to rebase merges instead of skipping them"),
> -			PARSE_OPT_OPTARG, NULL, (intptr_t)"no-rebase-cousins"},
> +			PARSE_OPT_OPTARG, parse_opt_rebase_merges),
>   		OPT_BOOL(0, "fork-point", &options.fork_point,
>   			 N_("use 'merge-base --fork-point' to refine upstream")),
>   		OPT_STRING('s', "strategy", &options.strategy,
> @@ -1436,21 +1477,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   	if (options.exec.nr)
>   		imply_merge(&options, "--exec");
>   
> -	if (rebase_merges) {
> -		if (!*rebase_merges)
> -			warning(_("--rebase-merges with an empty string "
> -				  "argument is deprecated and will stop "
> -				  "working in a future version of Git. Use "
> -				  "--rebase-merges without an argument "
> -				  "instead, which does the same thing."));
> -		else if (!strcmp("rebase-cousins", rebase_merges))
> -			options.rebase_cousins = 1;
> -		else if (strcmp("no-rebase-cousins", rebase_merges))
> -			die(_("Unknown mode: %s"), rebase_merges);
> -		options.rebase_merges = 1;
> -		imply_merge(&options, "--rebase-merges");
> -	}
> -
>   	if (options.type == REBASE_APPLY) {
>   		if (ignore_whitespace)
>   			strvec_push(&options.git_am_opts,
> @@ -1513,13 +1539,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   				break;
>   
>   		if (i >= 0 || options.type == REBASE_APPLY) {
> -			if (is_merge(&options))
> -				die(_("apply options and merge options "
> -					  "cannot be used together"));
> -			else if (options.autosquash == -1 && options.config_autosquash == 1)
> +			if (options.autosquash == -1 && options.config_autosquash == 1)
>   				die(_("apply options are incompatible with rebase.autosquash.  Consider adding --no-autosquash"));
> +			else if (options.rebase_merges == -1 && options.config_rebase_merges == 1)
> +				die(_("apply options are incompatible with rebase.rebaseMerges.  Consider adding --no-rebase-merges"));
>   			else if (options.update_refs == -1 && options.config_update_refs == 1)
>   				die(_("apply options are incompatible with rebase.updateRefs.  Consider adding --no-update-refs"));
> +			else if (is_merge(&options))
> +				die(_("apply options and merge options "
> +					  "cannot be used together"));
>   			else
>   				options.type = REBASE_APPLY;
>   		}
> @@ -1530,6 +1558,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   	options.update_refs = (options.update_refs >= 0) ? options.update_refs :
>   			     ((options.config_update_refs >= 0) ? options.config_update_refs : 0);
>   
> +	if (options.rebase_merges == 1)
> +		imply_merge(&options, "--rebase-merges");
> +	options.rebase_merges = (options.rebase_merges >= 0) ? options.rebase_merges :
> +				((options.config_rebase_merges >= 0) ? options.config_rebase_merges : 0);
> +
>   	if (options.autosquash == 1)
>   		imply_merge(&options, "--autosquash");
>   	options.autosquash = (options.autosquash >= 0) ? options.autosquash :
> diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
> index 4711b37a28..2eba00bdf5 100755
> --- a/t/t3422-rebase-incompatible-options.sh
> +++ b/t/t3422-rebase-incompatible-options.sh
> @@ -85,6 +85,11 @@ test_rebase_am_only () {
>   		test_must_fail git rebase $opt --reapply-cherry-picks A
>   	"
>   
> +	test_expect_success "$opt incompatible with --rebase-merges" "
> +		git checkout B^0 &&
> +		test_must_fail git rebase $opt --rebase-merges A
> +	"
> +
>   	test_expect_success "$opt incompatible with --update-refs" "
>   		git checkout B^0 &&
>   		test_must_fail git rebase $opt --update-refs A
> @@ -101,6 +106,12 @@ test_rebase_am_only () {
>   		grep -e --no-autosquash err
>   	"
>   
> +	test_expect_success "$opt incompatible with rebase.rebaseMerges" "
> +		git checkout B^0 &&
> +		test_must_fail git -c rebase.rebaseMerges=true rebase $opt A 2>err &&
> +		grep -e --no-rebase-merges err
> +	"
> +
>   	test_expect_success "$opt incompatible with rebase.updateRefs" "
>   		git checkout B^0 &&
>   		test_must_fail git -c rebase.updateRefs=true rebase $opt A 2>err &&
> @@ -113,6 +124,12 @@ test_rebase_am_only () {
>   		git -c rebase.autosquash=true rebase --no-autosquash $opt A
>   	"
>   
> +	test_expect_success "$opt okay with overridden rebase.rebaseMerges" "
> +		test_when_finished \"git reset --hard B^0\" &&
> +		git checkout B^0 &&
> +		git -c rebase.rebaseMerges=true rebase --no-rebase-merges $opt A
> +	"
> +
>   	test_expect_success "$opt okay with overridden rebase.updateRefs" "
>   		test_when_finished \"git reset --hard B^0\" &&
>   		git checkout B^0 &&
> diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
> index d46d9545f2..aa75e192d1 100755
> --- a/t/t3430-rebase-merges.sh
> +++ b/t/t3430-rebase-merges.sh
> @@ -278,6 +278,74 @@ test_expect_success 'do not rebase cousins unless asked for' '
>   	EOF
>   '
>   
> +test_expect_success '--rebase-merges="" is deprecated' '
> +	git rebase --rebase-merges="" HEAD^ 2>actual &&
> +	grep deprecated actual
> +'
> +
> +test_expect_success 'rebase.rebaseMerges=rebase-cousins is equivalent to --rebase-merges=rebase-cousins' '
> +	test_config rebase.rebaseMerges rebase-cousins &&
> +	git checkout -b config-rebase-cousins main &&
> +	git rebase HEAD^ &&
> +	test_cmp_graph HEAD^.. <<-\EOF
> +	*   Merge the topic branch '\''onebranch'\''
> +	|\
> +	| * D
> +	| * G
> +	|/
> +	o H
> +	EOF
> +'
> +
> +test_expect_success '--no-rebase-merges overrides rebase.rebaseMerges=no-rebase-cousins' '
> +	test_config rebase.rebaseMerges no-rebase-cousins &&
> +	git checkout -b override-config-no-rebase-cousins E &&
> +	git rebase --no-rebase-merges C &&
> +	test_cmp_graph C.. <<-\EOF
> +	* B
> +	* D
> +	o C
> +	EOF
> +'
> +
> +test_expect_success '--rebase-merges=no-rebase-cousins overrides rebase.rebaseMerges=rebase-cousins' '
> +	test_config rebase.rebaseMerges rebase-cousins &&
> +	git checkout -b override-config-rebase-cousins main &&
> +	git rebase --rebase-merges=no-rebase-cousins HEAD^ &&
> +	test_cmp_graph HEAD^.. <<-\EOF
> +	*   Merge the topic branch '\''onebranch'\''
> +	|\
> +	| * D
> +	| * G
> +	o | H
> +	|/
> +	o A
> +	EOF
> +'
> +
> +test_expect_success '--rebase-merges overrides rebase.rebaseMerges=false' '
> +	test_config rebase.rebaseMerges false &&
> +	git checkout -b override-config-merges-false E &&
> +	before="$(git rev-parse --verify HEAD)" &&
> +	test_tick &&
> +	git rebase --rebase-merges C &&
> +	test_cmp_rev HEAD $before
> +'
> +
> +test_expect_success '--rebase-merges does not override rebase.rebaseMerges=rebase-cousins' '
> +	test_config rebase.rebaseMerges rebase-cousins &&
> +	git checkout -b no-override-config-rebase-cousins main &&
> +	git rebase --rebase-merges HEAD^ &&
> +	test_cmp_graph HEAD^.. <<-\EOF
> +	*   Merge the topic branch '\''onebranch'\''
> +	|\
> +	| * D
> +	| * G
> +	|/
> +	o H
> +	EOF
> +'
> +
>   test_expect_success 'refs/rewritten/* is worktree-local' '
>   	git worktree add wt &&
>   	cat >wt/script-from-scratch <<-\EOF &&

>> +rebase.rebaseMerges::
>> +	Whether and how to set the `--rebase-merges` option by default. Can
>> +	be `rebase-cousins`, `no-rebase-cousins`, or a boolean. Setting to
>> +	true is equivalent to `--rebase-merges` without an argument,
>
> This is a bit picky but how can rebase.rebaseMerges=true be equivalent
> to --rebase-merges without an argument when the behavior of
> --rebase-merges without an argument depends on the value of
> rebase.rebaseMerges?

>> setting to
>> +	`rebase-cousins` or `no-rebase-cousins` is equivalent to
>> +	`--rebase-merges` with that value as its argument, and setting to false
>> +	is equivalent to `--no-rebase-merges`. Passing `--rebase-merges` on the
>> +	command line without an argument overrides a `rebase.rebaseMerges=false`
>> +	configuration, but the absence of a specific rebase-merges mode on the
>> +	command line does not counteract a specific mode set in the configuration.
>
> I may not agree the with the design choice but the documentation here
> and below is very clear about the behavior of --rebase-merges without
> an argument which is good.

> I think we need
>
> 	} else {
> 		opts->rebase_cousins = 0;
> 	}
>
> here. Otherwise if rebase.rebaseMerges is set twice we wont follow the
> usual "last config wins" convention. For example
>
> 	[rebase]
> 		rebaseMerges=rebase-cousins
> 		rebaseMerges=true
>
> will result in us unexpectedly rebasing cousins

>
> Phillip Wood <phillip.wood123@gmail.com> writes:
>
> >> +rebase.rebaseMerges::
> >> +    Whether and how to set the `--rebase-merges` option by default. Can
> >> +    be `rebase-cousins`, `no-rebase-cousins`, or a boolean. Setting to
> >> +    true is equivalent to `--rebase-merges` without an argument,
> >
> > This is a bit picky but how can rebase.rebaseMerges=true be equivalent
> > to --rebase-merges without an argument when the behavior of
> > --rebase-merges without an argument depends on the value of
> > rebase.rebaseMerges?
>
> True.  I think the configuration is meant to give (when set to
> something other than Boolean) the default value to the option
> "--rebase-merges" that is given without value, so setting to false
> should be a no-op (a command line option would override it if given,
> and if there is no command line option, --rebase-merges is not used
> by default), setting it to a specific value between cousin choices
> would give --rebase-merges=<value> unless --no-rebase-merges is
> given, but setting it to true here makes the result undefined,
> unless the built-in default between cousin choices is described
> here.
>
> "Setting to true is equivalent to setting to no-rebase-cousins" and
> "Setting to false is a no-op but accepted only for completeness",
> perhaps?

> >> +`no-rebase-cousins`. If the mode is not specified on the command line or in
> >> +the `rebase.rebaseMerges` config option, it defaults to `no-rebase-cousins`.
>
> This side could describe what setting it to "true" means, but it is
> a separate page so it would be more friendly to readers to cover it
> on both pages.

>> @@ -800,6 +813,15 @@ static int rebase_config(const char *var, const char *value, void *data)
>>   		return 0;
>>   	}
>>   
>> +	if (!strcmp(var, "rebase.rebasemerges")) {
>> +		opts->config_rebase_merges = git_parse_maybe_bool(value);
>> +		if (opts->config_rebase_merges < 0) {
>> +			opts->config_rebase_merges = 1;
>> +			parse_rebase_merges_value(opts, value);
>> +		}
>
> I think we need
>
> 	} else {
> 		opts->rebase_cousins = 0;
> 	}
>
> here. Otherwise if rebase.rebaseMerges is set twice we wont follow the 
> usual "last config wins" convention. For example
>
> 	[rebase]
> 		rebaseMerges=rebase-cousins
> 		rebaseMerges=true
>
> will result in us unexpectedly rebasing cousins

> The purpose of the new option is to accommodate users who would like
> --rebase-merges to be on by default and to facilitate turning on
> --rebase-merges by default without configuration in a future version of
> Git.
>
> Name the new option rebase.rebaseMerges, even though it is a little
> redundant, for consistency with the name of the command line option and
> to be clear when scrolling through values in the [rebase] section of
> .gitconfig.

>                                      Support setting rebase.rebaseMerges
> to the nonspecific value "true" for users who do not need or want to
> care about the default changing in the future. Similarly, for users who
> have --rebase-merges in an alias and want to get the future behavior
> now, use the specific rebase-merges mode from the config if a specific
> mode is not given on the command line.

> @@ -278,6 +278,74 @@ test_expect_success 'do not rebase cousins unless asked for' '
>  	EOF
>  '
>  
> +test_expect_success '--rebase-merges="" is deprecated' '
> +	git rebase --rebase-merges="" HEAD^ 2>actual &&
> +	grep deprecated actual
> +'

> > +test_expect_success '--rebase-merges="" is deprecated' '
> > +     git rebase --rebase-merges="" HEAD^ 2>actual &&
> > +     grep deprecated actual
> > +'
>
> I believe this used to be on 2/3, i.e.
>
>   https://lore.kernel.org/git/20230225180325.796624-3-alexhenrie24@gmail.com/
>
> but your cover letter suggests that it was removed. Mechanical error?

> > In the future, the default rebase-merges mode may change from
> > no-rebase-cousins to rebase-cousins.
>
> I suspect a more likely future would be that the default is to rebase
> 'evil' merges instead of trying to recreate merge commits, but of
> course, the important thing is that we promote the default, not what the
> default will be...

>> > In the future, the default rebase-merges mode may change from
>> > no-rebase-cousins to rebase-cousins.
>>
>> I suspect a more likely future would be that the default is to rebase
>> 'evil' merges instead of trying to recreate merge commits, but of
>> course, the important thing is that we promote the default, not what the
>> default will be...
>
> Glen, do you have any more thoughts? At this point, the only thing
> that's keeping me from implementing Phillip's request to make
> --rebase-merges without an argument clobber rebase.rebaseMerges is
> your suspicion that we might need to change the default rebase-merges
> mode in the future, and I assume that we would want to use the
> rebase.rebaseMerges config option to facilitate the transition.

> If users cleanly separate the two concepts, I think it is quite clear.
> (I'm not advocating for this approach, but) e.g. if we pretend that each
> behavior were configured separately, like:
>
> --[no-]rebase-merges [--rebase-merges-mode=(rebase-cousins|no-rebase-cousins)]
>
> I don't think there would be any confusion.

>> --[no-]rebase-merges [--rebase-merges-mode=(rebase-cousins|no-rebase-cousins)]
>>
>> I don't think there would be any confusion.
>> [...]
>> (Having --rebase-merges-mode
>> be a no-op without --rebase-merges is probably even more confusing to
>> users, plus this would break backwards compatibility, so I don't think
>> this is a good idea at all.)
>
> I don't find it confusing. And how would it break backwards
> compatibility if --rebase-merges-mode doesn't exist now?

>
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
> >> --[no-]rebase-merges [--rebase-merges-mode=(rebase-cousins|no-rebase-cousins)]
> >>
> >> I don't think there would be any confusion.
> >> [...]
> >> (Having --rebase-merges-mode
> >> be a no-op without --rebase-merges is probably even more confusing to
> >> users, plus this would break backwards compatibility, so I don't think
> >> this is a good idea at all.)
> >
> > I don't find it confusing. And how would it break backwards
> > compatibility if --rebase-merges-mode doesn't exist now?
>
> I meant that for the above example to work, we would need to have
> `--[no-]rebase-merges` as a boolean that says whether we rebase merges at
> all, and `--rebase-merges-mode=[whatever]` would tell us what mode to
> use _if_ we were rebasing merges.

>
> Alex Henrie <alexhenrie24@gmail.com> writes:
>
> >> > In the future, the default rebase-merges mode may change from
> >> > no-rebase-cousins to rebase-cousins.
> >>
> >> I suspect a more likely future would be that the default is to rebase
> >> 'evil' merges instead of trying to recreate merge commits, but of
> >> course, the important thing is that we promote the default, not what the
> >> default will be...
> >
> > Glen, do you have any more thoughts? At this point, the only thing
> > that's keeping me from implementing Phillip's request to make
> > --rebase-merges without an argument clobber rebase.rebaseMerges is
> > your suspicion that we might need to change the default rebase-merges
> > mode in the future, and I assume that we would want to use the
> > rebase.rebaseMerges config option to facilitate the transition.
>
> (Sorry for the late reply)
>
> Ah, I don't really have more thoughts on the matter. I am fairly
> confident that we would _like_ to change the default to rebase 'evil'
> merges, but I don't know how likely that will be.
>
> Perhaps it would help to enumerate the rules to see if it is too
> confusing or not?
>
> The behaviors we can tweak are:
>
> - Whether to rebase merges or not (true, false, specified mode, or
>   default)
> - What mode to use when rebasing merges (specified mode or default)
>
> And the sources are either CLI or config, with CLI always overriding
> config.
>
> Should we rebase a merge?
>
> - If neither CLI or config tells us whether or not to rebase a merge,
>   default to "don't rebase merges".
> - If one of CLI or config tells us whether or not to rebase a merge,
>   respect it.
> - If both CLI or config tell us whether or not to rebase a merge,
>   respect CLI and ignore config.
>
> What mode should we use?
>
> - If neither CLI or config tells us what mode to use, default to
>   "no-rebase-cousins" (or whatever default we decide).
> - If one of CLI or config tells us what mode to use, respect it.
> - If both CLI or config tell us what mode to use, respect CLI and ignore
>   config.
>
> If users cleanly separate the two concepts, I think it is quite clear.
> (I'm not advocating for this approach, but) e.g. if we pretend that each
> behavior were configured separately, like:
>
> --[no-]rebase-merges [--rebase-merges-mode=(rebase-cousins|no-rebase-cousins)]
>
> I don't think there would be any confusion. (Having --rebase-merges-mode
> be a no-op without --rebase-merges is probably even more confusing to
> users, plus this would break backwards compatibility, so I don't think
> this is a good idea at all.)
>
> Your doc patch explains the rules pretty clearly, but perhaps it doesn't
> explain this mental model clearly enough, hence the confusion. If we
> don't find a good way to communicate this (I think it is clear, but
> other reviewers seem yet unconvinced), I wouldn't mind taking Phillip's
> suggestion to have "--rebase-merges" override
> "rebase.rebaseMerges='specific-mode'".

>> Your doc patch explains the rules pretty clearly, but perhaps it doesn't
>> explain this mental model clearly enough, hence the confusion. If we
>> don't find a good way to communicate this (I think it is clear, but
>> other reviewers seem yet unconvinced), I wouldn't mind taking Phillip's
>> suggestion to have "--rebase-merges" override
>> "rebase.rebaseMerges='specific-mode'".
>
> I got the impression that everyone, including Phillip,[1] already
> agrees that the proposed documentation is clear about the interaction
> between the config option and the command line option. However, it is
> a little weird when you consider that other flags with optional
> arguments, like `git branch --track`, unconditionally override their
> corresponding config options.[2]

>
> -Alex
>
> [1] https://lore.kernel.org/git/7cf19017-518b-245e-aea2-5dee55f88276@dunelm.org.uk/
> [2] https://lore.kernel.org/git/5551d67b-3021-8cfc-53b5-318f223ded6d@dunelm.org.uk/

>
> Alex Henrie <alexhenrie24@gmail.com> writes:
>
> >> Your doc patch explains the rules pretty clearly, but perhaps it doesn't
> >> explain this mental model clearly enough, hence the confusion. If we
> >> don't find a good way to communicate this (I think it is clear, but
> >> other reviewers seem yet unconvinced), I wouldn't mind taking Phillip's
> >> suggestion to have "--rebase-merges" override
> >> "rebase.rebaseMerges='specific-mode'".
> >
> > I got the impression that everyone, including Phillip,[1] already
> > agrees that the proposed documentation is clear about the interaction
> > between the config option and the command line option. However, it is
> > a little weird when you consider that other flags with optional
> > arguments, like `git branch --track`, unconditionally override their
> > corresponding config options.[2]
>
> Ah, I didn't consider other options like `git branch --track`. I haven't
> looked into what is the norm, but I think we should follow it (whatever
> it is).
>
> If other reviewers have a strong idea of what this norm is, I am happy
> to defer to them. If not, I can look into it given some time.
>
> > Let me ask a different but related question: If we add a
> > rebase-evil-merges mode, do you think that would be orthogonal to the
> > rebase-cousins mode?
>
> I am not an expert on this, so perhaps my opinion isn't that important
> ;)
>
> My understanding is that `[no-]rebase-cousins` affects which commits get
> rebased and onto where, whereas `rebase-evil-merges` would affect how
> the merge commits are generated (by rebasing the evil or by simply
> recreating the merges). From that perspective, it seems like yes, the
> two would be orthogonal.
>
> Hm. Maybe this means that we'd be introducing a new option, and that my
> hunch that we would change the default to `rebase-evil-merges` is more
> wrong than I expected.
>
> Though I guess this doesn't really affects what we do with the CLI
> options _now_, since the discussion is about what we do about defaults,
> and what the default is is quite immaterial.

> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 6635f10d52..c36ddc0050 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1140,7 +1140,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   		{OPTION_STRING, 'r', "rebase-merges", &rebase_merges,
>   			N_("mode"),
>   			N_("try to rebase merges instead of skipping them"),
> -			PARSE_OPT_OPTARG, NULL, (intptr_t)""},
> +			PARSE_OPT_OPTARG, NULL, (intptr_t)"no-rebase-cousins"},

>   		OPT_BOOL(0, "fork-point", &options.fork_point,
>   			 N_("use 'merge-base --fork-point' to refine upstream")),
>   		OPT_STRING('s', "strategy", &options.strategy,
> @@ -1438,7 +1438,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   
>   	if (rebase_merges) {
>   		if (!*rebase_merges)
> -			; /* default mode; do nothing */
> +			warning(_("--rebase-merges with an empty string "
> +				  "argument is deprecated and will stop "
> +				  "working in a future version of Git. Use "
> +				  "--rebase-merges without an argument "
> +				  "instead, which does the same thing."));
>   		else if (!strcmp("rebase-cousins", rebase_merges))
>   			options.rebase_cousins = 1;
>   		else if (strcmp("no-rebase-cousins", rebase_merges))

>
> Alex Henrie <alexhenrie24@gmail.com> writes:
>
> [...]
>
> > - Rephrase the warning about --rebase-merges="" to recommend
> >   --rebase-merges without an argument instead, and clarify that that
> >   will do the same thing
>
> Not a strong objection to the series as they are, but, provided options
> with optional arguments are considered to be a bad practice in general
> (unless something had recently changed), I'd like to vote for adding:
>
>   --rebase-merges=on (≡ --reabase-merges="")
>
> and for suggesting to use that one instead of --rebase-merges="" rather
> than naked --rebase-merges.
>
> It'd be best to actually fix --rebase-merges to always have an argument,
> the more so as we have '-r' shortcut, but as this is unlikely to fly due
> to backward compatibility considerations, this way we would at least
> avoid promoting bad practices further.
>
> If accepted, please consider to introduce
>
>   --rebase-merges=off (≡ --no-rebase-merges)
>
> as well for completeness.
>
> BTW, that's how we settled upon in the implementation of --diff-merges,
> so these two will then be mutually consistent.

> On Sun, Mar 5, 2023 at 5:22 AM Sergey Organov <sorganov@gmail.com> wrote:
>>
>> Alex Henrie <alexhenrie24@gmail.com> writes:
>>
>> [...]
>>
>> > - Rephrase the warning about --rebase-merges="" to recommend
>> >   --rebase-merges without an argument instead, and clarify that that
>> >   will do the same thing
>>
>> Not a strong objection to the series as they are, but, provided options
>> with optional arguments are considered to be a bad practice in general
>> (unless something had recently changed), I'd like to vote for adding:
>>
>>   --rebase-merges=on (≡ --reabase-merges="")
>>
>> and for suggesting to use that one instead of --rebase-merges="" rather
>> than naked --rebase-merges.
>>
>> It'd be best to actually fix --rebase-merges to always have an argument,
>> the more so as we have '-r' shortcut, but as this is unlikely to fly due
>> to backward compatibility considerations, this way we would at least
>> avoid promoting bad practices further.
>>
>> If accepted, please consider to introduce
>>
>>   --rebase-merges=off (≡ --no-rebase-merges)
>>
>> as well for completeness.
>>
>> BTW, that's how we settled upon in the implementation of --diff-merges,
>> so these two will then be mutually consistent.
>
> I am curious as to why you say that flags with optional arguments are
> considered bad practice.

>
> Alex Henrie <alexhenrie24@gmail.com> writes:
>
> > On Sun, Mar 5, 2023 at 5:22 AM Sergey Organov <sorganov@gmail.com> wrote:
> >>
> >> Alex Henrie <alexhenrie24@gmail.com> writes:
> >>
> >> [...]
> >>
> >> > - Rephrase the warning about --rebase-merges="" to recommend
> >> >   --rebase-merges without an argument instead, and clarify that that
> >> >   will do the same thing
> >>
> >> Not a strong objection to the series as they are, but, provided options
> >> with optional arguments are considered to be a bad practice in general
> >> (unless something had recently changed), I'd like to vote for adding:
> >>
> >>   --rebase-merges=on (≡ --reabase-merges="")
> >>
> >> and for suggesting to use that one instead of --rebase-merges="" rather
> >> than naked --rebase-merges.
> >>
> >> It'd be best to actually fix --rebase-merges to always have an argument,
> >> the more so as we have '-r' shortcut, but as this is unlikely to fly due
> >> to backward compatibility considerations, this way we would at least
> >> avoid promoting bad practices further.
> >>
> >> If accepted, please consider to introduce
> >>
> >>   --rebase-merges=off (≡ --no-rebase-merges)
> >>
> >> as well for completeness.
> >>
> >> BTW, that's how we settled upon in the implementation of --diff-merges,
> >> so these two will then be mutually consistent.
> >
> > I am curious as to why you say that flags with optional arguments are
> > considered bad practice.
>
> As far as I can tell, the core problem with such options is that generic
> options parsing code can't tell if in "--option value" "value" is an
> argument to "--option", or separate argument, that in turn may lead to
> very surprising behaviors and bugs.

> > As it is, we're certainly not going to drop support for
> > --no-rebase-merges
>
> --no-rebase-merges is fine, as it has no optional arguments
>
> > or --rebase-merges without an argument,
>
> Yep, but that's a pity and the whole point of my comment: if we can't
> fix it, at least don't promote it.
>
> > so it seems fine to advertise them to users.
>
> --no-rebase-merges is fine, but then you don't advertise it anyway.

>> > so it seems fine to advertise them to users.
>>
>> --no-rebase-merges is fine, but then you don't advertise it anyway.
>
> I am not sure what you mean by this. The first patch of the series
> adds documentation and a test for --no-rebase-merges, so I am
> advertising it.

> Interesting, thank you for clarifying. I just tried it and it appears
> that --rebase-merges requires an equals sign when it has an argument.
> For example:
>
> $ git rebase --rebase-merges=rebase-cousins
> Current branch master is up to date.
>
> $ git rebase --rebase-merges rebase-cousins
> fatal: invalid upstream 'rebase-cousins'
>
> So there is no ambiguity because if an argument to a flag is optional,
> an equals sign is required.

>> I am curious as to why you say that flags with optional arguments are
>> considered bad practice.
>
> As far as I can tell, the core problem with such options is that generic
> options parsing code can't tell if in "--option value" "value" is an
> argument to "--option", or separate argument, that in turn may lead to
> very surprising behaviors and bugs.

> This patch series introduces a rebase.rebaseMerges config option to
> accommodate users who would like --rebase-merges to be on by default and
> to facilitate turning on --rebase-merges by default without
> configuration in a future version of Git. It also cleans up and
> documents the behavior of the --rebase-merges command line option to
> avoid confusion about how the config option and the command line option
> interact.
> 
> Changes from v5:
> - Add commit message note about --no-rebase-merges having always worked
> - Add commit message note about the test for --no-rebase-merges being
>    somewhat contrived
> - Rephrase the documentation to avoid using the phrase "By default" with
>    two different meanings, and in so doing clarify that --rebase-merges
>    without an argument is not the same as --no-rebase-merges and not
>    passing --rebase-merges is not the same as passing
>    --rebase-merges=no-rebase-cousins
> - Add commit message note about keeping --rebase-merges="" for now out
>    of an abundance of caution
> - Rephrase the warning about --rebase-merges="" to recommend
>    --rebase-merges without an argument instead, and clarify that that
>    will do the same thing
> - Remove the test for --rebase-merges=""
> - Rename the config option from rebase.merges to rebase.rebaseMerges and
>    explain why in the commit message
> - Add commit message note about why "true" is a valid option for
>    rebase.rebaseMerges and why --rebase-merges without an argument does
>    not clobber the mode specified in rebase.rebaseMerges
> - Rephrase the documentation to clarify that --rebase-merges without an
>    argument does not clobber the mode specified in rebase.rebaseMerges
> - Add another test for incompatible options
> 
> Suggestions on v5 not incorporated in v6:

> Thanks to Glen, Calvin, Junio, Jonathan, Elijah, and Phillip for your
> feedback on v5. As before, if you feel strongly about one of the
> suggestions that I have not incorporated into v6, or if you see
> something else amiss, let's continue the discussion.
> 
> Alex Henrie (3):
>    rebase: add documentation and test for --no-rebase-merges
>    rebase: deprecate --rebase-merges=""
>    rebase: add a config option for --rebase-merges
> 
>   Documentation/config/rebase.txt        | 11 ++++
>   Documentation/git-rebase.txt           | 19 ++++---
>   builtin/rebase.c                       | 75 ++++++++++++++++++-------
>   t/t3422-rebase-incompatible-options.sh | 17 ++++++
>   t/t3430-rebase-merges.sh               | 78 ++++++++++++++++++++++++++
>   5 files changed, 174 insertions(+), 26 deletions(-)
> 
> Range-diff against v5:
> 1:  76e38ef9f8 ! 1:  bf08c03ba7 rebase: add documentation and test for --no-rebase-merges
>      @@ Metadata
>        ## Commit message ##
>           rebase: add documentation and test for --no-rebase-merges
>       
>      +    As far as I can tell, --no-rebase-merges has always worked, but has
>      +    never been documented. It is especially important to document it before
>      +    a rebase.rebaseMerges option is introduced so that users know how to
>      +    override the config option on the command line. It's also important to
>      +    clarify that --rebase-merges without an argument is not the same as
>      +    --no-rebase-merges and not passing --rebase-merges is not the same as
>      +    passing --rebase-merges=no-rebase-cousins.
>      +
>      +    A test case is necessary to make sure that --no-rebase-merges keeps
>      +    working after its code is refactored in the following patches of this
>      +    series. The test case is a little contrived: It's unlikely that a user
>      +    would type both --rebase-merges and --no-rebase-merges at the same time.
>      +    However, if an alias is defined which includes --rebase-merges, the user
>      +    might decide to add --no-rebase-merges to countermand that part of the
>      +    alias but leave alone other flags set by the alias.
>      +
>           Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
>       
>        ## Documentation/git-rebase.txt ##
>      @@ Documentation/git-rebase.txt: See also INCOMPATIBLE OPTIONS below.
>       +	resolved/re-applied manually. `--no-rebase-merges` can be used to
>       +	countermand a previous `--rebase-merges`.
>        +
>      - By default, or when `no-rebase-cousins` was specified, commits which do not
>      - have `<upstream>` as direct ancestor will keep their original branch point,
>      +-By default, or when `no-rebase-cousins` was specified, commits which do not
>      +-have `<upstream>` as direct ancestor will keep their original branch point,
>      +-i.e. commits that would be excluded by linkgit:git-log[1]'s
>      +-`--ancestry-path` option will keep their original ancestry by default. If
>      +-the `rebase-cousins` mode is turned on, such commits are instead rebased
>      +-onto `<upstream>` (or `<onto>`, if specified).
>      ++When rebasing merges, there are two modes: `rebase-cousins` and
>      ++`no-rebase-cousins`. If the mode is not specified, it defaults to
>      ++`no-rebase-cousins`. In `no-rebase-cousins` mode, commits which do not have
>      ++`<upstream>` as direct ancestor will keep their original branch point, i.e.
>      ++commits that would be excluded by linkgit:git-log[1]'s `--ancestry-path`
>      ++option will keep their original ancestry by default. In `rebase-cousins` mode,
>      ++such commits are instead rebased onto `<upstream>` (or `<onto>`, if
>      ++specified).
>      + +
>      + It is currently only possible to recreate the merge commits using the
>      + `ort` merge strategy; different merge strategies can be used only via
>       
>        ## t/t3430-rebase-merges.sh ##
>       @@ t/t3430-rebase-merges.sh: test_expect_success 'with a branch tip that was cherry-picked already' '
> 2:  c6099e6dee ! 2:  26f98b8400 rebase: deprecate --rebase-merges=""
>      @@ Commit message
>       
>           The unusual syntax --rebase-merges="" (that is, --rebase-merges with an
>           empty string argument) has been an undocumented synonym of
>      -    --rebase-merges=no-rebase-cousins. Deprecate that syntax to avoid
>      -    confusion when a rebase.merges config option is introduced, where
>      -    rebase.merges="" will be equivalent to --no-rebase-merges.
>      +    --rebase-merges without an argument. Deprecate that syntax to avoid
>      +    confusion when a rebase.rebaseMerges config option is introduced, where
>      +    rebase.rebaseMerges="" will be equivalent to --no-rebase-merges.
>      +
>      +    It is not likely that anyone is actually using this syntax, but just in
>      +    case, deprecate the empty string argument instead of dropping support
>      +    for it immediately.
>       
>           Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
>       
>      @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix
>       +			warning(_("--rebase-merges with an empty string "
>       +				  "argument is deprecated and will stop "
>       +				  "working in a future version of Git. Use "
>      -+				  "--rebase-merges=no-rebase-cousins "
>      -+				  "instead."));
>      ++				  "--rebase-merges without an argument "
>      ++				  "instead, which does the same thing."));
>        		else if (!strcmp("rebase-cousins", rebase_merges))
>        			options.rebase_cousins = 1;
>        		else if (strcmp("no-rebase-cousins", rebase_merges))
>      -
>      - ## t/t3430-rebase-merges.sh ##
>      -@@ t/t3430-rebase-merges.sh: test_expect_success 'do not rebase cousins unless asked for' '
>      - 	EOF
>      - '
>      -
>      -+test_expect_success '--rebase-merges="" is deprecated' '
>      -+	git rebase --rebase-merges="" HEAD^ 2>actual &&
>      -+	grep deprecated actual
>      -+'
>      -+
>      - test_expect_success 'refs/rewritten/* is worktree-local' '
>      - 	git worktree add wt &&
>      - 	cat >wt/script-from-scratch <<-\EOF &&
> 3:  95cba9588c ! 3:  402365256c rebase: add a config option for --rebase-merges
>      @@ Commit message
>           rebase: add a config option for --rebase-merges
>       
>           The purpose of the new option is to accommodate users who would like
>      -    --rebase-merges to be on by default and to facilitate possibly turning
>      -    on --rebase-merges by default without configuration in a future version
>      -    of Git.
>      +    --rebase-merges to be on by default and to facilitate turning on
>      +    --rebase-merges by default without configuration in a future version of
>      +    Git.
>      +
>      +    Name the new option rebase.rebaseMerges, even though it is a little
>      +    redundant, for consistency with the name of the command line option and
>      +    to be clear when scrolling through values in the [rebase] section of
>      +    .gitconfig.
>      +
>      +    In the future, the default rebase-merges mode may change from
>      +    no-rebase-cousins to rebase-cousins. Support setting rebase.rebaseMerges
>      +    to the nonspecific value "true" for users who do not need or want to
>      +    care about the default changing in the future. Similarly, for users who
>      +    have --rebase-merges in an alias and want to get the future behavior
>      +    now, use the specific rebase-merges mode from the config if a specific
>      +    mode is not given on the command line.
>       
>           Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
>       
>      @@ Documentation/config/rebase.txt: rebase.rescheduleFailedExec::
>        rebase.forkPoint::
>        	If set to false set `--no-fork-point` option by default.
>       +
>      -+rebase.merges::
>      ++rebase.rebaseMerges::
>       +	Whether and how to set the `--rebase-merges` option by default. Can
>       +	be `rebase-cousins`, `no-rebase-cousins`, or a boolean. Setting to
>       +	true is equivalent to `--rebase-merges` without an argument, setting to
>       +	`rebase-cousins` or `no-rebase-cousins` is equivalent to
>       +	`--rebase-merges` with that value as its argument, and setting to false
>       +	is equivalent to `--no-rebase-merges`. Passing `--rebase-merges` on the
>      -+	command line without an argument overrides a `rebase.merges=false`
>      -+	configuration but does not override other values of `rebase.merge`.
>      ++	command line without an argument overrides a `rebase.rebaseMerges=false`
>      ++	configuration, but the absence of a specific rebase-merges mode on the
>      ++	command line does not counteract a specific mode set in the configuration.
>       
>        ## Documentation/git-rebase.txt ##
>       @@ Documentation/git-rebase.txt: See also INCOMPATIBLE OPTIONS below.
>      @@ Documentation/git-rebase.txt: See also INCOMPATIBLE OPTIONS below.
>        	manual amendments in these merge commits will have to be
>        	resolved/re-applied manually. `--no-rebase-merges` can be used to
>       -	countermand a previous `--rebase-merges`.
>      -+	countermand both the `rebase.merges` config option and a previous
>      ++	countermand both the `rebase.rebaseMerges` config option and a previous
>       +	`--rebase-merges`.
>        +
>      - By default, or when `no-rebase-cousins` was specified, commits which do not
>      - have `<upstream>` as direct ancestor will keep their original branch point,
>      + When rebasing merges, there are two modes: `rebase-cousins` and
>      +-`no-rebase-cousins`. If the mode is not specified, it defaults to
>      +-`no-rebase-cousins`. In `no-rebase-cousins` mode, commits which do not have
>      +-`<upstream>` as direct ancestor will keep their original branch point, i.e.
>      +-commits that would be excluded by linkgit:git-log[1]'s `--ancestry-path`
>      +-option will keep their original ancestry by default. In `rebase-cousins` mode,
>      +-such commits are instead rebased onto `<upstream>` (or `<onto>`, if
>      +-specified).
>      ++`no-rebase-cousins`. If the mode is not specified on the command line or in
>      ++the `rebase.rebaseMerges` config option, it defaults to `no-rebase-cousins`.
>      ++In `no-rebase-cousins` mode, commits which do not have `<upstream>` as direct
>      ++ancestor will keep their original branch point, i.e. commits that would be
>      ++excluded by linkgit:git-log[1]'s `--ancestry-path` option will keep their
>      ++original ancestry by default. In `rebase-cousins` mode, such commits are
>      ++instead rebased onto `<upstream>` (or `<onto>`, if specified).
>      + +
>      + It is currently only possible to recreate the merge commits using the
>      + `ort` merge strategy; different merge strategies can be used only via
>       
>        ## builtin/rebase.c ##
>       @@ builtin/rebase.c: struct rebase_options {
>      @@ builtin/rebase.c: static int rebase_config(const char *var, const char *value, v
>        		return 0;
>        	}
>        
>      -+	if (!strcmp(var, "rebase.merges")) {
>      ++	if (!strcmp(var, "rebase.rebasemerges")) {
>       +		opts->config_rebase_merges = git_parse_maybe_bool(value);
>       +		if (opts->config_rebase_merges < 0) {
>       +			opts->config_rebase_merges = 1;
>      @@ builtin/rebase.c: static int parse_opt_empty(const struct option *opt, const cha
>       +			warning(_("--rebase-merges with an empty string "
>       +				  "argument is deprecated and will stop "
>       +				  "working in a future version of Git. Use "
>      -+				  "--rebase-merges=no-rebase-cousins "
>      -+				  "instead."));
>      -+			arg = "no-rebase-cousins";
>      ++				  "--rebase-merges without an argument "
>      ++				  "instead, which does the same thing."));
>      ++			return 0;
>       +		}
>       +		parse_rebase_merges_value(options, arg);
>       +	}
>      @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix
>       -			warning(_("--rebase-merges with an empty string "
>       -				  "argument is deprecated and will stop "
>       -				  "working in a future version of Git. Use "
>      --				  "--rebase-merges=no-rebase-cousins "
>      --				  "instead."));
>      +-				  "--rebase-merges without an argument "
>      +-				  "instead, which does the same thing."));
>       -		else if (!strcmp("rebase-cousins", rebase_merges))
>       -			options.rebase_cousins = 1;
>       -		else if (strcmp("no-rebase-cousins", rebase_merges))
>      @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix
>       +			if (options.autosquash == -1 && options.config_autosquash == 1)
>        				die(_("apply options are incompatible with rebase.autosquash.  Consider adding --no-autosquash"));
>       +			else if (options.rebase_merges == -1 && options.config_rebase_merges == 1)
>      -+				die(_("apply options are incompatible with rebase.merges.  Consider adding --no-rebase-merges"));
>      ++				die(_("apply options are incompatible with rebase.rebaseMerges.  Consider adding --no-rebase-merges"));
>        			else if (options.update_refs == -1 && options.config_update_refs == 1)
>        				die(_("apply options are incompatible with rebase.updateRefs.  Consider adding --no-update-refs"));
>       +			else if (is_merge(&options))
>      @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix
>        	options.autosquash = (options.autosquash >= 0) ? options.autosquash :
>       
>        ## t/t3422-rebase-incompatible-options.sh ##
>      +@@ t/t3422-rebase-incompatible-options.sh: test_rebase_am_only () {
>      + 		test_must_fail git rebase $opt --reapply-cherry-picks A
>      + 	"
>      +
>      ++	test_expect_success "$opt incompatible with --rebase-merges" "
>      ++		git checkout B^0 &&
>      ++		test_must_fail git rebase $opt --rebase-merges A
>      ++	"
>      ++
>      + 	test_expect_success "$opt incompatible with --update-refs" "
>      + 		git checkout B^0 &&
>      + 		test_must_fail git rebase $opt --update-refs A
>       @@ t/t3422-rebase-incompatible-options.sh: test_rebase_am_only () {
>        		grep -e --no-autosquash err
>        	"
>        
>      -+	test_expect_success "$opt incompatible with rebase.merges" "
>      ++	test_expect_success "$opt incompatible with rebase.rebaseMerges" "
>       +		git checkout B^0 &&
>      -+		test_must_fail git -c rebase.merges=true rebase $opt A 2>err &&
>      ++		test_must_fail git -c rebase.rebaseMerges=true rebase $opt A 2>err &&
>       +		grep -e --no-rebase-merges err
>       +	"
>       +
>      @@ t/t3422-rebase-incompatible-options.sh: test_rebase_am_only () {
>        		git -c rebase.autosquash=true rebase --no-autosquash $opt A
>        	"
>        
>      -+	test_expect_success "$opt okay with overridden rebase.merges" "
>      ++	test_expect_success "$opt okay with overridden rebase.rebaseMerges" "
>       +		test_when_finished \"git reset --hard B^0\" &&
>       +		git checkout B^0 &&
>      -+		git -c rebase.merges=true rebase --no-rebase-merges $opt A
>      ++		git -c rebase.rebaseMerges=true rebase --no-rebase-merges $opt A
>       +	"
>       +
>        	test_expect_success "$opt okay with overridden rebase.updateRefs" "
>      @@ t/t3422-rebase-incompatible-options.sh: test_rebase_am_only () {
>        		git checkout B^0 &&
>       
>        ## t/t3430-rebase-merges.sh ##
>      -@@ t/t3430-rebase-merges.sh: test_expect_success '--rebase-merges="" is deprecated' '
>      - 	grep deprecated actual
>      +@@ t/t3430-rebase-merges.sh: test_expect_success 'do not rebase cousins unless asked for' '
>      + 	EOF
>        '
>        
>      -+test_expect_success 'rebase.merges=rebase-cousins is equivalent to --rebase-merges=rebase-cousins' '
>      -+	test_config rebase.merges rebase-cousins &&
>      ++test_expect_success '--rebase-merges="" is deprecated' '
>      ++	git rebase --rebase-merges="" HEAD^ 2>actual &&
>      ++	grep deprecated actual
>      ++'
>      ++
>      ++test_expect_success 'rebase.rebaseMerges=rebase-cousins is equivalent to --rebase-merges=rebase-cousins' '
>      ++	test_config rebase.rebaseMerges rebase-cousins &&
>       +	git checkout -b config-rebase-cousins main &&
>       +	git rebase HEAD^ &&
>       +	test_cmp_graph HEAD^.. <<-\EOF
>      @@ t/t3430-rebase-merges.sh: test_expect_success '--rebase-merges="" is deprecated'
>       +	EOF
>       +'
>       +
>      -+test_expect_success '--no-rebase-merges overrides rebase.merges=no-rebase-cousins' '
>      -+	test_config rebase.merges no-rebase-cousins &&
>      ++test_expect_success '--no-rebase-merges overrides rebase.rebaseMerges=no-rebase-cousins' '
>      ++	test_config rebase.rebaseMerges no-rebase-cousins &&
>       +	git checkout -b override-config-no-rebase-cousins E &&
>       +	git rebase --no-rebase-merges C &&
>       +	test_cmp_graph C.. <<-\EOF
>      @@ t/t3430-rebase-merges.sh: test_expect_success '--rebase-merges="" is deprecated'
>       +	EOF
>       +'
>       +
>      -+test_expect_success '--rebase-merges=no-rebase-cousins overrides rebase.merges=rebase-cousins' '
>      -+	test_config rebase.merges rebase-cousins &&
>      ++test_expect_success '--rebase-merges=no-rebase-cousins overrides rebase.rebaseMerges=rebase-cousins' '
>      ++	test_config rebase.rebaseMerges rebase-cousins &&
>       +	git checkout -b override-config-rebase-cousins main &&
>       +	git rebase --rebase-merges=no-rebase-cousins HEAD^ &&
>       +	test_cmp_graph HEAD^.. <<-\EOF
>      @@ t/t3430-rebase-merges.sh: test_expect_success '--rebase-merges="" is deprecated'
>       +	EOF
>       +'
>       +
>      -+test_expect_success '--rebase-merges overrides rebase.merges=false' '
>      -+	test_config rebase.merges false &&
>      ++test_expect_success '--rebase-merges overrides rebase.rebaseMerges=false' '
>      ++	test_config rebase.rebaseMerges false &&
>       +	git checkout -b override-config-merges-false E &&
>       +	before="$(git rev-parse --verify HEAD)" &&
>       +	test_tick &&
>      @@ t/t3430-rebase-merges.sh: test_expect_success '--rebase-merges="" is deprecated'
>       +	test_cmp_rev HEAD $before
>       +'
>       +
>      -+test_expect_success '--rebase-merges does not override rebase.merges=rebase-cousins' '
>      -+	test_config rebase.merges rebase-cousins &&
>      ++test_expect_success '--rebase-merges does not override rebase.rebaseMerges=rebase-cousins' '
>      ++	test_config rebase.rebaseMerges rebase-cousins &&
>       +	git checkout -b no-override-config-rebase-cousins main &&
>       +	git rebase --rebase-merges HEAD^ &&
>       +	test_cmp_graph HEAD^.. <<-\EOF

> > Suggestions on v5 not incorporated in v6:
>
> Thanks for adding this, it is really helpful to see what did not change
> as well as what did change. It is also helpful to briefly explain why
> you disagree with the suggestions so others can understand why you
> decided not to make these changes.
>
> > - Make --rebase-merges without an argument clobber the mode specified in
> >    rebase.rebaseMerges
>
> I'm still confused as to why we want the config value to change the
> behavior of --rebase-merges. Is it for "git pull --rebase=merges"? If
> that's the case then I think a better approach is for pull to parse
> rebase.merges and pass the appropriate argument to rebase. That way we
> don't break the expectation that command line arguments override config
> values.
>
> > - Remove the test for --rebase-merges=no-rebase-cousins overriding
> >    rebase.rebaseMerges=rebase-cousins
> > - In the tests, check the graph itself instead of checking that the
> >    graph has not changed by checking that the commit hash has not changed
>
> I'm not sure what value the existing test has if it only checks that
> HEAD is unchanged after the rebase. It could be unchanged because the
> rebase fast-forwarded or because it did nothing.

> Changes from v5:
> - Add commit message note about --no-rebase-merges having always worked
> - Add commit message note about the test for --no-rebase-merges being
>   somewhat contrived
> - Rephrase the documentation to avoid using the phrase "By default" with
>   two different meanings, and in so doing clarify that --rebase-merges
>   without an argument is not the same as --no-rebase-merges and not
>   passing --rebase-merges is not the same as passing
>   --rebase-merges=no-rebase-cousins
> - Add commit message note about keeping --rebase-merges="" for now out
>   of an abundance of caution
> - Rephrase the warning about --rebase-merges="" to recommend
>   --rebase-merges without an argument instead, and clarify that that
>   will do the same thing
> - Remove the test for --rebase-merges=""
> - Rename the config option from rebase.merges to rebase.rebaseMerges and
>   explain why in the commit message
> - Add commit message note about why "true" is a valid option for
>   rebase.rebaseMerges and why --rebase-merges without an argument does
>   not clobber the mode specified in rebase.rebaseMerges
> - Rephrase the documentation to clarify that --rebase-merges without an
>   argument does not clobber the mode specified in rebase.rebaseMerges
> - Add another test for incompatible options

> The purpose of the new option is to accommodate users who would like
> --rebase-merges to be on by default and to facilitate turning on
> --rebase-merges by default without configuration in a future version of
> Git.
> 
> Name the new option rebase.rebaseMerges, even though it is a little
> redundant, for consistency with the name of the command line option and
> to be clear when scrolling through values in the [rebase] section of
> .gitconfig.
> 
> Support setting rebase.rebaseMerges to the nonspecific value "true" for
> users who don't need to or don't want to learn about the difference
> between rebase-cousins and no-rebase-cousins.
> 
> Make --rebase-merges without an argument on the command line override
> any value of rebase.rebaseMerges in the configuration, for consistency
> with other command line flags with optional arguments that have an
> associated config option.

> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
> ---
>   Documentation/config/rebase.txt        | 10 ++++
>   Documentation/git-rebase.txt           |  3 +-
>   builtin/rebase.c                       | 81 ++++++++++++++++++--------
>   t/t3422-rebase-incompatible-options.sh | 17 ++++++
>   t/t3430-rebase-merges.sh               | 34 +++++++++++
>   5 files changed, 121 insertions(+), 24 deletions(-)
> 
> diff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt
> index f19bd0e040..afaf6dad99 100644
> --- a/Documentation/config/rebase.txt
> +++ b/Documentation/config/rebase.txt
> @@ -67,3 +67,13 @@ rebase.rescheduleFailedExec::
>   
>   rebase.forkPoint::
>   	If set to false set `--no-fork-point` option by default.
> +
> +rebase.rebaseMerges::
> +	Whether and how to set the `--rebase-merges` option by default. Can
> +	be `rebase-cousins`, `no-rebase-cousins`, or a boolean. Setting to
> +	true or to `no-rebase-cousins` is equivalent to
> +	`--rebase-merges=no-rebase-cousins`, setting to `rebase-cousins` is
> +	equivalent to `--rebase-merges=rebase-cousins`, and setting to false is
> +	equivalent to `--no-rebase-merges`. Passing `--rebase-merges` on the
> +	command line, with or without an argument, overrides any
> +	`rebase.rebaseMerges` configuration.

> [...]   
> +	if (!strcmp(var, "rebase.rebasemerges")) {
> +		opts->config_rebase_merges = git_parse_maybe_bool(value);
> +		if (opts->config_rebase_merges < 0) {
> +			opts->config_rebase_merges = 1;
> +			parse_rebase_merges_value(opts, value);
> +		} else
> +			opts->rebase_cousins = 0;

> [...]   
> +static int parse_opt_rebase_merges(const struct option *opt, const char *arg, int unset)
> +{
> +	struct rebase_options *options = opt->value;
> +
> +	options->rebase_merges = !unset;
> +	options->rebase_cousins = 0;

> @@ -1514,13 +1542,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   				break;
>   
>   		if (i >= 0 || options.type == REBASE_APPLY) {
> -			if (is_merge(&options))
> -				die(_("apply options and merge options "
> -					  "cannot be used together"));

> -			else if (options.autosquash == -1 && options.config_autosquash == 1)
> +			if (options.autosquash == -1 && options.config_autosquash == 1)
>   				die(_("apply options are incompatible with rebase.autoSquash.  Consider adding --no-autosquash"));
> +			else if (options.rebase_merges == -1 && options.config_rebase_merges == 1)
> +				die(_("apply options are incompatible with rebase.rebaseMerges.  Consider adding --no-rebase-merges"));
>   			else if (options.update_refs == -1 && options.config_update_refs == 1)
>   				die(_("apply options are incompatible with rebase.updateRefs.  Consider adding --no-update-refs"));
> +			else if (is_merge(&options))
> +				die(_("apply options and merge options "
> +					  "cannot be used together"));
>   			else
>   				options.type = REBASE_APPLY;
>   		}

>> @@ -1514,13 +1542,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>>   				break;
>>     		if (i >= 0 || options.type == REBASE_APPLY) {
>> -			if (is_merge(&options))
>> -				die(_("apply options and merge options "
>> -					  "cannot be used together"));
>
> This isn't a new change but having thought about it I'm not sure
> moving this code is a good idea. If the user runs
>
> 	git -c rebase.updateRefs=true rebase --whitespace=fix --exec "make test"
>
> then instead of getting an message saying that they cannot use apply
> and merge options together they'll get a message suggesting they pass
> --no-update-refs which wont fix the problem for them.

>> +			if (options.autosquash == -1 && options.config_autosquash == 1)
>>   				die(_("apply options are incompatible with rebase.autoSquash.  Consider adding --no-autosquash"));
>> +			else if (options.rebase_merges == -1 && options.config_rebase_merges == 1)
>> +				die(_("apply options are incompatible with rebase.rebaseMerges.  Consider adding --no-rebase-merges"));
>>   			else if (options.update_refs == -1 && options.config_update_refs == 1)
>>   				die(_("apply options are incompatible with rebase.updateRefs.  Consider adding --no-update-refs"));

>> +			else if (is_merge(&options))
>> +				die(_("apply options and merge options "
>> +					  "cannot be used together"));
>>   			else
>>   				options.type = REBASE_APPLY;

> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>>> @@ -1514,13 +1542,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>>>    				break;
>>>      		if (i >= 0 || options.type == REBASE_APPLY) {
>>> -			if (is_merge(&options))
>>> -				die(_("apply options and merge options "
>>> -					  "cannot be used together"));
>>
>> This isn't a new change but having thought about it I'm not sure
>> moving this code is a good idea. If the user runs
>>
>> 	git -c rebase.updateRefs=true rebase --whitespace=fix --exec "make test"
>>
>> then instead of getting an message saying that they cannot use apply
>> and merge options together they'll get a message suggesting they pass
>> --no-update-refs which wont fix the problem for them.
> 
> Hmph.  Instead of dying here, ...
> 
>>> +			if (options.autosquash == -1 && options.config_autosquash == 1)
>>>    				die(_("apply options are incompatible with rebase.autoSquash.  Consider adding --no-autosquash"));
>>> +			else if (options.rebase_merges == -1 && options.config_rebase_merges == 1)
>>> +				die(_("apply options are incompatible with rebase.rebaseMerges.  Consider adding --no-rebase-merges"));
>>>    			else if (options.update_refs == -1 && options.config_update_refs == 1)
>>>    				die(_("apply options are incompatible with rebase.updateRefs.  Consider adding --no-update-refs"));
> 
> ... we get caught here, and the next one is not triggered.
> 
>>> +			else if (is_merge(&options))
>>> +				die(_("apply options and merge options "
>>> +					  "cannot be used together"));
>>>    			else
>>>    				options.type = REBASE_APPLY;
> 
> What's the reason why "cannot be used together" is moved to the last
> in the chain?
> 
> The first two new conditions in this chain try to catch an
> invocation with some apply-specific command line option
> (e.g. "--whitespace=fix") when used with configuration variables
> specific to the merge-backend (e.g. "rebase.merges") and suggest
> overriding the configuration from the command line, and I suspect
> that the motivation behind this change is that their error messages
> are more specific than the generic "apply and merge do not mix".
> 
> But are these two new errors and hints universally a good suggestion
> to make?  I actually think a command line option forcing us to use
> the apply backend should simply silently ignore (aka "override")
> configuration variables that take effect only when we are using the
> merge-backend.  "git rebase --whitespace=fix --rebase-merges",
> giving both from the command line, is making conflicting and
> unsatisfyable request, and it is worth giving an error message.
> 
> But if you configure rebase.autosquash only because you want to it
> to be in effect for your invocations of "git rebase -i", you
> shouldn't have to override it from the command line when you say
> "git rebase" or "git rebase --whitespace=fix", should you?

>
> Phillip Wood <phillip.wood123@gmail.com> writes:
>
> >> @@ -1514,13 +1542,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> >>                              break;
> >>              if (i >= 0 || options.type == REBASE_APPLY) {
> >> -                    if (is_merge(&options))
> >> -                            die(_("apply options and merge options "
> >> -                                      "cannot be used together"));
> >
> > This isn't a new change but having thought about it I'm not sure
> > moving this code is a good idea. If the user runs
> >
> >       git -c rebase.updateRefs=true rebase --whitespace=fix --exec "make test"
> >
> > then instead of getting an message saying that they cannot use apply
> > and merge options together they'll get a message suggesting they pass
> > --no-update-refs which wont fix the problem for them.
>
> Hmph.  Instead of dying here, ...
>
> >> +                    if (options.autosquash == -1 && options.config_autosquash == 1)
> >>                              die(_("apply options are incompatible with rebase.autoSquash.  Consider adding --no-autosquash"));
> >> +                    else if (options.rebase_merges == -1 && options.config_rebase_merges == 1)
> >> +                            die(_("apply options are incompatible with rebase.rebaseMerges.  Consider adding --no-rebase-merges"));
> >>                      else if (options.update_refs == -1 && options.config_update_refs == 1)
> >>                              die(_("apply options are incompatible with rebase.updateRefs.  Consider adding --no-update-refs"));
>
> ... we get caught here, and the next one is not triggered.
>
> >> +                    else if (is_merge(&options))
> >> +                            die(_("apply options and merge options "
> >> +                                      "cannot be used together"));
> >>                      else
> >>                              options.type = REBASE_APPLY;
>
> What's the reason why "cannot be used together" is moved to the last
> in the chain?
>
> The first two new conditions in this chain try to catch an
> invocation with some apply-specific command line option
> (e.g. "--whitespace=fix") when used with configuration variables
> specific to the merge-backend (e.g. "rebase.merges") and suggest
> overriding the configuration from the command line, and I suspect
> that the motivation behind this change is that their error messages
> are more specific than the generic "apply and merge do not mix".

> > +     if (!strcmp(var, "rebase.rebasemerges")) {
> > +             opts->config_rebase_merges = git_parse_maybe_bool(value);
> > +             if (opts->config_rebase_merges < 0) {
> > +                     opts->config_rebase_merges = 1;
> > +                     parse_rebase_merges_value(opts, value);
> > +             } else
> > +                     opts->rebase_cousins = 0;
>
> The "else" clause should have braces because the "if" cause requires
> them (see Documentation/CodingGuidelines). I don't think it is worth
> re-rolling just for this though.

> This patch series introduces a rebase.rebaseMerges config option to
> accommodate users who would like --rebase-merges to be on by default and
> to facilitate turning on --rebase-merges by default without
> configuration in a future version of Git. It also cleans up and
> documents the behavior of the --rebase-merges command line option to
> avoid confusion about how the config option and the command line option
> interact.
> 
> Changes from v8:
> - Add braces around one-line else clause
> - Remove unnecessary change to error message priority

> Thanks to Phillip, Junio, Johannes and Sergey for your feedback on v8.
> 
> Alex Henrie (3):
>    rebase: add documentation and test for --no-rebase-merges
>    rebase: deprecate --rebase-merges=""
>    rebase: add a config option for --rebase-merges
> 
>   Documentation/config/rebase.txt        | 10 ++++
>   Documentation/git-rebase.txt           | 19 ++++---
>   builtin/rebase.c                       | 70 ++++++++++++++++++++------
>   t/t3422-rebase-incompatible-options.sh | 17 +++++++
>   t/t3430-rebase-merges.sh               | 44 ++++++++++++++++
>   5 files changed, 138 insertions(+), 22 deletions(-)
> 
> Range-diff against v8:
> 1:  09fb7c1b74 = 1:  a22b9d0da2 rebase: add documentation and test for --no-rebase-merges
> 2:  a846716a4a = 2:  112fee4833 rebase: deprecate --rebase-merges=""
> 3:  b12a3610ba ! 3:  868899cd6d rebase: add a config option for --rebase-merges
>      @@ builtin/rebase.c: static int rebase_config(const char *var, const char *value, v
>       +		if (opts->config_rebase_merges < 0) {
>       +			opts->config_rebase_merges = 1;
>       +			parse_rebase_merges_value(opts, value);
>      -+		} else
>      ++		} else {
>       +			opts->rebase_cousins = 0;
>      ++		}
>       +		return 0;
>       +	}
>       +
>      @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix
>        		if (ignore_whitespace)
>        			strvec_push(&options.git_am_opts,
>       @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
>      - 				break;
>      -
>      - 		if (i >= 0 || options.type == REBASE_APPLY) {
>      --			if (is_merge(&options))
>      --				die(_("apply options and merge options "
>      --					  "cannot be used together"));
>      --			else if (options.autosquash == -1 && options.config_autosquash == 1)
>      -+			if (options.autosquash == -1 && options.config_autosquash == 1)
>      + 					  "cannot be used together"));
>      + 			else if (options.autosquash == -1 && options.config_autosquash == 1)
>        				die(_("apply options are incompatible with rebase.autoSquash.  Consider adding --no-autosquash"));
>       +			else if (options.rebase_merges == -1 && options.config_rebase_merges == 1)
>       +				die(_("apply options are incompatible with rebase.rebaseMerges.  Consider adding --no-rebase-merges"));
>        			else if (options.update_refs == -1 && options.config_update_refs == 1)
>        				die(_("apply options are incompatible with rebase.updateRefs.  Consider adding --no-update-refs"));
>      -+			else if (is_merge(&options))
>      -+				die(_("apply options and merge options "
>      -+					  "cannot be used together"));
>        			else
>      - 				options.type = REBASE_APPLY;
>      - 		}
>       @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
>        	options.update_refs = (options.update_refs >= 0) ? options.update_refs :
>        			     ((options.config_update_refs >= 0) ? options.config_update_refs : 0);

> Hi Alex
>
> On 26/03/2023 04:06, Alex Henrie wrote:
>> This patch series introduces a rebase.rebaseMerges config option to
>> accommodate users who would like --rebase-merges to be on by default and
>> to facilitate turning on --rebase-merges by default without
>> configuration in a future version of Git. It also cleans up and
>> documents the behavior of the --rebase-merges command line option to
>> avoid confusion about how the config option and the command line option
>> interact.
>> Changes from v8:
>> - Add braces around one-line else clause
>> - Remove unnecessary change to error message priority
>
> The range-diff looks good to me. This iteration addresses all of my
> outstanding concerns.