Merge branch 'en/rebase-incompatible-opts'

"git rebase" often ignored incompatible options instead of
complaining, which has been corrected.

* en/rebase-incompatible-opts:
  rebase: provide better error message for apply options vs. merge config
  rebase: put rebase_options initialization in single place
  rebase: fix formatting of rebase --reapply-cherry-picks option in docs
  rebase: clarify the OPT_CMDMODE incompatibilities
  rebase: add coverage of other incompatible options
  rebase: fix incompatiblity checks for --[no-]reapply-cherry-picks
  rebase: fix docs about incompatibilities with --root
  rebase: remove --allow-empty-message from incompatible opts
  rebase: flag --apply and --merge as incompatible
  rebase: mark --update-refs as requiring the merge backend

3 files changed, 163 insertions, 64 deletions

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index d811c1cf44..9a295bcee4 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -208,6 +208,39 @@ Alternatively, you can undo the 'git rebase' with
 
     git rebase --abort
 
+MODE OPTIONS
+------------
+
+The options in this section cannot be used with any other option,
+including not with each other:
+
+--continue::
+	Restart the rebasing process after having resolved a merge conflict.
+
+--skip::
+	Restart the rebasing process by skipping the current patch.
+
+--abort::
+	Abort the rebase operation and reset HEAD to the original
+	branch. If `<branch>` was provided when the rebase operation was
+	started, then `HEAD` will be reset to `<branch>`. Otherwise `HEAD`
+	will be reset to where it was when the rebase operation was
+	started.
+
+--quit::
+	Abort the rebase operation but `HEAD` is not reset back to the
+	original branch. The index and working tree are also left
+	unchanged as a result. If a temporary stash entry was created
+	using `--autostash`, it will be saved to the stash list.
+
+--edit-todo::
+	Edit the todo list during an interactive rebase.
+
+--show-current-patch::
+	Show the current patch in an interactive rebase or when rebase
+	is stopped because of conflicts. This is the equivalent of
+	`git show REBASE_HEAD`.
+
 OPTIONS
 -------
 --onto <newbase>::
@@ -249,22 +282,6 @@ See also INCOMPATIBLE OPTIONS below.
 <branch>::
 	Working branch; defaults to `HEAD`.
 
---continue::
-	Restart the rebasing process after having resolved a merge conflict.
-
---abort::
-	Abort the rebase operation and reset HEAD to the original
-	branch. If `<branch>` was provided when the rebase operation was
-	started, then `HEAD` will be reset to `<branch>`. Otherwise `HEAD`
-	will be reset to where it was when the rebase operation was
-	started.
-
---quit::
-	Abort the rebase operation but `HEAD` is not reset back to the
-	original branch. The index and working tree are also left
-	unchanged as a result. If a temporary stash entry was created
-	using `--autostash`, it will be saved to the stash list.
-
 --apply::
 	Use applying strategies to rebase (calling `git-am`
 	internally).  This option may become a no-op in the future
@@ -321,7 +338,6 @@ See also INCOMPATIBLE OPTIONS below.
 	upstream changes, the behavior towards them is controlled by
 	the `--empty` flag.)
 +
-
 In the absence of `--keep-base` (or if `--no-reapply-cherry-picks` is
 given), these commits will be automatically dropped.  Because this
 necessitates reading all upstream commits, this can be expensive in
@@ -330,7 +346,6 @@ read. When using the 'merge' backend, warnings will be issued for each
 dropped commit (unless `--quiet` is given). Advice will also be issued
 unless `advice.skippedCherryPicks` is set to false (see
 linkgit:git-config[1]).
-
 +
 `--reapply-cherry-picks` allows rebase to forgo reading all upstream
 commits, potentially improving performance.
@@ -345,17 +360,6 @@ See also INCOMPATIBLE OPTIONS below.
 +
 See also INCOMPATIBLE OPTIONS below.
 
---skip::
-	Restart the rebasing process by skipping the current patch.
-
---edit-todo::
-	Edit the todo list during an interactive rebase.
-
---show-current-patch::
-	Show the current patch in an interactive rebase or when rebase
-	is stopped because of conflicts. This is the equivalent of
-	`git show REBASE_HEAD`.
-
 -m::
 --merge::
 	Using merging strategies to rebase (default).
@@ -574,10 +578,7 @@ See also INCOMPATIBLE OPTIONS below.
 --root::
 	Rebase all commits reachable from `<branch>`, instead of
 	limiting them with an `<upstream>`.  This allows you to rebase
-	the root commit(s) on a branch.  When used with `--onto`, it
-	will skip changes already contained in `<newbase>` (instead of
-	`<upstream>`) whereas without `--onto` it will operate on every
-	change.
+	the root commit(s) on a branch.
 +
 See also INCOMPATIBLE OPTIONS below.
 
@@ -630,6 +631,8 @@ start would be overridden by the presence of
 +
 If the configuration variable `rebase.updateRefs` is set, then this option
 can be used to override and disable this setting.
