{"thread":{"id":"32610","subject":"[PATCH] rebase --preserve-merges keeps empty merge commits","startedAt":"2013-01-12T20:46:01Z","lastAt":"2013-02-25T06:44:24Z","messageCount":9,"participants":["Phil Hord","Neil Horman","Matthieu Moy","Junio C Hamano","Martin von Zweigbergk"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"206625","messageId":"1358023561-26773-1-git-send-email-hordp@cisco.com","threadId":"32610","inReplyTo":null,"subject":"[PATCH] rebase --preserve-merges keeps empty merge commits","fromName":"Phil Hord","fromEmail":"hordp@cisco.com","sentAt":"2013-01-12T20:46:01Z","receivedAt":"2013-01-12T20:46:01Z","isPatch":true,"sender":{"key":"phil.hord@gmail.com","avatar":"https://avatars.githubusercontent.com/u/123908?v=4"},"body":"Since 90e1818f9a (git-rebase: add keep_empty flag, 2012-04-20)\n'git rebase --preserve-merges' fails to preserve empty merge commits\nunless --keep-empty is also specified. Merge commits should be\npreserved in order to preserve the structure of the rebased graph,\neven if the merge commit does not introduce changes to the parent.\n\nTeach rebase not to drop merge commits only because they are empty.\n\nA special case which is not handled by this change is for a merge commit\nwhose parents are now the same commit because all the previous different\nparents have been dropped as a result of this rebase or some previous\noperation.\n---\n git-rebase--interactive.sh | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 44901d5..8ed7fcc 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -190,6 +190,11 @@ is_empty_commit() {\n \ttest \"$tree\" = \"$ptree\"\n }\n \n+is_merge_commit()\n+{\n+\tgit rev-parse --verify --quiet \"$1\"^2 >/dev/null 2>&1\n+}\n+\n # Run command with GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and\n # GIT_AUTHOR_DATE exported from the current environment.\n do_with_author () {\n@@ -874,7 +879,7 @@ git rev-list $merges_option --pretty=oneline --abbrev-commit \\\n while read -r shortsha1 rest\n do\n \n-\tif test -z \"$keep_empty\" && is_empty_commit $shortsha1\n+\tif test -z \"$keep_empty\" && is_empty_commit $shortsha1 && ! is_merge_commit $shortsha1\n \tthen\n \t\tcomment_out=\"# \"\n \telse\n-- \n1.8.1.dirty\n"},{"id":"206797","messageId":"20130114140249.GA2373@hmsreliant.think-freely.org","threadId":"32610","inReplyTo":"1358023561-26773-1-git-send-email-hordp@cisco.com","subject":"Re: [PATCH] rebase --preserve-merges keeps empty merge commits","fromName":"Neil Horman","fromEmail":"nhorman@tuxdriver.com","sentAt":"2013-01-14T14:02:49Z","receivedAt":"2013-01-14T14:02:49Z","isPatch":true,"sender":{"key":"nhorman@tuxdriver.com","avatar":"https://avatars.githubusercontent.com/u/1032926?v=4"},"body":"On Sat, Jan 12, 2013 at 03:46:01PM -0500, Phil Hord wrote:\n> Since 90e1818f9a (git-rebase: add keep_empty flag, 2012-04-20)\n> 'git rebase --preserve-merges' fails to preserve empty merge commits\n> unless --keep-empty is also specified. Merge commits should be\n> preserved in order to preserve the structure of the rebased graph,\n> even if the merge commit does not introduce changes to the parent.\n> \n> Teach rebase not to drop merge commits only because they are empty.\n> \n> A special case which is not handled by this change is for a merge commit\n> whose parents are now the same commit because all the previous different\n> parents have been dropped as a result of this rebase or some previous\n> operation.\n> ---\n> git-rebase--interactive.sh | 7 ++++++-\n> 1 file changed, 6 insertions(+), 1 deletion(-)\n> \n> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\n> index 44901d5..8ed7fcc 100644\n> --- a/git-rebase--interactive.sh\n> +++ b/git-rebase--interactive.sh\n> @@ -190,6 +190,11 @@ is_empty_commit() {\n> \ttest \"$tree\" = \"$ptree\"\n> }\n> \n> +is_merge_commit()\n> +{\n> +\tgit rev-parse --verify --quiet \"$1\"^2 >/dev/null 2>&1\n> +}\n> +\n> # Run command with GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and\n> # GIT_AUTHOR_DATE exported from the current environment.\n> do_with_author () {\n> @@ -874,7 +879,7 @@ git rev-list $merges_option --pretty=oneline --abbrev-commit \\\n> while read -r shortsha1 rest\n> do\n> \n> -\tif test -z \"$keep_empty\" && is_empty_commit $shortsha1\n> +\tif test -z \"$keep_empty\" && is_empty_commit $shortsha1 && ! is_merge_commit $shortsha1\n> \tthen\n> \t\tcomment_out=\"# \"\n> \telse\n> -- \n> 1.8.1.dirty\n> \n> \nSeems reasonable \nAcked-by: Neil Horman \n"},{"id":"206798","messageId":"vpqpq17zwdl.fsf@grenoble-inp.fr","threadId":"32610","inReplyTo":"1358023561-26773-1-git-send-email-hordp@cisco.com","subject":"Re: [PATCH] rebase --preserve-merges keeps empty merge commits","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2013-01-14T14:12:38Z","receivedAt":"2013-01-14T14:12:38Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Phil Hord writes:\n\n> Subject: [PATCH] rebase --preserve-merges keeps empty merge commits\n\nI would rephrase it as\n\n rebase --preserve-merges: keep empty merge commits\n\nwe usually give orders in commit messages, not state facts (it's not\nclear from the existing subject line whether keeping merge commit is the\nnew behavior or a bug that the commit tries to fix).\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"206811","messageId":"7vwqvfele2.fsf@alter.siamese.dyndns.org","threadId":"32610","inReplyTo":"vpqpq17zwdl.fsf@grenoble-inp.fr","subject":"Re: [PATCH] rebase --preserve-merges keeps empty merge commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-14T17:15:33Z","receivedAt":"2013-01-14T17:15:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy writes:\n\n> Phil Hord writes:\n>\n>> Subject: [PATCH] rebase --preserve-merges keeps empty merge commits\n>\n> I would rephrase it as\n>\n> rebase --preserve-merges: keep empty merge commits\n>\n> we usually give orders in commit messages, not state facts (it's not\n> clear from the existing subject line whether keeping merge commit is the\n> new behavior or a bug that the commit tries to fix).\n\nThanks for giving a concise rationale on our use of imperative mood.\n\nPhil, I think you meant to and forgot to sign-off; here is what I'll\nqueue.\n\nThanks.\n\n-- >8 --\nFrom: Phil Hord \nDate: Sat, 12 Jan 2013 15:46:01 -0500\nSubject: [PATCH] rebase --preserve-merges: keep all merge commits including empty ones\n\nSince 90e1818f9a (git-rebase: add keep_empty flag, 2012-04-20)\n'git rebase --preserve-merges' fails to preserve empty merge commits\nunless --keep-empty is also specified. Merge commits should be\npreserved in order to preserve the structure of the rebased graph,\neven if the merge commit does not introduce changes to the parent.\n\nTeach rebase not to drop merge commits only because they are empty.\n\nA special case which is not handled by this change is for a merge commit\nwhose parents are now the same commit because all the previous different\nparents have been dropped as a result of this rebase or some previous\noperation.\n\nSigned-off-by: Phil Hord \nAcked-by: Neil Horman \nSigned-off-by: Junio C Hamano \n---\n git-rebase--interactive.sh | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 0c19b7c..2fed92f 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -175,6 +175,11 @@ is_empty_commit() {\n \ttest \"$tree\" = \"$ptree\"\n }\n \n+is_merge_commit()\n+{\n+\tgit rev-parse --verify --quiet \"$1\"^2 >/dev/null 2>&1\n+}\n+\n # Run command with GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and\n # GIT_AUTHOR_DATE exported from the current environment.\n do_with_author () {\n@@ -796,7 +801,7 @@ git rev-list $merges_option --pretty=oneline --abbrev-commit \\\n while read -r shortsha1 rest\n do\n \n-\tif test -z \"$keep_empty\" && is_empty_commit $shortsha1\n+\tif test -z \"$keep_empty\" && is_empty_commit $shortsha1 && ! is_merge_commit $shortsha1\n \tthen\n \t\tcomment_out=\"# \"\n \telse\n-- \n1.8.1.1.338.g126d652\n"},{"id":"206816","messageId":"50F44559.6040102@cisco.com","threadId":"32610","inReplyTo":"7vwqvfele2.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] rebase --preserve-merges keeps empty merge commits","fromName":"Phil Hord","fromEmail":"hordp@cisco.com","sentAt":"2013-01-14T17:50:17Z","receivedAt":"2013-01-14T17:50:17Z","isPatch":true,"sender":{"key":"phil.hord@gmail.com","avatar":"https://avatars.githubusercontent.com/u/123908?v=4"},"body":"\nJunio C Hamano wrote:\n> Matthieu Moy writes:\n>\n>> Phil Hord writes:\n>>\n>>> Subject: [PATCH] rebase --preserve-merges keeps empty merge commits\n>> I would rephrase it as\n>>\n>> rebase --preserve-merges: keep empty merge commits\n>>\n>> we usually give orders in commit messages, not state facts (it's not\n>> clear from the existing subject line whether keeping merge commit is the\n>> new behavior or a bug that the commit tries to fix).\n> Thanks for giving a concise rationale on our use of imperative mood.\n>\n> Phil, I think you meant to and forgot to sign-off; here is what I'll\n> queue.\n>\n> Thanks.\n>\n\nLooks good. Thanks for the help.\n\nPhil\n"},{"id":"208488","messageId":"CANiSa6gM1gpj0A6PC0qNVSaWvVrOBnSnjn2uKR9-cHSLAZ2OVA@mail.gmail.com","threadId":"32610","inReplyTo":"1358023561-26773-1-git-send-email-hordp@cisco.com","subject":"Re: [PATCH] rebase --preserve-merges keeps empty merge commits","fromName":"Martin von Zweigbergk","fromEmail":"martinvonz@gmail.com","sentAt":"2013-02-01T19:15:28Z","receivedAt":"2013-02-01T19:15:28Z","isPatch":true,"sender":{"key":"martinvonz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/891642?v=4"},"body":"I'm working on a re-roll of\n\nhttp://thread.gmane.org/gmane.comp.version-control.git/205796\n\nand finally got around to including test cases for what you fixed in\nthis patch. I want to make sure I'm testing what you fixed here. See\nquestions below.\n\nOn Sat, Jan 12, 2013 at 12:46 PM, Phil Hord wrote:\n> Since 90e1818f9a (git-rebase: add keep_empty flag, 2012-04-20)\n> 'git rebase --preserve-merges' fails to preserve empty merge commits\n> unless --keep-empty is also specified. Merge commits should be\n> preserved in order to preserve the structure of the rebased graph,\n> even if the merge commit does not introduce changes to the parent.\n>\n> Teach rebase not to drop merge commits only because they are empty.\n\nConsider a history like\n\n# a---b---c\n# \\ \\\n# d---l\n# \\\n# e\n# \\\n# C\n\nwhere 'l' is tree-same with 'd' and 'C' introduces the same change as 'c'.\n\nMy test case runs 'git rebase -p e l' and expects the result to look like\n\n# a---b---c\n# \\ \\\n# d \\\n# \\ \\\n# e---l\n\n> A special case which is not handled by this change is for a merge commit\n> whose parents are now the same commit because all the previous different\n> parents have been dropped as a result of this rebase or some previous\n> operation.\n\nAnd for this case, the test case runs 'git rebase -p C l'. Is that\nwhat you meant here?\n\nBefore your patch, git would just say \"Nothing to do\" and after your\npatch, we get\n\n# a---b---c\n# \\ \\\n# d \\\n# \\ \\\n# e \\\n# \\ \\\n# C---l\n\nAs you say, your patch doesn't try to handle this case, but at least\nthe new behavior seems better. I think we would ideally want the\nrecreated 'l' to have only 'C' as parent in this case. Does that make\nsense?\n\nMartin\n"},{"id":"208508","messageId":"510C2E10.1050403@cisco.com","threadId":"32610","inReplyTo":"CANiSa6gM1gpj0A6PC0qNVSaWvVrOBnSnjn2uKR9-cHSLAZ2OVA@mail.gmail.com","subject":"Re: [PATCH] rebase --preserve-merges keeps empty merge commits","fromName":"Phil Hord","fromEmail":"hordp@cisco.com","sentAt":"2013-02-01T21:05:20Z","receivedAt":"2013-02-01T21:05:20Z","isPatch":true,"sender":{"key":"phil.hord@gmail.com","avatar":"https://avatars.githubusercontent.com/u/123908?v=4"},"body":"\nMartin von Zweigbergk wrote:\n> I'm working on a re-roll of\n> http://thread.gmane.org/gmane.comp.version-control.git/205796\n>\n> and finally got around to including test cases for what you fixed in\n> this patch. I want to make sure I'm testing what you fixed here. See\n> questions below.\n\nThanks for that. I should have done this myself.\n\n> On Sat, Jan 12, 2013 at 12:46 PM, Phil Hord wrote:\n>> Since 90e1818f9a (git-rebase: add keep_empty flag, 2012-04-20)\n>> 'git rebase --preserve-merges' fails to preserve empty merge commits\n>> unless --keep-empty is also specified. Merge commits should be\n>> preserved in order to preserve the structure of the rebased graph,\n>> even if the merge commit does not introduce changes to the parent.\n>>\n>> Teach rebase not to drop merge commits only because they are empty.\n> Consider a history like\n>\n> # a---b---c\n> # \\ \\\n> # d---l\n> # \\\n> # e\n> # \\\n> # C\n>\n> where 'l' is tree-same with 'd' and 'C' introduces the same change as 'c'.\n>\n> My test case runs 'git rebase -p e l' and expects the result to look like\n>\n> # a---b---c\n> # \\ \\\n> # d \\\n> # \\ \\\n> # e---l\n>\n\nThis is probably right, but it is not exactly the case that caused my itch.\nI think my branch looked like this:\n\n# a---b---c\n# \\ \n# d---f\n# \\ \\\n# e---g\n# \\\n# l\n\nwhere g is tree-same with f. That is, e merged with f, but all of e's\nchanges were dropped in the merge.\n\nSo when I ran 'git rebase -p c l', I expected to end up with this:\n\n# a---b---c\n# \\ \n# d---f\n# \\ \\\n# e---g\n# \\\n# l\n\nBut instead, I got an error because git-rebase--interactive.sh decided\nthat g was empty, so it dropped it by commenting it out of the todo\nlist:\n\npick d\npick e\npick f\n#pick g\npick l\n\nAt the end of this attempt, I got some odd error about a cherry-pick\nhave incorrect parameters or somesuch. I bisected the problem to a\ncommit that clued me in to one of my commits being silently dropped.\nAnd that is specifically what I fixed.\n\nThis happened only because 'is_empty_commit' checks for tree-sameness\nwith the first parent; it does not consider whether there are multiple\nparents. Perhaps it should.\n\n>> A special case which is not handled by this change is for a merge commit\n>> whose parents are now the same commit because all the previous different\n>> parents have been dropped as a result of this rebase or some previous\n>> operation.\n> And for this case, the test case runs 'git rebase -p C l'. Is that\n> what you meant here?\n>\n> Before your patch, git would just say \"Nothing to do\"\n\nHuh. That is worse than I thought.\n\n> and after your\n> patch, we get\n>\n> # a---b---c\n> # \\ \\\n> # d \\\n> # \\ \\\n> # e \\\n> # \\ \\\n> # C---l\n>\n> As you say, your patch doesn't try to handle this case, but at least\n> the new behavior seems better. I think we would ideally want the\n> recreated 'l' to have only 'C' as parent in this case. Does that make\n> sense?\n\nThis is not what I meant, but it is a very interesting corner case. I\nam not sure I have a solid opinion on what the result should be here.\nI feel like it should look the same as you show here, since neither\n'c' nor 'C' is a candidate for collapsing during this rebase. But I may\nbe missing some subtlety here.\n\n\nHere is the corner case I was thinking of. I did not test this to see\nif this will happen, but I conceived that it might. Suppose you have\nthis tree where\n\n# a---b---c\n# \\ \n# d---g---l\n# \\ /\n# C\n\nwhere 'C' introduced the same changes as 'c'.\n\nWhen I execute 'git rebase -p l c', I expect that I will end up with\nthis:\n\n# a---b---c---d---\n# \\ \\\n# ---g---l\n\nThat is, 'C' gets skipped because it introduces the same changes already\nseen in 'c'. So 'g' now has two parents: 'd' and 'C^'. But 'C^' is 'd',\nso 'g' now has two parents, both of whom are 'd'. \n\nI think it should collapse to this instead:\n\n# a---b---c---d---g---l\n\nI don't think this occurs because of my patch, and I am not sure it\noccurs at all. It is something that I considered when I was thinking of\nfailure scenarios for my patch.\n\nI expect it also may happen if 'C' is an already-empty commit, or if\nit is made empty after conflict resolution involving the user. I\nmentioned it because I thought my patch _could_ address this if my\nis_merge_commit test would also consider whether the parents are\ndistinct from each other or not.\n\nI hope this is clear, but please let me know if I made it too confusing.\n\nPhil\n"},{"id":"208519","messageId":"CANiSa6gxoHPWO85ZYu5iyHxzpE4HAprpEgG4305_jhyOE0LWGQ@mail.gmail.com","threadId":"32610","inReplyTo":"510C2E10.1050403@cisco.com","subject":"Re: [PATCH] rebase --preserve-merges keeps empty merge commits","fromName":"Martin von Zweigbergk","fromEmail":"martinvonz@gmail.com","sentAt":"2013-02-02T08:21:56Z","receivedAt":"2013-02-02T08:21:56Z","isPatch":true,"sender":{"key":"martinvonz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/891642?v=4"},"body":"On Fri, Feb 1, 2013 at 1:05 PM, Phil Hord wrote:\n>\n> This is probably right, but it is not exactly the case that caused my itch.\n> I think my branch looked like [...]\n\nThat also makes sense. I'll add tests for both cases. Your patch makes\nboth of them pass.\n\n>> # a---b---c\n>> # \\ \\\n>> # d \\\n>> # \\ \\\n>> # e \\\n>> # \\ \\\n>> # C---l\n>>\n>> As you say, your patch doesn't try to handle this case, but at least\n>> the new behavior seems better. I think we would ideally want the\n>> recreated 'l' to have only 'C' as parent in this case. Does that make\n>> sense?\n>\n> This is not what I meant, but it is a very interesting corner case. I\n> am not sure I have a solid opinion on what the result should be here.\n\nNeither do I, so I'll just drop the test case. Thanks.\n\n> Here is the corner case I was thinking of. I did not test this to see\n> if this will happen, but I conceived that it might. Suppose you have\n> this tree where\n>\n> # a---b---c\n> # \\\n> # d---g---l\n> # \\ /\n> # C\n>\n> where 'C' introduced the same changes as 'c'.\n>\n> When I execute 'git rebase -p l c', I expect that I will end up with\n> this:\n>\n> # a---b---c---d---\n> # \\ \\\n> # ---g---l\n>\n> That is, 'C' gets skipped because it introduces the same changes already\n> seen in 'c'. So 'g' now has two parents: 'd' and 'C^'. But 'C^' is 'd',\n> so 'g' now has two parents, both of whom are 'd'.\n>\n> I think it should collapse to this instead:\n>\n> # a---b---c---d---g---l\n\nI think this is actually what you will get. But I think it will only\nbe linearized if the branch that should be dropped is the second\nparent. I have two tests for this, but I need to simplify them a\nlittle to see that that (parent number) is the only difference.\n\n> I hope this is clear, but please let me know if I made it too confusing.\n\nVery clear. Thanks.\n"},{"id":"210228","messageId":"7v7glw3nav.fsf@alter.siamese.dyndns.org","threadId":"32610","inReplyTo":"CANiSa6gM1gpj0A6PC0qNVSaWvVrOBnSnjn2uKR9-cHSLAZ2OVA@mail.gmail.com","subject":"Re: [PATCH] rebase --preserve-merges keeps empty merge commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-25T06:44:24Z","receivedAt":"2013-02-25T06:44:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin von Zweigbergk writes:\n\n> I'm working on a re-roll of\n>\n> http://thread.gmane.org/gmane.comp.version-control.git/205796\n>\n> and finally got around to including test cases for what you fixed in\n> this patch. I want to make sure I'm testing what you fixed here. See\n> questions below.\n\nDid anything further happen to this topic?\n"}]}