[PATCH v6 0/7] Reject non-ff pulls by default

[PATCH v6 0/7] Reject non-ff pulls by default

[Felipe Contreras]

NOTE: Added a commit to throw a warning before the final switch.

It is very typical for Git newcomers to inadvertently create merges and worst:
inadvertently pushing them. This is one of the reasons many experienced users
prefer to avoid 'git pull', and recommend newcomers to avoid it as well.

To avoid these problems and keep 'git pull' useful, it has been agreed that
'git pull' should barf by default if the merge is non-fast-forward.
Unfortunately this breaks backwards-compatibility, so we need to be careful
about the error messages we give, and that we provide enough information to our
users to move forward without distrupting their workflow too much.

With the proper error messages and documentation, it has been agreed that the
new behavior is OK.

These are the steps needed to achieve this:

4) Only allow fast-forward merges by default

We could pass --ff-only to `git merge`, however, if we do that we'll get an error like this:

  Not possible to fast-forward, aborting.

This is not friendly; we want an error that is user-friendly:

  The pull was not fast-forward, please either merge or rebase.
  If unsure, run 'git pull --merge'.

When we do this we want to give the users the option to go back to the previous
behavior, so a new configuration is needed.

3) Add ff-only config

This option would trigger a check inside `git pull` itself, and error out with
the aforementioned message if it's not possible to do a fast-forward merge.

However, this option conflicts with --rebase, and --no-rebase. Solution below.

2) Add --merge option

Since we have a message that says "If unsure, run 'git pull --merge'", which is
more friendly than 'git pull --no-rebase', we should add this option, and
deprecate --no-rebase.

However, the documentation would become confusing if --merge is configured in
pull.rebase, instead, we want something like this:

  See `pull.mode`, `branch.<name>.pullmode` in linkgit:git-config[1] if you want
  to make `git pull` always use `--merge`.

1) Rename pull.rename to pull.mode and
   branch.<name>.rebase to branch.<name>.pullmode

This way the configurations and options remain consistent:

  git pull --merge
  pull.mode = merge
  branch.<name>.pullmode = merge

  git pull --rebase
  pull.mode = rebase
  branch.<name>.pullmode = rebase

  git pull --rebase=preserve
  pull.mode = rebase-preserve
  branch.<name>.pullmode = rebase-preserve

  git pull
  pull.mode = ff-only
  branch.<name>.pullmode = ff-only
 
This patch series does all the steps mentioned, but in reverse order, and in
addition updates the tests to use the new configurations instead.

Changes since v5:

 * Add commit to enable a transitional warning
 * Renamed option to ff-only
 * Added deprecation documentation for configs
 * Removed config deprecation warnings

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 4ebd3b5..b391ec1 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -774,6 +774,8 @@ branch.<name>.pullmode::
 	'git rebase' so that locally committed merge commits will not be
 	flattened by running 'git pull'.
 +
+	It was named 'branch.<name>.rebase' but that is deprecated now.
++
 *NOTE*: this is a possibly dangerous operation; do *not* use
 it unless you understand the implications (see linkgit:git-rebase[1]
 for details).
@@ -1884,8 +1886,8 @@ pretty.<name>::
 pull.mode::
 	When "git pull" is run, this determines if it would either merge or
 	rebase the fetched branch. The possible values are 'merge',
-	'rebase', 'merge-ff-only,' and 'rebase-preserve'.
-	If 'merge-ff-only' is specified, the merge will only succeed if it's
+	'rebase', 'ff-only,' and 'rebase-preserve'.
+	If 'ff-only' is specified, the merge will only succeed if it's
 	fast-forward.
 	See "branch.<name>.pullmode" for doing this in a non branch-specific
 	manner.
@@ -1894,6 +1896,9 @@ pull.mode::
 	'git rebase' so that locally committed merge commits will not be
 	flattened by running 'git pull'.
 +
++
+	It was named 'pull.rebase' but that is deprecated now.
++
 *NOTE*: this is a possibly dangerous operation; do *not* use
 it unless you understand the implications (see linkgit:git-rebase[1]
 for details).
diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index ca8e951..968315c 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt
@@ -54,7 +54,7 @@ Then `git pull` will merge in a fast-foward way up to the new master.
 However, a non-fast-foward case looks very different.
 
 ------------
-	  A---B---C origin/master
+	  A---B---C master on origin
 	 /
     D---E---F---G master
 	^
diff --git a/git-pull.sh b/git-pull.sh
index 8cf8f68..bccaf27 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -16,7 +16,7 @@ cd_to_toplevel
 
 
 warn () {
-	printf >&2 '%s\n' "$*"
+	printf >&2 'warning: %s\n' "$*"
 }
 
 die_conflict () {
@@ -57,7 +57,7 @@ then
 	mode=$(git config pull.mode)
 fi
 case "$mode" in
-merge|rebase|merge-ff-only|'')
+merge|rebase|ff-only|'')
 	;;
 rebase-preserve)
 	mode="rebase"
@@ -69,7 +69,7 @@ rebase-preserve)
 	exit 1
 	;;
 esac
-
+# backwards compatibility
 if test -z "$mode"
 then
 	rebase=$(bool_or_string_config branch.$curr_branch_short.rebase)
@@ -77,13 +77,8 @@ then
 	then
 		rebase=$(bool_or_string_config pull.rebase)
 	fi
-	if test -n "$rebase"
-	then
-		warn "$(gettext "The configurations pull.rebase and branch.<name>.rebase are deprecated.")"
-		warn "$(gettext "Please use pull.mode and branch.<name>.pullmode instead.")"
-	fi
 fi
-test -z "$mode" && mode=merge-ff-only
+test -z "$mode" && mode=ff-only
 dry_run=
 while :
 do
@@ -322,7 +317,7 @@ case "$merge_head" in
 *)
 	# check if a non-fast-foward merge would be needed
 	merge_head=${merge_head% }
-	if test "$mode" = merge-ff-only -a -z "$no_ff$ff_only${squash#--no-squash}" &&
+	if test "$mode" = 'ff-only' && test -z "$no_ff$ff_only${squash#--no-squash}" &&
 		test -n "$orig_head" &&
 		! git merge-base --is-ancestor "$orig_head" "$merge_head" &&
 		! git merge-base --is-ancestor "$merge_head" "$orig_head"
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 2e2b476..dc7749b 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -365,9 +365,9 @@ test_expect_success 'git pull --rebase against local branch' '
 	test file = "$(cat file2)"
 '
 
-test_expect_success 'git pull fast-forward' '
+test_expect_success 'git pull fast-forward (ff-only)' '
 	test_when_finished "git checkout master && git branch -D other test" &&
-	test_config pull.mode merge-ff-only &&
+	test_config pull.mode ff-only &&
 	git checkout -b other master &&
 	>new &&
 	git add new &&
@@ -377,9 +377,9 @@ test_expect_success 'git pull fast-forward' '
 	git pull
 '
 
-test_expect_success 'git pull non-fast-forward' '
+test_expect_success 'git pull non-fast-forward (ff-only)' '
 	test_when_finished "git checkout master && git branch -D other test" &&
-	test_config pull.mode merge-ff-only &&
+	test_config pull.mode ff-only &&
 	git checkout -b other master^ &&
 	>new &&
 	git add new &&
@@ -389,9 +389,9 @@ test_expect_success 'git pull non-fast-forward' '
 	test_must_fail git pull
 '
 
-test_expect_success 'git pull non-fast-forward (merge)' '
+test_expect_success 'git pull non-fast-forward with merge (ff-only)' '
 	test_when_finished "git checkout master && git branch -D other test" &&
-	test_config pull.mode merge-ff-only &&
+	test_config pull.mode ff-only &&
 	git checkout -b other master^ &&
 	>new &&
 	git add new &&
@@ -401,4 +401,15 @@ test_expect_success 'git pull non-fast-forward (merge)' '
 	git pull --merge
 '
 
+test_expect_success 'git pull non-fast-forward (default)' '
+	test_when_finished "git checkout master && git branch -D other test" &&
+	git checkout -b other master^ &&
+	>new &&
+	git add new &&
+	git commit -m new &&
+	git checkout -b test -t other &&
+	git reset --hard master &&
+	test_must_fail git pull
+'
+
 test_done


Felipe Contreras (7):
  pull: rename pull.rebase to pull.mode
  pull: migrate all the tests to pull.mode
  pull: refactor $rebase variable into $mode
  pull: add --merge option
  pull: add merge-ff-only option
  pull: add warning on non-ff merges
  pull: only allow ff merges by default

 Documentation/config.txt     |  42 ++++++++++--------
 Documentation/git-pull.txt   |  28 ++++++++++--
 branch.c                     |   4 +-
 builtin/remote.c             |  14 +++++-
 git-pull.sh                  | 100 +++++++++++++++++++++++++++++++-----------
 t/t3200-branch.sh            |  40 ++++++++---------
 t/t4013-diff-various.sh      |   2 +-
 t/t5500-fetch-pack.sh        |   2 +-
 t/t5505-remote.sh            |   2 +-
 t/t5520-pull.sh              | 101 ++++++++++++++++++++++++++++++-------------
 t/t5524-pull-msg.sh          |   2 +-
 t/t5601-clone.sh             |   4 +-
 t/t5700-clone-reference.sh   |   4 +-
 t/t6022-merge-rename.sh      |  20 ++++-----
 t/t6026-merge-attr.sh        |   2 +-
 t/t6029-merge-subtree.sh     |   6 +--
 t/t6037-merge-ours-theirs.sh |  10 ++---
 17 files changed, 256 insertions(+), 127 deletions(-)

-- 
1.9.2+fc1.19.g85b6256

[PATCH v6 1/7] pull: rename pull.rebase to pull.mode

[Felipe Contreras]

Also 'branch.<name>.rebase' to 'branch.<name>.pullmode'.

This way we can add more modes and the default can be something else,
namely it can be set to merge-ff-only, so eventually we can reject
non-fast-forward merges by default.

