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

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

[Stephen Haberman]

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 this behavior, and instead replay on the merge commit
of the feature branch onto the new master, and not the entire feature branch
itself.

Add a -p/--preserve-merges, to pass along git rebase if --rebase is in affect.

Also add a new pull.preserve-merges config setting, to enable this behavior as
the default.

Signed-off-by: Stephen Haberman <stephen@exigencecorp.com>
---
 git-pull.sh     | 11 +++++++++--
 t/t5520-pull.sh | 15 +++++++++++++++
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/git-pull.sh b/git-pull.sh
index f0df41c..61d1efb 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -40,7 +40,7 @@ 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)
@@ -48,6 +48,10 @@ if test -z "$rebase"
 then
 	rebase=$(git config --bool pull.rebase)
 fi
+if [ $(git config --bool pull.preserve-merges) = "true" ] ;
+then
+	rebase_args=--preserve-merges
+fi
 dry_run=
 while :
 do
@@ -116,6 +120,9 @@ do
 	--no-r|--no-re|--no-reb|--no-reba|--no-rebas|--no-rebase)
 		rebase=false
 		;;
+	-p|--preserve-merges)
+		rebase_args=--preserve-merges
+		;;
 	--recurse-submodules)
 		recurse_submodules=--recurse-submodules
 		;;
@@ -292,7 +299,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..2a2ee97 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -148,6 +148,21 @@ test_expect_success 'branch.to-rebase.rebase should override pull.rebase' '
 	test new = $(git show HEAD:file2)
 '
 
+test_expect_success 'preserve merges' '
+	git reset --hard before-rebase &&
+	test_config pull.rebase true &&
+	test_config pull.preserve-merges true &&
+	git checkout -b keep-merge second^ &&
+	echo new > file3 &&
+	git add file3 &&
+	git commit -m "new file3" &&
+	git checkout to-rebase &&
+	git merge keep-merge &&
+	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 with rebased upstream' '
 
 	git remote add -f me . &&
-- 
1.8.1.2

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

[Stephen Haberman]

> This is because "git pull" currently does not know about rebase's
> preserve merges flag, which would this behavior, and instead replay
> on the merge commit of the feature branch onto the new master, and
> not the entire feature branch itself.

Ack, sorry, I was doing this too late last night--should say:

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

- Stephen

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

[Johannes Schindelin]

Hi Stephen,

On Thu, 8 Aug 2013, Stephen Haberman 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 this behavior, and instead replay on
> the merge commit of the feature branch onto the new master, and not the
> entire feature branch itself.
> 
> Add a -p/--preserve-merges, to pass along git rebase if --rebase is in affect.

ACK!

> Also add a new pull.preserve-merges config setting, to enable this
> behavior as the default.

This should probably be added to config.txt and
Documentation/git-pull.txt, too, right?

FYI I started to rewrite the complete --preserve-merges support for the
interactive rebase some time ago, based on the experience of the merging
rebases:

	https://github.com/msysgit/git/commit/b733454b

(part of the rebase-i-p branch). The idea is to use the 'exec' command of
the interactive rebase to do a much better job, and to allow reordering
(and in particular fixup commits) even when trying to preserve merges.

Unfortunately, the resulting branches look slightly differently now,
breaking the (horribly complicated -- my fault!) unit tests, and due to an
utter lack of time I had to stall that project.

Feel free to play with it if you want!

Ciao,
Johannes

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

[Stephen Haberman]

Hi Johannes,

> This should probably be added to config.txt and
> Documentation/git-pull.txt, too, right?

Yep, I meant to note that I'd do that after getting an initial
confirmation that the pull.preserve-merges was the preferred approach.

(I was being lazy and didn't want to write up docs only to switch to
overloading pull.rebase or what not.)

But I'll go ahead and do that.

> 	https://github.com/msysgit/git/commit/b733454b

Interesting!

> Feel free to play with it if you want!

I'll poke around out of curiosity, but no promises, as, yes, this is a
tricky bit of functionality that can quickly lead to a lot of lost
sleep. :-)

- Stephen

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

[Philip Oakley]

From: "Stephen Haberman" <stephen@exigencecorp.com>
> Hi Johannes,
>
>> This should probably be added to config.txt and
>> Documentation/git-pull.txt, too, right?
>
> Yep, I meant to note that I'd do that after getting an initial
> confirmation that the pull.preserve-merges was the preferred approach.
>
> (I was being lazy and didn't want to write up docs only to switch to
> overloading pull.rebase or what not.)
>
> But I'll go ahead and do that.
>
>> https://github.com/msysgit/git/commit/b733454b
>
> Interesting!
>
>> Feel free to play with it if you want!
>
> I'll poke around out of curiosity, but no promises, as, yes, this is a
> tricky bit of functionality that can quickly lead to a lot of lost
> sleep. :-)
>
> - Stephen
>
>
Johannes also kindly explained his merging-rebase script to me on the 
msysgit list a few days ago 
https://groups.google.com/forum/?hl=en_US?hl%3Den#!topic/msysgit/LiPa2T_K4C4 
which shows how msysgit and git both keep parallel lines of development 
with fast forwarding and rebasing at the same time.

The technique should also help those case for keeping 
private/independent lines of development that are discussed often.

Philip

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

[Junio C Hamano]

Stephen Haberman <stephen@exigencecorp.com> writes:

