{"thread":{"id":"33065","subject":"auto merge bug","startedAt":"2013-03-04T16:46:48Z","lastAt":"2013-03-06T09:15:15Z","messageCount":9,"participants":["David Krmpotic","Jeff King","Junio C Hamano","Andreas Ericsson"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"210580","messageId":"CAOFaZ+5F1BcWNU=AkcnS53bQt1VfAfsFjp9EvRCL=7kYiU1ejg@mail.gmail.com","threadId":"33065","inReplyTo":null,"subject":"auto merge bug","fromName":"David Krmpotic","fromEmail":"david.krmpotic@gmail.com","sentAt":"2013-03-04T16:46:48Z","receivedAt":"2013-03-04T16:46:48Z","isPatch":false,"sender":{"key":"david.krmpotic@gmail.com","avatar":"https://gravatar.com/avatar/e9f39032d6691c1688fa3a041f521b7a9e19fc1f1980ba29bc40dce9cabacfd9?d=mp&s=160"},"body":"Hi!\n\nWe started working on a .NET app and the XML project file (.csproj)\ngot corrupted (a few closing tag missing).\n\n79\t \n80\t SlovaricaForm.cs\n81\t+ \n82\t+ Form\n83\t+ \n84\t+ \n85\t+ WebCamForm.cs\n86\t \n\nbetween lines 80 and 81 there should be \n\nsimilarly:\n\n121\t \n122\t \n123\t SlovaricaForm.cs\n124\t+ \n125\t+ WebCamForm.cs\n126\t \n127\t \n128\t WordsSelectForm.cs\n\nbetween 123 and 124 there is missing.\n\nThe problematic commit is here:\n\nhttps://github.com/davidhq/logo_x/commit/e3e5fa4b60b7939999b2a8c44330312755b72f93\n\nit has two parents: ae2a364 and bd1a059\n\non both parents the project compiles in Visual Studio because\nLogo.csproj is not corrupted.\n\nHow to reproduce and see that really there were no conflicts and the\nfile became corrupted:\n\nC:\\temp> git clone git@github.com:davidhq/logo_x.git\nC:\\temp\\logo_x [master]> git checkout ae2a364\nNote: checking out 'ae2a364'.\n\nYou are in 'detached HEAD' state. You can look around, make experimental\nchanges and commit them, and you can discard any commits you make in this\nstate without impacting any branches by performing another checkout.\n\nIf you want to create a new branch to retain commits you create, you may\ndo so (now or later) by using -b with the checkout command again. Example:\n\n git checkout -b new_branch_name\n\nHEAD is now at ae2a364... general handler for letters\nC:\\temp\\logo_x [(ae2a364...)]> git merge bd1a059\nAuto-merging Logo/Logo.csproj\nMerge made by the 'recursive' strategy.\n Logo/Logo.csproj | 7 ++\n Logo/WebCamForm.Designer.cs | 88 +++++++++++++++++++\n Logo/WebCamForm.cs | 209 ++++++++++++++++++++++++++++++++++++++++++++\n Logo/WebCamForm.resx | 120 +++++++++++++++++++++++++\n Logo/WordsForm.Designer.cs | 1 +\n Logo/WordsForm.cs | 7 ++\n 6 files changed, 432 insertions(+)\n create mode 100644 Logo/WebCamForm.Designer.cs\n create mode 100644 Logo/WebCamForm.cs\n create mode 100644 Logo/WebCamForm.resx\n\nNow check Logo.csproj and observe line 81 (it should read \n\nIf I add both missing closing tags the project compiles again.\n\nPlease investigate and thank you!\n\nPS: on Windows I have version 1.8.0.msysgit.0 of git and on Mac I'm\nnot sure now, it's a bit older, but the same problem happens.\n\nDavid\n"},{"id":"210615","messageId":"20130305090326.GC13552@sigill.intra.peff.net","threadId":"33065","inReplyTo":"CAOFaZ+5F1BcWNU=AkcnS53bQt1VfAfsFjp9EvRCL=7kYiU1ejg@mail.gmail.com","subject":"Re: auto merge bug","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-05T09:03:26Z","receivedAt":"2013-03-05T09:03:26Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 04, 2013 at 05:46:48PM +0100, David Krmpotic wrote:\n\n> We started working on a .NET app and the XML project file (.csproj)\n> got corrupted (a few closing tag missing).\n> \n> 79\t \n> 80\t SlovaricaForm.cs\n> 81\t+ \n> 82\t+ Form\n> 83\t+ \n> 84\t+ \n> 85\t+ WebCamForm.cs\n> 86\t \n> \n> between lines 80 and 81 there should be \n>\n> [...]\n>\n> The problematic commit is here:\n> \n> https://github.com/davidhq/logo_x/commit/e3e5fa4b60b7939999b2a8c44330312755b72f93\n\nThanks for an easy-to-reproduce report. The problem here is that your\n.gitattributes file specifies the \"union\" merge driver for .csproj\n(and other) files. From \"git help attributes\":\n\n union\n Run 3-way file level merge for text files, but take lines\n from both versions, instead of leaving conflict markers.\n This tends to leave the added lines in the resulting file\n in random order and the user should verify the result. Do\n not use this if you do not understand the implications.\n\nYour stanzas each end on an identical line. So it sees that\none side has:\n\n A\n B\n Z\n\nand the other side has:\n\n C\n D\n Z\n\nIt realizes that the \"Z\" is common, so is not part of the conflict. But\nin the normal 3-way merge case, the rest of it conflicts, so you get a\nchance to inspect it. But with \"union\", it just silently concatenates\nthe conflicting bits.\n\nI suspect you can run into other problems with \"union\" here, too,\nbecause line order _does_ matter for you. It comes close to working if\nboth sides are just adding elements at the same level of the tree (as\nyou are here), but what about more complicated edits?\n\nI think what you really want is an XML-aware merge tool that can see you\njust added two independent ... stanzas that can\nco-exist (i.e., it could do a union, but at the level of XML tags, not\nat the level of individual lines). I do not know offhand of any such\ntool (or for that matter, a good general XML-aware 3-way merge tool),\nbut if you had one, you could plug it in as a custom merge driver.\n\nYou might be able to get by with a version of the \"union\" driver that\nasks the 3-way merge driver to be less aggressive about shrinking the\nconflict blocks. For example, with this patch to git:\n\ndiff --git a/ll-merge.c b/ll-merge.c\nindex fb61ea6..61b1d4e 100644\n--- a/ll-merge.c\n+++ b/ll-merge.c\n@@ -100,7 +100,6 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,\n \t}\n \n \tmemset(&xmp, 0, sizeof(xmp));\n-\txmp.level = XDL_MERGE_ZEALOUS;\n \txmp.favor = opts->variant;\n \txmp.xpp.flags = opts->xdl_opts;\n \tif (git_xmerge_style >= 0)\n\nI think the merge will produce the results you are looking for. This\nwould have to be configurable, though, as it is a regression for\nexisting users of \"union\", which would want the duplicate-line\nsuppression (or maybe not; it will only catch such duplicates at the\nbeginning and end of the conflict hunk, so maybe it is sane to always\nask \"union\" to keep all lines).\n\nI'd still worry about more complicated edits fooling \"union\", but at\nleast the simple cases would work.\n\nIn the meantime, I think you are better to drop those \"merge\"\ngitattributes, and just let the regular three way merge generate a\nconflict which you can inspect. For the merge in question, it yields:\n\n diff --cc Logo/Logo.csproj\n index 4113434,c681862..0000000\n --- a/Logo/Logo.csproj\n +++ b/Logo/Logo.csproj\n @@@ -67,17 -67,11 +67,25 @@@\n \n \n \n ++<<<<<<< HEAD\n + \n + Form\n + \n + \n + MainForm.cs\n + \n + \n + Form\n + \n + \n + SlovaricaForm.cs\n ++=======\n + \n + Form\n + \n + \n + WebCamForm.cs\n ++>>>>>>> bd1a059\n \n \n Form\n\nwhere you can see that it \"shrinks\" the conflict hunk to not include the\nline added by both sides (the file \"\" after the conflict). But\nby triggering a conflict, you can actually look at and fix it. That's\nmore work, of course.\n\nAnother alternate is to keep the \"union\" driver and just do better\ntesting of merges. Even with the stock 3-way driver, a merge that\nauto-resolves is not necessarily correct (e.g., even if there are not\ntextual conflicts, there may be semantic ones).\n\n-Peff\n"},{"id":"210616","messageId":"20130305091203.GD13552@sigill.intra.peff.net","threadId":"33065","inReplyTo":"20130305090326.GC13552@sigill.intra.peff.net","subject":"Re: auto merge bug","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-05T09:12:03Z","receivedAt":"2013-03-05T09:12:03Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 05, 2013 at 04:03:26AM -0500, Jeff King wrote:\n\n> You might be able to get by with a version of the \"union\" driver that\n> asks the 3-way merge driver to be less aggressive about shrinking the\n> conflict blocks. For example, with this patch to git:\n> \n> diff --git a/ll-merge.c b/ll-merge.c\n> index fb61ea6..61b1d4e 100644\n> --- a/ll-merge.c\n> +++ b/ll-merge.c\n> @@ -100,7 +100,6 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,\n> \t}\n> \n> \tmemset(&xmp, 0, sizeof(xmp));\n> -\txmp.level = XDL_MERGE_ZEALOUS;\n> \txmp.favor = opts->variant;\n> \txmp.xpp.flags = opts->xdl_opts;\n> \tif (git_xmerge_style >= 0)\n> \n> I think the merge will produce the results you are looking for. This\n> would have to be configurable, though, as it is a regression for\n> existing users of \"union\", which would want the duplicate-line\n> suppression (or maybe not; it will only catch such duplicates at the\n> beginning and end of the conflict hunk, so maybe it is sane to always\n> ask \"union\" to keep all lines).\n\nHere's what the patch would look like to make it non-configurable, but\nto just trigger for the \"union\" case:\n\ndiff --git a/ll-merge.c b/ll-merge.c\nindex fb61ea6..fc33a23 100644\n--- a/ll-merge.c\n+++ b/ll-merge.c\n@@ -83,7 +83,8 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,\n \t\t\tmmfile_t *src1, const char *name1,\n \t\t\tmmfile_t *src2, const char *name2,\n \t\t\tconst struct ll_merge_options *opts,\n-\t\t\tint marker_size)\n+\t\t\tint marker_size,\n+\t\t\tint level)\n {\n \txmparam_t xmp;\n \tassert(opts);\n@@ -100,7 +101,7 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,\n \t}\n \n \tmemset(&xmp, 0, sizeof(xmp));\n-\txmp.level = XDL_MERGE_ZEALOUS;\n+\txmp.level = level;\n \txmp.favor = opts->variant;\n \txmp.xpp.flags = opts->xdl_opts;\n \tif (git_xmerge_style >= 0)\n@@ -129,7 +130,23 @@ static int ll_union_merge(const struct ll_merge_driver *drv_unused,\n \to.variant = XDL_MERGE_FAVOR_UNION;\n \treturn ll_xdl_merge(drv_unused, result, path_unused,\n \t\t\t orig, NULL, src1, NULL, src2, NULL,\n-\t\t\t &o, marker_size);\n+\t\t\t &o, marker_size, XDL_MERGE_MINIMAL);\n+}\n+\n+static int ll_text_merge(const struct ll_merge_driver *drv,\n+\t\t\t mmbuffer_t *result,\n+\t\t\t const char *path,\n+\t\t\t mmfile_t *orig, const char *orig_name,\n+\t\t\t mmfile_t *src1, const char *name1,\n+\t\t\t mmfile_t *src2, const char *name2,\n+\t\t\t const struct ll_merge_options *opts,\n+\t\t\t int marker_size)\n+{\n+\treturn ll_xdl_merge(drv, result, path,\n+\t\t\t orig, orig_name,\n+\t\t\t src1, name1,\n+\t\t\t src2, name2,\n+\t\t\t opts, marker_size, XDL_MERGE_ZEALOUS);\n }\n \n #define LL_BINARY_MERGE 0\n@@ -137,7 +154,7 @@ static struct ll_merge_driver ll_merge_drv[] = {\n #define LL_UNION_MERGE 2\n static struct ll_merge_driver ll_merge_drv[] = {\n \t{ \"binary\", \"built-in binary merge\", ll_binary_merge },\n-\t{ \"text\", \"built-in 3-way text merge\", ll_xdl_merge },\n+\t{ \"text\", \"built-in 3-way text merge\", ll_text_merge },\n \t{ \"union\", \"built-in union merge\", ll_union_merge },\n };\n \n"},{"id":"210631","messageId":"7vtxopvoky.fsf@alter.siamese.dyndns.org","threadId":"33065","inReplyTo":"20130305090326.GC13552@sigill.intra.peff.net","subject":"Re: auto merge bug","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-05T15:44:13Z","receivedAt":"2013-03-05T15:44:13Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I think the merge will produce the results you are looking for. This\n> would have to be configurable, though, as it is a regression for\n> existing users of \"union\", which would want the duplicate-line\n> suppression (or maybe not; it will only catch such duplicates at the\n> beginning and end of the conflict hunk, so maybe it is sane to always\n> ask \"union\" to keep all lines).\n\nThe original use-case example of \"union\" was to merge two shopping\nlists (e.g. I add \"bread\" and \"orange juice\" to remind me that we\nneed to buy these things, while my wife adds \"bread\" and \"butter\").\n\nWe do not necessarily want to end up with a shopping list to buy two\nloaves of bread. When the user verifies and fixes up the result, we\ncan keep the current behaviour and those who want to re-dup can add\none back, or we can change the behaviour to leave the duplicates and\nthose who do not want to see duplicates can remove them manually.\n\nGiven that the caveat you quoted already tells the user to verify\nthe result and not to use it without understanding its implications,\nI think it technically is fine either way (read: keeping duplicates\nis not a clearly superiour solution). So let's leave it as-is.\n"},{"id":"210646","messageId":"20130305175904.GC9379@sigill.intra.peff.net","threadId":"33065","inReplyTo":"7vtxopvoky.fsf@alter.siamese.dyndns.org","subject":"Re: auto merge bug","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-05T17:59:04Z","receivedAt":"2013-03-05T17:59:04Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 05, 2013 at 07:44:13AM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > I think the merge will produce the results you are looking for. This\n> > would have to be configurable, though, as it is a regression for\n> > existing users of \"union\", which would want the duplicate-line\n> > suppression (or maybe not; it will only catch such duplicates at the\n> > beginning and end of the conflict hunk, so maybe it is sane to always\n> > ask \"union\" to keep all lines).\n> \n> The original use-case example of \"union\" was to merge two shopping\n> lists (e.g. I add \"bread\" and \"orange juice\" to remind me that we\n> need to buy these things, while my wife adds \"bread\" and \"butter\").\n> \n> We do not necessarily want to end up with a shopping list to buy two\n> loaves of bread. When the user verifies and fixes up the result, we\n> can keep the current behaviour and those who want to re-dup can add\n> one back, or we can change the behaviour to leave the duplicates and\n> those who do not want to see duplicates can remove them manually.\n> \n> Given that the caveat you quoted already tells the user to verify\n> the result and not to use it without understanding its implications,\n> I think it technically is fine either way (read: keeping duplicates\n> is not a clearly superiour solution). So let's leave it as-is.\n\nMy problem with the current behavior is that it is not predictable\nwhether it will de-dup or not. If your shopping lists are:\n\n bread\n orange juice\n\n bread\n butter\n\nit works; you get only one bread. If they are:\n\n milk\n bread\n orange juice\n\n beer\n bread\n butter\n\nyou get two. It depends on the exact behavior of the XDL_MERGE_ZEALOUS\nflag. What I'd propose is two different drivers:\n\n 1. Find conflicts via 3-way merge, and include both sides of the\n conflict verbatim. Do not use XDL_MERGE_ZEALOUS, as it is more\n important to retain items from both sides (in their original order)\n than it is to remove duplicates.\n\n 2. A true line-based union, which should act like \"cat $ours $theirs |\n sort | uniq\". That is what you want for the shopping list example,\n I think (you could also preserve existing ordering with a lookup\n table, though I prefer clobbering the ordering; the ordering of\n resolved conflicts will be arbitrary anyway, so it makes it clear\n from the outset that you should not use this driver if your content\n is not really a set (in the mathematical sense) of lines).\n\n You could also have sets of other objects (e.g., blank-line\n delimited paragraphs, changelog entries, etc). But you would need\n some way to specify the parsing then[1].\n\nI'm not sure which should be called \"union\". The first one would still\nneed careful examination of the result. The second one should always be\ncorrect, but only because it is limited to a much more constrained\nproblem.\n\nI'm also not sure how useful those really are in practice. I have not\nused \"union\" myself ever. And in the example that started this thread, I\nfind the use of \"union\" slightly dubious. I do not even know how it\nwould react to a line _changing_, or other complicated edit. Short of a\nspecialized XML-aware merge driver, using XDL_MERGE_ZEALOUS and kicking\nthe result out to the user (i.e., what the default merge driver does)\nseems like the only sane thing, even if it is more work at merge time.\n\n-Peff\n\n[1] Some of this is fairly easy to do with perl one-liners (e.g., \"perl\n -00 -ne 'print unless $h{$_}++\" for paragraph mode), so maybe it is\n just an education/documentation issue. I dunno. I have always been\n happy enough with the stock merge.\n"},{"id":"210651","messageId":"7va9qhu1jk.fsf@alter.siamese.dyndns.org","threadId":"33065","inReplyTo":"20130305175904.GC9379@sigill.intra.peff.net","subject":"Re: auto merge bug","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-05T18:47:11Z","receivedAt":"2013-03-05T18:47:11Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I'm also not sure how useful those really are in practice. I have not\n> used \"union\" myself ever. And in the example that started this thread, I\n> find the use of \"union\" slightly dubious.\n\nYeah, I do not think anybody sane used \"union\" outside toy examples.\nIIRC, it was originally done as a \"if you want a GIGO, here it is,\ngo hang yourself.\" response to \"I am too lazy to resolve conflicts\nmyself, Git should let me take both sides blindly.\"\n"},{"id":"210655","messageId":"51365C0F.8070207@op5.se","threadId":"33065","inReplyTo":"7va9qhu1jk.fsf@alter.siamese.dyndns.org","subject":"Re: auto merge bug","fromName":"Andreas Ericsson","fromEmail":"ae@op5.se","sentAt":"2013-03-05T20:56:47Z","receivedAt":"2013-03-05T20:56:47Z","isPatch":false,"sender":{"key":"ae@op5.se","avatar":"https://gravatar.com/avatar/426e89595c75a8f5252dd0c989e5fabe5bcac616e68557427ad9aef6b0ca342a?d=mp&s=160"},"body":"On 03/05/2013 07:47 PM, Junio C Hamano wrote:\n> Jeff King writes:\n> \n>> I'm also not sure how useful those really are in practice. I have not\n>> used \"union\" myself ever. And in the example that started this thread, I\n>> find the use of \"union\" slightly dubious.\n> \n> Yeah, I do not think anybody sane used \"union\" outside toy examples.\n\nI do, for lists used in tests or to generate perfect hashes from. It's\nreally quite handy for things like that but totally useless for any\ntype of multiline format, or even .ini style files unless you're very,\nvery careful with how you write them.\n\n-- \nAndreas Ericsson andreas.ericsson@op5.se\nOP5 AB www.op5.se\nTel: +46 8-230225 Fax: +46 8-230231\n\nConsidering the successes of the wars on alcohol, poverty, drugs and\nterror, I think we should give some serious thought to declaring war\non peace.\n"},{"id":"210664","messageId":"CAOFaZ+4oUD7eMvFmtPdca4AYooxW-PCOiPBUb0jjVw4LPBN8+Q@mail.gmail.com","threadId":"33065","inReplyTo":"194F685F-9460-42C6-B5A5-59475F53D038@gmail.com","subject":"Re: auto merge bug","fromName":"David Krmpotic","fromEmail":"david.krmpotic@gmail.com","sentAt":"2013-03-05T22:13:12Z","receivedAt":"2013-03-05T22:13:12Z","isPatch":false,"sender":{"key":"david.krmpotic@gmail.com","avatar":"https://gravatar.com/avatar/e9f39032d6691c1688fa3a041f521b7a9e19fc1f1980ba29bc40dce9cabacfd9?d=mp&s=160"},"body":"Hi guys! Thank you for responses.. I haven't suspected that repos\ncreated via GitHub windows app would have union set by default :( have\nto ask them about it.. it seems wrong to me… Here are the defaults for\na windows repo created with GitHub for windows app:\n\nlogo (master)$ cat .gitattributes\n# Auto detect text files and perform LF normalization\n* text=auto\n\n# Custom for Visual Studio\n*.cs diff=csharp\n*.sln merge=union\n*.csproj merge=union\n*.vbproj merge=union\n*.fsproj merge=union\n*.dbproj merge=union\n\n# Standard to msysgit\n*.doc\t diff=astextplain\n*.DOC\t diff=astextplain\n*.docx diff=astextplain\n*.DOCX diff=astextplain\n*.dot diff=astextplain\n*.DOT diff=astextplain\n*.pdf diff=astextplain\n*.PDF\t diff=astextplain\n*.rtf\t diff=astextplain\n*.RTF\t diff=astextplain\n\nWhile investigating my problem I have read about the special union\nmerge mode, but didn't check if maybe my repo was in that mode..\nreally didn't expect it.\n\nTHANK YOU again… now I'll write to the github guys..\n\n\nDavid\n"},{"id":"210680","messageId":"20130306091515.GC2018@sigill.intra.peff.net","threadId":"33065","inReplyTo":"CAOFaZ+4oUD7eMvFmtPdca4AYooxW-PCOiPBUb0jjVw4LPBN8+Q@mail.gmail.com","subject":"Re: auto merge bug","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-06T09:15:15Z","receivedAt":"2013-03-06T09:15:15Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 05, 2013 at 11:13:12PM +0100, David Krmpotic wrote:\n\n> Hi guys! Thank you for responses.. I haven't suspected that repos\n> created via GitHub windows app would have union set by default :( have\n> to ask them about it.. it seems wrong to me… Here are the defaults for\n> a windows repo created with GitHub for windows app:\n> [...]\n> # Custom for Visual Studio\n> *.cs diff=csharp\n> *.sln merge=union\n> *.csproj merge=union\n> *.vbproj merge=union\n> *.fsproj merge=union\n> *.dbproj merge=union\n\nYeah, I think defaulting to merge=union there is questionable. In an\nideal world, the GitHub for Windows folks would ship a specialized merge\nhelper for handling VS project files. It can be open-source and\ndistributed separately for people who don't use GitHub, but they can\nintegrate it seamlessly into the GitHub client. So everybody wins.\n\nI see you've already written to GitHub support; thanks. I'll make sure\nyour issue gets routed to the right people, and I'll see if I can\nconvince them to write the specialized tool. :)\n\n-Peff\n"}]}