The old configurations still work, but get deprecated.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/config.txt   | 39 ++++++++++++++++++++++-----------------
 Documentation/git-pull.txt |  2 +-
 branch.c                   |  4 ++--
 builtin/remote.c           | 14 ++++++++++++--
 git-pull.sh                | 31 +++++++++++++++++++++++++++++--
 t/t3200-branch.sh          | 40 ++++++++++++++++++++--------------------
 t/t5601-clone.sh           |  4 ++--
 7 files changed, 88 insertions(+), 46 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index c26a7c8..c028aeb 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -708,7 +708,7 @@ branch.autosetupmerge::
 branch.autosetuprebase::
 	When a new branch is created with 'git branch' or 'git checkout'
 	that tracks another branch, this variable tells Git to set
-	up pull to rebase instead of merge (see "branch.<name>.rebase").
+	up pull to rebase instead of merge (see "branch.<name>.pullmode").
 	When `never`, rebase is never automatically set to true.
 	When `local`, rebase is set to true for tracked branches of
 	other local branches.
@@ -764,15 +764,17 @@ branch.<name>.mergeoptions::
 	option values containing whitespace characters are currently not
 	supported.
 
-branch.<name>.rebase::
-	When true, rebase the branch <name> on top of the fetched branch,
-	instead of merging the default branch from the default remote when
-	"git pull" is run. See "pull.rebase" for doing this in a non
-	branch-specific manner.
+branch.<name>.pullmode::
+	When "git pull" is run, this determines if it would either merge or
+	rebase the fetched branch. The possible values are 'merge',
+	'rebase', and 'rebase-preserve'. See "pull.mode" for doing this in a
+	non branch-specific manner.
 +
-	When preserve, also pass `--preserve-merges` along to 'git rebase'
-	so that locally committed merge commits will not be flattened
-	by running 'git pull'.
+	When 'rebase-preserve', also pass `--preserve-merges` along to
+	'git rebase' so that locally committed merge commits will not be
+	flattened by running 'git pull'.
++
+	It was named 'branch.<name>.rebase' but that is deprecated now.
 +
 *NOTE*: this is a possibly dangerous operation; do *not* use
 it unless you understand the implications (see linkgit:git-rebase[1]
@@ -1881,15 +1883,18 @@ pretty.<name>::
 	Note that an alias with the same name as a built-in format
 	will be silently ignored.
 
-pull.rebase::
-	When true, rebase branches on top of the fetched branch, instead
-	of merging the default branch from the default remote when "git
-	pull" is run. See "branch.<name>.rebase" for setting this on a
-	per-branch basis.
+pull.mode::
+	When "git pull" is run, this determines if it would either merge or
+	rebase the fetched branch. The possible values are 'merge',
+	'rebase', and 'rebase-preserve'. See "branch.<name>.pullmode" for doing
+	this in a non branch-specific manner.
++
+	When 'rebase-preserve', also pass `--preserve-merges` along to
+	'git rebase' so that locally committed merge commits will not be
+	flattened by running 'git pull'.
++
 +
-	When preserve, also pass `--preserve-merges` along to 'git rebase'
-	so that locally committed merge commits will not be flattened
-	by running 'git pull'.
+	It was named 'pull.rebase' but that is deprecated now.
 +
 *NOTE*: this is a possibly dangerous operation; do *not* use
 it unless you understand the implications (see linkgit:git-rebase[1]
diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index 200eb22..9a91b9f 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt
@@ -117,7 +117,7 @@ locally created merge commits will not be flattened.
 +
 When false, merge the current branch into the upstream branch.
 +
-See `pull.rebase`, `branch.<name>.rebase` and `branch.autosetuprebase` in
+See `pull.mode`, `branch.<name>.pullmode` and `branch.autosetuprebase` in
 linkgit:git-config[1] if you want to make `git pull` always use
 `--rebase` instead of merging.
 +
diff --git a/branch.c b/branch.c
index 723a36b..63ce671 100644
--- a/branch.c
+++ b/branch.c
@@ -71,8 +71,8 @@ void install_branch_config(int flag, const char *local, const char *origin, cons
 
 	if (rebasing) {
 		strbuf_reset(&key);
-		strbuf_addf(&key, "branch.%s.rebase", local);
-		git_config_set(key.buf, "true");
+		strbuf_addf(&key, "branch.%s.pullmode", local);
+		git_config_set(key.buf, "rebase");
 	}
 	strbuf_release(&key);
 
diff --git a/builtin/remote.c b/builtin/remote.c
index b3ab4cf..46d3c4d 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -264,7 +264,7 @@ static int config_read_branches(const char *key, const char *value, void *cb)
 		char *name;
 		struct string_list_item *item;
 		struct branch_info *info;
-		enum { REMOTE, MERGE, REBASE } type;
+		enum { REMOTE, MERGE, REBASE, PULLMODE } type;
 
 		key += 7;
 		if (ends_with(key, ".remote")) {
@@ -276,6 +276,9 @@ static int config_read_branches(const char *key, const char *value, void *cb)
 		} else if (ends_with(key, ".rebase")) {
 			name = xstrndup(key, strlen(key) - 7);
 			type = REBASE;
+		} else if (ends_with(key, ".pullmode")) {
+			name = xstrndup(key, strlen(key) - 9);
+			type = PULLMODE;
 		} else
 			return 0;
 
@@ -299,12 +302,19 @@ static int config_read_branches(const char *key, const char *value, void *cb)
 				space = strchr(value, ' ');
 			}
 			string_list_append(&info->merge, xstrdup(value));
-		} else {
+		} else if (type == REBASE) {
 			int v = git_config_maybe_bool(orig_key, value);
 			if (v >= 0)
 				info->rebase = v;
 			else if (!strcmp(value, "preserve"))
 				info->rebase = 1;
+		} else {
+			if (!strcmp(value, "rebase"))
+				info->rebase = 1;
+			else if (!strcmp(value, "merge"))
+				info->rebase = 0;
+			else if (!strcmp(value, "rebase-preserve"))
+				info->rebase = 1;
 		}
 	}
 	return 0;
diff --git a/git-pull.sh b/git-pull.sh
index 0a5aa2c..3dbf9cf 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -47,10 +47,37 @@ log_arg= verbosity= progress= recurse_submodules= verify_signatures=
 merge_args= edit= rebase_args=
 curr_branch=$(git symbolic-ref -q HEAD)
 curr_branch_short="${curr_branch#refs/heads/}"
-rebase=$(bool_or_string_config branch.$curr_branch_short.rebase)
+mode=$(git config branch.${curr_branch_short}.pullmode)
+if test -z "$mode"
+then
+	mode=$(git config pull.mode)
+fi
+case "$mode" in
+merge)
+	rebase="false"
+	;;
+rebase)
+	rebase="true"
+	;;
+rebase-preserve)
+	rebase="preserve"
+	;;
+'')
+	;;
+*)
+	echo "Invalid value for 'mode'"
+	usage
+	exit 1
+	;;
+esac
+# backwards compatibility
 if test -z "$rebase"
 then
-	rebase=$(bool_or_string_config pull.rebase)
+	rebase=$(bool_or_string_config branch.$curr_branch_short.rebase)
+	if test -z "$rebase"
+	then
+		rebase=$(bool_or_string_config pull.rebase)
+	fi
 fi
 dry_run=
 while :
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index fcdb867..b79aa75 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -559,7 +559,7 @@ test_expect_success 'autosetuprebase local on a tracked local branch' '
 	git branch --track myr1 mybase &&
 	test "$(git config branch.myr1.remote)" = . &&
 	test "$(git config branch.myr1.merge)" = refs/heads/mybase &&
-	test "$(git config branch.myr1.rebase)" = true
+	test "$(git config branch.myr1.pullmode)" = rebase
 '
 
 test_expect_success 'autosetuprebase always on a tracked local branch' '
@@ -571,7 +571,7 @@ test_expect_success 'autosetuprebase always on a tracked local branch' '
 	git branch --track myr2 mybase &&
 	test "$(git config branch.myr2.remote)" = . &&
 	test "$(git config branch.myr2.merge)" = refs/heads/mybase &&
-	test "$(git config branch.myr2.rebase)" = true
+	test "$(git config branch.myr2.pullmode)" = rebase
 '
 
 test_expect_success 'autosetuprebase remote on a tracked local branch' '
@@ -583,7 +583,7 @@ test_expect_success 'autosetuprebase remote on a tracked local branch' '
 	git branch --track myr3 mybase2 &&
 	test "$(git config branch.myr3.remote)" = . &&
 	test "$(git config branch.myr3.merge)" = refs/heads/mybase2 &&
-	! test "$(git config branch.myr3.rebase)" = true
+	! test "$(git config branch.myr3.pullmode)" = rebase
 '
 
 test_expect_success 'autosetuprebase never on a tracked local branch' '
@@ -595,7 +595,7 @@ test_expect_success 'autosetuprebase never on a tracked local branch' '
 	git branch --track myr4 mybase2 &&
 	test "$(git config branch.myr4.remote)" = . &&
 	test "$(git config branch.myr4.merge)" = refs/heads/mybase2 &&
-	! test "$(git config branch.myr4.rebase)" = true
+	! test "$(git config branch.myr4.pullmode)" = rebase
 '
 
 test_expect_success 'autosetuprebase local on a tracked remote branch' '
@@ -606,7 +606,7 @@ test_expect_success 'autosetuprebase local on a tracked remote branch' '
 	git branch --track myr5 local/master &&
 	test "$(git config branch.myr5.remote)" = local &&
 	test "$(git config branch.myr5.merge)" = refs/heads/master &&
-	! test "$(git config branch.myr5.rebase)" = true
+	! test "$(git config branch.myr5.pullmode)" = rebase
 '
 
 test_expect_success 'autosetuprebase never on a tracked remote branch' '
@@ -617,7 +617,7 @@ test_expect_success 'autosetuprebase never on a tracked remote branch' '
 	git branch --track myr6 local/master &&
 	test "$(git config branch.myr6.remote)" = local &&
 	test "$(git config branch.myr6.merge)" = refs/heads/master &&
