From: Phillip Wood <phillip.wood@talktalk.net>
Date: Sun, 10 Jun 2018 19:40:24 GMT
Subject: Re: [PATCH 2/2] git-rebase: error out when incompatible options passed
Message-ID: <cec770a8-28ee-d546-8c27-905d2fcdb0c8@talktalk.net>
In-Reply-To: <20180607050654.19663-2-newren@gmail.com>

On 07/06/18 06:06, Elijah Newren wrote:
> git rebase has three different types: am, merge, and interactive, all of
> which are implemented in terms of separate scripts.  am builds on git-am,
> merge builds on git-merge-recursive, and interactive builds on
> git-cherry-pick.  We make use of features in those lower-level commands in
> the different rebase types, but those features don't exist in all of the
> lower level commands so we have a range of incompatibilities.  Previously,
> we just accepted nearly any argument and silently ignored whichever ones
> weren't implemented for the type of rebase specified.  Change this so the
> incompatibilities are documented, included in the testsuite, and tested
> for at runtime with an appropriate error message shown.

I think this is a great improvement, it has always bothered me that 
rebase silently ignored the am options when they're given with 
interactive ones.
> 
> Some exceptions I left out:
> 
>    * --merge and --interactive are technically incompatible since they are
>      supposed to run different underlying scripts, but with a few small
>      changes, --interactive can do everything that --merge can.  In fact,
>      I'll shortly be sending another patch to remove git-rebase--merge and
>      reimplement it on top of git-rebase--interactive.

Excellent I've wondered about doing that but never got round to it. One 
thing I was slightly concerned about was that someone maybe parsing the 
output of git-rebase--merge and they'll get a nasty shock when that 
output changes as a result of using the sequencer.

> 
>    * One could argue that --interactive and --quiet are incompatible since
>      --interactive doesn't implement a --quiet mode (perhaps since
>      cherry-pick itself does not implement one).  However, the interactive
>      mode is more quiet than the other modes in general with progress
>      messages, so one could argue that it's already quiet.
> 
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>   Documentation/git-rebase.txt           | 15 +++++++++++++--
>   git-rebase.sh                          | 17 +++++++++++++++++
>   t/t3422-rebase-incompatible-options.sh | 10 +++++-----
>   3 files changed, 35 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 0e20a66e73..451252c173 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -243,6 +243,10 @@ leave out at most one of A and B, in which case it defaults to HEAD.
>   --keep-empty::
>   	Keep the commits that do not change anything from its
>   	parents in the result.
> ++
> +This uses the `--interactive` machinery internally, and as such,
> +anything that is incompatible with --interactive is incompatible
> +with this option.
>   
>   --allow-empty-message::
>   	By default, rebasing commits with an empty message will fail.
> @@ -324,6 +328,8 @@ which makes little sense.
>   	and after each change.  When fewer lines of surrounding
>   	context exist they all must match.  By default no context is
>   	ever ignored.
> +	Incompatible with the --merge and --interactive options, or
> +	anything that implies those options or their machinery.

struct replay_opts has an allow_empty_message member so I'm not sure 
that's true.

>   -f::
>   --force-rebase::
> @@ -355,13 +361,15 @@ default is `--no-fork-point`, otherwise the default is `--fork-point`.
>   --whitespace=<option>::
>   	These flag are passed to the 'git apply' program
>   	(see linkgit:git-apply[1]) that applies the patch.
> -	Incompatible with the --interactive option.
> +	Incompatible with the --merge and --interactive options, or
> +	anything that implies those options or their machinery.

I wonder if it is better just to list the incompatible options it might 
be a bit long but it would be nicer for the user than them having to 
work out which options imply --interactive.

>   --committer-date-is-author-date::
>   --ignore-date::
>   	These flags are passed to 'git am' to easily change the dates
>   	of the rebased commits (see linkgit:git-am[1]).
> -	Incompatible with the --interactive option.
> +	Incompatible with the --merge and --interactive options, or
> +	anything that implies those options or their machinery.
>   
>   --signoff::
>   	Add a Signed-off-by: trailer to all the rebased commits. Note
> @@ -400,6 +408,9 @@ The `--rebase-merges` mode is similar in spirit to `--preserve-merges`, but
>   in contrast to that option works well in interactive rebases: commits can be
>   reordered, inserted and dropped at will.
>   +
> +This uses the `--interactive` machinery internally, but it can be run
> +without an explicit `--interactive`.
> ++

Without more context it's hard to judge but I'm not sure this adds 
anything useful

>   It is currently only possible to recreate the merge commits using the
>   `recursive` merge strategy; Different merge strategies can be used only via
>   explicit `exec git merge -s <strategy> [...]` commands.
> diff --git a/git-rebase.sh b/git-rebase.sh
> index 40be59ecc4..f1dbecba18 100755
> --- a/git-rebase.sh
> +++ b/git-rebase.sh
> @@ -503,6 +503,23 @@ then
>   	git_format_patch_opt="$git_format_patch_opt --progress"
>   fi
>   
> +if test -n "$git_am_opt"; then
> +	incompatible_opts=`echo "$git_am_opt" | sed -e 's/ -q//'`

I think the style guide recommends $() over ``

> +	if test -n "$interactive_rebase"
> +	then
> +		if test -n "$incompatible_opts"
> +		then
> +			die "$(gettext "error: cannot combine interactive options (--interactive, --exec, --rebase-merges, --preserve-merges, --keep-empty, --root + --onto) with am options ($incompatible_opts)")"
> +		fi
> +	fi
> +	if test -n "$do_merge"; then
> +		if test -n "$incompatible_opts"
> +		then
> +			die "$(gettext "error: cannot combine merge options (--merge, --strategy, --strategy-option) with am options ($incompatible_opts)")"
> +		fi
> +	fi
> +fi
> +
>   if test -n "$signoff"
>   then
>   	test -n "$preserve_merges" &&
> diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
> index 04cdae921b..66a83363bf 100755
> --- a/t/t3422-rebase-incompatible-options.sh
> +++ b/t/t3422-rebase-incompatible-options.sh
> @@ -34,27 +34,27 @@ test_expect_success 'setup' '
>   test_run_rebase () {
>   	opt=$1
>   	shift
> -	test_expect_failure "$opt incompatible with --merge" "
> +	test_expect_success "$opt incompatible with --merge" "
>   		git checkout B^0 &&
>   		test_must_fail git rebase $opt --merge A
>   	"
>   
> -	test_expect_failure "$opt incompatible with --strategy=ours" "
> +	test_expect_success "$opt incompatible with --strategy=ours" "
>   		git checkout B^0 &&
>   		test_must_fail git rebase $opt --strategy=ours A
>   	"
>   
> -	test_expect_failure "$opt incompatible with --strategy-option=ours" "
> +	test_expect_success "$opt incompatible with --strategy-option=ours" "
>   		git checkout B^0 &&
>   		test_must_fail git rebase $opt --strategy=ours A
>   	"
>   
> -	test_expect_failure "$opt incompatible with --interactive" "
> +	test_expect_success "$opt incompatible with --interactive" "
>   		git checkout B^0 &&
>   		test_must_fail git rebase $opt --interactive A
>   	"
>   
> -	test_expect_failure "$opt incompatible with --exec" "
> +	test_expect_success "$opt incompatible with --exec" "
>   		git checkout B^0 &&
>   		test_must_fail git rebase $opt --exec 'true' A
>   	"
>