[PATCH] pull: allow branch.<name>.rebase to override pull.ff=only

["D. Ben Knoble" <ben.knoble+github@gmail.com>]

When running "git pull" with the following configuration options, we
fail to merge divergent branches:

- pull.ff=only
- pull.rebase (unset)
- branch.<current_branch>.rebase=true

Yet it seems that the user intended to make rebase the default for the
current branch while using --ff-only for non-rebase pulls. Since this
case appears uncovered by existing tests, changing the behavior here
might be safe: it makes what was an error into a successful rebase.

Add a test for the behavior and make it pass: this requires knowing from
where the rebase was requested. Previous commits (e4dc25ed49 (pull:
since --ff-only overrides, handle it first, 2021-07-22), adc27d6a93
(pull: make --rebase and --no-rebase override pull.ff=only, 2021-07-22))
took care to differentiate that --rebase overrides pull.ff=only, but
don't distinguish which config setting requests the rebase. Split
config_get_rebase into 2 parts so that we know where the rebase comes
from, since we only want to allow branch-config to override pull.ff=only
(like --rebase does); pull.rebase should still be overridden by
pull.ff=only or --ff-only.

Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
---
Notes:
    - I also looked at ea1954af77 (pull: should be noop when already-up-to-date,
      2021-11-17) when trying to understand how some options override others,
      but it didnt' seem germane to the final version.
    - I think I've got the right test script, since it's the one that started
      failing before I added the "else" branch to the new code (which also
      confirms that it's necessary to preserve current behavior); the only new
      behavior should be the one mentioned by the new test.
    - A possible #leftoverbits: it would be good to document more clearly the
      interplay of --ff[-only], --rebase, pull.ff, pull.rebase, and
      branch.<name>.rebase, particularly when they override each other.
      Confusingly, branch.<name>.merge has nothing to do with whether pull will
      merge or rebase ;) lest you think I'd forgotten something that _looks_
      parallel to pull.rebase.

 builtin/pull.c               | 39 ++++++++++++++++++++++++++++--------
 t/t7601-merge-pull-config.sh |  8 ++++++++
 2 files changed, 39 insertions(+), 8 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 9c4a00620a..c30f233dcc 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -326,13 +326,13 @@ static const char *config_get_ff(void)
 }
 
 /**
- * Returns the default configured value for --rebase. It first looks for the
+ * Returns the default configured value for --rebase. It looks for the
  * value of "branch.$curr_branch.rebase", where $curr_branch is the current
  * branch, and if HEAD is detached or the configuration key does not exist,
- * looks for the value of "pull.rebase". If both configuration keys do not
- * exist, returns REBASE_FALSE.
+ * considers the result unspecified. Follow up by checking
+ * config_get_rebase_pull.
  */
-static enum rebase_type config_get_rebase(int *rebase_unspecified)
+static enum rebase_type config_get_rebase_branch(int *rebase_unspecified)
 {
 	struct branch *curr_branch = branch_get("HEAD");
 	const char *value;
@@ -349,11 +349,22 @@ static enum rebase_type config_get_rebase(int *rebase_unspecified)
 		free(key);
 	}
 
+	*rebase_unspecified = 1;
+	return REBASE_INVALID;
+}
+
+/*
+ * Looks for the value of "pull.rebase". If it does not exist, returns
+ * REBASE_FALSE.
+ */
+static enum rebase_type config_get_rebase_pull(int *rebase_unspecified)
+{
+	const char *value;
+
 	if (!git_config_get_value("pull.rebase", &value))
 		return parse_config_rebase("pull.rebase", value, 1);
 
 	*rebase_unspecified = 1;
-
 	return REBASE_FALSE;
 }
 
@@ -1026,7 +1037,7 @@ int cmd_pull(int argc,
 		 * are relying on the next if-condition happening before
 		 * the config_get_rebase() call so that an explicit
 		 * "--rebase" can override a config setting of
-		 * pull.ff=only.
+		 * pull.ff=only. [continued…]
 		 */
 		if (opt_rebase >= 0 && opt_ff && !strcmp(opt_ff, "--ff-only")) {
 			free(opt_ff);
@@ -1034,8 +1045,20 @@ int cmd_pull(int argc,
 		}
 	}
 
-	if (opt_rebase < 0)
-		opt_rebase = config_get_rebase(&rebase_unspecified);
+	if (opt_rebase < 0) {
+		/*
+		 * […continued] But, if the config requests rebase *for this
+		 * branch*, override --ff-only, which otherwise takes precedence
+		 * over pull.rebase=true.
+		 */
+		opt_rebase = config_get_rebase_branch(&rebase_unspecified);
+		if (opt_rebase >= 0 && opt_ff && !strcmp(opt_ff, "--ff-only")) {
+			free(opt_ff);
+			opt_ff = xstrdup("--ff");
+		} else {
+		    opt_rebase = config_get_rebase_pull(&rebase_unspecified);
+		}
+	}
 
 	if (repo_read_index_unmerged(the_repository))
 		die_resolve_conflict("pull");
diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
index 199a1d5db3..fd99f46aad 100755
--- a/t/t7601-merge-pull-config.sh
+++ b/t/t7601-merge-pull-config.sh
@@ -113,6 +113,14 @@
 	test_grep ! "You have divergent branches" err
 '
 
+test_expect_success 'pull.rebase not set and pull.ff=only and branch.<name>.rebase=true (not-fast-forward)' '
+	git reset --hard c2 &&
+	test_config pull.ff only &&
+	git switch -c bc2 &&
+	test_config branch.bc2.rebase true &&
+	git pull . c1
+'
+
 test_expect_success 'pull.rebase not set and --rebase given (not-fast-forward)' '
 	git reset --hard c2 &&
 	git pull --rebase . c1 2>err &&
-- 
2.47.0