-	! test "$(git config branch.myr6.rebase)" = true
+	! test "$(git config branch.myr6.pullmode)" = rebase
 '
 
 test_expect_success 'autosetuprebase remote on a tracked remote branch' '
@@ -628,7 +628,7 @@ test_expect_success 'autosetuprebase remote on a tracked remote branch' '
 	git branch --track myr7 local/master &&
 	test "$(git config branch.myr7.remote)" = local &&
 	test "$(git config branch.myr7.merge)" = refs/heads/master &&
-	test "$(git config branch.myr7.rebase)" = true
+	test "$(git config branch.myr7.pullmode)" = rebase
 '
 
 test_expect_success 'autosetuprebase always on a tracked remote branch' '
@@ -639,7 +639,7 @@ test_expect_success 'autosetuprebase always on a tracked remote branch' '
 	git branch --track myr8 local/master &&
 	test "$(git config branch.myr8.remote)" = local &&
 	test "$(git config branch.myr8.merge)" = refs/heads/master &&
-	test "$(git config branch.myr8.rebase)" = true
+	test "$(git config branch.myr8.pullmode)" = rebase
 '
 
 test_expect_success 'autosetuprebase unconfigured on a tracked remote branch' '
@@ -650,7 +650,7 @@ test_expect_success 'autosetuprebase unconfigured on a tracked remote branch' '
 	git branch --track myr9 local/master &&
 	test "$(git config branch.myr9.remote)" = local &&
 	test "$(git config branch.myr9.merge)" = refs/heads/master &&
-	test "z$(git config branch.myr9.rebase)" = z
+	test "z$(git config branch.myr9.pullmode)" = z
 '
 
 test_expect_success 'autosetuprebase unconfigured on a tracked local branch' '
@@ -661,7 +661,7 @@ test_expect_success 'autosetuprebase unconfigured on a tracked local branch' '
 	git branch --track myr10 mybase2 &&
 	test "$(git config branch.myr10.remote)" = . &&
 	test "$(git config branch.myr10.merge)" = refs/heads/mybase2 &&
-	test "z$(git config branch.myr10.rebase)" = z
+	test "z$(git config branch.myr10.pullmode)" = z
 '
 
 test_expect_success 'autosetuprebase unconfigured on untracked local branch' '
@@ -671,7 +671,7 @@ test_expect_success 'autosetuprebase unconfigured on untracked local branch' '
 	git branch --no-track myr11 mybase2 &&
 	test "z$(git config branch.myr11.remote)" = z &&
 	test "z$(git config branch.myr11.merge)" = z &&
-	test "z$(git config branch.myr11.rebase)" = z
+	test "z$(git config branch.myr11.pullmode)" = z
 '
 
 test_expect_success 'autosetuprebase unconfigured on untracked remote branch' '
@@ -681,7 +681,7 @@ test_expect_success 'autosetuprebase unconfigured on untracked remote branch' '
 	git branch --no-track myr12 local/master &&
 	test "z$(git config branch.myr12.remote)" = z &&
 	test "z$(git config branch.myr12.merge)" = z &&
-	test "z$(git config branch.myr12.rebase)" = z
+	test "z$(git config branch.myr12.pullmode)" = z
 '
 
 test_expect_success 'autosetuprebase never on an untracked local branch' '
@@ -692,7 +692,7 @@ test_expect_success 'autosetuprebase never on an untracked local branch' '
 	git branch --no-track myr13 mybase2 &&
 	test "z$(git config branch.myr13.remote)" = z &&
 	test "z$(git config branch.myr13.merge)" = z &&
-	test "z$(git config branch.myr13.rebase)" = z
+	test "z$(git config branch.myr13.pullmode)" = z
 '
 
 test_expect_success 'autosetuprebase local on an untracked local branch' '
@@ -703,7 +703,7 @@ test_expect_success 'autosetuprebase local on an untracked local branch' '
 	git branch --no-track myr14 mybase2 &&
 	test "z$(git config branch.myr14.remote)" = z &&
 	test "z$(git config branch.myr14.merge)" = z &&
-	test "z$(git config branch.myr14.rebase)" = z
+	test "z$(git config branch.myr14.pullmode)" = z
 '
 
 test_expect_success 'autosetuprebase remote on an untracked local branch' '
@@ -714,7 +714,7 @@ test_expect_success 'autosetuprebase remote on an untracked local branch' '
 	git branch --no-track myr15 mybase2 &&
 	test "z$(git config branch.myr15.remote)" = z &&
 	test "z$(git config branch.myr15.merge)" = z &&
-	test "z$(git config branch.myr15.rebase)" = z
+	test "z$(git config branch.myr15.pullmode)" = z
 '
 
 test_expect_success 'autosetuprebase always on an untracked local branch' '
@@ -725,7 +725,7 @@ test_expect_success 'autosetuprebase always on an untracked local branch' '
 	git branch --no-track myr16 mybase2 &&
 	test "z$(git config branch.myr16.remote)" = z &&
 	test "z$(git config branch.myr16.merge)" = z &&
-	test "z$(git config branch.myr16.rebase)" = z
+	test "z$(git config branch.myr16.pullmode)" = z
 '
 
 test_expect_success 'autosetuprebase never on an untracked remote branch' '
@@ -736,7 +736,7 @@ test_expect_success 'autosetuprebase never on an untracked remote branch' '
 	git branch --no-track myr17 local/master &&
 	test "z$(git config branch.myr17.remote)" = z &&
 	test "z$(git config branch.myr17.merge)" = z &&
-	test "z$(git config branch.myr17.rebase)" = z
+	test "z$(git config branch.myr17.pullmode)" = z
 '
 
 test_expect_success 'autosetuprebase local on an untracked remote branch' '
@@ -747,7 +747,7 @@ test_expect_success 'autosetuprebase local on an untracked remote branch' '
 	git branch --no-track myr18 local/master &&
 	test "z$(git config branch.myr18.remote)" = z &&
 	test "z$(git config branch.myr18.merge)" = z &&
-	test "z$(git config branch.myr18.rebase)" = z
+	test "z$(git config branch.myr18.pullmode)" = z
 '
 
 test_expect_success 'autosetuprebase remote on an untracked remote branch' '
@@ -758,7 +758,7 @@ test_expect_success 'autosetuprebase remote on an untracked remote branch' '
 	git branch --no-track myr19 local/master &&
 	test "z$(git config branch.myr19.remote)" = z &&
 	test "z$(git config branch.myr19.merge)" = z &&
-	test "z$(git config branch.myr19.rebase)" = z
+	test "z$(git config branch.myr19.pullmode)" = z
 '
 
 test_expect_success 'autosetuprebase always on an untracked remote branch' '
@@ -769,7 +769,7 @@ test_expect_success 'autosetuprebase always on an untracked remote branch' '
 	git branch --no-track myr20 local/master &&
 	test "z$(git config branch.myr20.remote)" = z &&
 	test "z$(git config branch.myr20.merge)" = z &&
-	test "z$(git config branch.myr20.rebase)" = z
+	test "z$(git config branch.myr20.pullmode)" = z
 '
 
 test_expect_success 'autosetuprebase always on detached HEAD' '
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 5e67035..0e91b67 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -191,8 +191,8 @@ test_expect_success 'clone respects global branch.autosetuprebase' '
 		rm -fr dst &&
 		git clone src dst &&
 		cd dst &&
-		actual="z$(git config branch.master.rebase)" &&
-		test ztrue = $actual
+		actual="$(git config branch.master.pullmode)" &&
+		test "$actual" = rebase
 	)
 '
 
-- 
1.9.2+fc1.19.g85b6256

[PATCH v6 2/7] pull: migrate all the tests to pull.mode

[Felipe Contreras]

And branch.$name.pullmode.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t5505-remote.sh |  2 +-
 t/t5520-pull.sh   | 54 +++++++++++++++++++++++-------------------------------
 2 files changed, 24 insertions(+), 32 deletions(-)

diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index ac79dd9..76376e4 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -181,7 +181,7 @@ test_expect_success 'show' '
 		git branch -d -r origin/master &&
 		git config --add remote.two.url ../two &&
 		git config --add remote.two.pushurl ../three &&
