Prepare for non-interactive merge-preserving rebase

This patch adds two tests (really three, but one of them just handles setup) which we currently expect to fail.

One of them tests "git rebase -p", without the -i flag, to make sure it works without phony editors and suchlike.

The other tests "git pull --rebase --preserve-merges" to make sure that the same functionality exists there.

The test was originally written by Stephen Habermann <[email protected]> but has been significantly modified since its creation.

---

Stephen, I had to modify the tests a bit to get them to work with how I implemented the merge-preserving rebase, and also to remove a lot of the cruft that was previously in there. Hope you're ok with the attribution in the commit message.

t/t3409-rebase-preserve-merges.sh | 68 +++++++++++++++++++++++++++++++++++++

1 files changed, 68 insertions(+), 0 deletions(-)

create mode 100644 t/t3409-rebase-preserve-merges.sh

new file mode 100644
index 0000000..532b220
@@ -0,0 +1,68 @@
+#!/bin/sh
+#
+# Copyright(C) 2008 Stephen Habermann & Andreas Ericsson
+#
+test_description='git rebase -p should preserve merges
+
+This test runs various incantations of "git rebase -p" and checks
+that merges are properly carried along
+'
+. ./test-lib.sh
+
+GIT_AUTHOR_EMAIL=bogus_email_address
+export GIT_AUTHOR_EMAIL
+
+#echo 'Setting up:
+#
+#A1--A2  <-- origin/master
+# \   \
+#  B1--M  <-- topic
+#   \
+#    B2  <-- origin/topic
+#
+#'
+
+test_expect_success 'setup for merge-preserving rebase' \
+	'echo First > A &&
+	git add A &&
+	git-commit -m "Add A1" &&
+	git checkout -b topic &&
+	echo Second > B &&
+	git add B &&
+	git-commit -m "Add B1" &&
+	git checkout -f master &&
+	echo Third >> A &&
+	git-commit -a -m "Modify A2" &&
+
+	git clone ./. clone1 &&
+	cd clone1 &&
+	git checkout -b topic origin/topic &&
+	git merge origin/master &&
+	cd ..
+
+	git clone ./. clone2
+	cd clone2 &&
+	git checkout -b topic origin/topic &&
+	git merge origin/master &&
+	cd .. &&
+
+	git checkout topic &&
+	echo Fourth >> B &&
+	git commit -a -m "Modify B2"
+'
+
+test_expect_failure 'git pull --rebase -p on moved topic' '
+	cd clone1 &&
+	git pull --rebase --preserve-merges &&
+	test $(git rev-list --all --pretty=oneline | grep "Modify A" | wc -l) = 1
+'
+
+test_expect_failure 'rebase -p merge on moved topic' '
+	cd ../clone2 &&
+	git fetch &&
+	git rebase -p origin/topic &&
+	test 1 = $(git rev-list --all --pretty=oneline | grep "Modify A" | wc -l) &&
+	test 1 = $(git rev-list --all --pretty=oneline | grep "Merge commit" | wc -l)
+'
+
+test_done

1.6.0.2.307.gc4275.dirty

No problem, it looks great.

This is awesome. Thanks for the insanely short turnaround. The GIT_EDITOR=: hack is neat. I did not think it would be that simple.

- Stephen

It requires a bit of testing though. All the t/t34* tests pass with all the patches applied, and some manual tries worked just fine too, but if you wanna give it a twirl where you work, that'd be great.

Actually, you should be able to use vanilla "git-rebase -i -p" without getting an editor by doing something like this:

GIT_EDITOR=: git rebase -i -p

but recommending a hack like that to work around a UI deficiency didn't really appeal to me. If Junio doesn't like the patches though, you could try using that.

Andreas Ericsson                   [email protected]
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Hi Andreas,

First of all, thanks for the work!

Unfortunately in my example workflow[1] posted earlier today your patch series does not work in the way I would like it to behave.

The following DAG is created by the commands below:

  -A---B      master
    \
     C---M    topic
      \ /
       D

