rebase-i-p: squashing and limiting todo
RFC PATCH v2 1/1 rebase-i-p: squashing and limiting todo
This is v2 of my attempt at keeping commits with no-parents-being-rewritten off the rebase-i-p todo list. This was prompted by Avi Kivity's request to squash commits on either side of a merge, but is something I had wanted as well without fully realizing it.
Stephan Beyer pointed out his git sequencer branch already does this, contrary to my incorrect reading of it. His rewrite of rebase-i-p changed how todo was generated to follow these semantics, and nicely, too, so I replaced my initial hacky algorithm for "only include commits with rewritten parents" with his.
The better implementation aside, I found Stephan's matching semantics encouraging as if Avi, myself, and Stephan (plus others/the list involved in past git sequencer discussions) all expect rebase-i-p to drop non-rewritten commits, then the semantics should be intuitive and what most users expect.
Also, even though git sequencer has these semantics (and more), I'd like to see this patch reviewed and applied anyway as I think it is a nice solution to the current rebase-i-p shortcomings. And, assuming its approved, it can be applied now instead of waiting for git sequencer to be finished, reviewed, and applied.
This is also my first patch series. I started a new thread--I'm not sure if I was supposed to use Avi's original message id or not. Also, I cheated and put the tests in the first commit instead of starting them as test_expect_failure and gradually changing them to test_expect_success.
Stephen Haberman (7):
rebase-i-p: test to exclude commits from todo based on its parents
rebase-i-p: use HEAD for updating the ref instead of mapping OLDHEAD
rebase-i-p: delay saving current-commit to REWRITTEN if squashing
rebase-i-p: fix 'no squashing merges' tripping up non-merges
rebase-i-p: only list commits that require rewriting in todo
rebase-i-p: do not include non-first-parent commits touching UPSTREAM
rebase-i-p: if todo was reordered use HEAD as the rewritten parent
git-rebase--interactive.sh | 80 ++++++++++++------ t/t3411-rebase-preserve-around-merges.sh | 136 ++++++++++++++++++++++++++++++ 2 files changed, 190 insertions(+), 26 deletions(-) create mode 100644 t/t3411-rebase-preserve-around-merges.sh
Quoted 3 lines
This is v2 of my attempt at keeping commits with no-parents-being-rewritten off the rebase-i-p todo list.
I forgot to mention, this series almost certainly conflicts with pu's sh/maint-rebase3. Given that pu is not next, I think I did the right thing by keeping this series separate, but I can merge them into one series if that is preferable.
Thanks,
Stephen
RFC PATCH v2 1/1 rebase-i-p: test to exclude commits from todo based on its parents
In reply to Stephen HabermanThe first case was based off a script from Avi Kivity <[email protected]>.
The second case includes a merge-of-a-merge to ensure both are included in todo.
--- t/t3411-rebase-preserve-around-merges.sh | 136 ++++++++++++++++++++++++++++++ 1 files changed, 136 insertions(+), 0 deletions(-) create mode 100644 t/t3411-rebase-preserve-around-merges.sh
t/t3411-rebase-preserve-around-merges.sh new file +136 -0
@@ -0,0 +1,136 @@ +#!/bin/sh +# +# Copyright (c) 2008 Stephen Haberman +# + +test_description='git rebase preserve merges + +This test runs git rebase with and tries to squash a commit from after a merge +to before the merge. +' +. ./test-lib.sh + +# Copy/paste from t3404-rebase-interactive.sh +echo "#!$SHELL_PATH" >fake-editor.sh +cat >> fake-editor.sh <<\EOF +case "$1" in +*/COMMIT_EDITMSG) + 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" +cat "$1".tmp +action=pick +for line in $FAKE_LINES; do + case $line in + squash|edit) + action="$line";; + *) + echo sed -n "${line}s/^pick/$action/p" + sed -n "${line}p" < "$1".tmp + sed -n "${line}s/^pick/$action/p" < "$1".tmp >> "$1" + action=pick;; + esac +done +EOF + +test_set_editor "$(pwd)/fake-editor.sh" +chmod a+x fake-editor.sh + +# set up two branches like this: +# +# A1 - B1 - D1 - E1 - F1 +# \ / +# -- C1 -- + +test_expect_success 'setup' ' + touch a && + touch b && + git add a && + git commit -m A1 && + git tag A1 + git add b && + git commit -m B1 && + git tag B1 && + git checkout -b branch && + touch c && + git add c && + git commit -m C1 && + git checkout master && + touch d && + git add d && + git commit -m D1 && + git merge branch && + touch f && + git add f && + git commit -m F1 && + git tag F1 +' + +# Should result in: +# +# A1 - B1 - D2 - E2 +# \ / +# -- C1 -- +# +test_expect_success 'squash F1 into D1' ' + FAKE_LINES="1 squash 3 2" git rebase -i -p B1 && + test "$(git rev-parse HEAD^2)" = "$(git rev-parse branch)" && + test "$(git rev-parse HEAD~2)" = "$(git rev-parse B1)" && + git tag E2 +' + +# Start with: +# +# A1 - B1 - D2 - E2 +# \ +# G1 ---- L1 ---- M1 +# \ / +# H1 -- J1 -- K1 +# \ / +# -- I1 -- +# +# And rebase G1..M1 onto E3 + +test_expect_success 'rebase two levels of merge' ' + git checkout -b branch2 A1 && + touch g && + git add g && + git commit -m G1 && + git checkout -b branch3 && + touch h + git add h && + git commit -m H1 && + git checkout -b branch4 && + touch i && + git add i && + git commit -m I1 && + git tag I1 && + git checkout branch3 && + touch j && + git add j && + git commit -m J1 && + git merge I1 --no-commit && + git commit -m K1 && + git tag K1 && + git checkout branch2 && + touch l && + git add l && + git commit -m L1 && + git merge K1 --no-commit && + git commit -m M1 && + GIT_EDITOR=: git rebase -i -p E2 && + test "$(git rev-parse HEAD~3)" = "$(git rev-parse E2)" && + test "$(git rev-parse HEAD~2)" = "$(git rev-parse HEAD^2^2~2)" && + test "$(git rev-parse HEAD^2^1^1)" = "$(git rev-parse HEAD^2^2^1)" +' + +test_done +
Signature · 1 line
1.6.0.2
Re: [PATCH v2 RFC] rebase-i-p: test to exclude commits from todo based on its parents
Quoted 12 lines · Stephen Haberman
t/t3411-rebase-preserve-around-merges.sh new file +10
-0+# set up two branches like this: +# +# A1 - B1 - D1 - E1 - F1 +# \ / +# -- C1 -- +test_expect_success 'setup' ' +' + +# Should result in: +#
t/t3411-rebase-preserve-around-merges.sh line 88
Quoted 10 lines
t/t3411-rebase-preserve-around-merges.sh new file +10
-0+# A1 - B1 - D2 - E2 +# \ / +# -- C1 -- +# +test_expect_success 'squash F1 into D1' ' + FAKE_LINES="1 squash 3 2" git rebase -i -p B1 && + test "$(git rev-parse HEAD^2)" = "$(git rev-parse branch)" && + test "$(git rev-parse HEAD~2)" = "$(git rev-parse B1)" && + git tag E2 +'
Very nicely written test sequence with pictures. I wish all the tests were written like this.
t/t3411-rebase-preserve-around-merges.sh line 100
Quoted 11 lines
t/t3411-rebase-preserve-around-merges.sh new file +11
-0+# Start with: +# +# A1 - B1 - D2 - E2 +# \ +# G1 ---- L1 ---- M1 +# \ / +# H1 -- J1 -- K1 +# \ / +# -- I1 -- +# +# And rebase G1..M1 onto E3
I think s/E3/E2/ is what you meant.
If this is the first one in the series, you may want to mark these tests with test_expect_failure and make a later patch that changes the behaviour of rebase-i-p include updates to the test script to turn expect-failure into expect-success.
Re: [PATCH v2 RFC] rebase-i-p: test to exclude commits from todo based on its parents
Quoted 4 lines
Quoted 2 lines
+# And rebase G1..M1 onto E3
I think s/E3/E2/ is what you meant.
Ah, yes.
Quoted 4 lines
If this is the first one in the series, you may want to mark these tests with
test_expect_failureand make a later patch that changes the behaviour of rebase-i-p include updates to the test script to turn expect-failure into expect-success.
Sounds good, I will do that.
I haven't seen any other feedback yet--should I repost the series with these two minor changes right now or wait and see if other feedback comes up?
Thanks,
Stephen
RFC PATCH v2 1/1 rebase-i-p: use HEAD for updating the ref instead of mapping OLDHEAD
In reply to Stephen HabermanIf OLDHEAD was reordered in the todo, and its mapped NEWHEAD was used to set the ref, commits reordered after OLDHEAD in the todo would should up as un-committed changes.
--- git-rebase--interactive.sh | 15 +-------------- 1 files changed, 1 insertions(+), 14 deletions(-)
git-rebase--interactive.sh +1 -14
@@ -350,20 +350,7 @@ do_next () { HEADNAME=$(cat "$DOTEST"/head-name) && OLDHEAD=$(cat "$DOTEST"/head) && SHORTONTO=$(git rev-parse --short $(cat "$DOTEST"/onto)) && - if test -d "$REWRITTEN" - then - test -f "$DOTEST"/current-commit && - current_commit=$(cat "$DOTEST"/current-commit) && - git rev-parse HEAD > "$REWRITTEN"/$current_commit - if test -f "$REWRITTEN"/$OLDHEAD - then - NEWHEAD=$(cat "$REWRITTEN"/$OLDHEAD) - else - NEWHEAD=$OLDHEAD - fi - else - NEWHEAD=$(git rev-parse HEAD) - fi && + NEWHEAD=$(git rev-parse HEAD) && case $HEADNAME in refs/*) message="$GIT_REFLOG_ACTION: $HEADNAME onto $SHORTONTO)" &&
Signature · 1 line
1.6.0.2
RFC PATCH v2 1/1 rebase-i-p: delay saving current-commit to REWRITTEN if squashing
If the current-commit was dumped to REWRITTEN, but then we squash the next commit in to it, we have invalidated the HEAD was just written to REWRITTEN. Instead, append the squash hash to current-commit and save both of them the next time around.
--- git-rebase--interactive.sh | 15 ++++++++++----- 1 files changed, 10 insertions(+), 5 deletions(-)
git-rebase--interactive.sh +10 -5
@@ -159,13 +159,18 @@ pick_one_preserving_merges () { if test -f "$DOTEST"/current-commit then - current_commit=$(cat "$DOTEST"/current-commit) && - git rev-parse HEAD > "$REWRITTEN"/$current_commit && - rm "$DOTEST"/current-commit || - die "Cannot write current commit's replacement sha1" + if [ "$fast_forward" == "t" ] + then + cat "$DOTEST"/current-commit | while read current_commit + do + git rev-parse HEAD > "$REWRITTEN"/$current_commit + done + rm "$DOTEST"/current-commit || + die "Cannot write current commit's replacement sha1" + fi fi - echo $sha1 > "$DOTEST"/current-commit + echo $sha1 >> "$DOTEST"/current-commit # rewrite parents; if none were rewritten, we can fast-forward. new_parents=
Signature · 1 line
1.6.0.2
RFC PATCH v2 1/1 rebase-i-p: fix 'no squashing merges' tripping up non-merges
Also only check out the first parent if this commit if not a squash--if it is a squash, we want to explicitly ignore the parent and leave the wc as is, as cherry-pick will apply the squash on top of it.
--- git-rebase--interactive.sh | 14 +++++++++----- 1 files changed, 9 insertions(+), 5 deletions(-)
git-rebase--interactive.sh +9 -5
@@ -198,15 +198,19 @@ pick_one_preserving_merges () { die "Cannot fast forward to $sha1" ;; f) - test "a$1" = a-n && die "Refusing to squash a merge: $sha1" - first_parent=$(expr "$new_parents" : ' \([^ ]*\)') - # detach HEAD to current parent - output git checkout $first_parent 2> /dev/null || - die "Cannot move HEAD to $first_parent" + + if [ "$1" != "-n" ] + then + # detach HEAD to current parent + output git checkout $first_parent 2> /dev/null || + die "Cannot move HEAD to $first_parent" + fi case "$new_parents" in ' '*' '*) + test "a$1" = a-n && die "Refusing to squash a merge: $sha1" + # redo merge author_script=$(get_author_ident_from_commit $sha1) eval "$author_script"
Signature · 1 line
1.6.0.2
RFC PATCH v2 1/1 rebase-i-p: only list commits that require rewriting in todo
This is heavily based on Stephan Beyer's git sequencer rewrite of rebase-i-p.
Each commit is still found by rev-list UPSTREAM..HEAD, but a commit is only included in todo if at least one its parents has been marked for rewriting.
--- git-rebase--interactive.sh | 26 ++++++++++++++++++++++++-- 1 files changed, 24 insertions(+), 2 deletions(-)
git-rebase--interactive.sh +24 -2
@@ -561,9 +561,31 @@ first and then run 'git rebase --continue' again." SHORTHEAD=$(git rev-parse --short $HEAD) SHORTONTO=$(git rev-parse --short $ONTO) git rev-list $MERGES_OPTION --pretty=oneline --abbrev-commit \ - --abbrev=7 --reverse --left-right --cherry-pick \ + --abbrev=7 --reverse --left-right --cherry-pick --topo-order \ $UPSTREAM...$HEAD | \ - sed -n "s/^>/pick /p" > "$TODO" + sed -n "s/^>//p" | while read shortsha1 rest + do + if test t != "$PRESERVE_MERGES" + then + echo "pick $shortsha1 $rest" >> "$TODO" + else + sha1=$(git rev-parse $shortsha1) + preserve=t + for p in $(git rev-list --parents -1 $sha1 | cut -d' ' -f2-) + do + if test -f "$REWRITTEN"/$p + then + preserve=f + fi + done + if test f = "$preserve" + then + touch "$REWRITTEN/$sha1" + echo "pick $shortsha1 $rest" >> "$TODO" + fi + fi + done + cat >> "$TODO" << EOF # Rebase $SHORTUPSTREAM..$SHORTHEAD onto $SHORTONTO
Signature · 1 line
1.6.0.2
RFC PATCH v2 1/1 rebase-i-p: do not include non-first-parent commits touching UPSTREAM
This covers an odd boundary case found by Avi Kivity's script where a branch coming off of UPSTREAM is merged into HEAD. Initially it show up in UPSTREAM..HEAD, but technically UPSTREAM is not moving, the rest of head is, so we should not need to rewrite the merge.
This adds a check saying we can keep preserve=t if p=UPSTREAM...unless this is the first first-parent commit in our UPSTREAM..HEAD rev-list, which could very well point to UPSTREAM, but we still need to consider it as rewritten so we start pulling in the rest of the UPSTREAM..HEAD commits that point to it.
--- git-rebase--interactive.sh | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
git-rebase--interactive.sh +2 -1
@@ -553,6 +553,7 @@ first and then run 'git rebase --continue' again." die "Could not init rewritten commits" done MERGES_OPTION= + first_after_upstream="$(git rev-list --reverse --first-parent $UPSTREAM..$HEAD | head -n 1)" else MERGES_OPTION=--no-merges fi
@@ -573,7 +574,7 @@ first and then run 'git rebase --continue' again." preserve=t for p in $(git rev-list --parents -1 $sha1 | cut -d' ' -f2-) do - if test -f "$REWRITTEN"/$p + if test -f "$REWRITTEN"/$p -a \( $p != $UPSTREAM -o $sha1 = $first_after_upstream \) then preserve=f fi
Signature · 1 line
1.6.0.2
RFC PATCH v2 1/1 rebase-i-p: if todo was reordered use HEAD as the rewritten parent
This seems like the best guess we can make until git sequencer marks are available. That being said, within the context of re-ordering a commit before its parent in todo, I think applying it on top of the current commit seems like a reasonable assumption of what the user intended.
--- git-rebase--interactive.sh | 9 +++++++++ 1 files changed, 9 insertions(+), 0 deletions(-)
git-rebase--interactive.sh +9 -0
@@ -179,6 +179,15 @@ pick_one_preserving_merges () { if test -f "$REWRITTEN"/$p then new_p=$(cat "$REWRITTEN"/$p) + + # If the todo reordered commits, and our parent is marked for + # rewriting, but hasn't been gotten to yet, assume the user meant to + # drop it on top of the current HEAD + if test -z "$new_p" + then + new_p=$(git rev-parse HEAD) + fi + test $p != $new_p && fast_forward=f case "$new_parents" in *$new_p*)
Signature · 1 line
1.6.0.2