-		git config branch.rebase.rebase true &&
+		git config branch.rebase.pullmode rebase &&
 		git config branch.octopus.merge "topic-a topic-b topic-c" &&
 		(
 			cd ../one &&
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 227d293..01ad17a 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -123,26 +123,26 @@ test_expect_success '--rebase' '
 	test $(git rev-parse HEAD^) = $(git rev-parse copy) &&
 	test new = $(git show HEAD:file2)
 '
-test_expect_success 'pull.rebase' '
+test_expect_success 'pull.mode=rebase' '
 	git reset --hard before-rebase &&
-	test_config pull.rebase true &&
+	test_config pull.mode rebase &&
 	git pull . copy &&
 	test $(git rev-parse HEAD^) = $(git rev-parse copy) &&
 	test new = $(git show HEAD:file2)
 '
 
-test_expect_success 'branch.to-rebase.rebase' '
+test_expect_success 'branch.to-rebase.pullmode=rebase' '
 	git reset --hard before-rebase &&
-	test_config branch.to-rebase.rebase true &&
+	test_config branch.to-rebase.pullmode rebase &&
 	git pull . copy &&
 	test $(git rev-parse HEAD^) = $(git rev-parse copy) &&
 	test new = $(git show HEAD:file2)
 '
 
-test_expect_success 'branch.to-rebase.rebase should override pull.rebase' '
+test_expect_success 'branch.to-rebase.pullmode should override pull.mode' '
 	git reset --hard before-rebase &&
-	test_config pull.rebase true &&
-	test_config branch.to-rebase.rebase false &&
+	test_config pull.mode merge &&
+	test_config branch.to-rebase.pullmode merge &&
 	git pull . copy &&
 	test $(git rev-parse HEAD^) != $(git rev-parse copy) &&
 	test new = $(git show HEAD:file2)
@@ -150,7 +150,7 @@ test_expect_success 'branch.to-rebase.rebase should override pull.rebase' '
 
 # add a feature branch, keep-merge, that is merged into master, so the
 # test can try preserving the merge commit (or not) with various
-# --rebase flags/pull.rebase settings.
+# --rebase flags/pull.mode settings.
 test_expect_success 'preserve merge setup' '
 	git reset --hard before-rebase &&
 	git checkout -b keep-merge second^ &&
@@ -160,48 +160,40 @@ test_expect_success 'preserve merge setup' '
 	git tag before-preserve-rebase
 '
 
-test_expect_success 'pull.rebase=false create a new merge commit' '
+test_expect_success 'pull.mode=merge create a new merge commit' '
 	git reset --hard before-preserve-rebase &&
-	test_config pull.rebase false &&
+	test_config pull.mode merge &&
 	git pull . copy &&
 	test $(git rev-parse HEAD^1) = $(git rev-parse before-preserve-rebase) &&
 	test $(git rev-parse HEAD^2) = $(git rev-parse copy) &&
 	test file3 = $(git show HEAD:file3.t)
 '
 
-test_expect_success 'pull.rebase=true flattens keep-merge' '
+test_expect_success 'pull.mode=rebase flattens keep-merge' '
 	git reset --hard before-preserve-rebase &&
-	test_config pull.rebase true &&
+	test_config pull.mode rebase &&
 	git pull . copy &&
 	test $(git rev-parse HEAD^^) = $(git rev-parse copy) &&
 	test file3 = $(git show HEAD:file3.t)
 '
 
-test_expect_success 'pull.rebase=1 is treated as true and flattens keep-merge' '
+test_expect_success 'pull.mode=rebase-preserve rebases and merges keep-merge' '
 	git reset --hard before-preserve-rebase &&
-	test_config pull.rebase 1 &&
-	git pull . copy &&
-	test $(git rev-parse HEAD^^) = $(git rev-parse copy) &&
-	test file3 = $(git show HEAD:file3.t)
-'
-
-test_expect_success 'pull.rebase=preserve rebases and merges keep-merge' '
-	git reset --hard before-preserve-rebase &&
-	test_config pull.rebase preserve &&
+	test_config pull.mode rebase-preserve &&
 	git pull . copy &&
 	test $(git rev-parse HEAD^^) = $(git rev-parse copy) &&
 	test $(git rev-parse HEAD^2) = $(git rev-parse keep-merge)
 '
 
-test_expect_success 'pull.rebase=invalid fails' '
+test_expect_success 'pull.mode=invalid fails' '
 	git reset --hard before-preserve-rebase &&
-	test_config pull.rebase invalid &&
+	test_config pull.mode invalid &&
 	! git pull . copy
 '
 
 test_expect_success '--rebase=false create a new merge commit' '
 	git reset --hard before-preserve-rebase &&
-	test_config pull.rebase true &&
+	test_config pull.mode rebase &&
 	git pull --rebase=false . copy &&
 	test $(git rev-parse HEAD^1) = $(git rev-parse before-preserve-rebase) &&
 	test $(git rev-parse HEAD^2) = $(git rev-parse copy) &&
@@ -210,7 +202,7 @@ test_expect_success '--rebase=false create a new merge commit' '
 
 test_expect_success '--rebase=true rebases and flattens keep-merge' '
 	git reset --hard before-preserve-rebase &&
-	test_config pull.rebase preserve &&
+	test_config pull.mode rebase-preserve &&
 	git pull --rebase=true . copy &&
 	test $(git rev-parse HEAD^^) = $(git rev-parse copy) &&
 	test file3 = $(git show HEAD:file3.t)
@@ -218,7 +210,7 @@ test_expect_success '--rebase=true rebases and flattens keep-merge' '
 
 test_expect_success '--rebase=preserve rebases and merges keep-merge' '
 	git reset --hard before-preserve-rebase &&
-	test_config pull.rebase true &&
+	test_config pull.mode rebase &&
 	git pull --rebase=preserve . copy &&
 	test $(git rev-parse HEAD^^) = $(git rev-parse copy) &&
 	test $(git rev-parse HEAD^2) = $(git rev-parse keep-merge)
@@ -229,9 +221,9 @@ test_expect_success '--rebase=invalid fails' '
 	! git pull --rebase=invalid . copy
 '
 
-test_expect_success '--rebase overrides pull.rebase=preserve and flattens keep-merge' '
+test_expect_success '--rebase overrides pull.mode=rebase-preserve and flattens keep-merge' '
 	git reset --hard before-preserve-rebase &&
-	test_config pull.rebase preserve &&
+	test_config pull.mode rebase-preserve &&
 	git pull --rebase . copy &&
 	test $(git rev-parse HEAD^^) = $(git rev-parse copy) &&
 	test file3 = $(git show HEAD:file3.t)
@@ -279,7 +271,7 @@ test_expect_success 'rebased upstream + fetch + pull --rebase' '
 
 '
 
-test_expect_success 'pull --rebase dies early with dirty working directory' '
+test_expect_success 'pull --mode=rebase dies early with dirty working directory' '
 
 	git checkout to-rebase &&
 	git update-ref refs/remotes/me/copy copy^ &&
@@ -287,7 +279,7 @@ test_expect_success 'pull --rebase dies early with dirty working directory' '
 	git rebase --onto $COPY copy &&
 	test_config branch.to-rebase.remote me &&
 	test_config branch.to-rebase.merge refs/heads/copy &&
-	test_config branch.to-rebase.rebase true &&
+	test_config branch.to-rebase.pullmode rebase &&
 	echo dirty >> file &&
 	git add file &&
 	test_must_fail git pull &&
-- 
1.9.2+fc1.19.g85b6256

[PATCH v6 3/7] pull: refactor $rebase variable into $mode

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 git-pull.sh | 64 +++++++++++++++++++++++++++++++------------------------------
 1 file changed, 33 insertions(+), 31 deletions(-)

diff --git a/git-pull.sh b/git-pull.sh
index 3dbf9cf..50c612f 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -53,16 +53,11 @@ then
 	mode=$(git config pull.mode)
 fi
 case "$mode" in
-merge)
-	rebase="false"
-	;;
-rebase)
-	rebase="true"
+merge|rebase|'')
 	;;
 rebase-preserve)
-	rebase="preserve"
-	;;
-'')
+	mode="rebase"
+	rebase_args="--preserve-merges"
 	;;
 *)
 	echo "Invalid value for 'mode'"
@@ -71,7 +66,7 @@ rebase-preserve)
 	;;
 esac
 # backwards compatibility
-if test -z "$rebase"
+if test -z "$mode"
 then
 	rebase=$(bool_or_string_config branch.$curr_branch_short.rebase)
 	if test -z "$rebase"
@@ -145,10 +140,10 @@ do
 		rebase="${1#*=}"
 		;;
 	-r|--r|--re|--reb|--reba|--rebas|--rebase)
-		rebase=true
+		mode=rebase
 		;;
 	--no-r|--no-re|--no-reb|--no-reba|--no-rebas|--no-rebase)
-		rebase=false
+		mode=
 		;;
 	--recurse-submodules)
 		recurse_submodules=--recurse-submodules
@@ -179,19 +174,26 @@ do
 	shift
 done
 
