threads / patch / 34884

v3, 5 partsPreparation for non-ff pulls by default

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

## tl;dr

24 messages between Sep 9, 2013 and Oct 12, 2013. Diffs are folded; open one to read it.

replies: 23people: 5as markdown or json

Felipe Contreras· Sep 9, 2013, 01:23 UTC · lore

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
Felipe Contreras· Sep 9, 2013, 01:23 UTC · re: Felipe Contreras · lore

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

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(-)
Show changes to 7 files +78 −38

Documentation/config.txt, Documentation/git-pull.txt, branch.c, git-pull.sh, t/t3200-branch.sh, t/t5520-pull.sh, t/t5601-clone.sh

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
Richard Hansen· Sep 9, 2013, 21:23 UTC · re: Felipe Contreras · lore

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

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.

Show 38 quoted lines
> 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).

Show 31 quoted lines
> 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
Felipe Contreras· Sep 9, 2013, 22:49 UTC · re: Richard Hansen · lore

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

On Mon, Sep 9, 2013 at 4:23 PM, Richard Hansen <rhansen@bbn.com> wrote:
Show 5 quoted lines
> 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.
Show 77 quoted lines
>> 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
Jeff King· Sep 10, 2013, 02:21 UTC · re: Felipe Contreras · lore

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

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
Felipe Contreras· Sep 10, 2013, 06:46 UTC · re: Jeff King · lore

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

On Mon, Sep 9, 2013 at 9:21 PM, Jeff King <peff@peff.net> wrote:
Show 11 quoted lines
> 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
Jeff King· Sep 10, 2013, 06:52 UTC · re: Felipe Contreras · lore

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

On Tue, Sep 10, 2013 at 01:46:27AM -0500, Felipe Contreras wrote:
Show 14 quoted lines
> 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
Matthieu Moy· Sep 10, 2013, 08:16 UTC · re: Felipe Contreras · lore

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

Felipe Contreras <felipe.contreras@gmail.com> writes:
Show 14 quoted lines
> 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/
Felipe Contreras· Oct 11, 2013, 23:56 UTC · re: Matthieu Moy · lore

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

Matthieu Moy wrote:
Show 19 quoted lines
> 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
Richard Hansen· Oct 12, 2013, 00:32 UTC · re: Felipe Contreras · lore

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

On 2013-10-11 19:56, Felipe Contreras wrote:
Show 24 quoted lines
> 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
Jeff King· Oct 12, 2013, 00:50 UTC · re: Felipe Contreras · lore

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

On Fri, Oct 11, 2013 at 06:56:23PM -0500, Felipe Contreras wrote:
Show 18 quoted lines
> > >>> > 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?

Show changes to git-pull.sh +2 −2
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
Felipe Contreras· Oct 12, 2013, 01:15 UTC · re: Jeff King · lore

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

On Fri, Oct 11, 2013 at 7:50 PM, Jeff King <peff@peff.net> wrote:
Show 25 quoted lines
> 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.
Show 26 quoted lines
> 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
Jeff King· Oct 12, 2013, 01:25 UTC · re: Felipe Contreras · lore

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

On Fri, Oct 11, 2013 at 08:15:46PM -0500, Felipe Contreras wrote:
Show 10 quoted lines
> >> 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.

Show 11 quoted lines
> > -               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
Felipe Contreras· Oct 12, 2013, 02:08 UTC · re: Jeff King · lore

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

On Fri, Oct 11, 2013 at 8:25 PM, Jeff King <peff@peff.net> wrote:
Show 15 quoted lines
> 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.
Show 15 quoted lines
>> > -               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
Richard Hansen· Oct 12, 2013, 04:40 UTC · re: Felipe Contreras · lore

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

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
Felipe Contreras· Oct 12, 2013, 06:21 UTC · re: Richard Hansen · lore

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

On Fri, Oct 11, 2013 at 11:40 PM, Richard Hansen <rhansen@bbn.com> wrote:
Show 7 quoted lines
> 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
Richard Hansen· Sep 19, 2013, 19:33 UTC · re: Felipe Contreras · lore

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

On 2013-09-09 18:49, Felipe Contreras wrote:
Show 8 quoted lines
> 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
Felipe Contreras· Oct 11, 2013, 23:54 UTC · re: Richard Hansen · lore

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

Richard Hansen wrote:
Show 12 quoted lines
> 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
Junio C Hamano· Sep 10, 2013, 21:34 UTC · re: Felipe Contreras · lore

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

Felipe Contreras <felipe.contreras@gmail.com> writes:
Show 6 quoted lines
> 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
Felipe Contreras· Sep 9, 2013, 01:23 UTC · re: Felipe Contreras · lore

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

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 git-pull.sh | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)
Show changes to git-pull.sh +11 −11
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
Felipe Contreras· Sep 9, 2013, 01:23 UTC · re: Felipe Contreras · lore

[PATCH v3 3/5] pull: add --merge option

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(-)
Show changes to 2 files +11 −2

Documentation/git-pull.txt, git-pull.sh

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
Felipe Contreras· Sep 9, 2013, 01:23 UTC · re: Felipe Contreras · lore

[PATCH v3 4/5] pull: add merge-ff-only option

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(-)
Show changes to 3 files +53 −3

Documentation/config.txt, git-pull.sh, t/t5520-pull.sh

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
Felipe Contreras· Sep 9, 2013, 01:23 UTC · re: Felipe Contreras · lore

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

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(-)
Show changes to git-pull.sh +12 −3
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
Felipe Contreras· Sep 9, 2013, 01:49 UTC · re: Felipe Contreras · lore

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

On Sun, Sep 8, 2013 at 8:23 PM, Felipe Contreras <felipe.contreras@gmail.com> wrote:

Show 8 quoted lines
> 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>
Show changes to Documentation/git-pull.txt +18 −0
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

← back to recent threads