> Hi Johannes,
>
>> This should probably be added to config.txt and
>> Documentation/git-pull.txt, too, right?
>
> Yep, I meant to note that I'd do that after getting an initial
> confirmation that the pull.preserve-merges was the preferred approach.

If you were to go that route, no dashes in the last component of
configuration variable names, please.

> (I was being lazy and didn't want to write up docs only to switch to
> overloading pull.rebase or what not.)

I think we have a recent update that allows you to say

	[pull]
        	rebase = false

to mean "I want 'git pull' to use merge".  Currently the other
choice is:

	[pull]
		rebase = true

to say "I want to run 'git pull --rebase'".  I do not think it is
unreasonable to extend it further so that

	[pull]
		rebase = preserve

is understood.

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

[Johannes Schindelin]

Hi,

On Thu, 8 Aug 2013, Junio C Hamano wrote:

> Stephen Haberman <stephen@exigencecorp.com> writes:
> 
> > Hi Johannes,
> >
> >> This should probably be added to config.txt and
> >> Documentation/git-pull.txt, too, right?
> >
> > Yep, I meant to note that I'd do that after getting an initial
> > confirmation that the pull.preserve-merges was the preferred approach.
> 
> If you were to go that route, no dashes in the last component of
> configuration variable names, please.
> 
> > (I was being lazy and didn't want to write up docs only to switch to
> > overloading pull.rebase or what not.)
> 
> I think we have a recent update that allows you to say
> 
> 	[pull]
>         	rebase = false
> 
> to mean "I want 'git pull' to use merge".  Currently the other
> choice is:
> 
> 	[pull]
> 		rebase = true
> 
> to say "I want to run 'git pull --rebase'".  I do not think it is
> unreasonable to extend it further so that
> 
> 	[pull]
> 		rebase = preserve
> 
> is understood.

We have a patch in Git for Windows allowing rebase = interactive which I
did not have time to send upstream.

Ciao,
Johannes

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

[Stephen Haberman]

> We have a patch in Git for Windows allowing rebase = interactive
> which I did not have time to send upstream.

Cool, so, would rebase=preserve and rebase=interactive be completely
orthogonal?

E.g. do we have to worry about the user wanting to do both, like with
something ugly like rebase=preserve-interactive?

Assuming not, rebase=preserve sounds good to me. I have a patch
that does that about ready to submit.

- Stephen

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

[Stephen Haberman]

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 <stephen@exigencecorp.com>
---
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

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

[Eric Sunshine]

On Sat, Aug 10, 2013 at 12:58 AM, Stephen Haberman
<stephen@exigencecorp.com> 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 <stephen@exigencecorp.com>
> ---
> 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.

> 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
>
> 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.

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.

> ++
> +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
> @@ -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"

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?

>         --no-r|--no-re|--no-reb|--no-reba|--no-rebas|--no-rebase)
>                 rebase=false
> 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)
> +'

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

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

[Eric Sunshine]

On Sun, Aug 11, 2013 at 2:16 AM, Eric Sunshine <sunshine@sunshineco.com> 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.

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

[Stephen Haberman]

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 <stephen@exigencecorp.com>
---
Hi,

This is v3 of my previous pull.rebase=preserve patch, previously discussed here:

http://thread.gmane.org/gmane.comp.version-control.git/232061
http://thread.gmane.org/gmane.comp.version-control.git/231909

In this version, I addressed all of Eric's excellent feedback.

I believe the patch is much better now, but would still appreciate more
detailed feedback. In particular, I kind of made up how to handle and
invalid "--rebase=invalid" value, and the resulting error message.

Also, I changed git-pull's usage to include the -r parameter...not
sure if that's okay or not. Let me know if not.

Thanks!

 Documentation/config.txt   |  8 +++++
 Documentation/git-pull.txt | 18 +++++++----
 git-pull.sh                | 42 ++++++++++++++++++++----
 t/t5520-pull.sh            | 81 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 137 insertions(+), 12 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..beea10b 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 flattened.
++
+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..78ad52d 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -4,7 +4,7 @@
 #
 # Fetch one or more remote refs and merge it/them into the current HEAD.
 
-USAGE='[-n | --no-stat] [--[no-]commit] [--[no-]squash] [--[no-]ff] [-s strategy]... [<fetch-options>] <repo> <head>...'
+USAGE='[-n | --no-stat] [--[no-]commit] [--[no-]squash] [--[no-]ff] [-r [true|false|preserve]] [-s strategy]... [<fetch-options>] <repo> <head>...'
 LONG_USAGE='Fetch one or more remote refs and integrate it/them with the current HEAD.'
 SUBDIRECTORY_OK=Yes
 OPTIONS_SPEC=
@@ -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 :
@@ -110,8 +110,27 @@ do
 		esac
 		merge_args="$merge_args$xx "
 		;;
+	-r=*|--r=*|--re=*|--reb=*|--reba=*|--rebas=*|--rebase=*|\
 	-r|--r|--re|--reb|--reba|--rebas|--rebase)
-		rebase=true
+		case "$#,$1" in
+		*,*=*)
+			rebase="${1#*=}"
+			;;
+		1,*)
+			rebase=true
+			;;
+		*)
+			# if the user typed 'git pull -r . copy', don't treat '.'
+			# as an argument to -r
+			if test true = "$2" || test false = "$2" || test preserve = "$3"
+			then
+				rebase="$2"
+				shift
+			else
+				rebase=true
+			fi
+			;;
+		esac
 		;;
 	--no-r|--no-re|--no-reb|--no-reba|--no-rebas|--no-rebase)
 		rebase=false