-case "$rebase" in
-preserve)
-	rebase=true
-	rebase_args=--preserve-merges
-	;;
-true|false|'')
-	;;
-*)
-	echo "Invalid value for --rebase, should be true, false, or preserve"
-	usage
-	exit 1
-	;;
-esac
+if test -n "$rebase"
+then
+	case "$rebase" in
+	true)
+		mode="rebase"
+		;;
+	false)
+		mode="merge"
+		;;
+	preserve)
+		mode="rebase"
+		rebase_args=--preserve-merges
+		;;
+	*)
+		echo "Invalid value for --rebase, should be true, false, or preserve"
+		usage
+		exit 1
+		;;
+	esac
+fi
 
 error_on_no_merge_candidates () {
 	exec >&2
@@ -205,7 +207,7 @@ error_on_no_merge_candidates () {
 		esac
 	done
 
-	if test true = "$rebase"
+	if test "$mode" = rebase
 	then
 		op_type=rebase
 		op_prep=against
@@ -218,7 +220,7 @@ error_on_no_merge_candidates () {
 	remote=$(git config "branch.$curr_branch_short.remote")
 
 	if [ $# -gt 1 ]; then
-		if [ "$rebase" = true ]; then
+		if [ "$mode" = rebase ]; then
 			printf "There is no candidate for rebasing against "
 		else
 			printf "There are no candidates for merging "
@@ -241,7 +243,7 @@ error_on_no_merge_candidates () {
 	exit 1
 }
 
-test true = "$rebase" && {
+test "$mode" = rebase && {
 	if ! git rev-parse -q --verify HEAD >/dev/null
 	then
 		# On an unborn branch
@@ -298,7 +300,7 @@ case "$merge_head" in
 	then
 		die "$(gettext "Cannot merge multiple branches into empty head")"
 	fi
-	if test true = "$rebase"
+	if test "$mode" = rebase
 	then
 		die "$(gettext "Cannot rebase onto multiple branches")"
 	fi
@@ -319,7 +321,7 @@ then
 	exit
 fi
 
-if test true = "$rebase"
+if test "$mode" = rebase
 then
 	o=$(git show-branch --merge-base $curr_branch $merge_head $oldremoteref)
 	if test "$oldremoteref" = "$o"
@@ -329,8 +331,8 @@ then
 fi
 
 merge_name=$(git fmt-merge-msg $log_arg <"$GIT_DIR/FETCH_HEAD") || exit
-case "$rebase" in
-true)
+case "$mode" in
+rebase)
 	eval="git-rebase $diffstat $strategy_args $merge_args $rebase_args $verbosity"
 	eval="$eval --onto $merge_head ${oldremoteref:-$merge_head}"
 	;;
-- 
1.9.2+fc1.19.g85b6256

[PATCH v6 4/7] pull: add --merge option

[Felipe Contreras]

Also, deprecate --no-rebase since there's no need for it any more.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/git-pull.txt |  8 ++++++--
 git-pull.sh                | 10 +++++++++-
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index 9a91b9f..767bca3 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt
@@ -127,8 +127,12 @@ It rewrites history, which does not bode well when you
 published that history already.  Do *not* use this option
 unless you have read linkgit:git-rebase[1] carefully.
 
---no-rebase::
-	Override earlier --rebase.
+-m::
+--merge::
+	Force a merge.
++
+See `pull.mode`, `branch.<name>.pullmode` in linkgit:git-config[1] if you want
+to make `git pull` always use `--merge`.
 
 Options related to fetching
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~
diff --git a/git-pull.sh b/git-pull.sh
index 50c612f..e7e52ec 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -15,6 +15,10 @@ require_work_tree_exists
 cd_to_toplevel
 
 
+warn () {
+	printf >&2 'warning: %s\n' "$*"
+}
+
 die_conflict () {
     git diff-index --cached --name-status -r --ignore-submodules HEAD --
     if [ $(git config --bool --get advice.resolveConflict || echo true) = "true" ]; then
@@ -142,8 +146,12 @@ do
 	-r|--r|--re|--reb|--reba|--rebas|--rebase)
 		mode=rebase
 		;;
+	-m|--m|--me|--mer|--merg|--merge)
+		mode=merge
+		;;
 	--no-r|--no-re|--no-reb|--no-reba|--no-rebas|--no-rebase)
-		mode=
+		mode=merge
+		warn "$(gettext "--no-rebase is deprecated, please use --merge instead")"
 		;;
 	--recurse-submodules)
 		recurse_submodules=--recurse-submodules
-- 
1.9.2+fc1.19.g85b6256

[PATCH v6 5/7] pull: add merge-ff-only option

[Felipe Contreras]

It is very typical for Git newcomers to inadvertently create merges and
worst; inadvertently pushing them. This is one of the reasons many
experienced users prefer to avoid 'git pull', and recommend newcomers to
avoid it as well.

To avoid these problems and keep 'git pull' useful, it has been
suggested that 'git pull' barfs by default if the merge is
non-fast-forward, which unfortunately would break backwards
compatibility.

This patch leaves everything in place to enable this new mode, but it
only gets enabled if the user specifically configures it; pull.mode =
merge-ff-only.

Later on this mode can be enabled by default.

For the full discussion you can read:

http://thread.gmane.org/gmane.comp.version-control.git/225146/focus=225305

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/config.txt |  7 +++++--
 git-pull.sh              | 15 ++++++++++++++-
 t/t5520-pull.sh          | 36 ++++++++++++++++++++++++++++++++++++
 3 files changed, 55 insertions(+), 3 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index c028aeb..b391ec1 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1886,8 +1886,11 @@ pretty.<name>::
 pull.mode::
 	When "git pull" is run, this determines if it would either merge or
 	rebase the fetched branch. The possible values are 'merge',
-	'rebase', and 'rebase-preserve'. See "branch.<name>.pullmode" for doing
-	this in a non branch-specific manner.
+	'rebase', 'ff-only,' and 'rebase-preserve'.
+	If 'ff-only' is specified, the merge will only succeed if it's
+	fast-forward.
+	See "branch.<name>.pullmode" for doing this in a non branch-specific
+	manner.
 +
 	When 'rebase-preserve', also pass `--preserve-merges` along to
 	'git rebase' so that locally committed merge commits will not be
diff --git a/git-pull.sh b/git-pull.sh
index e7e52ec..2446417 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -57,7 +57,7 @@ then
 	mode=$(git config pull.mode)
 fi
 case "$mode" in
-merge|rebase|'')
+merge|rebase|ff-only|'')
 	;;
 rebase-preserve)
 	mode="rebase"
@@ -78,6 +78,7 @@ then
 		rebase=$(bool_or_string_config pull.rebase)
 	fi
 fi
+test -z "$mode" && mode=merge
 dry_run=
 while :
 do
@@ -313,6 +314,18 @@ case "$merge_head" in
 		die "$(gettext "Cannot rebase onto multiple branches")"
 	fi
 	;;
+*)
+	# check if a non-fast-foward merge would be needed
+	merge_head=${merge_head% }
+	if test "$mode" = 'ff-only' && test -z "$no_ff$ff_only${squash#--no-squash}" &&
+		test -n "$orig_head" &&
+		! git merge-base --is-ancestor "$orig_head" "$merge_head" &&
+		! git merge-base --is-ancestor "$merge_head" "$orig_head"
+	then
+		die "$(gettext "The pull was not fast-forward, please either merge or rebase.
+If unsure, run 'git pull --merge'.")"
+	fi
+	;;
 esac
 
 # Pulling into unborn branch: a shorthand for branching off
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 01ad17a..a548c1b 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -365,4 +365,40 @@ test_expect_success 'git pull --rebase against local branch' '
 	test file = "$(cat file2)"
 '
 
+test_expect_success 'git pull fast-forward (ff-only)' '
+	test_when_finished "git checkout master && git branch -D other test" &&
+	test_config pull.mode ff-only &&
+	git checkout -b other master &&
+	>new &&
+	git add new &&
+	git commit -m new &&
+	git checkout -b test -t other &&
+	git reset --hard master &&
+	git pull
+'
+
+test_expect_success 'git pull non-fast-forward (ff-only)' '
+	test_when_finished "git checkout master && git branch -D other test" &&
+	test_config pull.mode ff-only &&
+	git checkout -b other master^ &&
+	>new &&
+	git add new &&
+	git commit -m new &&
+	git checkout -b test -t other &&
+	git reset --hard master &&
+	test_must_fail git pull
+'
+
+test_expect_success 'git pull non-fast-forward with merge (ff-only)' '
+	test_when_finished "git checkout master && git branch -D other test" &&
+	test_config pull.mode ff-only &&
+	git checkout -b other master^ &&
+	>new &&
+	git add new &&
+	git commit -m new &&
+	git checkout -b test -t other &&
+	git reset --hard master &&
+	git pull --merge
+'
+
 test_done
-- 
1.9.2+fc1.19.g85b6256

[PATCH v6 6/7] pull: add warning on non-ff merges

[Felipe Contreras]

To prepare our uses for the upcoming changes we should warn them and let
them know that they will need to specify a merge or a rebase in the
future (when a non-fast-forward situation arises). Also, let them know
we fallback to 'git pull --merge', so when the obsoletion of this mode
comes, they know what to type without interrupting or changing their
workflow.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/git-pull.txt | 18 ++++++++++++++++++
 git-pull.sh                | 15 ++++++++++++---
 t/t5520-pull.sh            | 14 ++++++++++++++
 3 files changed, 44 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index 767bca3..fe3d15d 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt
@@ -23,6 +23,7 @@ More precisely, 'git pull' runs 'git fetch' with the given
 parameters and calls 'git merge' to merge the retrieved branch
 heads into the current branch.
 With `--rebase`, it runs 'git rebase' instead of 'git merge'.
+With `--merge`, it forces the merge, even if it's non-fast forward.
 
 <repository> should be the name of a remote repository as
 passed to linkgit:git-fetch[1].  <refspec> can name an
@@ -41,11 +42,28 @@ Assume the following history exists and the current branch is
 ------------
 	  A---B---C master on origin
 	 /
+    D---E master
+------------
+
+Then `git pull` will merge in a fast-foward way up to the new master.
+
+------------
+    D---E---A---B---C master, origin/master
+------------
+
+However, a non-fast-foward case looks very different.
+
+------------
+	  A---B---C master on origin
+	 /
     D---E---F---G master
 	^
 	origin/master in your repository
 ------------
 
+By default `git pull` will warn about these situations, however, most likely
+you would want to force a merge, which you can do with `git pull --merge`.
+
 Then "`git pull`" will fetch and replay the changes from the remote
 `master` branch since it diverged from the local `master` (i.e., `E`)
 until its current commit (`C`) on top of `master` and record the
diff --git a/git-pull.sh b/git-pull.sh
index 2446417..c4a0b08 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -78,7 +78,7 @@ then
 		rebase=$(bool_or_string_config pull.rebase)
 	fi
 fi
-test -z "$mode" && mode=merge
+test -z "$mode" && mode=default
 dry_run=
 while :
 do
@@ -317,13 +317,22 @@ case "$merge_head" in
 *)
 	# check if a non-fast-foward merge would be needed
 	merge_head=${merge_head% }
-	if test "$mode" = 'ff-only' && test -z "$no_ff$ff_only${squash#--no-squash}" &&
+	if test -z "$no_ff$ff_only${squash#--no-squash}" &&
 		test -n "$orig_head" &&
 		! git merge-base --is-ancestor "$orig_head" "$merge_head" &&
 		! git merge-base --is-ancestor "$merge_head" "$orig_head"
 	then
-		die "$(gettext "The pull was not fast-forward, please either merge or rebase.
+		case "$mode" in
+		ff-only)
+			die "$(gettext "The pull was not fast-forward, please either merge or rebase.
 If unsure, run 'git pull --merge'.")"
+			;;
+		default)
+			warn "$(gettext "the pull was not fast-forward, in the future you would have to choose
+a merge or a rebase, falling back to old style for now (git pull --merge).
+Read 'git pull --help' for more information.")" >&2
+			;;
+		esac
 	fi
 	;;
 esac
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index a548c1b..c96834e 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -401,4 +401,18 @@ test_expect_success 'git pull non-fast-forward with merge (ff-only)' '
 	git pull --merge
 '
 
+test_expect_success 'git pull non-fast-forward (default)' '
+	test_when_finished "git checkout master && git branch -D other test" &&
+	git checkout -b other master^ &&
+	>new &&
+	git add new &&
+	git commit -m new &&
+	git checkout -b test -t other &&
+	git reset --hard master &&
+	git pull 2> error &&
+	cat error &&
+	grep -q "the pull was not fast-forward" error &&
+	grep -q "in the future you would have to choose" error
+'
+
 test_done
-- 
1.9.2+fc1.19.g85b6256

