{"thread":{"id":"27369","subject":"[BUG] rebase -p loses commits","startedAt":"2011-05-16T10:33:54Z","lastAt":"2011-06-18T22:13:05Z","messageCount":28,"participants":["Jeff King","Andrew Wong","Junio C Hamano","Johannes Sixt","Stephen Haberman"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"167980","messageId":"20110516103354.GA23564@sigill.intra.peff.net","threadId":"27369","inReplyTo":null,"subject":"[BUG] rebase -p loses commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-16T10:33:54Z","receivedAt":"2011-05-16T10:33:54Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I was trying to reproduce somebody's issue with a minimal test case, and\nI ran across this setup wherein \"rebase -p\" silently drops some commits:\n\n commit() {\n echo $1 >file && git add file && git commit -m $1\n }\n\n # repo with two branches, each with conflicting content\n git init repo && cd repo &&\n commit base &&\n commit master &&\n git checkout -b feature HEAD^ &&\n commit feature &&\n\n # now merge them, with some fake resolution\n ! git merge master &&\n commit resolved &&\n\n # now try to \"rebase -p\" on top of master.\n git rebase -p master\n\nThe rebase completes successfully, but the \"feature\" commit and the\nmerge resolution are gone!\n\nI'm totally unfamiliar with the preserve-merges code, and I won't have\ntime to dig further until later today or tomorrow, so I thought I'd\nthrow it out here and see if anybody has any clues.\n\n-Peff\n"},{"id":"168008","messageId":"4DD17E30.6030607@sohovfx.com","threadId":"27369","inReplyTo":"20110516103354.GA23564@sigill.intra.peff.net","subject":"Re: [BUG] rebase -p loses commits","fromName":"Andrew Wong","fromEmail":"andrew.w@sohovfx.com","sentAt":"2011-05-16T19:42:40Z","receivedAt":"2011-05-16T19:42:40Z","isPatch":false,"sender":{"key":"andrew.w@sohovfx.com","avatar":null},"body":"On 05/16/2011 06:33 AM, Jeff King wrote:\n> I was trying to reproduce somebody's issue with a minimal test case, and\n> I ran across this setup wherein \"rebase -p\" silently drops some commits:\n> \n\nThis particular patch seems to have something to do with the bug:\n d80d6bc146232d81f1bb4bc58e5d89263fd228d4\n http://thread.gmane.org/gmane.comp.version-control.git/98247/focus=98251\n\nHowever, I can't figure out what the \"odd boundary case\" that this patch\nwas supposed to fix is. Anyone have any idea?\n\nAndrew\n"},{"id":"168014","messageId":"7vfwoel6vw.fsf@alter.siamese.dyndns.org","threadId":"27369","inReplyTo":"20110516103354.GA23564@sigill.intra.peff.net","subject":"Re: [BUG] rebase -p loses commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-16T20:36:51Z","receivedAt":"2011-05-16T20:36:51Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I was trying to reproduce somebody's issue with a minimal test case, and\n> I ran across this setup wherein \"rebase -p\" silently drops some commits:\n>\n> commit() {\n> echo $1 >file && git add file && git commit -m $1\n> }\n>\n> # repo with two branches, each with conflicting content\n> git init repo && cd repo &&\n> commit base &&\n> commit master &&\n> git checkout -b feature HEAD^ &&\n> commit feature &&\n>\n> # now merge them, with some fake resolution\n> ! git merge master &&\n> commit resolved &&\n>\n> # now try to \"rebase -p\" on top of master.\n> git rebase -p master\n\nHmm, I am confused. You have this:\n\n F---* feature\n / /\n B---M master\n\nand you are at \"*\". If it were to rebase to linearize,\n\n B---M---F' \n\nwith F' that has the same the contents as '*', possibly autoresolved by\n\"am -3\" and/or \"rerere\", should be what you would get.\n\nBut what does it mean to rebase that on top of master, preserving merges\nin the first place? You are already on top of 'master' and '*' itself\nshould be what you should get, no? IOW, shouldn't you already be\nup-to-date?\n\n> I'm totally unfamiliar with the preserve-merges code, and I won't have\n> time to dig further until later today or tomorrow, so I thought I'd\n> throw it out here and see if anybody has any clues.\n\nI don't use preserve-merge rebase either, but at least when you are\nstrictly ahead of the target, nothing should happen, I think.\n\nPerhaps this should be a good start?\n\n git-rebase.sh | 11 +++++++----\n 1 files changed, 7 insertions(+), 4 deletions(-)\n\ndiff --git a/git-rebase.sh b/git-rebase.sh\nindex 7a54bfc..2be10d6 100755\n--- a/git-rebase.sh\n+++ b/git-rebase.sh\n@@ -466,10 +466,13 @@ require_clean_work_tree \"rebase\" \"Please commit or stash them.\"\n # but this should be done only when upstream and onto are the same\n # and if this is not an interactive rebase.\n mb=$(git merge-base \"$onto\" \"$orig_head\")\n-if test \"$type\" != interactive && test \"$upstream\" = \"$onto\" &&\n-\ttest \"$mb\" = \"$onto\" &&\n-\t# linear history?\n-\t! (git rev-list --parents \"$onto\"..\"$orig_head\" | sane_grep \" .* \") > /dev/null\n+if test \"$upstream\" = \"$onto\" && test \"$mb\" = \"$onto\" && {\n+ {\n+ test \"$type\" != interactive &&\n+ # linear history?\n+ ! (git rev-list --parents \"$onto\"..\"$orig_head\" | sane_grep \" .* \") > /dev/null\n+ } || test t,implied = \"$preserve_merges,$interactive_rebase\"\n+ }\n then\n \tif test -z \"$force_rebase\"\n \tthen\n"},{"id":"168033","messageId":"4DD1C277.9070605@sohovfx.com","threadId":"27369","inReplyTo":"7vfwoel6vw.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG] rebase -p loses commits","fromName":"Andrew Wong","fromEmail":"andrew.w@sohovfx.com","sentAt":"2011-05-17T00:33:59Z","receivedAt":"2011-05-17T00:33:59Z","isPatch":false,"sender":{"key":"andrew.w@sohovfx.com","avatar":null},"body":"On 05/16/2011 04:36 PM, Junio C Hamano wrote:\n> F---* feature\n> / /\n> B---M master\n>\n> But what does it mean to rebase that on top of master, preserving merges\n> in the first place? You are already on top of 'master' and '*' itself\n> should be what you should get, no? IOW, shouldn't you already be\n> up-to-date?\n> \nSince preserve-merge uses the interactive-rebase, I think \ninteractive-rebase should still pick the merge commit, which will then \nbe consistent with what's happening if we rebase onto \"F\". So, without \nknowing whether \"F\" or \"M\" is the first-parent, I think \ninteractive-rebase onto \"F\" and onto \"M\" should have the same effect. \ni.e. interactive-rebase picks the merge commit\n"},{"id":"168035","messageId":"7vpqnii1sx.fsf@alter.siamese.dyndns.org","threadId":"27369","inReplyTo":"4DD1C277.9070605@sohovfx.com","subject":"Re: [BUG] rebase -p loses commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-17T00:54:54Z","receivedAt":"2011-05-17T00:54:54Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andrew Wong writes:\n\n> On 05/16/2011 04:36 PM, Junio C Hamano wrote:\n>> F---* feature\n>> / /\n>> B---M master\n>>\n>> But what does it mean to rebase that on top of master, preserving merges\n>> in the first place? You are already on top of 'master' and '*' itself\n>> should be what you should get, no? IOW, shouldn't you already be\n>> up-to-date?\n>> \n> Since preserve-merge uses the interactive-rebase, I think\n> interactive-rebase should still pick the merge commit, which will then\n> be consistent with what's happening if we rebase onto \"F\". So, without\n> knowing whether \"F\" or \"M\" is the first-parent, I think\n> interactive-rebase onto \"F\" and onto \"M\" should have the same\n> effect. i.e. interactive-rebase picks the merge commit\n\nI agree that changing the behaviour based on the \"first-ness\" of the\nparent is a wrong thing to do in general. But even then, I have a more\nfundamental question.\n\nWhat does rebasing that '*' on top of M really mean?\n\nDoes it mean this?\n\n F---* F'--*'\n / / ---> / /\n B---M B---M---/\n\nIn other words:\n\n\tTake all the commits reachable from '*', exclude the ones that are\n\tancestors of M, and make sure that commit that corresponds to each\n\tof the remaining commits (in this case, F' and *' correspond to F\n\tand * respectively) are decendants of M, and reconstruct the graph\n\tpreserving the parenthood relationship between corresponding\n\tcommits.\n\nI would understand why some project may want to require you to rebase to\nthe tip to keep a linear history, and the above sentence would be the\nright specification for linearizing rebase except for the \"and reconstruct\nthe graph\" part.\n\nBut the above \"preserving\" rewrite does not even preserve the topology of\nthe graph (the original * is a true merge between two forks, but *' is\nnot) to begin with. Also, if you want to _usefully_ place F' on top of M,\nsuch a rewrite should resolve possible conflicts that was resolved at * in\nthe original graph at F' anyway, which would mean that the resulting *'\nshould become a totally empty commit.\n\nWhy would anybody want to do such a thing to begin with?\n"},{"id":"168036","messageId":"7vk4dqi1fr.fsf@alter.siamese.dyndns.org","threadId":"27369","inReplyTo":"7vpqnii1sx.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG] rebase -p loses commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-17T01:02:48Z","receivedAt":"2011-05-17T01:02:48Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> But the above \"preserving\" rewrite does not even preserve the topology of\n> the graph (the original * is a true merge between two forks, but *' is\n> not) to begin with. Also, if you want to _usefully_ place F' on top of M,\n> such a rewrite should resolve possible conflicts that was resolved at * in\n> the original graph at F' anyway, which would mean that the resulting *'\n> should become a totally empty commit.\n>\n> Why would anybody want to do such a thing to begin with?\n\nNote that I am not saying \"rebase -p\" is not useful in general. If you\nhad\n\n x---x---x---W---X\n / \\ \\\n ---M Y---Z\n\nit is entirely sensible to want to have this history to exclude 'x'\n\n x---x---x---W---X\n / \\ \\\n ---M---W'--X' Y---Z\n \\ \\\n Y'--Z'\n\nI think the patch I posted earlier should stop the problematic case Jeff\nmentioned from happening, but I am trying to see if it makes sense to stop\nwithout doing anything even when it is forced when onto and merge-base are\nthe same commit (which is not true for this \"sensible\" case).\n"},{"id":"168044","messageId":"20110517053934.GB10048@sigill.intra.peff.net","threadId":"27369","inReplyTo":"7vfwoel6vw.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG] rebase -p loses commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-17T05:39:34Z","receivedAt":"2011-05-17T05:39:34Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 16, 2011 at 01:36:51PM -0700, Junio C Hamano wrote:\n\n> Hmm, I am confused. You have this:\n> \n> F---* feature\n> / /\n> B---M master\n> \n> and you are at \"*\". If it were to rebase to linearize,\n> \n> B---M---F' \n> \n> with F' that has the same the contents as '*', possibly autoresolved by\n> \"am -3\" and/or \"rerere\", should be what you would get.\n> \n> But what does it mean to rebase that on top of master, preserving merges\n> in the first place? You are already on top of 'master' and '*' itself\n> should be what you should get, no? IOW, shouldn't you already be\n> up-to-date?\n\nTo be honest, I am not sure what should happen. How this example came\nabout was that somebody had the same graph, except that a third branch,\n\"origin\", also pointed at \"B\". They were confused why \"git rebase\n--pull\" made them re-resolve the same conflict that they had\nalready handled during the merge.\n\nThe answer, of course, is that rebase is linearizing the commits instead\nof trying to preserve the shape of history, and that they really wanted\n\"rebase -p\".\n\nI constructed the simplified example to show that the issue didn't have\nto do with origin, but rather with linearizing.\n\nSo it's not a real-world example, in that sense. If it had done one of:\n\n 1. Said \"you are up to date\" and one nothing.\n\n 2. Put F' on top of M.\n\n 3. Bailed and said \"what you're doing is silly\".\n\nI would probably have shrugged and left it alone. But claiming success\nand losing F entirely is pretty bad.\n\n> I don't use preserve-merge rebase either, but at least when you are\n> strictly ahead of the target, nothing should happen, I think.\n> \n> Perhaps this should be a good start?\n\nAside from how unreadable that shell conditional is getting, I think\nit's an improvement.\n\n-Peff\n"},{"id":"168045","messageId":"20110517054432.GC10048@sigill.intra.peff.net","threadId":"27369","inReplyTo":"4DD1C277.9070605@sohovfx.com","subject":"Re: [BUG] rebase -p loses commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-17T05:44:32Z","receivedAt":"2011-05-17T05:44:32Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 16, 2011 at 08:33:59PM -0400, Andrew Wong wrote:\n\n> On 05/16/2011 04:36 PM, Junio C Hamano wrote:\n> > F---* feature\n> > / /\n> > B---M master\n> >\n> >But what does it mean to rebase that on top of master, preserving merges\n> >in the first place? You are already on top of 'master' and '*' itself\n> >should be what you should get, no? IOW, shouldn't you already be\n> >up-to-date?\n> Since preserve-merge uses the interactive-rebase, I think\n> interactive-rebase should still pick the merge commit, which will\n> then be consistent with what's happening if we rebase onto \"F\". So,\n> without knowing whether \"F\" or \"M\" is the first-parent, I think\n> interactive-rebase onto \"F\" and onto \"M\" should have the same effect.\n> i.e. interactive-rebase picks the merge commit\n\nIs it really the first-parentness here that is important to the\nasymmetry? I thought it was more the fact that \"feature\" has the merge,\nbut \"master\" does not.\n\nTherefore, \"git rebase -p master feature\" knows that there is nothing to\ndo; we already contain all of \"master\".\n\nBut \"git rebase -p feature master\" sees that we have an extra commit in\n\"feature\" not in \"master\", and therefore we must attempt to rewrite on\ntop of that merge commit. Of course, when we try to do so, there are no\ncommits that are not already on \"feature\", so there is nothing to\nrewrite.\n\nSo the outcomes are the same, but the reasoning is different. And isn't\nthat what happens with Junio's patch (I tried a simple test and it\nseemed to be)?\n\n-Peff\n"},{"id":"168069","messageId":"4DD29D4A.8090703@sohovfx.com","threadId":"27369","inReplyTo":"20110517054432.GC10048@sigill.intra.peff.net","subject":"Re: [BUG] rebase -p loses commits","fromName":"Andrew Wong","fromEmail":"andrew.w@sohovfx.com","sentAt":"2011-05-17T16:07:38Z","receivedAt":"2011-05-17T16:07:38Z","isPatch":false,"sender":{"key":"andrew.w@sohovfx.com","avatar":null},"body":"On 05/17/2011 01:44 AM, Jeff King wrote:\n> Is it really the first-parentness here that is important to the\n> asymmetry? I thought it was more the fact that \"feature\" has the merge,\n> but \"master\" does not.\nTo know the fact that \"feature\" has the merge, don't we need to know\nthat \"feature\" is the first parent of the merge? For example, if \"F\" is\nthe head of \"feature\" and we're on \"*\" as a detached head, then we can\nonly say \"feature\" has the merge if we know \"feature\" is the first parent.\n> So the outcomes are the same, but the reasoning is different. And isn't\n> that what happens with Junio's patch (I tried a simple test and it\n> seemed to be)?\nI agree that the outcome of both should be the same. Junio's patch will\nfix the case when we do \"git rebase -p\", but the bug will still appear\nas soon as we do \"git rebase -p -i\", which I think is where the source\nof the problem is. So we should be looking to fix the issue with \"git\nrebase -p -i\", which will also fix \"git rebase -p\" too.\n\nI think it's pretty reasonable for \"rebase -p -i\" to pick the merge\ncommit, which is already happening with \"git rebase -p F\". A possible\nuse case for picking the merge commit is to do a fixup/squash/reword on\n\"G\" in the following graph:\n\n F---G---H\n / /\n B---M\n"},{"id":"168070","messageId":"20110517161234.GA21388@sigill.intra.peff.net","threadId":"27369","inReplyTo":"4DD29D4A.8090703@sohovfx.com","subject":"Re: [BUG] rebase -p loses commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-17T16:12:34Z","receivedAt":"2011-05-17T16:12:34Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 17, 2011 at 12:07:38PM -0400, Andrew Wong wrote:\n\n> > Is it really the first-parentness here that is important to the\n> > asymmetry? I thought it was more the fact that \"feature\" has the merge,\n> > but \"master\" does not.\n> To know the fact that \"feature\" has the merge, don't we need to know\n> that \"feature\" is the first parent of the merge? For example, if \"F\" is\n> the head of \"feature\" and we're on \"*\" as a detached head, then we can\n> only say \"feature\" has the merge if we know \"feature\" is the first parent.\n\nNo, if \"F\" is the head of \"feature\", then it does _not_ have the merge.\nBut it's not the merge that is important, it is really the fact that\nif we are rebasing \"feature\" on \"master\", that \"master ^feature\" is\nempty. IOW, we are already a superset. In this example, though, the\nmerge is the thing that gives us that superset.\n\n> I agree that the outcome of both should be the same. Junio's patch will\n> fix the case when we do \"git rebase -p\", but the bug will still appear\n> as soon as we do \"git rebase -p -i\", which I think is where the source\n> of the problem is. So we should be looking to fix the issue with \"git\n> rebase -p -i\", which will also fix \"git rebase -p\" too.\n\nAh, I see. Yes, in that case, we should definitely be fixing \"git rebase\n-p -i\".\n\n-Peff\n"},{"id":"168348","messageId":"1305957078-19111-1-git-send-email-andrew.kw.w@gmail.com","threadId":"27369","inReplyTo":"20110517161234.GA21388@sigill.intra.peff.net","subject":"[RFC] Interactive-rebase doesn't pick all children of \"upstream\"","fromName":"Andrew Wong","fromEmail":"andrew.kw.w@gmail.com","sentAt":"2011-05-21T05:51:17Z","receivedAt":"2011-05-21T05:51:17Z","isPatch":false,"sender":{"key":"andrew.kw.w@gmail.com","avatar":"https://avatars.githubusercontent.com/u/489311?v=4"},"body":"This is a work-in-progress patch for this issue. Since this patch changes a\nfairly significant behavior in interactive-rebase, I want to try to get some\nfeedbacks before I go ahead and start writing some tests for this new behavior.\n\nAndrew Wong (1):\n Interactive-rebase doesn't pick all children of \"upstream\"\n\n git-rebase--interactive.sh | 7 +++++--\n t/t3404-rebase-interactive.sh | 2 +-\n t/t3411-rebase-preserve-around-merges.sh | 2 +-\n 3 files changed, 7 insertions(+), 4 deletions(-)\n\n-- \n1.7.5.2.316.gd7d8c.dirty\n"},{"id":"168349","messageId":"1305957078-19111-2-git-send-email-andrew.kw.w@gmail.com","threadId":"27369","inReplyTo":"1305957078-19111-1-git-send-email-andrew.kw.w@gmail.com","subject":"[RFC] Interactive-rebase doesn't pick all children of \"upstream\"","fromName":"Andrew Wong","fromEmail":"andrew.kw.w@gmail.com","sentAt":"2011-05-21T05:51:18Z","receivedAt":"2011-05-21T05:51:18Z","isPatch":false,"sender":{"key":"andrew.kw.w@gmail.com","avatar":"https://avatars.githubusercontent.com/u/489311?v=4"},"body":"Consider this graph:\n\n D---E (topic, HEAD)\n / /\n A---B---C (master)\n \\\n F (topic2)\n\nand the following three commands:\n 1. git rebase -i A\n 2. git rebase -i --onto F B\n 3. git rebase -i B\n\nCurrently, (1) and (2) will pick B, D, C, and E onto A and F,\nrespectively. However, (3) will only pick D and E onto B. This\nbehavior of (3) is inconsistent with (1) and (2).\n\nThis also creates a bug if we do:\n 4. git rebase -i C\n\nIn (4), E is never picked. And since interactive-rebase resets \"HEAD\" to\n\"onto\", E is lost after the interactive-rebase.\n\nThis patch fixes the inconsistency and bug by ensuring that all children\nof upstream are always picked.\n\nTwo of the tests contain a scenario like (3). Since the new behavior\nadded more commits for picking, these tests need to be updated to edit\nthe \"todo\" list properly.\n---\n git-rebase--interactive.sh | 7 +++++--\n t/t3404-rebase-interactive.sh | 2 +-\n t/t3411-rebase-preserve-around-merges.sh | 2 +-\n 3 files changed, 7 insertions(+), 4 deletions(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 41ba96a..b6d1e5b 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -711,7 +711,7 @@ then\n \t# parents to rewrite and skipping dropped commits would\n \t# prematurely end our probe\n \tmerges_option=\n-\tfirst_after_upstream=\"$(git rev-list --reverse --first-parent $upstream..$orig_head | head -n 1)\"\n+\tcommits_after_upstream=\"$(git rev-list --reverse --parents $upstream..$orig_head | sane_grep \" $upstream\" | cut -d' ' -s -f1)\"\n else\n \tmerges_option=\"--no-merges --cherry-pick\"\n fi\n@@ -744,7 +744,10 @@ do\n \t\t\tpreserve=t\n \t\t\tfor p in $(git rev-list --parents -1 $sha1 | cut -d' ' -s -f2-)\n \t\t\tdo\n-\t\t\t\tif test -f \"$rewritten\"/$p -a \\( $p != $onto -o $sha1 = $first_after_upstream \\)\n+\t\t\t\tif test -f \"$rewritten\"/$p && (\n+\t\t\t\t\ttest $p != $onto ||\n+\t\t\t\t\texpr \"$commits_after_upstream\" \":\" \".*$sha1.*\"\n+\t\t\t\t\t)\n \t\t\t\tthen\n \t\t\t\t\tpreserve=f\n \t\t\t\tfi\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 7d8147b..c3cddcd 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -295,7 +295,7 @@ test_expect_success 'preserve merges with -p' '\n '\n \n test_expect_success 'edit ancestor with -p' '\n-\tFAKE_LINES=\"1 edit 2 3 4\" git rebase -i -p HEAD~3 &&\n+\tFAKE_LINES=\"1 2 edit 3 4\" git rebase -i -p HEAD~3 &&\n \techo 2 > unrelated-file &&\n \ttest_tick &&\n \tgit commit -m L2-modified --amend unrelated-file &&\ndiff --git a/t/t3411-rebase-preserve-around-merges.sh b/t/t3411-rebase-preserve-around-merges.sh\nindex 14a23cd..ace8e54 100755\n--- a/t/t3411-rebase-preserve-around-merges.sh\n+++ b/t/t3411-rebase-preserve-around-merges.sh\n@@ -37,7 +37,7 @@ test_expect_success 'setup' '\n # -- C1 --\n #\n test_expect_success 'squash F1 into D1' '\n-\tFAKE_LINES=\"1 squash 3 2\" git rebase -i -p B1 &&\n+\tFAKE_LINES=\"1 squash 4 2 3\" git rebase -i -p B1 &&\n \ttest \"$(git rev-parse HEAD^2)\" = \"$(git rev-parse C1)\" &&\n \ttest \"$(git rev-parse HEAD~2)\" = \"$(git rev-parse B1)\" &&\n \tgit tag E2\n-- \n1.7.5.2.316.gd7d8c.dirty\n"},{"id":"168368","messageId":"4DD76AFD.8040504@sohovfx.com","threadId":"27369","inReplyTo":"1305957078-19111-2-git-send-email-andrew.kw.w@gmail.com","subject":"Re: [RFC] Interactive-rebase doesn't pick all children of \"upstream\"","fromName":"Andrew Wong","fromEmail":"andrew.w@sohovfx.com","sentAt":"2011-05-21T07:34:21Z","receivedAt":"2011-05-21T07:34:21Z","isPatch":false,"sender":{"key":"andrew.w@sohovfx.com","avatar":null},"body":"On 11-05-21 1:51 AM, Andrew Wong wrote:\n> \t\t\tpreserve=t\n> \t\t\tfor p in $(git rev-list --parents -1 $sha1 | cut -d' ' -s -f2-)\n> \t\t\tdo\n> -\t\t\t\tif test -f \"$rewritten\"/$p -a \\( $p != $onto -o $sha1 = $first_after_upstream \\)\n> +\t\t\t\tif test -f \"$rewritten\"/$p&& (\n> +\t\t\t\t\ttest $p != $onto ||\n> +\t\t\t\t\texpr \"$commits_after_upstream\" \":\" \".*$sha1.*\"\n> +\t\t\t\t\t)\n> \t\t\t\tthen\n> \t\t\t\t\tpreserve=f\n> \t\t\t\tfi\n\nActually, I think this change might be effectively the same as removing \nboth of the OR-conditions, which leaves only the \"test -f\" condition. \nThat means commits are never skipped.\n\nI guess this goes back to my first response to this issue. What kind of \ncommits are skipped by the changes introduced in commit \nd80d6bc146232d81f1bb4bc58e5d89263fd228d4 ?\n"},{"id":"169311","messageId":"1307251953-25116-1-git-send-email-andrew.kw.w@gmail.com","threadId":"27369","inReplyTo":"20110517161234.GA21388@sigill.intra.peff.net","subject":"[PATCH] Interactive-rebase doesn't pick all children of \"upstream\"","fromName":"Andrew Wong","fromEmail":"andrew.kw.w@gmail.com","sentAt":"2011-06-05T05:32:33Z","receivedAt":"2011-06-05T05:32:33Z","isPatch":true,"sender":{"key":"andrew.kw.w@gmail.com","avatar":"https://avatars.githubusercontent.com/u/489311?v=4"},"body":"Consider this graph:\n\n D---E (topic, HEAD)\n / /\n A---B---C (master)\n \\\n F (topic2)\n\nand the following three commands:\n 1. git rebase -i A\n 2. git rebase -i --onto F A\n 3. git rebase -i B\n\nCurrently, (1) and (2) will pick B, D, C, and E onto A and F,\nrespectively. However, (3) will only pick D and E onto B. This\nbehavior of (3) is inconsistent with (1) and (2), and we cannot modify C\nin the interactive-rebase.\n\nThe current behavior also creates a bug if we do:\n 4. git rebase -i C\n\nIn (4), E is never picked. And since interactive-rebase resets \"HEAD\" to\n\"onto\" before picking any commits, D and E are lost after the\ninteractive-rebase.\n\nThis patch fixes the inconsistency and bug by ensuring that all children\nof upstream are always picked. This essentially reverts the commit:\n d80d6bc146232d81f1bb4bc58e5d89263fd228d4\nCommits reachable from \"upstream\" should never be skipped under any\ncondition. Otherwise we lose the chance to modify them like (3), and\ncreate bug like (4).\n\nTwo of the tests contain a scenario like (3). Since the new behavior\nadded more commits for picking, these tests need to be updated to edit\nthe \"todo\" list properly. Also added test for scenario (4).\n---\n git-rebase--interactive.sh | 3 +--\n t/t3404-rebase-interactive.sh | 2 +-\n t/t3409-rebase-preserve-merges.sh | 28 +++++++++++++++++++++++++++-\n t/t3411-rebase-preserve-around-merges.sh | 2 +-\n 4 files changed, 30 insertions(+), 5 deletions(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 65690af..c6ba7c1 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -713,7 +713,6 @@ then\n \t# parents to rewrite and skipping dropped commits would\n \t# prematurely end our probe\n \tmerges_option=\n-\tfirst_after_upstream=\"$(git rev-list --reverse --first-parent $upstream..$orig_head | head -n 1)\"\n else\n \tmerges_option=\"--no-merges --cherry-pick\"\n fi\n@@ -746,7 +745,7 @@ do\n \t\t\tpreserve=t\n \t\t\tfor p in $(git rev-list --parents -1 $sha1 | cut -d' ' -s -f2-)\n \t\t\tdo\n-\t\t\t\tif test -f \"$rewritten\"/$p -a \\( $p != $onto -o $sha1 = $first_after_upstream \\)\n+\t\t\t\tif test -f \"$rewritten\"/$p\n \t\t\t\tthen\n \t\t\t\t\tpreserve=f\n \t\t\t\tfi\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 47c8371..8538813 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -295,7 +295,7 @@ test_expect_success 'preserve merges with -p' '\n '\n \n test_expect_success 'edit ancestor with -p' '\n-\tFAKE_LINES=\"1 edit 2 3 4\" git rebase -i -p HEAD~3 &&\n+\tFAKE_LINES=\"1 2 edit 3 4\" git rebase -i -p HEAD~3 &&\n \techo 2 > unrelated-file &&\n \ttest_tick &&\n \tgit commit -m L2-modified --amend unrelated-file &&\ndiff --git a/t/t3409-rebase-preserve-merges.sh b/t/t3409-rebase-preserve-merges.sh\nindex 08201e2..16d316d 100755\n--- a/t/t3409-rebase-preserve-merges.sh\n+++ b/t/t3409-rebase-preserve-merges.sh\n@@ -37,7 +37,15 @@ export GIT_AUTHOR_EMAIL\n # \\\n # B2 <-- origin/topic\n #\n-# In all cases, 'topic' is rebased onto 'origin/topic'.\n+# Clone 4 ():\n+#\n+# A1--A2--B3 <-- origin/master\n+# \\\n+# B1--A3--M <-- topic\n+# \\ /\n+# \\--A4 <-- topic2\n+# \\\n+# B2 <-- origin/topic\n \n test_expect_success 'setup for merge-preserving rebase' \\\n \t'echo First > A &&\n@@ -57,6 +65,13 @@ test_expect_success 'setup for merge-preserving rebase' \\\n \tgit merge origin/master\n \t) &&\n \n+\tgit clone ./. clone4 &&\n+\t(\n+\t\tcd clone4 &&\n+\t\tgit checkout -b topic origin/topic &&\n+\t\tgit merge origin/master\n+\t) &&\n+\n \techo Fifth > B &&\n \tgit add B &&\n \tgit commit -m \"Add different B\" &&\n@@ -123,4 +138,15 @@ test_expect_success 'rebase -p preserves no-ff merges' '\n \t)\n '\n \n+test_expect_success '' '\n+\t(\n+\tcd clone4 &&\n+\tgit fetch &&\n+\tgit rebase -p HEAD^2 &&\n+\ttest 1 = $(git rev-list --all --pretty=oneline | grep \"Modify A\" | wc -l) &&\n+\ttest 1 = $(git rev-list --all --pretty=oneline | grep \"Modify B\" | wc -l) &&\n+\ttest 1 = $(git rev-list --all --pretty=oneline | grep \"Merge remote-tracking branch \" | wc -l)\n+\t)\n+'\n+\n test_done\ndiff --git a/t/t3411-rebase-preserve-around-merges.sh b/t/t3411-rebase-preserve-around-merges.sh\nindex 14a23cd..ace8e54 100755\n--- a/t/t3411-rebase-preserve-around-merges.sh\n+++ b/t/t3411-rebase-preserve-around-merges.sh\n@@ -37,7 +37,7 @@ test_expect_success 'setup' '\n # -- C1 --\n #\n test_expect_success 'squash F1 into D1' '\n-\tFAKE_LINES=\"1 squash 3 2\" git rebase -i -p B1 &&\n+\tFAKE_LINES=\"1 squash 4 2 3\" git rebase -i -p B1 &&\n \ttest \"$(git rev-parse HEAD^2)\" = \"$(git rev-parse C1)\" &&\n \ttest \"$(git rev-parse HEAD~2)\" = \"$(git rev-parse B1)\" &&\n \tgit tag E2\n-- \n1.7.6.rc0\n"},{"id":"169313","messageId":"4DEB495F.9080900@kdbg.org","threadId":"27369","inReplyTo":"1307251953-25116-1-git-send-email-andrew.kw.w@gmail.com","subject":"Re: [PATCH] Interactive-rebase doesn't pick all children of \"upstream\"","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2011-06-05T09:16:15Z","receivedAt":"2011-06-05T09:16:15Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 05.06.2011 07:32, schrieb Andrew Wong:\n> Consider this graph:\n> \n> D---E (topic, HEAD)\n> / /\n> A---B---C (master)\n> \\\n> F (topic2)\n> \n> and the following three commands:\n> 1. git rebase -i A\n> 2. git rebase -i --onto F A\n> 3. git rebase -i B\n> \n> Currently, (1) and (2) will pick B, D, C, and E onto A and F,\n> respectively. However, (3) will only pick D and E onto B. This\n> behavior of (3) is inconsistent with (1) and (2), and we cannot modify C\n> in the interactive-rebase.\n\nI cannot reproduce your claims:\n\n- (1) and (2) picks B,C,D top A and F, but not E because E is a merge.\n\n- (3) picks C and D, but not E because E is a merge.\n\n> The current behavior also creates a bug if we do:\n> 4. git rebase -i C\n> \n> In (4), E is never picked. And since interactive-rebase resets \"HEAD\" to\n> \"onto\" before picking any commits, D and E are lost after the\n> interactive-rebase.\n\n(4) picks only D, because E is a merge. I don't understand what you mean\nthat \"D and E are lost\"; E is not picked in the first place, but D is in\nthe todo-list; how can D be lost?\n\nBTW, rebase never picks merges by design. I don't see anything wrong so\nfar with the current behavior. Please explain!\n\n-- Hannes\n"},{"id":"169318","messageId":"4DEB8E7F.60705@gmail.com","threadId":"27369","inReplyTo":"4DEB495F.9080900@kdbg.org","subject":"Re: [PATCH] Interactive-rebase doesn't pick all children of \"upstream\"","fromName":"Andrew Wong","fromEmail":"andrew.kw.w@gmail.com","sentAt":"2011-06-05T14:11:11Z","receivedAt":"2011-06-05T14:11:11Z","isPatch":true,"sender":{"key":"andrew.kw.w@gmail.com","avatar":"https://avatars.githubusercontent.com/u/489311?v=4"},"body":"On 11-06-05 5:16 AM, Johannes Sixt wrote:\n> Am 05.06.2011 07:32, schrieb Andrew Wong:\n>> Currently, (1) and (2) will pick B, D, C, and E onto A and F,\n>> respectively. However, (3) will only pick D and E onto B. This\n>> behavior of (3) is inconsistent with (1) and (2), and we cannot modify C\n>> in the interactive-rebase.\n> I cannot reproduce your claims:\n>\n> - (1) and (2) picks B,C,D top A and F, but not E because E is a merge.\n>\n> - (3) picks C and D, but not E because E is a merge.\nAh, all those commands should have \"-p\" on them to preserve the merges. \nThanks for the catch!\n"},{"id":"169425","messageId":"1307419725-4470-1-git-send-email-andrew.kw.w@gmail.com","threadId":"27369","inReplyTo":"4DEB495F.9080900@kdbg.org","subject":"[PATCH v2] rebase -i -p: doesn't pick certain merge commits that are children of \"upstream\"","fromName":"Andrew Wong","fromEmail":"andrew.kw.w@gmail.com","sentAt":"2011-06-07T04:08:44Z","receivedAt":"2011-06-07T04:08:44Z","isPatch":true,"sender":{"key":"andrew.kw.w@gmail.com","avatar":"https://avatars.githubusercontent.com/u/489311?v=4"},"body":"This patch contains an updated commit message. The \"-p\" flags, which are vital\nto this issue, were completely missing from the previous message.\n\nAndrew Wong (1):\n rebase -i -p: doesn't pick certain merge commits that are children of\n \"upstream\"\n\n git-rebase--interactive.sh | 3 +--\n t/t3404-rebase-interactive.sh | 2 +-\n t/t3409-rebase-preserve-merges.sh | 28 +++++++++++++++++++++++++++-\n t/t3411-rebase-preserve-around-merges.sh | 2 +-\n 4 files changed, 30 insertions(+), 5 deletions(-)\n\n-- \n1.7.6.rc0.1.gf20d7\n"},{"id":"169426","messageId":"1307419725-4470-2-git-send-email-andrew.kw.w@gmail.com","threadId":"27369","inReplyTo":"1307419725-4470-1-git-send-email-andrew.kw.w@gmail.com","subject":"[PATCH] rebase -i -p: doesn't pick certain merge commits that are children of \"upstream\"","fromName":"Andrew Wong","fromEmail":"andrew.kw.w@gmail.com","sentAt":"2011-06-07T04:08:45Z","receivedAt":"2011-06-07T04:08:45Z","isPatch":true,"sender":{"key":"andrew.kw.w@gmail.com","avatar":"https://avatars.githubusercontent.com/u/489311?v=4"},"body":"Consider this graph:\n\n D---E (topic, HEAD)\n / /\n A---B---C (master)\n \\\n F (topic2)\n\nand the following three commands:\n 1. git rebase -i -p A\n 2. git rebase -i -p --onto F A\n 3. git rebase -i -p B\n\nCurrently, (1) and (2) will pick B, D, C, and E onto A and F,\nrespectively. However, (3) will only pick D and E onto B, but not C,\nwhich is inconsistent with (1) and (2). As a result, we cannot modify C\nduring the interactive-rebase.\n\nThe current behavior also creates a bug if we do:\n 4. git rebase -i -p C\n\nIn (4), E is never picked. And since interactive-rebase resets \"HEAD\"\nto \"onto\" before picking any commits, D and E are lost after the\ninteractive-rebase.\n\nThis patch fixes the inconsistency and bug by ensuring that all children\nof upstream are always picked. This essentially reverts the commit:\n d80d6bc146232d81f1bb4bc58e5d89263fd228d4\n\nWhen compiling the \"todo\" list, commits reachable from \"upstream\" should\nnever be skipped under any conditions. Otherwise, we lose the ability\nto modify them like (3), and create a bug like (4).\n\nTwo of the tests contain a scenario like (3). Since the new behavior\nadded more commits for picking, these tests need to be updated to edit\nthe \"todo\" list properly. A new test has also been added for (4).\n\nSigned-off-by: Andrew Wong \n---\n git-rebase--interactive.sh | 3 +--\n t/t3404-rebase-interactive.sh | 2 +-\n t/t3409-rebase-preserve-merges.sh | 28 +++++++++++++++++++++++++++-\n t/t3411-rebase-preserve-around-merges.sh | 2 +-\n 4 files changed, 30 insertions(+), 5 deletions(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 65690af..c6ba7c1 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -713,7 +713,6 @@ then\n \t# parents to rewrite and skipping dropped commits would\n \t# prematurely end our probe\n \tmerges_option=\n-\tfirst_after_upstream=\"$(git rev-list --reverse --first-parent $upstream..$orig_head | head -n 1)\"\n else\n \tmerges_option=\"--no-merges --cherry-pick\"\n fi\n@@ -746,7 +745,7 @@ do\n \t\t\tpreserve=t\n \t\t\tfor p in $(git rev-list --parents -1 $sha1 | cut -d' ' -s -f2-)\n \t\t\tdo\n-\t\t\t\tif test -f \"$rewritten\"/$p -a \\( $p != $onto -o $sha1 = $first_after_upstream \\)\n+\t\t\t\tif test -f \"$rewritten\"/$p\n \t\t\t\tthen\n \t\t\t\t\tpreserve=f\n \t\t\t\tfi\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 47c8371..8538813 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -295,7 +295,7 @@ test_expect_success 'preserve merges with -p' '\n '\n \n test_expect_success 'edit ancestor with -p' '\n-\tFAKE_LINES=\"1 edit 2 3 4\" git rebase -i -p HEAD~3 &&\n+\tFAKE_LINES=\"1 2 edit 3 4\" git rebase -i -p HEAD~3 &&\n \techo 2 > unrelated-file &&\n \ttest_tick &&\n \tgit commit -m L2-modified --amend unrelated-file &&\ndiff --git a/t/t3409-rebase-preserve-merges.sh b/t/t3409-rebase-preserve-merges.sh\nindex 08201e2..16d316d 100755\n--- a/t/t3409-rebase-preserve-merges.sh\n+++ b/t/t3409-rebase-preserve-merges.sh\n@@ -37,7 +37,15 @@ export GIT_AUTHOR_EMAIL\n # \\\n # B2 <-- origin/topic\n #\n-# In all cases, 'topic' is rebased onto 'origin/topic'.\n+# Clone 4 ():\n+#\n+# A1--A2--B3 <-- origin/master\n+# \\\n+# B1--A3--M <-- topic\n+# \\ /\n+# \\--A4 <-- topic2\n+# \\\n+# B2 <-- origin/topic\n \n test_expect_success 'setup for merge-preserving rebase' \\\n \t'echo First > A &&\n@@ -57,6 +65,13 @@ test_expect_success 'setup for merge-preserving rebase' \\\n \tgit merge origin/master\n \t) &&\n \n+\tgit clone ./. clone4 &&\n+\t(\n+\t\tcd clone4 &&\n+\t\tgit checkout -b topic origin/topic &&\n+\t\tgit merge origin/master\n+\t) &&\n+\n \techo Fifth > B &&\n \tgit add B &&\n \tgit commit -m \"Add different B\" &&\n@@ -123,4 +138,15 @@ test_expect_success 'rebase -p preserves no-ff merges' '\n \t)\n '\n \n+test_expect_success '' '\n+\t(\n+\tcd clone4 &&\n+\tgit fetch &&\n+\tgit rebase -p HEAD^2 &&\n+\ttest 1 = $(git rev-list --all --pretty=oneline | grep \"Modify A\" | wc -l) &&\n+\ttest 1 = $(git rev-list --all --pretty=oneline | grep \"Modify B\" | wc -l) &&\n+\ttest 1 = $(git rev-list --all --pretty=oneline | grep \"Merge remote-tracking branch \" | wc -l)\n+\t)\n+'\n+\n test_done\ndiff --git a/t/t3411-rebase-preserve-around-merges.sh b/t/t3411-rebase-preserve-around-merges.sh\nindex 14a23cd..ace8e54 100755\n--- a/t/t3411-rebase-preserve-around-merges.sh\n+++ b/t/t3411-rebase-preserve-around-merges.sh\n@@ -37,7 +37,7 @@ test_expect_success 'setup' '\n # -- C1 --\n #\n test_expect_success 'squash F1 into D1' '\n-\tFAKE_LINES=\"1 squash 3 2\" git rebase -i -p B1 &&\n+\tFAKE_LINES=\"1 squash 4 2 3\" git rebase -i -p B1 &&\n \ttest \"$(git rev-parse HEAD^2)\" = \"$(git rev-parse C1)\" &&\n \ttest \"$(git rev-parse HEAD~2)\" = \"$(git rev-parse B1)\" &&\n \tgit tag E2\n-- \n1.7.6.rc0.1.gf20d7\n"},{"id":"169897","messageId":"4DF4E93F.1020707@gmail.com","threadId":"27369","inReplyTo":"1307419725-4470-2-git-send-email-andrew.kw.w@gmail.com","subject":"Re: [PATCH] rebase -i -p: doesn't pick certain merge commits that are children of \"upstream\"","fromName":"Andrew Wong","fromEmail":"andrew.kw.w@gmail.com","sentAt":"2011-06-12T16:28:47Z","receivedAt":"2011-06-12T16:28:47Z","isPatch":true,"sender":{"key":"andrew.kw.w@gmail.com","avatar":"https://avatars.githubusercontent.com/u/489311?v=4"},"body":"Any chance we can look into getting this patch into git?\n\nhttp://permalink.gmane.org/gmane.comp.version-control.git/175185\n"},{"id":"169951","messageId":"7vmxhlpvob.fsf@alter.siamese.dyndns.org","threadId":"27369","inReplyTo":"1307419725-4470-2-git-send-email-andrew.kw.w@gmail.com","subject":"Re: [PATCH] rebase -i -p: doesn't pick certain merge commits that are children of \"upstream\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-06-13T16:01:40Z","receivedAt":"2011-06-13T16:01:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andrew Wong writes:\n\n> This patch fixes the inconsistency and bug by ensuring that all children\n> of upstream are always picked. This essentially reverts the commit:\n>\n> d80d6bc (rebase-i-p: do not include non-first-parent commits touching UPSTREAM, 2008-10-15)\n\n... whose commit log message mumbles about somebody's script but came with\nno tests, so we will not know if this is breaking the other guy's workflow\nwhile adding support to yours (Cc'ed Stephen Haberman who wrote the\nprevious one).\n\n> \n> +test_expect_success '' '\n\nThere is no title to this test?\n\n> +\t(\n> +\tcd clone4 &&\n> +\tgit fetch &&\n> +\tgit rebase -p HEAD^2 &&\n> +\ttest 1 = $(git rev-list --all --pretty=oneline | grep \"Modify A\" | wc -l) &&\n> +\ttest 1 = $(git rev-list --all --pretty=oneline | grep \"Modify B\" | wc -l) &&\n> +\ttest 1 = $(git rev-list --all --pretty=oneline | grep \"Merge remote-tracking branch \" | wc -l)\n> +\t)\n> +'\n> +\n> test_done\n\nIn general I think it is wrong to change behaviour depending on which\nparent of a merge we are looking at (unless of course the user tells us\nto, like \"git log --first-parent\"), so in that sense philosophically I\nthink the patch is going in the right direction, but I do worry about\npotential regressions.\n"},{"id":"169957","messageId":"4DF64932.1090607@sohovfx.com","threadId":"27369","inReplyTo":"7vmxhlpvob.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] rebase -i -p: doesn't pick certain merge commits that are children of \"upstream\"","fromName":"Andrew Wong","fromEmail":"andrew.w@sohovfx.com","sentAt":"2011-06-13T17:30:26Z","receivedAt":"2011-06-13T17:30:26Z","isPatch":true,"sender":{"key":"andrew.w@sohovfx.com","avatar":null},"body":"On 06/13/2011 12:01 PM, Junio C Hamano wrote:\n> There is no title to this test?\n> \nAh, that's embarrassing. I'll fix that. Thanks!\n\n> In general I think it is wrong to change behaviour depending on which\n> parent of a merge we are looking at (unless of course the user tells us\n> to, like \"git log --first-parent\"), so in that sense philosophically I\n> think the patch is going in the right direction, but I do worry about\n> potential regressions.\n> \nI totally agree. Ever since Jeff brought up this issue, I've been\nwondering what issue/workflow is that patch trying to fix. If the\n\"todo\" list doesn't change the parent of the merge commits, git should\nbe able to do a fast-forward on the merge, which means the merge won't\nbe rewritten anyway. Just a wild guess: maybe back then, git will\nactually rewrite the merge regardless? Anyway, let's wait for a reply\nfrom Stephen.\n"},{"id":"170171","messageId":"20110616172454.13ff1a18@sh9","threadId":"27369","inReplyTo":"4DF64932.1090607@sohovfx.com","subject":"Re: [PATCH] rebase -i -p: doesn't pick certain merge commits that are children of \"upstream\"","fromName":"Stephen Haberman","fromEmail":"stephen@exigencecorp.com","sentAt":"2011-06-16T22:24:54Z","receivedAt":"2011-06-16T22:24:54Z","isPatch":true,"sender":{"key":"stephen@exigencecorp.com","avatar":"https://gravatar.com/avatar/23b93ad70a06ce53505f17ddba65176edbcfb6588e7a4c1a2dca04aaf0a6aff1?d=mp&s=160"},"body":"Hey,\n\n> Ever since Jeff brought up this issue, I've been wondering what\n> issue/workflow is that patch trying to fix.\n\nI'm fairly sure the case the patch was fixing is t3411's \"squash F1 into\nD1\".\n\nWhere, starting with a tree like:\n\n# A1 - B1 - D1 - E1 - F1\n# \\ /\n# -- C1 --\n\nThe user is on F1 and issues: \"git rebase -i -p B1\", the todo list\nis \"D1, E1, F1\" (no C1), and they choose \"D1 squash F1, E1\",\nthe resulting tree should be:\n\n# A1 - B1 - D2 - E2\n# \\ /\n# -- C1 --\n\nAnd, the fix was that C1 should not be in the todo list.\n\nPerhaps that is unreasonable with whatever you guys are looking at now,\nbut, IIRC, the use case was that B1=some old commit, like a 2.0\nrelease, and a bunch of work happened on the C1 branch, it was merged\nin E1, but now when you want to rebase D1/E1/F1 on top of B1, you don't\nwant all of the noise of the C1 commit(s), since when rewriting E1 into\nE2, you can just reuse the un-rewritten C1 as its 2nd parent.\n\nWell, and not just the noise--since the todo is still flat, if C1\nwas listed in the todo, there's no way to recreate E2 as a merge and\nmaintain the C1 commit(s) as a separate branch. I think C1 would get\nflattened between D2/E2, depending on where it was in the todo. You'd\nlose a merge, contrary to the -p flag. That sounds like the core issue\nthat was being fixed.\n\nThe patch in question:\n\nhttp://article.gmane.org/gmane.comp.version-control.git/98251\n\nDid actually have a test (t3411) but it was still failing until\nthe following commit:\n\nhttp://article.gmane.org/gmane.comp.version-control.git/98253\n\nWhere the test changed from expect failure to expect success. I\nremember that looking odd at the time, but for some reason liked the\ncommits being separate.\n\n- Stephen\n"},{"id":"170193","messageId":"4DFC4863.2090803@sohovfx.com","threadId":"27369","inReplyTo":"20110616172454.13ff1a18@sh9","subject":"Re: [PATCH] rebase -i -p: doesn't pick certain merge commits that are children of \"upstream\"","fromName":"Andrew Wong","fromEmail":"andrew.w@sohovfx.com","sentAt":"2011-06-18T06:40:35Z","receivedAt":"2011-06-18T06:40:35Z","isPatch":true,"sender":{"key":"andrew.w@sohovfx.com","avatar":null},"body":"Here's a list of those commits in \"git log\"-order for easy reference:\n 80fe82e rebase-i-p: if todo was reordered use HEAD as the rewritten \nparent\n d80d6bc rebase-i-p: do not include non-first-parent commits touching \nUPSTREAM\n acc8559 rebase-i-p: only list commits that require rewriting in todo\n a4f25e3 rebase-i-p: fix 'no squashing merges' tripping up non-merges\n bb64507 rebase-i-p: delay saving current-commit to REWRITTEN if squashing\n 72583e6 rebase-i-p: use HEAD for updating the ref instead of mapping \nOLDHEAD\n 42f939e rebase-i-p: test to exclude commits from todo based on its \nparents\n\nOn 11-06-16 6:24 PM, Stephen Haberman wrote:\n> Perhaps that is unreasonable with whatever you guys are looking at now,\n> but, IIRC, the use case was that B1=some old commit, like a 2.0\n> release, and a bunch of work happened on the C1 branch, it was merged\n> in E1, but now when you want to rebase D1/E1/F1 on top of B1, you don't\n> want all of the noise of the C1 commit(s), since when rewriting E1 into\n> E2, you can just reuse the un-rewritten C1 as its 2nd parent.\nIn commit a4f25e3, we could already rebase B1 and squash F1 onto D1, \nwhile reusing C1 and recreating the merge. That means we could already \npass t3411.2if we adjusted the todo-list to account for the extra \"pick \nC1\" line.\n> Well, and not just the noise--since the todo is still flat, if C1\n> was listed in the todo, there's no way to recreate E2 as a merge and\n> maintain the C1 commit(s) as a separate branch. I think C1 would get\n> flattened between D2/E2, depending on where it was in the todo. You'd\n> lose a merge, contrary to the -p flag. That sounds like the core issue\n> that was being fixed\nThe merge shouldn't get flattened when the \"-p\" is used. As long as the \nmerge commit appears in the todo-list, git will trace the parents of the \nmerge commit, find the original or rewritten parents, and perform the \nmerge. Slightly off-topic, but I believe the branches will remain \nintact as long as the branch commits remain in the same topo-order \nrelative to each other in the todo-list. i.e. git will be confused if we \ntry to move a commit from one branch into the other.\n\nThe \"noise\" is filtered out by by commit d80d6bc. However, I think we \nshould keep the commits from branch C1, since there could be a scenario \nwhere we actually want to squash F1 onto C1 instead. That commit also \nintroduced a bug that Jeff King was running into: if we do \"git rebase \n-i -p C1\", the todo-list becomes a \"noop\", which means HEAD is reset to \nC1 and we lose the merge commit and F1.\n\nSo what I did in my patch is essentially revert the changes from \nd80d6bc, and adjust t3411.2 to account for the extra \"pick C1\" line.\n"},{"id":"170203","messageId":"20110618101718.6ff03688@sh9","threadId":"27369","inReplyTo":"4DFC4863.2090803@sohovfx.com","subject":"Re: [PATCH] rebase -i -p: doesn't pick certain merge commits that are children of \"upstream\"","fromName":"Stephen Haberman","fromEmail":"stephen@exigencecorp.com","sentAt":"2011-06-18T15:17:18Z","receivedAt":"2011-06-18T15:17:18Z","isPatch":true,"sender":{"key":"stephen@exigencecorp.com","avatar":"https://gravatar.com/avatar/23b93ad70a06ce53505f17ddba65176edbcfb6588e7a4c1a2dca04aaf0a6aff1?d=mp&s=160"},"body":"\n> In commit a4f25e3, we could already rebase B1 and squash F1 onto D1, \n> while reusing C1 and recreating the merge. That means we could\n> already pass t3411.2if we adjusted the todo-list to account for the\n> extra \"pick C1\" line.\n\nYou're right. I was wrong about that.\n\n> Slightly off-topic, but I believe the branches will remain intact as\n> long as the branch commits remain in the same topo-order relative to\n> each other in the todo-list.\n\nIf in topo-order, yeah, I guess that is right.\n\n> i.e. git will be confused if we try to move a commit from one branch\n> into the other.\n\nRight. If I do `rebase -i -p B1` and in the todo put C1 after F1, I get\na fatal message that E1 cannot be cherry picked.\n\nGiven rebase-i-p's limited ability to reorder graphs, e.g. the error\nabove, my understanding was that, when -p is used, only first-parent\nchanges should be in the todo. This straight line, non-graph list does\nlimit what the user can do, but, AFAIK, the benefit is that rebase-i-p\ncan then actually handle any given reordering of the todo.\n\nLetting C1 into the todo would mean having to explain to the user why\nsome of their reorderings worked and others didn't. Or else making\nrebase-i-p smart enough to handle all cases. Which, IIRC, was something\nconsidered unlikely just given the fact that todo is flat and there\nisn't a way for the user to express topo reorderings. At the time,\nthere was talk of another rewriting tool that would use marks and\nother hints to handle graphs and it was considered what, if anything,\nwould eventually handle complex rewrites like this.\n\nI think that Jeff's use case of rebase-i-p'ing C1, which is not on the\nfirst-parent list of commits, should be an error as it delves into\nterritory (topo reordering) that rebase-i-p can't fully handle.\n\n(If -p isn't used, just regular rebase, everything is being flattened,\nso there is no concern of topo reordering, so things are a lot simpler\nand C1 can/should be in the list.)\n\n- Stephen\n"},{"id":"170207","messageId":"4DFCD6A5.7000707@sohovfx.com","threadId":"27369","inReplyTo":"20110618101718.6ff03688@sh9","subject":"Re: [PATCH] rebase -i -p: doesn't pick certain merge commits that are children of \"upstream\"","fromName":"Andrew Wong","fromEmail":"andrew.w@sohovfx.com","sentAt":"2011-06-18T16:47:33Z","receivedAt":"2011-06-18T16:47:33Z","isPatch":true,"sender":{"key":"andrew.w@sohovfx.com","avatar":null},"body":"On 11-06-18 11:17 AM, Stephen Haberman wrote:\n> Letting C1 into the todo would mean having to explain to the user why\n> some of their reorderings worked and others didn't.\nThe bug section in rebase's documentation does mention that \"attempts to \nreorder commits tend to produce counterintuitive results\", which I think \nserves as a fairly good warning saying \"reorder at your own risk\". \nAlso, if we do a \"rebase-i-p A1\", the C1 branch will appear in the todo \nlist. A while ago I actually ran into this scenario, and I want to \nsquash a commit onto the C1 branch, which I can't if I simply choose B1 \nas the base. To workaround it, I just made A1 the base so that the C1 \nbranch will appear in the todo for me to squash upon. Otherwise, doing \nthe squash onto C1 manually would've involved several more steps.\n> I think that Jeff's use case of rebase-i-p'ing C1, which is not on the\n> first-parent list of commits, should be an error as it delves into\n> territory (topo reordering) that rebase-i-p can't fully handle.\nThere shouldn't be any topo-reordering unless the user explicitly \nchanges the order of the commit. The user is faced with the same \nlimitations (and bugs) as rebase-i-p'ing D1, so we shouldn't have to \nhandle the C1 case any different. rebase is perfectly capable of \nhandling the D1 case, just as how the C1 case is handled. We're only \nrunning into this issue because we're trying to filter out C1 when \nrebase-i-p'ing B1.\n"},{"id":"170208","messageId":"20110618121222.03ee0b79@sh9","threadId":"27369","inReplyTo":"4DFCD6A5.7000707@sohovfx.com","subject":"Re: [PATCH] rebase -i -p: doesn't pick certain merge commits that are children of \"upstream\"","fromName":"Stephen Haberman","fromEmail":"stephen@exigencecorp.com","sentAt":"2011-06-18T17:12:22Z","receivedAt":"2011-06-18T17:12:22Z","isPatch":true,"sender":{"key":"stephen@exigencecorp.com","avatar":"https://gravatar.com/avatar/23b93ad70a06ce53505f17ddba65176edbcfb6588e7a4c1a2dca04aaf0a6aff1?d=mp&s=160"},"body":"\n> The bug section in rebase's documentation does mention that \"attempts\n> to reorder commits tend to produce counterintuitive results\", which I\n> think serves as a fairly good warning saying \"reorder at your own\n> risk\".\n\nTrue.\n\n> Also, if we do a \"rebase-i-p A1\", the C1 branch will appear in\n> the todo list.\n\nHm, good point.\n\n> There shouldn't be any topo-reordering unless the user explicitly \n> changes the order of the commit. The user is faced with the same \n> limitations (and bugs) as rebase-i-p'ing D1, so we shouldn't have to \n> handle the C1 case any different. rebase is perfectly capable of \n> handling the D1 case, just as how the C1 case is handled. We're only \n> running into this issue because we're trying to filter out C1 when \n> rebase-i-p'ing B1.\n\nOkay, that makes sense.\n\nI agree with you then, with the behavior of \"rebase-i-p A1\" plus the\ndisclaimer in the docs warrants C1 showing up, C1 should be in the\ntodo list for \"rebase-i-p B1\" as well.\n\n...I can think of cases where personally I'd want to only move\naround commits on the first-parent line, e.g. even in the case of\n\"rebase-i-p A1\", to have less noise (C1 and any others on its branch)\nin the todo, but at that point it sounds like I'm projecting behavior\nonto rebase-i-p that isn't actually there.\n\n- Stephen\n"},{"id":"170221","messageId":"1308435121-9692-1-git-send-email-andrew.kw.w@gmail.com","threadId":"27369","inReplyTo":"20110618121222.03ee0b79@sh9","subject":"[PATCH] rebase -i -p: include non-first-parent commits in todo list","fromName":"Andrew Wong","fromEmail":"andrew.kw.w@gmail.com","sentAt":"2011-06-18T22:12:01Z","receivedAt":"2011-06-18T22:12:01Z","isPatch":true,"sender":{"key":"andrew.kw.w@gmail.com","avatar":"https://avatars.githubusercontent.com/u/489311?v=4"},"body":"Consider this graph:\n\n D---E (topic, HEAD)\n / /\n A---B---C (master)\n \\\n F (topic2)\n\nand the following three commands:\n 1. git rebase -i -p A\n 2. git rebase -i -p --onto F A\n 3. git rebase -i -p B\n\nCurrently, (1) and (2) will pick B, D, C, and E onto A and F,\nrespectively. However, (3) will only pick D and E onto B, but not C,\nwhich is inconsistent with (1) and (2). As a result, we cannot modify C\nduring the interactive-rebase.\n\nThe current behavior also creates a bug if we do:\n 4. git rebase -i -p C\n\nIn (4), E is never picked. And since interactive-rebase resets \"HEAD\"\nto \"onto\" before picking any commits, D and E are lost after the\ninteractive-rebase.\n\nThis patch fixes the inconsistency and bug by ensuring that all children\nof upstream are always picked. This essentially reverts the commit:\n d80d6bc146232d81f1bb4bc58e5d89263fd228d4\n\nWhen compiling the todo list, commits reachable from \"upstream\" should\nnever be skipped under any conditions. Otherwise, we lose the ability\nto modify them like (3), and create a bug like (4).\n\nTwo of the tests contain a scenario like (3). Since the new behavior\nadded more commits for picking, these tests need to be updated to\naccount for the additional pick lines. A new test has also been added\nfor (4).\n\nSigned-off-by: Andrew Wong \n---\n git-rebase--interactive.sh | 3 +--\n t/t3404-rebase-interactive.sh | 2 +-\n t/t3409-rebase-preserve-merges.sh | 28 +++++++++++++++++++++++++++-\n t/t3411-rebase-preserve-around-merges.sh | 2 +-\n 4 files changed, 30 insertions(+), 5 deletions(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 65690af..c6ba7c1 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -713,7 +713,6 @@ then\n \t# parents to rewrite and skipping dropped commits would\n \t# prematurely end our probe\n \tmerges_option=\n-\tfirst_after_upstream=\"$(git rev-list --reverse --first-parent $upstream..$orig_head | head -n 1)\"\n else\n \tmerges_option=\"--no-merges --cherry-pick\"\n fi\n@@ -746,7 +745,7 @@ do\n \t\t\tpreserve=t\n \t\t\tfor p in $(git rev-list --parents -1 $sha1 | cut -d' ' -s -f2-)\n \t\t\tdo\n-\t\t\t\tif test -f \"$rewritten\"/$p -a \\( $p != $onto -o $sha1 = $first_after_upstream \\)\n+\t\t\t\tif test -f \"$rewritten\"/$p\n \t\t\t\tthen\n \t\t\t\t\tpreserve=f\n \t\t\t\tfi\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 47c8371..8538813 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -295,7 +295,7 @@ test_expect_success 'preserve merges with -p' '\n '\n \n test_expect_success 'edit ancestor with -p' '\n-\tFAKE_LINES=\"1 edit 2 3 4\" git rebase -i -p HEAD~3 &&\n+\tFAKE_LINES=\"1 2 edit 3 4\" git rebase -i -p HEAD~3 &&\n \techo 2 > unrelated-file &&\n \ttest_tick &&\n \tgit commit -m L2-modified --amend unrelated-file &&\ndiff --git a/t/t3409-rebase-preserve-merges.sh b/t/t3409-rebase-preserve-merges.sh\nindex 08201e2..6de4e22 100755\n--- a/t/t3409-rebase-preserve-merges.sh\n+++ b/t/t3409-rebase-preserve-merges.sh\n@@ -37,7 +37,15 @@ export GIT_AUTHOR_EMAIL\n # \\\n # B2 <-- origin/topic\n #\n-# In all cases, 'topic' is rebased onto 'origin/topic'.\n+# Clone 4 (merge using second parent as base):\n+#\n+# A1--A2--B3 <-- origin/master\n+# \\\n+# B1--A3--M <-- topic\n+# \\ /\n+# \\--A4 <-- topic2\n+# \\\n+# B2 <-- origin/topic\n \n test_expect_success 'setup for merge-preserving rebase' \\\n \t'echo First > A &&\n@@ -57,6 +65,13 @@ test_expect_success 'setup for merge-preserving rebase' \\\n \tgit merge origin/master\n \t) &&\n \n+\tgit clone ./. clone4 &&\n+\t(\n+\t\tcd clone4 &&\n+\t\tgit checkout -b topic origin/topic &&\n+\t\tgit merge origin/master\n+\t) &&\n+\n \techo Fifth > B &&\n \tgit add B &&\n \tgit commit -m \"Add different B\" &&\n@@ -123,4 +138,15 @@ test_expect_success 'rebase -p preserves no-ff merges' '\n \t)\n '\n \n+test_expect_success 'rebase -p works when base inside second parent' '\n+\t(\n+\tcd clone4 &&\n+\tgit fetch &&\n+\tgit rebase -p HEAD^2 &&\n+\ttest 1 = $(git rev-list --all --pretty=oneline | grep \"Modify A\" | wc -l) &&\n+\ttest 1 = $(git rev-list --all --pretty=oneline | grep \"Modify B\" | wc -l) &&\n+\ttest 1 = $(git rev-list --all --pretty=oneline | grep \"Merge remote-tracking branch \" | wc -l)\n+\t)\n+'\n+\n test_done\ndiff --git a/t/t3411-rebase-preserve-around-merges.sh b/t/t3411-rebase-preserve-around-merges.sh\nindex 14a23cd..ace8e54 100755\n--- a/t/t3411-rebase-preserve-around-merges.sh\n+++ b/t/t3411-rebase-preserve-around-merges.sh\n@@ -37,7 +37,7 @@ test_expect_success 'setup' '\n # -- C1 --\n #\n test_expect_success 'squash F1 into D1' '\n-\tFAKE_LINES=\"1 squash 3 2\" git rebase -i -p B1 &&\n+\tFAKE_LINES=\"1 squash 4 2 3\" git rebase -i -p B1 &&\n \ttest \"$(git rev-parse HEAD^2)\" = \"$(git rev-parse C1)\" &&\n \ttest \"$(git rev-parse HEAD~2)\" = \"$(git rev-parse B1)\" &&\n \tgit tag E2\n-- \n1.7.2.2\n"},{"id":"170222","messageId":"4DFD22F1.3080007@sohovfx.com","threadId":"27369","inReplyTo":"20110618121222.03ee0b79@sh9","subject":"Re: [PATCH] rebase -i -p: doesn't pick certain merge commits that are children of \"upstream\"","fromName":"Andrew Wong","fromEmail":"andrew.w@sohovfx.com","sentAt":"2011-06-18T22:13:05Z","receivedAt":"2011-06-18T22:13:05Z","isPatch":true,"sender":{"key":"andrew.w@sohovfx.com","avatar":null},"body":"On 11-06-18 1:12 PM, Stephen Haberman wrote:\n> ...I can think of cases where personally I'd want to only move\n> around commits on the first-parent line, e.g. even in the case of\n> \"rebase-i-p A1\", to have less noise (C1 and any others on its branch)\n> in the todo, but at that point it sounds like I'm projecting behavior\n> onto rebase-i-p that isn't actually there.\nYes, it would definitely be useful to be able to do that. In fact, \nthere's a somewhat relevant expect-failure-test t3404.18 that is testing \nfor that. Like you said, we need a way to express topology in the todo \nlist, which I'm not sure what a good representation is.\n"}]}