rebase -r: support merge strategies other than recursive

PATCH Superseded Johannes Schindelin via GitGitGadget 5 participants 39 messages Jul 25, 2019 – Sep 9, 2019

Subsystems (2)

Documentation Kbuild

lore

Share & tools

lore

Johannes Schindelin via GitGitGadget

author

2019-07-25 10:11 UTC #0 lore raw

cover rebase -r: support merge strategies other than recursive

This is the most notable shortcoming that --rebase-merges has, still, relative to --preserve-merges' capabilities: it does not support passing custom merge strategies or custom merge strategy options.

Let's fix this.

While working on this patch series, of course I tried to copy-edit the test cases we have, to cover --preserve-merges' support for merge strategies. Oh my, did I regret this decision as soon as my eyes set sight on t3427-rebase-subtree.sh!

At first I tried my best to make heads or tails of t3427, for way too long. In the end the only way to understand what the heck it tries to do was to actually fix it. That's why this patch series looks as if it focuses on t3427 rather than on adding support for custom merge strategies to the --rebase-merges mode.

As a consolation to myself, this work was actually worth it, surprising as that may look. Not only is t3427 now really easy to understand, adding that test case for --rebase-merges -Xsubtree tickled the sequencer enough to reveal a long-standing bug: the --onto option was simply ignored when passed together with --rebase-merges and --root. For good measure, this patch series addresses this bug, too.

Johannes Schindelin (12):
t3427: add a clarifying comment
t3427: simplify the setup test case significantly
t3427: move the filter-branch invocation into the setup case
t3427: condense the unnecessarily repetitive test cases into three
t3427: fix erroneous assumption
t3427: accommodate for the rebase --merge backend having been
replaced
t3427: fix another incorrect assumption
t3427: mark two test cases as requiring support for git rebase -p rebase -r: support merge strategies other than recursive t/lib-rebase: prepare for testing git rebase --rebase-merges
t3418: test rebase -r with merge strategies
rebase -r: do not (re-)generate root commits with --root *and*

    `--onto`

 Documentation/git-rebase.txt           |   2 -
 builtin/rebase.c                       |  16 +--
 sequencer.c                            |  18 ++-
 sequencer.h                            |   6 +
 t/lib-rebase.sh                        |   8 +-
 t/t3418-rebase-continue.sh             |  14 +++
 t/t3422-rebase-incompatible-options.sh |  10 --
 t/t3427-rebase-subtree.sh              | 150 ++++++++++++-------------
 t/t3430-rebase-merges.sh               |  21 ++++
 9 files changed, 134 insertions(+), 111 deletions(-)

base-commit: 082ef75b7bfc90ac236afbb857a9552a026832b8
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-294%2Fdscho%2Frebase-r-with-strategies-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-294/dscho/rebase-r-with-strategies-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/294

Signature · 1 line

gitgitgadget

Johannes Schindelin via GitGitGadget

author

2019-07-25 10:11 UTC #2 lore raw

PATCH 2/12 t3427: simplify the `setup` test case significantly

From: Johannes Schindelin <[email protected]>

It still does the very same thing as before, but expresses it in a much more succinct (and still quite readable) manner.

Signed-off-by Johannes Schindelin <[email protected]>

---
 t/t3427-rebase-subtree.sh | 36 +++++++++++++++---------------------
 1 file changed, 15 insertions(+), 21 deletions(-)

t/t3427-rebase-subtree.sh +15 ~~-21~~

index a9b93c39d0..f41a08e436 100755
@@ -41,27 +41,21 @@ commit_message() {
 
 test_expect_success 'setup' '
 	test_commit README &&
-	mkdir files &&
-	(
-		cd files &&
-		git init &&
-		test_commit master1 &&
-		test_commit master2 &&
-		test_commit master3
-	) &&
-	git fetch files master &&
-	git branch files-master FETCH_HEAD &&
-	git read-tree --prefix=files_subtree files-master &&
-	git checkout -- files_subtree &&
-	tree=$(git write-tree) &&
-	head=$(git rev-parse HEAD) &&
-	rev=$(git rev-parse --verify files-master^0) &&
-	commit=$(git commit-tree -p $head -p $rev -m "Add subproject master" $tree) &&
-	git update-ref HEAD $commit &&
-	(
-		cd files_subtree &&
-		test_commit master4
-	) &&
+
+	git init files &&
+	test_commit -C files master1 &&
+	test_commit -C files master2 &&
+	test_commit -C files master3 &&
+
+	: perform subtree merge into files_subtree/ &&
+	git fetch files refs/heads/master:refs/heads/files-master &&
+	git merge -s ours --no-commit --allow-unrelated-histories \
+		files-master &&
+	git read-tree --prefix=files_subtree -u files-master &&
+	git commit -m "Add subproject master" &&
+
+	: add two extra commits to rebase &&
+	test_commit -C files_subtree master4 &&
 	test_commit files_subtree/master5
 '

Signature · 1 line

gitgitgadget

Johannes Schindelin via GitGitGadget

author

2019-07-25 10:11 UTC #1 lore raw

PATCH 1/12 t3427: add a clarifying comment

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

The flow of this test script is outright confusing, and to start the endeavor to address that, let's describe what this test is all about, and how it tries to do it.

Signed-off-by Johannes Schindelin <[email protected]>

---
 t/t3427-rebase-subtree.sh | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

t/t3427-rebase-subtree.sh +28 -0

index 3780877e4e..a9b93c39d0 100755
@@ -11,6 +11,34 @@ commit_message() {
 	git log --pretty=format:%s -1 "$1"
 }
 
+# There are a few bugs in the rebase with regards to the subtree strategy, and
+# this test script tries to document them.  First, the following commit history
+# is generated (the onelines are shown, time flows from left to right):
+#
+# master1 - master2 - master3
+#                             \
+# README ---------------------- Add subproject master - master4 - files_subtree/master5
+#
+# Where the merge moves the files master[123].t into the subdirectory
+# files_subtree/ and master4 as well as files_subtree/master5 add files to that
+# directory directly.
+#
+# Then, in subsequent test cases, `git filter-branch` is used to distill just
+# the commits that touch files_subtree/. To give it a final pre-rebase touch,
+# an empty commit is added on top. The pre-rebase commit history looks like
+# this:
+#
+# Add subproject master - master4 - files_subtree/master5 - Empty commit
+#
+# where the root commit adds three files: master1.t, master2.t and master3.t.
+#
+# This commit history is then rebased onto `master3` with the
+# `-Xsubtree=files_subtree` option in three different ways:
+#
+# 1. using `--preserve-merges`
+# 2. using `--preserve-merges` and --keep-empty
+# 3. without specifying a rebase backend
+
 test_expect_success 'setup' '
 	test_commit README &&
 	mkdir files &&

Signature · 1 line

gitgitgadget

Johannes Schindelin via GitGitGadget

author

2019-07-25 10:11 UTC #4 lore raw

PATCH 4/12 t3427: condense the unnecessarily repetitive test cases into three

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

Previously, this test script performed essentially three rebases and verified breakages by testing the post-rebase commits' messages.

To do so, the rebases were performed multiple times, though, once per commit message to test. This wastes electricity (and CO2) and time.

Let's condense the test cases to the essential number: the number of different rebases to validate.

On Windows, where the scripted nature of the --preserve-merges backend hurts performance rather badly, this reduces the overall runtime in this developer's setup from ~1m to ~28s while still performing the exact same testing as before.

Signed-off-by Johannes Schindelin <[email protected]>

---
 t/t3427-rebase-subtree.sh | 56 ++++++++-------------------------------
 1 file changed, 11 insertions(+), 45 deletions(-)

t/t3427-rebase-subtree.sh +11 ~~-45~~

index c0e6a49b61..7d3ba766de 100755
@@ -64,65 +64,31 @@ test_expect_success 'setup' '
 '
 
 # FAILURE: Does not preserve master4.
-test_expect_failure 'Rebase -Xsubtree --preserve-merges --onto commit 4' '
+test_expect_failure 'Rebase -Xsubtree --preserve-merges --onto commit' '
 	reset_rebase &&
-	git checkout -b rebase-preserve-merges-4 to-rebase &&
-	git rebase -Xsubtree=files_subtree --preserve-merges --onto files-master master &&
-	verbose test "$(commit_message HEAD~)" = "files_subtree/master4"
-'
-
-# FAILURE: Does not preserve master5.
-test_expect_failure 'Rebase -Xsubtree --preserve-merges --onto commit 5' '
-	reset_rebase &&
-	git checkout -b rebase-preserve-merges-5 to-rebase &&
+	git checkout -b rebase-preserve-merges to-rebase &&
 	git rebase -Xsubtree=files_subtree --preserve-merges --onto files-master master &&
+	verbose test "$(commit_message HEAD~)" = "files_subtree/master4" &&
 	verbose test "$(commit_message HEAD)" = "files_subtree/master5"
 '
 
 # FAILURE: Does not preserve master4.
-test_expect_failure 'Rebase -Xsubtree --keep-empty --preserve-merges --onto commit 4' '
+test_expect_failure 'Rebase -Xsubtree --keep-empty --preserve-merges --onto commit' '
 	reset_rebase &&
-	git checkout -b rebase-keep-empty-4 to-rebase &&
-	git rebase -Xsubtree=files_subtree --keep-empty --preserve-merges --onto files-master master &&
-	verbose test "$(commit_message HEAD~2)" = "files_subtree/master4"
-'
-
-# FAILURE: Does not preserve master5.
-test_expect_failure 'Rebase -Xsubtree --keep-empty --preserve-merges --onto commit 5' '
-	reset_rebase &&
-	git checkout -b rebase-keep-empty-5 to-rebase &&
-	git rebase -Xsubtree=files_subtree --keep-empty --preserve-merges --onto files-master master &&
-	verbose test "$(commit_message HEAD~)" = "files_subtree/master5"
-'
-
-# FAILURE: Does not preserve Empty.
-test_expect_failure 'Rebase -Xsubtree --keep-empty --preserve-merges --onto empty commit' '
-	reset_rebase &&
-	git checkout -b rebase-keep-empty-empty to-rebase &&
+	git checkout -b rebase-keep-empty to-rebase &&
 	git rebase -Xsubtree=files_subtree --keep-empty --preserve-merges --onto files-master master &&
+	verbose test "$(commit_message HEAD~2)" = "files_subtree/master4" &&
+	verbose test "$(commit_message HEAD~)" = "files_subtree/master5" &&
 	verbose test "$(commit_message HEAD)" = "Empty commit"
 '
 
 # FAILURE: fatal: Could not parse object
-test_expect_failure 'Rebase -Xsubtree --onto commit 4' '
-	reset_rebase &&
-	git checkout -b rebase-onto-4 to-rebase &&
-	git rebase -Xsubtree=files_subtree --onto files-master master &&
-	verbose test "$(commit_message HEAD~2)" = "files_subtree/master4"
-'
-
-# FAILURE: fatal: Could not parse object
-test_expect_failure 'Rebase -Xsubtree --onto commit 5' '
-	reset_rebase &&
-	git checkout -b rebase-onto-5 to-rebase &&
-	git rebase -Xsubtree=files_subtree --onto files-master master &&
-	verbose test "$(commit_message HEAD~)" = "files_subtree/master5"
-'
-# FAILURE: fatal: Could not parse object
-test_expect_failure 'Rebase -Xsubtree --onto empty commit' '
+test_expect_failure 'Rebase -Xsubtree --onto commit' '
 	reset_rebase &&
-	git checkout -b rebase-onto-empty to-rebase &&
+	git checkout -b rebase-onto to-rebase &&
 	git rebase -Xsubtree=files_subtree --onto files-master master &&
+	verbose test "$(commit_message HEAD~2)" = "files_subtree/master4" &&
+	verbose test "$(commit_message HEAD~)" = "files_subtree/master5" &&
 	verbose test "$(commit_message HEAD)" = "Empty commit"
 '

Signature · 1 line

gitgitgadget

Johannes Schindelin via GitGitGadget

author

2019-07-25 10:11 UTC #5 lore raw

PATCH 5/12 t3427: fix erroneous assumption

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

Apart from the setup test case, t3427-rebase-subtree.sh is made up exclusively of demonstrations of breakages. The tricky thing about such demonstrations is that they are often buggy themselves.

In this instance, somewhere over the course of the six iterations of the patch that eventually made it into Git's master as 5f35900849e (contrib/subtree: Add a test for subtree rebase that loses commits, 2016-06-28), the commit message "files_subtree/master4" was changed to just "master4", but the test cases still expected the old commit message.

Let's fix this, at long last.

Signed-off-by Johannes Schindelin <[email protected]>

---
 t/t3427-rebase-subtree.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

t/t3427-rebase-subtree.sh +3 -3

index 7d3ba766de..8c4ddd3408 100755
@@ -68,7 +68,7 @@ test_expect_failure 'Rebase -Xsubtree --preserve-merges --onto commit' '
 	reset_rebase &&
 	git checkout -b rebase-preserve-merges to-rebase &&
 	git rebase -Xsubtree=files_subtree --preserve-merges --onto files-master master &&
-	verbose test "$(commit_message HEAD~)" = "files_subtree/master4" &&
+	verbose test "$(commit_message HEAD~)" = "master4" &&
 	verbose test "$(commit_message HEAD)" = "files_subtree/master5"
 '

@@ -77,7 +77,7 @@ test_expect_failure 'Rebase -Xsubtree --keep-empty --preserve-merges --onto comm
 	reset_rebase &&
 	git checkout -b rebase-keep-empty to-rebase &&
 	git rebase -Xsubtree=files_subtree --keep-empty --preserve-merges --onto files-master master &&
-	verbose test "$(commit_message HEAD~2)" = "files_subtree/master4" &&
+	verbose test "$(commit_message HEAD~2)" = "master4" &&
 	verbose test "$(commit_message HEAD~)" = "files_subtree/master5" &&
 	verbose test "$(commit_message HEAD)" = "Empty commit"
 '

@@ -87,7 +87,7 @@ test_expect_failure 'Rebase -Xsubtree --onto commit' '
 	reset_rebase &&
 	git checkout -b rebase-onto to-rebase &&
 	git rebase -Xsubtree=files_subtree --onto files-master master &&
-	verbose test "$(commit_message HEAD~2)" = "files_subtree/master4" &&
+	verbose test "$(commit_message HEAD~2)" = "master4" &&
 	verbose test "$(commit_message HEAD~)" = "files_subtree/master5" &&
 	verbose test "$(commit_message HEAD)" = "Empty commit"
 '

Signature · 1 line

gitgitgadget

Johannes Schindelin via GitGitGadget

author

2019-07-25 10:11 UTC #3 lore raw

PATCH 3/12 t3427: move the `filter-branch` invocation into the `setup` case

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

The step to prepare a pre-rebase commit history is _identical_ in _all_ of the test cases (except of course the setup case). It should therefore clearly a part of the setup test case instead.

As the git filter-branch command is quite costly on platforms where Unix shell scripting is simply slow (meaning: on Windows), this shaves off a noticeable part of the runtime: in this developer's setup, the time was reduced from ~1m25s to ~1m.

Signed-off-by Johannes Schindelin <[email protected]>

---
 t/t3427-rebase-subtree.sh | 38 +++++++++++++-------------------------
 1 file changed, 13 insertions(+), 25 deletions(-)

t/t3427-rebase-subtree.sh +13 ~~-25~~

index f41a08e436..c0e6a49b61 100755
@@ -56,15 +56,17 @@ test_expect_success 'setup' '
 
 	: add two extra commits to rebase &&
 	test_commit -C files_subtree master4 &&
-	test_commit files_subtree/master5
+	test_commit files_subtree/master5 &&
+
+	git checkout -b to-rebase &&
+	git filter-branch --prune-empty -f --subdirectory-filter files_subtree &&
+	git commit -m "Empty commit" --allow-empty
 '
 
 # FAILURE: Does not preserve master4.
 test_expect_failure 'Rebase -Xsubtree --preserve-merges --onto commit 4' '
 	reset_rebase &&
-	git checkout -b rebase-preserve-merges-4 master &&
-	git filter-branch --prune-empty -f --subdirectory-filter files_subtree &&
-	git commit -m "Empty commit" --allow-empty &&
+	git checkout -b rebase-preserve-merges-4 to-rebase &&
 	git rebase -Xsubtree=files_subtree --preserve-merges --onto files-master master &&
 	verbose test "$(commit_message HEAD~)" = "files_subtree/master4"
 '

@@ -72,9 +74,7 @@ test_expect_failure 'Rebase -Xsubtree --preserve-merges --onto commit 4' '
 # FAILURE: Does not preserve master5.
 test_expect_failure 'Rebase -Xsubtree --preserve-merges --onto commit 5' '
 	reset_rebase &&
-	git checkout -b rebase-preserve-merges-5 master &&
-	git filter-branch --prune-empty -f --subdirectory-filter files_subtree &&
-	git commit -m "Empty commit" --allow-empty &&
+	git checkout -b rebase-preserve-merges-5 to-rebase &&
 	git rebase -Xsubtree=files_subtree --preserve-merges --onto files-master master &&
 	verbose test "$(commit_message HEAD)" = "files_subtree/master5"
 '

@@ -82,9 +82,7 @@ test_expect_failure 'Rebase -Xsubtree --preserve-merges --onto commit 5' '
 # FAILURE: Does not preserve master4.
 test_expect_failure 'Rebase -Xsubtree --keep-empty --preserve-merges --onto commit 4' '
 	reset_rebase &&
-	git checkout -b rebase-keep-empty-4 master &&
-	git filter-branch --prune-empty -f --subdirectory-filter files_subtree &&
-	git commit -m "Empty commit" --allow-empty &&
+	git checkout -b rebase-keep-empty-4 to-rebase &&
 	git rebase -Xsubtree=files_subtree --keep-empty --preserve-merges --onto files-master master &&
 	verbose test "$(commit_message HEAD~2)" = "files_subtree/master4"
 '

@@ -92,9 +90,7 @@ test_expect_failure 'Rebase -Xsubtree --keep-empty --preserve-merges --onto comm
 # FAILURE: Does not preserve master5.
 test_expect_failure 'Rebase -Xsubtree --keep-empty --preserve-merges --onto commit 5' '
 	reset_rebase &&
-	git checkout -b rebase-keep-empty-5 master &&
-	git filter-branch --prune-empty -f --subdirectory-filter files_subtree &&
-	git commit -m "Empty commit" --allow-empty &&
+	git checkout -b rebase-keep-empty-5 to-rebase &&
 	git rebase -Xsubtree=files_subtree --keep-empty --preserve-merges --onto files-master master &&
 	verbose test "$(commit_message HEAD~)" = "files_subtree/master5"
 '

@@ -102,9 +98,7 @@ test_expect_failure 'Rebase -Xsubtree --keep-empty --preserve-merges --onto comm
 # FAILURE: Does not preserve Empty.
 test_expect_failure 'Rebase -Xsubtree --keep-empty --preserve-merges --onto empty commit' '
 	reset_rebase &&
-	git checkout -b rebase-keep-empty-empty master &&
-	git filter-branch --prune-empty -f --subdirectory-filter files_subtree &&
-	git commit -m "Empty commit" --allow-empty &&
+	git checkout -b rebase-keep-empty-empty to-rebase &&
 	git rebase -Xsubtree=files_subtree --keep-empty --preserve-merges --onto files-master master &&
 	verbose test "$(commit_message HEAD)" = "Empty commit"
 '

@@ -112,9 +106,7 @@ test_expect_failure 'Rebase -Xsubtree --keep-empty --preserve-merges --onto empt
 # FAILURE: fatal: Could not parse object
 test_expect_failure 'Rebase -Xsubtree --onto commit 4' '
 	reset_rebase &&
-	git checkout -b rebase-onto-4 master &&
-	git filter-branch --prune-empty -f --subdirectory-filter files_subtree &&
-	git commit -m "Empty commit" --allow-empty &&
+	git checkout -b rebase-onto-4 to-rebase &&
 	git rebase -Xsubtree=files_subtree --onto files-master master &&
 	verbose test "$(commit_message HEAD~2)" = "files_subtree/master4"
 '

@@ -122,18 +114,14 @@ test_expect_failure 'Rebase -Xsubtree --onto commit 4' '
 # FAILURE: fatal: Could not parse object
 test_expect_failure 'Rebase -Xsubtree --onto commit 5' '
 	reset_rebase &&
-	git checkout -b rebase-onto-5 master &&
-	git filter-branch --prune-empty -f --subdirectory-filter files_subtree &&
-	git commit -m "Empty commit" --allow-empty &&
+	git checkout -b rebase-onto-5 to-rebase &&
 	git rebase -Xsubtree=files_subtree --onto files-master master &&
 	verbose test "$(commit_message HEAD~)" = "files_subtree/master5"
 '
 # FAILURE: fatal: Could not parse object
 test_expect_failure 'Rebase -Xsubtree --onto empty commit' '
 	reset_rebase &&
-	git checkout -b rebase-onto-empty master &&
-	git filter-branch --prune-empty -f --subdirectory-filter files_subtree &&
-	git commit -m "Empty commit" --allow-empty &&
+	git checkout -b rebase-onto-empty to-rebase &&
 	git rebase -Xsubtree=files_subtree --onto files-master master &&
 	verbose test "$(commit_message HEAD)" = "Empty commit"
 '

Signature · 1 line

gitgitgadget

Johannes Schindelin via GitGitGadget

author

2019-07-25 10:11 UTC #7 lore raw

PATCH 7/12 t3427: fix another incorrect assumption

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

The test case that concerns git rebase -Xsubtree (with the default rebase backend, not with --preserve-merges) starts out with a pre-rebase commit history that begins with a commit that introduces three files: master1.t, master2.t and master3.t.

This commit was generated by passing a subtree merge commit through git filter-branch --subdirectory-filter, so it looks as if this commit really introduces all those files.

The commit history onto which this commit is then rebased, however, introduced those files in individual commits. For that reason, the rebase will fail, it _must_ fail, because the first pick results in no changes to be committed.

Let's fix the test case to expect exactly this situation.

With this change, we can mark the original bug that this test case tried to demonstrate as fixed.

Signed-off-by Johannes Schindelin <[email protected]>

---
 t/t3427-rebase-subtree.sh | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

t/t3427-rebase-subtree.sh +4 -3

index b490919c60..c0ff3241e4 100755
@@ -82,11 +82,12 @@ test_expect_failure 'Rebase -Xsubtree --keep-empty --preserve-merges --onto comm
 	verbose test "$(commit_message HEAD)" = "Empty commit"
 '
 
-# FAILURE: fatal: Could not parse object
-test_expect_failure 'Rebase -Xsubtree --keep-empty --onto commit' '
+test_expect_success 'Rebase -Xsubtree --keep-empty --onto commit' '
 	reset_rebase &&
 	git checkout -b rebase-onto to-rebase &&
-	git rebase -Xsubtree=files_subtree --keep-empty --onto files-master master &&
+	test_must_fail git rebase -Xsubtree=files_subtree --keep-empty --onto files-master master &&
+	: first pick results in no changes &&
+	git rebase --continue &&
 	verbose test "$(commit_message HEAD~2)" = "master4" &&
 	verbose test "$(commit_message HEAD~)" = "files_subtree/master5" &&
 	verbose test "$(commit_message HEAD)" = "Empty commit"

Signature · 1 line

gitgitgadget

Johannes Schindelin via GitGitGadget

author

2019-07-25 10:11 UTC #9 lore raw

PATCH 9/12 rebase -r: support merge strategies other than `recursive`

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

We already support merge strategies in the sequencer, but only for pick commands.

With this commit, we now also support them in merge commands. The approach is simple: if any merge strategy option is specified, or if any merge strategy other than recursive is specified, we simply spawn the git merge command. Otherwise, we handle the merge in-process just as before.

Signed-off-by Johannes Schindelin <[email protected]>

---
 Documentation/git-rebase.txt           |  2 --
 builtin/rebase.c                       |  9 ---------
 sequencer.c                            | 14 ++++++++++++--
 t/t3422-rebase-incompatible-options.sh | 10 ----------
 t/t3430-rebase-merges.sh               | 21 +++++++++++++++++++++
 5 files changed, 33 insertions(+), 23 deletions(-)

Documentation/git-rebase.txt +0 -2

index f5e6ae3907..f67f96425c 100644
@@ -543,8 +543,6 @@ In addition, the following pairs of options are incompatible:
  * --preserve-merges and --interactive
  * --preserve-merges and --signoff
  * --preserve-merges and --rebase-merges
- * --rebase-merges and --strategy
- * --rebase-merges and --strategy-option
 
 BEHAVIORAL DIFFERENCES
 -----------------------

builtin/rebase.c +0 -9

index 9c52144fc4..c1ea617125 100644
@@ -1815,15 +1815,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			      "'--reschedule-failed-exec'"));
 	}
 