[PATCH v6 7/7] pull: only allow ff merges by default

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/git-pull.txt   |  2 +-
 git-pull.sh                  | 15 +++------------
 t/t4013-diff-various.sh      |  2 +-
 t/t5500-fetch-pack.sh        |  2 +-
 t/t5520-pull.sh              |  5 +----
 t/t5524-pull-msg.sh          |  2 +-
 t/t5700-clone-reference.sh   |  4 ++--
 t/t6022-merge-rename.sh      | 20 ++++++++++----------
 t/t6026-merge-attr.sh        |  2 +-
 t/t6029-merge-subtree.sh     |  6 +++---
 t/t6037-merge-ours-theirs.sh | 10 +++++-----
 11 files changed, 29 insertions(+), 41 deletions(-)

diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index fe3d15d..968315c 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt
@@ -61,7 +61,7 @@ However, a non-fast-foward case looks very different.
 	origin/master in your repository
 ------------
 
-By default `git pull` will warn about these situations, however, most likely
+By default `git pull` will fail on these situations, however, most likely
 you would want to force a merge, which you can do with `git pull --merge`.
 
 Then "`git pull`" will fetch and replay the changes from the remote
diff --git a/git-pull.sh b/git-pull.sh
index c4a0b08..bccaf27 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -78,7 +78,7 @@ then
 		rebase=$(bool_or_string_config pull.rebase)
 	fi
 fi
-test -z "$mode" && mode=default
+test -z "$mode" && mode=ff-only
 dry_run=
 while :
 do
@@ -317,22 +317,13 @@ case "$merge_head" in
 *)
 	# check if a non-fast-foward merge would be needed
 	merge_head=${merge_head% }
-	if test -z "$no_ff$ff_only${squash#--no-squash}" &&
+	if test "$mode" = 'ff-only' && test -z "$no_ff$ff_only${squash#--no-squash}" &&
 		test -n "$orig_head" &&
 		! git merge-base --is-ancestor "$orig_head" "$merge_head" &&
 		! git merge-base --is-ancestor "$merge_head" "$orig_head"
 	then
-		case "$mode" in
-		ff-only)
-			die "$(gettext "The pull was not fast-forward, please either merge or rebase.
+		die "$(gettext "The pull was not fast-forward, please either merge or rebase.
 If unsure, run 'git pull --merge'.")"
-			;;
-		default)
-			warn "$(gettext "the pull was not fast-forward, in the future you would have to choose
-a merge or a rebase, falling back to old style for now (git pull --merge).
-Read 'git pull --help' for more information.")" >&2
-			;;
-		esac
 	fi
 	;;
 esac
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index e77c09c..1840767 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -64,7 +64,7 @@ test_expect_success setup '
 	export GIT_AUTHOR_DATE GIT_COMMITTER_DATE &&
 
 	git checkout master &&
-	git pull -s ours . side &&
+	git pull --merge -s ours . side &&
 
 	GIT_AUTHOR_DATE="2006-06-26 00:05:00 +0000" &&
 	GIT_COMMITTER_DATE="2006-06-26 00:05:00 +0000" &&
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 5b2b1c2..f735cfe 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -259,7 +259,7 @@ test_expect_success 'clone shallow object count' '
 test_expect_success 'pull in shallow repo with missing merge base' '
 	(
 		cd shallow &&
-		test_must_fail git pull --depth 4 .. A
+		test_must_fail git pull --merge --depth 4 .. A
 	)
 '
 
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index c96834e..dc7749b 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -409,10 +409,7 @@ test_expect_success 'git pull non-fast-forward (default)' '
 	git commit -m new &&
 	git checkout -b test -t other &&
 	git reset --hard master &&
-	git pull 2> error &&
-	cat error &&
-	grep -q "the pull was not fast-forward" error &&
-	grep -q "in the future you would have to choose" error
+	test_must_fail git pull
 '
 
 test_done
diff --git a/t/t5524-pull-msg.sh b/t/t5524-pull-msg.sh
index 8cccecc..ec9f413 100755
--- a/t/t5524-pull-msg.sh
+++ b/t/t5524-pull-msg.sh
@@ -25,7 +25,7 @@ test_expect_success setup '
 test_expect_success pull '
 (
 	cd cloned &&
-	git pull --log &&
+	git pull --merge --log &&
 	git log -2 &&
 	git cat-file commit HEAD >result &&
 	grep Dollar result
diff --git a/t/t5700-clone-reference.sh b/t/t5700-clone-reference.sh
index 6537911..306badf 100755
--- a/t/t5700-clone-reference.sh
+++ b/t/t5700-clone-reference.sh
@@ -94,7 +94,7 @@ cd "$base_dir"
 
 test_expect_success 'pulling changes from origin' \
 'cd C &&
-git pull origin'
+git pull --merge origin'
 
 cd "$base_dir"
 
@@ -109,7 +109,7 @@ cd "$base_dir"
 
 test_expect_success 'pulling changes from origin' \
 'cd D &&
-git pull origin'
+git pull --merge origin'
 
 cd "$base_dir"
 
diff --git a/t/t6022-merge-rename.sh b/t/t6022-merge-rename.sh
index a89dfbe..f63300f 100755
--- a/t/t6022-merge-rename.sh
+++ b/t/t6022-merge-rename.sh
@@ -100,7 +100,7 @@ git checkout master'
 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 --merge . 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 --merge . 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 --merge . 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 --merge . 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 --merge . 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 --merge . 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 --merge . 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 --merge . 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 --merge . 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 --merge . master &&
 	test_path_is_file M &&
 	! {
 		git ls-files -s |
diff --git a/t/t6026-merge-attr.sh b/t/t6026-merge-attr.sh
index 5e43997..5428f19 100755
--- a/t/t6026-merge-attr.sh
+++ b/t/t6026-merge-attr.sh
@@ -172,7 +172,7 @@ test_expect_success 'up-to-date merge without common ancestor' '
 	test_tick &&
 	(
 		cd repo1 &&
-		git pull ../repo2 master
+		git pull --merge ../repo2 master
 	)
 '
 
diff --git a/t/t6029-merge-subtree.sh b/t/t6029-merge-subtree.sh
index 73fc240..0e626d1 100755
--- a/t/t6029-merge-subtree.sh
+++ b/t/t6029-merge-subtree.sh
@@ -69,7 +69,7 @@ test_expect_success 'merge update' '
 	git checkout -b master2 &&
 	git commit -m "update git-gui" &&
 	cd ../git &&
-	git pull -s subtree gui master2 &&
+	git pull --merge -s subtree gui master2 &&
 	git ls-files -s >actual &&
 	(
 		echo "100644 $o3 0	git-gui/git-gui.sh"
@@ -98,7 +98,7 @@ test_expect_success 'initial ambiguous subtree' '
 test_expect_success 'merge using explicit' '
 	cd ../git &&
 	git reset --hard master2 &&
-	git pull -Xsubtree=git-gui gui master2 &&
+	git pull --merge -Xsubtree=git-gui gui master2 &&
 	git ls-files -s >actual &&
 	(
 		echo "100644 $o3 0	git-gui/git-gui.sh"
@@ -111,7 +111,7 @@ test_expect_success 'merge using explicit' '
 test_expect_success 'merge2 using explicit' '
 	cd ../git &&
 	git reset --hard master2 &&
-	git pull -Xsubtree=git-gui2 gui master2 &&
+	git pull --merge -Xsubtree=git-gui2 gui master2 &&
 	git ls-files -s >actual &&
 	(
 		echo "100644 $o1 0	git-gui/git-gui.sh"
diff --git a/t/t6037-merge-ours-theirs.sh b/t/t6037-merge-ours-theirs.sh
index 3889eca..8e67d2a 100755
--- a/t/t6037-merge-ours-theirs.sh
+++ b/t/t6037-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 -m -s recursive -Xours . side &&
+	git reset --hard master && git pull -m -s recursive -X ours . side &&
+	git reset --hard master && git pull -m -s recursive -Xtheirs . side &&
+	git reset --hard master && git pull -m -s recursive -X theirs . side &&
+	git reset --hard master && test_must_fail git pull -m -s recursive -X bork . side
 '
 
 test_done
-- 
1.9.2+fc1.19.g85b6256

Re: [PATCH v6 4/7] pull: add --merge option

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1573 bytes --]

On Thu, May 01, 2014 at 07:00:05PM -0500, Felipe Contreras wrote:
> Also, deprecate --no-rebase since there's no need for it any more.
> 
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  Documentation/git-pull.txt |  8 ++++++--
>  git-pull.sh                | 10 +++++++++-
>  2 files changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
> index 9a91b9f..767bca3 100644
> --- a/Documentation/git-pull.txt
> +++ b/Documentation/git-pull.txt
> @@ -127,8 +127,12 @@ It rewrites history, which does not bode well when you
>  published that history already.  Do *not* use this option
>  unless you have read linkgit:git-rebase[1] carefully.
>  
> ---no-rebase::
> -	Override earlier --rebase.
> +-m::
> +--merge::
> +	Force a merge.
> ++
> +See `pull.mode`, `branch.<name>.pullmode` in linkgit:git-config[1] if you want
> +to make `git pull` always use `--merge`.

So I'm confused here, and maybe you can enlighten me.  As I read this
documentation, --merge would always force a merge, like --no-ff.  If so,
I don't see an option to preserve the existing behavior, which is the
I-don't-care-just-do-it case.  If the behavior is different, then this
documentation needs to be improved, I think, along with the
documentation earlier in the series.

-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v6 4/7] pull: add --merge option

[Felipe Contreras]

brian m. carlson wrote:
> On Thu, May 01, 2014 at 07:00:05PM -0500, Felipe Contreras wrote:
> > Also, deprecate --no-rebase since there's no need for it any more.
> > 
> > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> > ---
> >  Documentation/git-pull.txt |  8 ++++++--
> >  git-pull.sh                | 10 +++++++++-
> >  2 files changed, 15 insertions(+), 3 deletions(-)
> > 
> > diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
> > index 9a91b9f..767bca3 100644
> > --- a/Documentation/git-pull.txt
> > +++ b/Documentation/git-pull.txt
> > @@ -127,8 +127,12 @@ It rewrites history, which does not bode well when you
> >  published that history already.  Do *not* use this option
> >  unless you have read linkgit:git-rebase[1] carefully.
> >  
> > ---no-rebase::
> > -	Override earlier --rebase.
> > +-m::
> > +--merge::
> > +	Force a merge.
> > ++
> > +See `pull.mode`, `branch.<name>.pullmode` in linkgit:git-config[1] if you want
> > +to make `git pull` always use `--merge`.
> 
> So I'm confused here, and maybe you can enlighten me.  As I read this
> documentation, --merge would always force a merge, like --no-ff.  If so,
> I don't see an option to preserve the existing behavior, which is the
> I-don't-care-just-do-it case.  If the behavior is different, then this
> documentation needs to be improved, I think, along with the
> documentation earlier in the series.

I don't understand what is your point.

So basically you think these should be the same?

  % git pull --merge --no-merge --rebase --no-rebase
  % git pull

-- 
Felipe Contreras

Re: [PATCH v6 1/7] pull: rename pull.rebase to pull.mode

[W. Trevor King]

[-- Attachment #1: Type: text/plain, Size: 676 bytes --]

On Thu, May 01, 2014 at 07:00:02PM -0500, Felipe Contreras wrote:
> Also 'branch.<name>.rebase' to 'branch.<name>.pullmode'.

Perhaps this has already been hashed out in a previous version of this
series, but we may want to use pull.update and branch.<name>.update to
match the existing submodule.<name>.update.  Both settings are
selecting the default integration style between HEAD and some other
reference (pull's remote branch, the gitlinked commit, or the
submodule's --remote branch).

Cheers,
Trevor

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v6 5/7] pull: add merge-ff-only option

[W. Trevor King]

[-- Attachment #1: Type: text/plain, Size: 950 bytes --]

On Thu, May 01, 2014 at 07:00:06PM -0500, Felipe Contreras wrote:
> It is very typical for Git newcomers to inadvertently create merges and
> worst; inadvertently pushing them. This is one of the reasons many
> experienced users prefer to avoid 'git pull', and recommend newcomers to
> avoid it as well.
> 
> To avoid these problems and keep 'git pull' useful, it has been
> suggested that 'git pull' barfs by default if the merge is
> non-fast-forward, which unfortunately would break backwards
> compatibility.
> 
> This patch leaves everything in place to enable this new mode, but it
> only gets enabled if the user specifically configures it; pull.mode =
> merge-ff-only.

The subject and commit message also need “merge-ff-only” → “ff-only”.

Cheers,
Trevor

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v6 4/7] pull: add --merge option

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 2509 bytes --]

On Thu, May 01, 2014 at 09:41:34PM -0500, Felipe Contreras wrote:
> brian m. carlson wrote:
> > On Thu, May 01, 2014 at 07:00:05PM -0500, Felipe Contreras wrote:
> > > Also, deprecate --no-rebase since there's no need for it any more.
> > > 
> > > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> > > ---
> > >  Documentation/git-pull.txt |  8 ++++++--
> > >  git-pull.sh                | 10 +++++++++-
> > >  2 files changed, 15 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
> > > index 9a91b9f..767bca3 100644
> > > --- a/Documentation/git-pull.txt
> > > +++ b/Documentation/git-pull.txt
> > > @@ -127,8 +127,12 @@ It rewrites history, which does not bode well when you
> > >  published that history already.  Do *not* use this option
> > >  unless you have read linkgit:git-rebase[1] carefully.
> > >  
> > > ---no-rebase::
> > > -	Override earlier --rebase.
> > > +-m::
> > > +--merge::
> > > +	Force a merge.
> > > ++
> > > +See `pull.mode`, `branch.<name>.pullmode` in linkgit:git-config[1] if you want
> > > +to make `git pull` always use `--merge`.
> > 
> > So I'm confused here, and maybe you can enlighten me.  As I read this
> > documentation, --merge would always force a merge, like --no-ff.  If so,
> > I don't see an option to preserve the existing behavior, which is the
> > I-don't-care-just-do-it case.  If the behavior is different, then this
> > documentation needs to be improved, I think, along with the
> > documentation earlier in the series.
> 
> I don't understand what is your point.
> 
> So basically you think these should be the same?
> 
>   % git pull --merge --no-merge --rebase --no-rebase
>   % git pull

My point is that it's unclear to me what options I need to use to retain
the current behavior (fast-forward if possible, merge otherwise) without
a warning.  Right now, it looks like --merge is equivalent to --no-ff,
which seems silly, since we already have an option for that.

So my request is that you add an option (command-line and configuration)
that maintains the current behavior, or if there's already such an
option, that the documentation be clear enough so that I can figure it
out.  Because right now, it's not.

-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v6 4/7] pull: add --merge option

[Felipe Contreras]

brian m. carlson wrote:
> My point is that it's unclear to me what options I need to use to
> retain the current behavior (fast-forward if possible, merge
> otherwise) without a warning.

The current behavior is to always merge (ff or otherwise), just what
`git merge` would do, so `git pull --merge`. I don't see what's
confusing about that.

-- 
Felipe Contreras

Re: [PATCH v6 4/7] pull: add --merge option

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 957 bytes --]

On Fri, May 02, 2014 at 03:14:44PM -0500, Felipe Contreras wrote:
> brian m. carlson wrote:
> > My point is that it's unclear to me what options I need to use to
> > retain the current behavior (fast-forward if possible, merge
> > otherwise) without a warning.
> 
> The current behavior is to always merge (ff or otherwise), just what
> `git merge` would do, so `git pull --merge`. I don't see what's
> confusing about that.

When the documentation says "Forces a merge" without any clarifying
statement, that implies to me that it always creates a new commit.  I'm
certain that I'm not the only person who is going to think that.

Could you please clarify the documentation for --merge and pull.mode to
avoid confusing users?

-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v6 1/7] pull: rename pull.rebase to pull.mode

