{"thread":{"id":"62004","subject":"How to turn off rename detection for cherry-pick?","startedAt":"2024-08-26T11:09:54Z","lastAt":"2024-08-30T17:17:17Z","messageCount":10,"participants":["Pavel Rappo","Jeff King","Elijah Newren","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"501668","messageId":"CAChcVumj41NCAcjzLyDGAyb+-QuL0Ha1AANe67jKVBT8xDRYdg@mail.gmail.com","threadId":"62004","inReplyTo":null,"subject":"How to turn off rename detection for cherry-pick?","fromName":"Pavel Rappo","fromEmail":"pavel.rappo@gmail.com","sentAt":"2024-08-26T11:09:43Z","receivedAt":"2024-08-26T11:09:54Z","isPatch":false,"sender":{"key":"pavel.rappo@gmail.com","avatar":null},"body":"As far as I understand, the \"ort\" strategy does not allow turning off\nrename detection. The best you can do is set the similarity index\nthreshold to 100%. However, it does not address the case of candidate\nfiles being exactly the same.\n\nSo, it seems to me that the only way to do it would be to downgrade to\nthe \"recursive\" strategy and set the \"no-renames\" option:\n\n git cherry-pick --strategy=recursive --strategy-option=no-renames\n...\n\nIs my understanding correct? Thanks.\n"},{"id":"501803","messageId":"CAChcVunYDO_KAmEOoWEL2q63_Gzua-Kt3BmE5Snb8==K9Cww1w@mail.gmail.com","threadId":"62004","inReplyTo":"CAChcVumj41NCAcjzLyDGAyb+-QuL0Ha1AANe67jKVBT8xDRYdg@mail.gmail.com","subject":"Re: How to turn off rename detection for cherry-pick?","fromName":"Pavel Rappo","fromEmail":"pavel.rappo@gmail.com","sentAt":"2024-08-29T08:47:52Z","receivedAt":"2024-08-29T08:48:04Z","isPatch":false,"sender":{"key":"pavel.rappo@gmail.com","avatar":null},"body":"The reason I ask this is that we've run into a (probably practically\nrare) case where cherry-pick changes a wrong file. We want to be able\nto detect such cases.\n\nDistilled from the actual repo, here is the case. Create a repo first:\n\n mkdir cherry-pick-carefully\n cd cherry-pick-carefully\n git init -b main .\n cat << EOF > x.txt\n 1\n 2\n 3\n 4\n 5\n EOF\n git add x.txt\n git commit -m \"Add x.txt\"\n git checkout -b feature\n git rm x.txt\n git add x.txt\n git commit -m \"Delete x.txt\"\n cat << EOF > y.txt\n 1\n 2\n 3\n 4\n EOF\n git add y.txt\n git commit -m \"Add y.txt, similar but not equal\"\n cat << EOF > y.txt\n 1\n 2\n 4\n EOF\n git add y.txt\n git commit -m \"Slightly change y.txt\"\n git checkout main\n\nNow try to cherry-pick feature's head commit onto main:\n\n cd cherry-pick-carefully\n git cherry-pick feature\n\nWith a default git configuration, this should (surprisingly to many)\nchange x.txt instead of y.txt, which is not what the user would\nexpect.\n"},{"id":"501875","messageId":"20240829214336.GA440013@coredump.intra.peff.net","threadId":"62004","inReplyTo":"CAChcVunYDO_KAmEOoWEL2q63_Gzua-Kt3BmE5Snb8==K9Cww1w@mail.gmail.com","subject":"Re: How to turn off rename detection for cherry-pick?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-08-29T21:43:36Z","receivedAt":"2024-08-29T21:43:38Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 29, 2024 at 09:47:52AM +0100, Pavel Rappo wrote:\n\n> The reason I ask this is that we've run into a (probably practically\n> rare) case where cherry-pick changes a wrong file. We want to be able\n> to detect such cases.\n\nYou can pass merge strategy options on the command line. The old\n\"recursive\" strategy has a \"no-renames\" option, so:\n\n git cherry-pick --strategy=recursive -Xno-renames feature\n\ngenerates a modify/delete conflict for your example. Curiously, the\nmodern default, \"ort\", does not seem to respect that option. You can\nbump up the limit to require exact renames, though, which does prevent\nthe mismerge in your case. Like:\n\n git cherry-pick -Xfind-renames=100% feature\n\nThere are also other strategies that do not do rename detection, but I\nthink you are better off using one of the more commonly-used strategies\nand just disabling renames. IMHO it's a bug that ort doesn't respect\n-Xno-renames.\n\n-Peff\n"},{"id":"501882","messageId":"CAChcVukzk=-1JNAoffWQEEv4Ne1FozGEwzGuaUWuiwhoHkcUng@mail.gmail.com","threadId":"62004","inReplyTo":"20240829214336.GA440013@coredump.intra.peff.net","subject":"Re: How to turn off rename detection for cherry-pick?","fromName":"Pavel Rappo","fromEmail":"pavel.rappo@gmail.com","sentAt":"2024-08-29T23:12:07Z","receivedAt":"2024-08-29T23:12:19Z","isPatch":false,"sender":{"key":"pavel.rappo@gmail.com","avatar":null},"body":"You seem to have confirmed my understanding that I described in my\ninitial email (you replied to my second email in this thread).\n\nOn Thu, Aug 29, 2024 at 10:43 PM Jeff King wrote:\n>\n> On Thu, Aug 29, 2024 at 09:47:52AM +0100, Pavel Rappo wrote:\n>\n> > The reason I ask this is that we've run into a (probably practically\n> > rare) case where cherry-pick changes a wrong file. We want to be able\n> > to detect such cases.\n>\n> You can pass merge strategy options on the command line. The old\n> \"recursive\" strategy has a \"no-renames\" option, so:\n>\n> git cherry-pick --strategy=recursive -Xno-renames feature\n>\n> generates a modify/delete conflict for your example. Curiously, the\n> modern default, \"ort\", does not seem to respect that option. You can\n> bump up the limit to require exact renames, though, which does prevent\n> the mismerge in your case. Like:\n>\n> git cherry-pick -Xfind-renames=100% feature\n>\n> There are also other strategies that do not do rename detection, but I\n> think you are better off using one of the more commonly-used strategies\n> and just disabling renames. IMHO it's a bug that ort doesn't respect\n> -Xno-renames.\n>\n> -Peff\n"},{"id":"501885","messageId":"20240830003147.GA450797@coredump.intra.peff.net","threadId":"62004","inReplyTo":"CAChcVukzk=-1JNAoffWQEEv4Ne1FozGEwzGuaUWuiwhoHkcUng@mail.gmail.com","subject":"Re: How to turn off rename detection for cherry-pick?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-08-30T00:31:47Z","receivedAt":"2024-08-30T00:31:48Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 30, 2024 at 12:12:07AM +0100, Pavel Rappo wrote:\n\n> You seem to have confirmed my understanding that I described in my\n> initial email (you replied to my second email in this thread).\n\nHeh, I did not even see the first message in the thread. But since we\nindependently arrived at the same conclusions, I guess we can consider\neverything there accurate. :)\n\nI do think it's a bug that ort doesn't respect -Xno-renames. The fix is\nprobably something like this:\n \ndiff --git a/merge-ort.c b/merge-ort.c\nindex 3752c7e595..94b3ce734c 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -3338,7 +3338,7 @@ static int detect_regular_renames(struct merge_options *opt,\n \trepo_diff_setup(opt->repo, &diff_opts);\n \tdiff_opts.flags.recursive = 1;\n \tdiff_opts.flags.rename_empty = 0;\n-\tdiff_opts.detect_rename = DIFF_DETECT_RENAME;\n+\tdiff_opts.detect_rename = opts->detect_rename;\n \tdiff_opts.rename_limit = opt->rename_limit;\n \tif (opt->rename_limit <= 0)\n \t\tdiff_opts.rename_limit = 7000;\n\nthough I'm not sure how DIFF_DETECT_COPIES should be handled\n(\"recursive\" silently downgrades it to DIFF_DETECT_RENAME).\n\nI've added ort's author to the thread, so hopefully he should have a\nmore clueful response.\n\n-Peff\n"},{"id":"501917","messageId":"CAChcVu=MSLr7Gaf8nmQdVr=Mm=nXkWEjMd9MNNM3dcr=-1igDA@mail.gmail.com","threadId":"62004","inReplyTo":"20240830003147.GA450797@coredump.intra.peff.net","subject":"Re: How to turn off rename detection for cherry-pick?","fromName":"Pavel Rappo","fromEmail":"pavel.rappo@gmail.com","sentAt":"2024-08-30T10:35:03Z","receivedAt":"2024-08-30T10:35:15Z","isPatch":false,"sender":{"key":"pavel.rappo@gmail.com","avatar":null},"body":"On Fri, Aug 30, 2024 at 1:31 AM Jeff King wrote:\n>\n> \n>\n> I've added ort's author to the thread, so hopefully he should have a\n> more clueful response.\n>\n\nThanks!\n\nSome time ago I created a gist which might better summarise the issue:\nhttps://gist.github.com/pavelrappo/3b1cd89b6015ab0eade1b11876d563ff\n"},{"id":"501922","messageId":"CABPp-BG-Nx6SCxxkGXn_Fwd2wseifMFND8eddvWxiZVZk0zRaA@mail.gmail.com","threadId":"62004","inReplyTo":"20240830003147.GA450797@coredump.intra.peff.net","subject":"Re: How to turn off rename detection for cherry-pick?","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2024-08-30T14:33:51Z","receivedAt":"2024-08-30T14:34:04Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Aug 29, 2024 at 5:31 PM Jeff King wrote:\n>\n> On Fri, Aug 30, 2024 at 12:12:07AM +0100, Pavel Rappo wrote:\n>\n> > You seem to have confirmed my understanding that I described in my\n> > initial email (you replied to my second email in this thread).\n>\n> Heh, I did not even see the first message in the thread. But since we\n> independently arrived at the same conclusions, I guess we can consider\n> everything there accurate. :)\n>\n> I do think it's a bug that ort doesn't respect -Xno-renames.\n\nI'm kind of splitting hairs, but doesn't \"bug\" imply it was\noverlooked? Documentation/merge-strategies.txt makes it clear it\nwasn't. ;-)\n\nHowever, I agree that it makes sense to start supporting. I didn't at\nfirst because (a) I could find no evidence at the time that anyone\nactually ever used the option in conjunction with merges for\nbehavioral reasons (this thread from Pavel is the first counterexample\nI've seen), and (b) there was lots of evidence that the related config\noption was in widespread use as a workaround to the unnecessary\nperformance problems of rename handling with the recursive merge\nalgorithm. In particular, I wanted to hear about any performance\nissues with renames in merges, and it was far easier to make the new\n(then-experimental) algorithm just ignore these options than attempt\nto do widespread convincing of folks to switch the relevant config\nback on. I think that a few years has given us more than ample time\nto hear about potential remaining performance issues with renames in\nmerges, and this thread is a good example of why to support this\noption.\n\n> The fix is probably something like this:\n\nThe fix probably starts with something like this...\n\n> diff --git a/merge-ort.c b/merge-ort.c\n> index 3752c7e595..94b3ce734c 100644\n> --- a/merge-ort.c\n> +++ b/merge-ort.c\n> @@ -3338,7 +3338,7 @@ static int detect_regular_renames(struct merge_options *opt,\n> repo_diff_setup(opt->repo, &diff_opts);\n> diff_opts.flags.recursive = 1;\n> diff_opts.flags.rename_empty = 0;\n> - diff_opts.detect_rename = DIFF_DETECT_RENAME;\n> + diff_opts.detect_rename = opts->detect_rename;\n> diff_opts.rename_limit = opt->rename_limit;\n> if (opt->rename_limit <= 0)\n> diff_opts.rename_limit = 7000;\n>\n> though I'm not sure how DIFF_DETECT_COPIES should be handled\n> (\"recursive\" silently downgrades it to DIFF_DETECT_RENAME).\n\nNot downgrading DIFF_DETECT_COPIES for merging would break both\n\"recursive\" and \"ort\" in all kinds of interesting ways. I once spent\na little time thinking how copy detection might affect things (long\nbefore working on ort), and noted that under such a scenario,\n\"recursive\" would have multiple logic bugs (mostly in the form of\nmissing additional needed logic rather than existing logic being\nincorrect, but in places that aren't necessarily obvious at first).\nSomeone would have to very carefully audit the whole file containing\neither recursive or ort's algorithms if they wanted to make either\nsomehow support copy detection. \"ort\" would be even more problematic\nthan \"recursive\" for such a case -- I took full advantage of the\ndifferences between rename detection and copy detection while\noptimizing ort and I think it was intrinsic to every one of the major\noptimizations I did. So, if you really wanted to support copy\ndetection in ort, the very first step would be adding code to turn off\nevery single one of its major optimizations (including tree-level\nmerging, which didn't sound like a rename or copy detection like\nthing, but hinged on how rename detection works to make sure it didn't\nmiss important halfs of renames).\n\nBut, the bigger issue with copies is how exactly could a merge\nalgorithm use them in any way that would make any logical sense? What\nare you going to do, take the modifications that one side of history\nmade to some source file, and apply those modifications to all the\ncopies of the file that the other side of history made? That sounds\ncrazy and counter-intuitive to me. (...and incidentally, like a\nfactory for creating all kinds of crazy corner case issues; we could\nprobably make things even messier than the mod6 testcases in the\ntestsuite.)\n\n> I've added ort's author to the thread, so hopefully he should have a\n> more clueful response.\n\nYeah, not so straightforward. Even with the downgrading of copy\ndetection to rename detection in this area (like \"recursive\" does),\nthis isn't sufficient. IIRC (I looked at this about 2 years ago or so\nwhen some others asked off-list), at least one of the optimizations\nmanaged to bake in an assumption about having gone through the rename\ndetection codepaths. As best I remember, when you attempt to turn\nrename detection off, it triggers an assertion failure in a\nnon-obvious place far removed from the original issue, and I only ever\ngot a slightly hacky workaround or two for the issue. I didn't have\ngood motivation or rationale to pursue very far at the time, though,\nso after some hours of looking at it, I just decided to move on to\nsomething else.\n\nAnyway, I'll add support for no-renames to my list of things needed\nbefore we can delete merge-recursive.[ch] and make requests for\n\"recursive\" just map to \"ort\".\n"},{"id":"501923","messageId":"xmqq8qwet2c8.fsf@gitster.g","threadId":"62004","inReplyTo":"20240830003147.GA450797@coredump.intra.peff.net","subject":"Re: How to turn off rename detection for cherry-pick?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-30T16:14:31Z","receivedAt":"2024-08-30T16:14:39Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> though I'm not sure how DIFF_DETECT_COPIES should be handled\n> (\"recursive\" silently downgrades it to DIFF_DETECT_RENAME).\n\nI somehow suspect that we would want to go there to avoid\noverloading ourselves (and end-users) with conceptual complexity.\n\nIf we detect that the file the side branch modified was copied to\ncreate multiple files on our side (with or without modification on\nour end), where would we want to reapply their changes to? To the\noriginal file only? To all copies? The same story goes for the\nopposite direction. How should possibly different changes they made\nto their copies be merged back to the sole original file we have\n(which we may or may not have modified)? What if the both sides\nmade copies of the same file, and now we need to shuffle NxM\ncombination of changes? I would expect it would be a whole can of\nworms, which we _may_ be able to define concrete rules how we would\nhandle each of these cases but we may have a hard time to explain in\nsimple terms to end-users.\n\n\n"},{"id":"501927","messageId":"xmqqmskurm69.fsf@gitster.g","threadId":"62004","inReplyTo":"CABPp-BG-Nx6SCxxkGXn_Fwd2wseifMFND8eddvWxiZVZk0zRaA@mail.gmail.com","subject":"Re: How to turn off rename detection for cherry-pick?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-30T16:49:02Z","receivedAt":"2024-08-30T16:49:06Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> Anyway, I'll add support for no-renames to my list of things needed\n> before we can delete merge-recursive.[ch] and make requests for\n> \"recursive\" just map to \"ort\".\n\nSounds sensible. Thanks.\n"},{"id":"501928","messageId":"xmqqikvirkv8.fsf@gitster.g","threadId":"62004","inReplyTo":"xmqq8qwet2c8.fsf@gitster.g","subject":"Re: How to turn off rename detection for cherry-pick?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-30T17:17:15Z","receivedAt":"2024-08-30T17:17:17Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jeff King writes:\n>\n>> though I'm not sure how DIFF_DETECT_COPIES should be handled\n>> (\"recursive\" silently downgrades it to DIFF_DETECT_RENAME).\n>\n> I somehow suspect that we would want to go there to avoid\n> overloading ourselves (and end-users) with conceptual complexity.\n\nOf course, we would *NOT* want to go there X-<.\n"}]}