[RFC 1/2] pull: warn that pulling will not merge by default in Git 3.0

[RFC 1/2] pull: warn that pulling will not merge by default in Git 3.0

[Alex Henrie]

Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
---
 builtin/pull.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 17aa63cd35..4d80efe5b7 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -345,13 +345,14 @@ static enum rebase_type config_get_rebase(void)
 		return parse_config_rebase("pull.rebase", value, 1);
 
 	if (opt_verbosity >= 0 && !opt_ff) {
-		warning(_("Pulling without specifying how to reconcile divergent branches is\n"
-			"discouraged. You can squelch this message by running one of the following\n"
-			"commands sometime before your next pull:\n"
+		warning(_("Starting in Git 3.0, the default behavior of `git pull` will change\n"
+			"when it is not specified how to reconcile divergent branches. You can\n"
+			"squelch this message by running one of the following commands sometime\n"
+			"before your next pull:\n"
 			"\n"
-			"  git config pull.rebase false  # merge (the default strategy)\n"
+			"  git config pull.rebase false  # merge (the current default)\n"
 			"  git config pull.rebase true   # rebase\n"
-			"  git config pull.ff only       # fast-forward only\n"
+			"  git config pull.ff only       # fast-forward only (the future default)\n"
 			"\n"
 			"You can replace \"git config\" with \"git config --global\" to set a default\n"
 			"preference for all repositories. You can also pass --rebase, --no-rebase,\n"
-- 
2.29.2

[RFC 2/2] pull: default pull.ff to "only" when pull.rebase is not set either

[Alex Henrie]

Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
---
 Documentation/merge-options.txt | 14 +++++---
 builtin/pull.c                  | 41 +++++++++---------------
 t/t5520-pull.sh                 | 16 ++++-----
 t/t5521-pull-options.sh         |  4 +--
 t/t5524-pull-msg.sh             |  4 +--
 t/t5553-set-upstream.sh         | 20 ++++++------
 t/t5604-clone-reference.sh      |  4 +--
 t/t6402-merge-rename.sh         | 20 ++++++------
 t/t6409-merge-subtree.sh        |  6 ++--
 t/t6417-merge-ours-theirs.sh    | 10 +++---
 t/t7600-merge.sh                |  2 +-
 t/t7601-merge-pull-config.sh    | 57 ---------------------------------
 t/t7603-merge-reduce-heads.sh   |  2 +-
 13 files changed, 70 insertions(+), 130 deletions(-)

diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index eb0aabd396..1eb79e2939 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -43,10 +43,16 @@ set to `no` at the beginning of them.
 --no-ff::
 --ff-only::
 	Specifies how a merge is handled when the merged-in history is
-	already a descendant of the current history.  `--ff` is the
-	default unless merging an annotated (and possibly signed) tag
-	that is not stored in its natural place in the `refs/tags/`
-	hierarchy, in which case `--no-ff` is assumed.
+	already a descendant of the current history.
+	+
+	In `git pull`, `--ff-only` is the default unless `--rebase` or
+	`--no-rebase` is specified, in which case the default is `--ff`.
+	However, the default changes again to `--no-ff` in the unlikely
+	scenario where an annotated (and possibly signed) tag that is not
+	stored in its natural place in the `refs/tags/` hierarchy is being
+	merged.
+	+
+	In `git merge`, `--ff` is always the default.
 +
 With `--ff`, when possible resolve the merge as a fast-forward (only
 update the branch pointer to match the merged branch; do not create a
diff --git a/builtin/pull.c b/builtin/pull.c
index 4d80efe5b7..c703560a77 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -292,17 +292,21 @@ static void set_reflog_message(int argc, const char **argv)
 }
 
 /**
- * If pull.ff is unset, returns NULL. If pull.ff is "true", returns "--ff". If
- * pull.ff is "false", returns "--no-ff". If pull.ff is "only", returns
- * "--ff-only". Otherwise, if pull.ff is set to an invalid value, die with an
- * error.
+ * If pull.ff is unset, returns NULL if pull.rebase is set or "--ff-only" if
+ * pull.rebase is not set. If pull.ff is "true", returns "--ff". If pull.ff is
+ * "false", returns "--no-ff". If pull.ff is "only", returns "--ff-only".
+ * Otherwise, if pull.ff is set to an invalid value, die with an error.
  */
 static const char *config_get_ff(void)
 {
 	const char *value;
 
-	if (git_config_get_value("pull.ff", &value))
-		return NULL;
+	if (git_config_get_value("pull.ff", &value)) {
+		if (opt_rebase < 0)
+			return "--ff-only";
+		else
+			return NULL;
+	}
 
 	switch (git_parse_maybe_bool(value)) {
 	case 0:
@@ -322,7 +326,7 @@ static const char *config_get_ff(void)
  * 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.
+ * exist, returns REBASE_INVALID.
  */
 static enum rebase_type config_get_rebase(void)
 {
@@ -344,23 +348,7 @@ static enum rebase_type config_get_rebase(void)
 	if (!git_config_get_value("pull.rebase", &value))
 		return parse_config_rebase("pull.rebase", value, 1);
 
-	if (opt_verbosity >= 0 && !opt_ff) {
-		warning(_("Starting in Git 3.0, the default behavior of `git pull` will change\n"
-			"when it is not specified how to reconcile divergent branches. You can\n"
-			"squelch this message by running one of the following commands sometime\n"
-			"before your next pull:\n"
-			"\n"
-			"  git config pull.rebase false  # merge (the current default)\n"
-			"  git config pull.rebase true   # rebase\n"
-			"  git config pull.ff only       # fast-forward only (the future default)\n"
-			"\n"
-			"You can replace \"git config\" with \"git config --global\" to set a default\n"
-			"preference for all repositories. You can also pass --rebase, --no-rebase,\n"
-			"or --ff-only on the command line to override the configured default per\n"
-			"invocation.\n"));
-	}
-
-	return REBASE_FALSE;
+	return REBASE_INVALID;
 }
 
 /**
@@ -931,11 +919,14 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 
 	parse_repo_refspecs(argc, argv, &repo, &refspecs);
 
+	if (opt_rebase < 0)
+		opt_rebase = config_get_rebase();
+
 	if (!opt_ff)
 		opt_ff = xstrdup_or_null(config_get_ff());
 
 	if (opt_rebase < 0)
-		opt_rebase = config_get_rebase();
+		opt_rebase = REBASE_FALSE;
 
 	if (read_cache_unmerged())
 		die_resolve_conflict("pull");
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 9fae07cdfa..856eccd706 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -15,7 +15,7 @@ test_pull_autostash () {
 	git reset --hard before-rebase &&
 	echo dirty >new_file &&
 	git add new_file &&
-	git pull "$@" . copy &&
+	git pull --ff "$@" . copy &&
 	test_cmp_rev HEAD^"$expect_parent_num" copy &&
 	echo dirty >expect &&
 	test_cmp expect new_file &&
@@ -27,7 +27,7 @@ test_pull_autostash_fail () {
 	git reset --hard before-rebase &&
 	echo dirty >new_file &&
 	git add new_file &&
-	test_must_fail git pull "$@" . copy 2>err &&
+	test_must_fail git pull --ff "$@" . copy 2>err &&
 	test_i18ngrep -E "uncommitted changes.|overwritten by merge:" err
 }
 
@@ -210,7 +210,7 @@ test_expect_success 'fail if upstream branch does not exist' '
 	test_config branch.test.merge refs/heads/nonexisting &&
 	echo file >expect &&
 	test_cmp expect file &&
-	test_must_fail git pull 2>err &&
+	test_must_fail git pull --no-rebase 2>err &&
 	test_i18ngrep "no such ref was fetched" err &&
 	test_cmp expect file
 '
@@ -223,17 +223,17 @@ test_expect_success 'fail if the index has unresolved entries' '
 	test_commit modified2 file &&
 	git ls-files -u >unmerged &&
 	test_must_be_empty unmerged &&
-	test_must_fail git pull . second &&
+	test_must_fail git pull --no-rebase . second &&
 	git ls-files -u >unmerged &&
 	test_file_not_empty unmerged &&
 	cp file expected &&
-	test_must_fail git pull . second 2>err &&
+	test_must_fail git pull --no-rebase . second 2>err &&
 	test_i18ngrep "Pulling is not possible because you have unmerged files." err &&
 	test_cmp expected file &&
 	git add file &&
 	git ls-files -u >unmerged &&
 	test_must_be_empty unmerged &&
-	test_must_fail git pull . second 2>err &&
+	test_must_fail git pull --no-rebase . second 2>err &&
 	test_i18ngrep "You have not concluded your merge" err &&
 	test_cmp expected file
 '
@@ -243,7 +243,7 @@ test_expect_success 'fast-forwards working tree if branch head is updated' '
 	test_when_finished "git checkout -f copy && git branch -D third" &&
 	echo file >expect &&
 	test_cmp expect file &&
-	git pull . second:third 2>err &&
+	git pull --no-rebase . second:third 2>err &&
 	test_i18ngrep "fetch updated the current branch head" err &&
 	echo modified >expect &&
 	test_cmp expect file &&
@@ -256,7 +256,7 @@ test_expect_success 'fast-forward fails with conflicting work tree' '
 	echo file >expect &&
 	test_cmp expect file &&
 	echo conflict >file &&
-	test_must_fail git pull . second:third 2>err &&
+	test_must_fail git pull --no-rebase . second:third 2>err &&
 	test_i18ngrep "Cannot fast-forward your working tree" err &&
 	echo conflict >expect &&
 	test_cmp expect file &&
diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
index db1a381cd9..24801857ec 100755
--- a/t/t5521-pull-options.sh
+++ b/t/t5521-pull-options.sh
@@ -175,8 +175,8 @@ test_expect_success 'git pull --allow-unrelated-histories' '
 	) &&
 	(
 		cd dst &&
-		test_must_fail git pull ../src side &&
-		git pull --allow-unrelated-histories ../src side
+		test_must_fail git pull --no-rebase ../src side &&
+		git pull --no-rebase --allow-unrelated-histories ../src side
 	)
 '
 
diff --git a/t/t5524-pull-msg.sh b/t/t5524-pull-msg.sh
index c278adaa5a..b2be3605f5 100755
--- a/t/t5524-pull-msg.sh
+++ b/t/t5524-pull-msg.sh
@@ -28,7 +28,7 @@ test_expect_success setup '
 test_expect_success pull '
 (
 	cd cloned &&
-	git pull --log &&
+	git pull --no-rebase --log &&
 	git log -2 &&
 	git cat-file commit HEAD >result &&
 	grep Dollar result
@@ -41,7 +41,7 @@ test_expect_success '--log=1 limits shortlog length' '
 	git reset --hard HEAD^ &&
 	test "$(cat afile)" = original &&
 	test "$(cat bfile)" = added &&
-	git pull --log=1 &&
+	git pull --no-rebase --log=1 &&
 	git log -3 &&
 	git cat-file commit HEAD >result &&
 	grep Dollar result &&
diff --git a/t/t5553-set-upstream.sh b/t/t5553-set-upstream.sh
index 7622981cbf..53b9add9f6 100755
--- a/t/t5553-set-upstream.sh
+++ b/t/t5553-set-upstream.sh
@@ -105,34 +105,34 @@ test_expect_success 'setup commit on master and other pull' '
 
 test_expect_success 'pull --set-upstream upstream master sets branch master but not other' '
 	clear_config master other &&
-	git pull --set-upstream upstream master &&
+	git pull --no-rebase --set-upstream upstream master &&
 	check_config master upstream refs/heads/master &&
 	check_config_missing other
 '
 
 test_expect_success 'pull --set-upstream master:other2 does not set the branch other2' '
 	clear_config other2 &&
-	git pull --set-upstream upstream master:other2 &&
+	git pull --no-rebase --set-upstream upstream master:other2 &&
 	check_config_missing other2
 '
 
 test_expect_success 'pull --set-upstream upstream other sets branch master' '
 	clear_config master other &&
-	git pull --set-upstream upstream other &&
+	git pull --no-rebase --set-upstream upstream other &&
 	check_config master upstream refs/heads/other &&
 	check_config_missing other
 '
 
 test_expect_success 'pull --set-upstream upstream tag does not set the tag' '
 	clear_config three &&
-	git pull --tags --set-upstream upstream three &&
+	git pull --no-rebase --tags --set-upstream upstream three &&
 	check_config_missing three
 '
 
 test_expect_success 'pull --set-upstream http://nosuchdomain.example.com fails with invalid url' '
 	# master explicitly not cleared, we check that it is not touched from previous value
 	clear_config other other2 three &&
-	test_must_fail git pull --set-upstream http://nosuchdomain.example.com &&
+	test_must_fail git pull --no-rebase --set-upstream http://nosuchdomain.example.com &&
 	check_config master upstream refs/heads/other &&
 	check_config_missing other &&
 	check_config_missing other2 &&
@@ -141,16 +141,16 @@ test_expect_success 'pull --set-upstream http://nosuchdomain.example.com fails w
 
 test_expect_success 'pull --set-upstream upstream HEAD sets branch HEAD' '
 	clear_config master other &&
-	git pull --set-upstream upstream HEAD &&
+	git pull --no-rebase --set-upstream upstream HEAD &&
 	check_config master upstream HEAD &&
 	git checkout other &&
-	git pull --set-upstream upstream HEAD &&
+	git pull --no-rebase --set-upstream upstream HEAD &&
 	check_config other upstream HEAD
 '
 
 test_expect_success 'pull --set-upstream upstream with more than one branch does nothing' '
 	clear_config master three &&
-	git pull --set-upstream upstream master three &&
+	git pull --no-rebase --set-upstream upstream master three &&
 	check_config_missing master &&
 	check_config_missing three
 '
@@ -159,7 +159,7 @@ test_expect_success 'pull --set-upstream with valid URL sets upstream to URL' '
 	clear_config master other other2 &&
 	git checkout master &&
 	url="file://$PWD" &&
-	git pull --set-upstream "$url" &&
+	git pull --no-rebase --set-upstream "$url" &&
 	check_config master "$url" HEAD &&
 	check_config_missing other &&
 	check_config_missing other2
@@ -169,7 +169,7 @@ test_expect_success 'pull --set-upstream with valid URL and branch sets branch'
 	clear_config master other other2 &&
 	git checkout master &&
 	url="file://$PWD" &&
-	git pull --set-upstream "$url" master &&
+	git pull --no-rebase --set-upstream "$url" master &&
 	check_config master "$url" refs/heads/master &&
 	check_config_missing other &&
 	check_config_missing other2
diff --git a/t/t5604-clone-reference.sh b/t/t5604-clone-reference.sh
index 2f7be23044..b9e11eceec 100755
--- a/t/t5604-clone-reference.sh
+++ b/t/t5604-clone-reference.sh
@@ -84,7 +84,7 @@ test_expect_success 'updating origin' '
 '
 
 test_expect_success 'pulling changes from origin' '
-	git -C C pull origin
+	git -C C pull --no-rebase origin
 '
 
 # the 2 local objects are commit and tree from the merge
@@ -93,7 +93,7 @@ test_expect_success 'that alternate to origin gets used' '
 '
 
 test_expect_success 'pulling changes from origin' '
-	git -C D pull origin
+	git -C D pull --no-rebase origin
 '
 
 # the 5 local objects are expected; file3 blob, commit in A to add it
diff --git a/t/t6402-merge-rename.sh b/t/t6402-merge-rename.sh
index 3f64f62224..ad140a8def 100755
--- a/t/t6402-merge-rename.sh
+++ b/t/t6402-merge-rename.sh
@@ -100,7 +100,7 @@ test_expect_success 'setup' '
 test_expect_success 'pull renaming branch into unrenaming one' \
 '
 	git show-branch &&
-	test_expect_code 1 git pull . white &&
+	test_expect_code 1 git pull --no-rebase . white &&
 	git ls-files -s &&
 	git ls-files -u B >b.stages &&
 	test_line_count = 3 b.stages &&
@@ -118,7 +118,7 @@ test_expect_success 'pull renaming branch into another renaming one' \
 	rm -f B &&
 	git reset --hard &&
 	git checkout red &&
-	test_expect_code 1 git pull . white &&
+	test_expect_code 1 git pull --no-rebase . white &&
 	git ls-files -u B >b.stages &&
 	test_line_count = 3 b.stages &&
 	git ls-files -s N >n.stages &&
@@ -134,7 +134,7 @@ test_expect_success 'pull unrenaming branch into renaming one' \
 '
 	git reset --hard &&
 	git show-branch &&
-	test_expect_code 1 git pull . master &&
+	test_expect_code 1 git pull --no-rebase . master &&
 	git ls-files -u B >b.stages &&
 	test_line_count = 3 b.stages &&
 	git ls-files -s N >n.stages &&
@@ -150,7 +150,7 @@ test_expect_success 'pull conflicting renames' \
 '
 	git reset --hard &&
 	git show-branch &&
-	test_expect_code 1 git pull . blue &&
+	test_expect_code 1 git pull --no-rebase . blue &&
 	git ls-files -u A >a.stages &&
 	test_line_count = 1 a.stages &&
 	git ls-files -u B >b.stages &&
@@ -170,7 +170,7 @@ test_expect_success 'interference with untracked working tree file' '
 	git reset --hard &&
 	git show-branch &&
 	echo >A this file should not matter &&
-	test_expect_code 1 git pull . white &&
+	test_expect_code 1 git pull --no-rebase . white &&
 	test_path_is_file A
 '
 
@@ -180,7 +180,7 @@ test_expect_success 'interference with untracked working tree file' '
 	git show-branch &&
 	rm -f A &&
 	echo >A this file should not matter &&
-	test_expect_code 1 git pull . red &&
+	test_expect_code 1 git pull --no-rebase . red &&
 	test_path_is_file A
 '
 
@@ -190,7 +190,7 @@ test_expect_success 'interference with untracked working tree file' '
 	git checkout -f master &&
 	git tag -f anchor &&
 	git show-branch &&
-	git pull . yellow &&
+	git pull --no-rebase . yellow &&
 	test_path_is_missing M &&
 	git reset --hard anchor
 '
@@ -203,7 +203,7 @@ test_expect_success 'updated working tree file should prevent the merge' '
 	git show-branch &&
 	echo >>M one line addition &&
 	cat M >M.saved &&
-	test_expect_code 128 git pull . yellow &&
+	test_expect_code 128 git pull --no-rebase . yellow &&
 	test_cmp M M.saved &&
 	rm -f M.saved
 '
@@ -217,7 +217,7 @@ test_expect_success 'updated working tree file should prevent the merge' '
 	echo >>M one line addition &&
 	cat M >M.saved &&
 	git update-index M &&
-	test_expect_code 128 git pull . yellow &&
+	test_expect_code 128 git pull --no-rebase . yellow &&
 	test_cmp M M.saved &&
 	rm -f M.saved
 '
@@ -229,7 +229,7 @@ test_expect_success 'interference with untracked working tree file' '
 	git tag -f anchor &&
 	git show-branch &&
 	echo >M this file should not matter &&
-	git pull . master &&
+	git pull --no-rebase . master &&
 	test_path_is_file M &&
 	! {
 		git ls-files -s |
diff --git a/t/t6409-merge-subtree.sh b/t/t6409-merge-subtree.sh
index b8e8b6f642..b79cb0a368 100755
--- a/t/t6409-merge-subtree.sh
+++ b/t/t6409-merge-subtree.sh
@@ -97,7 +97,7 @@ test_expect_success 'merge update' '
 	git checkout -b topic_2 &&
 	git commit -m "update git-gui" &&
 	cd ../git &&
-	git pull -s subtree gui topic_2 &&
+	git pull --no-rebase -s subtree gui topic_2 &&
 	git ls-files -s >actual &&
 	(
 		echo "100644 $o3 0	git-gui/git-gui.sh" &&
@@ -126,7 +126,7 @@ test_expect_success 'initial ambiguous subtree' '
 test_expect_success 'merge using explicit' '
 	cd ../git &&
 	git reset --hard topic_2 &&
-	git pull -Xsubtree=git-gui gui topic_2 &&
+	git pull --no-rebase -Xsubtree=git-gui gui topic_2 &&
 	git ls-files -s >actual &&
 	(
 		echo "100644 $o3 0	git-gui/git-gui.sh" &&
@@ -139,7 +139,7 @@ test_expect_success 'merge using explicit' '
 test_expect_success 'merge2 using explicit' '
 	cd ../git &&
 	git reset --hard topic_2 &&
-	git pull -Xsubtree=git-gui2 gui topic_2 &&
+	git pull --no-rebase -Xsubtree=git-gui2 gui topic_2 &&
 	git ls-files -s >actual &&
 	(
 		echo "100644 $o1 0	git-gui/git-gui.sh" &&
diff --git a/t/t6417-merge-ours-theirs.sh b/t/t6417-merge-ours-theirs.sh
index 0aebc6c028..ba9b100562 100755
--- a/t/t6417-merge-ours-theirs.sh
+++ b/t/t6417-merge-ours-theirs.sh
@@ -66,11 +66,11 @@ test_expect_success 'binary file with -Xours/-Xtheirs' '
 '
 
 test_expect_success 'pull passes -X to underlying merge' '
-	git reset --hard master && git pull -s recursive -Xours . side &&
-	git reset --hard master && git pull -s recursive -X ours . side &&
-	git reset --hard master && git pull -s recursive -Xtheirs . side &&
-	git reset --hard master && git pull -s recursive -X theirs . side &&
-	git reset --hard master && test_must_fail git pull -s recursive -X bork . side
+	git reset --hard master && git pull --no-rebase -s recursive -Xours . side &&
+	git reset --hard master && git pull --no-rebase -s recursive -X ours . side &&
+	git reset --hard master && git pull --no-rebase -s recursive -Xtheirs . side &&
+	git reset --hard master && git pull --no-rebase -s recursive -X theirs . side &&
+	git reset --hard master && test_must_fail git pull --no-rebase -s recursive -X bork . side
 '
 
 test_expect_success SYMLINKS 'symlink with -Xours/-Xtheirs' '
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 1c85f75555..3ad47f0925 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -891,7 +891,7 @@ test_expect_success 'merge annotated/signed tag w/o tracking' '
 		# tag from the "upstream", this pull defaults to --no-ff
 		cd dst &&
 		git pull .. c0 &&
-		git pull .. anno1 &&
+		git pull --no-rebase .. anno1 &&
 		git rev-parse HEAD^2 >actual &&
 		test_cmp expect actual
 	)
diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
index c5c4ea5fc0..c6c44ec570 100755
--- a/t/t7601-merge-pull-config.sh
+++ b/t/t7601-merge-pull-config.sh
@@ -27,63 +27,6 @@ test_expect_success 'setup' '
 	git tag c3
 '
 
-test_expect_success 'pull.rebase not set' '
-	git reset --hard c0 &&
-	git pull . c1 2>err &&
-	test_i18ngrep "Pulling without specifying how to reconcile" err
-'
-
-test_expect_success 'pull.rebase not set and pull.ff=true' '
-	git reset --hard c0 &&
-	test_config pull.ff true &&
-	git pull . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
-'
-
-test_expect_success 'pull.rebase not set and pull.ff=false' '
-	git reset --hard c0 &&
-	test_config pull.ff false &&
-	git pull . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
-'
-
-test_expect_success 'pull.rebase not set and pull.ff=only' '
-	git reset --hard c0 &&
-	test_config pull.ff only &&
-	git pull . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
-'
-
-test_expect_success 'pull.rebase not set and --rebase given' '
-	git reset --hard c0 &&
-	git pull --rebase . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
-'
-
-test_expect_success 'pull.rebase not set and --no-rebase given' '
-	git reset --hard c0 &&
-	git pull --no-rebase . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
-'
-
-test_expect_success 'pull.rebase not set and --ff given' '
-	git reset --hard c0 &&
-	git pull --ff . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
-'
-
-test_expect_success 'pull.rebase not set and --no-ff given' '
-	git reset --hard c0 &&
-	git pull --no-ff . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
-'
-
-test_expect_success 'pull.rebase not set and --ff-only given' '
-	git reset --hard c0 &&
-	git pull --ff-only . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
-'
-
 test_expect_success 'merge c1 with c2' '
 	git reset --hard c1 &&
 	test -f c0.c &&
diff --git a/t/t7603-merge-reduce-heads.sh b/t/t7603-merge-reduce-heads.sh
index 98948955ae..27cd94ad6f 100755
--- a/t/t7603-merge-reduce-heads.sh
+++ b/t/t7603-merge-reduce-heads.sh
@@ -68,7 +68,7 @@ test_expect_success 'merge c1 with c2, c3, c4, c5' '
 
 test_expect_success 'pull c2, c3, c4, c5 into c1' '
 	git reset --hard c1 &&
-	git pull . c2 c3 c4 c5 &&
+	git pull --no-rebase . c2 c3 c4 c5 &&
 	test "$(git rev-parse c1)" != "$(git rev-parse HEAD)" &&
 	test "$(git rev-parse c1)" = "$(git rev-parse HEAD^1)" &&
 	test "$(git rev-parse c2)" = "$(git rev-parse HEAD^2)" &&
-- 
2.29.2

Re: [RFC 2/2] pull: default pull.ff to "only" when pull.rebase is not set either

[Felipe Contreras]

On Tue, Nov 24, 2020 at 8:14 PM Alex Henrie <alexhenrie24@gmail.com> wrote:

Before making this the default we need a solution *right now* that is
a sane default.

> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
> ---
>  Documentation/merge-options.txt | 14 +++++---
>  builtin/pull.c                  | 41 +++++++++---------------
>  t/t5520-pull.sh                 | 16 ++++-----
>  t/t5521-pull-options.sh         |  4 +--
>  t/t5524-pull-msg.sh             |  4 +--
>  t/t5553-set-upstream.sh         | 20 ++++++------
>  t/t5604-clone-reference.sh      |  4 +--
>  t/t6402-merge-rename.sh         | 20 ++++++------
>  t/t6409-merge-subtree.sh        |  6 ++--
>  t/t6417-merge-ours-theirs.sh    | 10 +++---
>  t/t7600-merge.sh                |  2 +-
>  t/t7601-merge-pull-config.sh    | 57 ---------------------------------
>  t/t7603-merge-reduce-heads.sh   |  2 +-
>  13 files changed, 70 insertions(+), 130 deletions(-)
>
> diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
> index eb0aabd396..1eb79e2939 100644
> --- a/Documentation/merge-options.txt
> +++ b/Documentation/merge-options.txt
> @@ -43,10 +43,16 @@ set to `no` at the beginning of them.
>  --no-ff::
>  --ff-only::
>         Specifies how a merge is handled when the merged-in history is
> -       already a descendant of the current history.  `--ff` is the
> -       default unless merging an annotated (and possibly signed) tag
> -       that is not stored in its natural place in the `refs/tags/`
> -       hierarchy, in which case `--no-ff` is assumed.
> +       already a descendant of the current history.
> +       +
> +       In `git pull`, `--ff-only` is the default unless `--rebase` or
> +       `--no-rebase` is specified, in which case the default is `--ff`.
> +       However, the default changes again to `--no-ff` in the unlikely
> +       scenario where an annotated (and possibly signed) tag that is not
> +       stored in its natural place in the `refs/tags/` hierarchy is being
> +       merged.
> +       +
> +       In `git merge`, `--ff` is always the default.
>  +
>  With `--ff`, when possible resolve the merge as a fast-forward (only
>  update the branch pointer to match the merged branch; do not create a
> diff --git a/builtin/pull.c b/builtin/pull.c
> index 4d80efe5b7..c703560a77 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -292,17 +292,21 @@ static void set_reflog_message(int argc, const char **argv)
>  }
>
>  /**
> - * If pull.ff is unset, returns NULL. If pull.ff is "true", returns "--ff". If
> - * pull.ff is "false", returns "--no-ff". If pull.ff is "only", returns
> - * "--ff-only". Otherwise, if pull.ff is set to an invalid value, die with an
> - * error.
> + * If pull.ff is unset, returns NULL if pull.rebase is set or "--ff-only" if
> + * pull.rebase is not set. If pull.ff is "true", returns "--ff". If pull.ff is
> + * "false", returns "--no-ff". If pull.ff is "only", returns "--ff-only".
> + * Otherwise, if pull.ff is set to an invalid value, die with an error.
>   */
>  static const char *config_get_ff(void)
>  {
>         const char *value;
>
> -       if (git_config_get_value("pull.ff", &value))
> -               return NULL;
> +       if (git_config_get_value("pull.ff", &value)) {
> +               if (opt_rebase < 0)
> +                       return "--ff-only";
> +               else
> +                       return NULL;
> +       }
>
>         switch (git_parse_maybe_bool(value)) {
>         case 0:
> @@ -322,7 +326,7 @@ static const char *config_get_ff(void)
>   * 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.
> + * exist, returns REBASE_INVALID.
>   */
>  static enum rebase_type config_get_rebase(void)
>  {
> @@ -344,23 +348,7 @@ static enum rebase_type config_get_rebase(void)
>         if (!git_config_get_value("pull.rebase", &value))
>                 return parse_config_rebase("pull.rebase", value, 1);
>
> -       if (opt_verbosity >= 0 && !opt_ff) {
> -               warning(_("Starting in Git 3.0, the default behavior of `git pull` will change\n"
> -                       "when it is not specified how to reconcile divergent branches. You can\n"
> -                       "squelch this message by running one of the following commands sometime\n"
> -                       "before your next pull:\n"
> -                       "\n"
> -                       "  git config pull.rebase false  # merge (the current default)\n"
> -                       "  git config pull.rebase true   # rebase\n"
> -                       "  git config pull.ff only       # fast-forward only (the future default)\n"
> -                       "\n"
> -                       "You can replace \"git config\" with \"git config --global\" to set a default\n"
> -                       "preference for all repositories. You can also pass --rebase, --no-rebase,\n"
> -                       "or --ff-only on the command line to override the configured default per\n"
> -                       "invocation.\n"));
> -       }
> -
> -       return REBASE_FALSE;
> +       return REBASE_INVALID;
>  }
>
>  /**
> @@ -931,11 +919,14 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>
>         parse_repo_refspecs(argc, argv, &repo, &refspecs);
>
> +       if (opt_rebase < 0)
> +               opt_rebase = config_get_rebase();
> +
>         if (!opt_ff)
>                 opt_ff = xstrdup_or_null(config_get_ff());
>
>         if (opt_rebase < 0)
> -               opt_rebase = config_get_rebase();
> +               opt_rebase = REBASE_FALSE;
>
>         if (read_cache_unmerged())
>                 die_resolve_conflict("pull");
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> index 9fae07cdfa..856eccd706 100755
> --- a/t/t5520-pull.sh
> +++ b/t/t5520-pull.sh
> @@ -15,7 +15,7 @@ test_pull_autostash () {
>         git reset --hard before-rebase &&
>         echo dirty >new_file &&
>         git add new_file &&
> -       git pull "$@" . copy &&
> +       git pull --ff "$@" . copy &&
>         test_cmp_rev HEAD^"$expect_parent_num" copy &&
>         echo dirty >expect &&
>         test_cmp expect new_file &&
> @@ -27,7 +27,7 @@ test_pull_autostash_fail () {
>         git reset --hard before-rebase &&
>         echo dirty >new_file &&
>         git add new_file &&
> -       test_must_fail git pull "$@" . copy 2>err &&
> +       test_must_fail git pull --ff "$@" . copy 2>err &&
>         test_i18ngrep -E "uncommitted changes.|overwritten by merge:" err
>  }
>
> @@ -210,7 +210,7 @@ test_expect_success 'fail if upstream branch does not exist' '
>         test_config branch.test.merge refs/heads/nonexisting &&
>         echo file >expect &&
>         test_cmp expect file &&
> -       test_must_fail git pull 2>err &&
> +       test_must_fail git pull --no-rebase 2>err &&
>         test_i18ngrep "no such ref was fetched" err &&
>         test_cmp expect file
>  '
> @@ -223,17 +223,17 @@ test_expect_success 'fail if the index has unresolved entries' '
>         test_commit modified2 file &&
>         git ls-files -u >unmerged &&
>         test_must_be_empty unmerged &&
> -       test_must_fail git pull . second &&
> +       test_must_fail git pull --no-rebase . second &&
>         git ls-files -u >unmerged &&
>         test_file_not_empty unmerged &&
>         cp file expected &&
> -       test_must_fail git pull . second 2>err &&
> +       test_must_fail git pull --no-rebase . second 2>err &&
>         test_i18ngrep "Pulling is not possible because you have unmerged files." err &&
>         test_cmp expected file &&
>         git add file &&
>         git ls-files -u >unmerged &&
>         test_must_be_empty unmerged &&
> -       test_must_fail git pull . second 2>err &&
> +       test_must_fail git pull --no-rebase . second 2>err &&
>         test_i18ngrep "You have not concluded your merge" err &&
>         test_cmp expected file
>  '
> @@ -243,7 +243,7 @@ test_expect_success 'fast-forwards working tree if branch head is updated' '
>         test_when_finished "git checkout -f copy && git branch -D third" &&
>         echo file >expect &&
>         test_cmp expect file &&
> -       git pull . second:third 2>err &&
> +       git pull --no-rebase . second:third 2>err &&
>         test_i18ngrep "fetch updated the current branch head" err &&
>         echo modified >expect &&
>         test_cmp expect file &&
> @@ -256,7 +256,7 @@ test_expect_success 'fast-forward fails with conflicting work tree' '
>         echo file >expect &&
>         test_cmp expect file &&
>         echo conflict >file &&
> -       test_must_fail git pull . second:third 2>err &&
> +       test_must_fail git pull --no-rebase . second:third 2>err &&
>         test_i18ngrep "Cannot fast-forward your working tree" err &&
>         echo conflict >expect &&
>         test_cmp expect file &&
> diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
> index db1a381cd9..24801857ec 100755
> --- a/t/t5521-pull-options.sh
> +++ b/t/t5521-pull-options.sh
> @@ -175,8 +175,8 @@ test_expect_success 'git pull --allow-unrelated-histories' '
>         ) &&
>         (
>                 cd dst &&
> -               test_must_fail git pull ../src side &&
> -               git pull --allow-unrelated-histories ../src side
> +               test_must_fail git pull --no-rebase ../src side &&
> +               git pull --no-rebase --allow-unrelated-histories ../src side
>         )
>  '
>
> diff --git a/t/t5524-pull-msg.sh b/t/t5524-pull-msg.sh
> index c278adaa5a..b2be3605f5 100755
> --- a/t/t5524-pull-msg.sh
> +++ b/t/t5524-pull-msg.sh
> @@ -28,7 +28,7 @@ test_expect_success setup '
>  test_expect_success pull '
>  (
>         cd cloned &&
> -       git pull --log &&
> +       git pull --no-rebase --log &&
>         git log -2 &&
>         git cat-file commit HEAD >result &&
>         grep Dollar result
> @@ -41,7 +41,7 @@ test_expect_success '--log=1 limits shortlog length' '
>         git reset --hard HEAD^ &&
>         test "$(cat afile)" = original &&
>         test "$(cat bfile)" = added &&
> -       git pull --log=1 &&
> +       git pull --no-rebase --log=1 &&
>         git log -3 &&
>         git cat-file commit HEAD >result &&
>         grep Dollar result &&
> diff --git a/t/t5553-set-upstream.sh b/t/t5553-set-upstream.sh
> index 7622981cbf..53b9add9f6 100755
> --- a/t/t5553-set-upstream.sh
> +++ b/t/t5553-set-upstream.sh
> @@ -105,34 +105,34 @@ test_expect_success 'setup commit on master and other pull' '
>
>  test_expect_success 'pull --set-upstream upstream master sets branch master but not other' '
>         clear_config master other &&
> -       git pull --set-upstream upstream master &&
> +       git pull --no-rebase --set-upstream upstream master &&
>         check_config master upstream refs/heads/master &&
>         check_config_missing other
>  '
>
>  test_expect_success 'pull --set-upstream master:other2 does not set the branch other2' '
>         clear_config other2 &&
> -       git pull --set-upstream upstream master:other2 &&
> +       git pull --no-rebase --set-upstream upstream master:other2 &&
>         check_config_missing other2
>  '
>
>  test_expect_success 'pull --set-upstream upstream other sets branch master' '
>         clear_config master other &&
> -       git pull --set-upstream upstream other &&
> +       git pull --no-rebase --set-upstream upstream other &&
>         check_config master upstream refs/heads/other &&
>         check_config_missing other
>  '
>
>  test_expect_success 'pull --set-upstream upstream tag does not set the tag' '
>         clear_config three &&
> -       git pull --tags --set-upstream upstream three &&
> +       git pull --no-rebase --tags --set-upstream upstream three &&
>         check_config_missing three
>  '
>
>  test_expect_success 'pull --set-upstream http://nosuchdomain.example.com fails with invalid url' '
>         # master explicitly not cleared, we check that it is not touched from previous value
>         clear_config other other2 three &&
> -       test_must_fail git pull --set-upstream http://nosuchdomain.example.com &&
> +       test_must_fail git pull --no-rebase --set-upstream http://nosuchdomain.example.com &&
>         check_config master upstream refs/heads/other &&
>         check_config_missing other &&
>         check_config_missing other2 &&
> @@ -141,16 +141,16 @@ test_expect_success 'pull --set-upstream http://nosuchdomain.example.com fails w
>
>  test_expect_success 'pull --set-upstream upstream HEAD sets branch HEAD' '
>         clear_config master other &&
> -       git pull --set-upstream upstream HEAD &&
> +       git pull --no-rebase --set-upstream upstream HEAD &&
>         check_config master upstream HEAD &&
>         git checkout other &&
> -       git pull --set-upstream upstream HEAD &&
> +       git pull --no-rebase --set-upstream upstream HEAD &&
>         check_config other upstream HEAD
>  '
>
>  test_expect_success 'pull --set-upstream upstream with more than one branch does nothing' '
>         clear_config master three &&
> -       git pull --set-upstream upstream master three &&
> +       git pull --no-rebase --set-upstream upstream master three &&
>         check_config_missing master &&
>         check_config_missing three
>  '
> @@ -159,7 +159,7 @@ test_expect_success 'pull --set-upstream with valid URL sets upstream to URL' '
>         clear_config master other other2 &&
>         git checkout master &&
>         url="file://$PWD" &&
> -       git pull --set-upstream "$url" &&
> +       git pull --no-rebase --set-upstream "$url" &&
>         check_config master "$url" HEAD &&
>         check_config_missing other &&
>         check_config_missing other2
> @@ -169,7 +169,7 @@ test_expect_success 'pull --set-upstream with valid URL and branch sets branch'
>         clear_config master other other2 &&
>         git checkout master &&
>         url="file://$PWD" &&
> -       git pull --set-upstream "$url" master &&
> +       git pull --no-rebase --set-upstream "$url" master &&
>         check_config master "$url" refs/heads/master &&
>         check_config_missing other &&
>         check_config_missing other2
> diff --git a/t/t5604-clone-reference.sh b/t/t5604-clone-reference.sh
> index 2f7be23044..b9e11eceec 100755
> --- a/t/t5604-clone-reference.sh
> +++ b/t/t5604-clone-reference.sh
> @@ -84,7 +84,7 @@ test_expect_success 'updating origin' '
>  '
>
>  test_expect_success 'pulling changes from origin' '
> -       git -C C pull origin
> +       git -C C pull --no-rebase origin
>  '
>
>  # the 2 local objects are commit and tree from the merge
> @@ -93,7 +93,7 @@ test_expect_success 'that alternate to origin gets used' '
>  '
>
>  test_expect_success 'pulling changes from origin' '
> -       git -C D pull origin
> +       git -C D pull --no-rebase origin
>  '
>
>  # the 5 local objects are expected; file3 blob, commit in A to add it
> diff --git a/t/t6402-merge-rename.sh b/t/t6402-merge-rename.sh
> index 3f64f62224..ad140a8def 100755
> --- a/t/t6402-merge-rename.sh
> +++ b/t/t6402-merge-rename.sh
> @@ -100,7 +100,7 @@ test_expect_success 'setup' '
>  test_expect_success 'pull renaming branch into unrenaming one' \
>  '
>         git show-branch &&
> -       test_expect_code 1 git pull . white &&
> +       test_expect_code 1 git pull --no-rebase . white &&
>         git ls-files -s &&
>         git ls-files -u B >b.stages &&
>         test_line_count = 3 b.stages &&
> @@ -118,7 +118,7 @@ test_expect_success 'pull renaming branch into another renaming one' \
>         rm -f B &&
>         git reset --hard &&
>         git checkout red &&
> -       test_expect_code 1 git pull . white &&
> +       test_expect_code 1 git pull --no-rebase . white &&
>         git ls-files -u B >b.stages &&
>         test_line_count = 3 b.stages &&
>         git ls-files -s N >n.stages &&
> @@ -134,7 +134,7 @@ test_expect_success 'pull unrenaming branch into renaming one' \
>  '
>         git reset --hard &&
>         git show-branch &&
> -       test_expect_code 1 git pull . master &&
> +       test_expect_code 1 git pull --no-rebase . master &&
>         git ls-files -u B >b.stages &&
>         test_line_count = 3 b.stages &&
>         git ls-files -s N >n.stages &&
> @@ -150,7 +150,7 @@ test_expect_success 'pull conflicting renames' \
>  '
>         git reset --hard &&
>         git show-branch &&
> -       test_expect_code 1 git pull . blue &&
> +       test_expect_code 1 git pull --no-rebase . blue &&
>         git ls-files -u A >a.stages &&
>         test_line_count = 1 a.stages &&
>         git ls-files -u B >b.stages &&
> @@ -170,7 +170,7 @@ test_expect_success 'interference with untracked working tree file' '
>         git reset --hard &&
>         git show-branch &&
>         echo >A this file should not matter &&
> -       test_expect_code 1 git pull . white &&
> +       test_expect_code 1 git pull --no-rebase . white &&
>         test_path_is_file A
>  '
>
> @@ -180,7 +180,7 @@ test_expect_success 'interference with untracked working tree file' '
>         git show-branch &&
>         rm -f A &&
>         echo >A this file should not matter &&
> -       test_expect_code 1 git pull . red &&
> +       test_expect_code 1 git pull --no-rebase . red &&
>         test_path_is_file A
>  '
>
> @@ -190,7 +190,7 @@ test_expect_success 'interference with untracked working tree file' '
>         git checkout -f master &&
>         git tag -f anchor &&
>         git show-branch &&
> -       git pull . yellow &&
> +       git pull --no-rebase . yellow &&
>         test_path_is_missing M &&
>         git reset --hard anchor
>  '
> @@ -203,7 +203,7 @@ test_expect_success 'updated working tree file should prevent the merge' '
>         git show-branch &&
>         echo >>M one line addition &&
>         cat M >M.saved &&
> -       test_expect_code 128 git pull . yellow &&
> +       test_expect_code 128 git pull --no-rebase . yellow &&
>         test_cmp M M.saved &&
>         rm -f M.saved
>  '
> @@ -217,7 +217,7 @@ test_expect_success 'updated working tree file should prevent the merge' '
>         echo >>M one line addition &&
>         cat M >M.saved &&
>         git update-index M &&
> -       test_expect_code 128 git pull . yellow &&
> +       test_expect_code 128 git pull --no-rebase . yellow &&
>         test_cmp M M.saved &&
>         rm -f M.saved
>  '
> @@ -229,7 +229,7 @@ test_expect_success 'interference with untracked working tree file' '
>         git tag -f anchor &&
>         git show-branch &&
>         echo >M this file should not matter &&
> -       git pull . master &&
> +       git pull --no-rebase . master &&
>         test_path_is_file M &&
>         ! {
>                 git ls-files -s |
> diff --git a/t/t6409-merge-subtree.sh b/t/t6409-merge-subtree.sh
> index b8e8b6f642..b79cb0a368 100755
> --- a/t/t6409-merge-subtree.sh
> +++ b/t/t6409-merge-subtree.sh
> @@ -97,7 +97,7 @@ test_expect_success 'merge update' '
>         git checkout -b topic_2 &&
>         git commit -m "update git-gui" &&
>         cd ../git &&
> -       git pull -s subtree gui topic_2 &&
> +       git pull --no-rebase -s subtree gui topic_2 &&
>         git ls-files -s >actual &&
>         (
>                 echo "100644 $o3 0      git-gui/git-gui.sh" &&
> @@ -126,7 +126,7 @@ test_expect_success 'initial ambiguous subtree' '
>  test_expect_success 'merge using explicit' '
>         cd ../git &&
>         git reset --hard topic_2 &&
> -       git pull -Xsubtree=git-gui gui topic_2 &&
> +       git pull --no-rebase -Xsubtree=git-gui gui topic_2 &&
>         git ls-files -s >actual &&
>         (
>                 echo "100644 $o3 0      git-gui/git-gui.sh" &&
> @@ -139,7 +139,7 @@ test_expect_success 'merge using explicit' '
>  test_expect_success 'merge2 using explicit' '
>         cd ../git &&
>         git reset --hard topic_2 &&
> -       git pull -Xsubtree=git-gui2 gui topic_2 &&
> +       git pull --no-rebase -Xsubtree=git-gui2 gui topic_2 &&
>         git ls-files -s >actual &&
>         (
>                 echo "100644 $o1 0      git-gui/git-gui.sh" &&
> diff --git a/t/t6417-merge-ours-theirs.sh b/t/t6417-merge-ours-theirs.sh
> index 0aebc6c028..ba9b100562 100755
> --- a/t/t6417-merge-ours-theirs.sh
> +++ b/t/t6417-merge-ours-theirs.sh
> @@ -66,11 +66,11 @@ test_expect_success 'binary file with -Xours/-Xtheirs' '
>  '
>
>  test_expect_success 'pull passes -X to underlying merge' '
> -       git reset --hard master && git pull -s recursive -Xours . side &&
> -       git reset --hard master && git pull -s recursive -X ours . side &&
> -       git reset --hard master && git pull -s recursive -Xtheirs . side &&
> -       git reset --hard master && git pull -s recursive -X theirs . side &&
> -       git reset --hard master && test_must_fail git pull -s recursive -X bork . side
> +       git reset --hard master && git pull --no-rebase -s recursive -Xours . side &&
> +       git reset --hard master && git pull --no-rebase -s recursive -X ours . side &&
> +       git reset --hard master && git pull --no-rebase -s recursive -Xtheirs . side &&
> +       git reset --hard master && git pull --no-rebase -s recursive -X theirs . side &&
> +       git reset --hard master && test_must_fail git pull --no-rebase -s recursive -X bork . side
>  '
>
>  test_expect_success SYMLINKS 'symlink with -Xours/-Xtheirs' '
> diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
> index 1c85f75555..3ad47f0925 100755
> --- a/t/t7600-merge.sh
> +++ b/t/t7600-merge.sh
> @@ -891,7 +891,7 @@ test_expect_success 'merge annotated/signed tag w/o tracking' '
>                 # tag from the "upstream", this pull defaults to --no-ff
>                 cd dst &&
>                 git pull .. c0 &&
> -               git pull .. anno1 &&
> +               git pull --no-rebase .. anno1 &&
>                 git rev-parse HEAD^2 >actual &&
>                 test_cmp expect actual
>         )
> diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
> index c5c4ea5fc0..c6c44ec570 100755
> --- a/t/t7601-merge-pull-config.sh
> +++ b/t/t7601-merge-pull-config.sh
> @@ -27,63 +27,6 @@ test_expect_success 'setup' '
>         git tag c3
>  '
>
> -test_expect_success 'pull.rebase not set' '
> -       git reset --hard c0 &&
> -       git pull . c1 2>err &&
> -       test_i18ngrep "Pulling without specifying how to reconcile" err
> -'
> -
> -test_expect_success 'pull.rebase not set and pull.ff=true' '
> -       git reset --hard c0 &&
> -       test_config pull.ff true &&
> -       git pull . c1 2>err &&
> -       test_i18ngrep ! "Pulling without specifying how to reconcile" err
> -'
> -
> -test_expect_success 'pull.rebase not set and pull.ff=false' '
> -       git reset --hard c0 &&
> -       test_config pull.ff false &&
> -       git pull . c1 2>err &&
> -       test_i18ngrep ! "Pulling without specifying how to reconcile" err
> -'
> -
> -test_expect_success 'pull.rebase not set and pull.ff=only' '
> -       git reset --hard c0 &&
> -       test_config pull.ff only &&
> -       git pull . c1 2>err &&
> -       test_i18ngrep ! "Pulling without specifying how to reconcile" err
> -'
> -
> -test_expect_success 'pull.rebase not set and --rebase given' '
> -       git reset --hard c0 &&
> -       git pull --rebase . c1 2>err &&
> -       test_i18ngrep ! "Pulling without specifying how to reconcile" err
> -'
> -
> -test_expect_success 'pull.rebase not set and --no-rebase given' '
> -       git reset --hard c0 &&
> -       git pull --no-rebase . c1 2>err &&
> -       test_i18ngrep ! "Pulling without specifying how to reconcile" err
> -'
> -
> -test_expect_success 'pull.rebase not set and --ff given' '
> -       git reset --hard c0 &&
> -       git pull --ff . c1 2>err &&
> -       test_i18ngrep ! "Pulling without specifying how to reconcile" err
> -'
> -
> -test_expect_success 'pull.rebase not set and --no-ff given' '
> -       git reset --hard c0 &&
> -       git pull --no-ff . c1 2>err &&
> -       test_i18ngrep ! "Pulling without specifying how to reconcile" err
> -'
> -
> -test_expect_success 'pull.rebase not set and --ff-only given' '
> -       git reset --hard c0 &&
> -       git pull --ff-only . c1 2>err &&
> -       test_i18ngrep ! "Pulling without specifying how to reconcile" err
> -'
> -
>  test_expect_success 'merge c1 with c2' '
>         git reset --hard c1 &&
>         test -f c0.c &&
> diff --git a/t/t7603-merge-reduce-heads.sh b/t/t7603-merge-reduce-heads.sh
> index 98948955ae..27cd94ad6f 100755
> --- a/t/t7603-merge-reduce-heads.sh
> +++ b/t/t7603-merge-reduce-heads.sh
> @@ -68,7 +68,7 @@ test_expect_success 'merge c1 with c2, c3, c4, c5' '
>
>  test_expect_success 'pull c2, c3, c4, c5 into c1' '
>         git reset --hard c1 &&
> -       git pull . c2 c3 c4 c5 &&
> +       git pull --no-rebase . c2 c3 c4 c5 &&
>         test "$(git rev-parse c1)" != "$(git rev-parse HEAD)" &&
>         test "$(git rev-parse c1)" = "$(git rev-parse HEAD^1)" &&
>         test "$(git rev-parse c2)" = "$(git rev-parse HEAD^2)" &&
> --
> 2.29.2
>


-- 
Felipe Contreras

Re: [RFC 2/2] pull: default pull.ff to "only" when pull.rebase is not set either

[Felipe Contreras]

On Tue, Nov 24, 2020 at 9:45 PM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> On Tue, Nov 24, 2020 at 8:14 PM Alex Henrie <alexhenrie24@gmail.com> wrote:
>
> Before making this the default we need a solution *right now* that is
> a sane default.

This mail was sent by mistake. I was going to say:

We need a warning like:

  The pull was not fast-forward, please either merge or rebase.

When the default (pull.ff=only) is set.

It is the current status that is more urgent to fix.

-- 
Felipe Contreras

Re: [RFC 2/2] pull: default pull.ff to "only" when pull.rebase is not set either

[Philip Oakley]

On 25/11/2020 03:47, Felipe Contreras wrote:
> On Tue, Nov 24, 2020 at 9:45 PM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
>> On Tue, Nov 24, 2020 at 8:14 PM Alex Henrie <alexhenrie24@gmail.com> wrote:
>>
>> Before making this the default we need a solution *right now* that is
>> a sane default.
> This mail was sent by mistake. I was going to say:
>
> We need a warning like:
>
>   The pull was not fast-forward, please either merge or rebase.
>
> When the default (pull.ff=only) is set.
>
> It is the current status that is more urgent to fix.
>

Maybe this could also, as an interim measure, be a doc change right in
the first paragraph of the `git pull --help` description section to warn
that its current default may not be suitable for most users, and to see
the `--ff-only` option (and variants) and its matching config variable.

e.g. "In its default mode, git pull uses the --ff option and is
shorthand for git fetch followed by git merge FETCH_HEAD. The --ff-only
option may be more suitable for modern usage. It can be set using `git
config pull.ff only`."
 (then once v3.0 arrives the discussion can be flipped)

It's worth making sure that the manuals are easy to read.

Philip

Re: [RFC 2/2] pull: default pull.ff to "only" when pull.rebase is not set either

[Felipe Contreras]

On Wed, Nov 25, 2020 at 7:25 AM Philip Oakley <philipoakley@iee.email> wrote:
>
> On 25/11/2020 03:47, Felipe Contreras wrote:
> > On Tue, Nov 24, 2020 at 9:45 PM Felipe Contreras
> > <felipe.contreras@gmail.com> wrote:
> >> On Tue, Nov 24, 2020 at 8:14 PM Alex Henrie <alexhenrie24@gmail.com> wrote:
> >>
> >> Before making this the default we need a solution *right now* that is
> >> a sane default.
> > This mail was sent by mistake. I was going to say:
> >
> > We need a warning like:
> >
> >   The pull was not fast-forward, please either merge or rebase.
> >
> > When the default (pull.ff=only) is set.
> >
> > It is the current status that is more urgent to fix.
> >
>
> Maybe this could also, as an interim measure, be a doc change right in
> the first paragraph of the `git pull --help` description section to warn
> that its current default may not be suitable for most users, and to see
> the `--ff-only` option (and variants) and its matching config variable.
>
> e.g. "In its default mode, git pull uses the --ff option and is
> shorthand for git fetch followed by git merge FETCH_HEAD. The --ff-only
> option may be more suitable for modern usage. It can be set using `git
> config pull.ff only`."
>  (then once v3.0 arrives the discussion can be flipped)
>
> It's worth making sure that the manuals are easy to read.

This is what my patch attempted to do:

https://lore.kernel.org/git/20201125032938.786393-11-felipe.contreras@gmail.com/

Before warning that a non-fast-forward pull failed, it's probably
sensible to explain what a fast-forward is.

-- 
Felipe Contreras

Re: [RFC 2/2] pull: default pull.ff to "only" when pull.rebase is not set either

[Junio C Hamano]

Alex Henrie <alexhenrie24@gmail.com> writes:

> diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
> index eb0aabd396..1eb79e2939 100644
> --- a/Documentation/merge-options.txt
> +++ b/Documentation/merge-options.txt
> @@ -43,10 +43,16 @@ set to `no` at the beginning of them.
>  --no-ff::
>  --ff-only::
>  	Specifies how a merge is handled when the merged-in history is
> -	already a descendant of the current history.  `--ff` is the
> -	default unless merging an annotated (and possibly signed) tag
> -	that is not stored in its natural place in the `refs/tags/`
> -	hierarchy, in which case `--no-ff` is assumed.
> +	already a descendant of the current history.
> +	+
> +	In `git pull`, `--ff-only` is the default unless `--rebase` or
> +	`--no-rebase` is specified, in which case the default is `--ff`.
> +	However, the default changes again to `--no-ff` in the unlikely
> +	scenario where an annotated (and possibly signed) tag that is not
> +	stored in its natural place in the `refs/tags/` hierarchy is being
> +	merged.
> +	+
> +	In `git merge`, `--ff` is always the default.

As we can see one line below, I think these new paragraphs must be
dedented.

> +
> With `--ff`, when possible resolve the merge as a fast-forward (only
> update the branch pointer to match the merged branch; do not create a
> diff --git a/builtin/pull.c b/builtin/pull.c
> index 4d80efe5b7..c703560a77 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -292,17 +292,21 @@ static void set_reflog_message(int argc, const char **argv)
>  }
>  
>  /**
> - * If pull.ff is unset, returns NULL. If pull.ff is "true", returns "--ff". If
> - * pull.ff is "false", returns "--no-ff". If pull.ff is "only", returns
> - * "--ff-only". Otherwise, if pull.ff is set to an invalid value, die with an
> - * error.
> + * If pull.ff is unset, returns NULL if pull.rebase is set or "--ff-only" if
> + * pull.rebase is not set. If pull.ff is "true", returns "--ff". If pull.ff is
> + * "false", returns "--no-ff". If pull.ff is "only", returns "--ff-only".
> + * Otherwise, if pull.ff is set to an invalid value, die with an error.
>   */
>  static const char *config_get_ff(void)
>  {
>  	const char *value;
>  
> -	if (git_config_get_value("pull.ff", &value))
> -		return NULL;
> +	if (git_config_get_value("pull.ff", &value)) {
> +		if (opt_rebase < 0)
> +			return "--ff-only";
> +		else
> +			return NULL;
> +	}

So, the idea is that we currently return NULL when pull.ff is set,
but now we also check "pull.rebase" etc. and give "--ff-only" when
we did not choose --[no-]rebase and lack the configuration.  So the
default (i.e. when pull.ff and pull.rebase are not set) would be as
if the user said

	git pull --ff-only --no-rebase

I am not seeing what problem this tries to solve, exactly, though.

I would have expected that for those who did not choose between
merge and rebase (either with the pull.rebase configuration or from
the command line --[no-]rebase option) the command would behave as
if --ff-only is given, regardless of how pull.ff configuration is
set.  That way, those who are unconfigured will just follow along
what happens at the upstream, until they have their own development,
at which point "--ff-only" can no longer be satisfied and their
"pull" would fail until they choose how to consolidate their work
with the upstream.

And once they choose between merge and rebase, we no longer have to
force "--ff-only" and things will start working as specified by the
user.  In other words, what I would have expected in the previous
paragraph is a minimum and natural extension of the current "you
must pick between merge and rebase or you'll get an advice message
until you do so"---we do not warn for those who haven't chosen as
long as they are only riding along with the upstream without having
their own development.

Having said that, back to what you did in your patch (read: the
following comments are not about achieving what I would have
expected; it is about what this patch from you seems to be trying to
do).

The above code in config_get_ff() obviously depends on the order in
the caller to make this call and determine opt_rebase's value and I
see you tweaked the caller to call config_get_rebase() before it
makes a call to this helper.

It feels a bit brittle now because of this subtle dependency.

Perhaps it is a good idea to introduce another helper that
cmd_pull() calls, and make that helper call these two helpers that
determine the value of opt_rebase and opt_ff, to hide the
dependency.

> @@ -322,7 +326,7 @@ static const char *config_get_ff(void)
>   * 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.
> + * exist, returns REBASE_INVALID.
>   */
>  static enum rebase_type config_get_rebase(void)
>  {
> @@ -344,23 +348,7 @@ static enum rebase_type config_get_rebase(void)
>  	if (!git_config_get_value("pull.rebase", &value))
>  		return parse_config_rebase("pull.rebase", value, 1);
>  
> -	if (opt_verbosity >= 0 && !opt_ff) {
> -		warning(_("Starting in Git 3.0, the default behavior of `git pull` will change\n"
> -			"when it is not specified how to reconcile divergent branches. You can\n"
> -			"squelch this message by running one of the following commands sometime\n"
> -			"before your next pull:\n"
> -			"\n"
> -			"  git config pull.rebase false  # merge (the current default)\n"
> -			"  git config pull.rebase true   # rebase\n"
> -			"  git config pull.ff only       # fast-forward only (the future default)\n"
> -			"\n"
> -			"You can replace \"git config\" with \"git config --global\" to set a default\n"
> -			"preference for all repositories. You can also pass --rebase, --no-rebase,\n"
> -			"or --ff-only on the command line to override the configured default per\n"
> -			"invocation.\n"));
> -	}
> -
> -	return REBASE_FALSE;
> +	return REBASE_INVALID;
>  }

Hmph, and who reacts to this REBASE_INVALID value?  Shouldn't there
be something that catches this INVALID value and stop/complain?

>  
>  /**
> @@ -931,11 +919,14 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>  
>  	parse_repo_refspecs(argc, argv, &repo, &refspecs);
>  
> +	if (opt_rebase < 0)
> +		opt_rebase = config_get_rebase();
> +
>  	if (!opt_ff)
>  		opt_ff = xstrdup_or_null(config_get_ff());
>  
>  	if (opt_rebase < 0)
> -		opt_rebase = config_get_rebase();
> +		opt_rebase = REBASE_FALSE;
>  
>  	if (read_cache_unmerged())
>  		die_resolve_conflict("pull");

Another thing.  Does "git pull --rebase --ff-only" behave sensibly?
"rebase our history on top of theirs only if we are truly ahead" is
a bit strange way to say "we only ride along without having our own
development, and once we discover we have our own stuff, stop us".
IIRC, "git pull --rebase" ignored the "--ff-only" request and
rebased our own stuff on top of their history anyway, which we would
want to fix.

Re: [RFC 2/2] pull: default pull.ff to "only" when pull.rebase is not set either

[Felipe Contreras]

On Wed, Dec 2, 2020 at 8:21 PM Junio C Hamano <gitster@pobox.com> wrote:

> So, the idea is that we currently return NULL when pull.ff is set,
> but now we also check "pull.rebase" etc. and give "--ff-only" when
> we did not choose --[no-]rebase and lack the configuration.  So the
> default (i.e. when pull.ff and pull.rebase are not set) would be as
> if the user said
>
>         git pull --ff-only --no-rebase
>
> I am not seeing what problem this tries to solve, exactly, though.
>
> I would have expected that for those who did not choose between
> merge and rebase (either with the pull.rebase configuration or from
> the command line --[no-]rebase option) the command would behave as
> if --ff-only is given, regardless of how pull.ff configuration is
> set.

That's right, I would have expected the same as well, but right now
the pull warning is disabled when you have a pull.ff configuration (or
--[no-]ff[-only]), as it was done in commit: 54200cef86 (pull: don't
warn if pull.ff has been set).

In my mind doing "git pull" is the equivalent of doing an implicit
"git pull --ff", so I don't see why setting "pull.ff=true" should
change the warning. The warning should not be about lack of
configuration (we want git to eventually work correctly without
configuration), it should be about something unexpected about to
happen (i.e. a non-fast-forward merge).

So quite likely Alex extended the logic of the current warning to the
default mode, which I don't think is right.

We want the logic of the warning to change: to be displayed a) only
when there's a non-fast-forward, b) when the user has not specified or
configured either a merge or a rebase.

Only after a while of this warning on the wild should the default mode
be changed.

But we need to fix the logic of the warning first.

> > @@ -344,23 +348,7 @@ static enum rebase_type config_get_rebase(void)
> >       if (!git_config_get_value("pull.rebase", &value))
> >               return parse_config_rebase("pull.rebase", value, 1);

...

> > -     return REBASE_FALSE;
> > +     return REBASE_INVALID;
> >  }
>
> Hmph, and who reacts to this REBASE_INVALID value?  Shouldn't there
> be something that catches this INVALID value and stop/complain?

Later on there's a "if (opt_rebase < 0) opt_rebase = REBASE_FALSE", so
this is only temporarily set as REBASE_INVALID for config_get_ff().

I don't find the semantics of this approach appealing, which is why I
introduced REBASE_DEFAULT in my patch series, so that the meaning of
REBASE_INVALID is not muddled.

> Another thing.  Does "git pull --rebase --ff-only" behave sensibly?
> "rebase our history on top of theirs only if we are truly ahead" is
> a bit strange way to say "we only ride along without having our own
> development, and once we discover we have our own stuff, stop us".
> IIRC, "git pull --rebase" ignored the "--ff-only" request and
> rebased our own stuff on top of their history anyway, which we would
> want to fix.

We would want this new mode to consistently fail with a good
user-friendly message, better than "fatal: Not possible to
fast-forward, aborting.".

But this mode is the opposite of "git pull --rebase", which is why it
doesn't make sense to do "git pull --rebase --ff-only" (which is
ignored right now).

In other words: --rebase should disable the --ff-only mode.

Also, we would want --no-rebase to disable the default --ff-only mode.
That would require changing the semantics of --ff-only, so that "git
pull --no-rebase --ff-only" doesn't make sense (as --ff-only is
overridden by --no-rebase).

If we do this, then the only time where --ff-only would make sense is
in "git pull --ff-only" (no --rebase or --no-rebase), and if we change
the semantics this way, then there's no need for my suggested
pull.mode (although it still might be desired).

Moreover, I see no tests that check for this new behavior.

Cheers.

-- 
Felipe Contreras

Re: [RFC 2/2] pull: default pull.ff to "only" when pull.rebase is not set either

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> In other words: --rebase should disable the --ff-only mode.
>
> Also, we would want --no-rebase to disable the default --ff-only mode.

Yes, and for that a configured pull.rebase counts the same as a
command line option.  As you agreed earlier in the part omitted from
the quote with my "I would have expected...", we want to say:

    There are two ways to consolidate your own work with the history
    of the other side, that gives a result in vastly different
    shapes that suit for different workflows, and "git pull" will
    not choose between them for you.  Unless you say which one
    between rebase and merge you want to use, it will work only for
    fast-forward updating from the other side and nothing else.

and the "default to --ff-only when the user does not say" is a means
to do so.  When the end-user gives an explicit preference between
the merge/rebase, none of the above would apply.

> That would require changing the semantics of --ff-only, so that "git
> pull --no-rebase --ff-only" doesn't make sense (as --ff-only is
> overridden by --no-rebase).

I do not think such a conclusion follows from "we do not want to use
the 'by default force the --ff-only' when the user chooses between
merge and rebase".  Specifically, I do not agree with "as --ff-only
is overridden" in your statement.

When the end user says "git pull --no-rebase --ff-only", it means to
me:

    I choose not to use rebase---my preference is to merge in the
    other history into mine, and I want to reject any non-fast
    update in this invocation.

and I find it quite sensible, especially in a modern world where you
practically must set pull.rebase to one way or the other.  A
developer X, an individual contributor to the project who uses
rebase most of the time, may use "git pull --no-rebase" from
somebody else's repository (i.e. not X's "upstream") when a help is
offered by another developer Y who forked from X to advance X's
work.  If the upstream project does not want to see merge commits in
a side branch (i.e. the history X offers to the project), X may ask
Y to make sure Y builds on top of the whole of X's work, and adding
"--ff-only" to the command line would be a way to make sure the
result is fast-forward.

> If we do this, then the only time where --ff-only would make sense is
> in "git pull --ff-only" (no --rebase or --no-rebase), and if we change
> the semantics this way, then there's no need for my suggested
> pull.mode (although it still might be desired).
>
> Moreover, I see no tests that check for this new behavior.

A proposal to change behaviour needs to come with tests to protect
new behaviour before we can merge, but we should be more lenient on
a patch with RFC label on it ;-).

Apparently the patch had me say "I am not seeing what problem this
tries to solve, exactly", and a test may have helped to demonstrate
the intention of the change better, even in its RFC state.

Re: [RFC 2/2] pull: default pull.ff to "only" when pull.rebase is not set either

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>> That would require changing the semantics of --ff-only, so that "git
>> pull --no-rebase --ff-only" doesn't make sense (as --ff-only is
>> overridden by --no-rebase).
>
> I do not think such a conclusion follows from "we do not want to use
> the 'by default force the --ff-only' when the user chooses between
> merge and rebase".  Specifically, I do not agree with "as --ff-only
> is overridden" in your statement.

Ah, sorry, I mis-read your three lines above.

There are currently two ways "git pull" consolidates your work with
the other history.  By default, you are "pulling" work from your
contributors (and that is what "pull request" means---contributors
ask you to pull, and you take their work at your discretion) and the
only way that makes sense is to merge their history into yours.  The
other is you are updating your branch by rebasing your work on top
of what happend in their history.

And if we introduce a third-way, i.e. "we do not handle the case
where you have your own development at all, this is only to maintain
pristine copy from your upstream", and repurpose "--ff-only" for
that purpose, yes, what you said above does make sense.  At that
point, there is no reason to disagree with "as --ff-only is
overridden" part of your statement---in your new world, "--ff-only"
is redesigned to act that way.

In retrospect, "git pull --rebase" was a UI mistake.  What the other
side means is totally different in the operation from what the other
side is in "git pull".  The former is for you to catch up with your
upstream and the latter is for you, who _is_ the upstream to others,
to take others work in as their upstream.  If we instead introduced
a separate command, say "git update", that is "fetch followed by
rebase" (just like "git pull" is "fetch followed by merge"), to
rebase your work on top of updated upstream, there wouldn't be a
need for us to be having this discussion.

It probably is water under the bridge at this point.  Perhaps if
somebody builds a time-machine for me, I'll go back 13 years and
give my younger self this wisdom ;-)

Re: [RFC 2/2] pull: default pull.ff to "only" when pull.rebase is not set either

[Felipe Contreras]

On Thu, Dec 3, 2020 at 1:30 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> >> That would require changing the semantics of --ff-only, so that "git
> >> pull --no-rebase --ff-only" doesn't make sense (as --ff-only is
> >> overridden by --no-rebase).
> >
> > I do not think such a conclusion follows from "we do not want to use
> > the 'by default force the --ff-only' when the user chooses between
> > merge and rebase".  Specifically, I do not agree with "as --ff-only
> > is overridden" in your statement.
>
> Ah, sorry, I mis-read your three lines above.
>
> There are currently two ways "git pull" consolidates your work with
> the other history.  By default, you are "pulling" work from your
> contributors (and that is what "pull request" means---contributors
> ask you to pull, and you take their work at your discretion) and the
> only way that makes sense is to merge their history into yours.

And this is the reason why I warn about using one's worldview too
much. The *vast* majority of users do not use "git pull" this way;
there are more contributors than maintainers.

What they want is to "merge your history into theirs", not the other
way around. So the only way they can do that correctly is by doing
"git fetch" + "git merge". Which is why so many people say to avoid
"git pull" altogether.

So, a newcomer that doesn't know much about git and does a "git pull",
is pretty much guaranteed to do something unintended. If he/she is a
user, the merge will have the parents the other way around, but even
as a maintainer, the project might not like merges, and he/she will
introduce an unwanted merge, or worse; an evil merge.

> The other is you are updating your branch by rebasing your work on
> top of what happend in their history.

Again, what the user might want is the opposite. If the user is a
maintainer, these two:

  git pull --merge github john
  git pull --rebase github john

Should be about their history to yours (or on top of yours).

> And if we introduce a third-way, i.e. "we do not handle the case
> where you have your own development at all, this is only to maintain
> pristine copy from your upstream", and repurpose "--ff-only" for
> that purpose, yes, what you said above does make sense.  At that
> point, there is no reason to disagree with "as --ff-only is
> overridden" part of your statement---in your new world, "--ff-only"
> is redesigned to act that way.

That's right. Otherwise "git pull --no-rebase" will fail; you will
have to specify --ff (or --no-ff) for it to work. And that doesn't
make sense to me.

Specifying --no-rebase should override the default --ff-only mode (or
pull.mode=ff-only).

> In retrospect, "git pull --rebase" was a UI mistake.  What the other
> side means is totally different in the operation from what the other
> side is in "git pull".  The former is for you to catch up with your
> upstream and the latter is for you, who _is_ the upstream to others,
> to take others work in as their upstream.  If we instead introduced
> a separate command, say "git update", that is "fetch followed by
> rebase" (just like "git pull" is "fetch followed by merge"), to
> rebase your work on top of updated upstream, there wouldn't be a
> need for us to be having this discussion.
>
> It probably is water under the bridge at this point.  Perhaps if
> somebody builds a time-machine for me, I'll go back 13 years and
> give my younger self this wisdom ;-)

You don't have to go back 13 years ago, you can go back 6 years ago
when I wrote all the patches for git update [1], explained the summary
of the problem [2], and others urged git developers to pay more
attention to the patch [3].

But as you say; water under the bridge.

Today there are 3 things to do:

1. Improve the annoying warning
2. Consider changing the semantics of --ff-only, or implement pull.mode=ff-only
3. Consider a new "git update" command

Since my new (2020) patches for pull.mode (solve 1 and 2) have not
been reviewed, I'm thinking there's too much inertia and perhaps it's
time to cash in the chips and concentrate only on 1.

Cheers.

[1] https://github.com/felipec/git/commit/d38f1641fc33535aa3c92cf6d3a30334324d3488
[2] https://lore.kernel.org/git/5366db742d494_18f9e4b308aa@nysa.notmuch/
[3] https://lore.kernel.org/git/CAGK7Mr4uucBN=17ph5pBjrz7yP60By1sERU9oBL+c2-gsMDmrw@mail.gmail.com/

-- 
Felipe Contreras

Re: [RFC 2/2] pull: default pull.ff to "only" when pull.rebase is not set either

[Jacob Keller]

On Thu, Dec 3, 2020 at 3:08 PM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> On Thu, Dec 3, 2020 at 1:30 PM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > Junio C Hamano <gitster@pobox.com> writes:
> >
> > >> That would require changing the semantics of --ff-only, so that "git
> > >> pull --no-rebase --ff-only" doesn't make sense (as --ff-only is
> > >> overridden by --no-rebase).
> > >
> > > I do not think such a conclusion follows from "we do not want to use
> > > the 'by default force the --ff-only' when the user chooses between
> > > merge and rebase".  Specifically, I do not agree with "as --ff-only
> > > is overridden" in your statement.
> >
> > Ah, sorry, I mis-read your three lines above.
> >
> > There are currently two ways "git pull" consolidates your work with
> > the other history.  By default, you are "pulling" work from your
> > contributors (and that is what "pull request" means---contributors
> > ask you to pull, and you take their work at your discretion) and the
> > only way that makes sense is to merge their history into yours.
>
> And this is the reason why I warn about using one's worldview too
> much. The *vast* majority of users do not use "git pull" this way;
> there are more contributors than maintainers.
>
> What they want is to "merge your history into theirs", not the other
> way around. So the only way they can do that correctly is by doing
> "git fetch" + "git merge". Which is why so many people say to avoid
> "git pull" altogether.
>
> So, a newcomer that doesn't know much about git and does a "git pull",
> is pretty much guaranteed to do something unintended. If he/she is a
> user, the merge will have the parents the other way around, but even
> as a maintainer, the project might not like merges, and he/she will
> introduce an unwanted merge, or worse; an evil merge.
>

As someone who spends a significant amount of energy at work educating
newcomers to git, this is a significant problem.

> > The other is you are updating your branch by rebasing your work on
> > top of what happend in their history.
>
> Again, what the user might want is the opposite. If the user is a
> maintainer, these two:
>
>   git pull --merge github john
>   git pull --rebase github john
>
> Should be about their history to yours (or on top of yours).
>
> > And if we introduce a third-way, i.e. "we do not handle the case
> > where you have your own development at all, this is only to maintain
> > pristine copy from your upstream", and repurpose "--ff-only" for
> > that purpose, yes, what you said above does make sense.  At that
> > point, there is no reason to disagree with "as --ff-only is
> > overridden" part of your statement---in your new world, "--ff-only"
> > is redesigned to act that way.
>
> That's right. Otherwise "git pull --no-rebase" will fail; you will
> have to specify --ff (or --no-ff) for it to work. And that doesn't
> make sense to me.
>
> Specifying --no-rebase should override the default --ff-only mode (or
> pull.mode=ff-only).
>
> > In retrospect, "git pull --rebase" was a UI mistake.  What the other
> > side means is totally different in the operation from what the other
> > side is in "git pull".  The former is for you to catch up with your
> > upstream and the latter is for you, who _is_ the upstream to others,
> > to take others work in as their upstream.  If we instead introduced
> > a separate command, say "git update", that is "fetch followed by
> > rebase" (just like "git pull" is "fetch followed by merge"), to
> > rebase your work on top of updated upstream, there wouldn't be a
> > need for us to be having this discussion.
> >
> > It probably is water under the bridge at this point.  Perhaps if
> > somebody builds a time-machine for me, I'll go back 13 years and
> > give my younger self this wisdom ;-)
>
> You don't have to go back 13 years ago, you can go back 6 years ago
> when I wrote all the patches for git update [1], explained the summary
> of the problem [2], and others urged git developers to pay more
> attention to the patch [3].
>
> But as you say; water under the bridge.
>


> Today there are 3 things to do:
>
> 1. Improve the annoying warning
> 2. Consider changing the semantics of --ff-only, or implement pull.mode=ff-only
> 3. Consider a new "git update" command
>
> Since my new (2020) patches for pull.mode (solve 1 and 2) have not
> been reviewed, I'm thinking there's too much inertia and perhaps it's
> time to cash in the chips and concentrate only on 1.
>

I think that solving (1) is relatively simple and worth doing. I think
solving (2) is trickier but worth doing in the medium time frame.

Doing (3) may have value.. We did it for git checkout already
recently, by adding git switch and git restore-files.. But it's a
bigger undertaking.

I personally like changing the default of pull to be "I will only fast
forward, and you need to tell me what to do if history isn't fast
forward". This does cause some strain as it forces users to now have
to understand and pick which thing they want, (rebase or merge) when
combining history.. But I think that's better than users not being
asked to pick and just having merge be the default unless they go out
of their way to set the rebase by default option.

> Cheers.
>
> [1] https://github.com/felipec/git/commit/d38f1641fc33535aa3c92cf6d3a30334324d3488
> [2] https://lore.kernel.org/git/5366db742d494_18f9e4b308aa@nysa.notmuch/
> [3] https://lore.kernel.org/git/CAGK7Mr4uucBN=17ph5pBjrz7yP60By1sERU9oBL+c2-gsMDmrw@mail.gmail.com/
>
> --
> Felipe Contreras

Re: [RFC 2/2] pull: default pull.ff to "only" when pull.rebase is not set either

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>>> That would require changing the semantics of --ff-only, so that "git
>>> pull --no-rebase --ff-only" doesn't make sense (as --ff-only is
>>> overridden by --no-rebase).
>>
>> I do not think such a conclusion follows from "we do not want to use
>> the 'by default force the --ff-only' when the user chooses between
>> merge and rebase".  Specifically, I do not agree with "as --ff-only
>> is overridden" in your statement.
>
> Ah, sorry, I mis-read your three lines above.
> ...
> And if we introduce a third-way, i.e. "we do not handle the case
> where you have your own development at all, this is only to maintain
> pristine copy from your upstream", and repurpose "--ff-only" for
> that purpose, yes, what you said above does make sense.  At that
> point, there is no reason to disagree with "as --ff-only is
> overridden" part of your statement---in your new world, "--ff-only"
> is redesigned to act that way.

Just to avoid misunderstanding, I only meant to say that the first
three lines I quoted is internally consistent (unlike the message I
was responding to, in which I said "I disagree---the conclusion does
not follow").  It no way means I think such a re-definition of what
"--ff-only" means is a great design.

What we want to see can be done without such backward incompatible
changes, e.g. declaring the combination of "--ff-only" and
"--[no-]rebase" incompatible and/or the last one wins, I would say,
and I suspect Alex's RFC was an attempt to make the first step in
that direction.

What is most missing in the current system is a fix for the way in
which "--rebase" interacts with "--ff-only".  Immediately after
fetching, if our current branch is not a subset of what we fetched
from the other side, the command should die.  Otherwise, it should
just rebase what we have (which is nothing) on top of the tip of the
history of the other side (which is to fast-forward our tip and the
working tree to their tip).  

Another thing we would want to change is to make the "you must
choose between rebase and merge" trigger a lot more lazily.  If our
side does not have our own development and their history is a
descendent of what we have, the user shouldn't have to choose.  Only
when the history they have does not fast-forward, we have to abort
giving the "you must choose between the two" warning message.

When the user tells us to do rebase or merge, nothing (except that
"--ff-only" should prevent the rebase from going forward) should
have to be changed.

Re: [RFC 2/2] pull: default pull.ff to "only" when pull.rebase is not set either

[Felipe Contreras]

On Thu, Dec 3, 2020 at 8:06 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Junio C Hamano <gitster@pobox.com> writes:
> >
> >>> That would require changing the semantics of --ff-only, so that "git
> >>> pull --no-rebase --ff-only" doesn't make sense (as --ff-only is
> >>> overridden by --no-rebase).
> >>
> >> I do not think such a conclusion follows from "we do not want to use
> >> the 'by default force the --ff-only' when the user chooses between
> >> merge and rebase".  Specifically, I do not agree with "as --ff-only
> >> is overridden" in your statement.
> >
> > Ah, sorry, I mis-read your three lines above.
> > ...
> > And if we introduce a third-way, i.e. "we do not handle the case
> > where you have your own development at all, this is only to maintain
> > pristine copy from your upstream", and repurpose "--ff-only" for
> > that purpose, yes, what you said above does make sense.  At that
> > point, there is no reason to disagree with "as --ff-only is
> > overridden" part of your statement---in your new world, "--ff-only"
> > is redesigned to act that way.
>
> Just to avoid misunderstanding, I only meant to say that the first
> three lines I quoted is internally consistent (unlike the message I
> was responding to, in which I said "I disagree---the conclusion does
> not follow").  It no way means I think such a re-definition of what
> "--ff-only" means is a great design.

Yes, that's what I parsed.

> What we want to see can be done without such backward incompatible
> changes, e.g. declaring the combination of "--ff-only" and
> "--[no-]rebase" incompatible and/or the last one wins, I would say,
> and I suspect Alex's RFC was an attempt to make the first step in
> that direction.

It's debatable whether or not that is "backwards incompatible".

Currently "--no-rebase --ff-only" fails if the merge is
non-fast-forward. With the proposed change of semantics it would work.
That's a change.

> What is most missing in the current system is a fix for the way in
> which "--rebase" interacts with "--ff-only".  Immediately after
> fetching, if our current branch is not a subset of what we fetched
> from the other side, the command should die.  Otherwise, it should
> just rebase what we have (which is nothing) on top of the tip of the
> history of the other side (which is to fast-forward our tip and the
> working tree to their tip).

Keep in mind the whole point of the changes: to make --ff-only the
default. If you make "git pull --rebase --ff-only" fail if
not-fast-forward, then "git pull --rebase" will fail if you make
--ff-only the default.

I don't know if I'm not doing a good job of stating that --rebase
should override (and ignore) --ff-only, that's the way it should
interact.

Here it goes again. In short, this is the target behavior:

* git pull --ff-only # fail unless fast-forward
* git pull --merge --ff-only # ignore --ff-only
* git pull --rebase --ff-only # ignore --ff-only

> Another thing we would want to change is to make the "you must
> choose between rebase and merge" trigger a lot more lazily.  If our
> side does not have our own development and their history is a
> descendent of what we have, the user shouldn't have to choose.  Only
> when the history they have does not fast-forward, we have to abort
> giving the "you must choose between the two" warning message.

Yes, I already sent patches for that [1].

> When the user tells us to do rebase or merge, nothing (except that
> "--ff-only" should prevent the rebase from going forward) should
> have to be changed.

If --ff-only prevents the rebase and the merge from going forward,
then it cannot be the default.

Maybe my last patch series can make things clearer [2].

Cheers.

[1] https://lore.kernel.org/git/20201204061623.1170745-11-felipe.contreras@gmail.com/
[2] https://lore.kernel.org/git/20201204061623.1170745-1-felipe.contreras@gmail.com/T/

-- 
Felipe Contreras

Re: [RFC 2/2] pull: default pull.ff to "only" when pull.rebase is not set either

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

>> What we want to see can be done without such backward incompatible
>> changes, e.g. declaring the combination of "--ff-only" and
>> "--[no-]rebase" incompatible and/or the last one wins, I would say,
>> and I suspect Alex's RFC was an attempt to make the first step in
>> that direction.
>
> It's debatable whether or not that is "backwards incompatible".
>
> Currently "--no-rebase --ff-only" fails if the merge is
> non-fast-forward. With the proposed change of semantics it would work.
> That's a change.

But with such a change, "--ff-only --no-rebase" would work by
ignoring the "I want to reject non-ff situation" request from the
user, no?

> Keep in mind the whole point of the changes: to make --ff-only the
> default.

Sorry, I know you keep repeating that "keep in mind", but I do not
quite see why anybody needs to keep that in mind.  Has a concensus
that the repurposed --ff-only should be the default been
established?

Re: [RFC 2/2] pull: default pull.ff to "only" when pull.rebase is not set either

[Felipe Contreras]

On Fri, Dec 4, 2020 at 1:37 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
> >> What we want to see can be done without such backward incompatible
> >> changes, e.g. declaring the combination of "--ff-only" and
> >> "--[no-]rebase" incompatible and/or the last one wins, I would say,
> >> and I suspect Alex's RFC was an attempt to make the first step in
> >> that direction.
> >
> > It's debatable whether or not that is "backwards incompatible".
> >
> > Currently "--no-rebase --ff-only" fails if the merge is
> > non-fast-forward. With the proposed change of semantics it would work.
> > That's a change.
>
> But with such a change, "--ff-only --no-rebase" would work by
> ignoring the "I want to reject non-ff situation" request from the
> user, no?

Yes. And that's a change.

That's why the "pull.mode=ff-only" solution is cleaner; it doesn't
need --ff-only to change its semantics.

> > Keep in mind the whole point of the changes: to make --ff-only the
> > default.
>
> Sorry, I know you keep repeating that "keep in mind", but I do not
> quite see why anybody needs to keep that in mind.  Has a concensus
> that the repurposed --ff-only should be the default been
> established?

That's the whole point of this patch (pull: default pull.ff to "only"
when pull.rebase is not set either).

If not --ff-only, then "pull.mode=ff-only".

Are you saying making the default be fast-forward merges only is not
the eventual end-goal?

Cheers.

-- 
Felipe Contreras

Re: [RFC 2/2] pull: default pull.ff to "only" when pull.rebase is not set either

[Alex Henrie]

On Wed, Dec 2, 2020 at 7:21 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> So, the idea is that we currently return NULL when pull.ff is set,
> but now we also check "pull.rebase" etc. and give "--ff-only" when
> we did not choose --[no-]rebase and lack the configuration.  So the
> default (i.e. when pull.ff and pull.rebase are not set) would be as
> if the user said
>
>         git pull --ff-only --no-rebase
>
> I am not seeing what problem this tries to solve, exactly, though.
>
> I would have expected that for those who did not choose between
> merge and rebase (either with the pull.rebase configuration or from
> the command line --[no-]rebase option) the command would behave as
> if --ff-only is given, regardless of how pull.ff configuration is
> set.  That way, those who are unconfigured will just follow along
> what happens at the upstream, until they have their own development,
> at which point "--ff-only" can no longer be satisfied and their
> "pull" would fail until they choose how to consolidate their work
> with the upstream.

My goal was to make `git pull` without arguments and without any
configuration set (the most common case) reject non-fast-forwards, and
not add any new config variables or substantially change the semantics
of the existing ones. In my opinion it's fine that setting pull.ff
would change the behavior of `git pull` without arguments; I don't
think that that would be bad behavior.

I am glad to see so much discussion surrounding the behavior of and
documentation for `git pull`, but unfortunately at this point I'm
having trouble following the discussion as a whole. I'm hoping that it
settles down some so that it becomes more clear what I can do to help.

-Alex

Re: [RFC 2/2] pull: default pull.ff to "only" when pull.rebase is not set either

[Felipe Contreras]

On Fri, Dec 11, 2020 at 2:38 PM Alex Henrie <alexhenrie24@gmail.com> wrote:
>
> On Wed, Dec 2, 2020 at 7:21 PM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > So, the idea is that we currently return NULL when pull.ff is set,
> > but now we also check "pull.rebase" etc. and give "--ff-only" when
> > we did not choose --[no-]rebase and lack the configuration.  So the
> > default (i.e. when pull.ff and pull.rebase are not set) would be as
> > if the user said
> >
> >         git pull --ff-only --no-rebase
> >
> > I am not seeing what problem this tries to solve, exactly, though.
> >
> > I would have expected that for those who did not choose between
> > merge and rebase (either with the pull.rebase configuration or from
> > the command line --[no-]rebase option) the command would behave as
> > if --ff-only is given, regardless of how pull.ff configuration is
> > set.  That way, those who are unconfigured will just follow along
> > what happens at the upstream, until they have their own development,
> > at which point "--ff-only" can no longer be satisfied and their
> > "pull" would fail until they choose how to consolidate their work
> > with the upstream.
>
> My goal was to make `git pull` without arguments and without any
> configuration set (the most common case) reject non-fast-forwards,

This is what we all want, in my opinion.

> and not add any new config variables

But we need first to give users the ability to test this new mode with
a configuration variable.

It's how we get there that we disagree.

> In my opinion it's fine that setting pull.ff
> would change the behavior of `git pull` without arguments; I don't
> think that that would be bad behavior.

No, it's not bad behavior. But we need to warn our users that this
change is coming first, and how to try it out.

That's what I think.

Cheers.

-- 
Felipe Contreras