-	if (options.rebase_merges) {
-		if (strategy_options.nr)
-			die(_("cannot combine '--rebase-merges' with "
-			      "'--strategy-option'"));
-		if (options.strategy)
-			die(_("cannot combine '--rebase-merges' with "
-			      "'--strategy'"));
-	}
-
 	if (!options.root) {
 		if (argc < 1) {
 			struct branch *branch;

sequencer.c +12 -2

index 334de14542..d228448cd8 100644
@@ -3256,6 +3256,9 @@ static int do_merge(struct repository *r,
 	struct commit *head_commit, *merge_commit, *i;
 	struct commit_list *bases, *j, *reversed = NULL;
 	struct commit_list *to_merge = NULL, **tail = &to_merge;
+	const char *strategy = !opts->xopts_nr &&
+		(!opts->strategy || !strcmp(opts->strategy, "recursive")) ?
+		NULL : opts->strategy;
 	struct merge_options o;
 	int merge_arg_len, oneline_offset, can_fast_forward, ret, k;
 	static struct lock_file lock;

@@ -3404,7 +3407,7 @@ static int do_merge(struct repository *r,
 		goto leave_merge;
 	}
 
-	if (to_merge->next) {
+	if (strategy || to_merge->next) {
 		/* Octopus merge */
 		struct child_process cmd = CHILD_PROCESS_INIT;

@@ -3418,7 +3421,14 @@ static int do_merge(struct repository *r,
 		cmd.git_cmd = 1;
 		argv_array_push(&cmd.args, "merge");
 		argv_array_push(&cmd.args, "-s");
-		argv_array_push(&cmd.args, "octopus");
+		if (!strategy)
+			argv_array_push(&cmd.args, "octopus");
+		else {
+			argv_array_push(&cmd.args, strategy);
+			for (k = 0; k < opts->xopts_nr; k++)
+				argv_array_pushf(&cmd.args,
+						 "-X%s", opts->xopts[k]);
+		}
 		argv_array_push(&cmd.args, "--no-edit");
 		argv_array_push(&cmd.args, "--no-ff");
 		argv_array_push(&cmd.args, "--no-log");

t/t3422-rebase-incompatible-options.sh +0 ~~-10~~

index bb78a6ec86..596caf168a 100755
@@ -75,14 +75,4 @@ test_expect_success '--preserve-merges incompatible with --rebase-merges' '
 	test_must_fail git rebase --preserve-merges --rebase-merges A
 '
 
-test_expect_success '--rebase-merges incompatible with --strategy' '
-	git checkout B^0 &&
-	test_must_fail git rebase --rebase-merges -s resolve A
-'
-
-test_expect_success '--rebase-merges incompatible with --strategy-option' '
-	git checkout B^0 &&
-	test_must_fail git rebase --rebase-merges -Xignore-space-change A
-'
-
 test_done

t/t3430-rebase-merges.sh +21 -0

index 42ba5b9f09..8ea6ff3548 100755
@@ -412,4 +412,25 @@ test_expect_success '--continue after resolving conflicts after a merge' '
 	test_path_is_missing .git/MERGE_HEAD
 '
 
+test_expect_success '--rebase-merges with strategies' '
+	git checkout -b with-a-strategy F &&
+	test_tick &&
+	git merge -m "Merge conflicting-G" conflicting-G &&
+
+	: first, test with a merge strategy option &&
+	git rebase -ir -Xtheirs G &&
+	echo conflicting-G >expect &&
+	test_cmp expect G.t &&
+
+	: now, try with a merge strategy other than recursive &&
+	git reset --hard @{1} &&
+	write_script git-merge-override <<-\EOF &&
+	echo overridden$1 >>G.t
+	git add G.t
+	EOF
+	PATH="$PWD:$PATH" git rebase -ir -s override -Xxopt G &&
+	test_write_lines G overridden--xopt >expect &&
+	test_cmp expect G.t
+'
+
 test_done

Signature · 1 line

gitgitgadget

Johannes Schindelin via GitGitGadget

author

2019-07-25 10:11 UTC #10 lore raw

PATCH 10/12 t/lib-rebase: prepare for testing `git rebase --rebase-merges`

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

The format of the todo list is quite a bit different in the --rebase-merges mode; Let's prepare the fake editor to handle those todo lists properly, too.

Signed-off-by Johannes Schindelin <[email protected]>

---
 t/lib-rebase.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

t/lib-rebase.sh +4 -4

index 7ea30e5006..662a958575 100644
@@ -44,10 +44,10 @@ set_fake_editor () {
 	rm -f "$1"
 	echo 'rebase -i script before editing:'
 	cat "$1".tmp
-	action=pick
+	action=\&
 	for line in $FAKE_LINES; do
 		case $line in
-		pick|p|squash|s|fixup|f|edit|e|reword|r|drop|d)
+		pick|p|squash|s|fixup|f|edit|e|reword|r|drop|d|label|l|reset|r|merge|m)
 			action="$line";;
 		exec_*|x_*|break|b)
 			echo "$line" | sed 's/_/ /g' >> "$1";;

@@ -61,8 +61,8 @@ set_fake_editor () {
 			echo "$action XXXXXXX False commit" >> "$1"
 			action=pick;;
 		*)
-			sed -n "${line}s/^pick/$action/p" < "$1".tmp >> "$1"
-			action=pick;;
+			sed -n "${line}s/^[a-z][a-z]*/$action/p" < "$1".tmp >> "$1"
+			action=\&;;
 		esac
 	done
 	echo 'rebase -i script after editing:'

Signature · 1 line

gitgitgadget

Johannes Schindelin via GitGitGadget

author

2019-07-25 10:11 UTC #16 lore raw

PATCH 12/12 rebase -r: do not (re-)generate root commits with `--root` *and* `--onto`

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

When rebasing a complete commit history onto a given commit, it is pretty obvious that the root commits should be rebased on top of said given commit.

To test this, let's kill two birds with one stone and add a test case to t3427-rebase-subtree.sh that not only demonstrates that this works, but also that git rebase -r works with merge strategies now.

Signed-off-by Johannes Schindelin <[email protected]>

---
 builtin/rebase.c          |  7 +++++--
 sequencer.c               |  4 +++-
 sequencer.h               |  6 ++++++
 t/t3427-rebase-subtree.sh | 11 +++++++++++
 4 files changed, 25 insertions(+), 3 deletions(-)

builtin/rebase.c +5 -2

index c1ea617125..6a789c4421 100644
@@ -62,7 +62,7 @@ struct rebase_options {
 	const char *onto_name;
 	const char *revisions;
 	const char *switch_to;
-	int root;
+	int root, root_with_onto;
 	struct object_id *squash_onto;
 	struct commit *restrict_revision;
 	int dont_finish_rebase;

@@ -374,6 +374,7 @@ static int run_rebase_interactive(struct rebase_options *opts,
 	flags |= abbreviate_commands ? TODO_LIST_ABBREVIATE_CMDS : 0;
 	flags |= opts->rebase_merges ? TODO_LIST_REBASE_MERGES : 0;
 	flags |= opts->rebase_cousins > 0 ? TODO_LIST_REBASE_COUSINS : 0;
+	flags |= opts->root_with_onto ? TODO_LIST_ROOT_WITH_ONTO : 0;
 	flags |= command == ACTION_SHORTEN_OIDS ? TODO_LIST_SHORTEN_IDS : 0;
 
 	switch (command) {

@@ -1845,7 +1846,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			options.squash_onto = &squash_onto;
 			options.onto_name = squash_onto_name =
 				xstrdup(oid_to_hex(&squash_onto));
-		}
+		} else
+			options.root_with_onto = 1;
+
 		options.upstream_name = NULL;
 		options.upstream = NULL;
 		if (argc > 1)

sequencer.c +3 -1

index d228448cd8..ca119c84e5 100644
@@ -4440,6 +4440,7 @@ static int make_script_with_merges(struct pretty_print_context *pp,
 {
 	int keep_empty = flags & TODO_LIST_KEEP_EMPTY;
 	int rebase_cousins = flags & TODO_LIST_REBASE_COUSINS;
+	int root_with_onto = flags & TODO_LIST_ROOT_WITH_ONTO;
 	struct strbuf buf = STRBUF_INIT, oneline = STRBUF_INIT;
 	struct strbuf label = STRBUF_INIT;
 	struct commit_list *commits = NULL, **tail = &commits, *iter;

@@ -4606,7 +4607,8 @@ static int make_script_with_merges(struct pretty_print_context *pp,
 
 		if (!commit)
 			strbuf_addf(out, "%s %s\n", cmd_reset,
-				    rebase_cousins ? "onto" : "[new root]");
+				    rebase_cousins || root_with_onto ?
+				    "onto" : "[new root]");
 		else {
 			const char *to = NULL;

sequencer.h +6 -0

index 0c494b83d4..d506081d3c 100644
@@ -142,6 +142,12 @@ int sequencer_remove_state(struct replay_opts *opts);
  */
 #define TODO_LIST_REBASE_COUSINS (1U << 4)
 #define TODO_LIST_APPEND_TODO_HELP (1U << 5)
+/*
+ * When generating a script that rebases merges with `--root` *and* with
+ * `--onto`, we do not want to re-generate the root commits.
+ */
+#define TODO_LIST_ROOT_WITH_ONTO (1U << 6)
+
 
 int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
 			  const char **argv, unsigned flags);

t/t3427-rebase-subtree.sh +11 -0

index 7a37235768..39e348de16 100755
@@ -93,4 +93,15 @@ test_expect_success 'Rebase -Xsubtree --keep-empty --onto commit' '
 	verbose test "$(commit_message HEAD)" = "Empty commit"
 '
 
+test_expect_success 'Rebase -Xsubtree --keep-empty --rebase-merges --onto commit' '
+	reset_rebase &&
+	git checkout -b rebase-merges-onto to-rebase &&
+	test_must_fail git rebase -Xsubtree=files_subtree --keep-empty --rebase-merges --onto files-master --root &&
+	: first pick results in no changes &&
+	git rebase --continue &&
+	verbose test "$(commit_message HEAD~2)" = "master4" &&
+	verbose test "$(commit_message HEAD~)" = "files_subtree/master5" &&
+	verbose test "$(commit_message HEAD)" = "Empty commit"
+'
+
 test_done

Signature · 1 line

gitgitgadget

Johannes Schindelin via GitGitGadget

author

2019-07-25 10:11 UTC #8 lore raw

PATCH 8/12 t3427: mark two test cases as requiring support for `git rebase -p`

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

To prepare for the long game, where the --preserve-merges backend will be dropped eventually, we already introduced the REBASE_P prerequisite to allow saving time by skipping the now-almost-pointless test cases that verify that that backend works as expected.

Due to the nature of the tests in t3427 (test_expect_failure is happy as long as the scriptlet fails, whether it is for the intended reason or because git-rebase--preserve-merges.sh was deleted), these two test cases were missed.

When running with GIT_TEST_SKIP_REBASE_P=OhYesPlease, this drops the overall run time of t3427 on this developer's machine from ~28s to ~8.5s.

Signed-off-by Johannes Schindelin <[email protected]>

---
 t/t3427-rebase-subtree.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

t/t3427-rebase-subtree.sh +2 -2

index c0ff3241e4..7a37235768 100755
@@ -64,7 +64,7 @@ test_expect_success 'setup' '
 '
 
 # FAILURE: Does not preserve master4.
-test_expect_failure 'Rebase -Xsubtree --preserve-merges --onto commit' '
+test_expect_failure REBASE_P 'Rebase -Xsubtree --preserve-merges --onto commit' '
 	reset_rebase &&
 	git checkout -b rebase-preserve-merges to-rebase &&
 	git rebase -Xsubtree=files_subtree --preserve-merges --onto files-master master &&

@@ -73,7 +73,7 @@ test_expect_failure 'Rebase -Xsubtree --preserve-merges --onto commit' '
 '
 
 # FAILURE: Does not preserve master4.
-test_expect_failure 'Rebase -Xsubtree --keep-empty --preserve-merges --onto commit' '
+test_expect_failure REBASE_P 'Rebase -Xsubtree --keep-empty --preserve-merges --onto commit' '
 	reset_rebase &&
 	git checkout -b rebase-keep-empty to-rebase &&
 	git rebase -Xsubtree=files_subtree --keep-empty --preserve-merges --onto files-master master &&

Signature · 1 line

gitgitgadget

Johannes Schindelin via GitGitGadget

author

2019-07-25 10:11 UTC #15 lore raw

PATCH 11/12 t3418: test `rebase -r` with merge strategies

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

There is a test case in this script that verifies that git rebase --preserve-merges works all right with non-default merge strategies or non-default merge strategy options.

Now that git rebase --rebase-merges learned about merge strategies, let's copy-edit this test case to verify that that works as intended, too.

Signed-off-by Johannes Schindelin <[email protected]>

---
 t/t3418-rebase-continue.sh | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

t/t3418-rebase-continue.sh +14 -0

index bdaa511bb0..fbf9addfd1 100755
@@ -120,6 +120,20 @@ test_expect_success REBASE_P 'rebase passes merge strategy options correctly' '
 	git rebase --continue
 '
 
+test_expect_success 'rebase -r passes merge strategy options correctly' '
+	rm -fr .git/rebase-* &&
+	git reset --hard commit-new-file-F3-on-topic-branch &&
+	test_commit merge-theirs &&
+	git reset --hard HEAD^ &&
+	test_commit some-other-commit &&
+	test_tick &&
+	git merge --no-ff merge-theirs &&
+	FAKE_LINES="1 3 edit 4 5 7 8 9" git rebase -i -f -r -m \
+		-s recursive --strategy-option=theirs HEAD~2 &&
+	test_commit force-change-ours &&
+	git rebase --continue
+'
+
 test_expect_success '--skip after failed fixup cleans commit message' '
 	test_when_finished "test_might_fail git rebase --abort" &&
 	git checkout -b with-conflicting-fixup &&

Signature · 1 line

gitgitgadget

Johannes Schindelin via GitGitGadget

author

2019-07-25 10:11 UTC #6 lore raw

PATCH 6/12 t3427: accommodate for the `rebase --merge` backend having been replaced

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

Since 68aa495b590 (rebase: implement --merge via the interactive machinery, 2018-12-11), the job of the old --merge backend is now performed by the --interactive backend, too.

One consequence is that empty commits are no longer rebased by default.

Meaning that the test case that calls git rebase -Xsubtree (which used to be handled by the --merge backend) now needs to ask explicitly for the empty commit to be rebased.

Signed-off-by Johannes Schindelin <[email protected]>

---
 t/t3427-rebase-subtree.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

t/t3427-rebase-subtree.sh +2 -2

index 8c4ddd3408..b490919c60 100755
@@ -83,10 +83,10 @@ test_expect_failure 'Rebase -Xsubtree --keep-empty --preserve-merges --onto comm
 '
 
 # FAILURE: fatal: Could not parse object
-test_expect_failure 'Rebase -Xsubtree --onto commit' '
+test_expect_failure 'Rebase -Xsubtree --keep-empty --onto commit' '
 	reset_rebase &&
 	git checkout -b rebase-onto to-rebase &&
-	git rebase -Xsubtree=files_subtree --onto files-master master &&
+	git rebase -Xsubtree=files_subtree --keep-empty --onto files-master master &&
 	verbose test "$(commit_message HEAD~2)" = "master4" &&
 	verbose test "$(commit_message HEAD~)" = "files_subtree/master5" &&
 	verbose test "$(commit_message HEAD)" = "Empty commit"

Signature · 1 line

gitgitgadget

Junio C Hamano

2019-07-25 17:10 UTC #17 lore raw

In reply to Johannes Schindelin via GitGitGadget

"Johannes Schindelin via GitGitGadget" <[email protected]>

Quoted 6 lines · writes:

... As a consolation to myself, this work was actually worth it, surprising as that may look. Not only is t3427 now really easy to understand, adding that test case for --rebase-merges -Xsubtree tickled the sequencer enough to reveal a long-standing bug: the --onto option was simply ignored when passed together with --rebase-merges and --root. For good measure, this patch series addresses this bug, too.

Very nice.

Quoted 4 lines

base-commit: 082ef75b7bfc90ac236afbb857a9552a026832b8
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-294%2Fdscho%2Frebase-r-with-strategies-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-294/dscho/rebase-r-with-strategies-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/294

brian m. carlson

2019-07-26 07:43 UTC #11 lore raw

Re: [PATCH 10/12] t/lib-rebase: prepare for testing `git rebase --rebase-merges`

In reply to Johannes Schindelin via GitGitGadget

Quoted 16 lines · Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>
The format of the todo list is quite a bit different in the --rebase-merges mode; Let's prepare the fake editor to handle those todo lists properly, too.
Signed-off-by Johannes Schindelin <[email protected]>
---
 t/lib-rebase.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
t/lib-rebase.sh +0 -0
index 7ea30e5006..662a958575 100644

t/lib-rebase.sh set_fake_editor () { · line 47

Quoted 5 lines

t/lib-rebase.sh +1 -1

@@ -44,10 +44,10 @@ set_fake_editor () {
 	rm -f "$1"
 	echo 'rebase -i script before editing:'
 	cat "$1".tmp
-	action=pick
+	action=\&

So we set action to "&" so we can use it as the result in the sed expression below…

t/lib-rebase.sh set_fake_editor () { · line 61

Quoted 9 lines

t/lib-rebase.sh +1 -1

 	for line in $FAKE_LINES; do
 		case $line in
-		pick|p|squash|s|fixup|f|edit|e|reword|r|drop|d)
+		pick|p|squash|s|fixup|f|edit|e|reword|r|drop|d|label|l|reset|r|merge|m)
 			action="$line";;
 		exec_*|x_*|break|b)
 			echo "$line" | sed 's/_/ /g' >> "$1";;

@@ -61,8 +61,8 @@ set_fake_editor () {
 			echo "$action XXXXXXX False commit" >> "$1"

but then here it doesn't look like "&" is a thing we'd want to use. Is there something I'm missing about this particular case?

Signature · 2 lines

brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

brian m. carlson

2019-07-26 07:52 UTC #18 lore raw

In reply to Johannes Schindelin via GitGitGadget

Quoted 5 lines · Johannes Schindelin via GitGitGadget

This is the most notable shortcoming that --rebase-merges has, still, relative to --preserve-merges' capabilities: it does not support passing custom merge strategies or custom merge strategy options.
Let's fix this.

This looks like a great improvement. I'm glad to see --rebase-merges gaining feature parity with --preserve-merges.

Signature · 2 lines

brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

Johannes Schindelin

2019-07-26 14:01 UTC #12 lore raw

Re: [PATCH 10/12] t/lib-rebase: prepare for testing `git rebase --rebase-merges`

In reply to brian m. carlson

Hi brian,

Quoted 26 lines · brian m. carlson

Quoted 16 lines · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]>
The format of the todo list is quite a bit different in the --rebase-merges mode; Let's prepare the fake editor to handle those todo lists properly, too.
Signed-off-by Johannes Schindelin <[email protected]>
---
 t/lib-rebase.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
t/lib-rebase.sh +0 -0
index 7ea30e5006..662a958575 100644
t/lib-rebase.sh set_fake_editor () { · line 47
Quoted 5 lines
t/lib-rebase.sh +1 -1
@@ -44,10 +44,10 @@ set_fake_editor () {
 	rm -f "$1"
 	echo 'rebase -i script before editing:'
 	cat "$1".tmp
-	action=pick
+	action=\&
So we set action to "&" so we can use it as the result in the sed expression below…

Quoted 12 lines

t/lib-rebase.sh set_fake_editor () { · line 61
Quoted 9 lines
t/lib-rebase.sh +1 -1
 	for line in $FAKE_LINES; do
 		case $line in
-		pick|p|squash|s|fixup|f|edit|e|reword|r|drop|d)
+		pick|p|squash|s|fixup|f|edit|e|reword|r|drop|d|label|l|reset|r|merge|m)
 			action="$line";;
 		exec_*|x_*|break|b)
 			echo "$line" | sed 's/_/ /g' >> "$1";;
@@ -61,8 +61,8 @@ set_fake_editor () {
 			echo "$action XXXXXXX False commit" >> "$1"
but then here it doesn't look like "&" is a thing we'd want to use. Is there something I'm missing about this particular case?

Actually, the part that uses it is not shown in the patch (one of the many, many reasons why I try to discourage patch review and encourage code review instead). The relevant section currently looks somewhat like this:

-- snip --

set_fake_editor () {
	write_script fake-editor.sh <<-\EOF
	case "$1" in
	*/COMMIT_EDITMSG)
		test -z "$EXPECT_HEADER_COUNT" ||
			test "$EXPECT_HEADER_COUNT" = "$(sed -n '1s/^# This is a combination of \(.*\) commits\./\1/p' < "$1")" ||
			test "# # GETTEXT POISON #" = "$(sed -n '1p' < "$1")" ||
			exit
		test -z "$FAKE_COMMIT_MESSAGE" || echo "$FAKE_COMMIT_MESSAGE" > "$1"
		test -z "$FAKE_COMMIT_AMEND" || echo "$FAKE_COMMIT_AMEND" >> "$1"
		exit
		;;
	esac
	test -z "$EXPECT_COUNT" ||
		test "$EXPECT_COUNT" = $(sed -e '/^#/d' -e '/^$/d' < "$1" | wc -l) ||
		exit
	test -z "$FAKE_LINES" && exit
	grep -v '^#' < "$1" > "$1".tmp
	rm -f "$1"
	echo 'rebase -i script before editing:'
	cat "$1".tmp
	action=pick
	for line in $FAKE_LINES; do
		case $line in
		pick|p|squash|s|fixup|f|edit|e|reword|r|drop|d)
			action="$line";;
		exec_*|x_*|break|b)
			echo "$line" | sed 's/_/ /g' >> "$1";;
		"#")
			echo '# comment' >> "$1";;
		">")
			echo >> "$1";;
		bad)
			action="badcmd";;
		fakesha)
			echo "$action XXXXXXX False commit" >> "$1"
			action=pick;;
		*)
			sed -n "${line}s/^pick/$action/p" < "$1".tmp >> "$1"
			action=pick;;
		esac
	done
	echo 'rebase -i script after editing:'
	cat "$1"
	EOF

	test_set_editor "$(pwd)/fake-editor.sh"
}

