{"thread":{"id":"16793","subject":"[PATCH] Make git revert warn the user when reverting a merge commit.","startedAt":"2008-12-19T02:39:15Z","lastAt":"2008-12-21T22:56:33Z","messageCount":24,"participants":["Boyd Stephen Smith Jr.","Johannes Schindelin","Junio C Hamano","Jay Soffian","Alan","Robin Rosenberg"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"98322","messageId":"200812182039.15169.bss@iguanasuicide.net","threadId":"16793","inReplyTo":null,"subject":"[PATCH] Make git revert warn the user when reverting a merge commit.","fromName":"Boyd Stephen Smith Jr.","fromEmail":"bss@iguanasuicide.net","sentAt":"2008-12-19T02:39:15Z","receivedAt":"2008-12-19T02:39:15Z","isPatch":true,"sender":{"key":"bss@iguanasuicide.net","avatar":"https://gravatar.com/avatar/84b95eeff194b816c1568b1339e63e4b229825298664a9037b9f1ec713ead1e3?d=mp&s=160"},"body":"Signed-off-by: Boyd Stephen Smith Jr \n---\nOn Thursday 2008 December 18 18:21:25 Linus Torvalds wrote:\n> I suspect we should warn about reverting merges.\n\nHere is a patch (aginst c0ceb2c, which I believe is master currently) that\ndoes just that.\n\nAfter applying the patch I get the following test results:\nfixed 1\nsuccess 4108\nfailed 0\nbroken 4\ntotal 4113\n\n builtin-revert.c | 15 +++++++++++++++\n 1 files changed, 15 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin-revert.c b/builtin-revert.c\nindex 4038b41..7f121a5 100644\n--- a/builtin-revert.c\n+++ b/builtin-revert.c\n@@ -296,6 +296,21 @@ static int revert_or_cherry_pick(int argc, const char \n**argv)\n \t\tint cnt;\n \t\tstruct commit_list *p;\n \n+\t\tdo {\n+\t\t\tswitch (action) {\n+\t\t\tcase REVERT:\n+\t\t\t\twarning(\"revert on a merge commit may not do what you expect.\");\n+\t\t\t\tcontinue;\n+\t\t\tcase CHERRY_PICK:\n+\t\t\t\t/* Cherry picking a merge doesn't merge the history, but\n+\t\t\t\t * I don't think many people expect that.\n+\t\t\t\t */\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\t\t\t/* Unhandled enum member. */\n+\t\t\tdie(\"Unknown action on a merge commit.\");\n+\t\t} while (0);\n+\n \t\tif (!mainline)\n \t\t\tdie(\"Commit %s is a merge but no -m option was given.\",\n \t\t\t sha1_to_hex(commit->object.sha1));\n-- \n1.5.6\n-- \nBoyd Stephen Smith Jr. ,= ,-_-. =. \nbss@iguanasuicide.net ((_/)o o(\\_))\nICQ: 514984 YM/AIM: DaTwinkDaddy `-'(. .)`-' \nhttp://iguanasuicide.net/ \\_/ \n"},{"id":"98324","messageId":"alpine.DEB.1.00.0812190353520.14632@racer","threadId":"16793","inReplyTo":"200812182039.15169.bss@iguanasuicide.net","subject":"Re: [PATCH] Make git revert warn the user when reverting a merge commit.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-12-19T02:57:57Z","receivedAt":"2008-12-19T02:57:57Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 18 Dec 2008, Boyd Stephen Smith Jr. wrote:\n\n> +\t\tdo {\n> +\t\t\tswitch (action) {\n> +\t\t\tcase REVERT:\n> +\t\t\t\twarning(\"revert on a merge commit may not do what you expect.\");\n> +\t\t\t\tcontinue;\n> +\t\t\tcase CHERRY_PICK:\n> +\t\t\t\t/* Cherry picking a merge doesn't merge the history, but\n> +\t\t\t\t * I don't think many people expect that.\n> +\t\t\t\t */\n> +\t\t\t\tcontinue;\n> +\t\t\t}\n> +\t\t\t/* Unhandled enum member. */\n> +\t\t\tdie(\"Unknown action on a merge commit.\");\n> +\t\t} while (0);\n> +\n\nWow. That must be one of the, uhm, less beautiful ways to write\n\n\t\tif (action == REVERT)\n\t\t\twarning(\"revert on a merge commit may not do what you \"\n\t\t\t\t\"expect.\");\n\t\telse if (action != CHERRY_PICK)\n\t\t\tdie(\"Unknown action on a merge commit.\");\n\nBesides, I am actually pretty much against this change. You already have \nto ask very explicitely to revert a merge, by specifying a parent number. \nIf I ask for something explicitely, I do not want the tool to tell me that \nit's dangerous. I know that already, thankyouverymuch.\n\nCiao,\nDscho\n"},{"id":"98325","messageId":"7vej04eui5.fsf@gitster.siamese.dyndns.org","threadId":"16793","inReplyTo":"alpine.DEB.1.00.0812190353520.14632@racer","subject":"Re: [PATCH] Make git revert warn the user when reverting a merge commit.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-19T03:03:46Z","receivedAt":"2008-12-19T03:03:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Wow. That must be one of the, uhm, less beautiful ways to write\n>\n> \t\tif (action == REVERT)\n> \t\t\twarning(\"revert on a merge commit may not do what you \"\n> \t\t\t\t\"expect.\");\n> \t\telse if (action != CHERRY_PICK)\n> \t\t\tdie(\"Unknown action on a merge commit.\");\n>\n> Besides, I am actually pretty much against this change. You already have \n> to ask very explicitely to revert a merge, by specifying a parent number. \n> If I ask for something explicitely, I do not want the tool to tell me that \n> it's dangerous. I know that already, thankyouverymuch.\n\nOr you may not have known that it is dangerous, but the new warning does\nnot give you enough clue where to go next, so this warning does not give\nreal value. It is pretty much meaningless noise to users.\n"},{"id":"98326","messageId":"200812182124.15568.bss@iguanasuicide.net","threadId":"16793","inReplyTo":"alpine.DEB.1.00.0812190353520.14632@racer","subject":"Re: [PATCH] Make git revert warn the user when reverting a merge commit.","fromName":"Boyd Stephen Smith Jr.","fromEmail":"bss@iguanasuicide.net","sentAt":"2008-12-19T03:24:11Z","receivedAt":"2008-12-19T03:24:11Z","isPatch":true,"sender":{"key":"bss@iguanasuicide.net","avatar":"https://gravatar.com/avatar/84b95eeff194b816c1568b1339e63e4b229825298664a9037b9f1ec713ead1e3?d=mp&s=160"},"body":"Blah, my --in-reply-to didn't work so this didn't thread right.\n\nOn Thursday 2008 December 18 20:57:57 you wrote:\n> On Thu, 18 Dec 2008, Boyd Stephen Smith Jr. wrote:\n> > +\t\tdo {\n> > +\t\t\tswitch (action) {\n> > +\t\t\tcase REVERT:\n> > +\t\t\t\twarning(\"revert on a merge commit may not do what you expect.\");\n> > +\t\t\t\tcontinue;\n> > +\t\t\tcase CHERRY_PICK:\n> > +\t\t\t\t/* Cherry picking a merge doesn't merge the history, but\n> > +\t\t\t\t * I don't think many people expect that.\n> > +\t\t\t\t */\n> > +\t\t\t\tcontinue;\n> > +\t\t\t}\n> > +\t\t\t/* Unhandled enum member. */\n> > +\t\t\tdie(\"Unknown action on a merge commit.\");\n> > +\t\t} while (0);\n> > +\n>\n> Wow. That must be one of the, uhm, less beautiful ways to write\n>\n> \t\tif (action == REVERT)\n> \t\t\twarning(\"revert on a merge commit may not do what you \"\n> \t\t\t\t\"expect.\");\n> \t\telse if (action != CHERRY_PICK)\n> \t\t\tdie(\"Unknown action on a merge commit.\");\n\nMy way, a smart compiler will warn at compile time that there's a new enum \nmember that needs to be handled. Your way, no such compile-time warning will \nbe emitted. At runtime, they have the same behavior. Athestically, I agree \nwith you, but my way may have technical advantages.\n\nI did check the CodingGuidelines and didn't see this construct mentioned.\n\n> Besides, I am actually pretty much against this change.\n\nI've never had a need to revert a merge commit, so it's not a big win either \nway for me. I wrote the patch because alan@clueserver.org had the revert \nbehavior bite him and Linus suggested a warning might be apropos.\n-- \nBoyd Stephen Smith Jr. ,= ,-_-. =. \nbss@iguanasuicide.net ((_/)o o(\\_))\nICQ: 514984 YM/AIM: DaTwinkDaddy `-'(. .)`-' \nhttp://iguanasuicide.net/ \\_/ \n"},{"id":"98327","messageId":"200812182129.01021.bss@iguanasuicide.net","threadId":"16793","inReplyTo":"7vej04eui5.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Make git revert warn the user when reverting a merge commit.","fromName":"Boyd Stephen Smith Jr.","fromEmail":"bss@iguanasuicide.net","sentAt":"2008-12-19T03:29:00Z","receivedAt":"2008-12-19T03:29:00Z","isPatch":true,"sender":{"key":"bss@iguanasuicide.net","avatar":"https://gravatar.com/avatar/84b95eeff194b816c1568b1339e63e4b229825298664a9037b9f1ec713ead1e3?d=mp&s=160"},"body":"On Thursday 2008 December 18 21:03:46 Junio C Hamano wrote:\n> Johannes Schindelin writes:\n> > \t\t\twarning(\"revert on a merge commit may not do what you \"\n> > \t\t\t\t\"expect.\");\n>\n> [T]he new warning does\n> not give you enough clue where to go next, so this warning does not give\n> real value. It is pretty much meaningless noise to users.\n\nAt least, it might make someone read the manpage again. Still, I'm unhappy \nwith the message, but I didn't want to be too wordy. A URL or manpage \nreference would be nice, but I didn't know of a good guide that explained the \ndangers of reverting a merge commit as well as Linus's emails.\n-- \nBoyd Stephen Smith Jr. ,= ,-_-. =. \nbss@iguanasuicide.net ((_/)o o(\\_))\nICQ: 514984 YM/AIM: DaTwinkDaddy `-'(. .)`-' \nhttp://iguanasuicide.net/ \\_/ \n"},{"id":"98331","messageId":"76718490812181955u5f56180en47b3a8268c3538bb@mail.gmail.com","threadId":"16793","inReplyTo":"200812182129.01021.bss@iguanasuicide.net","subject":"Re: [PATCH] Make git revert warn the user when reverting a merge commit.","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2008-12-19T03:55:13Z","receivedAt":"2008-12-19T03:55:13Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Thu, Dec 18, 2008 at 10:29 PM, Boyd Stephen Smith Jr.\n wrote:\n> At least, it might make someone read the manpage again. Still, I'm unhappy\n> with the message, but I didn't want to be too wordy. A URL or manpage\n> reference would be nice, but I didn't know of a good guide that explained the\n> dangers of reverting a merge commit as well as Linus's emails.\n\nPut his email in Documentation/howto/undoing-merge-commits.txt and\nreference that?\n\nj.\n"},{"id":"98338","messageId":"200812182354.16269.bss@iguanasuicide.net","threadId":"16793","inReplyTo":"76718490812181955u5f56180en47b3a8268c3538bb@mail.gmail.com","subject":"Re: [PATCH] Make git revert warn the user when reverting a merge commit.","fromName":"Boyd Stephen Smith Jr.","fromEmail":"bss@iguanasuicide.net","sentAt":"2008-12-19T05:54:12Z","receivedAt":"2008-12-19T05:54:12Z","isPatch":true,"sender":{"key":"bss@iguanasuicide.net","avatar":"https://gravatar.com/avatar/84b95eeff194b816c1568b1339e63e4b229825298664a9037b9f1ec713ead1e3?d=mp&s=160"},"body":"On Thursday 2008 December 18 21:55:13 Jay Soffian wrote:\n> On Thu, Dec 18, 2008 at 10:29 PM, Boyd Stephen Smith Jr.\n>\n> wrote:\n> > At least, it might make someone read the manpage again. Still, I'm\n> > unhappy with the message, but I didn't want to be too wordy. A URL or\n> > manpage reference would be nice, but I didn't know of a good guide that\n> > explained the dangers of reverting a merge commit as well as Linus's\n> > emails.\n>\n> Put his email in Documentation/howto/undoing-merge-commits.txt and\n> reference that?\n\nOkay, I've got a documentation patch brewing, but it's too late here to work \non it more. I'll post it over the weekend.\n\nIn addition, I think a one-time-per-user warning would be nice, but I'm not \nsure the best way to implement that. My initial thoughts would be reading a \nboolean config option, if unset/true issuing the warning and then if unset \nset it to false. However, that seems a bit... unclean and I fear there might \nbe a policy against writing ~/.gitconfig configuration options from a \nsubcommand other than 'git config'. Any suggestions on the implementation?\n-- \nBoyd Stephen Smith Jr. ,= ,-_-. =. \nbss@iguanasuicide.net ((_/)o o(\\_))\nICQ: 514984 YM/AIM: DaTwinkDaddy `-'(. .)`-' \nhttp://iguanasuicide.net/ \\_/ \n"},{"id":"98340","messageId":"7vljucd64b.fsf@gitster.siamese.dyndns.org","threadId":"16793","inReplyTo":"200812182354.16269.bss@iguanasuicide.net","subject":"Re: [PATCH] Make git revert warn the user when reverting a merge commit.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-19T06:35:48Z","receivedAt":"2008-12-19T06:35:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Boyd Stephen Smith Jr.\" writes:\n\n> In addition, I think a one-time-per-user warning would be nice, but I'm not \n> sure the best way to implement that. My initial thoughts would be reading a \n> boolean config option, if unset/true issuing the warning and then if unset \n> set it to false. However, that seems a bit... unclean and I fear there might \n> be a policy against writing ~/.gitconfig configuration options from a \n> subcommand other than 'git config'. Any suggestions on the implementation?\n\nAs an end user, I find one-time-per-user warning more frustrating than it\nis worth. I may see the warning issued for the first time of my using\ncertain feature, and because I am so novice to the program suite that I do\nnot fully understand what the warning is trying to say when I see it.\nThanks to the \"one-time-per-user\"-ness, that is the only chance for me to\nsee the message --- which often means that I won't see the warning before\nthe gravity of it has any chance to really sink in my mind.\n\n\"You can set i-know-what-i-am-doing in your ~/.xyzzyconfig file to squelch\nthis message\" is slightly better, as (1) I can control when I stop seeing\nit, and (2) because setting that in my config is done by me, as opposed to\nthe tool doing behind my back, it is much more likely for me to recall how\nto get the warning back when I choose to see it again.\n\nThe above discussion is \"in general\". In this particular case, I am not\nconvinced if the warning itself is worth it, though.\n"},{"id":"98364","messageId":"1229710058.5569.1.camel@rotwang.fnordora.org","threadId":"16793","inReplyTo":"200812182129.01021.bss@iguanasuicide.net","subject":"Re: [PATCH] Make git revert warn the user when reverting a merge commit.","fromName":"Alan","fromEmail":"alan@clueserver.org","sentAt":"2008-12-19T18:07:38Z","receivedAt":"2008-12-19T18:07:38Z","isPatch":true,"sender":{"key":"alan@clueserver.org","avatar":null},"body":"On Thu, 2008-12-18 at 21:29 -0600, Boyd Stephen Smith Jr. wrote:\n> On Thursday 2008 December 18 21:03:46 Junio C Hamano wrote:\n> > Johannes Schindelin writes:\n> > > \t\t\twarning(\"revert on a merge commit may not do what you \"\n> > > \t\t\t\t\"expect.\");\n> >\n> > [T]he new warning does\n> > not give you enough clue where to go next, so this warning does not give\n> > real value. It is pretty much meaningless noise to users.\n> \n> At least, it might make someone read the manpage again. Still, I'm unhappy \n> with the message, but I didn't want to be too wordy. A URL or manpage \n> reference would be nice, but I didn't know of a good guide that explained the \n> dangers of reverting a merge commit as well as Linus's emails.\n\nThat would be OK if the man page actually explained how this is supposed\nto work. it does not. (Especially where it concerns \"parent number\"\nand reverts of merges, which has no real explanation.)\n"},{"id":"98418","messageId":"200812200808.02011.robin.rosenberg.lists@dewire.com","threadId":"16793","inReplyTo":"200812182039.15169.bss@iguanasuicide.net","subject":"Re: [PATCH] Make git revert warn the user when reverting a merge commit.","fromName":"Robin Rosenberg","fromEmail":"robin.rosenberg.lists@dewire.com","sentAt":"2008-12-20T07:08:01Z","receivedAt":"2008-12-20T07:08:01Z","isPatch":true,"sender":{"key":"robin.rosenberg@dewire.com","avatar":"https://avatars.githubusercontent.com/u/46357?v=4"},"body":"fredag 19 december 2008 03:39:15 skrev Boyd Stephen Smith Jr.:\n> Signed-off-by: Boyd Stephen Smith Jr \n> ---\n> On Thursday 2008 December 18 18:21:25 Linus Torvalds wrote:\n> > I suspect we should warn about reverting merges.\n> \n\nOr mention the reverted parent in the commit message since it is not obvious.\n\n-- robin\n\n>From e982c8cefcdeefd6e8aabc8c354bed69161f40ee Mon Sep 17 00:00:00 2001\nFrom: Robin Rosenberg \nDate: Sat, 20 Dec 2008 07:22:39 +0100\nSubject: [PATCH] Mention reverted parent in commit message for reverted merge.\n\nSigned-off-by: Robin Rosenberg \n---\n builtin-revert.c | 4 ++++\n 1 files changed, 4 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin-revert.c b/builtin-revert.c\nindex 4038b41..fc59229 100644\n--- a/builtin-revert.c\n+++ b/builtin-revert.c\n@@ -352,6 +352,10 @@ static int revert_or_cherry_pick(int argc, const char **argv)\n \t\tadd_to_msg(oneline_body + 1);\n \t\tadd_to_msg(\"\\\"\\n\\nThis reverts commit \");\n \t\tadd_to_msg(sha1_to_hex(commit->object.sha1));\n+\t\tif (commit->parents->next) {\n+\t\t\tadd_to_msg(\" removing\\ncontributions from \");\n+\t\t\tadd_to_msg(sha1_to_hex(parent->object.sha1));\n+\t\t}\n \t\tadd_to_msg(\".\\n\");\n \t} else {\n \t\tbase = parent;\n-- \n1.6.1.rc3.36.g43d5.dirty\n"},{"id":"98461","messageId":"200812201654.23110.bss@iguanasuicide.net","threadId":"16793","inReplyTo":"200812200808.02011.robin.rosenberg.lists@dewire.com","subject":"Re: [PATCH] Make git revert warn the user when reverting a merge commit.","fromName":"Boyd Stephen Smith Jr.","fromEmail":"bss@iguanasuicide.net","sentAt":"2008-12-20T22:54:19Z","receivedAt":"2008-12-20T22:54:19Z","isPatch":true,"sender":{"key":"bss@iguanasuicide.net","avatar":"https://gravatar.com/avatar/84b95eeff194b816c1568b1339e63e4b229825298664a9037b9f1ec713ead1e3?d=mp&s=160"},"body":"On Saturday 2008 December 20 01:08:01 Robin Rosenberg wrote:\n> fredag 19 december 2008 03:39:15 skrev Boyd Stephen Smith Jr.:\n> > On Thursday 2008 December 18 18:21:25 Linus Torvalds wrote:\n> > > I suspect we should warn about reverting merges.\n>\n> Or mention the reverted parent in the commit message since it is not\n> obvious.\n>\n> ---\n> builtin-revert.c | 4 ++++\n> 1 files changed, 4 insertions(+), 0 deletions(-)\n>\n> diff --git a/builtin-revert.c b/builtin-revert.c\n> index 4038b41..fc59229 100644\n> --- a/builtin-revert.c\n> +++ b/builtin-revert.c\n> @@ -352,6 +352,10 @@ static int revert_or_cherry_pick(int argc, const char\n> **argv) add_to_msg(oneline_body + 1);\n> \t\tadd_to_msg(\"\\\"\\n\\nThis reverts commit \");\n> \t\tadd_to_msg(sha1_to_hex(commit->object.sha1));\n> +\t\tif (commit->parents->next) {\n> +\t\t\tadd_to_msg(\" removing\\ncontributions from \");\n> +\t\t\tadd_to_msg(sha1_to_hex(parent->object.sha1));\n> +\t\t}\n> \t\tadd_to_msg(\".\\n\");\n> \t} else {\n> \t\tbase = parent;\n\nI'm still new to the code, but parent is the \"mainline\" specified on the \ncommand-line, which (I think) is actually the parent to be reverted to, so we \nare actually removing contributions from all the *other* parents. So, the \nmessage may be backward. Because of that, I'd say the patch doesn't handle \noctopus merges well, either.\n-- \nBoyd Stephen Smith Jr. ,= ,-_-. =. \nbss@iguanasuicide.net ((_/)o o(\\_))\nICQ: 514984 YM/AIM: DaTwinkDaddy `-'(. .)`-' \nhttp://iguanasuicide.net/ \\_/ \n"},{"id":"98463","messageId":"200812210031.08443.robin.rosenberg.lists@dewire.com","threadId":"16793","inReplyTo":"200812201654.23110.bss@iguanasuicide.net","subject":"Re: [PATCH] Make git revert warn the user when reverting a merge commit.","fromName":"Robin Rosenberg","fromEmail":"robin.rosenberg.lists@dewire.com","sentAt":"2008-12-20T23:31:08Z","receivedAt":"2008-12-20T23:31:08Z","isPatch":true,"sender":{"key":"robin.rosenberg@dewire.com","avatar":"https://avatars.githubusercontent.com/u/46357?v=4"},"body":"lördag 20 december 2008 23:54:19 skrev Boyd Stephen Smith Jr.:\n> On Saturday 2008 December 20 01:08:01 Robin Rosenberg wrote:\n> > fredag 19 december 2008 03:39:15 skrev Boyd Stephen Smith Jr.:\n> > > On Thursday 2008 December 18 18:21:25 Linus Torvalds wrote:\n> > > > I suspect we should warn about reverting merges.\n> >\n> > Or mention the reverted parent in the commit message since it is not\n> > obvious.\n> >\n> > ---\n> > builtin-revert.c | 4 ++++\n> > 1 files changed, 4 insertions(+), 0 deletions(-)\n> >\n> > diff --git a/builtin-revert.c b/builtin-revert.c\n> > index 4038b41..fc59229 100644\n> > --- a/builtin-revert.c\n> > +++ b/builtin-revert.c\n> > @@ -352,6 +352,10 @@ static int revert_or_cherry_pick(int argc, const char\n> > **argv) add_to_msg(oneline_body + 1);\n> > \t\tadd_to_msg(\"\\\"\\n\\nThis reverts commit \");\n> > \t\tadd_to_msg(sha1_to_hex(commit->object.sha1));\n> > +\t\tif (commit->parents->next) {\n> > +\t\t\tadd_to_msg(\" removing\\ncontributions from \");\n> > +\t\t\tadd_to_msg(sha1_to_hex(parent->object.sha1));\n> > +\t\t}\n> > \t\tadd_to_msg(\".\\n\");\n> > \t} else {\n> > \t\tbase = parent;\n> \n> I'm still new to the code, but parent is the \"mainline\" specified on the \n> command-line, which (I think) is actually the parent to be reverted to, so we \n> are actually removing contributions from all the *other* parents. So, the \n> message may be backward. Because of that, I'd say the patch doesn't handle \n\nIndeed the message is backward. How about \"removing all contributions except from\"... etc ?\n\nAn alternative, would be \"removing changes relative to ..\" (mainline). The changes are\nthe contributions from all other parents. I have to huge interest in the exact phrase used.\n\n> octopus merges well, either.\n\nSame problem, I think.\n\n-- robin\n"},{"id":"98473","messageId":"7viqpetfs3.fsf@gitster.siamese.dyndns.org","threadId":"16793","inReplyTo":"200812210031.08443.robin.rosenberg.lists@dewire.com","subject":"Re: [PATCH] Make git revert warn the user when reverting a merge commit.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-21T02:37:16Z","receivedAt":"2008-12-21T02:37:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Robin Rosenberg writes:\n\n> An alternative, would be \"removing changes relative to ..\"\n> (mainline). The changes are the contributions from all other parents. I\n> have to huge interest in the exact phrase used.\n\nBut that is exactly what \"This reverts commit X\" means, isn't it?\n"},{"id":"98475","messageId":"200812202111.17831.bss@iguanasuicide.net","threadId":"16793","inReplyTo":"7viqpetfs3.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Make git revert warn the user when reverting a merge commit.","fromName":"Boyd Stephen Smith Jr.","fromEmail":"bss@iguanasuicide.net","sentAt":"2008-12-21T03:11:13Z","receivedAt":"2008-12-21T03:11:13Z","isPatch":true,"sender":{"key":"bss@iguanasuicide.net","avatar":"https://gravatar.com/avatar/84b95eeff194b816c1568b1339e63e4b229825298664a9037b9f1ec713ead1e3?d=mp&s=160"},"body":"On Saturday 2008 December 20 20:37:16 Junio C Hamano wrote:\n> Robin Rosenberg writes:\n> > An alternative, would be \"removing changes relative to ..\"\n> > (mainline).\n>\n> But that is exactly what \"This reverts commit X\" means, isn't it?\n\nWhen X is a merge commit, the phrase \"the reverts commit X\" is ambiguous. Did \nyou revert the tree to X^, X^2, or X^8? I'd be fine with \"This reverts \ncommit X to X^y\", but we definitely need some mention of X^y.\n-- \nBoyd Stephen Smith Jr. ,= ,-_-. =. \nbss@iguanasuicide.net ((_/)o o(\\_))\nICQ: 514984 YM/AIM: DaTwinkDaddy `-'(. .)`-' \nhttp://iguanasuicide.net/ \\_/ \n"},{"id":"98484","messageId":"200812211109.36788.robin.rosenberg.lists@dewire.com","threadId":"16793","inReplyTo":"200812202111.17831.bss@iguanasuicide.net","subject":"Re: [PATCH] Make git revert warn the user when reverting a merge commit.","fromName":"Robin Rosenberg","fromEmail":"robin.rosenberg.lists@dewire.com","sentAt":"2008-12-21T10:09:36Z","receivedAt":"2008-12-21T10:09:36Z","isPatch":true,"sender":{"key":"robin.rosenberg@dewire.com","avatar":"https://avatars.githubusercontent.com/u/46357?v=4"},"body":"söndag 21 december 2008 04:11:13 skrev Boyd Stephen Smith Jr.:\n> On Saturday 2008 December 20 20:37:16 Junio C Hamano wrote:\n> > Robin Rosenberg writes:\n> > > An alternative, would be \"removing changes relative to ..\"\n> > > (mainline).\n> >\n> > But that is exactly what \"This reverts commit X\" means, isn't it?\n> \n> When X is a merge commit, the phrase \"the reverts commit X\" is ambiguous. Did \n> you revert the tree to X^, X^2, or X^8? I'd be fine with \"This reverts \n> commit X to X^y\", but we definitely need some mention of X^y.\n\nOne could consider keeping the contributions from ^1 a special case and not\nmention the parent, making it look like any revert commit. I guess most merge\nreverts are like this in practice.\n\n-- robin\n"},{"id":"98485","messageId":"7v8wq9rdyl.fsf@gitster.siamese.dyndns.org","threadId":"16793","inReplyTo":"200812211109.36788.robin.rosenberg.lists@dewire.com","subject":"Re: [PATCH] Make git revert warn the user when reverting a merge commit.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-21T10:59:30Z","receivedAt":"2008-12-21T10:59:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Robin Rosenberg writes:\n\n> One could consider keeping the contributions from ^1 a special case and not\n> mention the parent, making it look like any revert commit. I guess most merge\n> reverts are like this in practice.\n\nI think that makes sense. There are cases where the mainline maintainer\npunts a merge and pass the baton to a subsystem maintainer, saying \"Your\ntree has many conflicts with my tip, and I'd rather ask you to resolve it\"\n(and after such a merge, the mainline maintainer will fast forward to the\nresult), in which case the merge will be in the reverse direction, but\nthat should be rare. Reverting such a merge later from the mainline's\npoint of view would involve \"revert -m 2\".\n\nSo if your patch is tightened a bit to record extra information only in\nsuch a case, I think that would be an acceptable approach to the issue.\n"},{"id":"98497","messageId":"200812211359.31991.bss@iguanasuicide.net","threadId":"16793","inReplyTo":"200812211109.36788.robin.rosenberg.lists@dewire.com","subject":"Re: [PATCH] Make git revert warn the user when reverting a merge commit.","fromName":"Boyd Stephen Smith Jr.","fromEmail":"bss@iguanasuicide.net","sentAt":"2008-12-21T19:59:14Z","receivedAt":"2008-12-21T19:59:14Z","isPatch":true,"sender":{"key":"bss@iguanasuicide.net","avatar":"https://gravatar.com/avatar/84b95eeff194b816c1568b1339e63e4b229825298664a9037b9f1ec713ead1e3?d=mp&s=160"},"body":"On Sunday 2008 December 21 04:09:36 Robin Rosenberg wrote:\n> söndag 21 december 2008 04:11:13 skrev Boyd Stephen Smith Jr.:\n> > On Saturday 2008 December 20 20:37:16 Junio C Hamano wrote:\n> > > Robin Rosenberg writes:\n> > > > An alternative, would be \"removing changes relative to ..\"\n> > > > (mainline).\n> > >\n> > > But that is exactly what \"This reverts commit X\" means, isn't it?\n> >\n> > When X is a merge commit, the phrase \"the reverts commit X\" is ambiguous.\n> > Did you revert the tree to X^, X^2, or X^8? I'd be fine with \"This\n> > reverts commit X to X^y\", but we definitely need some mention of X^y.\n>\n> One could consider keeping the contributions from ^1 a special case and not\n> mention the parent, making it look like any revert commit. I guess most\n> merge reverts are like this in practice.\n\nThen why not have \"-m 1\" be assumed instead of forcing the user to specify it? \nIf we force the user to specify that information, shouldn't we hold the code \nto the same standard and have it output a message with that information?\n\nI think git should mention the parent to which we reverted whenever there are \nmultiple parents.\n-- \nBoyd Stephen Smith Jr. ,= ,-_-. =. \nbss@iguanasuicide.net ((_/)o o(\\_))\nICQ: 514984 YM/AIM: DaTwinkDaddy `-'(. .)`-' \nhttp://iguanasuicide.net/ \\_/ \n"},{"id":"98498","messageId":"7vwsdtmg5m.fsf@gitster.siamese.dyndns.org","threadId":"16793","inReplyTo":"200812211359.31991.bss@iguanasuicide.net","subject":"Re: [PATCH] Make git revert warn the user when reverting a merge commit.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-21T20:23:17Z","receivedAt":"2008-12-21T20:23:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Boyd Stephen Smith Jr.\" writes:\n\n> Then why not have \"-m 1\" be assumed instead of forcing the user to specify it? \n\nThe reason we don't is because until very recently we did not even allow\nyou to revert a merge relative to any parent. We wanted to avoid\nsurprising people who are _relying on_ that behaviour to make sure that\nthey do not revert a merge by accident.\n\nWe could certainly do what you suggest to imply \"-m 1\" when the commit\nrequested to be reverted happens to be a merge, but we shouldn't be doing\nthat without thinking things through. It will break people's longstanding\nexpectations.\n"},{"id":"98499","messageId":"200812211513.26808.bss@iguanasuicide.net","threadId":"16793","inReplyTo":"7vwsdtmg5m.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Make git revert warn the user when reverting a merge commit.","fromName":"Boyd Stephen Smith Jr.","fromEmail":"bss@iguanasuicide.net","sentAt":"2008-12-21T21:13:22Z","receivedAt":"2008-12-21T21:13:22Z","isPatch":true,"sender":{"key":"bss@iguanasuicide.net","avatar":"https://gravatar.com/avatar/84b95eeff194b816c1568b1339e63e4b229825298664a9037b9f1ec713ead1e3?d=mp&s=160"},"body":"On Sunday 2008 December 21 14:23:17 Junio C Hamano wrote:\n> \"Boyd Stephen Smith Jr.\" writes:\n> > Then why not have \"-m 1\" be assumed instead of forcing the user to\n> > specify it?\n>\n> The reason we don't is because until very recently we did not even allow\n> you to revert a merge relative to any parent. We wanted to avoid\n> surprising people who are _relying on_ that behaviour to make sure that\n> they do not revert a merge by accident.\n\nThat makes sense.\n\n> We could certainly do what you suggest to imply \"-m 1\" when the commit\n> requested to be reverted happens to be a merge, but we shouldn't be doing\n> that without thinking things through. It will break people's longstanding\n> expectations.\n\nI wasn't really suggesting that. I was pointing out what I thought was an \ninconsistency: making the user specify the parent but not making the commit \nmessage specify the parent.\n\nI still think the parent we are reverting to should be mentioned in the \nautomatically generated commit message, even if it is the first parent. Even \nif it is decided to elide that information for the first parent, I agree \nthat, at least for now, the \"-m\" should still be required when reverting a \nmerge commit.\n-- \nBoyd Stephen Smith Jr. ,= ,-_-. =. \nbss@iguanasuicide.net ((_/)o o(\\_))\nICQ: 514984 YM/AIM: DaTwinkDaddy `-'(. .)`-' \nhttp://iguanasuicide.net/ \\_/ \n"},{"id":"98511","messageId":"7vprjlkwbb.fsf@gitster.siamese.dyndns.org","threadId":"16793","inReplyTo":"200812211513.26808.bss@iguanasuicide.net","subject":"Re: [PATCH] Make git revert warn the user when reverting a merge commit.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-21T22:17:12Z","receivedAt":"2008-12-21T22:17:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Robin Rosenberg \nSubject: git-revert: record the parent against which a revert was made\n\nAs described in Documentation/howto/revert-a-faulty-merge.txt, re-merging\nfrom a previously reverted a merge of a side branch may need a revert of\nthe revert beforehand. Record against which parent the revert was made in\nthe commit, so that later the user can figure out what went on.\n\n[jc: original had the logic in the message reversed, so I tweaked it.]\n\nSigned-off-by: Robin Rosenberg \nSigned-off-by: Junio C Hamano \n---\n \"Boyd Stephen Smith Jr.\" writes:\n\n > I still think the parent we are reverting to should be mentioned in the \n > automatically generated commit message, even if it is the first parent. Even \n > if it is decided to elide that information for the first parent, I agree \n > that, at least for now, the \"-m\" should still be required when reverting a \n > merge commit.\n\n Ok, so here is Robin's patch with a bit of rewording. I want to have\n something usable now, so that I can tag -rc4 and still have time left\n for sipping my Caipirinha in the evening ;-)\n\n I think we later _could_ use this information inside ancestry traversal\n made while computing the merge base in such a way to eliminate the\n necessity of the \"revert of the revert\". When we see a message that\n records a revert of a merge, we keep a mental note of it, and when we\n encounter such a merge during the ancestry traversal, we pretend as if\n the merge never happened (i.e. instead we traverse only the named\n parent).\n\n But that needs more thought, and we do not have to do that now.\n\n builtin-revert.c | 5 +++++\n 1 files changed, 5 insertions(+), 0 deletions(-)\n\ndiff --git c/builtin-revert.c w/builtin-revert.c\nindex 4038b41..fae0fe8 100644\n--- c/builtin-revert.c\n+++ w/builtin-revert.c\n@@ -352,6 +352,11 @@ static int revert_or_cherry_pick(int argc, const char **argv)\n \t\tadd_to_msg(oneline_body + 1);\n \t\tadd_to_msg(\"\\\"\\n\\nThis reverts commit \");\n \t\tadd_to_msg(sha1_to_hex(commit->object.sha1));\n+\n+\t\tif (commit->parents->next) {\n+\t\t\tadd_to_msg(\",\\nreverting damages made to %s\");\n+\t\t\tadd_to_msg(sha1_to_hex(parent->object.sha1));\n+\t\t}\n \t\tadd_to_msg(\".\\n\");\n \t} else {\n \t\tbase = parent;\n"},{"id":"98516","messageId":"7vhc4xkvb6.fsf@gitster.siamese.dyndns.org","threadId":"16793","inReplyTo":"7vprjlkwbb.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Make git revert warn the user when reverting a merge commit.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-21T22:38:53Z","receivedAt":"2008-12-21T22:38:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Ok, so here is Robin's patch with a bit of rewording. I want to have\n> something usable now, so that I can tag -rc4 and still have time left\n> for sipping my Caipirinha in the evening ;-)\n> ...\n> +\t\t\tadd_to_msg(\",\\nreverting damages made to %s\");\n> +\t\t\tadd_to_msg(sha1_to_hex(parent->object.sha1));\n\nCrap. Scratch that. Obviously I should have done this:\n\ndiff --git a/builtin-revert.c b/builtin-revert.c\nindex 4038b41..c188150 100644\n--- a/builtin-revert.c\n+++ b/builtin-revert.c\n@@ -352,6 +352,11 @@ static int revert_or_cherry_pick(int argc, const char **argv)\n \t\tadd_to_msg(oneline_body + 1);\n \t\tadd_to_msg(\"\\\"\\n\\nThis reverts commit \");\n \t\tadd_to_msg(sha1_to_hex(commit->object.sha1));\n+\n+\t\tif (commit->parents->next) {\n+\t\t\tadd_to_msg(\",\\nreverting damages made to \");\n+\t\t\tadd_to_msg(sha1_to_hex(parent->object.sha1));\n+\t\t}\n \t\tadd_to_msg(\".\\n\");\n \t} else {\n \t\tbase = parent;\n-- \n1.6.1.rc3.72.gf4bf6\n"},{"id":"98517","messageId":"200812212340.46375.robin.rosenberg.lists@dewire.com","threadId":"16793","inReplyTo":"7vprjlkwbb.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Make git revert warn the user when reverting a merge commit.","fromName":"Robin Rosenberg","fromEmail":"robin.rosenberg.lists@dewire.com","sentAt":"2008-12-21T22:40:45Z","receivedAt":"2008-12-21T22:40:45Z","isPatch":true,"sender":{"key":"robin.rosenberg@dewire.com","avatar":"https://avatars.githubusercontent.com/u/46357?v=4"},"body":"söndag 21 december 2008 23:17:12 skrev Junio C Hamano:\n> From: Robin Rosenberg \n> Subject: git-revert: record the parent against which a revert was made\n> \n> As described in Documentation/howto/revert-a-faulty-merge.txt, re-merging\n> from a previously reverted a merge of a side branch may need a revert of\n> the revert beforehand. Record against which parent the revert was made in\n> the commit, so that later the user can figure out what went on.\n> \n> [jc: original had the logic in the message reversed, so I tweaked it.]\nNo need for this comment.\n\n> +\t\t\tadd_to_msg(\",\\nreverting damages made to %s\");\nmaybe \"changes\" is more neutrral language. I also think you break\nthe line too early.\n\nAre we done now?\n\n-- robin\n"},{"id":"98518","messageId":"7vd4flkuy2.fsf@gitster.siamese.dyndns.org","threadId":"16793","inReplyTo":"200812212340.46375.robin.rosenberg.lists@dewire.com","subject":"Re: [PATCH] Make git revert warn the user when reverting a merge commit.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-21T22:46:45Z","receivedAt":"2008-12-21T22:46:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Robin Rosenberg writes:\n\n> söndag 21 december 2008 23:17:12 skrev Junio C Hamano:\n>> From: Robin Rosenberg \n>> Subject: git-revert: record the parent against which a revert was made\n>> \n>> As described in Documentation/howto/revert-a-faulty-merge.txt, re-merging\n>> from a previously reverted a merge of a side branch may need a revert of\n>> the revert beforehand. Record against which parent the revert was made in\n>> the commit, so that later the user can figure out what went on.\n>> \n>> [jc: original had the logic in the message reversed, so I tweaked it.]\n> No need for this comment.\n\nOk.\n\n>> +\t\t\tadd_to_msg(\",\\nreverting damages made to %s\");\n> maybe \"changes\" is more neutrral language. I also think you break\n> the line too early.\n\nThe above (without %s which shouldn't have been there) would give: \n\n This reverts commit efe05b019ca19328d27c07ef32b4698a7f36166f,\n reverting damages made to ec9f0ea3e6ecf1237223dec8428e7bb73d339320.\n \nDo you want:\n\n This reverts commit efe05b019ca19328d27c07ef32b4698a7f36166f, reversing\n changes made to ec9f0ea3e6ecf1237223dec8428e7bb73d339320.\n\nthis instead?\n"},{"id":"98519","messageId":"200812212356.33434.robin.rosenberg.lists@dewire.com","threadId":"16793","inReplyTo":"7vd4flkuy2.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Make git revert warn the user when reverting a merge commit.","fromName":"Robin Rosenberg","fromEmail":"robin.rosenberg.lists@dewire.com","sentAt":"2008-12-21T22:56:33Z","receivedAt":"2008-12-21T22:56:33Z","isPatch":true,"sender":{"key":"robin.rosenberg@dewire.com","avatar":"https://avatars.githubusercontent.com/u/46357?v=4"},"body":"söndag 21 december 2008 23:46:45 skrev Junio C Hamano:\n> Do you want:\n> \n> This reverts commit efe05b019ca19328d27c07ef32b4698a7f36166f, reversing\n> changes made to ec9f0ea3e6ecf1237223dec8428e7bb73d339320.\n\nYes, it fills the paragraph nicely. It is a normal text flow after all.\n\n-- robin\n"}]}