rebase -r: support merge strategies other than `recursive` #294

dscho · 2019-07-24T11:32:40Z

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.

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

Cc: brian m. carlson [email protected]

dscho · 2019-07-25T10:10:29Z

/submit

gitgitgadget · 2019-07-25T10:11:37Z

Submitted as [email protected]

gitgitgadget · 2019-07-25T22:40:23Z

This branch is now known as js/rebase-r-strategy.

gitgitgadget · 2019-07-25T22:40:24Z

This patch series was integrated into pu via git@be90704.

t/lib-rebase.sh

gitgitgadget · 2019-07-26T22:47:41Z

This patch series was integrated into pu via git@9643821.

gitgitgadget · 2019-07-27T16:17:27Z

This patch series was integrated into pu via git@4f09964.

gitgitgadget · 2019-07-29T05:26:32Z

This patch series was integrated into pu via git@201a12a.

gitgitgadget · 2019-07-29T18:51:04Z

This patch series was integrated into pu via git@c00c9f6.

gitgitgadget · 2019-07-29T21:45:00Z

This patch series was integrated into pu via git@bef7796.

gitgitgadget · 2019-07-30T21:52:05Z

This patch series was integrated into pu via git@c7c300f.

Since 2185362 (built-in rebase: call `git am` directly, 2019-01-18), the built-in rebase already uses the built-in `git am` directly. Now that d03ebd4 (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]>

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

This went away in 0609b74 (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]>

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

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

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

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

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

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

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 5f35900 (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]>

gitgitgadget · 2019-08-09T18:13:25Z

This patch series was integrated into pu via git@61d822e.

gitgitgadget · 2019-08-12T17:55:37Z

This patch series was integrated into pu via git@6adb078.

gitgitgadget · 2019-08-14T21:13:08Z

This patch series was integrated into pu via git@0c5f2bd.

gitgitgadget · 2019-08-16T20:23:25Z

This patch series was integrated into pu via git@9ab2e9b.

gitgitgadget · 2019-08-22T20:48:04Z

This patch series was integrated into pu via git@6187117.

gitgitgadget · 2019-08-24T01:11:27Z

This patch series was integrated into pu via git@16495a3.

gitgitgadget · 2019-08-26T22:14:56Z

This patch series was integrated into pu via git@1886162.

gitgitgadget · 2019-09-03T23:49:23Z

This patch series was integrated into pu via git@79ffd00.

gitgitgadget · 2019-09-04T21:42:08Z

On the Git mailing list, Elijah Newren wrote (reply to this):

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

diff --git a/t/t3427-rebase-subtree.sh b/t/t3427-rebase-subtree.sh
index 39e348de16..bec48e6a1f 100755
--- a/t/t3427-rebase-subtree.sh
+++ b/t/t3427-rebase-subtree.sh
@@ -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

gitgitgadget · 2019-09-05T22:29:52Z

This patch series was integrated into pu via git@be46f04.

gitgitgadget · 2019-09-07T17:21:52Z

This patch series was integrated into pu via git@14950d8.

gitgitgadget · 2019-09-07T17:21:53Z

This patch series was integrated into next via git@71e2451.

gitgitgadget · 2019-09-09T20:32:40Z

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi Elijah,

On Wed, 4 Sep 2019, Elijah Newren wrote:

> 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 o=
f
> a handful.  Still a nice speedup, though.

ACK!

Thanks,
Dscho

>
>  t/t3427-rebase-subtree.sh | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/t/t3427-rebase-subtree.sh b/t/t3427-rebase-subtree.sh
> index 39e348de16..bec48e6a1f 100755
> --- a/t/t3427-rebase-subtree.sh
> +++ b/t/t3427-rebase-subtree.sh
> @@ -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
>
>

gitgitgadget · 2019-09-09T20:44:15Z

This patch series was integrated into pu via git@4537165.

gitgitgadget · 2019-09-09T21:08:40Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

Johannes Schindelin <[email protected]> writes:

> Hi Elijah,
>
> On Wed, 4 Sep 2019, Elijah Newren wrote:
>
>> 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.
>
> ACK!
>
> Thanks,
> Dscho

Thanks, both.  This indeed is a good update.

>
>>
>>  t/t3427-rebase-subtree.sh | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/t/t3427-rebase-subtree.sh b/t/t3427-rebase-subtree.sh
>> index 39e348de16..bec48e6a1f 100755
>> --- a/t/t3427-rebase-subtree.sh
>> +++ b/t/t3427-rebase-subtree.sh
>> @@ -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
>>
>>

gitgitgadget · 2019-09-16T21:51:16Z

This patch series was integrated into pu via git@aa2d3a6.

gitgitgadget · 2019-09-17T22:23:33Z

This patch series was integrated into pu via git@1b55d3b.

gitgitgadget · 2019-09-18T19:51:19Z

This patch series was integrated into pu via git@917a319.

gitgitgadget · 2019-09-18T19:51:20Z

This patch series was integrated into master via git@917a319.

gitgitgadget · 2019-09-18T19:51:23Z

Closed via 917a319.

dscho force-pushed the rebase-r-with-strategies branch 2 times, most recently from 1bf55c5 to e671356 Compare July 24, 2019 21:38

dscho mentioned this pull request Jul 25, 2019

Add the REBASE_P prerequisite to two more test cases #296

Closed

This comment has been minimized.

Sign in to view

dscho force-pushed the js/rebase-cleanup branch from 082ef75 to 80dfc92 Compare July 25, 2019 22:34

gitgitgadget bot added the pu label Jul 25, 2019

gitgitgadget bot reviewed Jul 26, 2019