-- snap --

Most importantly, action is used here:

                        sed -n "${line}s/^pick/$action/p" < "$1".tmp >> "$1"

and I changed it to

			sed -n "${line}s/^[a-z][a-z]*/$action/p" < "$1".tmp >> "$1"

In other words, rather than expecting the lines that are used by the fake editor to start with pick, after this patch, the tests expect the todo lists to start with a command consisting of lower-case ASCII letters (which catches pick, of course, but also label, reset and merge).

After this patch, the fake editor also does not try to replace whatever command it finds by pick, but it keeps it as-is instead.

Ciao,
Dscho

brian m. carlson

2019-07-26 21:08 UTC #13 lore raw

Re: [PATCH 10/12] t/lib-rebase: prepare for testing `git rebase --rebase-merges`

Quoted 4 lines · Johannes Schindelin

Actually, the part that uses it is not shown in the patch (one of the many, many reasons why I try to discourage patch review and encourage code review instead). The relevant section currently looks somewhat like this:

I feel like I may have communicated poorly earlier, so let me retry asking this in a different way.

Quoted 15 lines

-- snip --

set_fake_editor () {
	write_script fake-editor.sh <<-\EOF
	case "$1" in
	*/COMMIT_EDITMSG)
		test -z "$EXPECT_HEADER_COUNT" ||
			test "$EXPECT_HEADER_COUNT" = "$(sed -n '1s/^# This is a combination of \(.*\) commits\./\1/p' < "$1")" ||
			test "# # GETTEXT POISON #" = "$(sed -n '1p' < "$1")" ||
			exit
		test -z "$FAKE_COMMIT_MESSAGE" || echo "$FAKE_COMMIT_MESSAGE" > "$1"
		test -z "$FAKE_COMMIT_AMEND" || echo "$FAKE_COMMIT_AMEND" >> "$1"
		exit
		;;
	esac
	test -z "$EXPECT_COUNT" ||

