On Sat, Aug 10, 2013 at 12:58 AM, Stephen Haberman <stephen@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx> > --- > 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html