++
+See also INCOMPATIBLE OPTIONS below.
 
 INCOMPATIBLE OPTIONS
 --------------------
@@ -645,17 +648,15 @@ are incompatible with the following options:
  * --merge
  * --strategy
  * --strategy-option
- * --allow-empty-message
- * --[no-]autosquash
+ * --autosquash
  * --rebase-merges
  * --interactive
  * --exec
  * --no-keep-empty
  * --empty=
- * --reapply-cherry-picks
- * --edit-todo
+ * --[no-]reapply-cherry-picks when used without --keep-base
  * --update-refs
- * --root when used in combination with --onto
+ * --root when used without --onto
 
 In addition, the following pairs of options are incompatible:
 
diff --git a/builtin/rebase.c b/builtin/rebase.c
index a26cc0cfdb..7171be40ee 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -122,6 +122,8 @@ struct rebase_options {
 	int reapply_cherry_picks;
 	int fork_point;
 	int update_refs;
+	int config_autosquash;
+	int config_update_refs;
 };
 
 #define REBASE_OPTIONS_INIT {			  	\
@@ -134,6 +136,12 @@ struct rebase_options {
 		.exec = STRING_LIST_INIT_NODUP,		\
 		.git_format_patch_opt = STRBUF_INIT,	\
 		.fork_point = -1,			\
+		.reapply_cherry_picks = -1,             \
+		.allow_empty_message = 1,               \
+		.autosquash = -1,                       \
+		.config_autosquash = -1,                \
+		.update_refs = -1,                      \
+		.config_update_refs = -1,               \
 	}
 
 static struct replay_opts get_replay_opts(const struct rebase_options *opts)
@@ -776,7 +784,7 @@ static int rebase_config(const char *var, const char *value, void *data)
 	}
 
 	if (!strcmp(var, "rebase.autosquash")) {
-		opts->autosquash = git_config_bool(var, value);
+		opts->config_autosquash = git_config_bool(var, value);
 		return 0;
 	}
 
@@ -793,7 +801,7 @@ static int rebase_config(const char *var, const char *value, void *data)
 	}
 
 	if (!strcmp(var, "rebase.updaterefs")) {
-		opts->update_refs = git_config_bool(var, value);
+		opts->config_update_refs = git_config_bool(var, value);
 		return 0;
 	}
 
@@ -907,6 +915,9 @@ static int parse_opt_am(const struct option *opt, const char *arg, int unset)
 	BUG_ON_OPT_NEG(unset);
 	BUG_ON_OPT_ARG(arg);
 
+	if (opts->type != REBASE_UNSPECIFIED && opts->type != REBASE_APPLY)
+	    die(_("apply options and merge options cannot be used together"));
+
 	opts->type = REBASE_APPLY;
 
 	return 0;
@@ -920,8 +931,10 @@ static int parse_opt_merge(const struct option *opt, const char *arg, int unset)
 	BUG_ON_OPT_NEG(unset);
 	BUG_ON_OPT_ARG(arg);
 
-	if (!is_merge(opts))
-		opts->type = REBASE_MERGE;
+	if (opts->type != REBASE_UNSPECIFIED && opts->type != REBASE_MERGE)
+	    die(_("apply options and merge options cannot be used together"));
+
+	opts->type = REBASE_MERGE;
 
 	return 0;
 }
@@ -935,6 +948,9 @@ static int parse_opt_interactive(const struct option *opt, const char *arg,
 	BUG_ON_OPT_NEG(unset);
 	BUG_ON_OPT_ARG(arg);
 
+	if (opts->type != REBASE_UNSPECIFIED && opts->type != REBASE_MERGE)
+	    die(_("apply options and merge options cannot be used together"));
+
 	opts->type = REBASE_MERGE;
 	opts->flags |= REBASE_INTERACTIVE_EXPLICIT;
 
@@ -1150,8 +1166,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	prepare_repo_settings(the_repository);
 	the_repository->settings.command_requires_full_index = 0;
 
-	options.reapply_cherry_picks = -1;
-	options.allow_empty_message = 1;
 	git_config(rebase_config, &options);
 	/* options.gpg_sign_opt will be either "-S" or NULL */
 	gpg_sign = options.gpg_sign_opt ? "" : NULL;
@@ -1216,13 +1230,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		if (options.fork_point < 0)
 			options.fork_point = 0;
 	}
-	/*
-	 * --keep-base defaults to --reapply-cherry-picks to avoid losing
-	 * commits when using this option.
-	 */
-	if (options.reapply_cherry_picks < 0)
-		options.reapply_cherry_picks = keep_base;
-
 	if (options.root && options.fork_point > 0)
 		die(_("options '%s' and '%s' cannot be used together"), "--root", "--fork-point");
 
