{"thread":{"id":"20874","subject":"Improving merge failure message","startedAt":"2009-09-08T06:31:01Z","lastAt":"2009-09-08T16:34:39Z","messageCount":11,"participants":["Nanako Shiraishi","Junio C Hamano","Sverre Rabbelier","Jeff King","Mike Ralphson","Johannes Sixt","Alex Riesen"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"122667","messageId":"20090908153101.6117@nanako3.lavabit.com","threadId":"20874","inReplyTo":null,"subject":"Improving merge failure message","fromName":"Nanako Shiraishi","fromEmail":"nanako3@lavabit.com","sentAt":"2009-09-08T06:31:01Z","receivedAt":"2009-09-08T06:31:01Z","isPatch":false,"sender":{"key":"nanako3@lavabit.com","avatar":"https://gravatar.com/avatar/3777b9e201c5883a62b1a6fdf7c53f2d712d1d80989146063ea861e33aad72a8?d=mp&s=160"},"body":"I often see my students confused after a failed merge and they can't\nfigure out what to do next. Two typical error messages they get are\n\n[1]% git merge master\nAuto-merging cool\nCONFLICT (content): Merge conflict in cool\nAutomatic merge failed; fix conflicts and then commit the result.\n\n[2]% git merge feature\nerror: Entry 'cool' not uptodate. Cannot merge.\nfatal: merging of trees 8ec1d96451ff05451720e4e8968812c46b35e5e4 and aad8d5cef3915ab78b3227abaaac99b62db9eb54 failed\n\nIn the former case, the merge command gives a helpful message that\nautomatic merge failed because it found a conflict and tells enough\ninstruction to the user.\n\nBut in the latter case, the messages look unnecessarily scary, with two\n\"error\" and \"fatal\" comments, and long sha1 commit names.\n\nThose of us who used git for some time can tell what it wants to say.\nThe merge checked the files in the working tree before doing anything,\nfound that the user has uncommitted change to a file that is involved in\nthe merge, and it stopped. And it didn't change anything. It may be \"fatal\"\nbut the user has much less reason to be scared about this failure than\nthe conflicting case.\n\nIt would be nice if the message in the latter case can be toned down.\n\n-- \nNanako Shiraishi\nhttp://ivory.ap.teacup.com/nanako3/\n"},{"id":"122669","messageId":"7vbplmhr0i.fsf@alter.siamese.dyndns.org","threadId":"20874","inReplyTo":"20090908153101.6117@nanako3.lavabit.com","subject":"Re: Improving merge failure message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-09-08T06:47:25Z","receivedAt":"2009-09-08T06:47:25Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nanako Shiraishi writes:\n\n> [2]% git merge feature\n> error: Entry 'cool' not uptodate. Cannot merge.\n> fatal: merging of trees 8ec1d96451ff05451720e4e8968812c46b35e5e4 and aad8d5cef3915ab78b3227abaaac99b62db9eb54 failed\n>\n> ... the messages look unnecessarily scary, with two\n> \"error\" and \"fatal\" comments, and long sha1 commit names.\n\nJust a technical nit. I think these are tree object names.\n\n> Those of us who used git for some time can tell what it wants to say.\n> The merge checked the files in the working tree before doing anything,\n> found that the user has uncommitted change to a file that is involved in\n> the merge, and it stopped. And it didn't change anything. It may be \"fatal\"\n> but the user has much less reason to be scared about this failure than\n> the conflicting case.\n>\n> It would be nice if the message in the latter case can be toned down.\n\nYeah, it would be nice. This actually was something that bothered me as\nwell while trying to explain the recovery procedure for these two cases.\nGive me half an hour or so to cook up something...\n"},{"id":"122678","messageId":"7veiqhgb4y.fsf@alter.siamese.dyndns.org","threadId":"20874","inReplyTo":"7vbplmhr0i.fsf@alter.siamese.dyndns.org","subject":"Re: Improving merge failure message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-09-08T07:15:41Z","receivedAt":"2009-09-08T07:15:41Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Nanako Shiraishi writes:\n>\n>> [2]% git merge feature\n>> error: Entry 'cool' not uptodate. Cannot merge.\n>> fatal: merging of trees 8ec1d96451ff05451720e4e8968812c46b35e5e4 and aad8d5cef3915ab78b3227abaaac99b62db9eb54 failed\n>>\n>> ... the messages look unnecessarily scary, with two\n>> \"error\" and \"fatal\" comments, and long sha1 commit names.\n>\n> Just a technical nit. I think these are tree object names.\n>\n>> Those of us who used git for some time can tell what it wants to say.\n>> The merge checked the files in the working tree before doing anything,\n>> found that the user has uncommitted change to a file that is involved in\n>> the merge, and it stopped. And it didn't change anything. It may be \"fatal\"\n>> but the user has much less reason to be scared about this failure than\n>> the conflicting case.\n>>\n>> It would be nice if the message in the latter case can be toned down.\n>\n> Yeah, it would be nice. This actually was something that bothered me as\n> well while trying to explain the recovery procedure for these two cases.\n> Give me half an hour or so to cook up something...\n\nIt turns out to be a lot simpler than I thought, because 8ccba00\n(unpack-trees: allow Porcelain to give different error messages,\n2008-05-17) already laid enough groundwork for doing this kind of thing\neasily.\n\nNotable points are:\n\n - End the messages with \"Aborting.\"; they are given when the three-way\n merge stops without harming the work tree;\n\n - Do not give the extra message after unpack_trees() already errored out.\n This \"merging of trees failed\" message was primarily for debugging\n merge-recursive itself, and the end user cannot do much with the object\n names given in the message anyway.\n\n But do give it under higher verbosity level, or when it happens during\n the inner merge (the \"recursive\" one), as unpack_trees() should not\n fail for the inner merge under normal conditions.\n\nWe could later add instructions on how to recover (i.e. \"stash changes\naway or commit on a side branch and retry\") instead of the silent\nexit(128) I have down there, and then use Peff's advice.* mechanism to\nsquelch it (e.g. \"advice.mergeindirtytree\"), but they are separate topics.\n\n merge-recursive.c | 25 +++++++++++++++++++++----\n 1 files changed, 21 insertions(+), 4 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 10d7913..a237240 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -170,6 +170,18 @@ static int git_merge_trees(int index_only,\n \tint rc;\n \tstruct tree_desc t[3];\n \tstruct unpack_trees_options opts;\n+\tstatic struct unpack_trees_error_msgs msgs = {\n+\t\t/* would_overwrite */\n+\t\t\"Your local changes to '%s' will be clobbered by merge. Aborting.\",\n+\t\t/* not_uptodate_file */\n+\t\t\"Your local changes to '%s' will be clobbered by merge. Aborting.\",\n+\t\t/* not_uptodate_dir */\n+\t\t\"Updating '%s' would lose untracked files in it. Aborting.\",\n+\t\t/* would_lose_untracked */\n+\t\t\"Untracked working tree file '%s' would be %s by merge. Aborting\",\n+\t\t/* bind_overlap -- will not happen here */\n+\t\tNULL,\n+\t};\n \n \tmemset(&opts, 0, sizeof(opts));\n \tif (index_only)\n@@ -181,6 +193,7 @@ static int git_merge_trees(int index_only,\n \topts.fn = threeway_merge;\n \topts.src_index = &the_index;\n \topts.dst_index = &the_index;\n+\topts.msgs = msgs;\n \n \tinit_tree_desc_from_tree(t+0, common);\n \tinit_tree_desc_from_tree(t+1, head);\n@@ -1188,10 +1201,14 @@ int merge_trees(struct merge_options *o,\n \n \tcode = git_merge_trees(o->call_depth, common, head, merge);\n \n-\tif (code != 0)\n-\t\tdie(\"merging of trees %s and %s failed\",\n-\t\t sha1_to_hex(head->object.sha1),\n-\t\t sha1_to_hex(merge->object.sha1));\n+\tif (code != 0) {\n+\t\tif (show(o, 4) || o->call_depth)\n+\t\t\tdie(\"merging of trees %s and %s failed\",\n+\t\t\t sha1_to_hex(head->object.sha1),\n+\t\t\t sha1_to_hex(merge->object.sha1));\n+\t\telse\n+\t\t\texit(128);\n+\t}\n \n \tif (unmerged_cache()) {\n \t\tstruct string_list *entries, *re_head, *re_merge;\n"},{"id":"122679","messageId":"fabb9a1e0909080020i3daa92aar952fc5102df62bdb@mail.gmail.com","threadId":"20874","inReplyTo":"7veiqhgb4y.fsf@alter.siamese.dyndns.org","subject":"Re: Improving merge failure message","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2009-09-08T07:20:07Z","receivedAt":"2009-09-08T07:20:07Z","isPatch":false,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Tue, Sep 8, 2009 at 09:15, Junio C Hamano wrote:\n> +               /* would_overwrite */\n> +               \"Your local changes to '%s' will be clobbered by merge.  Aborting.\",\n\nStill scary, shouldn't that be s/will be/would be/ ?\n\n> +               /* not_uptodate_file */\n> +               \"Your local changes to '%s' will be clobbered by merge.  Aborting.\",\n\nDitto.\n\n> +               /* not_uptodate_dir */\n> +               \"Updating '%s' would lose untracked files in it.  Aborting.\",\n\nNot sure, but maybe s/in it//\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"122681","messageId":"7vk509ev20.fsf@alter.siamese.dyndns.org","threadId":"20874","inReplyTo":"fabb9a1e0909080020i3daa92aar952fc5102df62bdb@mail.gmail.com","subject":"Re: Improving merge failure message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-09-08T07:48:23Z","receivedAt":"2009-09-08T07:48:23Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sverre Rabbelier writes:\n\n> On Tue, Sep 8, 2009 at 09:15, Junio C Hamano wrote:\n>> + /* would_overwrite */\n>> + \"Your local changes to '%s' will be clobbered by merge. Aborting.\",\n>\n> Still scary, shouldn't that be s/will be/would be/ ?\n\nThanks, very true indeed. \"It would be clobbered if we were to continue\nhence we abort.\" is how we want to explain our behaviour, so \"would\" is\ndefinitely better here.\n\n>> + /* not_uptodate_dir */\n>> + \"Updating '%s' would lose untracked files in it. Aborting.\",\n\nThis is \"merge would resolve to have a file X, but you have a directory X\nin your work tree and it is not empty\" case.\n\nI'll leave the exact wording up to other people. My primary focus was to\nend all of these messages with \"Aborting.\"\n\nThis turns out to be a continuation of an older discussion thread back in\nMay 2008, and I do not know if anybody took up the challenge back then. I\nwouldn't be surprised if \"checkout\", which was the topic of the old\nthread, has some other scary plumbing message still seeping through to the\nUI layer. Perhaps there are some other commands that needs similar kind\nof love.\n"},{"id":"122682","messageId":"20090908075156.GA19294@coredump.intra.peff.net","threadId":"20874","inReplyTo":"7vk509ev20.fsf@alter.siamese.dyndns.org","subject":"Re: Improving merge failure message","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-09-08T07:51:56Z","receivedAt":"2009-09-08T07:51:56Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 08, 2009 at 12:48:23AM -0700, Junio C Hamano wrote:\n\n> Thanks, very true indeed. \"It would be clobbered if we were to continue\n> hence we abort.\" is how we want to explain our behaviour, so \"would\" is\n> definitely better here.\n\nWhile we're picking apart your wording, is \"clobbered\" the word we want\nto use? Everywhere else that is user-facing we tend to use the term\n\"overwritten\".\n\n-Peff\n"},{"id":"122684","messageId":"e2b179460909080059x62eb9d30t4d723d185e6316e3@mail.gmail.com","threadId":"20874","inReplyTo":"7vk509ev20.fsf@alter.siamese.dyndns.org","subject":"Re: Improving merge failure message","fromName":"Mike Ralphson","fromEmail":"mike.ralphson@gmail.com","sentAt":"2009-09-08T07:59:53Z","receivedAt":"2009-09-08T07:59:53Z","isPatch":false,"sender":{"key":"mike.ralphson@gmail.com","avatar":"https://avatars.githubusercontent.com/u/21603?v=4"},"body":"2009/9/8 Junio C Hamano :\n> I'll leave the exact wording up to other people.  My primary focus was to\n> end all of these messages with \"Aborting.\"\n>\n> This turns out to be a continuation of an older discussion thread back in\n> May 2008, and I do not know if anybody took up the challenge back then.  I\n> wouldn't be surprised if \"checkout\", which was the topic of the old\n> thread, has some other scary plumbing message still seeping through to the\n> UI layer.  Perhaps there are some other commands that needs similar kind\n> of love.\n\nJust a note that Documentation/git-checkout.txt references this\nmessage in an example and should be kept in step with the final\nwording change. It would be ideal to be able to regression-test the\nexamples in the documentation somehow but that might involve abusing\nthe asciidoc markup somewhat.\n\n2009/9/8 Jeff King :\n> While we're picking apart your wording, is \"clobbered\" the word we want\n> to use?\n\nIf we're debating 'clobbered', then maybe the non-word 'uptodate' is\nfair game too? 8-)\n"},{"id":"122685","messageId":"4AA614A2.2060908@viscovery.net","threadId":"20874","inReplyTo":"7vbplmhr0i.fsf@alter.siamese.dyndns.org","subject":"Re: Improving merge failure message","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-09-08T08:24:02Z","receivedAt":"2009-09-08T08:24:02Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Junio C Hamano schrieb:\n> Nanako Shiraishi writes:\n> \n>> [2]% git merge feature\n>> error: Entry 'cool' not uptodate. Cannot merge.\n>> fatal: merging of trees 8ec1d96451ff05451720e4e8968812c46b35e5e4 and aad8d5cef3915ab78b3227abaaac99b62db9eb54 failed\n>>\n>> ... the messages look unnecessarily scary, with two\n>> \"error\" and \"fatal\" comments, and long sha1 commit names.\n> \n>> It would be nice if the message in the latter case can be toned down.\n> \n> Yeah, it would be nice. This actually was something that bothered me as\n> well while trying to explain the recovery procedure for these two cases.\n> Give me half an hour or so to cook up something...\n\nI think that this is a symptom of a much more involved issue about error\nhandling.\n\nCurrently, it is customary in the code to report an error at each location\nwhere an exceptional condition is detected:\n\n\tif (frotz())\n\t\tret = error(\"frotz messed up\");\n\nIf frotz() is a \"low-level\" routine, like a C library function, then this\nis the right thing to do. However, if frotz() is our own function\n(\"high-level\") which itself calls low-level functions with the same\npattern, then this is obviously the wrong thing to do, because it results\ninto two error messages.\n\nWe need a guideline how errors are reported. This guideline could be:\n\n(1) Low-level functions do not print error messages, but set an error code\nand leave the reporting to the caller.\n\n(2) High-level functions must print an error message (through error() or\ndie()) when they detect a failure of a low-level function that was called\ndirectly.\n\n(3) High-level functions must not print an error message when they detect\na failure of another high-level function that was called directly.\n\nThere remains to classify our functions into high-level or low-level. But\nI think we won't have a lot of low-level functions.\n\nBTW, I applied this guideline when I worked on {start,finish,run}_command\nrecently, and IMHO it worked out quite nicely because exactly one error is\nreported after a failure.\n\n-- Hannes\n"},{"id":"122689","messageId":"81b0412b0909080211k19470327x97b19eb0bd6ab700@mail.gmail.com","threadId":"20874","inReplyTo":"7veiqhgb4y.fsf@alter.siamese.dyndns.org","subject":"Re: Improving merge failure message","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2009-09-08T09:11:24Z","receivedAt":"2009-09-08T09:11:24Z","isPatch":false,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On Tue, Sep 8, 2009 at 09:15, Junio C Hamano wrote:\n> @@ -170,6 +170,18 @@ static int git_merge_trees(int index_only,\n>        int rc;\n>        struct tree_desc t[3];\n>        struct unpack_trees_options opts;\n> +       static struct unpack_trees_error_msgs msgs = {\n\nYou can make these const, the struct is copied anyway:\n\n> +       opts.msgs = msgs;\n\nHere --^\n"},{"id":"122693","messageId":"20090908191237.6117@nanako3.lavabit.com","threadId":"20874","inReplyTo":"7veiqhgb4y.fsf@alter.siamese.dyndns.org","subject":"Re: Improving merge failure message","fromName":"Nanako Shiraishi","fromEmail":"nanako3@lavabit.com","sentAt":"2009-09-08T10:12:37Z","receivedAt":"2009-09-08T10:12:37Z","isPatch":false,"sender":{"key":"nanako3@lavabit.com","avatar":"https://gravatar.com/avatar/3777b9e201c5883a62b1a6fdf7c53f2d712d1d80989146063ea861e33aad72a8?d=mp&s=160"},"body":"Quoting Junio C Hamano \n\n> Notable points are:\n>\n> - End the messages with \"Aborting.\"; they are given when the three-way\n> merge stops without harming the work tree;\n>\n> - Do not give the extra message after unpack_trees() already errored out.\n> This \"merging of trees failed\" message was primarily for debugging\n> merge-recursive itself, and the end user cannot do much with the object\n> names given in the message anyway.\n>\n> But do give it under higher verbosity level, or when it happens during\n> the inner merge (the \"recursive\" one), as unpack_trees() should not\n> fail for the inner merge under normal conditions.\n>\n> We could later add instructions on how to recover (i.e. \"stash changes\n> away or commit on a side branch and retry\") instead of the silent\n> exit(128) I have down there, and then use Peff's advice.* mechanism to\n> squelch it (e.g. \"advice.mergeindirtytree\"), but they are separate topics.\n\nThank you for a quick response. The patch works fine here, so if you want\nplease add:\n\nTested-by: Nanako Shiraishi \n\n-- \nNanako Shiraishi\nhttp://ivory.ap.teacup.com/nanako3/\n"},{"id":"122713","messageId":"7vzl958kf4.fsf@alter.siamese.dyndns.org","threadId":"20874","inReplyTo":"e2b179460909080059x62eb9d30t4d723d185e6316e3@mail.gmail.com","subject":"Re: Improving merge failure message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-09-08T16:34:39Z","receivedAt":"2009-09-08T16:34:39Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Ralphson writes:\n\n> 2009/9/8 Junio C Hamano :\n>> I'll leave the exact wording up to other people. My primary focus was to\n>> end all of these messages with \"Aborting.\"\n>>\n>> This turns out to be a continuation of an older discussion thread back in\n>> May 2008, and I do not know if anybody took up the challenge back then. I\n>> wouldn't be surprised if \"checkout\", which was the topic of the old\n>> thread, has some other scary plumbing message still seeping through to the\n>> UI layer. Perhaps there are some other commands that needs similar kind\n>> of love.\n>\n> Just a note that Documentation/git-checkout.txt references this\n> message in an example and should be kept in step with the final\n> wording change. It would be ideal to be able to regression-test the\n> examples in the documentation somehow but that might involve abusing\n> the asciidoc markup somewhat.\n\nThanks.\n\n> 2009/9/8 Jeff King :\n>> While we're picking apart your wording, is \"clobbered\" the word we want\n>> to use?\n>\n> If we're debating 'clobbered', then maybe the non-word 'uptodate' is\n> fair game too? 8-)\n\nI think you added smiley because you already knew the answer, but if that\nis not the case, please see\n\n http://thread.gmane.org/gmane.comp.version-control.git/81100/focus=82358\n"}]}