[Richard Hansen]

On 2014-05-01 20:00, Felipe Contreras wrote:
> Also 'branch.<name>.rebase' to 'branch.<name>.pullmode'.
> 
> This way we can add more modes and the default can be something else,
> namely it can be set to merge-ff-only, so eventually we can reject
> non-fast-forward merges by default.
> 
> The old configurations still work, but get deprecated.

s/get/are/

> 
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  Documentation/config.txt   | 39 ++++++++++++++++++++++-----------------
>  Documentation/git-pull.txt |  2 +-
>  branch.c                   |  4 ++--
>  builtin/remote.c           | 14 ++++++++++++--
>  git-pull.sh                | 31 +++++++++++++++++++++++++++++--
>  t/t3200-branch.sh          | 40 ++++++++++++++++++++--------------------
>  t/t5601-clone.sh           |  4 ++--
>  7 files changed, 88 insertions(+), 46 deletions(-)
> 
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index c26a7c8..c028aeb 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -708,7 +708,7 @@ branch.autosetupmerge::
>  branch.autosetuprebase::
>  	When a new branch is created with 'git branch' or 'git checkout'
>  	that tracks another branch, this variable tells Git to set
> -	up pull to rebase instead of merge (see "branch.<name>.rebase").
> +	up pull to rebase instead of merge (see "branch.<name>.pullmode").
>  	When `never`, rebase is never automatically set to true.
>  	When `local`, rebase is set to true for tracked branches of
>  	other local branches.

Should branch.autosetuprebase be replaced with a new
branch.autosetupmode setting?

> @@ -764,15 +764,17 @@ branch.<name>.mergeoptions::
>  	option values containing whitespace characters are currently not
>  	supported.
>  
> -branch.<name>.rebase::
> -	When true, rebase the branch <name> on top of the fetched branch,
> -	instead of merging the default branch from the default remote when
> -	"git pull" is run. See "pull.rebase" for doing this in a non
> -	branch-specific manner.
> +branch.<name>.pullmode::
> +	When "git pull" is run, this determines if it would either merge or
> +	rebase the fetched branch.

To me this sentence implies that 'rebase' would rebase the fetched
branch onto HEAD, when it's actually the other way around.

>                                  The possible values are 'merge',
> +	'rebase', and 'rebase-preserve'.

While the name 'merge' is mostly self-explanatory, I think it needs
further clarification:  Does 'merge' imply --no-ff?  Or --ff?  Or the
value of merge.ff?  Which side will be the first parent?

Similarly, 'rebase' could use some clarification:
  * the local branch is rebased onto the pulled branch, not the other
    way around
  * it doesn't simply do 'git rebase FETCH_HEAD' -- it also walks the
    reflog of the upstream ref until it finds an ancestor of the local
    branch

>                                        See "pull.mode" for doing this in a
> +	non branch-specific manner.

I find this sentence to be a bit unclear and would prefer something
like:  "Defaults to the value of pull.mode."

>  +
> -	When preserve, also pass `--preserve-merges` along to 'git rebase'
> -	so that locally committed merge commits will not be flattened
> -	by running 'git pull'.
> +	When 'rebase-preserve', also pass `--preserve-merges` along to
> +	'git rebase' so that locally committed merge commits will not be
> +	flattened by running 'git pull'.
> ++
> +	It was named 'branch.<name>.rebase' but that is deprecated now.

To me this sentence implies that .rebase was simply renamed to .pullmode
with no other changes.  I'd prefer something like this:

branch.<name>.rebase::
    Deprecated in favor of branch.<name>.pullmode.

(Same goes for pull.rebase.)