Quoted 8 lines

		test "$EXPECT_COUNT" = $(sed -e '/^#/d' -e '/^$/d' < "$1" | wc -l) ||
		exit
	test -z "$FAKE_LINES" && exit
	grep -v '^#' < "$1" > "$1".tmp
	rm -f "$1"
	echo 'rebase -i script before editing:'
	cat "$1".tmp
	action=pick

I believe you changed this line to "action=\&".

Quoted 6 lines

	for line in $FAKE_LINES; do
		case $line in
		pick|p|squash|s|fixup|f|edit|e|reword|r|drop|d)
			action="$line";;
		exec_*|x_*|break|b)
			echo "$line" | sed 's/_/ /g' >> "$1";;

Quoted 8 lines

		"#")
			echo '# comment' >> "$1";;
		">")
			echo >> "$1";;
		bad)
			action="badcmd";;
		fakesha)
			echo "$action XXXXXXX False commit" >> "$1"

And my question was about this line.

Quoted 22 lines

			action=pick;;
		*)
			sed -n "${line}s/^pick/$action/p" < "$1".tmp >> "$1"
			action=pick;;
		esac
	done
	echo 'rebase -i script after editing:'
	cat "$1"
	EOF

	test_set_editor "$(pwd)/fake-editor.sh"
}

-- snap --

Most importantly, action is used here:

                        sed -n "${line}s/^pick/$action/p" < "$1".tmp >> "$1"

and I changed it to

			sed -n "${line}s/^[a-z][a-z]*/$action/p" < "$1".tmp >> "$1"

Quoted 8 lines

In other words, rather than expecting the lines that are used by the fake editor to start with pick, after this patch, the tests expect the todo lists to start with a command consisting of lower-case ASCII letters (which catches pick, of course, but also label, reset and merge).
After this patch, the fake editor also does not try to replace whatever command it finds by pick, but it keeps it as-is instead.

Right, that's how I read it, and that part I agree with. I think my question is this: in what case do we execute the "fakesha" case? Are we guaranteed that when we do, action isn't "&"? "&" seems fine for the right-hand side of a sed s-statement, but not as the beginning of a typical line in a sequencer file.

I ask because if we're testing a failure case, we want it to fail for the right reason (e.g., the commit doesn't exist), and not because we're producing invalid data. If the answer in this case is, "Well, we'll always have something else before it which will set $action properly," then that's fine. This is test code, so it need not be bulletproof, but I did want to ask.

If I'm still misunderstanding something, I apologize.

Signature · 2 lines

brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

Johannes Schindelin

2019-07-31 11:25 UTC #14 lore raw

Re: [PATCH 10/12] t/lib-rebase: prepare for testing `git rebase --rebase-merges`

Hi brian,

Quoted 8 lines · brian m. carlson

Quoted 4 lines · Johannes Schindelin
Actually, the part that uses it is not shown in the patch (one of the many, many reasons why I try to discourage patch review and encourage code review instead). The relevant section currently looks somewhat like this:
I feel like I may have communicated poorly earlier, so let me retry asking this in a different way.

Actually, your communication was just fine, the misunderstanding was entirely on my side. My apologies.

Quoted 41 lines

Quoted 15 lines

-- snip --

set_fake_editor () {
	write_script fake-editor.sh <<-\EOF
	case "$1" in
	*/COMMIT_EDITMSG)
		test -z "$EXPECT_HEADER_COUNT" ||
			test "$EXPECT_HEADER_COUNT" = "$(sed -n '1s/^# This is a combination of \(.*\) commits\./\1/p' < "$1")" ||
			test "# # GETTEXT POISON #" = "$(sed -n '1p' < "$1")" ||
			exit
		test -z "$FAKE_COMMIT_MESSAGE" || echo "$FAKE_COMMIT_MESSAGE" > "$1"
		test -z "$FAKE_COMMIT_AMEND" || echo "$FAKE_COMMIT_AMEND" >> "$1"
		exit
		;;
	esac
	test -z "$EXPECT_COUNT" ||

Quoted 8 lines

		test "$EXPECT_COUNT" = $(sed -e '/^#/d' -e '/^$/d' < "$1" | wc -l) ||
		exit
	test -z "$FAKE_LINES" && exit
	grep -v '^#' < "$1" > "$1".tmp
	rm -f "$1"
	echo 'rebase -i script before editing:'
	cat "$1".tmp
	action=pick

I believe you changed this line to "action=\&".

Quoted 6 lines

	for line in $FAKE_LINES; do
		case $line in
		pick|p|squash|s|fixup|f|edit|e|reword|r|drop|d)
			action="$line";;
		exec_*|x_*|break|b)
			echo "$line" | sed 's/_/ /g' >> "$1";;

Quoted 8 lines

		"#")
			echo '# comment' >> "$1";;
		">")
			echo >> "$1";;
		bad)
			action="badcmd";;
		fakesha)
			echo "$action XXXXXXX False commit" >> "$1"

Quoted 1 line

And my question was about this line.

Right. It would append & XXXXXXX False commit, which is not a valid todo command.

So something like

- echo "$action XXXXXXX False commit" >> "$1"
+ test \& = "$action" && c=pick || c=$action
+ echo "$c XXXXXXX False commit" >>"$1"

would be needed.

However, what makes me really worried now is that our test suite did not have a fit about this. The CI build passes the test suite on Windows, macOS and Linux: https://github.com/gitgitgadget/git/runs/176651523.

Quoted 29 lines

Quoted 29 lines
			action=pick;;
		*)
			sed -n "${line}s/^pick/$action/p" < "$1".tmp >> "$1"
			action=pick;;
		esac
	done
	echo 'rebase -i script after editing:'
	cat "$1"
	EOF

	test_set_editor "$(pwd)/fake-editor.sh"
}
-- snap --
Most importantly, action is used here:
                        sed -n "${line}s/^pick/$action/p" < "$1".tmp >> "$1"
and I changed it to
			sed -n "${line}s/^[a-z][a-z]*/$action/p" < "$1".tmp >> "$1"
In other words, rather than expecting the lines that are used by the fake editor to start with pick, after this patch, the tests expect the todo lists to start with a command consisting of lower-case ASCII letters (which catches pick, of course, but also label, reset and merge).

Quoted 8 lines

Quoted 2 lines
After this patch, the fake editor also does not try to replace whatever command it finds by pick, but it keeps it as-is instead.
Right, that's how I read it, and that part I agree with. I think my question is this: in what case do we execute the "fakesha" case? Are we guaranteed that when we do, action isn't "&"? "&" seems fine for the right-hand side of a sed s-statement, but not as the beginning of a typical line in a sequencer file.

Indeed, the sequencer should throw a real tantrum about this and not even bother to start.

But then, the same would hold true for an obviously invalid commit hash.

Quoted 3 lines

I ask because if we're testing a failure case, we want it to fail for the right reason (e.g., the commit doesn't exist), and not because we're producing invalid data.

Indeed. I have even come to the conclusion that our test_expect_failure command encourages exactly this type of problem:
in the beginning, those test cases might actually be correct, but over time they are prone to fail for all the wrong reasons, because we do not actually test for a specific failure more, we just say that we expect this test case to fail (and that this indicates a bug).

Quoted 3 lines

If the answer in this case is, "Well, we'll always have something else before it which will set $action properly," then that's fine. This is test code, so it need not be bulletproof, but I did want to ask.

I think you are perfectly sane to question this, and to expect me to double check.

So, double check I did. Turns out there is a single user of the fakesha thing, and it is hidden deep in t3404, prefixed by test_must_fail:

-- snip --
test_expect_success 'static check of bad SHA-1' '

	rebase_setup_and_clean bad-sha &&
	set_fake_editor &&
	test_must_fail env FAKE_LINES="1 2 edit fakesha 3 4 5 #" \
		git rebase -i --root 2>actual &&
	test_i18ngrep "edit XXXXXXX False commit" actual &&
	test_i18ngrep "You can fix this with .git rebase --edit-todo.." actual &&
	FAKE_LINES="1 2 4 5 6" git rebase --edit-todo &&
	git rebase --continue &&
	test E = $(git cat-file commit HEAD | sed -ne \$p)

'
-- snap --

As you can see, contrary to my expectations it does verify the output. It *also* changes the action to edit, which is the reason why there is no & ;-)

But I think you are correct, I should make sure that the fake editor is still correct with respect to the pick command.

Quoted 1 line

If I'm still misunderstanding something, I apologize.

I am really impressed and inspired by your gentle language. Thank you for this.

Ciao,
Dscho

Johannes Schindelin via GitGitGadget

author

2019-07-31 15:18 UTC #21 lore raw

PATCH v2 2/16 t3400: stop referring to the scripted rebase

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

One test case's title mentioned the then-current implementation detail that the --am backend was implemented in git-rebase--am.sh.

This is no longer the case, so let's update the title to reflect the current reality.

Signed-off-by Johannes Schindelin <[email protected]>

Signed-off-by Junio C Hamano <[email protected]>

---
 t/t3400-rebase.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

t/t3400-rebase.sh +1 -1

index 42f147858d..80b23fd326 100755
@@ -285,7 +285,7 @@ EOF
 	test_cmp From_.msg out
 '
 