View reviewed changes

t/lib-rebase.sh Show resolved Hide resolved

This comment has been minimized.

Sign in to view

weekly-digest bot mentioned this pull request Jul 28, 2019

Weekly Digest (21 July, 2019 - 28 July, 2019) #297

Closed

dscho added the needs-work These patches have pending issues that need to be resolved before submitting label Jul 30, 2019

dscho added 10 commits July 31, 2019 13:29

.gitignore: there is no longer a built-in git-rebase--interactive

9e00e66

This went away in 0609b74 (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]>

sequencer: the am and rebase--interactive scripts are gone

db9ec24

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

t3427: add a clarifying comment

b3daf07

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

t3427: simplify the setup test case significantly

c168c44

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

weekly-digest bot mentioned this pull request Aug 11, 2019

Weekly Digest (4 August, 2019 - 11 August, 2019) #310

Closed

weekly-digest bot mentioned this pull request Aug 18, 2019

Weekly Digest (11 August, 2019 - 18 August, 2019) #313

Closed

weekly-digest bot mentioned this pull request Aug 25, 2019

Weekly Digest (18 August, 2019 - 25 August, 2019) #321

Closed

weekly-digest bot mentioned this pull request Sep 1, 2019

Weekly Digest (25 August, 2019 - 1 September, 2019) #326

Closed

gitgitgadget bot added the next label Sep 7, 2019

weekly-digest bot mentioned this pull request Sep 8, 2019

Weekly Digest (1 September, 2019 - 8 September, 2019) #336

Closed

weekly-digest bot mentioned this pull request Sep 15, 2019

Weekly Digest (8 September, 2019 - 15 September, 2019) #338

Closed

gitgitgadget bot added the master label Sep 18, 2019

gitgitgadget bot closed this Sep 18, 2019

dscho deleted the rebase-r-with-strategies branch September 22, 2019 08:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rebase -r: support merge strategies other than `recursive` #294

rebase -r: support merge strategies other than `recursive` #294

dscho commented Jul 24, 2019 •

edited

Loading

dscho commented Jul 25, 2019

gitgitgadget bot commented Jul 25, 2019

This comment has been minimized.

gitgitgadget bot commented Jul 25, 2019

gitgitgadget bot commented Jul 25, 2019

This comment has been minimized.

gitgitgadget bot commented Jul 26, 2019

gitgitgadget bot commented Jul 27, 2019

gitgitgadget bot commented Jul 29, 2019

gitgitgadget bot commented Jul 29, 2019

gitgitgadget bot commented Jul 29, 2019

gitgitgadget bot commented Jul 30, 2019

gitgitgadget bot commented Aug 9, 2019

gitgitgadget bot commented Aug 12, 2019

gitgitgadget bot commented Aug 14, 2019

gitgitgadget bot commented Aug 16, 2019

gitgitgadget bot commented Aug 22, 2019

gitgitgadget bot commented Aug 24, 2019

gitgitgadget bot commented Aug 26, 2019

gitgitgadget bot commented Sep 3, 2019

gitgitgadget bot commented Sep 4, 2019

gitgitgadget bot commented Sep 5, 2019

gitgitgadget bot commented Sep 7, 2019

gitgitgadget bot commented Sep 7, 2019

gitgitgadget bot commented Sep 9, 2019

gitgitgadget bot commented Sep 9, 2019

gitgitgadget bot commented Sep 9, 2019

gitgitgadget bot commented Sep 16, 2019

gitgitgadget bot commented Sep 17, 2019

gitgitgadget bot commented Sep 18, 2019

gitgitgadget bot commented Sep 18, 2019

gitgitgadget bot commented Sep 18, 2019

rebase -r: support merge strategies other than recursive #294

rebase -r: support merge strategies other than recursive #294

Conversation

dscho commented Jul 24, 2019 • edited Loading

dscho commented Jul 25, 2019

gitgitgadget bot commented Jul 25, 2019

This comment has been minimized.

gitgitgadget bot commented Jul 25, 2019

gitgitgadget bot commented Jul 25, 2019

This comment has been minimized.

gitgitgadget bot commented Jul 26, 2019

gitgitgadget bot commented Jul 27, 2019

gitgitgadget bot commented Jul 29, 2019

gitgitgadget bot commented Jul 29, 2019

gitgitgadget bot commented Jul 29, 2019

gitgitgadget bot commented Jul 30, 2019

gitgitgadget bot commented Aug 9, 2019

gitgitgadget bot commented Aug 12, 2019

gitgitgadget bot commented Aug 14, 2019

gitgitgadget bot commented Aug 16, 2019

gitgitgadget bot commented Aug 22, 2019

gitgitgadget bot commented Aug 24, 2019

gitgitgadget bot commented Aug 26, 2019

gitgitgadget bot commented Sep 3, 2019

gitgitgadget bot commented Sep 4, 2019

gitgitgadget bot commented Sep 5, 2019

gitgitgadget bot commented Sep 7, 2019

gitgitgadget bot commented Sep 7, 2019

gitgitgadget bot commented Sep 9, 2019

gitgitgadget bot commented Sep 9, 2019

gitgitgadget bot commented Sep 9, 2019

gitgitgadget bot commented Sep 16, 2019

gitgitgadget bot commented Sep 17, 2019

gitgitgadget bot commented Sep 18, 2019

gitgitgadget bot commented Sep 18, 2019

gitgitgadget bot commented Sep 18, 2019

rebase -r: support merge strategies other than `recursive` #294

rebase -r: support merge strategies other than `recursive` #294

dscho commented Jul 24, 2019 •

edited

Loading