diff mbox series

[v2] pull: abort if --ff-only is given and fast-forwarding is impossible

Message ID 20210715050559.3371470-1-alexhenrie24@gmail.com (mailing list archive)
State New, archived
Headers show
Series [v2] pull: abort if --ff-only is given and fast-forwarding is impossible | expand

Commit Message

Alex Henrie July 15, 2021, 5:05 a.m. UTC
The warning about pulling without specifying how to reconcile divergent
branches says that after setting pull.rebase to true, --ff-only can
still be passed on the command line to require a fast-forward. Make that
actually work.

Per the discussion on the mailing list, explicitly passing --rebase on
the command line countermands pull.ff=only in the configuration or a
previous --ff-only on the command line.

The --no-ff in `git pull --rebase --no-ff` is still ignored.

Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
---
This is the code I was working on when Elijah sent his patch set to
accomplish the same general goals. Mine is a less ambitious (and less
invasive) approach, but I don't care which solution wins.
---
 Documentation/git-pull.txt   |  2 +-
 advice.c                     |  5 +++++
 advice.h                     |  1 +
 builtin/merge.c              |  2 +-
 builtin/pull.c               | 23 +++++++++++++++++------
 t/t7601-merge-pull-config.sh | 35 +++++++++++++++++++++++++++++++++++
 6 files changed, 60 insertions(+), 8 deletions(-)

Comments

Felipe Contreras July 15, 2021, 9:55 a.m. UTC | #1
Alex Henrie wrote:
> The warning about pulling without specifying how to reconcile divergent
> branches says that after setting pull.rebase to true, --ff-only can
> still be passed on the command line to require a fast-forward. Make that
> actually work.
> 
> Per the discussion on the mailing list, explicitly passing --rebase on
> the command line countermands pull.ff=only in the configuration or a
> previous --ff-only on the command line.
> 
> The --no-ff in `git pull --rebase --no-ff` is still ignored.
> 
> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
> ---
> This is the code I was working on when Elijah sent his patch set to
> accomplish the same general goals. Mine is a less ambitious (and less
> invasive) approach, but I don't care which solution wins.

This is a better approach than Elijah's because it doesn't fail if both
pull.ff and pull.rebase are configured.

But there's still the problem that the new behavior cannot be
configured.
diff mbox series

Patch

diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index 5c3fb67c01..3cf957d2ed 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt
@@ -107,7 +107,7 @@  include::merge-options.txt[]
 	branch after fetching. If there is a remote-tracking branch
 	corresponding to the upstream branch and the upstream branch
 	was rebased since last fetched, the rebase uses that information
-	to avoid rebasing non-local changes.
+	to avoid rebasing non-local changes. Implies --ff.
 +
 When set to `merges`, rebase using `git rebase --rebase-merges` so that
 the local merge commits are included in the rebase (see
diff --git a/advice.c b/advice.c
index 0b9c89c48a..337e8f342b 100644
--- a/advice.c
+++ b/advice.c
@@ -286,6 +286,11 @@  void NORETURN die_conclude_merge(void)
 	die(_("Exiting because of unfinished merge."));
 }
 
+void NORETURN die_ff_impossible(void)
+{
+	die(_("Not possible to fast-forward, aborting."));
+}
+
 void advise_on_updating_sparse_paths(struct string_list *pathspec_list)
 {
 	struct string_list_item *item;
diff --git a/advice.h b/advice.h
index bd26c385d0..1624043838 100644
--- a/advice.h
+++ b/advice.h
@@ -95,6 +95,7 @@  void advise_if_enabled(enum advice_type type, const char *advice, ...);
 int error_resolve_conflict(const char *me);
 void NORETURN die_resolve_conflict(const char *me);
 void NORETURN die_conclude_merge(void);
+void NORETURN die_ff_impossible(void);
 void advise_on_updating_sparse_paths(struct string_list *pathspec_list);
 void detach_advice(const char *new_name);
 
diff --git a/builtin/merge.c b/builtin/merge.c
index a8a843b1f5..aa920ac524 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1620,7 +1620,7 @@  int cmd_merge(int argc, const char **argv, const char *prefix)
 	}
 
 	if (fast_forward == FF_ONLY)