git init
echo 1 >foo
git add foo
git commit -m 'first on master' # A
echo 2 >>foo
git commit -m 'second on master' foo # B
git checkout -b topic HEAD^
echo 1 >bar
git add bar
git commit -m 'first on topic' # C
git checkout -b subtopic
echo 1 >baz
git add baz
git commit -m 'first on subtopic' # D
git checkout topic
git merge --no-ff subtopic # M

If I now execute 'git rebase -p master topic', I get the following:

  -A---B            master
    \   \
     \   C'---M'    topic
      \      /
       C----D

But I would rather like to have the following:

  -A---B            master
        \
         C'---M'    topic
          \  /
           D'

Would such a behaviour possible at all?

Thanks,
Gábor

[1] http://article.gmane.org/gmane.comp.version-control.git/96548

...

I think that rebase -i -p was meant to deal only with the situation where the merged-in branch is from outside the topic branch. What you want is called "git-sequencer". Search the archives.

-- Hannes

See Johannes Sixt's reply (git sequencer).
What I provided was a hack to access existing functionality in a way that was previously not possible. While that can be neat in itself, the patch series doesn't alter how the merge-preserving rebase works in the slightest.

Andreas Ericsson                   [email protected]
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Following up on this old thread, I can't get M' to have the old parent D. I always see D change to D' and then topic is fast fowarded to D' instead of an M' showing up. (I've tried 1.6.0.2, my rebase-i-p changes, and sp/maint.)

Yes, I think it just takes the following patch:

