Web lists-archives.com

Re: [PATCH 2/2] git-rebase: error out when incompatible options passed




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@xxxxxxxxx>
---
  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
  	"