-test_expect_success 'rebase--am.sh and --show-current-patch' '
+test_expect_success 'rebase --am and --show-current-patch' '
 	test_create_repo conflict-apply &&
 	(
 		cd conflict-apply &&

Signature · 1 line

gitgitgadget

Johannes Schindelin via GitGitGadget

author

2019-07-31 15:18 UTC #22 lore raw

PATCH v2 3/16 .gitignore: there is no longer a built-in `git-rebase--interactive`

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

This went away in 0609b741a4 (rebase -i: combine rebase--interactive.c with rebase.c, 2019-04-17).

Signed-off-by Johannes Schindelin <[email protected]>

Signed-off-by Junio C Hamano <[email protected]>

---
 .gitignore | 1 -
 1 file changed, 1 deletion(-)

.gitignore +0 -1

index 875f3fc6e8..bcee4fda81 100644
@@ -123,7 +123,6 @@
 /git-read-tree
 /git-rebase
 /git-rebase--common
-/git-rebase--interactive
 /git-rebase--preserve-merges
 /git-receive-pack
 /git-reflog

Signature · 1 line

gitgitgadget

Johannes Schindelin via GitGitGadget

author

2019-07-31 15:18 UTC #25 lore raw

PATCH v2 6/16 t3427: add a clarifying comment

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

The flow of this test script is outright confusing, and to start the endeavor to address that, let's describe what this test is all about, and how it tries to do it.

Signed-off-by Johannes Schindelin <[email protected]>

---
 t/t3427-rebase-subtree.sh | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

t/t3427-rebase-subtree.sh +28 -0

index d8640522a0..3a2ae7b55d 100755
@@ -11,6 +11,34 @@ commit_message() {
 	git log --pretty=format:%s -1 "$1"
 }
 
+# There are a few bugs in the rebase with regards to the subtree strategy, and
+# this test script tries to document them.  First, the following commit history
+# is generated (the onelines are shown, time flows from left to right):
+#
+# master1 - master2 - master3
+#                             \
+# README ---------------------- Add subproject master - master4 - files_subtree/master5
+#
+# Where the merge moves the files master[123].t into the subdirectory
+# files_subtree/ and master4 as well as files_subtree/master5 add files to that
+# directory directly.
+#
+# Then, in subsequent test cases, `git filter-branch` is used to distill just
+# the commits that touch files_subtree/. To give it a final pre-rebase touch,
+# an empty commit is added on top. The pre-rebase commit history looks like
+# this:
+#
+# Add subproject master - master4 - files_subtree/master5 - Empty commit
+#
+# where the root commit adds three files: master1.t, master2.t and master3.t.
+#
+# This commit history is then rebased onto `master3` with the
+# `-Xsubtree=files_subtree` option in three different ways:
+#
+# 1. using `--preserve-merges`
+# 2. using `--preserve-merges` and --keep-empty
+# 3. without specifying a rebase backend
+
 test_expect_success 'setup' '
 	test_commit README &&
 	mkdir files &&

Signature · 1 line

gitgitgadget

Johannes Schindelin via GitGitGadget

author

2019-07-31 15:18 UTC #19 lore raw

v2 cover rebase -r: support merge strategies other than recursive

In reply to Johannes Schindelin via GitGitGadget

Let's fix this.

Changes since v1:

* Rebased to the current js/rebase-cleanup: that branch itself was rebased, and as a consequence, one already-applied patch was dropped from this patch series.
* Forward-fixed the fakesha handling, just in case that anybody wants to use it with a regular pick command in the future (thanks, brian).

Johannes Schindelin (16):
Drop unused git-rebase--am.sh
t3400: stop referring to the scripted rebase
.gitignore: there is no longer a built-in git-rebase--interactive
sequencer: the am and rebase--interactive scripts are gone
rebase: fold git-rebase--common into the -p backend
t3427: add a clarifying comment
t3427: simplify the setup test case significantly
t3427: move the filter-branch invocation into the setup case
t3427: condense the unnecessarily repetitive test cases into three
t3427: fix erroneous assumption
t3427: accommodate for the rebase --merge backend having been
replaced
t3427: fix another incorrect assumption
rebase -r: support merge strategies other than recursive t/lib-rebase: prepare for testing git rebase --rebase-merges
t3418: test rebase -r with merge strategies
rebase -r: do not (re-)generate root commits with --root *and*

    `--onto`

 .gitignore                             |   3 -
 Documentation/git-rebase.txt           |   2 -
 Makefile                               |   2 -
 builtin/rebase.c                       |  23 +---
 git-rebase--am.sh                      |  85 --------------
 git-rebase--common.sh                  |  69 -----------
 git-rebase--preserve-merges.sh         |  55 +++++++++
 sequencer.c                            |  20 +++-
 sequencer.h                            |   6 +
 t/lib-rebase.sh                        |   9 +-
 t/t3400-rebase.sh                      |   2 +-
 t/t3418-rebase-continue.sh             |  14 +++
 t/t3422-rebase-incompatible-options.sh |  10 --
 t/t3427-rebase-subtree.sh              | 155 +++++++++++--------------
 t/t3430-rebase-merges.sh               |  21 ++++
 15 files changed, 193 insertions(+), 283 deletions(-)
 delete mode 100644 git-rebase--am.sh
 delete mode 100644 git-rebase--common.sh

base-commit: 80dfc9242ebaba357ffedececd88641a1a752411
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-294%2Fdscho%2Frebase-r-with-strategies-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-294/dscho/rebase-r-with-strategies-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/294

Range-diff vs v1:

  -:  ---------- >  1:  e0645b3ad7 Drop unused git-rebase--am.sh
  -:  ---------- >  2:  6e8be7d873 t3400: stop referring to the scripted rebase
  -:  ---------- >  3:  9e00e66dc7 .gitignore: there is no longer a built-in `git-rebase--interactive`
  -:  ---------- >  4:  db9ec248e1 sequencer: the `am` and `rebase--interactive` scripts are gone
  -:  ---------- >  5:  38c8e3e284 rebase: fold git-rebase--common into the -p backend
  1:  05be92d921 =  6:  b3daf078e8 t3427: add a clarifying comment
  2:  df096e054d =  7:  c168c4499b t3427: simplify the `setup` test case significantly
  3:  a3944c5480 !  8:  138ff362fb t3427: move the `filter-branch` invocation into the `setup` case
     @@ -29,7 +29,8 @@
       '
       
       # FAILURE: Does not preserve master4.
     - test_expect_failure 'Rebase -Xsubtree --preserve-merges --onto commit 4' '
     + test_expect_failure REBASE_P \
     + 	'Rebase -Xsubtree --preserve-merges --onto commit 4' '
       	reset_rebase &&
      -	git checkout -b rebase-preserve-merges-4 master &&
      -	git filter-branch --prune-empty -f --subdirectory-filter files_subtree &&
     @@ -39,8 +40,8 @@
       	verbose test "$(commit_message HEAD~)" = "files_subtree/master4"
       '
      @@
     - # FAILURE: Does not preserve master5.
     - test_expect_failure 'Rebase -Xsubtree --preserve-merges --onto commit 5' '
     + test_expect_failure REBASE_P \
     + 	'Rebase -Xsubtree --preserve-merges --onto commit 5' '
       	reset_rebase &&
      -	git checkout -b rebase-preserve-merges-5 master &&
      -	git filter-branch --prune-empty -f --subdirectory-filter files_subtree &&
     @@ -50,8 +51,8 @@
       	verbose test "$(commit_message HEAD)" = "files_subtree/master5"
       '
      @@
     - # FAILURE: Does not preserve master4.
     - test_expect_failure 'Rebase -Xsubtree --keep-empty --preserve-merges --onto commit 4' '
     + test_expect_failure REBASE_P \
     + 	'Rebase -Xsubtree --keep-empty --preserve-merges --onto commit 4' '
       	reset_rebase &&
      -	git checkout -b rebase-keep-empty-4 master &&
      -	git filter-branch --prune-empty -f --subdirectory-filter files_subtree &&
     @@ -61,8 +62,8 @@
       	verbose test "$(commit_message HEAD~2)" = "files_subtree/master4"
       '
      @@
     - # FAILURE: Does not preserve master5.
     - test_expect_failure 'Rebase -Xsubtree --keep-empty --preserve-merges --onto commit 5' '
     + test_expect_failure REBASE_P \
     + 	'Rebase -Xsubtree --keep-empty --preserve-merges --onto commit 5' '
       	reset_rebase &&
      -	git checkout -b rebase-keep-empty-5 master &&
      -	git filter-branch --prune-empty -f --subdirectory-filter files_subtree &&
     @@ -72,8 +73,8 @@
       	verbose test "$(commit_message HEAD~)" = "files_subtree/master5"
       '
      @@
     - # FAILURE: Does not preserve Empty.
     - test_expect_failure 'Rebase -Xsubtree --keep-empty --preserve-merges --onto empty commit' '
     + test_expect_failure REBASE_P \
     + 	'Rebase -Xsubtree --keep-empty --preserve-merges --onto empty commit' '
       	reset_rebase &&
      -	git checkout -b rebase-keep-empty-empty master &&
      -	git filter-branch --prune-empty -f --subdirectory-filter files_subtree &&
  4:  9aeb57fa8f !  9:  46ef005365 t3427: condense the unnecessarily repetitive test cases into three
     @@ -25,8 +25,9 @@
       '
       
       # FAILURE: Does not preserve master4.
     --test_expect_failure 'Rebase -Xsubtree --preserve-merges --onto commit 4' '
     -+test_expect_failure 'Rebase -Xsubtree --preserve-merges --onto commit' '
     +-test_expect_failure REBASE_P \
     +-	'Rebase -Xsubtree --preserve-merges --onto commit 4' '
     ++test_expect_failure REBASE_P 'Rebase -Xsubtree --preserve-merges --onto commit' '
       	reset_rebase &&
      -	git checkout -b rebase-preserve-merges-4 to-rebase &&
      -	git rebase -Xsubtree=files_subtree --preserve-merges --onto files-master master &&
     @@ -34,7 +35,8 @@
      -'
      -
      -# FAILURE: Does not preserve master5.
     --test_expect_failure 'Rebase -Xsubtree --preserve-merges --onto commit 5' '
     +-test_expect_failure REBASE_P \
     +-	'Rebase -Xsubtree --preserve-merges --onto commit 5' '
      -	reset_rebase &&
      -	git checkout -b rebase-preserve-merges-5 to-rebase &&
      +	git checkout -b rebase-preserve-merges to-rebase &&
     @@ -44,8 +46,9 @@
       '
       
       # FAILURE: Does not preserve master4.
     --test_expect_failure 'Rebase -Xsubtree --keep-empty --preserve-merges --onto commit 4' '
     -+test_expect_failure 'Rebase -Xsubtree --keep-empty --preserve-merges --onto commit' '
     +-test_expect_failure REBASE_P \
     +-	'Rebase -Xsubtree --keep-empty --preserve-merges --onto commit 4' '
     ++test_expect_failure REBASE_P 'Rebase -Xsubtree --keep-empty --preserve-merges --onto commit' '
       	reset_rebase &&
      -	git checkout -b rebase-keep-empty-4 to-rebase &&
      -	git rebase -Xsubtree=files_subtree --keep-empty --preserve-merges --onto files-master master &&
     @@ -53,7 +56,8 @@
      -'
      -
      -# FAILURE: Does not preserve master5.
     --test_expect_failure 'Rebase -Xsubtree --keep-empty --preserve-merges --onto commit 5' '
     +-test_expect_failure REBASE_P \
     +-	'Rebase -Xsubtree --keep-empty --preserve-merges --onto commit 5' '
      -	reset_rebase &&
      -	git checkout -b rebase-keep-empty-5 to-rebase &&
      -	git rebase -Xsubtree=files_subtree --keep-empty --preserve-merges --onto files-master master &&
     @@ -61,7 +65,8 @@
      -'
      -
      -# FAILURE: Does not preserve Empty.
     --test_expect_failure 'Rebase -Xsubtree --keep-empty --preserve-merges --onto empty commit' '
     +-test_expect_failure REBASE_P \
     +-	'Rebase -Xsubtree --keep-empty --preserve-merges --onto empty commit' '
      -	reset_rebase &&
      -	git checkout -b rebase-keep-empty-empty to-rebase &&
      +	git checkout -b rebase-keep-empty to-rebase &&
  5:  3196413c2a = 10:  8afec74cfc t3427: fix erroneous assumption
  6:  261825fe44 = 11:  d73b0a05d9 t3427: accommodate for the `rebase --merge` backend having been replaced
  7:  f7452d3740 = 12:  57c63309bf t3427: fix another incorrect assumption
  8:  7f60b8e745 <  -:  ---------- t3427: mark two test cases as requiring support for `git rebase -p`
  9:  30405df99f = 13:  75b1395dae rebase -r: support merge strategies other than `recursive`
 10:  ae9e72b73b ! 14:  8f74bfbc53 t/lib-rebase: prepare for testing `git rebase --rebase-merges`

@@ -6,6 +6,26 @@
--rebase-merges mode; Let's prepare the fake editor to handle those todo lists properly, too.

+ The original idea was that we keep the original command unless
+ overridden, and because the original todo lists only had pick lines
+ anyway, we could be sloppy and "override" the command by the same
+ command (i.e. use the sed replacement pattern "pick" instead of "&").
+
+ This actually would not have worked with fixup and squash commands,
+ but it would appear that we never tried to use the fake editor with
+ --autosquash.
+
+ However, in the next commit we want to use the fake editor in
+ conjunction with --rebase-merges, so let's use the correct sed
+ replacement pattern.
+
+ Technically, it is not necessary to take care of the fakesha thing
+ (where we reuse the sed replacement pattern to craft a new todo
+ command), at least for now, as the only user of that thing overrides the
+ action anyway. Nevertheless, for completeness' sake, we do take care
+ of it.
+
+ Helped-by: brian m. carlson <[email protected]>

Signed-off-by Johannes Schindelin <[email protected]>

diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
@@ -25,6 +45,10 @@
exec_*|x_*|break|b)
echo "$line" | sed 's/_/ /g' >> "$1";;
@@
+ bad)
+ action="badcmd";;
+ fakesha)
++ test \& != "$action" || action=pick
echo "$action XXXXXXX False commit" >> "$1" action=pick;;
*)

 11:  a326be77aa = 15:  0c938d02b3 t3418: test `rebase -r` with merge strategies
 12:  e6713568dc = 16:  be62d267cb rebase -r: do not (re-)generate root commits with `--root` *and* `--onto`

Signature · 1 line

gitgitgadget

