Re: [PATCH v4 3/3] rebase: add a config option for --rebase-merges

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

Hi Alex

On 23/02/2023 05:34, Alex Henrie wrote:
> 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`.

Thanks for updating this, it is much clearer what the different settings 
mean now. I not sure if having the config setting override the default 
when the user passes --rebase-merges without an argument is a good idea.

> [...]   
> +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;
> +}

It's a shame we seem to have grown yet another callback since v2, I'm 
not sure we should need to add quite so much code just to support a new 
config option

 > [...]
> @@ -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");
> -	}

As I have said before I really think this patch needs to follow the lead 
of eddfcd8ece (rebase: provide better error message for apply options 
vs. merge config, 2023-01-25) and provide an error message if 
--rebase-merges is enabled by rebase.merges and the user provides a 
command line option that requires the apply backend. So

	git -c rebase.merges=true rebase --whitespace=fix

would result in

	error: apply options are incompatible with rebase.merges.
	Consider adding --no-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^ &&

I think this behavior is confusing for users and will break scripts that 
quite reasonably assume --rebase-merges is equivalent to 
--rebase-merges=no-rebase-cousins

> [...]
> +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
> +'

These two tests seem to be testing the config subsystem rather than this 
patch. As Dscho has pointed out it is important to get a balance between 
test coverage and test run time. I think these two tests can definitely 
be dropped.

Best Wishes

Phillip