@@ -145,6 +164,17 @@ do
 	shift
 done
 
+if test preserve = "$rebase"
+then
+	rebase=true
+	rebase_args=--preserve-merges
+elif test ! -z "$rebase" && test false != "$rebase" && test true != "$rebase"
+then
+	echo "Invalid value for --rebase, should be true, false, or preserve"
+	usage
+	exit 1
+fi
+
 error_on_no_merge_candidates () {
 	exec >&2
 	for opt
@@ -292,7 +322,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..8be0482 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -148,6 +148,87 @@ test_expect_success 'branch.to-rebase.rebase should override pull.rebase' '
 	test new = $(git show HEAD:file2)
 '
 
+# add a feature branch, keep-merge, that is merged into master, so the
+# test can try preserving the merge commit (or not) with various
+# --rebase flags/pull.rebase settings.
+test_expect_success 'preserve merge setup' '
+	git reset --hard before-rebase &&
+	git checkout -b keep-merge second^ &&
+	test_commit file3 &&
+	git checkout to-rebase &&
+	git merge keep-merge &&
+	git tag before-preserve-rebase
+'
+
+test_expect_success 'pull.rebase=false create a new merge commit' '
+	git reset --hard before-preserve-rebase &&
+	test_config pull.rebase false &&
+	git pull . copy &&
+	test $(git rev-parse HEAD^1) = $(git rev-parse before-preserve-rebase) &&
+	test $(git rev-parse HEAD^2) = $(git rev-parse copy) &&
+	test file3 = $(git show HEAD:file3.t)
+'
+
+test_expect_success 'pull.rebase=true flattens keep-merge' '
+	git reset --hard before-preserve-rebase &&
+	test_config pull.rebase true &&
+	git pull . copy &&
+	test $(git rev-parse HEAD^^) = $(git rev-parse copy) &&
+	test file3 = $(git show HEAD:file3.t)
+'
+
+test_expect_success 'pull.rebase=preserve rebases and merges keep-merge' '
+	git reset --hard before-preserve-rebase &&
+	test_config pull.rebase preserve &&
+	git pull . copy &&
+	test $(git rev-parse HEAD^^) = $(git rev-parse copy) &&
+	test $(git rev-parse HEAD^2) = $(git rev-parse keep-merge)
+'
+
+test_expect_success 'pull.rebase=invalid fails' '
+	git reset --hard before-preserve-rebase &&
+	test_config pull.rebase invalid &&
+	! git pull . copy
+'
+
+test_expect_success '--rebase=false create a new merge commit' '
+	git reset --hard before-preserve-rebase &&
+	test_config pull.rebase true &&
+	git pull --rebase=false . copy &&
+	test $(git rev-parse HEAD^1) = $(git rev-parse before-preserve-rebase) &&
+	test $(git rev-parse HEAD^2) = $(git rev-parse copy) &&
+	test file3 = $(git show HEAD:file3.t)
+'
+
+test_expect_success '--rebase=true rebases and flattens keep-merge' '
+	git reset --hard before-preserve-rebase &&
+	test_config pull.rebase preserve &&
+	git pull --rebase=true . copy &&
+	test $(git rev-parse HEAD^^) = $(git rev-parse copy) &&
+	test file3 = $(git show HEAD:file3.t)
+'
+
+test_expect_success '--rebase=preserve rebases and merges keep-merge' '
+	git reset --hard before-preserve-rebase &&
+	test_config pull.rebase true &&
+	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=invalid fails' '
+	git reset --hard before-preserve-rebase &&
+	! git pull --rebase=invalid . copy
+'
+
+test_expect_success '--rebase overrides pull.rebase=preserve and flattens keep-merge' '
+	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 file3 = $(git show HEAD:file3.t)
+'
+
 test_expect_success '--rebase with rebased upstream' '
 
 	git remote add -f me . &&
-- 
1.8.1.2

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

[Andres Perera]

hello, comments inline

On Sun, Aug 11, 2013 at 4:56 PM, Stephen Haberman
<stephen@exigencecorp.com> 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 <stephen@exigencecorp.com>
> ---
> Hi,
>
> This is v3 of my previous pull.rebase=preserve patch, previously discussed here:
>
> http://thread.gmane.org/gmane.comp.version-control.git/232061
> http://thread.gmane.org/gmane.comp.version-control.git/231909
>
> In this version, I addressed all of Eric's excellent feedback.
>
> I believe the patch is much better now, but would still appreciate more
> detailed feedback. In particular, I kind of made up how to handle and
> invalid "--rebase=invalid" value, and the resulting error message.
>
> Also, I changed git-pull's usage to include the -r parameter...not
> sure if that's okay or not. Let me know if not.
>
> Thanks!
>
>  Documentation/config.txt   |  8 +++++
>  Documentation/git-pull.txt | 18 +++++++----
>  git-pull.sh                | 42 ++++++++++++++++++++----
>  t/t5520-pull.sh            | 81 ++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 137 insertions(+), 12 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..beea10b 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 flattened.
> ++
> +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..78ad52d 100755
> --- a/git-pull.sh
> +++ b/git-pull.sh
> @@ -4,7 +4,7 @@
>  #
>  # Fetch one or more remote refs and merge it/them into the current HEAD.
>
> -USAGE='[-n | --no-stat] [--[no-]commit] [--[no-]squash] [--[no-]ff] [-s strategy]... [<fetch-options>] <repo> <head>...'
> +USAGE='[-n | --no-stat] [--[no-]commit] [--[no-]squash] [--[no-]ff] [-r [true|false|preserve]] [-s strategy]... [<fetch-options>] <repo> <head>...'
>  LONG_USAGE='Fetch one or more remote refs and integrate it/them with the current HEAD.'
>  SUBDIRECTORY_OK=Yes
>  OPTIONS_SPEC=
> @@ -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 :
> @@ -110,8 +110,27 @@ do
>                 esac
>                 merge_args="$merge_args$xx "
>                 ;;
> +       -r=*|--r=*|--re=*|--reb=*|--reba=*|--rebas=*|--rebase=*|\
>         -r|--r|--re|--reb|--reba|--rebas|--rebase)
> -               rebase=true
> +               case "$#,$1" in
> +               *,*=*)
> +                       rebase="${1#*=}"
> +                       ;;
> +               1,*)
> +                       rebase=true
> +                       ;;
> +               *)

