Re: [PATCH v5 2/3] rebase: deprecate --rebase-merges=""

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

Hi Alex

On 25/02/2023 18:03, 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. Deprecate that syntax to avoid
> confusion when a rebase.merges config option is introduced, where
> rebase.merges="" will be equivalent to --no-rebase-merges.

I think deprecating this rather than making it an error is a good idea. 
I don't think we need the test though. The test suite is incredibly slow 
to run on windows (I'd heard people complain about it but it was not 
until I tried running it myself I realized just how diabolically slow it 
really is) and so it is important to balance adding tests to catch 
regression vs test run time. Tests that catch bugs in the rebase 
implementation are useful, ones like this just make everything slower 
which makes it harder to catch real regressions.

Best Wishes

Phillip

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