From: Phillip Wood <phillip.wood123@gmail.com>
Date: Mon, 23 Jan 2023 20:08:50 GMT
Subject: Re: [PATCH v4 5/9] rebase: add coverage of other incompatible options
Message-ID: <759fb313-ce88-4eb7-96c0-4adeb75ca9f9@dunelm.org.uk>
In-Reply-To: <5e4851e611ee18112bd71939ee900e02a8d590c5.1674367961.git.gitgitgadget@gmail.com>

Hi Elijah

On 22/01/2023 06:12, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
> 
> The git-rebase manual noted several sets of incompatible options, but
> we were missing tests for a few of these.  Further, we were missing
> code checks for some of these, which could result in command line
> options being silently ignored.
> 
> Also, note that adding a check for autosquash means that using
> --whitespace=fix together with the config setting rebase.autosquash=true
> will trigger an error.  A subsequent commit will improve the error
> message.

Thanks for updating the commit message and for the new commits at the 
end of the series.

> Signed-off-by: Elijah Newren <newren@gmail.com>
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1224,6 +1224,26 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   		if (options.fork_point < 0)
>   			options.fork_point = 0;
>   	}
> +	/*
> +	 * The apply backend does not support --[no-]reapply-cherry-picks.
> +	 * The behavior it implements by default is equivalent to
> +	 * --no-reapply-cherry-picks (due to passing --cherry-picks to
> +	 * format-patch), but --keep-base alters the upstream such that no
> +	 * cherry-picks can be found (effectively making it act like
> +	 * --reapply-cherry-picks).
> +	 *
> +	 * Now, if the user does specify --[no-]reapply-cherry-picks, but
> +	 * does so in such a way that options.reapply_cherry_picks ==
> +	 * keep_base, then the behavior they get will match what they
> +	 * expect despite options.reapply_cherry_picks being ignored.  We
> +	 * could just allow the flag in that case, but it seems better to
> +	 * just alert the user that they've specified a flag that the
> +	 * backend ignores.
> +	 */

I'm a bit confused by this. --keep-base works with either 
--reapply-cherry-picks (which is the default if --keep-base is given) or 
--no-reapply-cherry-picks. Just below this hunk we have

	if (options.reapply_cherry_picks < 0)
		options.reapply_cherry_picks = keep_base;

So we only set options.reapply_cherry_picks to match keep_base if the 
user did not specify -[-no]-reapply-cherry-picks on the commandline.

Best Wishes

Phillip

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