@@ -251,7 +251,7 @@ pick_one_preserving_merges () {
                                GIT_AUTHOR_EMAIL="$GIT_AUTHOR_EMAIL" \
                                GIT_AUTHOR_DATE="$GIT_AUTHOR_DATE" \
                                output git merge $STRATEGY -m "$msg" \
-                                       $new_parents
+                                       --no-ff $new_parents
 

Applying this to either sp/maint or my rebase-i-p changes gets your desired output.

With the only caveat being that the subtopic branch stays pointing at the old D--since you are rebasing topic, it does not change where subtopic points when rewriting D -> D'.

Musing, I could see moving subtopic being possible, definitely with git sequencer, but also with a --other-branches-follow-rewrites flag of some sort that, after rewriting hash1->hash2, just finds any local branches pointing at hash1 and updates their refs to be hash2. Not that I'm really suggesting it, but I don't think it would be that hard.

Anyway, subtopic still pointing at D aside, I think your desired output makes sense, given you've explicitly told rebase to preserve merges. If you wanted a non-ff M in the first place, I think passing along a --no-ff to keep M' around is reasonable. And would otherwise be harmless.

I can write a test/patch for this unless you beat me to it or other think it is unreasonable.

- Stephen

Stephen, are you using this in production? How's it turning out?

Shawn, I haven't seen this in any of your branches. Overlooked or dropped? I think 1-2 are probably master material, while I'm not so sure about 3/3. Would you prefer a re-send that turns it into a 2-patch series, adding each test with the functionality it tests?

Let me know how you want it and I'll work something up tomorrow morning, gmt + 1.

Thanks

Andreas Ericsson                   [email protected]
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Kind of--I have not distributed a patched version of pull. But I have written test cases on our side and manually executing `GIT_EDITOR=:
git rebase -i -p` works very well.

Past occurrences aside, no one has needed to rebase a local merge yet.

I think it's great, but the primary problem will be getting devs to actually remember to use it. E.g. I don't think they will type out:

    git pull --rebase --preserve-rebase

Every time they pull. And they definitely don't do our current hack:

git fetch
GIT_EDITOR=: git rebase -i -p

I do have a wrapper shell script for people to use, but it hasn't seen wide adoption yet. We have a draconian hook script that tries to detect merges that should have been rebases and reject them, but it's disabled for tweaking right now--when it gets turned back on, I think more people will use the script.

In the long term, having "branch.name.preservemerges" and "branch.autosetuppreservemerges" config options to parallel the "branch.name.rebase" option and get us back to just "git pull" would be great.

I've been meaning to submit patches for these two config options--I figure I can hunt down how "branch.name.rebase" works and do the appropriate copy/paste, but I haven't dedicated any time to it yet.

Thanks,
Stephen

Thanks for the reminder. It just got lost in the shuffle. I dragged them out of the archives and will queue into this morning's update, so no need for a resend.

Shawn.

Hold off on that if you haven't already applied them. I just noticed something strange in passing 15 minutes ago that I need to investigate a bit more. I need to get home now though, so I won't have time to test it further until later tonight.

Andreas Ericsson                   [email protected]
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Its only in a topic branch right now. I'll schedule them into 'pu' today just so they are available, but I'll be happy to replace any (or all) of the patches when you come up with something better.

Shawn.

---

This builds on top of Andreas's work on git rebase -p. I basically copy/pasted how autosetuprebase works so that, if appropriately configured, git pull will do the "right thing" for our environment, i.e. rebasing and preserving merges.

I'm not sure how to handle patches on patches, so apologies if I did this wrong. Let me know if there are things I should be doing differently.

Thanks.

 Documentation/config.txt          |   20 ++++++++
 branch.c                          |   20 ++++++++
 cache.h                           |    9 ++++
 config.c                          |   15 ++++++
 environment.c                     |    1 +
 git-pull.sh                       |   10 +++-
 t/t3200-branch.sh                 |   89 ++++++++++++++++++++++++++++--------
 t/t3409-rebase-preserve-merges.sh |   65 ++++++++++++++++----------
 8 files changed, 182 insertions(+), 47 deletions(-)

index bea867d..0abf1b8 100644
@@ -438,6 +438,21 @@ branch.autosetuprebase::
 	branch to track another branch.
 	This option defaults to never.
 
+branch.autosetuppreservemerges::
+	When a new branch is created with 'git-branch' or 'git-checkout'
+	that tracks another branch, this variable tells git to set
+	up pull to rebase with preserve merges (see "branch.<name>.rebase").
+	When `never`, rebase is never automatically set to true.
+	When `local`, rebase is set to true for tracked branches of
+	other local branches.
+	When `remote`, rebase is set to true for tracked branches of
+	remote branches.
+	When `always`, rebase will be set to true for all tracking
+	branches.
+	See "branch.autosetupmerge" for details on how to set up a
+	branch to track another branch.
+	This option defaults to never.
+
 branch.<name>.remote::
 	When in branch <name>, it tells 'git-fetch' which remote to fetch.
 	If this option is not given, 'git-fetch' defaults to remote "origin".

@@ -471,6 +486,11 @@ branch.<name>.rebase::
 	it unless you understand the implications (see linkgit:git-rebase[1]
 	for details).
 
+branch.<name>.preservemerges::
+	When true, and branch.<name>.rebase is true, preserve merges when
+	rebasing the branch <name> on top of the fetched branch when
+	"git pull" is run.
+
 browser.<tool>.cmd::
 	Specify the command to invoke the specified browser. The
 	specified command is evaluated in shell with the URLs passed

index b1e59f2..a5e62c8 100644
@@ -47,6 +47,21 @@ static int should_setup_rebase(const struct tracking *tracking)
 	return 0;
 }
 