> +                       # if the user typed 'git pull -r . copy', don't treat '.'
> +                       # as an argument to -r
> +                       if test true = "$2" || test false = "$2" || test preserve = "$3"
> +                       then
> +                               rebase="$2"
> +                               shift
> +                       else
> +                               rebase=true
> +                       fi

1. i'm not sure why you are testing $3 == preserve. it looks like a typo

2. clearer than a string of yoda conditions:

case $2 in
true|false|preserve)
    rebase=$2
    shift
    ;;
*)
    rebase=true
    ;;
esac

> +                       ;;
> +               esac
>                 ;;
>         --no-r|--no-re|--no-reb|--no-reba|--no-rebas|--no-rebase)
>                 rebase=false
> @@ -145,6 +164,17 @@ do
>         shift
>  done
>

> +if test preserve = "$rebase"
> +then
> +       rebase=true
> +       rebase_args=--preserve-merges
> +elif test ! -z "$rebase" && test false != "$rebase" && test true != "$rebase"
> +then
> +       echo "Invalid value for --rebase, should be true, false, or preserve"
> +       usage
> +       exit 1
> +fi
> +

1. in the error message you say that rebase should be a trystate of
true, false, or preserve. why then do you allow $rebase == '' ?

2. clearer than a string of yoda conditions:

case $rebase in
preserve)
    rebase_args=--preserve-merges
    rebase=true
    ;;
true|false)
    ;;
*)
    echo "Invalid value for --rebase, should be true, false, or preserve" >&2
    usage
    exit 1
esac


>  error_on_no_merge_candidates () {
>         exec >&2
>         for opt
> @@ -292,7 +322,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..8be0482 100755
> --- a/t/t5520-pull.sh
> +++ b/t/t5520-pull.sh
> @@ -148,6 +148,87 @@ test_expect_success 'branch.to-rebase.rebase should override pull.rebase' '
>         test new = $(git show HEAD:file2)
>  '
>
> +# add a feature branch, keep-merge, that is merged into master, so the
> +# test can try preserving the merge commit (or not) with various
> +# --rebase flags/pull.rebase settings.
> +test_expect_success 'preserve merge setup' '
> +       git reset --hard before-rebase &&
> +       git checkout -b keep-merge second^ &&
> +       test_commit file3 &&
> +       git checkout to-rebase &&
> +       git merge keep-merge &&
> +       git tag before-preserve-rebase
> +'
> +
> +test_expect_success 'pull.rebase=false create a new merge commit' '
> +       git reset --hard before-preserve-rebase &&
> +       test_config pull.rebase false &&
> +       git pull . copy &&
> +       test $(git rev-parse HEAD^1) = $(git rev-parse before-preserve-rebase) &&
> +       test $(git rev-parse HEAD^2) = $(git rev-parse copy) &&
> +       test file3 = $(git show HEAD:file3.t)
> +'
> +
> +test_expect_success 'pull.rebase=true flattens keep-merge' '
> +       git reset --hard before-preserve-rebase &&
> +       test_config pull.rebase true &&
> +       git pull . copy &&
> +       test $(git rev-parse HEAD^^) = $(git rev-parse copy) &&
> +       test file3 = $(git show HEAD:file3.t)
> +'
> +
> +test_expect_success 'pull.rebase=preserve rebases and merges keep-merge' '
> +       git reset --hard before-preserve-rebase &&
> +       test_config pull.rebase preserve &&
> +       git pull . copy &&
> +       test $(git rev-parse HEAD^^) = $(git rev-parse copy) &&
> +       test $(git rev-parse HEAD^2) = $(git rev-parse keep-merge)
> +'
> +
> +test_expect_success 'pull.rebase=invalid fails' '
> +       git reset --hard before-preserve-rebase &&
> +       test_config pull.rebase invalid &&
> +       ! git pull . copy
> +'
> +
> +test_expect_success '--rebase=false create a new merge commit' '
> +       git reset --hard before-preserve-rebase &&
> +       test_config pull.rebase true &&
> +       git pull --rebase=false . copy &&
> +       test $(git rev-parse HEAD^1) = $(git rev-parse before-preserve-rebase) &&
> +       test $(git rev-parse HEAD^2) = $(git rev-parse copy) &&
> +       test file3 = $(git show HEAD:file3.t)
> +'
> +
> +test_expect_success '--rebase=true rebases and flattens keep-merge' '
> +       git reset --hard before-preserve-rebase &&
> +       test_config pull.rebase preserve &&
> +       git pull --rebase=true . copy &&
> +       test $(git rev-parse HEAD^^) = $(git rev-parse copy) &&
> +       test file3 = $(git show HEAD:file3.t)
> +'
> +
> +test_expect_success '--rebase=preserve rebases and merges keep-merge' '
> +       git reset --hard before-preserve-rebase &&
> +       test_config pull.rebase true &&
> +       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=invalid fails' '
> +       git reset --hard before-preserve-rebase &&
> +       ! git pull --rebase=invalid . copy
> +'
> +
> +test_expect_success '--rebase overrides pull.rebase=preserve and flattens keep-merge' '
> +       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 file3 = $(git show HEAD:file3.t)
> +'
> +
>  test_expect_success '--rebase with rebased upstream' '
>
>         git remote add -f me . &&
> --
> 1.8.1.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

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

[Stephen Haberman]

> 1. i'm not sure why you are testing $3 == preserve. it looks like a
> typo

Yes, good catch. I've added a test that fails, and will fix that.

> 2. clearer than a string of yoda conditions:
> 
> case $2 in
> true|false|preserve)

