{"thread":{"id":"7561","subject":"[PATCH 4/5] merge-recursive: handle D/F conflict case more carefully.","startedAt":"2007-04-07T14:42:55Z","lastAt":"2007-04-10T18:20:30Z","messageCount":5,"participants":["Junio C Hamano","Alex Riesen"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"38848","messageId":"7v6488ckk0.fsf@assigned-by-dhcp.cox.net","threadId":"7561","inReplyTo":null,"subject":"[PATCH 4/5] merge-recursive: handle D/F conflict case more carefully.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-04-07T14:42:55Z","receivedAt":"2007-04-07T14:42:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When a path D that originally was blob in the ancestor was\nmodified on our branch while it was removed on the other branch,\nwe keep stages 1 and 2, and leave our version in the working\ntree. If the other branch created a path D/F, however, that\npath can cleanly be resolved in the index (after all, the\nancestor nor we do not have it and only the other side added),\nbut it cannot be checked out. The issue is the same when the\nother branch had D and we had renamed it to D/F, or the ancestor\nhad D/F instead of D (so there are four combinations).\n\nDo not stop the merge, but leave both D and D/F paths in the\nindex so that the user can clear things up.\n\nSigned-off-by: Junio C Hamano \n---\n\n * This one is quite iffy, as I suspect it can leave higher\n stages for intermediate merges in its current form, and I\n suspect that the caller of this function should be checking\n for D/F conflicts (there are some provisions in the existing\n code, but apparently they are not good enough to do the right\n thing to pass t3030 test which is the next patch).\n\n merge-recursive.c | 29 ++++++++++++++++++++++++++---\n 1 files changed, 26 insertions(+), 3 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex aa3d913..857eeee 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -580,9 +580,31 @@ static void update_file_flags(const unsigned char *sha,\n \n \t\tif (S_ISREG(mode) || (!has_symlinks && S_ISLNK(mode))) {\n \t\t\tint fd;\n-\t\t\tif (mkdir_p(path, 0777))\n-\t\t\t\tdie(\"failed to create path %s: %s\", path, strerror(errno));\n-\t\t\tunlink(path);\n+\t\t\tint status;\n+\t\t\tconst char *msg = \"failed to create path '%s'%s\";\n+\n+\t\t\tstatus = mkdir_p(path, 0777);\n+\t\t\tif (status) {\n+\t\t\t\tif (status == -3) {\n+\t\t\t\t\t/* something else exists */\n+\t\t\t\t\terror(msg, path, \": perhaps a D/F conflict?\");\n+\t\t\t\t\tupdate_wd = 0;\n+\t\t\t\t\tgoto update_index;\n+\t\t\t\t}\n+\t\t\t\tdie(msg, path, \"\");\n+\t\t\t}\n+\t\t\tif (unlink(path)) {\n+\t\t\t\tif (errno == EISDIR) {\n+\t\t\t\t\t/* something else exists */\n+\t\t\t\t\terror(msg, path, \": perhaps a D/F conflict?\");\n+\t\t\t\t\tupdate_wd = 0;\n+\t\t\t\t\tgoto update_index;\n+\t\t\t\t}\n+\t\t\t\tif (errno != ENOENT)\n+\t\t\t\t\tdie(\"failed to unlink %s \"\n+\t\t\t\t\t \"in preparation to update: %s\",\n+\t\t\t\t\t path, strerror(errno));\n+\t\t\t}\n \t\t\tif (mode & 0100)\n \t\t\t\tmode = 0777;\n \t\t\telse\n@@ -604,6 +626,7 @@ static void update_file_flags(const unsigned char *sha,\n \t\t\tdie(\"do not know what to do with %06o %s '%s'\",\n \t\t\t mode, sha1_to_hex(sha), path);\n \t}\n+ update_index:\n \tif (update_cache)\n \t\tadd_cacheinfo(mode, sha, path, 0, update_wd, ADD_CACHE_OK_TO_ADD);\n }\n-- \n1.5.1.81.gee969\n"},{"id":"38954","messageId":"20070409185809.GA13212@steel.home","threadId":"7561","inReplyTo":"7v6488ckk0.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 4/5] merge-recursive: handle D/F conflict case more carefully.","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-04-09T18:58:09Z","receivedAt":"2007-04-09T18:58:09Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Junio C Hamano, Sat, Apr 07, 2007 16:42:55 +0200:\n> +\t\t\tif (unlink(path)) {\n> +\t\t\t\tif (errno == EISDIR) {\n> +\t\t\t\t\t/* something else exists */\n> +\t\t\t\t\terror(msg, path, \": perhaps a D/F conflict?\");\n\nisn't this one an F/D conflict?\n"},{"id":"38949","messageId":"7vk5wlz5mg.fsf@assigned-by-dhcp.cox.net","threadId":"7561","inReplyTo":"20070409185809.GA13212@steel.home","subject":"Re: [PATCH 4/5] merge-recursive: handle D/F conflict case more carefully.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-04-09T19:53:43Z","receivedAt":"2007-04-09T19:53:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Riesen writes:\n\n> Junio C Hamano, Sat, Apr 07, 2007 16:42:55 +0200:\n>> +\t\t\tif (unlink(path)) {\n>> +\t\t\t\tif (errno == EISDIR) {\n>> +\t\t\t\t\t/* something else exists */\n>> +\t\t\t\t\terror(msg, path, \": perhaps a D/F conflict?\");\n>\n> isn't this one an F/D conflict?\n\nYes, as I said in a separate message, the current D/F detector\ncode in merge-recursive does not catch this case in t3030 test\nand comes to this codepath to write it out:\n\n From: Junio C Hamano \n Subject: Re: [PATCH 5/5] t3030: merge-recursive backend test.\n Date: Sat, 07 Apr 2007 22:17:25 -0700\n Message-ID: <7vmz1ja1i2.fsf@assigned-by-dhcp.cox.net>\n\n > +test_expect_success 'merge-recursive d/f conflict result' '\n > +\n > +\tgit ls-files -s >actual &&\n > +\t(\n > +\t\techo \"100644 $o0 0\tA\"\n > +\t\techo \"100644 $o0 1\ta\"\n > +\t\techo \"100644 $o1 2\ta\"\n > +\t\techo \"100644 $o4 0\ta/c\"\n > +\t\techo \"100644 $o0 0\tb\"\n > +\t\techo \"100644 $o1 0\td/e\"\n > +\t) >expected &&\n > +\tgit diff -u expected actual\n > +\n > +'\n\n The expected result of this test is what I find very iffy. The\n input trees are:\n\n * Ancestor has a file \"a\"\n * Our tree has a file \"a\", which was modified from the\n ancestor's\n * The other tree has a file \"a/c\"\n\n merge-recursive.c::git_merge_trees() calls unpack_trees() using\n the threeway_merge function and what it gets back are stage 1\n and 2 entries for \"a\", and stage 3 entry for \"a/c\" (you can see\n it by running \"git-read-tree -m $c0 $c1 $c4\" instead). But the\n output from merge-recursive has \"a/c\" resolved down to stage 0.\n\n The problem is that process_entry() finds that there is only\n stage 3 entry for that path and decides to resolve it. I think\n it should do the D/F check and leave \"a/c\" in stage 3.\n\nThe part you quoted is merely working this problem around.\n"},{"id":"38997","messageId":"81b0412b0704100140g7684e128g75c986c213ac9d0f@mail.gmail.com","threadId":"7561","inReplyTo":"7vk5wlz5mg.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 4/5] merge-recursive: handle D/F conflict case more carefully.","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-04-10T08:40:51Z","receivedAt":"2007-04-10T08:40:51Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On 4/9/07, Junio C Hamano wrote:\n> Alex Riesen writes:\n>\n> > Junio C Hamano, Sat, Apr 07, 2007 16:42:55 +0200:\n> >> + if (unlink(path)) {\n> >> + if (errno == EISDIR) {\n> >> + /* something else exists */\n> >> + error(msg, path, \": perhaps a D/F conflict?\");\n> >\n> > isn't this one an F/D conflict?\n>\n> Yes, as I said in a separate message, the current D/F detector\n> code in merge-recursive does not catch this case in t3030 test\n> and comes to this codepath to write it out:\n>\n\nI mean, maybe the error could be spelled differently:\n\": perhaps an F/D conflict?\"\nRight now we have two exactly the same errors,\nas seen on the output. The messages refer to different\nconflicts, though. It is usually useful to know what is really\ngoing on.\n"},{"id":"39036","messageId":"7vabxgw0pd.fsf@assigned-by-dhcp.cox.net","threadId":"7561","inReplyTo":"81b0412b0704100140g7684e128g75c986c213ac9d0f@mail.gmail.com","subject":"Re: [PATCH 4/5] merge-recursive: handle D/F conflict case more carefully.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-04-10T18:20:30Z","receivedAt":"2007-04-10T18:20:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Alex Riesen\" writes:\n\n> On 4/9/07, Junio C Hamano wrote:\n>> Alex Riesen writes:\n>>\n>> > Junio C Hamano, Sat, Apr 07, 2007 16:42:55 +0200:\n>> >> + if (unlink(path)) {\n>> >> + if (errno == EISDIR) {\n>> >> + /* something else exists */\n>> >> + error(msg, path, \": perhaps a D/F conflict?\");\n>> >\n>> > isn't this one an F/D conflict?\n>>\n>> Yes, as I said in a separate message, the current D/F detector\n>> code in merge-recursive does not catch this case in t3030 test\n>> and comes to this codepath to write it out:\n>>\n>\n> I mean, maybe the error could be spelled differently:\n> \": perhaps an F/D conflict?\"\n> Right now we have two exactly the same errors,\n> as seen on the output. The messages refer to different\n> conflicts, though. It is usually useful to know what is really\n> going on.\n\nPerhaps, but I think the bigger issue is that existing D/F or\nF/D conflict detection is simply buggy, and this patch shouldn't\nbe needed if they were working correctly.\n"}]}