@@ -1365,7 +1372,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	if ((options.flags & REBASE_INTERACTIVE_EXPLICIT) ||
 	    (options.action != ACTION_NONE) ||
 	    (options.exec.nr > 0) ||
-	    options.autosquash) {
+	    (options.autosquash == -1 && options.config_autosquash == 1) ||
+	    options.autosquash == 1) {
 		allow_preemptive_ff = 0;
 	}
 	if (options.committer_date_is_author_date || options.ignore_date)
@@ -1398,12 +1406,27 @@ 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 (options.reapply_cherry_picks < 0)
+		/*
+		 * We default to --no-reapply-cherry-picks unless
+		 * --keep-base is given; when --keep-base is given, we want
+		 * to default to --reapply-cherry-picks.
+		 */
+		options.reapply_cherry_picks = keep_base;
+	else if (!keep_base)
+		/*
+		 * The apply backend always searches for and drops cherry
+		 * picks.  This is often not wanted with --keep-base, so
+		 * --keep-base allows --reapply-cherry-picks to be
+		 * simulated by altering the upstream such that
+		 * cherry-picks cannot be detected and thus all commits are
+		 * reapplied.  Thus, --[no-]reapply-cherry-picks is
+		 * supported when --keep-base is specified, but not when
+		 * --keep-base is left out.
+		 */
+		imply_merge(&options, options.reapply_cherry_picks ?
+					  "--reapply-cherry-picks" :
+					  "--no-reapply-cherry-picks");
 
 	if (gpg_sign)
 		options.gpg_sign_opt = xstrfmt("-S%s", gpg_sign);
@@ -1483,15 +1506,29 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			if (strcmp(options.git_am_opts.v[i], "-q"))
 				break;
 
-		if (i >= 0) {
+		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)
+				die(_("apply options are incompatible with rebase.autosquash.  Consider adding --no-autosquash"));
+			else if (options.update_refs == -1 && options.config_update_refs == 1)
+				die(_("apply options are incompatible with rebase.updateRefs.  Consider adding --no-update-refs"));
 			else
 				options.type = REBASE_APPLY;
 		}
 	}
 
+	if (options.update_refs == 1)
+		imply_merge(&options, "--update-refs");
+	options.update_refs = (options.update_refs >= 0) ? options.update_refs :
+			     ((options.config_update_refs >= 0) ? options.config_update_refs : 0);
+
+	if (options.autosquash == 1)
+		imply_merge(&options, "--autosquash");
+	options.autosquash = (options.autosquash >= 0) ? options.autosquash :
+			     ((options.config_autosquash >= 0) ? options.config_autosquash : 0);
+
 	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 6dabb05a2a..4711b37a28 100755
--- a/t/t3422-rebase-incompatible-options.sh
+++ b/t/t3422-rebase-incompatible-options.sh
@@ -25,11 +25,11 @@ test_expect_success 'setup' '
 '
 
 #
-# Rebase has lots of useful options like --whitepsace=fix, which are
-# actually all built in terms of flags to git-am.  Since neither
-# --merge nor --interactive (nor any options that imply those two) use
-# git-am, using them together will result in flags like --whitespace=fix
-# being ignored.  Make sure rebase warns the user and aborts instead.
+# Rebase has a couple options which are specific to the apply backend,
+# and several options which are specific to the merge backend.  Flags
+# from the different sets cannot work together, and we do not want to
+# just ignore one of the sets of flags.  Make sure rebase warns the
+# user and aborts instead.
 #
 
 test_rebase_am_only () {
@@ -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,9 +65,65 @@ 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
+	"
+
+	test_expect_success "$opt incompatible with --root without --onto" "
+		git checkout B^0 &&
+		test_must_fail git rebase $opt --root A
+	"
+
+	test_expect_success "$opt incompatible with rebase.autosquash" "
+		git checkout B^0 &&
+		test_must_fail git -c rebase.autosquash=true rebase $opt A 2>err &&
+		grep -e --no-autosquash err
+	"
+
+	test_expect_success "$opt incompatible with rebase.updateRefs" "
+		git checkout B^0 &&
+		test_must_fail git -c rebase.updateRefs=true rebase $opt A 2>err &&
+		grep -e --no-update-refs err
+	"
+
+	test_expect_success "$opt okay with overridden rebase.autosquash" "
+		test_when_finished \"git reset --hard B^0\" &&
+		git checkout B^0 &&
+		git -c rebase.autosquash=true rebase --no-autosquash $opt A
+	"
+
+	test_expect_success "$opt okay with overridden rebase.updateRefs" "
+		test_when_finished \"git reset --hard B^0\" &&
+		git checkout B^0 &&
+		git -c rebase.updateRefs=true rebase --no-update-refs $opt A
+	"
 }
 
+# Check options which imply --apply
 test_rebase_am_only --whitespace=fix
 test_rebase_am_only -C4
+# Also check an explicit --apply
+test_rebase_am_only --apply
 
 test_done