[PATCH v3 0/5] Preparation for non-ff pulls by default

[PATCH v3 0/5] Preparation for non-ff pulls by default

[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 series 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 (e.g. in v2.0).

To achieve that first some configurations are renamed: for example: pull.rebase
=> pull.mode = rebase, but the old ones remain functional, thus there are no
functional changes.

Then the new mode 'merge-ff-only' is added, but not enabled by default, and
finally, a warning is added so the users will know what to do when the new
mode is indeed enabled by default.

Felipe Contreras (5):
  pull: rename pull.rename 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

 Documentation/config.txt   | 24 ++++++++++--------
 Documentation/git-pull.txt | 10 +++++---
 branch.c                   |  4 +--
 git-pull.sh                | 63 +++++++++++++++++++++++++++++++++++++---------
 t/t3200-branch.sh          | 40 ++++++++++++++---------------
 t/t5520-pull.sh            | 62 +++++++++++++++++++++++++++++++++++++++++++++
 t/t5601-clone.sh           |  4 +--
 7 files changed, 157 insertions(+), 50 deletions(-)

-- 
1.8.4-338-gefd7fa6

[PATCH v3 1/5] pull: rename pull.rename to pull.mode

[Felipe Contreras]

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

This way 'pull.mode' can be set to 'merge', and the default can be
something else.

The old configurations still work, but get deprecated.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/config.txt   | 22 +++++++++++-----------
 Documentation/git-pull.txt |  2 +-
 branch.c                   |  4 ++--
 git-pull.sh                | 18 ++++++++++++++++--
 t/t3200-branch.sh          | 40 ++++++++++++++++++++--------------------
 t/t5520-pull.sh            | 26 ++++++++++++++++++++++++++
 t/t5601-clone.sh           |  4 ++--
 7 files changed, 78 insertions(+), 38 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index ec57a15..9489a59 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -706,7 +706,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.
@@ -760,11 +760,11 @@ 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' and
+	'rebase'. See "pull.mode" for doing this in a non branch-specific
+	manner.
 +
 *NOTE*: this is a possibly dangerous operation; do *not* use
 it unless you understand the implications (see linkgit:git-rebase[1]
@@ -1820,11 +1820,11 @@ 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' and
+	'rebase'. See "branch.<name>.pullmode" for doing this in a non
+	branch-specific manner.
 +
 *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 6ef8d59..991352f 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt
@@ -109,7 +109,7 @@ include::merge-options.txt[]
 	fetched, the rebase uses that information to avoid rebasing
 	non-local changes.
 +
-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 c5c6984..c6b70c2 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/git-pull.sh b/git-pull.sh
index f0df41c..de57c1d 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -43,10 +43,24 @@ log_arg= verbosity= progress= recurse_submodules= verify_signatures=
 merge_args= edit=
 curr_branch=$(git symbolic-ref -q HEAD)
 curr_branch_short="${curr_branch#refs/heads/}"
-rebase=$(git config --bool branch.$curr_branch_short.rebase)
+mode=$(git config branch.${curr_branch_short}.pullmode)
+if test -z "$mode"
+then
+	mode=$(git config pull.mode)
+fi
+test "$mode" == 'rebase' && rebase="true"
 if test -z "$rebase"
 then
-	rebase=$(git config --bool pull.rebase)
+	rebase=$(git config --bool branch.$curr_branch_short.rebase)
+	if test -z "$rebase"
+	then
+		rebase=$(git config --bool pull.rebase)
+	fi
+	if test "$rebase" = 'true'
+	then
+		echo "The configurations pull.rebase and branch.<name>.rebase are deprecated."
+		echo "Please use pull.mode and branch.<name>.pullmode instead."
+	fi
 fi
 dry_run=
 while :
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 44ec6a4..b620798 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -557,7 +557,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' '
@@ -569,7 +569,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' '
@@ -581,7 +581,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' '
@@ -593,7 +593,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' '
@@ -604,7 +604,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' '
@@ -615,7 +615,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' '
@@ -626,7 +626,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' '
@@ -637,7 +637,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' '
@@ -648,7 +648,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' '
@@ -659,7 +659,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' '
@@ -669,7 +669,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' '
@@ -679,7 +679,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' '
@@ -690,7 +690,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' '
@@ -701,7 +701,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' '
@@ -712,7 +712,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' '
@@ -723,7 +723,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' '
@@ -734,7 +734,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' '
@@ -745,7 +745,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' '
@@ -756,7 +756,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' '
@@ -767,7 +767,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/t5520-pull.sh b/t/t5520-pull.sh
index ed4d9c8..978a3c1 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -284,4 +284,30 @@ test_expect_success 'git pull --rebase against local branch' '
 	test file = "$(cat file2)"
 '
 
+test_expect_success 'pull.mode' '
+	git checkout to-rebase &&
+	git reset --hard before-rebase &&
+	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.pullmode' '
+	git reset --hard before-rebase &&
+	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.pullmode should override pull.mode' '
+	git reset --hard before-rebase &&
+	test_config pull.mode rebase &&
+	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)
+'
+
 test_done
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 0629149..6c888ae 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.8.4-338-gefd7fa6

[PATCH v3 2/5] pull: refactor $rebase variable into $mode

[Felipe Contreras]

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

diff --git a/git-pull.sh b/git-pull.sh
index de57c1d..f53d193 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -48,8 +48,7 @@ if test -z "$mode"
 then
 	mode=$(git config pull.mode)
 fi
-test "$mode" == 'rebase' && rebase="true"
-if test -z "$rebase"
+if test -z "$mode"
 then
 	rebase=$(git config --bool branch.$curr_branch_short.rebase)
 	if test -z "$rebase"
@@ -58,6 +57,7 @@ then
 	fi
 	if test "$rebase" = 'true'
 	then
+		mode="rebase"
 		echo "The configurations pull.rebase and branch.<name>.rebase are deprecated."
 		echo "Please use pull.mode and branch.<name>.pullmode instead."
 	fi
@@ -125,10 +125,10 @@ do
 		merge_args="$merge_args$xx "
 		;;
 	-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
@@ -171,7 +171,7 @@ error_on_no_merge_candidates () {
 		esac
 	done
 
-	if test true = "$rebase"
+	if test "$mode" = rebase
 	then
 		op_type=rebase
 		op_prep=against
@@ -185,7 +185,7 @@ error_on_no_merge_candidates () {
 	remote=$(git config "branch.$curr_branch.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 "
@@ -208,7 +208,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
@@ -273,7 +273,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
@@ -294,7 +294,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"
@@ -304,8 +304,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 $verbosity"
 	eval="$eval --onto $merge_head ${oldremoteref:-$merge_head}"
 	;;
-- 
1.8.4-338-gefd7fa6

[PATCH v3 3/5] 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                | 6 +++++-
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index 991352f..e09f004 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt
@@ -119,8 +119,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 f53d193..fbb8a9b 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -127,8 +127,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
+		echo "--no-rebase is deprecated, please use --merge instead"
 		;;
 	--recurse-submodules)
 		recurse_submodules=--recurse-submodules
-- 
1.8.4-338-gefd7fa6

[PATCH v3 4/5] 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 (e.g. in v2.0).

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 |  8 +++++---
 git-pull.sh              | 12 ++++++++++++
 t/t5520-pull.sh          | 36 ++++++++++++++++++++++++++++++++++++
 3 files changed, 53 insertions(+), 3 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 9489a59..13635e0 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1822,9 +1822,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' and
-	'rebase'. See "branch.<name>.pullmode" for doing this in a non
-	branch-specific manner.
+	rebase the fetched branch. The possible values are 'merge',
+	'rebase', and 'merge-ff-only'. If 'merge-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.
 +
 *NOTE*: this is a possibly dangerous operation; do *not* use
 it unless you understand the implications (see linkgit:git-rebase[1]
diff --git a/git-pull.sh b/git-pull.sh
index fbb8a9b..580d633 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -62,6 +62,7 @@ then
 		echo "Please use pull.mode and branch.<name>.pullmode instead."
 	fi
 fi
+test -z "$mode" && mode=merge
 dry_run=
 while :
 do
@@ -282,6 +283,17 @@ 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" = merge-ff-only -a -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 "The pull was not fast-forward, please either merge or rebase."
+	fi
+	;;
 esac
 
 # Pulling into unborn branch: a shorthand for branching off
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 978a3c1..798ae2f 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -310,4 +310,40 @@ test_expect_success 'branch.to-rebase.pullmode should override pull.mode' '
 	test new = $(git show HEAD:file2)
 '
 
+test_expect_success 'git pull fast-forward' '
+	test_when_finished "git checkout master && git branch -D other test" &&
+	test_config pull.mode merge-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' '
+	test_when_finished "git checkout master && git branch -D other test" &&
+	test_config pull.mode merge-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 (merge)' '
+	test_when_finished "git checkout master && git branch -D other test" &&
+	test_config pull.mode merge-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.8.4-338-gefd7fa6

[PATCH v3 5/5] 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>
---
 git-pull.sh | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/git-pull.sh b/git-pull.sh
index 580d633..1f777de 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -62,7 +62,7 @@ then
 		echo "Please use pull.mode and branch.<name>.pullmode instead."
 	fi
 fi
-test -z "$mode" && mode=merge
+test -z "$mode" && mode=default
 dry_run=
 while :
 do
@@ -286,12 +286,21 @@ 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 -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 "The pull was not fast-forward, please either merge or rebase."
+		case "$mode" in
+		merge-ff-only)
+			die "The pull was not fast-forward, please either merge or rebase."
+			;;
+		default)
+			say "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."
+			;;
+		esac
 	fi
 	;;
 esac
-- 
1.8.4-338-gefd7fa6

Re: [PATCH v3 5/5] pull: add warning on non-ff merges

[Felipe Contreras]

On Sun, Sep 8, 2013 at 8:23 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> 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>

On top of this, we probably would want to update the documentation, so
the user can already read about what's going on when they type 'git
pull --help' as the warning suggests.

commit 60a2f5de7da2e119b858c7ab57b5f02cce9bdb9f
Author: Felipe Contreras <felipe.contreras@gmail.com>
Date:   Sun Sep 8 20:40:21 2013 -0500

    pull: add documentation about non-ff merges

    Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>

diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index 6d55737..d503924 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,9 +42,26 @@ Assume the following history exists and the current branch is
 ------------
          A---B---C origin/master
         /
+    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 origin/master
+        /
     D---E---F---G master
 ------------

+In the future, `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
 `master` branch since it diverged from the local `master` (i.e., `E`)
 until its current commit (`C`) on top of `master` and record the

-- 
Felipe Contreras

Re: [PATCH v3 1/5] pull: rename pull.rename to pull.mode

[Richard Hansen]

On 2013-09-08 21:23, Felipe Contreras wrote:
> The old configurations still work, but get deprecated.

Should some tests for the deprecated configs be added?  We wouldn't want
to accidentally break those.

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index ec57a15..9489a59 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -760,11 +760,11 @@ 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' and
> +	'rebase'. See "pull.mode" for doing this in a non branch-specific
> +	manner.
>  +
>  *NOTE*: this is a possibly dangerous operation; do *not* use
>  it unless you understand the implications (see linkgit:git-rebase[1]
> @@ -1820,11 +1820,11 @@ 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' and
> +	'rebase'. See "branch.<name>.pullmode" for doing this in a non
> +	branch-specific manner.
>  +
>  *NOTE*: this is a possibly dangerous operation; do *not* use
>  it unless you understand the implications (see linkgit:git-rebase[1]

Somewhere something should mention what the default values are
(branch.<name>.pullmode defaults to pull.mode and pull.mode defaults to
merge).

> diff --git a/git-pull.sh b/git-pull.sh
> index f0df41c..de57c1d 100755
> --- a/git-pull.sh
> +++ b/git-pull.sh
> @@ -43,10 +43,24 @@ log_arg= verbosity= progress= recurse_submodules= verify_signatures=
>  merge_args= edit=
>  curr_branch=$(git symbolic-ref -q HEAD)
>  curr_branch_short="${curr_branch#refs/heads/}"
> -rebase=$(git config --bool branch.$curr_branch_short.rebase)
> +mode=$(git config branch.${curr_branch_short}.pullmode)
> +if test -z "$mode"
> +then
> +	mode=$(git config pull.mode)
> +fi
> +test "$mode" == 'rebase' && rebase="true"
>  if test -z "$rebase"
>  then
> -	rebase=$(git config --bool pull.rebase)
> +	rebase=$(git config --bool branch.$curr_branch_short.rebase)
> +	if test -z "$rebase"
> +	then
> +		rebase=$(git config --bool pull.rebase)
> +	fi
> +	if test "$rebase" = 'true'
> +	then
> +		echo "The configurations pull.rebase and branch.<name>.rebase are deprecated."
> +		echo "Please use pull.mode and branch.<name>.pullmode instead."
> +	fi
>  fi
>  dry_run=
>  while :

These deprecation warning messages should be written to stderr, and
should probably be prefixed with "WARNING: ".

-Richard

Re: [PATCH v3 1/5] pull: rename pull.rename to pull.mode

[Felipe Contreras]

On Mon, Sep 9, 2013 at 4:23 PM, Richard Hansen <rhansen@bbn.com> wrote:
> On 2013-09-08 21:23, Felipe Contreras wrote:
>> The old configurations still work, but get deprecated.
>
> Should some tests for the deprecated configs be added?  We wouldn't want
> to accidentally break those.

Probably, but Junio is not picking this patch anyway.

>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> index ec57a15..9489a59 100644
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -760,11 +760,11 @@ 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' and
>> +     'rebase'. See "pull.mode" for doing this in a non branch-specific
>> +     manner.
>>  +
>>  *NOTE*: this is a possibly dangerous operation; do *not* use
>>  it unless you understand the implications (see linkgit:git-rebase[1]
>> @@ -1820,11 +1820,11 @@ 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' and
>> +     'rebase'. See "branch.<name>.pullmode" for doing this in a non
>> +     branch-specific manner.
>>  +
>>  *NOTE*: this is a possibly dangerous operation; do *not* use
>>  it unless you understand the implications (see linkgit:git-rebase[1]
>
> Somewhere something should mention what the default values are
> (branch.<name>.pullmode defaults to pull.mode and pull.mode defaults to
> merge).
>
>> diff --git a/git-pull.sh b/git-pull.sh
>> index f0df41c..de57c1d 100755
>> --- a/git-pull.sh
>> +++ b/git-pull.sh
>> @@ -43,10 +43,24 @@ log_arg= verbosity= progress= recurse_submodules= verify_signatures=
>>  merge_args= edit=
>>  curr_branch=$(git symbolic-ref -q HEAD)
>>  curr_branch_short="${curr_branch#refs/heads/}"
>> -rebase=$(git config --bool branch.$curr_branch_short.rebase)
>> +mode=$(git config branch.${curr_branch_short}.pullmode)
>> +if test -z "$mode"
>> +then
>> +     mode=$(git config pull.mode)
>> +fi
>> +test "$mode" == 'rebase' && rebase="true"
>>  if test -z "$rebase"
>>  then
>> -     rebase=$(git config --bool pull.rebase)
>> +     rebase=$(git config --bool branch.$curr_branch_short.rebase)
>> +     if test -z "$rebase"
>> +     then
>> +             rebase=$(git config --bool pull.rebase)
>> +     fi
>> +     if test "$rebase" = 'true'
>> +     then
>> +             echo "The configurations pull.rebase and branch.<name>.rebase are deprecated."
>> +             echo "Please use pull.mode and branch.<name>.pullmode instead."
>> +     fi
>>  fi
>>  dry_run=
>>  while :
>
> These deprecation warning messages should be written to stderr, and
> should probably be prefixed with "WARNING: ".

Is there any deprecation warning that works this way?

-- 
Felipe Contreras

Re: [PATCH v3 1/5] pull: rename pull.rename to pull.mode

[Jeff King]

On Mon, Sep 09, 2013 at 05:49:36PM -0500, Felipe Contreras wrote:

> > These deprecation warning messages should be written to stderr, and
> > should probably be prefixed with "WARNING: ".
> 
> Is there any deprecation warning that works this way?

The ones in C code typically use warning(), which will prefix with
"warning:" and write to stderr. They do not use all-caps, though.

Try "git log --grep=deprecate -Swarning" for some examples.

-Peff

Re: [PATCH v3 1/5] pull: rename pull.rename to pull.mode

[Felipe Contreras]

On Mon, Sep 9, 2013 at 9:21 PM, Jeff King <peff@peff.net> wrote:
> On Mon, Sep 09, 2013 at 05:49:36PM -0500, Felipe Contreras wrote:
>
>> > These deprecation warning messages should be written to stderr, and
>> > should probably be prefixed with "WARNING: ".
>>
>> Is there any deprecation warning that works this way?
>
> The ones in C code typically use warning(), which will prefix with
> "warning:" and write to stderr. They do not use all-caps, though.
>
> Try "git log --grep=deprecate -Swarning" for some examples.

I'm asking about the ones in shell, because this is a shell script.

-- 
Felipe Contreras

Re: [PATCH v3 1/5] pull: rename pull.rename to pull.mode

[Jeff King]

On Tue, Sep 10, 2013 at 01:46:27AM -0500, Felipe Contreras wrote:

> On Mon, Sep 9, 2013 at 9:21 PM, Jeff King <peff@peff.net> wrote:
> > On Mon, Sep 09, 2013 at 05:49:36PM -0500, Felipe Contreras wrote:
> >
> >> > These deprecation warning messages should be written to stderr, and
> >> > should probably be prefixed with "WARNING: ".
> >>
> >> Is there any deprecation warning that works this way?
> >
> > The ones in C code typically use warning(), which will prefix with
> > "warning:" and write to stderr. They do not use all-caps, though.
> >
> > Try "git log --grep=deprecate -Swarning" for some examples.
> 
> I'm asking about the ones in shell, because this is a shell script.

Why would we want a deprecation warning to behave any differently if it
is written in shell versus in C? It cannot use the warning() function,
but you can easily emulate it with "echo".

Do you have some reason for not wanting the warning to go to stderr?

-Peff

Re: [PATCH v3 1/5] pull: rename pull.rename to pull.mode

[Matthieu Moy]

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

> On Mon, Sep 9, 2013 at 9:21 PM, Jeff King <peff@peff.net> wrote:
>> On Mon, Sep 09, 2013 at 05:49:36PM -0500, Felipe Contreras wrote:
>>
>>> > These deprecation warning messages should be written to stderr, and
>>> > should probably be prefixed with "WARNING: ".
>>>
>>> Is there any deprecation warning that works this way?
>>
>> The ones in C code typically use warning(), which will prefix with
>> "warning:" and write to stderr. They do not use all-caps, though.
>>
>> Try "git log --grep=deprecate -Swarning" for some examples.
>
> I'm asking about the ones in shell, because this is a shell script.

No user cares whether "git pull" is written in shell. shell Vs C is an
implementation detail, stdout Vs stderr is user-visible.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v3 1/5] pull: rename pull.rename to pull.mode

[Junio C Hamano]

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

> Also 'branch.<name>.rebase' to 'branch.<name>.pullmode'.
>
> This way 'pull.mode' can be set to 'merge', and the default can be
> something else.
>
> The old configurations still work, but get deprecated.

Sounds good, but we should still mention these old names in the
documentation (and mark them as deprecated).

> +test "$mode" == 'rebase' && rebase="true"

POSIX test does not take "==" as string equality; it is a bash-ism.

	test "$mode" = rebase

Re: [PATCH v3 1/5] pull: rename pull.rename to pull.mode

[Richard Hansen]

On 2013-09-09 18:49, Felipe Contreras wrote:
> On Mon, Sep 9, 2013 at 4:23 PM, Richard Hansen <rhansen@bbn.com> wrote:
>> On 2013-09-08 21:23, Felipe Contreras wrote:
>>> The old configurations still work, but get deprecated.
>>
>> Should some tests for the deprecated configs be added?  We wouldn't want
>> to accidentally break those.
> 
> Probably, but Junio is not picking this patch anyway.

It sounds to me like he would with some mods:
http://thread.gmane.org/gmane.comp.version-control.git/233554/focus=234488

-Richard

Re: [PATCH v3 1/5] pull: rename pull.rename to pull.mode

[Felipe Contreras]

Richard Hansen wrote:
> On 2013-09-09 18:49, Felipe Contreras wrote:
> > On Mon, Sep 9, 2013 at 4:23 PM, Richard Hansen <rhansen@bbn.com> wrote:
> >> On 2013-09-08 21:23, Felipe Contreras wrote:
> >>> The old configurations still work, but get deprecated.
> >>
> >> Should some tests for the deprecated configs be added?  We wouldn't want
> >> to accidentally break those.
> > 
> > Probably, but Junio is not picking this patch anyway.
> 
> It sounds to me like he would with some mods:
> http://thread.gmane.org/gmane.comp.version-control.git/233554/focus=234488

The modifications are already in this series.

-- 
Felipe Contreras

Re: [PATCH v3 1/5] pull: rename pull.rename to pull.mode

[Felipe Contreras]

Matthieu Moy wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> > On Mon, Sep 9, 2013 at 9:21 PM, Jeff King <peff@peff.net> wrote:
> >> On Mon, Sep 09, 2013 at 05:49:36PM -0500, Felipe Contreras wrote:
> >>
> >>> > These deprecation warning messages should be written to stderr, and
> >>> > should probably be prefixed with "WARNING: ".
> >>>
> >>> Is there any deprecation warning that works this way?
> >>
> >> The ones in C code typically use warning(), which will prefix with
> >> "warning:" and write to stderr. They do not use all-caps, though.
> >>
> >> Try "git log --grep=deprecate -Swarning" for some examples.
> >
> > I'm asking about the ones in shell, because this is a shell script.
> 
> No user cares whether "git pull" is written in shell. shell Vs C is an
> implementation detail, stdout Vs stderr is user-visible.

You are free to go ahead and implement 'warning ()' in git-sh-setup.sh, in the
meantime no shell script does that, and that's no reason to reject this patch
series.

-- 
Felipe Contreras

Re: [PATCH v3 1/5] pull: rename pull.rename to pull.mode

[Richard Hansen]

On 2013-10-11 19:56, Felipe Contreras wrote:
> Matthieu Moy wrote:
>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>
>>> On Mon, Sep 9, 2013 at 9:21 PM, Jeff King <peff@peff.net> wrote:
>>>> On Mon, Sep 09, 2013 at 05:49:36PM -0500, Felipe Contreras wrote:
>>>>
>>>>>> These deprecation warning messages should be written to stderr, and
>>>>>> should probably be prefixed with "WARNING: ".
>>>>>
>>>>> Is there any deprecation warning that works this way?
>>>>
>>>> The ones in C code typically use warning(), which will prefix with
>>>> "warning:" and write to stderr. They do not use all-caps, though.
>>>>
>>>> Try "git log --grep=deprecate -Swarning" for some examples.
>>>
>>> I'm asking about the ones in shell, because this is a shell script.
>>
>> No user cares whether "git pull" is written in shell. shell Vs C is an
>> implementation detail, stdout Vs stderr is user-visible.
> 
> You are free to go ahead and implement 'warning ()' in git-sh-setup.sh, in the
> meantime no shell script does that, and that's no reason to reject this patch
> series.

Sure, but is is a reason to reroll the patch series.  Maybe it's not a
strong enough reason on its own to reroll, but if you're going to reroll
anyway, this should be fixed.

-Richard

Re: [PATCH v3 1/5] pull: rename pull.rename to pull.mode

[Jeff King]

On Fri, Oct 11, 2013 at 06:56:23PM -0500, Felipe Contreras wrote:

> > >>> > These deprecation warning messages should be written to stderr, and
> > >>> > should probably be prefixed with "WARNING: ".
> > >>>
> > >>> Is there any deprecation warning that works this way?
> > >>
> > >> The ones in C code typically use warning(), which will prefix with
> > >> "warning:" and write to stderr. They do not use all-caps, though.
> > >>
> > >> Try "git log --grep=deprecate -Swarning" for some examples.
> > >
> > > I'm asking about the ones in shell, because this is a shell script.
> > 
> > No user cares whether "git pull" is written in shell. shell Vs C is an
> > implementation detail, stdout Vs stderr is user-visible.
> 
> You are free to go ahead and implement 'warning ()' in git-sh-setup.sh, in the
> meantime no shell script does that, and that's no reason to reject this patch
> series.

You are completely missing Matthieu's point that we attempt to be
consistent in the format of messages, as well as where they are output,
and from a user's perspective it does not matter what language the tool
is implemented in.

Also, you are wrong that there are no other shell scripts that behave as
Richard said:

  $ git grep '>&2' | grep -i deprecate
  contrib/completion/git-completion.bash: echo "WARNING: this script is deprecated, please see git-completion.zsh" 1>&2
  contrib/examples/git-resolve.sh:echo 'WARNING: This command is DEPRECATED and will be removed very soon.' >&2
  git-lost-found.sh:echo "WARNING: '$0' is deprecated in favor of 'git fsck --lost-found'" >&2

Please, can we just squash in the patch below and stop talking about
this?

diff --git a/git-pull.sh b/git-pull.sh
index a9cf7ac..9c4091c 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -58,8 +58,8 @@ then
 	if test "$rebase" = 'true'
 	then
 		mode="rebase"
-		echo "The configurations pull.rebase and branch.<name>.rebase are deprecated."
-		echo "Please use pull.mode and branch.<name>.pullmode instead."
+		echo >&2 "warning: The configurations pull.rebase and branch.<name>.rebase are deprecated."
+		echo >&2 "Please use pull.mode and branch.<name>.pullmode instead."
 	fi
 fi
 test -z "$mode" && mode=merge

Re: [PATCH v3 1/5] pull: rename pull.rename to pull.mode

[Felipe Contreras]

On Fri, Oct 11, 2013 at 7:50 PM, Jeff King <peff@peff.net> wrote:
> On Fri, Oct 11, 2013 at 06:56:23PM -0500, Felipe Contreras wrote:
>
>> > >>> > These deprecation warning messages should be written to stderr, and
>> > >>> > should probably be prefixed with "WARNING: ".
>> > >>>
>> > >>> Is there any deprecation warning that works this way?
>> > >>
>> > >> The ones in C code typically use warning(), which will prefix with
>> > >> "warning:" and write to stderr. They do not use all-caps, though.
>> > >>
>> > >> Try "git log --grep=deprecate -Swarning" for some examples.
>> > >
>> > > I'm asking about the ones in shell, because this is a shell script.
>> >
>> > No user cares whether "git pull" is written in shell. shell Vs C is an
>> > implementation detail, stdout Vs stderr is user-visible.
>>
>> You are free to go ahead and implement 'warning ()' in git-sh-setup.sh, in the
>> meantime no shell script does that, and that's no reason to reject this patch
>> series.
>
> You are completely missing Matthieu's point that we attempt to be
> consistent in the format of messages, as well as where they are output,
> and from a user's perspective it does not matter what language the tool
> is implemented in.

If we truly did that, there should be a warning () function, like in C.

> Also, you are wrong that there are no other shell scripts that behave as
> Richard said:
>
>   $ git grep '>&2' | grep -i deprecate
>   contrib/completion/git-completion.bash: echo "WARNING: this script is deprecated, please see git-completion.zsh" 1>&2
>   contrib/examples/git-resolve.sh:echo 'WARNING: This command is DEPRECATED and will be removed very soon.' >&2
>   git-lost-found.sh:echo "WARNING: '$0' is deprecated in favor of 'git fsck --lost-found'" >&2
>
> Please, can we just squash in the patch below and stop talking about
> this?
>
> diff --git a/git-pull.sh b/git-pull.sh
> index a9cf7ac..9c4091c 100755
> --- a/git-pull.sh
> +++ b/git-pull.sh
> @@ -58,8 +58,8 @@ then
>         if test "$rebase" = 'true'
>         then
>                 mode="rebase"
> -               echo "The configurations pull.rebase and branch.<name>.rebase are deprecated."
> -               echo "Please use pull.mode and branch.<name>.pullmode instead."
> +               echo >&2 "warning: The configurations pull.rebase and branch.<name>.rebase are deprecated."
> +               echo >&2 "Please use pull.mode and branch.<name>.pullmode instead."
>         fi
>  fi
>  test -z "$mode" && mode=merge

Are you sure you want me to squash that in? Because the warnings
wouldn't be consistent. Some would be "WARNING: " and others would be
"warning: ". Personally I don't care, but if your argument is
consistency, you should. If we had a warning () function, we could
truly be consistent.

-- 
Felipe Contreras

Re: [PATCH v3 1/5] pull: rename pull.rename to pull.mode

[Jeff King]

On Fri, Oct 11, 2013 at 08:15:46PM -0500, Felipe Contreras wrote:

> >> You are free to go ahead and implement 'warning ()' in git-sh-setup.sh, in the
> >> meantime no shell script does that, and that's no reason to reject this patch
> >> series.
> >
> > You are completely missing Matthieu's point that we attempt to be
> > consistent in the format of messages, as well as where they are output,
> > and from a user's perspective it does not matter what language the tool
> > is implemented in.
> 
> If we truly did that, there should be a warning () function, like in C.

Or people could hand-code them to look similar, which is exactly what
has happened.

If you want to factor out a warning function to clean up the code, be my
guest. But the lack of one does not provide an argument that you should
break consistency.

> > -               echo "The configurations pull.rebase and branch.<name>.rebase are deprecated."
> > -               echo "Please use pull.mode and branch.<name>.pullmode instead."
> > +               echo >&2 "warning: The configurations pull.rebase and branch.<name>.rebase are deprecated."
> > +               echo >&2 "Please use pull.mode and branch.<name>.pullmode instead."
> [...]
> 
> Are you sure you want me to squash that in? Because the warnings
> wouldn't be consistent. Some would be "WARNING: " and others would be
> "warning: ". Personally I don't care, but if your argument is
> consistency, you should. If we had a warning () function, we could
> truly be consistent.

It is significant in the most important ways, which are labeling it at
all, and sending it to stderr. Capitalization is less important, in my
opinion.

Using a lowercase version is much more consistent with the warnings
produced by C code, which is why I chose it over the capitalized
version. Again, if you want to change the existing WARNING cases in the
shell scripts to be consistent with C output, be my guest.

Do you actually have some reason for wanting to output to go to stdout?

-Peff

Re: [PATCH v3 1/5] pull: rename pull.rename to pull.mode

[Felipe Contreras]

On Fri, Oct 11, 2013 at 8:25 PM, Jeff King <peff@peff.net> wrote:
> On Fri, Oct 11, 2013 at 08:15:46PM -0500, Felipe Contreras wrote:
>
>> >> You are free to go ahead and implement 'warning ()' in git-sh-setup.sh, in the
>> >> meantime no shell script does that, and that's no reason to reject this patch
>> >> series.
>> >
>> > You are completely missing Matthieu's point that we attempt to be
>> > consistent in the format of messages, as well as where they are output,
>> > and from a user's perspective it does not matter what language the tool
>> > is implemented in.
>>
>> If we truly did that, there should be a warning () function, like in C.
>
> Or people could hand-code them to look similar, which is exactly what
> has happened.

And by doing that be prone to make mistakes, like using 'WARNING',
instead of 'warning'. But I guess you don't care about consistency
_that much_.

> If you want to factor out a warning function to clean up the code, be my
> guest. But the lack of one does not provide an argument that you should
> break consistency.

Consistency is already broken.

>> > -               echo "The configurations pull.rebase and branch.<name>.rebase are deprecated."
>> > -               echo "Please use pull.mode and branch.<name>.pullmode instead."
>> > +               echo >&2 "warning: The configurations pull.rebase and branch.<name>.rebase are deprecated."
>> > +               echo >&2 "Please use pull.mode and branch.<name>.pullmode instead."
>> [...]
>>
>> Are you sure you want me to squash that in? Because the warnings
>> wouldn't be consistent. Some would be "WARNING: " and others would be
>> "warning: ". Personally I don't care, but if your argument is
>> consistency, you should. If we had a warning () function, we could
>> truly be consistent.
>
> It is significant in the most important ways, which are labeling it at
> all, and sending it to stderr. Capitalization is less important, in my
> opinion.

It's still inconsistent.

> Using a lowercase version is much more consistent with the warnings
> produced by C code, which is why I chose it over the capitalized
> version. Again, if you want to change the existing WARNING cases in the
> shell scripts to be consistent with C output, be my guest.

It seems you are not that interested in consistency after all.

> Do you actually have some reason for wanting to output to go to stdout?

I'm fine with 'echo "warning: foo" >&2', but still, if you really
cared about consistency, there would be a warn() function, precisely
to avoid the mistakes of WARNING vs. warning which are already there,
plus future ones.

-- 
Felipe Contreras

Re: [PATCH v3 1/5] pull: rename pull.rename to pull.mode

[Richard Hansen]

On 2013-10-11 22:08, Felipe Contreras wrote:
> I'm fine with 'echo "warning: foo" >&2', but still, if you really
> cared about consistency, there would be a warn() function

So add one!  It's only one simple line:

    warning() { printf %s\\n "warning: $*" >&2; }

So much discussion for something so trivial...

-Richard

Re: [PATCH v3 1/5] pull: rename pull.rename to pull.mode

[Felipe Contreras]

On Fri, Oct 11, 2013 at 11:40 PM, Richard Hansen <rhansen@bbn.com> wrote:
> On 2013-10-11 22:08, Felipe Contreras wrote:
>> I'm fine with 'echo "warning: foo" >&2', but still, if you really
>> cared about consistency, there would be a warn() function
>
> So add one!  It's only one simple line:
>
>     warning() { printf %s\\n "warning: $*" >&2; }

It probably should be

warning () {
  printf 'warning: %s\n' "$*" >&2
}

But why don't _you_ do it? I have enough patches being ignored to add
more to the list, specially for something I care nothing about.

> So much discussion for something so trivial...

Trivial things are what cause more discussion.

-- 
Felipe Contreras