+static int should_setup_preservemerges(const struct tracking *tracking)
+{
+	switch (autopreservemerges) {
+	case AUTOPRESERVEMERGES_NEVER:
+		return 0;
+	case AUTOPRESERVEMERGES_LOCAL:
+		return tracking->remote == NULL;
+	case AUTOPRESERVEMERGES_REMOTE:
+		return tracking->remote != NULL;
+	case AUTOPRESERVEMERGES_ALWAYS:
+		return 1;
+	}
+	return 0;
+}
+
 /*
  * This is called when new_ref is branched off of orig_ref, and tries
  * to infer the settings for branch.<new_ref>.{remote,merge} from the

@@ -91,6 +106,11 @@ static int setup_tracking(const char *new_ref, const char *orig_ref,
 		git_config_set(key, "true");
 		printf("This branch will rebase on pull.\n");
 	}
+	if (should_setup_preservemerges(&tracking)) {
+		sprintf(key, "branch.%s.preservemerges", new_ref);
+		git_config_set(key, "true");
+		printf("This branch will preserve merges on pull.\n");
+	}
 	free(tracking.src);
 
 	return 0;

index de8c2b6..97be98c 100644
@@ -467,8 +467,17 @@ enum rebase_setup_type {
 	AUTOREBASE_ALWAYS,
 };
 
+enum preservemerges_setup_type {
+	AUTOPRESERVEMERGES_NEVER = 0,
+	AUTOPRESERVEMERGES_LOCAL,
+	AUTOPRESERVEMERGES_REMOTE,
+	AUTOPRESERVEMERGES_ALWAYS,
+};
+
+
 extern enum branch_track git_branch_track;
 extern enum rebase_setup_type autorebase;
+extern enum preservemerges_setup_type autopreservemerges;
 
 #define GIT_REPO_VERSION 0
 extern int repository_format_version;

index 53f04a0..6302f5a 100644
@@ -536,6 +536,21 @@ static int git_default_branch_config(const char *var, const char *value)
 			return error("Malformed value for %s", var);
 		return 0;
 	}
+	if (!strcmp(var, "branch.autosetuppreservemerges")) {
+		if (!value)
+			return config_error_nonbool(var);
+		else if (!strcmp(value, "never"))
+			autopreservemerges = AUTOPRESERVEMERGES_NEVER;
+		else if (!strcmp(value, "local"))
+			autopreservemerges = AUTOPRESERVEMERGES_LOCAL;
+		else if (!strcmp(value, "remote"))
+			autopreservemerges = AUTOPRESERVEMERGES_REMOTE;
+		else if (!strcmp(value, "always"))
+			autopreservemerges = AUTOPRESERVEMERGES_ALWAYS;
+		else
+			return error("Malformed value for %s", var);
+		return 0;
+	}
 
 	/* Add other config variables here and to Documentation/config.txt. */
 	return 0;

index 0c6d11f..72e735c 100644
@@ -42,6 +42,7 @@ enum safe_crlf safe_crlf = SAFE_CRLF_WARN;
 unsigned whitespace_rule_cfg = WS_DEFAULT_RULE;
 enum branch_track git_branch_track = BRANCH_TRACK_REMOTE;
 enum rebase_setup_type autorebase = AUTOREBASE_NEVER;
+enum preservemerges_setup_type autopreservemerges = AUTOPRESERVEMERGES_NEVER;
 
 /* This is set by setup_git_dir_gently() and/or git_default_config() */
 char *git_work_tree_cfg;

index 270a50d..03b7da0 100755
@@ -20,6 +20,7 @@ strategy_args= no_stat= no_commit= squash= no_ff= log_arg=
 curr_branch=$(git symbolic-ref -q HEAD)
 curr_branch_short=$(echo "$curr_branch" | sed "s|refs/heads/||")
 rebase=$(git config --bool branch.$curr_branch_short.rebase)
+preservemerges=$(git config --bool branch.$curr_branch_short.preservemerges)
 while :
 do
 	case "$1" in

@@ -59,7 +60,7 @@ do
 		rebase=true
 		;;
 	--preserve-merges) # no short option for this
-		preserve_merges="--preserve-merges"
+		preservemerges=true
 		rebase=true
 		;;
 	--no-r|--no-re|--no-reb|--no-reba|--no-rebas|--no-rebase)

@@ -181,9 +182,14 @@ then
 	exit
 fi
 
+if test true = "$preservemerges"
+then
+	preservemerges_flag="--preserve-merges"
+fi
+
 merge_name=$(git fmt-merge-msg $log_arg <"$GIT_DIR/FETCH_HEAD") || exit
 test true = "$rebase" &&
-	exec git-rebase $preserve_merges $strategy_args --onto $merge_head \
+	exec git-rebase $preservemerges_flag $strategy_args --onto $merge_head \
 	${oldremoteref:-$merge_head}
 exec git-merge $no_stat $no_commit $squash $no_ff $log_arg $strategy_args \
 	"$merge_name" HEAD $merge_head

1.6.0.2