Johannes Schindelin via GitGitGadget

author

2019-07-31 15:18 UTC #23 lore raw

PATCH v2 4/16 sequencer: the `am` and `rebase--interactive` scripts are gone

From: Johannes Schindelin <[email protected]>

Update a code comment that referred to those files as if they were still there.

Signed-off-by Johannes Schindelin <[email protected]>

Signed-off-by Junio C Hamano <[email protected]>

---
 sequencer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

sequencer.c +1 -1

index f88a97fb10..334de14542 100644
@@ -767,7 +767,7 @@ static int parse_key_value_squoted(char *buf, struct string_list *list)
  *	GIT_AUTHOR_DATE='$author_date'
  *
  * where $author_name, $author_email and $author_date are quoted. We are strict
- * with our parsing, as the file was meant to be eval'd in the old
+ * with our parsing, as the file was meant to be eval'd in the now-removed
  * git-am.sh/git-rebase--interactive.sh scripts, and thus if the file differs
  * from what this function expects, it is better to bail out than to do
  * something that the user does not expect.

Signature · 1 line

gitgitgadget

Johannes Schindelin via GitGitGadget

author

2019-07-31 15:18 UTC #24 lore raw

PATCH v2 5/16 rebase: fold git-rebase--common into the -p backend

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

The only remaining scripted part of git rebase is the --preserve-merges backend. Meaning: there is little reason to keep the "library of common rebase functions" as a separate file.

While moving the functions to git-rebase--preserve-merges.sh, we also drop the move_to_original_branch function that is no longer used.

Signed-off-by Johannes Schindelin <[email protected]>

Signed-off-by Junio C Hamano <[email protected]>

---
 .gitignore                     |  1 -
 Makefile                       |  1 -
 builtin/rebase.c               |  3 +-
 git-rebase--common.sh          | 69 ----------------------------------
 git-rebase--preserve-merges.sh | 55 +++++++++++++++++++++++++++
 5 files changed, 56 insertions(+), 73 deletions(-)
 delete mode 100644 git-rebase--common.sh

.gitignore +0 -1

index bcee4fda81..4470d7cfc0 100644
@@ -122,7 +122,6 @@
 /git-range-diff
 /git-read-tree
 /git-rebase
-/git-rebase--common
 /git-rebase--preserve-merges
 /git-receive-pack
 /git-reflog

Makefile +0 -1

index 63e1973333..6c3bfb1733 100644
@@ -624,7 +624,6 @@ SCRIPT_SH += git-web--browse.sh
 
 SCRIPT_LIB += git-mergetool--lib
 SCRIPT_LIB += git-parse-remote
-SCRIPT_LIB += git-rebase--common
 SCRIPT_LIB += git-rebase--preserve-merges
 SCRIPT_LIB += git-sh-setup
 SCRIPT_LIB += git-sh-i18n

builtin/rebase.c +1 -2

index 4dd76ee576..74a60e8c83 100644
@@ -1163,8 +1163,7 @@ static int run_specific_rebase(struct rebase_options *opts, enum action action)
 	}
 
 	strbuf_addf(&script_snippet,
-		    ". git-sh-setup && . git-rebase--common &&"
-		    " . %s && %s", backend, backend_func);
+		    ". git-sh-setup && . %s && %s", backend, backend_func);
 	argv[0] = script_snippet.buf;
 
 	status = run_command_v_opt(argv, RUN_USING_SHELL);

git-rebase--common.sh deleted +0 ~~-69~~

69 lines not on this page · raw message

git-rebase--preserve-merges.sh +55 -0

61 lines not on this page · raw message

Signature · 1 line

gitgitgadget

Johannes Schindelin via GitGitGadget

author

2019-07-31 15:18 UTC #20 lore raw

PATCH v2 1/16 Drop unused git-rebase--am.sh

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

Since 21853626ea (built-in rebase: call git am directly, 2019-01-18), the built-in rebase already uses the built-in git am directly.

Now that d03ebd411c (rebase: remove the rebase.useBuiltin setting, 2019-03-18) even removed the scripted rebase, there is no longer any user of git-rebase--am.sh, so let's just remove it.

Signed-off-by Johannes Schindelin <[email protected]>

Signed-off-by Junio C Hamano <[email protected]>

---
 .gitignore        |  1 -
 Makefile          |  1 -
 builtin/rebase.c  |  4 ---
 git-rebase--am.sh | 85 -----------------------------------------------
 4 files changed, 91 deletions(-)
 delete mode 100644 git-rebase--am.sh

.gitignore +0 -1

index 2374f77a1a..875f3fc6e8 100644
@@ -122,7 +122,6 @@
 /git-range-diff
 /git-read-tree
 /git-rebase
-/git-rebase--am
 /git-rebase--common
 /git-rebase--interactive
 /git-rebase--preserve-merges

Makefile +0 -1

index 8a7e235352..63e1973333 100644
@@ -624,7 +624,6 @@ SCRIPT_SH += git-web--browse.sh
 
 SCRIPT_LIB += git-mergetool--lib
 SCRIPT_LIB += git-parse-remote
-SCRIPT_LIB += git-rebase--am
 SCRIPT_LIB += git-rebase--common
 SCRIPT_LIB += git-rebase--preserve-merges
 SCRIPT_LIB += git-sh-setup

builtin/rebase.c +0 -4

index 21681a551b..4dd76ee576 100644
@@ -1153,10 +1153,6 @@ static int run_specific_rebase(struct rebase_options *opts, enum action action)
 	}
 
 	switch (opts->type) {
-	case REBASE_AM:
-		backend = "git-rebase--am";
-		backend_func = "git_rebase__am";
-		break;
 	case REBASE_PRESERVE_MERGES:
 		backend = "git-rebase--preserve-merges";
 		backend_func = "git_rebase__preserve_merges";

git-rebase--am.sh deleted +0 ~~-85~~

85 lines not on this page · raw message

Signature · 1 line

gitgitgadget

Johannes Schindelin via GitGitGadget

author

2019-07-31 15:18 UTC #27 lore raw

PATCH v2 8/16 t3427: move the `filter-branch` invocation into the `setup` case

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

The step to prepare a pre-rebase commit history is _identical_ in _all_ of the test cases (except of course the setup case). It should therefore clearly a part of the setup test case instead.

Signed-off-by Johannes Schindelin <[email protected]>

---
 t/t3427-rebase-subtree.sh | 38 +++++++++++++-------------------------
 1 file changed, 13 insertions(+), 25 deletions(-)

t/t3427-rebase-subtree.sh +13 ~~-25~~

index 36c4f92e06..64ba95f3e0 100755
@@ -56,16 +56,18 @@ test_expect_success 'setup' '
 
 	: add two extra commits to rebase &&
 	test_commit -C files_subtree master4 &&
-	test_commit files_subtree/master5
+	test_commit files_subtree/master5 &&
+
+	git checkout -b to-rebase &&
+	git filter-branch --prune-empty -f --subdirectory-filter files_subtree &&
+	git commit -m "Empty commit" --allow-empty
 '
 
 # FAILURE: Does not preserve master4.
 test_expect_failure REBASE_P \
 	'Rebase -Xsubtree --preserve-merges --onto commit 4' '
 	reset_rebase &&
-	git checkout -b rebase-preserve-merges-4 master &&
-	git filter-branch --prune-empty -f --subdirectory-filter files_subtree &&
-	git commit -m "Empty commit" --allow-empty &&
+	git checkout -b rebase-preserve-merges-4 to-rebase &&
 	git rebase -Xsubtree=files_subtree --preserve-merges --onto files-master master &&
 	verbose test "$(commit_message HEAD~)" = "files_subtree/master4"
 '

@@ -74,9 +76,7 @@ test_expect_failure REBASE_P \
 test_expect_failure REBASE_P \
 	'Rebase -Xsubtree --preserve-merges --onto commit 5' '
 	reset_rebase &&
-	git checkout -b rebase-preserve-merges-5 master &&
-	git filter-branch --prune-empty -f --subdirectory-filter files_subtree &&
-	git commit -m "Empty commit" --allow-empty &&
+	git checkout -b rebase-preserve-merges-5 to-rebase &&
 	git rebase -Xsubtree=files_subtree --preserve-merges --onto files-master master &&
 	verbose test "$(commit_message HEAD)" = "files_subtree/master5"
 '

@@ -85,9 +85,7 @@ test_expect_failure REBASE_P \
 test_expect_failure REBASE_P \
 	'Rebase -Xsubtree --keep-empty --preserve-merges --onto commit 4' '
 	reset_rebase &&
-	git checkout -b rebase-keep-empty-4 master &&
-	git filter-branch --prune-empty -f --subdirectory-filter files_subtree &&
-	git commit -m "Empty commit" --allow-empty &&
+	git checkout -b rebase-keep-empty-4 to-rebase &&
 	git rebase -Xsubtree=files_subtree --keep-empty --preserve-merges --onto files-master master &&
 	verbose test "$(commit_message HEAD~2)" = "files_subtree/master4"
 '

@@ -96,9 +94,7 @@ test_expect_failure REBASE_P \
 test_expect_failure REBASE_P \
 	'Rebase -Xsubtree --keep-empty --preserve-merges --onto commit 5' '
 	reset_rebase &&
-	git checkout -b rebase-keep-empty-5 master &&
-	git filter-branch --prune-empty -f --subdirectory-filter files_subtree &&
-	git commit -m "Empty commit" --allow-empty &&
+	git checkout -b rebase-keep-empty-5 to-rebase &&
 	git rebase -Xsubtree=files_subtree --keep-empty --preserve-merges --onto files-master master &&
 	verbose test "$(commit_message HEAD~)" = "files_subtree/master5"
 '

@@ -107,9 +103,7 @@ test_expect_failure REBASE_P \
 test_expect_failure REBASE_P \
 	'Rebase -Xsubtree --keep-empty --preserve-merges --onto empty commit' '
 	reset_rebase &&
-	git checkout -b rebase-keep-empty-empty master &&
-	git filter-branch --prune-empty -f --subdirectory-filter files_subtree &&
-	git commit -m "Empty commit" --allow-empty &&
+	git checkout -b rebase-keep-empty-empty to-rebase &&
 	git rebase -Xsubtree=files_subtree --keep-empty --preserve-merges --onto files-master master &&
 	verbose test "$(commit_message HEAD)" = "Empty commit"
 '

@@ -117,9 +111,7 @@ test_expect_failure REBASE_P \
 # FAILURE: fatal: Could not parse object
 test_expect_failure 'Rebase -Xsubtree --onto commit 4' '
 	reset_rebase &&
-	git checkout -b rebase-onto-4 master &&
-	git filter-branch --prune-empty -f --subdirectory-filter files_subtree &&
-	git commit -m "Empty commit" --allow-empty &&
+	git checkout -b rebase-onto-4 to-rebase &&
 	git rebase -Xsubtree=files_subtree --onto files-master master &&
 	verbose test "$(commit_message HEAD~2)" = "files_subtree/master4"
 '

@@ -127,18 +119,14 @@ test_expect_failure 'Rebase -Xsubtree --onto commit 4' '
 # FAILURE: fatal: Could not parse object
 test_expect_failure 'Rebase -Xsubtree --onto commit 5' '
 	reset_rebase &&
-	git checkout -b rebase-onto-5 master &&
-	git filter-branch --prune-empty -f --subdirectory-filter files_subtree &&
-	git commit -m "Empty commit" --allow-empty &&
+	git checkout -b rebase-onto-5 to-rebase &&
 	git rebase -Xsubtree=files_subtree --onto files-master master &&
 	verbose test "$(commit_message HEAD~)" = "files_subtree/master5"
 '
 # FAILURE: fatal: Could not parse object
 test_expect_failure 'Rebase -Xsubtree --onto empty commit' '
 	reset_rebase &&
-	git checkout -b rebase-onto-empty master &&
-	git filter-branch --prune-empty -f --subdirectory-filter files_subtree &&
-	git commit -m "Empty commit" --allow-empty &&
+	git checkout -b rebase-onto-empty to-rebase &&
 	git rebase -Xsubtree=files_subtree --onto files-master master &&
 	verbose test "$(commit_message HEAD)" = "Empty commit"
 '

Signature · 1 line

gitgitgadget

Johannes Schindelin via GitGitGadget

author

2019-07-31 15:18 UTC #35 lore raw

PATCH v2 16/16 rebase -r: do not (re-)generate root commits with `--root` *and* `--onto`

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

When rebasing a complete commit history onto a given commit, it is pretty obvious that the root commits should be rebased on top of said given commit.

Signed-off-by Johannes Schindelin <[email protected]>

---
 builtin/rebase.c          |  7 +++++--
 sequencer.c               |  4 +++-
 sequencer.h               |  6 ++++++
 t/t3427-rebase-subtree.sh | 11 +++++++++++
 4 files changed, 25 insertions(+), 3 deletions(-)

builtin/rebase.c +5 -2

index 625f50c637..ee2bc8b032 100644
@@ -62,7 +62,7 @@ struct rebase_options {
 	const char *onto_name;
 	const char *revisions;
 	const char *switch_to;
-	int root;
+	int root, root_with_onto;
 	struct object_id *squash_onto;
 	struct commit *restrict_revision;
 	int dont_finish_rebase;

@@ -374,6 +374,7 @@ static int run_rebase_interactive(struct rebase_options *opts,
 	flags |= abbreviate_commands ? TODO_LIST_ABBREVIATE_CMDS : 0;
 	flags |= opts->rebase_merges ? TODO_LIST_REBASE_MERGES : 0;
 	flags |= opts->rebase_cousins > 0 ? TODO_LIST_REBASE_COUSINS : 0;
+	flags |= opts->root_with_onto ? TODO_LIST_ROOT_WITH_ONTO : 0;
 	flags |= command == ACTION_SHORTEN_OIDS ? TODO_LIST_SHORTEN_IDS : 0;
 
 	switch (command) {

@@ -1841,7 +1842,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			options.squash_onto = &squash_onto;
 			options.onto_name = squash_onto_name =
 				xstrdup(oid_to_hex(&squash_onto));
-		}
+		} else
+			options.root_with_onto = 1;
+
 		options.upstream_name = NULL;
 		options.upstream = NULL;
 		if (argc > 1)

sequencer.c +3 -1

index d228448cd8..ca119c84e5 100644
@@ -4440,6 +4440,7 @@ static int make_script_with_merges(struct pretty_print_context *pp,
 {
 	int keep_empty = flags & TODO_LIST_KEEP_EMPTY;
 	int rebase_cousins = flags & TODO_LIST_REBASE_COUSINS;
+	int root_with_onto = flags & TODO_LIST_ROOT_WITH_ONTO;
 	struct strbuf buf = STRBUF_INIT, oneline = STRBUF_INIT;
 	struct strbuf label = STRBUF_INIT;
 	struct commit_list *commits = NULL, **tail = &commits, *iter;

@@ -4606,7 +4607,8 @@ static int make_script_with_merges(struct pretty_print_context *pp,
 
 		if (!commit)
 			strbuf_addf(out, "%s %s\n", cmd_reset,
-				    rebase_cousins ? "onto" : "[new root]");
+				    rebase_cousins || root_with_onto ?
+				    "onto" : "[new root]");
 		else {
 			const char *to = NULL;

sequencer.h +6 -0

index 0c494b83d4..d506081d3c 100644
@@ -142,6 +142,12 @@ int sequencer_remove_state(struct replay_opts *opts);
  */
 #define TODO_LIST_REBASE_COUSINS (1U << 4)
 #define TODO_LIST_APPEND_TODO_HELP (1U << 5)
+/*
+ * When generating a script that rebases merges with `--root` *and* with
+ * `--onto`, we do not want to re-generate the root commits.
+ */
+#define TODO_LIST_ROOT_WITH_ONTO (1U << 6)
+
 
 int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
 			  const char **argv, unsigned flags);

t/t3427-rebase-subtree.sh +11 -0

index 7a37235768..39e348de16 100755
@@ -93,4 +93,15 @@ test_expect_success 'Rebase -Xsubtree --keep-empty --onto commit' '
 	verbose test "$(commit_message HEAD)" = "Empty commit"
 '
 
+test_expect_success 'Rebase -Xsubtree --keep-empty --rebase-merges --onto commit' '
+	reset_rebase &&
+	git checkout -b rebase-merges-onto to-rebase &&
+	test_must_fail git rebase -Xsubtree=files_subtree --keep-empty --rebase-merges --onto files-master --root &&
+	: first pick results in no changes &&
+	git rebase --continue &&
+	verbose test "$(commit_message HEAD~2)" = "master4" &&
+	verbose test "$(commit_message HEAD~)" = "files_subtree/master5" &&
+	verbose test "$(commit_message HEAD)" = "Empty commit"
+'
+
 test_done

Signature · 1 line

gitgitgadget

Johannes Schindelin via GitGitGadget

author

2019-07-31 15:18 UTC #33 lore raw

PATCH v2 14/16 t/lib-rebase: prepare for testing `git rebase --rebase-merges`

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

The format of the todo list is quite a bit different in the --rebase-merges mode; Let's prepare the fake editor to handle those todo lists properly, too.

The original idea was that we keep the original command unless overridden, and because the original todo lists only had pick lines anyway, we could be sloppy and "override" the command by the same command (i.e. use the sed replacement pattern "pick" instead of "&").

This actually would not have worked with fixup and squash commands, but it would appear that we never tried to use the fake editor with --autosquash.

However, in the next commit we want to use the fake editor in conjunction with --rebase-merges, so let's use the correct sed replacement pattern.

Technically, it is not necessary to take care of the fakesha thing (where we reuse the sed replacement pattern to craft a new todo command), at least for now, as the only user of that thing overrides the action anyway. Nevertheless, for completeness' sake, we do take care of it.

Helped-by brian m. carlson <[email protected]>

Signed-off-by Johannes Schindelin <[email protected]>

---
 t/lib-rebase.sh | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

t/lib-rebase.sh +5 -4

index 7ea30e5006..6d87961e41 100644
@@ -44,10 +44,10 @@ set_fake_editor () {
 	rm -f "$1"
 	echo 'rebase -i script before editing:'
 	cat "$1".tmp
-	action=pick
+	action=\&
 	for line in $FAKE_LINES; do
 		case $line in
-		pick|p|squash|s|fixup|f|edit|e|reword|r|drop|d)
+		pick|p|squash|s|fixup|f|edit|e|reword|r|drop|d|label|l|reset|r|merge|m)
 			action="$line";;
 		exec_*|x_*|break|b)
 			echo "$line" | sed 's/_/ /g' >> "$1";;

@@ -58,11 +58,12 @@ set_fake_editor () {
 		bad)
 			action="badcmd";;
 		fakesha)
+			test \& != "$action" || action=pick
 			echo "$action XXXXXXX False commit" >> "$1"
 			action=pick;;
 		*)
