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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Mar 2, 2023, 09:37 UTC
Message-ID: <1021e6d0-0cd0-d92c-4cb3-45dbf2f6626e@dunelm.org.uk>
In-Reply-To: <20230225180325.796624-4-alexhenrie24@gmail.com>

Hi Alex

On 25/02/2023 18:03, Alex Henrie wrote:

Show 32 quoted lines

> 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`.

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.

 > [...]

Show 12 quoted lines

> @@ -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"));

This is good, thanks for adding it

 > [...]

Show 47 quoted lines

> 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
> +'

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.

Show 7 quoted lines

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

Best Wishes

Phillip

Show 19 quoted lines

> +'
> +
> +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 &&