{"thread":{"id":"15788","subject":"git rebase -i -p broken?","startedAt":"2008-10-05T15:30:06Z","lastAt":"2008-10-07T14:38:02Z","messageCount":8,"participants":["Avi Kivity","Stephen Haberman","Stephan Beyer","Shawn O. Pearce"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"92335","messageId":"48E8DD7E.9040706@redhat.com","threadId":"15788","inReplyTo":null,"subject":"git rebase -i -p broken?","fromName":"Avi Kivity","fromEmail":"avi@redhat.com","sentAt":"2008-10-05T15:30:06Z","receivedAt":"2008-10-05T15:30:06Z","isPatch":false,"sender":{"key":"avi@redhat.com","avatar":null},"body":"Consider this scenario:\n- commit some patch (named 'a')\n- merge a random branch\n- commit a fix to patch a\n- since I haven't pushed yet, I want to squash a and a-fix together, to \nprevent bisect problems\n- fire up 'git rebase -i -p a^'\n\nNow the problems begin:\n- the todo list shows up the branch's commits as well as my current \nbranch.  But I don't want to commit the branch's commits in my own \nbranch.  Replaying the merge should be enough.  Looks like a missing \n--first-parent somewhere.\n- removing the spurious commit from the todo, and moving a-fix after a \nand marking it as a squash action, I get\n   Refusing to squash a merge: 51ca22d7afb7433332ae41d0c2e3bab598048c21\n  even though that's not a merge\n- using git commit --amend instead of squash confuses git in some other way\n\nAttached is a script that generates a test case.  With some $EDITOR \nhacks it can even be convinced to be an automated test case.\n\nAll this using 1.6.0.2.\n\n-- \nerror compiling committee.c: too many arguments to function\n\n\n\n!#/bin/sh -e\n\ngit --version\nmkdir repo\ncd repo\ngit init\ntouch a\ntouch 0\ngit add 0\ngit commit -m 'zeroth commit'\ngit add a\ngit commit -m 'first commit'\ngit checkout -b branch\ntouch b\ngit add b\ngit commit -m 'second commit (branch)'\ngit checkout master\ntouch c\ngit add c\ngit commit -m 'third commit'\ngit merge branch\ntouch d\ngit add d\ngit commit -m 'fifth commit'\ngit rebase -i -p HEAD~4"},{"id":"92418","messageId":"20081006102118.3e817a0f.stephen@exigencecorp.com","threadId":"15788","inReplyTo":"48E8DD7E.9040706@redhat.com","subject":"[PATCH RFC] rebase--interactive: if preserving merges, use first-parent to limit what is shown.","fromName":"Stephen Haberman","fromEmail":"stephen@exigencecorp.com","sentAt":"2008-10-06T15:21:18Z","receivedAt":"2008-10-06T15:21:18Z","isPatch":true,"sender":{"key":"stephen@exigencecorp.com","avatar":"https://gravatar.com/avatar/23b93ad70a06ce53505f17ddba65176edbcfb6588e7a4c1a2dca04aaf0a6aff1?d=mp&s=160"},"body":"This commit fixes Avi Kivity's use case of squashing two commits on either side\nof a merge together.\n\nChanges include:\n\n- Delaying storing the rewrite information if the current commit we are\n  applying is being squashed. This means storing multiple lines in the\n  current-commit file and recording each of them as rewritten to the\n  same HEAD on the next commit.\n\n- Move the \"no squashing merges\" check into the case statement for\n  merges as previously it was catching catching even single-parent\n  squashes.\n\n- Conditionally pass \"--first-parent\" to `git rev-list` based on whether\n  this is a rebase is preserving merges or not.\n\n- At the end, just take the current head for the new branch ref instead\n  of trying to look up what the OLDHEAD was rewritten to. This fails\n  because the OLDHEAD was squashed/moved up earlier in the timeline, so\n  even if we can find its rewritten HEAD, that is no longer what we\n  ended up at.\n\nSigned-off-by: Stephen Haberman <stephen@exigencecorp.com>\n---\n\nI agree with Avi on what the rebase -i -p behavior should be for his\nscenario. This patch makes it so. However, the bane of my existence,\nt3404 is failing ~12 tests in, which is a real PITA to debug, so please\nlet me know if this is a worthwhile tangent to continue on.\n\n(That last change of dropping the OLDHEAD->NEWHEAD guessing is probably\nwhat is causing t3404 to fail, but I can't reason why it'd need to do\nthat rather than just use HEAD.)\n\nI've read in the archives about the git-sequencer stuff, which sounds\ncool, my thought is that, if anything, this will clarify git rebase -i -p\nbehavior and add tests that can later be ensured to still pass when\ngit-sequencer is dropped in.\n\n git-rebase--interactive.sh               |   53 ++++++++++---------\n t/t3411-rebase-preserve-around-merges.sh |   83 ++++++++++++++++++++++++++++++\n 2 files changed, 111 insertions(+), 25 deletions(-)\n create mode 100644 t/t3411-rebase-preserve-around-merges.sh\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex edb6ec6..9914111 100755\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -159,13 +159,18 @@ pick_one_preserving_merges () {\n \n \tif test -f \"$DOTEST\"/current-commit\n \tthen\n-\t\tcurrent_commit=$(cat \"$DOTEST\"/current-commit) &&\n-\t\tgit rev-parse HEAD > \"$REWRITTEN\"/$current_commit &&\n-\t\trm \"$DOTEST\"/current-commit ||\n-\t\tdie \"Cannot write current commit's replacement sha1\"\n+\t\tif [ \"$fast_forward\" == \"t\" ]\n+\t\tthen\n+\t\t\tcat \"$DOTEST\"/current-commit | while read current_commit\n+\t\t\tdo\n+\t\t\t\tgit rev-parse HEAD > \"$REWRITTEN\"/$current_commit\n+\t\t\tdone\n+\t\t\trm \"$DOTEST\"/current-commit ||\n+\t\t\tdie \"Cannot write current commit's replacement sha1\"\n+\t\tfi\n \tfi\n \n-\techo $sha1 > \"$DOTEST\"/current-commit\n+\techo $sha1 >> \"$DOTEST\"/current-commit\n \n \t# rewrite parents; if none were rewritten, we can fast-forward.\n \tnew_parents=\n@@ -193,15 +198,19 @@ pick_one_preserving_merges () {\n \t\t\tdie \"Cannot fast forward to $sha1\"\n \t\t;;\n \tf)\n-\t\ttest \"a$1\" = a-n && die \"Refusing to squash a merge: $sha1\"\n-\n \t\tfirst_parent=$(expr \"$new_parents\" : ' \\([^ ]*\\)')\n-\t\t# detach HEAD to current parent\n-\t\toutput git checkout $first_parent 2> /dev/null ||\n-\t\t\tdie \"Cannot move HEAD to $first_parent\"\n+\n+\t\tif [ \"$1\" != \"-n\" ]\n+\t\tthen\n+\t\t\t# detach HEAD to current parent\n+\t\t\toutput git checkout $first_parent 2> /dev/null ||\n+\t\t\t\tdie \"Cannot move HEAD to $first_parent\"\n+\t\tfi\n \n \t\tcase \"$new_parents\" in\n \t\t' '*' '*)\n+\t\t\ttest \"a$1\" = a-n && die \"Refusing to squash a merge: $sha1\"\n+\n \t\t\t# redo merge\n \t\t\tauthor_script=$(get_author_ident_from_commit $sha1)\n \t\t\teval \"$author_script\"\n@@ -350,20 +359,7 @@ do_next () {\n \tHEADNAME=$(cat \"$DOTEST\"/head-name) &&\n \tOLDHEAD=$(cat \"$DOTEST\"/head) &&\n \tSHORTONTO=$(git rev-parse --short $(cat \"$DOTEST\"/onto)) &&\n-\tif test -d \"$REWRITTEN\"\n-\tthen\n-\t\ttest -f \"$DOTEST\"/current-commit &&\n-\t\t\tcurrent_commit=$(cat \"$DOTEST\"/current-commit) &&\n-\t\t\tgit rev-parse HEAD > \"$REWRITTEN\"/$current_commit\n-\t\tif test -f \"$REWRITTEN\"/$OLDHEAD\n-\t\tthen\n-\t\t\tNEWHEAD=$(cat \"$REWRITTEN\"/$OLDHEAD)\n-\t\telse\n-\t\t\tNEWHEAD=$OLDHEAD\n-\t\tfi\n-\telse\n-\t\tNEWHEAD=$(git rev-parse HEAD)\n-\tfi &&\n+\tNEWHEAD=$(git rev-parse HEAD) &&\n \tcase $HEADNAME in\n \trefs/*)\n \t\tmessage=\"$GIT_REFLOG_ACTION: $HEADNAME onto $SHORTONTO)\" &&\n@@ -561,11 +557,18 @@ first and then run 'git rebase --continue' again.\"\n \t\t\tMERGES_OPTION=--no-merges\n \t\tfi\n \n+\t\tif test t = \"$PRESERVE_MERGES\"\n+\t\tthen\n+\t\t\tfirst_parent=\"--first-parent\"\n+\t\telse\n+\t\t\tfirst_parent=\"\"\n+\t\tfi\n+\n \t\tSHORTUPSTREAM=$(git rev-parse --short $UPSTREAM)\n \t\tSHORTHEAD=$(git rev-parse --short $HEAD)\n \t\tSHORTONTO=$(git rev-parse --short $ONTO)\n \t\tgit rev-list $MERGES_OPTION --pretty=oneline --abbrev-commit \\\n-\t\t\t--abbrev=7 --reverse --left-right --cherry-pick \\\n+\t\t\t--abbrev=7 --reverse --left-right --cherry-pick $first_parent \\\n \t\t\t$UPSTREAM...$HEAD | \\\n \t\t\tsed -n \"s/^>/pick /p\" > \"$TODO\"\n \t\tcat >> \"$TODO\" << EOF\ndiff --git a/t/t3411-rebase-preserve-around-merges.sh b/t/t3411-rebase-preserve-around-merges.sh\nnew file mode 100644\nindex 0000000..b130f5f\n--- /dev/null\n+++ b/t/t3411-rebase-preserve-around-merges.sh\n@@ -0,0 +1,83 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2008 Stephen Haberman\n+#\n+\n+test_description='git rebase preserve merges\n+\n+This test runs git rebase with and tries to squash a commit from after a merge\n+to before the merge.\n+'\n+. ./test-lib.sh\n+\n+# Copy/paste from t3404-rebase-interactive.sh\n+echo \"#!$SHELL_PATH\" >fake-editor.sh\n+cat >> fake-editor.sh <<\\EOF\n+case \"$1\" in\n+*/COMMIT_EDITMSG)\n+\ttest -z \"$FAKE_COMMIT_MESSAGE\" || echo \"$FAKE_COMMIT_MESSAGE\" > \"$1\"\n+\ttest -z \"$FAKE_COMMIT_AMEND\" || echo \"$FAKE_COMMIT_AMEND\" >> \"$1\"\n+\texit\n+\t;;\n+esac\n+test -z \"$EXPECT_COUNT\" ||\n+\ttest \"$EXPECT_COUNT\" = $(sed -e '/^#/d' -e '/^$/d' < \"$1\" | wc -l) ||\n+\texit\n+test -z \"$FAKE_LINES\" && exit\n+grep -v '^#' < \"$1\" > \"$1\".tmp\n+rm -f \"$1\"\n+cat \"$1\".tmp\n+action=pick\n+for line in $FAKE_LINES; do\n+\tcase $line in\n+\tsquash|edit)\n+\t\taction=\"$line\";;\n+\t*)\n+\t\techo sed -n \"${line}s/^pick/$action/p\"\n+\t\tsed -n \"${line}p\" < \"$1\".tmp\n+\t\tsed -n \"${line}s/^pick/$action/p\" < \"$1\".tmp >> \"$1\"\n+\t\taction=pick;;\n+\tesac\n+done\n+EOF\n+\n+test_set_editor \"$(pwd)/fake-editor.sh\"\n+chmod a+x fake-editor.sh\n+\n+# set up two branches like this:\n+#\n+# A - B - D - E - F\n+#      \\     /\n+#       - C -\n+\n+test_expect_success 'setup' '\n+\ttouch a &&\n+\ttouch b &&\n+\tgit add a &&\n+\tgit commit -m A &&\n+\tgit add b &&\n+\tgit commit -m B &&\n+\tgit tag B &&\n+\tgit checkout -b branch &&\n+\ttouch c &&\n+\tgit add c &&\n+\tgit commit -m C &&\n+\tgit checkout master &&\n+\ttouch d &&\n+\tgit add d &&\n+\tgit commit -m D &&\n+\tgit merge branch &&\n+\ttouch f &&\n+\tgit add f &&\n+\tgit commit -m F &&\n+\tgit tag F\n+'\n+\n+test_expect_success 'squash F into D' '\n+\tFAKE_LINES=\"1 squash 3 2\" git rebase -i -p B &&\n+\ttest \"$(git rev-parse HEAD^2)\" = \"$(git rev-parse branch)\" &&\n+\ttest \"$(git rev-parse HEAD~2)\" = \"$(git rev-parse B)\"\n+'\n+\n+test_done\n+\n-- \n1.6.0.2\n"},{"id":"92477","messageId":"20081006212021.04ba9214.stephen@exigencecorp.com","threadId":"15788","inReplyTo":"20081006102118.3e817a0f.stephen@exigencecorp.com","subject":"Re: [PATCH RFC] rebase--interactive: if preserving merges, use first-parent to limit what is shown.","fromName":"Stephen Haberman","fromEmail":"stephen@exigencecorp.com","sentAt":"2008-10-07T02:20:21Z","receivedAt":"2008-10-07T02:20:21Z","isPatch":true,"sender":{"key":"stephen@exigencecorp.com","avatar":"https://gravatar.com/avatar/23b93ad70a06ce53505f17ddba65176edbcfb6588e7a4c1a2dca04aaf0a6aff1?d=mp&s=160"},"body":"\n> I agree with Avi on what the rebase -i -p behavior should be for his\n> scenario. This patch makes it so. However, the bane of my existence,\n> t3404 is failing ~12 tests in, which is a real PITA to debug, so\n> please let me know if this is a worthwhile tangent to continue on.\n\nAh, good old t3404--it caught me on an aspect I had considered but\nwanted to avoid--Avi's (and my) preferred \"--first-parent\" way of\nlisting merges works great if the right hand side of the merge commits\nare outside of the branch getting rebased.\n\nE.g. my use case is when I merge in a stable release with ~100 commits\nor so and could potentially want to move it around, perhaps squash\naround it as Avi pointed it, I don't want all 100 commits that are in\nthe stable branch to be listed in my todo file.\n\nHowever, t3404 makes a good point that if the right hand of the merge\nhas parents that are going to get rebased, the right hand side does\nneed to be included/shown/rewritten.\n\nI also went and looked at the git-sequencer rewrite of rebase-i and it\nlooks slick. I don't fully understand it yet, but I'm much more\ninclined now to just let Stephan (& sponors/list) ably handle the\nproblem. Especially since its moving to builtin, it moves the required\ntechnical ability to contribute above my current skillset--perhaps that\nis the intent. :-)\n\nSo, unless I think of something else, I'm done hacking on this and\nam withdrawing the patch.\n\nThough I am curious--with the sequencer, is the Avi/my request of not\nlisting out-of-band commits in the todo file going to be handled?\n\nSome sort of \"--first-parent-unless-included-in-rebase\" flag.\n\nThanks,\nStephen\n"},{"id":"92496","messageId":"20081007013654.274e5cf6.stephen@exigencecorp.com","threadId":"15788","inReplyTo":"20081006212021.04ba9214.stephen@exigencecorp.com","subject":"Re: [PATCH RFC] rebase--interactive: if preserving merges, use first-parent to limit what is shown.","fromName":"Stephen Haberman","fromEmail":"stephen@exigencecorp.com","sentAt":"2008-10-07T06:36:54Z","receivedAt":"2008-10-07T06:36:54Z","isPatch":true,"sender":{"key":"stephen@exigencecorp.com","avatar":"https://gravatar.com/avatar/23b93ad70a06ce53505f17ddba65176edbcfb6588e7a4c1a2dca04aaf0a6aff1?d=mp&s=160"},"body":"\n> So, unless I think of something else, I'm done hacking on this and am\n> withdrawing the patch.\n>\n> Though I am curious--with the sequencer, is the Avi/my request of not\n> listing out-of-band commits in the todo file going to be handled?\n> \n> Some sort of \"--first-parent-unless-included-in-rebase\" flag.\n\nOkay, I lied--I have a patch that implements this behavior, passes Avi's\nscript-turned-test, and passes t3404. It implements the above algorithm\nof, if in preserving merges mode, start with only first parents in the\ntodo list, and then recursively prepend right hand side commits to the\ntodo list only if their parents are going to be rewritten. This drops a\nlot of cruft from rebase-i-p with large merges and is very cool, IMHO.\n\nHowever, lest I burn my \"PATCH v2\" opportunity, I'm holding off on\nposting the updated patch. It works and passes tests but I'm sure I'll\ntinker with it some more over the next few days. It will also likely\nconflict with my pu sh/maint-rebase3 patch, so I don't know whether to\nbase it on top of that one or not (guessing not).\n\nAlso, I think the patch itself is less interesting than the discussion\nof whether this \"first parent only\" behavior is desired or not.\n\nObviously I think so--do others agree/disagree?\n\nI've read more into the sequencer, and from what I can tell it still\njust drives off a todo of pick/etc. input, and does not generate the\ntodo itself. So I think my patch is still fair game in terms of how to\ngenerate either the current or the next generation rebase-i-p todo list.\n\nI could be wrong on that though.\n\nThanks,\nStephen\n"},{"id":"92506","messageId":"48EB32A4.80809@redhat.com","threadId":"15788","inReplyTo":"20081006212021.04ba9214.stephen@exigencecorp.com","subject":"Re: [PATCH RFC] rebase--interactive: if preserving merges, use first-parent to limit what is shown.","fromName":"Avi Kivity","fromEmail":"avi@redhat.com","sentAt":"2008-10-07T09:57:56Z","receivedAt":"2008-10-07T09:57:56Z","isPatch":true,"sender":{"key":"avi@redhat.com","avatar":null},"body":"Stephen Haberman wrote:\n> However, t3404 makes a good point that if the right hand of the merge\n> has parents that are going to get rebased, the right hand side does\n> need to be included/shown/rewritten.\n>\n>   \n\nBut, won't those commits get linearized?  Won't git rebase pick the \ncommits into the left-hand side of the merge instead of into the right \nhand side?\n\nIf git rebase is to handle nonlinear history, it needs much more \nexpressive commands; not only saying which commit to pick, but also what \nthe commit's parents shall be.\n\n-- \nerror compiling committee.c: too many arguments to function\n"},{"id":"92519","messageId":"20081007120700.GC7209@leksak.fem-net","threadId":"15788","inReplyTo":"48EB32A4.80809@redhat.com","subject":"Re: [PATCH RFC] rebase--interactive: if preserving merges, use first-parent to limit what is shown.","fromName":"Stephan Beyer","fromEmail":"s-beyer@gmx.net","sentAt":"2008-10-07T12:07:00Z","receivedAt":"2008-10-07T12:07:00Z","isPatch":true,"sender":{"key":"s-beyer@gmx.net","avatar":"https://avatars.githubusercontent.com/u/143889?v=4"},"body":"Hi,\n\nAvi Kivity wrote:\n> If git rebase is to handle nonlinear history, it needs much more\n> expressive commands; not only saying which commit to pick, but also what  \n> the commit's parents shall be.\n\ngit-sequencer has a \"merge\" command for that. I'm really sorry that this has\nnot been sent to the list yet. Nevertheless I'm always glad to find testers\nfor sequencer, so if you like, fetch\n\tgit://repo.or.cz/git/sbeyer.git seq-builtin-dev\n\nRegards,\n  Stephan\n\n-- \nStephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F\n"},{"id":"92521","messageId":"48EB5482.7050207@redhat.com","threadId":"15788","inReplyTo":"20081007120700.GC7209@leksak.fem-net","subject":"Re: [PATCH RFC] rebase--interactive: if preserving merges, use first-parent to limit what is shown.","fromName":"Avi Kivity","fromEmail":"avi@redhat.com","sentAt":"2008-10-07T12:22:26Z","receivedAt":"2008-10-07T12:22:26Z","isPatch":true,"sender":{"key":"avi@redhat.com","avatar":null},"body":"Stephan Beyer wrote:\n> Hi,\n>\n> Avi Kivity wrote:\n>   \n>> If git rebase is to handle nonlinear history, it needs much more\n>> expressive commands; not only saying which commit to pick, but also what  \n>> the commit's parents shall be.\n>>     \n>\n> git-sequencer has a \"merge\" command for that. I'm really sorry that this has\n> not been sent to the list yet. Nevertheless I'm always glad to find testers\n> for sequencer, so if you like, fetch\n> \tgit://repo.or.cz/git/sbeyer.git seq-builtin-dev\n>\n>   \n\nBut this isn't a merge; it's more of a 'pick into this branch' instead.\n\nMaybe 'merge' can do this, but we also need to populate the todo with \nthe required information (otherwise, git rebase -i without changes to \nthe todo file will not be a no-op).\n\n\n-- \nerror compiling committee.c: too many arguments to function\n"},{"id":"92529","messageId":"20081007143802.GI8203@spearce.org","threadId":"15788","inReplyTo":"20081007013654.274e5cf6.stephen@exigencecorp.com","subject":"Re: [PATCH RFC] rebase--interactive: if preserving merges, use first-parent to limit what is shown.","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-10-07T14:38:02Z","receivedAt":"2008-10-07T14:38:02Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Stephen Haberman <stephen@exigencecorp.com> wrote:\n> However, lest I burn my \"PATCH v2\" opportunity, I'm holding off on\n> posting the updated patch. It works and passes tests but I'm sure I'll\n> tinker with it some more over the next few days. It will also likely\n> conflict with my pu sh/maint-rebase3 patch, so I don't know whether to\n> base it on top of that one or not (guessing not).\n\nWhen a patch series is in `pu` it can be rebased/replaced/amended\nat any time.  That's why I parked it there.  The pu branch rewinds\nand is rebuilt on a daily basis.  Any commits not yet merged into\nmaint, master or next are automatically rebased onto the latest\nmaint or master branch and get merged into that day's pu.\n\nSo don't hold back on posting patches.  Folks expect to see\npatches on this list; talking is less productive than posting code.\nShowing code that purports to solve a problem, or that at least\ndisplays a problem concretely is worthy of discussion.\n\nAnd don't worry about replacing what is currently in pu.  Its easily\ndone by the maintainer.  However don't expect daily updates to a\ntopic in pu.  Junio (and I) just don't have the bandwidth to keep\nreplacing patches every day.\n \n-- \nShawn.\n"}]}