-			sed -n "${line}s/^pick/$action/p" < "$1".tmp >> "$1"
-			action=pick;;
+			sed -n "${line}s/^[a-z][a-z]*/$action/p" < "$1".tmp >> "$1"
+			action=\&;;
 		esac
 	done
 	echo 'rebase -i script after editing:'

Signature · 1 line

gitgitgadget

Johannes Schindelin via GitGitGadget

author

2019-07-31 15:18 UTC #34 lore raw

PATCH v2 15/16 t3418: test `rebase -r` with merge strategies

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

There is a test case in this script that verifies that git rebase --preserve-merges works all right with non-default merge strategies or non-default merge strategy options.

Now that git rebase --rebase-merges learned about merge strategies, let's copy-edit this test case to verify that that works as intended, too.

Signed-off-by Johannes Schindelin <[email protected]>

---
 t/t3418-rebase-continue.sh | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

t/t3418-rebase-continue.sh +14 -0

index bdaa511bb0..fbf9addfd1 100755
@@ -120,6 +120,20 @@ test_expect_success REBASE_P 'rebase passes merge strategy options correctly' '
 	git rebase --continue
 '
 
+test_expect_success 'rebase -r passes merge strategy options correctly' '
+	rm -fr .git/rebase-* &&
+	git reset --hard commit-new-file-F3-on-topic-branch &&
+	test_commit merge-theirs &&
+	git reset --hard HEAD^ &&
+	test_commit some-other-commit &&
+	test_tick &&
+	git merge --no-ff merge-theirs &&
+	FAKE_LINES="1 3 edit 4 5 7 8 9" git rebase -i -f -r -m \
+		-s recursive --strategy-option=theirs HEAD~2 &&
+	test_commit force-change-ours &&
+	git rebase --continue
+'
+
 test_expect_success '--skip after failed fixup cleans commit message' '
 	test_when_finished "test_might_fail git rebase --abort" &&
 	git checkout -b with-conflicting-fixup &&

Signature · 1 line

gitgitgadget

Johannes Schindelin via GitGitGadget

author

2019-07-31 15:19 UTC #32 lore raw

PATCH v2 13/16 rebase -r: support merge strategies other than `recursive`

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

We already support merge strategies in the sequencer, but only for pick commands.

Signed-off-by Johannes Schindelin <[email protected]>

---
 Documentation/git-rebase.txt           |  2 --
 builtin/rebase.c                       |  9 ---------
 sequencer.c                            | 14 ++++++++++++--
 t/t3422-rebase-incompatible-options.sh | 10 ----------
 t/t3430-rebase-merges.sh               | 21 +++++++++++++++++++++
 5 files changed, 33 insertions(+), 23 deletions(-)

Documentation/git-rebase.txt +0 -2

index 5e4e927647..bc620c44e9 100644
@@ -543,8 +543,6 @@ In addition, the following pairs of options are incompatible:
  * --preserve-merges and --interactive
  * --preserve-merges and --signoff
  * --preserve-merges and --rebase-merges
- * --rebase-merges and --strategy
- * --rebase-merges and --strategy-option
 
 BEHAVIORAL DIFFERENCES
 -----------------------

builtin/rebase.c +0 -9

index 74a60e8c83..625f50c637 100644
@@ -1811,15 +1811,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			      "'--reschedule-failed-exec'"));
 	}
 
