[PATCH] pull: Allow pull to preserve merges when rebasing.

If a user is working on master, and has merged in their feature branch, but now
has to "git pull" because master moved, with pull.rebase their feature branch
will be flattened into master.

This is because "git pull" currently does not know about rebase's preserve
merges flag, which would avoid this behavior, as it would instead replay just
the merge commit of the feature branch onto the new master, and not replay each
individual commit in the feature branch.

Add a --rebase=preserve option, which will pass along --preserve-merges to
rebase.

Also add 'preserve' to the allowed values for the pull.rebase config setting.

Signed-off-by: Stephen Haberman <redacted>
---
Hey,

This is version 2 of my previous patch--I changed over to the --rebase=preserve
syntax as suggested by Johannes and Junio.

I also updated the documentation.

I believe it is ready for serious consideration. Please let me know if I'm
missing anything subtle or obvious.

Thanks,
Stephen

 Documentation/config.txt   |  8 ++++++++
 Documentation/git-pull.txt | 18 ++++++++++++------
 git-pull.sh                | 23 ++++++++++++++++++-----
 t/t5520-pull.sh            | 28 ++++++++++++++++++++++++++++
 4 files changed, 66 insertions(+), 11 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index ec57a15..4c22be2 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt

@@ -766,6 +766,10 @@ branch.<name>.rebase::
 	"git pull" is run. See "pull.rebase" for doing this in a non
 	branch-specific manner.
 +
+	When preserve, also pass `--preserve-merges` along to 'git rebase'
+	so that locally committed merge commits will not be flattened
+	by running 'git pull'.
++
 *NOTE*: this is a possibly dangerous operation; do *not* use
 it unless you understand the implications (see linkgit:git-rebase[1]
 for details).

@@ -1826,6 +1830,10 @@ pull.rebase::
 	pull" is run. See "branch.<name>.rebase" for setting this on a
 	per-branch basis.
 +
+	When preserve, also pass `--preserve-merges` along to 'git rebase'
+	so that locally committed merge commits will not be flattened
+	by running 'git pull'.
++
 *NOTE*: this is a possibly dangerous operation; do *not* use
 it unless you understand the implications (see linkgit:git-rebase[1]
 for details).

diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index 6ef8d59..87ff938 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt

@@ -102,12 +102,18 @@ include::merge-options.txt[]
 :git-pull: 1
 
 -r::
---rebase::
-	Rebase the current branch on top of the upstream branch after
-	fetching.  If there is a remote-tracking branch corresponding to
-	the upstream branch and the upstream branch was rebased since last
-	fetched, the rebase uses that information to avoid rebasing
-	non-local changes.
+--rebase[=false|true|preserve]::
+	When true, rebase the current branch on top of the upstream
+	branch after fetching. If there is a remote-tracking branch
+	corresponding to the upstream branch and the upstream branch
+	was rebased since last fetched, the rebase uses that information
+	to avoid rebasing non-local changes.
++
+When preserve, also rebase the current branch on top of the upstream
+branch, but pass `--preserve-merges` along to `git rebase` so that
+locally created merge commits will not be flatten.
++
+When false, merge the current branch into the upstream branch.
 +
 See `pull.rebase`, `branch.<name>.rebase` and `branch.autosetuprebase` in
 linkgit:git-config[1] if you want to make `git pull` always use

diff --git a/git-pull.sh b/git-pull.sh
index f0df41c..d142b38 100755
--- a/git-pull.sh
+++ b/git-pull.sh

@@ -40,13 +40,13 @@ test -f "$GIT_DIR/MERGE_HEAD" && die_merge
 
 strategy_args= diffstat= no_commit= squash= no_ff= ff_only=
 log_arg= verbosity= progress= recurse_submodules= verify_signatures=
-merge_args= edit=
+merge_args= edit= rebase_args=
 curr_branch=$(git symbolic-ref -q HEAD)
 curr_branch_short="${curr_branch#refs/heads/}"
-rebase=$(git config --bool branch.$curr_branch_short.rebase)
+rebase=$(git config branch.$curr_branch_short.rebase)
 if test -z "$rebase"
 then
-	rebase=$(git config --bool pull.rebase)
+	rebase=$(git config pull.rebase)
 fi
 dry_run=
 while :

@@ -111,7 +111,14 @@ do
 		merge_args="$merge_args$xx "
 		;;
 	-r|--r|--re|--reb|--reba|--rebas|--rebase)
-		rebase=true
+		# if the value is already non-false, like preserve, leave it alone
+		if test -z "$rebase" -o false = "$rebase"
+		then
+			rebase=true
+		fi
+		;;
+	--rebase=*)
+		rebase="${1#*=}"
 		;;
 	--no-r|--no-re|--no-reb|--no-reba|--no-rebas|--no-rebase)
 		rebase=false

@@ -145,6 +152,12 @@ do
 	shift
 done
 
+if test preserve = "$rebase"
+then
+	rebase=true
+	rebase_args=--preserve-merges
+fi
+
 error_on_no_merge_candidates () {
 	exec >&2
 	for opt

@@ -292,7 +305,7 @@ fi
 merge_name=$(git fmt-merge-msg $log_arg <"$GIT_DIR/FETCH_HEAD") || exit
 case "$rebase" in
 true)