Makes sense, will change.

> 1. in the error message you say that rebase should be a trystate of
> true, false, or preserve. why then do you allow $rebase == '' ?

Because it may be unset, like if the user ran "git pull . copy" and
the pull.rebase setting was not set.

> 2. clearer than a string of yoda conditions:

Will change again.

I'll wait to see if I get any more feedback and then will send out
another version.

Thanks!

- Stephen

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

[Andres Perera]

On Sun, Aug 11, 2013 at 6:39 PM, Stephen Haberman
<stephen@exigencecorp.com> wrote:
>
>> 1. i'm not sure why you are testing $3 == preserve. it looks like a
>> typo
>
> Yes, good catch. I've added a test that fails, and will fix that.
>
>> 2. clearer than a string of yoda conditions:
>>
>> case $2 in
>> true|false|preserve)
>
> Makes sense, will change.
>
>> 1. in the error message you say that rebase should be a trystate of
>> true, false, or preserve. why then do you allow $rebase == '' ?
>
> Because it may be unset, like if the user ran "git pull . copy" and
> the pull.rebase setting was not set.
>
>> 2. clearer than a string of yoda conditions:
>
> Will change again.
>
> I'll wait to see if I get any more feedback and then will send out
> another version.

i just realized that there are ambiguities:

pull -r (true|false|preserve) foo

there are 2 ways to interpret this:

pull --rebase=(true|false|preserve) foo # pull from remote named foo

pull --rebase (true|false|preserve) foo # pull from remote named
(true|false|preserve), branch foo

options with optional operands usually require that the operands be
concatenated with the option argument, so that

pull --rebase[=(true|false|preserve)] | -r[(true|false|preserve)]

avoids the ambiguity of

pull --rebase [(true|false|preserve)] | -r [(true|false|preserve)]

1. you can make it a disambiguation by appending ? to the optspec
(according to man git-rev-parse)

2. you could also disambiguate by testing if the argument is a
configured remote and warn the user, but this makes option parsing
inconsistent, requires additional logic, and is overall inelegant

>
> Thanks!
>
> - Stephen
>
>
>

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

[Stephen Haberman]

Hi Andres,

> i just realized that there are ambiguities:

> pull --rebase (true|false|preserve) foo # pull from remote named
> (true|false|preserve), branch foo

Yeah.

Right now, I did the latter. Around line 125, when parsing "--rebase
<somearg>", we accept <somearg> only if it's true, false, or preserve,
and shift it off. Otherwise we leave it alone and assume it's a remote
name.

Without this logic, t5520 fails because it uses "git pull --rebase .
copy", which, as you noted, is ambiguous, so "." was showing up as the
rebase argument.

So, this is technically handled right now, but I'm fine removing the
ambiguous "--rebase true|false|preserve" option if that is what is
preferred.

- Stephen

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

[Junio C Hamano]

Andres Perera <andres.p@zoho.com> writes:

> i just realized that there are ambiguities:
>
> pull -r (true|false|preserve) foo
>
> there are 2 ways to interpret this:
>
> pull --rebase=(true|false|preserve) foo # pull from remote named foo
>
> pull --rebase (true|false|preserve) foo # pull from remote named
> (true|false|preserve), branch foo
>
> options with optional operands usually require that the operands be
> concatenated with the option argument.

Yes.  This command line option should be like this:

 - "--rebase" and "--no-rebase" are accepted as "true" and "false";

 - "--rebase=preserve" should be the _only_ way to spell the new
   mode of operation (if we were to add "--rebase=interactive"
   later, that should follow suit); and

 - "--rebase=true" and "--rebase=false" is nice to have for
   consistency.

Thanks.

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

[Stephen Haberman]

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 <stephen@exigencecorp.com>
---
Hi,

This is v4 of my pull.rebase=preserve patch, previously discussed here:

http://thread.gmane.org/gmane.comp.version-control.git/232140

This version addresses feedback from Andreas about using case statements
instead of yoda conditions, and feedback from both Andreas and Junio
about avoiding ambiguity with "--rebase preserve". Now it must be
"--rebase=preseve".

I assume I'm doing the right thing by just posting another version of
this patch to the git list; let me know if I'm missing something.