>  +
>  *NOTE*: this is a possibly dangerous operation; do *not* use
>  it unless you understand the implications (see linkgit:git-rebase[1]
> @@ -1881,15 +1883,18 @@ pretty.<name>::
>  	Note that an alias with the same name as a built-in format
>  	will be silently ignored.
>  
> -pull.rebase::
> -	When true, rebase branches on top of the fetched branch, instead
> -	of merging the default branch from the default remote when "git
> -	pull" is run. See "branch.<name>.rebase" for setting this on a
> -	per-branch basis.
> +pull.mode::
> +	When "git pull" is run, this determines if it would either merge or
> +	rebase the fetched branch. The possible values are 'merge',
> +	'rebase', and 'rebase-preserve'. See "branch.<name>.pullmode" for doing
> +	this in a non branch-specific manner.
> ++
> +	When 'rebase-preserve', also pass `--preserve-merges` along to
> +	'git rebase' so that locally committed merge commits will not be
> +	flattened by running 'git pull'.
> ++
>  +
> -	When preserve, also pass `--preserve-merges` along to 'git rebase'
> -	so that locally committed merge commits will not be flattened
> -	by running 'git pull'.

The default value should be documented.  Also, rather than copy+paste
the description from branch.<name>.pullmode, I'd prefer a brief
reference.  For example:

pull.mode::
    See branch.<name>.pullmode.  Defaults to 'merge'.

-Richard

Re: [PATCH v6 1/7] pull: rename pull.rebase to pull.mode

[Felipe Contreras]

Richard Hansen wrote:
> On 2014-05-01 20:00, Felipe Contreras wrote:
> > Also 'branch.<name>.rebase' to 'branch.<name>.pullmode'.
> > 
> > This way we can add more modes and the default can be something else,
> > namely it can be set to merge-ff-only, so eventually we can reject
> > non-fast-forward merges by default.
> > 
> > The old configurations still work, but get deprecated.
> 
> s/get/are/
> 
> > 
> > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> > ---
> >  Documentation/config.txt   | 39 ++++++++++++++++++++++-----------------
> >  Documentation/git-pull.txt |  2 +-
> >  branch.c                   |  4 ++--
> >  builtin/remote.c           | 14 ++++++++++++--
> >  git-pull.sh                | 31 +++++++++++++++++++++++++++++--
> >  t/t3200-branch.sh          | 40 ++++++++++++++++++++--------------------
> >  t/t5601-clone.sh           |  4 ++--
> >  7 files changed, 88 insertions(+), 46 deletions(-)
> > 
> > diff --git a/Documentation/config.txt b/Documentation/config.txt
> > index c26a7c8..c028aeb 100644
> > --- a/Documentation/config.txt
> > +++ b/Documentation/config.txt
> > @@ -708,7 +708,7 @@ branch.autosetupmerge::
> >  branch.autosetuprebase::
> >  	When a new branch is created with 'git branch' or 'git checkout'
> >  	that tracks another branch, this variable tells Git to set
> > -	up pull to rebase instead of merge (see "branch.<name>.rebase").
> > +	up pull to rebase instead of merge (see "branch.<name>.pullmode").
> >  	When `never`, rebase is never automatically set to true.
> >  	When `local`, rebase is set to true for tracked branches of
> >  	other local branches.
> 
> Should branch.autosetuprebase be replaced with a new
> branch.autosetupmode setting?

Maybe. But if so, I think that should be done in another series.
Otherwise we'll never have a chance to change anything.

> > @@ -764,15 +764,17 @@ branch.<name>.mergeoptions::
> >  	option values containing whitespace characters are currently not
> >  	supported.
> >  
> > -branch.<name>.rebase::
> > -	When true, rebase the branch <name> on top of the fetched branch,
> > -	instead of merging the default branch from the default remote when
> > -	"git pull" is run. See "pull.rebase" for doing this in a non
> > -	branch-specific manner.
> > +branch.<name>.pullmode::
> > +	When "git pull" is run, this determines if it would either merge or
> > +	rebase the fetched branch.
> 
> To me this sentence implies that 'rebase' would rebase the fetched
> branch onto HEAD, when it's actually the other way around.

Right.

This actually interesting mode of thinking:

a) git pull --rebase

We want to rebase HEAD onto @{upstream}.

b) git pull --merge

We want to merge HEAD into @{upstream}. (Why are we doing the opposite?)

c) git pull --rebase github john

We weant to rebase github/john onto HEAD. (We are doing the opposite?)

d) git pull --merge github john

We want to merge github/john into HEAD.

> >                                  The possible values are 'merge',
> > +	'rebase', and 'rebase-preserve'.
> 
> While the name 'merge' is mostly self-explanatory, I think it needs
> further clarification:  Does 'merge' imply --no-ff?  Or --ff?  Or the
> value of merge.ff?

'pull.mode=merge' will do the same as `git merge`, I don't see where or
how it can be explained more clearly.

> Which side will be the first parent?

The same as things currently are: the pulled branch into the current
branch (current branch is first parent).

We should probably change this, but that's out of scope of this series,
and hasn't been decided yet.

> >                                        See "pull.mode" for doing this in a
> > +	non branch-specific manner.
> 
> I find this sentence to be a bit unclear and would prefer something
> like:  "Defaults to the value of pull.mode."

Hmm, might make sense.

> >  +
> > -	When preserve, also pass `--preserve-merges` along to 'git rebase'
> > -	so that locally committed merge commits will not be flattened
> > -	by running 'git pull'.
> > +	When 'rebase-preserve', also pass `--preserve-merges` along to
> > +	'git rebase' so that locally committed merge commits will not be
> > +	flattened by running 'git pull'.
> > ++
> > +	It was named 'branch.<name>.rebase' but that is deprecated now.
> 
> To me this sentence implies that .rebase was simply renamed to .pullmode
> with no other changes.  I'd prefer something like this:
> 
> branch.<name>.rebase::
>     Deprecated in favor of branch.<name>.pullmode.
> 
> (Same goes for pull.rebase.)

Right.

> >  +
> >  *NOTE*: this is a possibly dangerous operation; do *not* use
> >  it unless you understand the implications (see linkgit:git-rebase[1]
> > @@ -1881,15 +1883,18 @@ pretty.<name>::
> >  	Note that an alias with the same name as a built-in format
> >  	will be silently ignored.
> >  
> > -pull.rebase::
> > -	When true, rebase branches on top of the fetched branch, instead
> > -	of merging the default branch from the default remote when "git
> > -	pull" is run. See "branch.<name>.rebase" for setting this on a
> > -	per-branch basis.
> > +pull.mode::
> > +	When "git pull" is run, this determines if it would either merge or
> > +	rebase the fetched branch. The possible values are 'merge',
> > +	'rebase', and 'rebase-preserve'. See "branch.<name>.pullmode" for doing
> > +	this in a non branch-specific manner.
> > ++
> > +	When 'rebase-preserve', also pass `--preserve-merges` along to
> > +	'git rebase' so that locally committed merge commits will not be
> > +	flattened by running 'git pull'.
> > ++
> >  +
> > -	When preserve, also pass `--preserve-merges` along to 'git rebase'
> > -	so that locally committed merge commits will not be flattened
> > -	by running 'git pull'.
> 
> The default value should be documented.  Also, rather than copy+paste
> the description from branch.<name>.pullmode, I'd prefer a brief
> reference.  For example:
> 
> pull.mode::
>     See branch.<name>.pullmode.  Defaults to 'merge'.

I'd say pull.mode is the important one.

-- 
Felipe Contreras

Re: [PATCH v6 1/7] pull: rename pull.rebase to pull.mode

[Richard Hansen]

On 2014-05-02 17:12, Felipe Contreras wrote:
> Richard Hansen wrote:
>> Should branch.autosetuprebase be replaced with a new
>> branch.autosetupmode setting?
> 
> Maybe. But if so, I think that should be done in another series.
> Otherwise we'll never have a chance to change anything.

Sure.

>>>                                  The possible values are 'merge',
>>> +	'rebase', and 'rebase-preserve'.
>>
>> While the name 'merge' is mostly self-explanatory, I think it needs
>> further clarification:  Does 'merge' imply --no-ff?  Or --ff?  Or the
>> value of merge.ff?
> 
> 'pull.mode=merge' will do the same as `git merge`, I don't see where or
> how it can be explained more clearly.

How about:

branch.<name>.pullmode::
	Determines how 'git pull' integrates the fetched branch into
	branch <name>.
	Defaults to the value of `pull.mode`.
	Supported values:
+
--
	`merge`:::
		Merge the fetched branch into <name>.
		See also `merge.ff`.
	`rebase`:::
		Find the point at which <name> forked from the fetched
		branch (see the `--fork-point` option of
		linkgit:git-merge-base[1]), then rebase the commits
		between the fork point and branch <name> onto the
		fetched branch.
	`rebase-preserve`:::
		Like `rebase` but passes `--preserve-merges` to 'git
		rebase'.
--
+
*NOTE*: `rebase` and `rebase-preserve` are potentially dangerous; do
*not* use them unless you understand the implications (see
linkgit:git-rebase[1] for details).

pull.mode::
	See `branch.<name>.pullmode`.  Defaults to `merge`.

> 
>> Which side will be the first parent?
> 
> The same as things currently are: the pulled branch into the current
> branch (current branch is first parent).

This was a rhetorical question -- I was trying to point out that the
current behavior should be documented.

> 
> We should probably change this, but that's out of scope of this series,
> and hasn't been decided yet.

Agreed.  If this series is merged, a future series could add a
'merge-there' pull mode.

>> Also, rather than copy+paste
>> the description from branch.<name>.pullmode, I'd prefer a brief
>> reference.  For example:
>>
>> pull.mode::
>>     See branch.<name>.pullmode.  Defaults to 'merge'.
> 
> I'd say pull.mode is the important one.

Either way works for me.  How about this:

branch.<name>.pullmode::
	Overrides the value of `pull.mode` for branch <name>.

pull.mode::
	Determines how 'git pull' integrates the fetched branch into
	the current branch.
	Supported values:
+
--
	`merge`:::
		(default)
		Merge the fetched branch into the current branch.
		See also `merge.ff`.
	`rebase`:::
		Find the point at which the current branch forked from
		the fetched branch (see the `--fork-point` option of
		linkgit:git-merge-base[1]), then rebase the commits
		between the fork point and the current branch onto the
		fetched branch.
	`rebase-preserve`:::
		Like `rebase` but passes `--preserve-merges` to 'git
		rebase'.
--
+
*NOTE*: `rebase` and `rebase-preserve` are potentially dangerous; do
*not* use them unless you understand the implications (see
linkgit:git-rebase[1] for details).

-Richard