-	if (options.rebase_merges) {
-		if (strategy_options.nr)
-			die(_("cannot combine '--rebase-merges' with "
-			      "'--strategy-option'"));
-		if (options.strategy)
-			die(_("cannot combine '--rebase-merges' with "
-			      "'--strategy'"));
-	}
-
 	if (!options.root) {
 		if (argc < 1) {
 			struct branch *branch;

sequencer.c +12 -2

index 334de14542..d228448cd8 100644
@@ -3256,6 +3256,9 @@ static int do_merge(struct repository *r,
 	struct commit *head_commit, *merge_commit, *i;
 	struct commit_list *bases, *j, *reversed = NULL;
 	struct commit_list *to_merge = NULL, **tail = &to_merge;
+	const char *strategy = !opts->xopts_nr &&
+		(!opts->strategy || !strcmp(opts->strategy, "recursive")) ?
+		NULL : opts->strategy;
 	struct merge_options o;
 	int merge_arg_len, oneline_offset, can_fast_forward, ret, k;
 	static struct lock_file lock;

@@ -3404,7 +3407,7 @@ static int do_merge(struct repository *r,
 		goto leave_merge;
 	}
 
-	if (to_merge->next) {
+	if (strategy || to_merge->next) {
 		/* Octopus merge */
 		struct child_process cmd = CHILD_PROCESS_INIT;

@@ -3418,7 +3421,14 @@ static int do_merge(struct repository *r,
 		cmd.git_cmd = 1;
 		argv_array_push(&cmd.args, "merge");
 		argv_array_push(&cmd.args, "-s");
-		argv_array_push(&cmd.args, "octopus");
+		if (!strategy)
+			argv_array_push(&cmd.args, "octopus");
+		else {
+			argv_array_push(&cmd.args, strategy);
+			for (k = 0; k < opts->xopts_nr; k++)
+				argv_array_pushf(&cmd.args,
+						 "-X%s", opts->xopts[k]);
+		}
 		argv_array_push(&cmd.args, "--no-edit");
 		argv_array_push(&cmd.args, "--no-ff");
 		argv_array_push(&cmd.args, "--no-log");

t/t3422-rebase-incompatible-options.sh +0 ~~-10~~

index a5868ea152..50e7960702 100755
@@ -76,14 +76,4 @@ test_expect_success REBASE_P \
 	test_must_fail git rebase --preserve-merges --rebase-merges A
 '
 
-test_expect_success '--rebase-merges incompatible with --strategy' '
-	git checkout B^0 &&
-	test_must_fail git rebase --rebase-merges -s resolve A
-'
-
-test_expect_success '--rebase-merges incompatible with --strategy-option' '
-	git checkout B^0 &&
-	test_must_fail git rebase --rebase-merges -Xignore-space-change A
-'
-
 test_done

t/t3430-rebase-merges.sh +21 -0

index 42ba5b9f09..8ea6ff3548 100755
@@ -412,4 +412,25 @@ test_expect_success '--continue after resolving conflicts after a merge' '
 	test_path_is_missing .git/MERGE_HEAD
 '
 
+test_expect_success '--rebase-merges with strategies' '
+	git checkout -b with-a-strategy F &&
+	test_tick &&
+	git merge -m "Merge conflicting-G" conflicting-G &&
+
+	: first, test with a merge strategy option &&
+	git rebase -ir -Xtheirs G &&
+	echo conflicting-G >expect &&
+	test_cmp expect G.t &&
+
+	: now, try with a merge strategy other than recursive &&
+	git reset --hard @{1} &&
+	write_script git-merge-override <<-\EOF &&
+	echo overridden$1 >>G.t
+	git add G.t
+	EOF
+	PATH="$PWD:$PATH" git rebase -ir -s override -Xxopt G &&
+	test_write_lines G overridden--xopt >expect &&
+	test_cmp expect G.t
+'
+
 test_done

Signature · 1 line

gitgitgadget

Johannes Schindelin via GitGitGadget

author

2019-07-31 15:19 UTC #28 lore raw

PATCH v2 9/16 t3427: condense the unnecessarily repetitive test cases into three

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

Previously, this test script performed essentially three rebases and verified breakages by testing the post-rebase commits' messages.

To do so, the rebases were performed multiple times, though, once per commit message to test. This wastes electricity (and CO2) and time.

Let's condense the test cases to the essential number: the number of different rebases to validate.

Signed-off-by Johannes Schindelin <[email protected]>

---
 t/t3427-rebase-subtree.sh | 61 +++++++--------------------------------
 1 file changed, 11 insertions(+), 50 deletions(-)

t/t3427-rebase-subtree.sh +11 ~~-50~~

index 64ba95f3e0..b21925d279 100755
@@ -64,70 +64,31 @@ test_expect_success 'setup' '
 '
 
 # FAILURE: Does not preserve master4.
-test_expect_failure REBASE_P \
-	'Rebase -Xsubtree --preserve-merges --onto commit 4' '
+test_expect_failure REBASE_P 'Rebase -Xsubtree --preserve-merges --onto commit' '
 	reset_rebase &&
-	git checkout -b rebase-preserve-merges-4 to-rebase &&
-	git rebase -Xsubtree=files_subtree --preserve-merges --onto files-master master &&
-	verbose test "$(commit_message HEAD~)" = "files_subtree/master4"
-'
-
-# FAILURE: Does not preserve master5.
-test_expect_failure REBASE_P \
-	'Rebase -Xsubtree --preserve-merges --onto commit 5' '
-	reset_rebase &&
-	git checkout -b rebase-preserve-merges-5 to-rebase &&
+	git checkout -b rebase-preserve-merges to-rebase &&
 	git rebase -Xsubtree=files_subtree --preserve-merges --onto files-master master &&
+	verbose test "$(commit_message HEAD~)" = "files_subtree/master4" &&
 	verbose test "$(commit_message HEAD)" = "files_subtree/master5"
 '
 
 # FAILURE: Does not preserve master4.
-test_expect_failure REBASE_P \
-	'Rebase -Xsubtree --keep-empty --preserve-merges --onto commit 4' '
+test_expect_failure REBASE_P 'Rebase -Xsubtree --keep-empty --preserve-merges --onto commit' '
 	reset_rebase &&
-	git checkout -b rebase-keep-empty-4 to-rebase &&
-	git rebase -Xsubtree=files_subtree --keep-empty --preserve-merges --onto files-master master &&
-	verbose test "$(commit_message HEAD~2)" = "files_subtree/master4"
-'
-
-# FAILURE: Does not preserve master5.
-test_expect_failure REBASE_P \
-	'Rebase -Xsubtree --keep-empty --preserve-merges --onto commit 5' '
-	reset_rebase &&
-	git checkout -b rebase-keep-empty-5 to-rebase &&
-	git rebase -Xsubtree=files_subtree --keep-empty --preserve-merges --onto files-master master &&
-	verbose test "$(commit_message HEAD~)" = "files_subtree/master5"
-'
-
-# FAILURE: Does not preserve Empty.
-test_expect_failure REBASE_P \
-	'Rebase -Xsubtree --keep-empty --preserve-merges --onto empty commit' '
-	reset_rebase &&
-	git checkout -b rebase-keep-empty-empty to-rebase &&
+	git checkout -b rebase-keep-empty to-rebase &&
 	git rebase -Xsubtree=files_subtree --keep-empty --preserve-merges --onto files-master master &&
+	verbose test "$(commit_message HEAD~2)" = "files_subtree/master4" &&
+	verbose test "$(commit_message HEAD~)" = "files_subtree/master5" &&
 	verbose test "$(commit_message HEAD)" = "Empty commit"
 '
 
 # FAILURE: fatal: Could not parse object
-test_expect_failure 'Rebase -Xsubtree --onto commit 4' '
-	reset_rebase &&
-	git checkout -b rebase-onto-4 to-rebase &&
-	git rebase -Xsubtree=files_subtree --onto files-master master &&
-	verbose test "$(commit_message HEAD~2)" = "files_subtree/master4"
-'
-
-# FAILURE: fatal: Could not parse object
-test_expect_failure 'Rebase -Xsubtree --onto commit 5' '
-	reset_rebase &&
-	git checkout -b rebase-onto-5 to-rebase &&
-	git rebase -Xsubtree=files_subtree --onto files-master master &&
-	verbose test "$(commit_message HEAD~)" = "files_subtree/master5"
-'
-# FAILURE: fatal: Could not parse object
-test_expect_failure 'Rebase -Xsubtree --onto empty commit' '
+test_expect_failure 'Rebase -Xsubtree --onto commit' '
 	reset_rebase &&
-	git checkout -b rebase-onto-empty to-rebase &&
+	git checkout -b rebase-onto to-rebase &&
 	git rebase -Xsubtree=files_subtree --onto files-master master &&
+	verbose test "$(commit_message HEAD~2)" = "files_subtree/master4" &&
+	verbose test "$(commit_message HEAD~)" = "files_subtree/master5" &&
 	verbose test "$(commit_message HEAD)" = "Empty commit"
 '

Signature · 1 line

gitgitgadget

Johannes Schindelin via GitGitGadget

author

2019-07-31 15:19 UTC #31 lore raw

PATCH v2 12/16 t3427: fix another incorrect assumption

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

This commit was generated by passing a subtree merge commit through git filter-branch --subdirectory-filter, so it looks as if this commit really introduces all those files.

Let's fix the test case to expect exactly this situation.

With this change, we can mark the original bug that this test case tried to demonstrate as fixed.

Signed-off-by Johannes Schindelin <[email protected]>

---
 t/t3427-rebase-subtree.sh | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

t/t3427-rebase-subtree.sh +4 -3

index a734716ea3..7a37235768 100755
@@ -82,11 +82,12 @@ test_expect_failure REBASE_P 'Rebase -Xsubtree --keep-empty --preserve-merges --
 	verbose test "$(commit_message HEAD)" = "Empty commit"
 '
 
-# FAILURE: fatal: Could not parse object
-test_expect_failure 'Rebase -Xsubtree --keep-empty --onto commit' '
+test_expect_success 'Rebase -Xsubtree --keep-empty --onto commit' '
 	reset_rebase &&
 	git checkout -b rebase-onto to-rebase &&
-	git rebase -Xsubtree=files_subtree --keep-empty --onto files-master master &&
+	test_must_fail git rebase -Xsubtree=files_subtree --keep-empty --onto files-master master &&
+	: first pick results in no changes &&
+	git rebase --continue &&
 	verbose test "$(commit_message HEAD~2)" = "master4" &&
 	verbose test "$(commit_message HEAD~)" = "files_subtree/master5" &&
 	verbose test "$(commit_message HEAD)" = "Empty commit"

Signature · 1 line

gitgitgadget

Johannes Schindelin via GitGitGadget

author

2019-07-31 15:19 UTC #29 lore raw

PATCH v2 10/16 t3427: fix erroneous assumption

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

Apart from the setup test case, t3427-rebase-subtree.sh is made up exclusively of demonstrations of breakages. The tricky thing about such demonstrations is that they are often buggy themselves.

Let's fix this, at long last.

Signed-off-by Johannes Schindelin <[email protected]>

---
 t/t3427-rebase-subtree.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

t/t3427-rebase-subtree.sh +3 -3

index b21925d279..94cc532e10 100755
@@ -68,7 +68,7 @@ test_expect_failure REBASE_P 'Rebase -Xsubtree --preserve-merges --onto commit'
 	reset_rebase &&
 	git checkout -b rebase-preserve-merges to-rebase &&
 	git rebase -Xsubtree=files_subtree --preserve-merges --onto files-master master &&
-	verbose test "$(commit_message HEAD~)" = "files_subtree/master4" &&
+	verbose test "$(commit_message HEAD~)" = "master4" &&
 	verbose test "$(commit_message HEAD)" = "files_subtree/master5"
 '

@@ -77,7 +77,7 @@ test_expect_failure REBASE_P 'Rebase -Xsubtree --keep-empty --preserve-merges --
 	reset_rebase &&
 	git checkout -b rebase-keep-empty to-rebase &&
 	git rebase -Xsubtree=files_subtree --keep-empty --preserve-merges --onto files-master master &&
-	verbose test "$(commit_message HEAD~2)" = "files_subtree/master4" &&
+	verbose test "$(commit_message HEAD~2)" = "master4" &&
 	verbose test "$(commit_message HEAD~)" = "files_subtree/master5" &&
 	verbose test "$(commit_message HEAD)" = "Empty commit"
 '

@@ -87,7 +87,7 @@ test_expect_failure 'Rebase -Xsubtree --onto commit' '
 	reset_rebase &&
 	git checkout -b rebase-onto to-rebase &&
 	git rebase -Xsubtree=files_subtree --onto files-master master &&
-	verbose test "$(commit_message HEAD~2)" = "files_subtree/master4" &&
+	verbose test "$(commit_message HEAD~2)" = "master4" &&
 	verbose test "$(commit_message HEAD~)" = "files_subtree/master5" &&
 	verbose test "$(commit_message HEAD)" = "Empty commit"
 '

Signature · 1 line

gitgitgadget

Johannes Schindelin via GitGitGadget

author

2019-07-31 15:19 UTC #30 lore raw

PATCH v2 11/16 t3427: accommodate for the `rebase --merge` backend having been replaced

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

Since 68aa495b590 (rebase: implement --merge via the interactive machinery, 2018-12-11), the job of the old --merge backend is now performed by the --interactive backend, too.

One consequence is that empty commits are no longer rebased by default.

Meaning that the test case that calls git rebase -Xsubtree (which used to be handled by the --merge backend) now needs to ask explicitly for the empty commit to be rebased.

Signed-off-by Johannes Schindelin <[email protected]>

---
 t/t3427-rebase-subtree.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

t/t3427-rebase-subtree.sh +2 -2

index 94cc532e10..a734716ea3 100755
@@ -83,10 +83,10 @@ test_expect_failure REBASE_P 'Rebase -Xsubtree --keep-empty --preserve-merges --
 '
 
 # FAILURE: fatal: Could not parse object
-test_expect_failure 'Rebase -Xsubtree --onto commit' '
+test_expect_failure 'Rebase -Xsubtree --keep-empty --onto commit' '
 	reset_rebase &&
 	git checkout -b rebase-onto to-rebase &&
-	git rebase -Xsubtree=files_subtree --onto files-master master &&
+	git rebase -Xsubtree=files_subtree --keep-empty --onto files-master master &&
 	verbose test "$(commit_message HEAD~2)" = "master4" &&
 	verbose test "$(commit_message HEAD~)" = "files_subtree/master5" &&
 	verbose test "$(commit_message HEAD)" = "Empty commit"

Signature · 1 line

gitgitgadget

Johannes Schindelin via GitGitGadget

author

2019-07-31 15:19 UTC #26 lore raw

PATCH v2 7/16 t3427: simplify the `setup` test case significantly

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

It still does the very same thing as before, but expresses it in a much more succinct (and still quite readable) manner.

Signed-off-by Johannes Schindelin <[email protected]>

---
 t/t3427-rebase-subtree.sh | 36 +++++++++++++++---------------------
 1 file changed, 15 insertions(+), 21 deletions(-)

t/t3427-rebase-subtree.sh +15 ~~-21~~

index 3a2ae7b55d..36c4f92e06 100755
@@ -41,27 +41,21 @@ commit_message() {
 
 test_expect_success 'setup' '
 	test_commit README &&
-	mkdir files &&
-	(
-		cd files &&
-		git init &&
-		test_commit master1 &&
-		test_commit master2 &&
-		test_commit master3
-	) &&
-	git fetch files master &&
-	git branch files-master FETCH_HEAD &&
-	git read-tree --prefix=files_subtree files-master &&
-	git checkout -- files_subtree &&
-	tree=$(git write-tree) &&
-	head=$(git rev-parse HEAD) &&
-	rev=$(git rev-parse --verify files-master^0) &&
-	commit=$(git commit-tree -p $head -p $rev -m "Add subproject master" $tree) &&
-	git update-ref HEAD $commit &&
-	(
-		cd files_subtree &&
-		test_commit master4
-	) &&
+
+	git init files &&
+	test_commit -C files master1 &&
+	test_commit -C files master2 &&
+	test_commit -C files master3 &&
+
+	: perform subtree merge into files_subtree/ &&
+	git fetch files refs/heads/master:refs/heads/files-master &&
+	git merge -s ours --no-commit --allow-unrelated-histories \
+		files-master &&
+	git read-tree --prefix=files_subtree -u files-master &&
+	git commit -m "Add subproject master" &&
+
+	: add two extra commits to rebase &&
+	test_commit -C files_subtree master4 &&
 	test_commit files_subtree/master5
 '

Signature · 1 line

gitgitgadget

Elijah Newren

2019-09-04 21:40 UTC #36 lore raw

[PATCH v2 17/16] t3427: accelerate this test by using fast-export and fast-import

In reply to Johannes Schindelin via GitGitGadget

fast-export and fast-import can easily handle the simple rewrite that was being done by filter-branch, and should be faster on systems with a slow fork. Measuring the overall time taken for all of t3427 (not just the difference between filter-branch and fast-export/fast-import) shows a speedup of about 5% on Linux and 11% on Mac.

Signed-off-by Elijah Newren <[email protected]>

---
This patch is meant to be added onto the end of js/rebase-r-strategy; an earlier version of this patch conflicted js/rebase-r-strategy so now I'm basing on top of that series. The speedup is also less impressive now that there is only one filter-branch invocation being replaced instead of a handful. Still a nice speedup, though.

 t/t3427-rebase-subtree.sh | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

t/t3427-rebase-subtree.sh +4 -1

index 39e348de16..bec48e6a1f 100755
@@ -59,7 +59,10 @@ test_expect_success 'setup' '
 	test_commit files_subtree/master5 &&
 
 	git checkout -b to-rebase &&
-	git filter-branch --prune-empty -f --subdirectory-filter files_subtree &&
+	git fast-export --no-data HEAD -- files_subtree/ |
+		sed -e "s%\([0-9a-f]\{40\} \)files_subtree/%\1%" |
+		git fast-import --force --quiet &&
+	git reset --hard &&
 	git commit -m "Empty commit" --allow-empty
 '

Signature · 1 line

2.22.0.19.ga495766805

Johannes Schindelin

2019-09-09 20:29 UTC #37 lore raw

Re: [PATCH v2 17/16] t3427: accelerate this test by using fast-export and fast-import

Hi Elijah,

Quoted 6 lines · Elijah Newren

fast-export and fast-import can easily handle the simple rewrite that was being done by filter-branch, and should be faster on systems with a slow fork. Measuring the overall time taken for all of t3427 (not just the difference between filter-branch and fast-export/fast-import) shows a speedup of about 5% on Linux and 11% on Mac.

Quoted 7 lines

Signed-off-by Elijah Newren <[email protected]>
---
This patch is meant to be added onto the end of js/rebase-r-strategy; an earlier version of this patch conflicted js/rebase-r-strategy so now I'm basing on top of that series. The speedup is also less impressive now that there is only one filter-branch invocation being replaced instead of a handful. Still a nice speedup, though.

ACK!

Thanks,
Dscho

Quoted 24 lines

 t/t3427-rebase-subtree.sh | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

t/t3427-rebase-subtree.sh +4 -2

index 39e348de16..bec48e6a1f 100755
@@ -59,7 +59,10 @@ test_expect_success 'setup' '
 	test_commit files_subtree/master5 &&
 
 	git checkout -b to-rebase &&
-	git filter-branch --prune-empty -f --subdirectory-filter files_subtree &&
+	git fast-export --no-data HEAD -- files_subtree/ |
+		sed -e "s%\([0-9a-f]\{40\} \)files_subtree/%\1%" |
+		git fast-import --force --quiet &&
+	git reset --hard &&
 	git commit -m "Empty commit" --allow-empty
 '
 
--

2.22.0.19.ga495766805

Junio C Hamano

2019-09-09 21:06 UTC #38 lore raw

Re: [PATCH v2 17/16] t3427: accelerate this test by using fast-export and fast-import

Quoted 18 lines · Johannes Schindelin

Hi Elijah,
Quoted 6 lines · Elijah Newren
fast-export and fast-import can easily handle the simple rewrite that was being done by filter-branch, and should be faster on systems with a slow fork. Measuring the overall time taken for all of t3427 (not just the difference between filter-branch and fast-export/fast-import) shows a speedup of about 5% on Linux and 11% on Mac.
Quoted 7 lines
Signed-off-by Elijah Newren <[email protected]>
---
This patch is meant to be added onto the end of js/rebase-r-strategy; an earlier version of this patch conflicted js/rebase-r-strategy so now I'm basing on top of that series. The speedup is also less impressive now that there is only one filter-branch invocation being replaced instead of a handful. Still a nice speedup, though.

Quoted 4 lines

ACK!
Thanks,
Dscho

Thanks, both. This indeed is a good update.

Quoted 25 lines


 t/t3427-rebase-subtree.sh | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

t/t3427-rebase-subtree.sh +4 -2

index 39e348de16..bec48e6a1f 100755
@@ -59,7 +59,10 @@ test_expect_success 'setup' '
 	test_commit files_subtree/master5 &&
 
 	git checkout -b to-rebase &&
-	git filter-branch --prune-empty -f --subdirectory-filter files_subtree &&
+	git fast-export --no-data HEAD -- files_subtree/ |
+		sed -e "s%\([0-9a-f]\{40\} \)files_subtree/%\1%" |
+		git fast-import --force --quiet &&
+	git reset --hard &&
 	git commit -m "Empty commit" --allow-empty
 '
 
--

2.22.0.19.ga495766805