Thanks!

 Documentation/config.txt   |  8 +++++
 Documentation/git-pull.txt | 18 +++++++----
 git-pull.sh                | 27 +++++++++++++---
 t/t5520-pull.sh            | 81 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 123 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..beea10b 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 flattened.
++
+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..6ae6640 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -4,7 +4,7 @@
 #
 # Fetch one or more remote refs and merge it/them into the current HEAD.
 
-USAGE='[-n | --no-stat] [--[no-]commit] [--[no-]squash] [--[no-]ff] [-s strategy]... [<fetch-options>] <repo> <head>...'
+USAGE='[-n | --no-stat] [--[no-]commit] [--[no-]squash] [--[no-]ff] [-r=[true|false|preserve]] [-s strategy]... [<fetch-options>] <repo> <head>...'
 LONG_USAGE='Fetch one or more remote refs and integrate it/them with the current HEAD.'
 SUBDIRECTORY_OK=Yes
 OPTIONS_SPEC=
@@ -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 :
@@ -110,6 +110,9 @@ do
 		esac
 		merge_args="$merge_args$xx "
 		;;
+	-r=*|--r=*|--re=*|--reb=*|--reba=*|--rebas=*|--rebase=*)
+		rebase="${1#*=}"
+		;;
 	-r|--r|--re|--reb|--reba|--rebas|--rebase)
 		rebase=true
 		;;
@@ -145,6 +148,20 @@ do
 	shift
 done
 
+case "$rebase" in
+preserve)
+	rebase=true
+	rebase_args=--preserve-merges
+	;;
+true|false|'')
+	;;
+*)
+	echo "Invalid value for --rebase, should be true, false, or preserve"
+	usage
+	exit 1
+	;;
+esac
+
 error_on_no_merge_candidates () {
 	exec >&2
 	for opt
@@ -292,7 +309,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..8be0482 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -148,6 +148,87 @@ test_expect_success 'branch.to-rebase.rebase should override pull.rebase' '
 	test new = $(git show HEAD:file2)
 '
 
+# add a feature branch, keep-merge, that is merged into master, so the
+# test can try preserving the merge commit (or not) with various
+# --rebase flags/pull.rebase settings.
+test_expect_success 'preserve merge setup' '
+	git reset --hard before-rebase &&
+	git checkout -b keep-merge second^ &&
+	test_commit file3 &&
+	git checkout to-rebase &&
+	git merge keep-merge &&
+	git tag before-preserve-rebase
+'
+
+test_expect_success 'pull.rebase=false create a new merge commit' '
+	git reset --hard before-preserve-rebase &&
+	test_config pull.rebase false &&
+	git pull . copy &&
+	test $(git rev-parse HEAD^1) = $(git rev-parse before-preserve-rebase) &&
+	test $(git rev-parse HEAD^2) = $(git rev-parse copy) &&
+	test file3 = $(git show HEAD:file3.t)
+'
+
+test_expect_success 'pull.rebase=true flattens keep-merge' '
+	git reset --hard before-preserve-rebase &&
+	test_config pull.rebase true &&
+	git pull . copy &&
+	test $(git rev-parse HEAD^^) = $(git rev-parse copy) &&
+	test file3 = $(git show HEAD:file3.t)
+'
+
+test_expect_success 'pull.rebase=preserve rebases and merges keep-merge' '
+	git reset --hard before-preserve-rebase &&
+	test_config pull.rebase preserve &&
+	git pull . copy &&
+	test $(git rev-parse HEAD^^) = $(git rev-parse copy) &&
+	test $(git rev-parse HEAD^2) = $(git rev-parse keep-merge)
+'
+
+test_expect_success 'pull.rebase=invalid fails' '
+	git reset --hard before-preserve-rebase &&
+	test_config pull.rebase invalid &&
+	! git pull . copy
+'
+
+test_expect_success '--rebase=false create a new merge commit' '
+	git reset --hard before-preserve-rebase &&
+	test_config pull.rebase true &&
+	git pull --rebase=false . copy &&
+	test $(git rev-parse HEAD^1) = $(git rev-parse before-preserve-rebase) &&
+	test $(git rev-parse HEAD^2) = $(git rev-parse copy) &&
+	test file3 = $(git show HEAD:file3.t)
+'
+
+test_expect_success '--rebase=true rebases and flattens keep-merge' '
+	git reset --hard before-preserve-rebase &&
+	test_config pull.rebase preserve &&
+	git pull --rebase=true . copy &&
+	test $(git rev-parse HEAD^^) = $(git rev-parse copy) &&
+	test file3 = $(git show HEAD:file3.t)
+'
+
+test_expect_success '--rebase=preserve rebases and merges keep-merge' '
+	git reset --hard before-preserve-rebase &&
+	test_config pull.rebase true &&
+	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=invalid fails' '
+	git reset --hard before-preserve-rebase &&
+	! git pull --rebase=invalid . copy
+'
+
+test_expect_success '--rebase overrides pull.rebase=preserve and flattens keep-merge' '
+	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 file3 = $(git show HEAD:file3.t)
+'
+
 test_expect_success '--rebase with rebased upstream' '
 
 	git remote add -f me . &&
-- 
1.8.1.2

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

[Junio C Hamano]

Stephen Haberman <stephen@exigencecorp.com> writes:

> I assume I'm doing the right thing by just posting another version of
> this patch to the git list; let me know if I'm missing something.

Thanks.  Writing the "story so far..." summary like you did after
the three-dash line was very helpful.

> diff --git a/git-pull.sh b/git-pull.sh
> index f0df41c..6ae6640 100755
> --- a/git-pull.sh
> +++ b/git-pull.sh
> @@ -4,7 +4,7 @@
>  #
>  # Fetch one or more remote refs and merge it/them into the current HEAD.
>  
> -USAGE='[-n | --no-stat] [--[no-]commit] [--[no-]squash] [--[no-]ff] [-s strategy]... [<fetch-options>] <repo> <head>...'
> +USAGE='[-n | --no-stat] [--[no-]commit] [--[no-]squash] [--[no-]ff] [-r=[true|false|preserve]] [-s strategy]... [<fetch-options>] <repo> <head>...'

"-r", even though it happens to be accepted, is not a good idea
here, as there are other --r* commands that are not --rebase.

	[--[no-]rebase|--rebase=preserve]

would be better.

> @@ -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)


