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

[Eric Sunshine <sunshine@sunshineco.com>]

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