-		die(_("Not possible to fast-forward, aborting."));
+		die_ff_impossible();
 
 	if (autostash)
 		create_autostash(the_repository,
diff --git a/builtin/pull.c b/builtin/pull.c
index 3e13f81084..256b96633e 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -55,12 +55,17 @@  static enum rebase_type parse_config_rebase(const char *key, const char *value,
 static int parse_opt_rebase(const struct option *opt, const char *arg, int unset)
 {
 	enum rebase_type *value = opt->value;
+	char **ff_value = (char **)opt->extra;
 
 	if (arg)
 		*value = parse_config_rebase("--rebase", arg, 0);
 	else
 		*value = unset ? REBASE_FALSE : REBASE_TRUE;
-	return *value == REBASE_INVALID ? -1 : 0;
+	if (*value == REBASE_INVALID)
+		return -1;
+	if (*value != REBASE_FALSE)
+		*ff_value = xstrdup("--ff");
+	return 0;
 }
 
 static const char * const pull_usage[] = {
@@ -125,10 +130,11 @@  static struct option pull_options[] = {
 
 	/* Options passed to git-merge or git-rebase */
 	OPT_GROUP(N_("Options related to merging")),
-	OPT_CALLBACK_F('r', "rebase", &opt_rebase,
+	{ OPTION_CALLBACK, 'r', "rebase", &opt_rebase,
 		"(false|true|merges|preserve|interactive)",
 		N_("incorporate changes by rebasing rather than merging"),
-		PARSE_OPT_OPTARG, parse_opt_rebase),
+		PARSE_OPT_OPTARG, parse_opt_rebase, REBASE_FALSE, NULL,
+		(intptr_t)&opt_ff },
 	OPT_PASSTHRU('n', NULL, &opt_diffstat, NULL,
 		N_("do not show a diffstat at the end of the merge"),
 		PARSE_OPT_NOARG | PARSE_OPT_NONEG),
@@ -1046,9 +1052,14 @@  int cmd_pull(int argc, const char **argv, const char *prefix)
 
 	can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]);
 
-	if (rebase_unspecified && !opt_ff && !can_ff) {
-		if (opt_verbosity >= 0)
-			show_advice_pull_non_ff();
+	if (!can_ff) {
+		if (opt_ff) {
+			if (!strcmp(opt_ff, "--ff-only"))
+				die_ff_impossible();
+		} else {
+			if (rebase_unspecified && opt_verbosity >= 0)
+				show_advice_pull_non_ff();
+		}
 	}
 
 	if (opt_rebase) {
diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
index 52e8ccc933..8922aa6f7c 100755
--- a/t/t7601-merge-pull-config.sh
+++ b/t/t7601-merge-pull-config.sh
@@ -183,6 +183,41 @@  test_expect_success 'pull prevents non-fast-forward with "only" in pull.ff' '
 	test_must_fail git pull . c3
 '
 
+test_expect_success 'pull prevents non-fast-forward with pull.ff=only and pull.rebase=true' '
+	git reset --hard c1 &&
+	test_config pull.ff only &&
+	test_config pull.rebase true &&
+	test_must_fail git pull . c3
+'
+
+test_expect_success 'pull prevents non-fast-forward with pull.ff=only and pull.rebase=false' '
+	git reset --hard c1 &&
+	test_config pull.ff only &&
+	test_config pull.rebase false &&
+	test_must_fail git pull . c3
+'
+
+test_expect_success 'pull prevents non-fast-forward with --rebase --ff-only' '
+	git reset --hard c1 &&
+	test_must_fail git pull --rebase --ff-only . c3
+'
+
+test_expect_success 'pull prevents non-fast-forward with --no-rebase --ff-only' '
+	git reset --hard c1 &&
+	test_must_fail git pull --no-rebase --ff-only . c3
+'
+
+test_expect_success 'pull allows non-fast-forward with --ff-only --rebase' '
+	git reset --hard c1 &&
+	git pull --ff-only --rebase . c3
+'
+
+test_expect_success 'pull allows non-fast-forward with pull.ff=only and --rebase' '
+	git reset --hard c1 &&
+	test_config pull.ff only &&
+	git pull --rebase . c3
+'
+
 test_expect_success 'merge c1 with c2 (ours in pull.twohead)' '
 	git reset --hard c1 &&
 	git config pull.twohead ours &&