This is a grave regression (the same for the earlier one that reads
the branch.*.rebase configuraiton).  Those who did any of the
following will be broken:

	[pull]
		;; any of the following are "true"
        	rebase
                rebase = yes
                rebase = 1
                ;; any of the following are "false"
                rebase = no
                rebase = 0

You would want "bool or string" helper function to parse it
correctly, something like:

	bool_or_string_config () {
        	git config --bool "$1" 2>/dev/null || git config "$1"
	}

	rebase=$(boo_or_string_config pull.rebase)

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

[Junio C Hamano]

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

> Andres Perera <andres.p@zoho.com> writes:
>
>> i just realized that there are ambiguities:
>>
>> pull -r (true|false|preserve) foo
>>
>> there are 2 ways to interpret this:
>>
>> pull --rebase=(true|false|preserve) foo # pull from remote named foo
>>
>> pull --rebase (true|false|preserve) foo # pull from remote named
>> (true|false|preserve), branch foo
>>
>> options with optional operands usually require that the operands be
>> concatenated with the option argument.
>
> Yes.  This command line option should be like this:
>
>  - "--rebase" and "--no-rebase" are accepted as "true" and "false";
>
>  - "--rebase=preserve" should be the _only_ way to spell the new
>    mode of operation (if we were to add "--rebase=interactive"
>    later, that should follow suit); and
>
>  - "--rebase=true" and "--rebase=false" is nice to have for
>    consistency.
>
> Thanks.

Oh, another thing.

How should this interact with 949e0d8e (pull: require choice between
rebase/merge on non-fast-forward pull, 2013-06-27) which has been in
'next' and will likely to be one of the earlier topics to graduate
to 'master' after 1.8.4 is released?

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

[Stephen Haberman]

Hi Junio,

> "-r", even though it happens to be accepted, is not a good idea
> here, as there are other --r* commands that are not --rebase.
> 
> 	[--[no-]rebase|--rebase=preserve]

Cool, I will change that.

> You would want "bool or string" helper function to parse it
> correctly, something like:

Oh, interesting, I had not thought of that--thanks for that
git config --bool snippet, that's really helpful.

- Stephen

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

[Stephen Haberman]

> How should this interact with 949e0d8e (pull: require choice between
> rebase/merge on non-fast-forward pull, 2013-06-27)

I believe there should not be any conflicts in functionality, other
than just tweaking the docs to mention --rebase=preserve as an option.

Personally, I would assert that, for people using a rebase workflow with
"git pull", --prebase=preserve should be the default behavior, otherwise
they'll be surprised when their feature branches get flattened.

Unfortunately, we can't change the behavior of the naked "--rebase"
flag to really mean "--rebase=preserve", but I think that would be
ideal. I think it's what people mean they do "git pull". If you want a
more raw rebase, they would likely (I think/assume) be running "git
rebase" directly.

Nonetheless, thanks for pointing out 949e0d8e, I did not know about it.

Perhaps after that commit graduates to master, I can base this commit
on it, and tweak the new docs to suggest --rebase=preserve as the
least-surprising behavior.

(Since I'm offering opinions, I think --rebase=preserve would be a great
default for "git pull" in 2.0, but please ignore this statement if
you've already hashed out the future/2.0 behavior of git pull.)

- Stephen

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

[Stephen Haberman]

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 <stephen@exigencecorp.com>
---
Hi,

This is v5 of my --rebase=preserve patch, the last discussion of which
was here:

http://thread.gmane.org/gmane.comp.version-control.git/232156

This update has two small changes:

* Change the usage output to match Junio's suggestion, and

* Respect existing true-but-not-"true" values of pull.rebase, using
Junio's bool_or_string_config function (with a test for pull.rebase=1).

Thanks!

 Documentation/config.txt   |  8 +++++
 Documentation/git-pull.txt | 18 ++++++----
 git-pull.sh                | 31 +++++++++++++---
 t/t5520-pull.sh            | 89 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 135 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..beea10b 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 flattened.
++
+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..e11d9a0 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -4,7 +4,7 @@
 #
 # Fetch one or more remote refs and merge it/them into the current HEAD.
 
-USAGE='[-n | --no-stat] [--[no-]commit] [--[no-]squash] [--[no-]ff] [-s strategy]... [<fetch-options>] <repo> <head>...'
+USAGE='[-n | --no-stat] [--[no-]commit] [--[no-]squash] [--[no-]ff] [--[no-]rebase|--rebase=preserve] [-s strategy]... [<fetch-options>] <repo> <head>...'
 LONG_USAGE='Fetch one or more remote refs and integrate it/them with the current HEAD.'
 SUBDIRECTORY_OK=Yes
 OPTIONS_SPEC=
@@ -38,15 +38,19 @@ Please, commit your changes before you can merge.")"
 test -z "$(git ls-files -u)" || die_conflict
 test -f "$GIT_DIR/MERGE_HEAD" && die_merge
 
+bool_or_string_config () {
+	git config --bool "$1" 2>/dev/null || git config "$1"
+}
+
 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=$(bool_or_string_config branch.$curr_branch_short.rebase)
 if test -z "$rebase"
 then
-	rebase=$(git config --bool pull.rebase)
+	rebase=$(bool_or_string_config pull.rebase)
 fi
 dry_run=
 while :
@@ -110,6 +114,9 @@ do
 		esac
 		merge_args="$merge_args$xx "
 		;;
+	-r=*|--r=*|--re=*|--reb=*|--reba=*|--rebas=*|--rebase=*)
+		rebase="${1#*=}"
+		;;
 	-r|--r|--re|--reb|--reba|--rebas|--rebase)
 		rebase=true
 		;;
