{"thread":{"id":"47314","subject":"[PATCH] merge-recursive: ignore_case shouldn't reject intentional removals","startedAt":"2017-11-24T19:59:47Z","lastAt":"2017-11-28T01:02:52Z","messageCount":10,"participants":["Elijah Newren","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"333456","messageId":"20171124195901.2581-1-newren@gmail.com","threadId":"47314","inReplyTo":null,"subject":"[PATCH] merge-recursive: ignore_case shouldn't reject intentional removals","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2017-11-24T19:59:01Z","receivedAt":"2017-11-24T19:59:47Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"In commit ae352c7f3 (merge-recursive.c: fix case-changing merge bug,\n2014-05-01), it was observed that removing files could be problematic on\ncase insensitive file systems, because we could end up removing files\nthat differed in case only rather than deleting the intended file --\nsomething that happened when files were renamed on one branch in a way\nthat differed only in case.  To avoid that problem, that commit added\nlogic to avoid removing files other than the one intended, rejecting the\nremoval if the files differed only in case.\n\nUnfortunately, the logic it used didn't fully implement that condition as\nstated above; instead it merely checked that a case-insensitive lookup of\nthe file that was requested resulted in finding a file in the index at\nstage 0, not that the file found in the index actually differed in case.\nAlternatively, one could view the implementation as making an implicit\nassumption that the file we actually wanted to remove would never appear\nin the index with a stage of 0, and thus that if we found a file with our\nlookup, that it had to be a different file (but different in case only).\n\nThe net result of this implementation is that it can ignore more requests\nthan it should, leaving a file around in the working copy that should\nhave been removed.  Make sure that the file found in the index actually\ndiffers in case before silently ignoring the request to remove the file.\n\n---\n\nIf that description leaves more questions than answers, we may need to\naugment the above commit message with the following explanation...\n\nBut, you may ask, why didn't we ever discover this problem before?  And\nwhy would we have a file at stage 0 in the index that we wanted to remove\nfrom the working copy?  Great questions, both, but the answer is fairly\nlengthy.  The short answer to the first question is that due to a myriad\nof details, this bug is only currently triggerable:\n\n    * on case insensitive filesystems\n    * with rename/rename(2to1) conflicts\n    * once one has taken care to avoid allowing renames to overwrite\n      untracked or dirty files (see commit 30fd3a5 (merge overwrites\n      unstaged changes in renamed file, 2012-04-15), which was fixed in\n      my in-flight en/rename-directory-detection series currently sitting\n      in pu)\n    * AND where one of the renames is implicitly done (e.g. via\n      directory rename detection).\n\nThus, this bug remained hidden/latent until those other conditions are\nall triggered.  Luckily, testcase 7b of t6043 added in\nen/rename-directory-detection was written to carefully check all details\nof the index and working copy to ensure they had the right content,\ngiving us an early notification of this bug.  (I was worried when I wrote\nthose tests that I was being too laborious in checking all details, but I\napparently got lucky and the extra checks in one of them paid off.)\n\nTo explain why we would have a file at stage 0 in the index that we\nwanted to remove from the working copy can be explained by four points\n(most of which bring up further questions, but be patient and I'll try to\nwrap up all the loose ends):\n\n  * If we have two conflicting files at PATH, we will want the index to\n    have two higher stage entries for PATH.  There are a few choices for\n    the working copy, but one prominent choice is to remove PATH from the\n    working copy, then create PATH~HEAD and PATH~$MERGE files.  This\n    strategy for the working copy is currently used for\n    rename/rename(2to1) conflicts, though it has also been proposed (in\n    my pending rename-perf series I submitted this month) for some\n    add/add and rename/add conflicts.\n\n  * When unpack_trees() (called by merge-recursive) notices two paths at\n    the same location (which could mean an add/add conflict, or after\n    rename detection it might more precisely turn out to be a rename/add\n    or rename/rename(2to1) conflict), unpack_trees() removes the stage 0\n    index entry for the path and just creates higher stage entries.\n\n    HOWEVER, if files can be implicitly renamed to a path that didn't\n    exist on either side of the merge (such as from directory rename\n    detection), then merge-recursive can see two conflicting files at the\n    same location, despite unpack_trees() having left the path at stage\n    0.\n\n  * merge-recursive is traditionally thought of as doing a 3-way merge.\n    But what it really is forced to do is more of a 4-way merge; a good\n    chunk of its annoying complexity is based around this (undocumented\n    and unfortunate) reality.  It derives from what I consider a simple\n    design flaw.\n\n  * In order to get the 4-way merge right (i.e. avoiding spurious error\n    messages and taking care to not overwrite important dirty or\n    untracked files), we MUST handle the working copy BEFORE updating the\n    index.\n\nThe combination of these four items in aggregate answers how we get a\nstage 0 file that we want to remove from the working copy, but leaves two\nquestions -- what do I mean by 4-way merge, and why does the working copy\nhave to be updated before the index to get that merge right?  Since those\nare crucial to understanding this bug, let me begin by explaining the\n4-way merge:\n\nThe recursive merge strategy is built on first running unpack_trees() and\nthen \"fixing up\" the parts it can't resolve.  unpack_trees() does three\nthings\n\n  * Check whether any untracked or dirty (not-uptodate) files would be\n    overwritten by the merge.\n  * Update the index AND working tree for trivial cases\n  * Store conflicts as higher order stages in the index for later steps\n    (e.g. merge-recursive.c) to resolve.\n\nNote that unpack_trees() doesn't understand renames, thus its checks for\nwhether a dirty or untracked file would be overwritten by the merge is\ngoing to have both false positives and false negatives.  For false\npositives, see testcase 10e of the new t6043 introduced in the in-flight\nen/rename-directory-detection series.  For false negatives, see commit\n30fd3a5 (merge overwrites unstaged changes in renamed file, 2012-04-15),\nand most testcases in sections 10 and 11 of the new t6043 introduced in the\nen/rename-directory-detection series.\n\nThe false positive cannot be fixed with the current design; unpack_trees()\nsimply aborts early in some highly uncommon cases.\n\nThe false negatives cannot be fixed either, but they can be worked around;\nfundamentally, it is too late to \"abort the merge early\" with an error\nmessage, because unpack_trees() already wrote lots of changes to the\nworking copy for all the trivial merge cases and thus aborting would mean\nbeing left with a dirty working copy.  Since it can't abort early, the code\ninstead needs to consider what untracked or dirty files might exist in the\nworking copy and take care to not overwrite them, essentially forcing us to\nnot only consider HEAD, the merge branch, and the merge base, but also the\nworking copy as a fourth set of content we need to interact with.  (It\ncould be worse, though.  If not for merge's check that the index matches\nHEAD and aborting early if they don't, we could have been forced to deal\nwith a 5-way merge.)  The workarounds for false negatives take the form of\ncalling the would_lose_untracked() and was_dirty() functions (the latter of\nwhich was added in the en/rename-directory-detection series to fix\noverwriting dirty files involved in both normal and directory renames).\n\nThat brings us to our final loose end -- why does the working copy have to\nbe updated before the index?  The short answer is that\nwould_lose_untracked() and was_dirty() both rely on information in the\nindex in order to do their work.  The current index has information copied\nfrom the original index by unpack_trees() that we need in these functions.\nSince unpack_trees() discards the original index before returning, the\ncurrent index is our one source for this information, so we need to be\ncareful not to discard it until after we have handled the working copy.\n\nAnd it may be important to point out that discarding the information in the\nindex before updating the working copy is an easy mistake to make that is\npainful to debug.  This is evidenced by the existence of commit f53d39778\n(merge-recursive: Fix spurious 'refusing to lose untracked file...'\nmessages, 2011-08-11), which added a big comment at the beginning of\nupdate_stages() pointing out the perils of trying to update the stages in\nthe index before updating the working copy.  In fact, despite adding that\nbig comment, I hit the same kind of problem again later and added two more\nvery similar large warnings to the top of the conflict_rename_rename_2to1()\nand apply_directory_rename_modifications() functions in the\nen/rename-directory-detection series.\n\nTo try to recap, 4-way merge is a pain.  It has always been a pain as\nevidenced by commits like\n  * f53d39778 (merge-recursive: Fix spurious 'refusing to lose untracked\n    file...' messages, 2011)\n  * 30fd3a5 (merge overwrites unstaged changes in renamed file, 2012)\nbut it becomes even more so with additional features and requirements like\ncase-insensitive filesystems and directory rename detection.  4-way merges\nsimply cause the complexity to increase with every new capability.\n\nThe cleanest fix would be to switch to a 3-way index-only merge (making\nunpack_trees() completely ignore the working copy, and making most of\nmerge-recursive do the same), then checking for untracked or dirty\nworking tree files in the way of the merge, then adding code that allows\nus to update the working copy from one index to a new index.  That new\nupdate code would be very similar to checkout, except that the new index\nmight have conflicts, and the new update may need to be supplemented with\nadditional info in order to correctly report various special conflict\ncases such as rename/rename(2to1) or rename/add).\n\nBut such a large rewrite is a big task.  In the mean time, fix the\nignore_case code.  Allow it to continue rejecting the removal of files\nthat differ in case only, but only allow it to reject such removals.\n\n\n merge-recursive.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\nSidenote: I built this commit on master, but it cleanly cherry-picks to\nmaint, to next, and to pu.\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex b48b15a6f..100fb913f 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -646,7 +646,7 @@ static int remove_file(struct merge_options *o, int clean,\n \t\tif (ignore_case) {\n \t\t\tstruct cache_entry *ce;\n \t\t\tce = cache_file_exists(path, strlen(path), ignore_case);\n-\t\t\tif (ce && ce_stage(ce) == 0)\n+\t\t\tif (ce && ce_stage(ce) == 0 && strcmp(path, ce->name))\n \t\t\t\treturn 0;\n \t\t}\n \t\tif (remove_path(path))\n-- \n2.11.0\n\n"},{"id":"333457","messageId":"CAPig+cTtT2xLHy6gE9fW9G_KdzGTNEfgwz7NUOUwHxGKOkiKtA@mail.gmail.com","threadId":"47314","inReplyTo":"20171124195901.2581-1-newren@gmail.com","subject":"Re: [PATCH] merge-recursive: ignore_case shouldn't reject intentional removals","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2017-11-24T20:04:05Z","receivedAt":"2017-11-24T20:04:10Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Nov 24, 2017 at 2:59 PM, Elijah Newren <newren@gmail.com> wrote:\n> In commit ae352c7f3 (merge-recursive.c: fix case-changing merge bug,\n> 2014-05-01), it was observed that removing files could be problematic on\n> case insensitive file systems, because we could end up removing files\n> that differed in case only rather than deleting the intended file --\n> something that happened when files were renamed on one branch in a way\n> that differed only in case.  To avoid that problem, that commit added\n> logic to avoid removing files other than the one intended, rejecting the\n> removal if the files differed only in case.\n>\n> Unfortunately, the logic it used didn't fully implement that condition as\n> stated above; instead it merely checked that a case-insensitive lookup of\n> the file that was requested resulted in finding a file in the index at\n> stage 0, not that the file found in the index actually differed in case.\n> Alternatively, one could view the implementation as making an implicit\n> assumption that the file we actually wanted to remove would never appear\n> in the index with a stage of 0, and thus that if we found a file with our\n> lookup, that it had to be a different file (but different in case only).\n>\n> The net result of this implementation is that it can ignore more requests\n> than it should, leaving a file around in the working copy that should\n> have been removed.  Make sure that the file found in the index actually\n> differs in case before silently ignoring the request to remove the file.\n>\n> ---\n\nMissing sign-off.\n\n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index b48b15a6f..100fb913f 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -646,7 +646,7 @@ static int remove_file(struct merge_options *o, int clean,\n>                 if (ignore_case) {\n>                         struct cache_entry *ce;\n>                         ce = cache_file_exists(path, strlen(path), ignore_case);\n> -                       if (ce && ce_stage(ce) == 0)\n> +                       if (ce && ce_stage(ce) == 0 && strcmp(path, ce->name))\n>                                 return 0;\n>                 }\n>                 if (remove_path(path))\n> --\n> 2.11.0\n"},{"id":"333459","messageId":"CABPp-BG+WCrPY3XNHCGK3fLGAO11wMtf1_kn1tuKojRAHSDvDg@mail.gmail.com","threadId":"47314","inReplyTo":"CAPig+cTtT2xLHy6gE9fW9G_KdzGTNEfgwz7NUOUwHxGKOkiKtA@mail.gmail.com","subject":"Re: [PATCH] merge-recursive: ignore_case shouldn't reject intentional removals","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2017-11-24T20:29:57Z","receivedAt":"2017-11-24T20:30:03Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Nov 24, 2017 at 12:04 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Fri, Nov 24, 2017 at 2:59 PM, Elijah Newren <newren@gmail.com> wrote:\n>> In commit ae352c7f3 (merge-recursive.c: fix case-changing merge bug,\n>> 2014-05-01), it was observed that removing files could be problematic on\n>> case insensitive file systems, because we could end up removing files\n>> that differed in case only rather than deleting the intended file --\n>> something that happened when files were renamed on one branch in a way\n>> that differed only in case.  To avoid that problem, that commit added\n>> logic to avoid removing files other than the one intended, rejecting the\n>> removal if the files differed only in case.\n>>\n>> Unfortunately, the logic it used didn't fully implement that condition as\n>> stated above; instead it merely checked that a case-insensitive lookup of\n>> the file that was requested resulted in finding a file in the index at\n>> stage 0, not that the file found in the index actually differed in case.\n>> Alternatively, one could view the implementation as making an implicit\n>> assumption that the file we actually wanted to remove would never appear\n>> in the index with a stage of 0, and thus that if we found a file with our\n>> lookup, that it had to be a different file (but different in case only).\n>>\n>> The net result of this implementation is that it can ignore more requests\n>> than it should, leaving a file around in the working copy that should\n>> have been removed.  Make sure that the file found in the index actually\n>> differs in case before silently ignoring the request to remove the file.\n>>\n>> ---\n>\n> Missing sign-off.\n\nWhoops!\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n"},{"id":"333470","messageId":"xmqqd147kpdm.fsf@gitster.mtv.corp.google.com","threadId":"47314","inReplyTo":"20171124195901.2581-1-newren@gmail.com","subject":"Re: [PATCH] merge-recursive: ignore_case shouldn't reject intentional removals","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-25T03:29:09Z","receivedAt":"2017-11-25T03:29:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n>     But what it really is forced to do is more of a 4-way merge; a good\n>     chunk of its annoying complexity is based around this (undocumented\n>     and unfortunate) reality.  It derives from what I consider a simple\n>     design flaw.\n\nYes, and it does not help that it wants to write into the filesystem\nwhile it performs the outermost merges.\n\nIn the ideal world, we should\n\n - ask unpack_trees() to do \"read-tree -m\" without \"-u\";\n\n - do all the merge-recursive computations in-core and prepare the\n   resulting index, while keeping the current index intact;\n\n - compare the current in-core index and the resulting in-core\n   index, and notice the paths that need to be added, updated or\n   removed in the working tree, and ensure that there is no loss of\n   information when the change is reflected to the working tree,\n   e.g. the result wants to create a file where the working tree\n   currently has a directory with non-expendable contents in it, the\n   result wants to remove a file where the working tree file has\n   local modification, etc.; and then finally\n\n - carry out the working tree update to make it match what the\n   resulting in-core index says it should look like.\n\n\n\n   \n"},{"id":"333492","messageId":"CABPp-BGcfFOvv+-uW2_EaftLiU-xs_D4g8hsYOZqjk1XwK_8Fw@mail.gmail.com","threadId":"47314","inReplyTo":"xmqqd147kpdm.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] merge-recursive: ignore_case shouldn't reject intentional removals","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2017-11-25T22:35:46Z","receivedAt":"2017-11-25T22:35:56Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Nov 24, 2017 at 7:29 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Elijah Newren <newren@gmail.com> writes:\n>\n>>     But what it really is forced to do is more of a 4-way merge; a good\n>>     chunk of its annoying complexity is based around this (undocumented\n>>     and unfortunate) reality.  It derives from what I consider a simple\n>>     design flaw.\n>\n> Yes, and it does not help that it wants to write into the filesystem\n> while it performs the outermost merges.\n>\n> In the ideal world, we should\n>\n>  - ask unpack_trees() to do \"read-tree -m\" without \"-u\";\n>\n>  - do all the merge-recursive computations in-core and prepare the\n>    resulting index, while keeping the current index intact;\n>\n>  - compare the current in-core index and the resulting in-core\n>    index, and notice the paths that need to be added, updated or\n>    removed in the working tree, and ensure that there is no loss of\n>    information when the change is reflected to the working tree,\n>    e.g. the result wants to create a file where the working tree\n>    currently has a directory with non-expendable contents in it, the\n>    result wants to remove a file where the working tree file has\n>    local modification, etc.; and then finally\n>\n>  - carry out the working tree update to make it match what the\n>    resulting in-core index says it should look like.\n\nI had another email I had been composing to try to argue for changing\nmerge-recursive.c's design to the above, assuming I could get the time\nto work on it.  Nice to see that I'm not crazy, and that I apparently\ndon't need to do much convincing.  :-)\n"},{"id":"333509","messageId":"xmqqk1ydkbx0.fsf@gitster.mtv.corp.google.com","threadId":"47314","inReplyTo":"CABPp-BGcfFOvv+-uW2_EaftLiU-xs_D4g8hsYOZqjk1XwK_8Fw@mail.gmail.com","subject":"Re: [PATCH] merge-recursive: ignore_case shouldn't reject intentional removals","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-26T02:32:11Z","receivedAt":"2017-11-26T02:32:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n> I had another email I had been composing to try to argue for changing\n> merge-recursive.c's design to the above, assuming I could get the time\n> to work on it.  Nice to see that I'm not crazy, and that I apparently\n> don't need to do much convincing.  :-)\n\nYou might even be better off coming up with a *new* merge strategy\nbackend if you want to do this, without using much from the existing\ncode in merge-recursive.c at all (I've written off that code as\nmostly unsalvageable long time ago, and thanked for whoever had a\nclever idea to allow different strategy backend to be made without\ndisrupting the rest of the system).\n\nAfter we gain more confidence with the rewrite, we can switch the\ninternally built-in backend used by different codepaths from\nmerge-recursive.c::merge_recursive() to the new thing.\n"},{"id":"333570","messageId":"xmqq4lpgfkyb.fsf@gitster.mtv.corp.google.com","threadId":"47314","inReplyTo":"20171124195901.2581-1-newren@gmail.com","subject":"Re: [PATCH] merge-recursive: ignore_case shouldn't reject intentional removals","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-27T03:40:28Z","receivedAt":"2017-11-27T03:40:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n> In commit ae352c7f3 (merge-recursive.c: fix case-changing merge bug,\n> 2014-05-01), it was observed that removing files could be problematic on\n> case insensitive file systems, because we could end up removing files\n> that differed in case only rather than deleting the intended file --\n> something that happened when files were renamed on one branch in a way\n> that differed only in case.  To avoid that problem, that commit added\n> logic to avoid removing files other than the one intended, rejecting the\n> removal if the files differed only in case.\n>\n> Unfortunately, the logic it used didn't fully implement that condition as\n> stated above...\n>\n> If that description leaves more questions than answers, we may need to\n> augment the above commit message with the following explanation...\n> ...\n>  merge-recursive.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n\nAs a fix, this sorely wants something new in t/ directory.\n"},{"id":"333609","messageId":"CABPp-BELci79tuiPFVRKQwm_-j6tN-DxqcKsvjD2xG_8ZWNxGw@mail.gmail.com","threadId":"47314","inReplyTo":"xmqq4lpgfkyb.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] merge-recursive: ignore_case shouldn't reject intentional removals","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2017-11-27T16:40:17Z","receivedAt":"2017-11-27T16:40:26Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"[Removed cc's that just bounce]\n\nOn Sun, Nov 26, 2017 at 7:40 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Elijah Newren <newren@gmail.com> writes:\n>\n>> In commit ae352c7f3 (merge-recursive.c: fix case-changing merge bug,\n>> 2014-05-01), it was observed that removing files could be problematic on\n<snip>\n>>\n>> If that description leaves more questions than answers, we may need to\n>> augment the above commit message with the following explanation...\n>> ...\n>>  merge-recursive.c | 2 +-\n>>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> As a fix, this sorely wants something new in t/ directory.\n\nWell, then perhaps I was wrong to submit it independent of my\ndirectory rename series.  As noted in the (very lengthy) extended\ncommit message explanation, the assumption the previous code made just\nhappened to work until a few extra tweaks (from directory renames)\ncaused us to want to remove a file from the working copy that was\nfound at stage 0 in the index.  Thus, the only testcase we can really\nuse for this commit, is testcase 7b of the new t6043 added by that\nother patch series, and it's only valid with the code from that other\nseries.\n\nWhen I submitted this patch, I was thinking about just including this\nfix with the next reroll of my rename-directory-detection series but\nit partially felt like an independent fix...but maybe I chose wrong.\n\nWould you prefer I include it in my next en/rename-directory-detection reroll?\n"},{"id":"333653","messageId":"xmqqshcz9tqa.fsf@gitster.mtv.corp.google.com","threadId":"47314","inReplyTo":"CABPp-BELci79tuiPFVRKQwm_-j6tN-DxqcKsvjD2xG_8ZWNxGw@mail.gmail.com","subject":"Re: [PATCH] merge-recursive: ignore_case shouldn't reject intentional removals","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-27T23:39:41Z","receivedAt":"2017-11-27T23:39:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n>> As a fix, this sorely wants something new in t/ directory.\n>\n> Well, then perhaps I was wrong to submit it independent of my\n> directory rename series.  As noted in the (very lengthy) extended\n> commit message explanation, the assumption the previous code made just\n> happened to work ...\n\nHere is what I wrote in What's cooking draft (which automatically\ngets copied to the merge log message and becomes part of release\nnotes when a topic graduates) for this thing.  Am I on the right\ntrack?\n\n    The code internal to the recursive merge strategy was not fully\n    prepared to see a path that is renamed to try overwriting another\n    path that is only different in case on case insensitive systems.\n    This does not matter in the current code, but will start to matter\n    once the rename detection logic starts taking hints from nearby\n    paths moving to some directory and moves a path that is otherwise\n    not changed along with them.\n\n"},{"id":"333662","messageId":"CABPp-BHt0O_NQtJsAkxxkj0yAvaZDsDtxGJTd9wqmvUOvy7x6w@mail.gmail.com","threadId":"47314","inReplyTo":"xmqqshcz9tqa.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] merge-recursive: ignore_case shouldn't reject intentional removals","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2017-11-28T01:02:45Z","receivedAt":"2017-11-28T01:02:52Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Nov 27, 2017 at 3:39 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Elijah Newren <newren@gmail.com> writes:\n>\n>>> As a fix, this sorely wants something new in t/ directory.\n>>\n>> Well, then perhaps I was wrong to submit it independent of my\n>> directory rename series.  As noted in the (very lengthy) extended\n>> commit message explanation, the assumption the previous code made just\n>> happened to work ...\n>\n> Here is what I wrote in What's cooking draft (which automatically\n> gets copied to the merge log message and becomes part of release\n> notes when a topic graduates) for this thing.  Am I on the right\n> track?\n>\n>     The code internal to the recursive merge strategy was not fully\n>     prepared to see a path that is renamed to try overwriting another\n>     path that is only different in case on case insensitive systems.\n>     This does not matter in the current code, but will start to matter\n>     once the rename detection logic starts taking hints from nearby\n>     paths moving to some directory and moves a path that is otherwise\n>     not changed along with them.\n\nYes, though I have one minor nit: I'd prefer \"a new path\" to \"a path\nthat is otherwise not changed\".\n\n(Reason for the nit: \"Not changed\" to me implies the file existed in\nthe merge base and didn't change name on either side of the merge,\nwhich in turn implies the directory remains on both sides, which means\nthere was no directory rename.  Since directory rename detection is\nmostly about moving file adds into the right directory, this seemed\nlike the simplest correction.  There are also transitive renames, but\nthey're much less common and trying to cover them too might make it\neven harder to parse.)\n\nAs a side note, the two sentences are a little bit unwieldy to try to\nparse.  I couldn't come up with any concrete suggestions to improve\nit, so it's probably fine, but thought I'd mention in case others spot\nan easy way to simplify it.\n"}]}