-	eval="git-rebase $diffstat $strategy_args $merge_args $verbosity"
+	eval="git-rebase $diffstat $strategy_args $merge_args $rebase_args $verbosity"
 	eval="$eval --onto $merge_head ${oldremoteref:-$merge_head}"
 	;;
 *)

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index ed4d9c8..29cd45d 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh

@@ -148,6 +148,34 @@ test_expect_success 'branch.to-rebase.rebase should override pull.rebase' '
 	test new = $(git show HEAD:file2)
 '
 
+test_expect_success 'pull.rebase=preserve' '
+	git reset --hard before-rebase &&
+	test_config pull.rebase preserve &&
+	git checkout -b keep-merge second^ &&
+	test_commit file3 &&
+	git checkout to-rebase &&
+	git merge keep-merge &&
+	git tag before-preserve-rebase &&
+	git pull . copy &&
+	test $(git rev-parse HEAD^^) = $(git rev-parse copy) &&
+	test $(git rev-parse HEAD^2) = $(git rev-parse keep-merge)
+'
+
+test_expect_success '--rebase=preserve' '
+	git reset --hard before-preserve-rebase &&
+	git pull --rebase=preserve . copy &&
+	test $(git rev-parse HEAD^^) = $(git rev-parse copy) &&
+	test $(git rev-parse HEAD^2) = $(git rev-parse keep-merge)
+'
+
+test_expect_success '--rebase respects pull.rebase=preserve' '
+	git reset --hard before-preserve-rebase &&
+	test_config pull.rebase preserve &&
+	git pull --rebase . copy &&
+	test $(git rev-parse HEAD^^) = $(git rev-parse copy) &&
+	test $(git rev-parse HEAD^2) = $(git rev-parse keep-merge)
+'
+
 test_expect_success '--rebase with rebased upstream' '
 
 	git remote add -f me . &&

-- 
1.8.1.2

On Sat, Aug 10, 2013 at 12:58 AM, Stephen Haberman
[off-list ref] wrote:

If a user is working on master, and has merged in their feature branch, but now
has to "git pull" because master moved, with pull.rebase their feature branch
will be flattened into master.

This is because "git pull" currently does not know about rebase's preserve
merges flag, which would avoid this behavior, as it would instead replay just
the merge commit of the feature branch onto the new master, and not replay each
individual commit in the feature branch.

Add a --rebase=preserve option, which will pass along --preserve-merges to
rebase.

Also add 'preserve' to the allowed values for the pull.rebase config setting.

Signed-off-by: Stephen Haberman <redacted>
---
Hey,

This is version 2 of my previous patch--I changed over to the --rebase=preserve
syntax as suggested by Johannes and Junio.

It is helpful to mention a link to the previous submission [1] in
order to make it easier for people to refer to the associated
discussion.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/231909

More comments below.

s/flatten/flattened/

Also, it's not clear from the documentation how one would override
pull.rebase=preserve in order to do a normal non-preserving rebase.
From reading the code, one can see that --preserve=true (or
--no-rebase followed by --rebase) will override pull.rebase=preserve,
but it would be hard for someone to guess this. One could imagine
people thinking that --rebase alone would intuitively override
pull.rebase=preserve, or that --preserve=no-preserve would do so, but
that's getting ugly.

Unportable use of test -o [2]. Better to say 'test foo || test bar'.
(There is already an -o in git-pull.sh, but no need to add more.)

[2]: http://www.gnu.org/savannah-checkouts/gnu/autoconf/manual/autoconf-2.69/html_node/Limitations-of-Builtins.html#Limitations-of-Builtins

+               then
+                       rebase=true
+               fi
+               ;;
+       --rebase=*)
+               rebase="${1#*=}"
                ;;

There are a couple inconsistencies here.

First, short-forms of --rebase are recognized (--r, --re, --reb,
etc.), however only long-form --rebase= is accepted.

Second, it is standard practice to allow the option's argument to
bundled or not. For instance, in git-pull, both '--strategy=foo' and
'--strategy foo' are accepted. --rebase should follow suit.

Additionally, shouldn't the code be checking for valid values of
--rebase's argument ("true", "false", "preserve") and complain if
something other is encountered?

Since --rebase= now accepts an argument and since only three values
are allowed for that argument, it makes sense to add tests for
--rebase=preserve (already done), --rebase=true, --rebase=false,
--rebase=bogus.

Moreover, there are additional combinations of --rebase=foo and
pull.rebase which can be tested. For instance, --rebase=false
overrides pull.rebase=preserve, --rebase=true overrides
pull.rebase=preserve, --rebase=preserve overrides pull.rebase=true,
etc.

 test_expect_success '--rebase with rebased upstream' '

        git remote add -f me . &&
--
1.8.1.2

On Sun, Aug 11, 2013 at 2:16 AM, Eric Sunshine [off-list ref] wrote:

Also, it's not clear from the documentation how one would override
pull.rebase=preserve in order to do a normal non-preserving rebase.
From reading the code, one can see that --preserve=true (or

s/--preserve=true/--rebase=true/

--no-rebase followed by --rebase) will override pull.rebase=preserve,
but it would be hard for someone to guess this. One could imagine
people thinking that --rebase alone would intuitively override
pull.rebase=preserve, or that --preserve=no-preserve would do so, but
that's getting ugly.