@@ -145,6 +152,20 @@ do
 	shift
 done
 
+case "$rebase" in
+preserve)
+	rebase=true
+	rebase_args=--preserve-merges
+	;;
+true|false|'')
+	;;
+*)
+	echo "Invalid value for --rebase, should be true, false, or preserve"
+	usage
+	exit 1
+	;;
+esac
+
 error_on_no_merge_candidates () {
 	exec >&2
 	for opt
@@ -292,7 +313,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..227d293 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -148,6 +148,95 @@ test_expect_success 'branch.to-rebase.rebase should override pull.rebase' '
 	test new = $(git show HEAD:file2)
 '
 
+# add a feature branch, keep-merge, that is merged into master, so the
+# test can try preserving the merge commit (or not) with various
+# --rebase flags/pull.rebase settings.
+test_expect_success 'preserve merge setup' '
+	git reset --hard before-rebase &&
+	git checkout -b keep-merge second^ &&
+	test_commit file3 &&
+	git checkout to-rebase &&
+	git merge keep-merge &&
+	git tag before-preserve-rebase
+'
+
+test_expect_success 'pull.rebase=false create a new merge commit' '
+	git reset --hard before-preserve-rebase &&
+	test_config pull.rebase false &&
+	git pull . copy &&
+	test $(git rev-parse HEAD^1) = $(git rev-parse before-preserve-rebase) &&
+	test $(git rev-parse HEAD^2) = $(git rev-parse copy) &&
+	test file3 = $(git show HEAD:file3.t)
+'
+
+test_expect_success 'pull.rebase=true flattens keep-merge' '
+	git reset --hard before-preserve-rebase &&
+	test_config pull.rebase true &&
+	git pull . copy &&
+	test $(git rev-parse HEAD^^) = $(git rev-parse copy) &&
+	test file3 = $(git show HEAD:file3.t)
+'
+
+test_expect_success 'pull.rebase=1 is treated as true and flattens keep-merge' '
+	git reset --hard before-preserve-rebase &&
+	test_config pull.rebase 1 &&
+	git pull . copy &&
+	test $(git rev-parse HEAD^^) = $(git rev-parse copy) &&
+	test file3 = $(git show HEAD:file3.t)
+'
+
+test_expect_success 'pull.rebase=preserve rebases and merges keep-merge' '
+	git reset --hard before-preserve-rebase &&
+	test_config pull.rebase preserve &&
+	git pull . copy &&
+	test $(git rev-parse HEAD^^) = $(git rev-parse copy) &&
+	test $(git rev-parse HEAD^2) = $(git rev-parse keep-merge)
+'
+
+test_expect_success 'pull.rebase=invalid fails' '
+	git reset --hard before-preserve-rebase &&
+	test_config pull.rebase invalid &&
+	! git pull . copy
+'
+
+test_expect_success '--rebase=false create a new merge commit' '
+	git reset --hard before-preserve-rebase &&
+	test_config pull.rebase true &&
+	git pull --rebase=false . copy &&
+	test $(git rev-parse HEAD^1) = $(git rev-parse before-preserve-rebase) &&
+	test $(git rev-parse HEAD^2) = $(git rev-parse copy) &&
+	test file3 = $(git show HEAD:file3.t)
+'
+
+test_expect_success '--rebase=true rebases and flattens keep-merge' '
+	git reset --hard before-preserve-rebase &&
+	test_config pull.rebase preserve &&
+	git pull --rebase=true . copy &&
+	test $(git rev-parse HEAD^^) = $(git rev-parse copy) &&
+	test file3 = $(git show HEAD:file3.t)
+'
+
+test_expect_success '--rebase=preserve rebases and merges keep-merge' '
+	git reset --hard before-preserve-rebase &&
+	test_config pull.rebase true &&
+	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=invalid fails' '
+	git reset --hard before-preserve-rebase &&
+	! git pull --rebase=invalid . copy
+'
+
+test_expect_success '--rebase overrides pull.rebase=preserve and flattens keep-merge' '
+	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 file3 = $(git show HEAD:file3.t)
+'
+
 test_expect_success '--rebase with rebased upstream' '
 
 	git remote add -f me . &&
-- 
1.8.1.2