{"thread":{"id":"41409","subject":"Custom merge driver with no rename detection","startedAt":"2016-02-14T19:51:10Z","lastAt":"2016-02-16T01:04:40Z","messageCount":8,"participants":["Felipe Gonçalves Assis","Junio C Hamano","Johannes Schindelin"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"278119","messageId":"CALMa68ovz=VZYkCcUDvEn1d7=xJDx__71caqsPXUFASZ1phfdw@mail.gmail.com","threadId":"41409","inReplyTo":null,"subject":"Custom merge driver with no rename detection","fromName":"Felipe Gonçalves Assis","fromEmail":"felipeg.assis@gmail.com","sentAt":"2016-02-14T19:51:10Z","receivedAt":"2016-02-14T19:51:10Z","isPatch":false,"sender":{"key":"felipeg.assis@gmail.com","avatar":null},"body":"Hi,\n\nI would like to set up a Git repository with a custom merge driver,\nand then disable rename detection when merging.\n\nUnfortunately, the recursive strategy has no \"no-renames\" option. Note\nthat I would like to avoid rename detection even when the file\ncontents perfectly match.\n\nThe usual workaround is using the resolve strategy, but apparently it\nignores the custom merge driver. Besides, I would really like to use\nthe recursive strategy logic.\n\nIs there any solution to this in the current version?\n\nIf not, what do you think about adding a \"no-renames\" option to the\nrecursive strategy? I could work on a patch.\n\nAttached is a quick and dirty patch that emulates the effect by\nallowing greater than 100% rename thresholds to mean \"no-renames\".\n\nAnd here is a related question on StackOverflow:\nhttp://stackoverflow.com/questions/35135517/custom-git-merge-driver-with-no-rename-detection\n\nThanks in advance for any attention provided.\n\nRegards,\nFelipe Gonçalves Assis\n\n\nFrom bcef6c44fac3a29afe03408ef27024776da861ee Mon Sep 17 00:00:00 2001\nFrom: =?UTF-8?q?Felipe=20Gon=C3=A7alves=20Assis?= \nDate: Sun, 14 Feb 2016 17:02:00 -0200\nSubject: [PATCH] diff: rename threshold above 100% means no renames\n\n---\n diff.c | 2 +-\n diffcore-rename.c | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 2136b69..43b9e0a 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4003,7 +4003,7 @@ int parse_rename_score(const char **cp_p)\n \t/* user says num divided by scale and we say internally that\n \t * is MAX_SCORE * num / scale.\n \t */\n-\treturn (int)((num >= scale) ? MAX_SCORE : (MAX_SCORE * num / scale));\n+\treturn (int)(MAX_SCORE * num / scale);\n }\n \n static int diff_scoreopt_parse(const char *opt)\ndiff --git a/diffcore-rename.c b/diffcore-rename.c\nindex af1fe08..7cb5a3b 100644\n--- a/diffcore-rename.c\n+++ b/diffcore-rename.c\n@@ -497,7 +497,7 @@ void diffcore_rename(struct diff_options *options)\n \t\t\tregister_rename_src(p);\n \t\t}\n \t}\n-\tif (rename_dst_nr == 0 || rename_src_nr == 0)\n+\tif (rename_dst_nr == 0 || rename_src_nr == 0 || minimum_score > MAX_SCORE)\n \t\tgoto cleanup; /* nothing to do */\n \n \t/*\n-- \n2.7.1.287.g4943984\n\n"},{"id":"278130","messageId":"xmqqpovyr22u.fsf@gitster.mtv.corp.google.com","threadId":"41409","inReplyTo":"CALMa68ovz=VZYkCcUDvEn1d7=xJDx__71caqsPXUFASZ1phfdw@mail.gmail.com","subject":"Re: Custom merge driver with no rename detection","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-15T05:10:17Z","receivedAt":"2016-02-15T05:10:17Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Gonçalves Assis writes:\n\n> The usual workaround is using the resolve strategy, but apparently it\n> ignores the custom merge driver.\n\nHmph.\n\nIndeed, git-merge-file seems to call xdl_merge() directly, bypassing\nthe ll_merge(), which is understandable as the former predates the\nlatter. That needs to be fixed, I think.\n"},{"id":"278136","messageId":"alpine.DEB.2.20.1602150858340.2964@virtualbox","threadId":"41409","inReplyTo":"CALMa68ovz=VZYkCcUDvEn1d7=xJDx__71caqsPXUFASZ1phfdw@mail.gmail.com","subject":"Re: Custom merge driver with no rename detection","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-02-15T08:06:49Z","receivedAt":"2016-02-15T08:06:49Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Felipe,\n\nOn Sun, 14 Feb 2016, Felipe Gonçalves Assis wrote:\n\n> Attached is a quick and dirty patch that emulates the effect by\n> allowing greater than 100% rename thresholds to mean \"no-renames\".\n\nIt is really hard to comment on attached patches.\n\nFirst comment: the commit message is awfully empty, and lacks a sign-off.\n\n> /* user says num divided by scale and we say internally that\n> * is MAX_SCORE * num / scale.\n> */\n> - return (int)((num >= scale) ? MAX_SCORE : (MAX_SCORE * num / scale));\n> + return (int)(MAX_SCORE * num / scale);\n\nUh oh. I suspect this opens the door pretty wide for integer overflows. I\ncould imagine that something like\n\n\treturn (int)(num > scale ? MAX_SCORE + 1 : MAX_SCORE * num / scale);\n\nwould work better, but it still would need some careful consideration.\n\n> static int diff_scoreopt_parse(const char *opt)\n> diff --git a/diffcore-rename.c b/diffcore-rename.c\n> index af1fe08..7cb5a3b 100644\n> --- a/diffcore-rename.c\n> +++ b/diffcore-rename.c\n> @@ -497,7 +497,7 @@ void diffcore_rename(struct diff_options *options)\n> register_rename_src(p);\n> }\n> }\n> - if (rename_dst_nr == 0 || rename_src_nr == 0)\n> + if (rename_dst_nr == 0 || rename_src_nr == 0 || minimum_score > MAX_SCORE)\n\nThis line is too long now.\n\nCiao,\nJohannes"},{"id":"278137","messageId":"alpine.DEB.2.20.1602150854520.2964@virtualbox","threadId":"41409","inReplyTo":"xmqqpovyr22u.fsf@gitster.mtv.corp.google.com","subject":"Re: Custom merge driver with no rename detection","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-02-15T08:06:53Z","receivedAt":"2016-02-15T08:06:53Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Sun, 14 Feb 2016, Junio C Hamano wrote:\n\n> Felipe Gonçalves Assis writes:\n> \n> > The usual workaround is using the resolve strategy, but apparently it\n> > ignores the custom merge driver.\n> \n> Hmph.\n> \n> Indeed, git-merge-file seems to call xdl_merge() directly, bypassing\n> the ll_merge(), which is understandable as the former predates the\n> latter. That needs to be fixed, I think.\n\nI think this is by design. (Because I designed it.)\n\nThe original idea of git-merge-file was to serve as a drop-in replacement\nfor GNU/BSD merge when you want to avoid to be subject to the vagaries of\nthe GNU vs BSD implementations.\n\nCiao,\nDscho"},{"id":"278148","messageId":"xmqqlh6mqors.fsf@gitster.mtv.corp.google.com","threadId":"41409","inReplyTo":"alpine.DEB.2.20.1602150854520.2964@virtualbox","subject":"Re: Custom merge driver with no rename detection","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-15T09:57:43Z","receivedAt":"2016-02-15T09:57:43Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> On Sun, 14 Feb 2016, Junio C Hamano wrote:\n>\n>> Felipe Gonçalves Assis writes:\n>> \n>> > The usual workaround is using the resolve strategy, but apparently it\n>> > ignores the custom merge driver.\n>> \n>> Hmph.\n>> \n>> Indeed, git-merge-file seems to call xdl_merge() directly, bypassing\n>> the ll_merge(), which is understandable as the former predates the\n>> latter. That needs to be fixed, I think.\n>\n> I think this is by design. (Because I designed it.)\n>\n> The original idea of git-merge-file was to serve as a drop-in replacement\n> for GNU/BSD merge when you want to avoid to be subject to the vagaries of\n> the GNU vs BSD implementations.\n\nWe did use to use \"merge\" from the RCS suite originally, and we did\nwant to use our own, but the primary reason we added our own was so\nthat it can be enhanced in sync with the remainder of Git in a\nconsistent way.\n\nIf the rest of Git can be told via the attribute system to make use\nof a three-way merge driver, it should have learnt the trick to be\nkept up with the rest of the system. I see the current state of the\nprogram merely be staying a bit behind; I do not think it was part\nof the grand design to forbidd it forever from learning new optional\ntricks.\n"},{"id":"278157","messageId":"CALMa68p9jdO63k2NLTUJXyms=Fc+woXx00UXCxzJ5_zV8jO8Rw@mail.gmail.com","threadId":"41409","inReplyTo":"alpine.DEB.2.20.1602150858340.2964@virtualbox","subject":"Re: Custom merge driver with no rename detection","fromName":"Felipe Gonçalves Assis","fromEmail":"felipeg.assis@gmail.com","sentAt":"2016-02-15T10:42:34Z","receivedAt":"2016-02-15T10:42:34Z","isPatch":false,"sender":{"key":"felipeg.assis@gmail.com","avatar":null},"body":"On 15 February 2016 at 06:06, Johannes Schindelin\n wrote:\n> Hi Felipe,\n>\n> On Sun, 14 Feb 2016, Felipe Gonçalves Assis wrote:\n>\n>> Attached is a quick and dirty patch that emulates the effect by\n>> allowing greater than 100% rename thresholds to mean \"no-renames\".\n>\n> It is really hard to comment on attached patches.\n>\n> First comment: the commit message is awfully empty, and lacks a sign-off.\n>\n\nThanks. I am sorry for not submitting the patch separately in an\nemail, but, it was really not meant for serious review, just for\nemulating the desired effect with minimal effort.\n\nHowever, if you do find this approach acceptable/desirable\n(rename-threshold > 100%), I can work on the issues pointed out and\npropose a proper patch.\n\nRegards,\nFelipe\n"},{"id":"278158","messageId":"xmqqd1ryqlpr.fsf@gitster.mtv.corp.google.com","threadId":"41409","inReplyTo":"CALMa68p9jdO63k2NLTUJXyms=Fc+woXx00UXCxzJ5_zV8jO8Rw@mail.gmail.com","subject":"Re: Custom merge driver with no rename detection","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-15T11:03:44Z","receivedAt":"2016-02-15T11:03:44Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Gonçalves Assis writes:\n\n> However, if you do find this approach acceptable/desirable\n> (rename-threshold > 100%), I can work on the issues pointed out and\n> propose a proper patch.\n\nThe caller asks diffcore-rename to detect rename, and the algorithm\ncompares things to come up with a similarity score and match things\nup. And you add an option to the rename detection logic to forbid\nfinding any?\n\nTo be bluntly honest, the approach sounds like a crazy talk.\n\nIf your goal is not to allow rename detection at all, why not teach\nthe caller of the diff machinery not to even ask for rename\ndetection at all? merge-recursive.c has a helper function called\nget_renames(), and it calls into the diff machinery passing\nDIFF_DETECT_RENAME option. As a dirty hack, I think you would\nachieve your desired result if you stop passing that option there.\n\nI called that a \"dirty hack\", because for the purpose of not\nallowing rename detection inside merge-recursive, I think an even\nbetter thing to do is to teach the get_renames() helper to report to\nits caller, under your new option, \"I found no renames at all\"\nwithout doing anything.\n\nIt might be just a simple matter of teaching its callers (there\nprobably are two of them, one between the common ancestor and our\nbranch and the other between the common ancestor and their branch)\nnot call the get_renames() helper at all under your new option, but\nI didn't check these callers closely.\n"},{"id":"278253","messageId":"CALMa68o3EsnLaKUmQstkF5KOtiqVkLF4_-Q1eWPyAQQF_adS+Q@mail.gmail.com","threadId":"41409","inReplyTo":"xmqqd1ryqlpr.fsf@gitster.mtv.corp.google.com","subject":"Re: Custom merge driver with no rename detection","fromName":"Felipe Gonçalves Assis","fromEmail":"felipeg.assis@gmail.com","sentAt":"2016-02-16T01:04:40Z","receivedAt":"2016-02-16T01:04:40Z","isPatch":false,"sender":{"key":"felipeg.assis@gmail.com","avatar":null},"body":"On 15 February 2016 at 09:03, Junio C Hamano wrote:\n> Felipe Gonçalves Assis writes:\n>\n>> However, if you do find this approach acceptable/desirable\n>> (rename-threshold > 100%), I can work on the issues pointed out and\n>> propose a proper patch.\n>\n> The caller asks diffcore-rename to detect rename, and the algorithm\n> compares things to come up with a similarity score and match things\n> up. And you add an option to the rename detection logic to forbid\n> finding any?\n>\n> To be bluntly honest, the approach sounds like a crazy talk.\n>\n> If your goal is not to allow rename detection at all, why not teach\n> the caller of the diff machinery not to even ask for rename\n> detection at all? merge-recursive.c has a helper function called\n> get_renames(), and it calls into the diff machinery passing\n> DIFF_DETECT_RENAME option. As a dirty hack, I think you would\n> achieve your desired result if you stop passing that option there.\n>\n> I called that a \"dirty hack\", because for the purpose of not\n> allowing rename detection inside merge-recursive, I think an even\n> better thing to do is to teach the get_renames() helper to report to\n> its caller, under your new option, \"I found no renames at all\"\n> without doing anything.\n>\n> It might be just a simple matter of teaching its callers (there\n> probably are two of them, one between the common ancestor and our\n> branch and the other between the common ancestor and their branch)\n> not call the get_renames() helper at all under your new option, but\n> I didn't check these callers closely.\n\nThanks for all the ideas. In order to have something concrete to\ndiscuss, I submitted the patch \"merge-recursive: option to disable\nrenames\".\n\nIs that what you had in mind? I would be happy to receive comments and\neither improve it or try something else.\n\nFelipe\n"}]}