{"thread":{"id":"48335","subject":"[PATCH v1 1/2] merge: Add merge.renames config setting","startedAt":"2018-04-20T13:36:54Z","lastAt":"2018-05-04T03:07:50Z","messageCount":65,"participants":["Ben Peart","Elijah Newren","Junio C Hamano","Eckhard Maaß","Johannes Schindelin","Jonathan Tan"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"345222","messageId":"20180420133632.17580-2-benpeart@microsoft.com","threadId":"48335","inReplyTo":"20180420133632.17580-1-benpeart@microsoft.com","subject":"[PATCH v1 1/2] merge: Add merge.renames config setting","fromName":"Ben Peart","fromEmail":"ben.peart@microsoft.com","sentAt":"2018-04-20T13:36:48Z","receivedAt":"2018-04-20T13:36:54Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"Add the ability to control rename detection for merge via a config setting.\n\nReviewed-by: Johannes Schindelin \nSigned-off-by: Ben Peart \n---\n Documentation/merge-config.txt | 5 +++++\n merge-recursive.c | 1 +\n 2 files changed, 6 insertions(+)\n\ndiff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt\nindex 12b6bbf591..656f909eb3 100644\n--- a/Documentation/merge-config.txt\n+++ b/Documentation/merge-config.txt\n@@ -37,6 +37,11 @@ merge.renameLimit::\n \tduring a merge; if not specified, defaults to the value of\n \tdiff.renameLimit.\n \n+merge.renames::\n+\tWhether and how Git detects renames. If set to \"false\",\n+\trename detection is disabled. If set to \"true\", basic rename\n+\tdetection is enabled. This is the default.\n+\n merge.renormalize::\n \tTell Git that canonical representation of files in the\n \trepository has changed over time (e.g. earlier commits record\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 9c05eb7f70..cd5367e890 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -3256,6 +3256,7 @@ static void merge_recursive_config(struct merge_options *o)\n \tgit_config_get_int(\"merge.verbosity\", &o->verbosity);\n \tgit_config_get_int(\"diff.renamelimit\", &o->diff_rename_limit);\n \tgit_config_get_int(\"merge.renamelimit\", &o->merge_rename_limit);\n+\tgit_config_get_bool(\"merge.renames\", &o->detect_rename);\n \tgit_config(git_xmerge_config, NULL);\n }\n \n-- \n2.17.0.windows.1\n\n"},{"id":"345223","messageId":"20180420133632.17580-1-benpeart@microsoft.com","threadId":"48335","inReplyTo":null,"subject":"[PATCH v1 0/2] add additional config settings for merge","fromName":"Ben Peart","fromEmail":"ben.peart@microsoft.com","sentAt":"2018-04-20T13:36:47Z","receivedAt":"2018-04-20T13:36:58Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"This enables the user to set a couple of additional options for merge.\n\n1. merge.aggressive - this is to try to resolve a few more trivial\n merge cases. It is documented in read-tree and is not something you\n can pass into merge itself.\n\n2. merge.renames - this is to save git from having to go through the entire\n 3 trees to see if there were any renames that happened.\n\nFor the work item repro that I have been using this drops the merge time\nfrom ~1 hour to ~5 minutes and the unmerged entries goes down from\n~40,000 to 1.\n\nHelped-by: Kevin Willford \nReviewed-by: Johannes Schindelin \nSigned-off-by: Ben Peart \n\nBase Ref: master\nWeb-Diff: https://github.com/benpeart/git/commit/a3d157f5be\nCheckout: git fetch https://github.com/benpeart/git merge-options-v1 && git checkout a3d157f5be\n\nBen Peart (2):\n merge: Add merge.renames config setting\n merge: Add merge.aggressive config setting\n\n Documentation/merge-config.txt | 9 +++++++++\n merge-recursive.c | 2 ++\n 2 files changed, 11 insertions(+)\n\n\nbase-commit: 0b0cc9f86731f894cff8dd25299a9b38c254569e\n-- \n2.17.0.windows.1\n\n\n"},{"id":"345224","messageId":"20180420133632.17580-3-benpeart@microsoft.com","threadId":"48335","inReplyTo":"20180420133632.17580-1-benpeart@microsoft.com","subject":"[PATCH v1 2/2] merge: Add merge.aggressive config setting","fromName":"Ben Peart","fromEmail":"ben.peart@microsoft.com","sentAt":"2018-04-20T13:36:49Z","receivedAt":"2018-04-20T13:37:02Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"Add the ability to control the aggressive flag passed to read-tree via a config setting.\n\nReviewed-by: Johannes Schindelin \nSigned-off-by: Ben Peart \n---\n Documentation/merge-config.txt | 4 ++++\n merge-recursive.c | 1 +\n 2 files changed, 5 insertions(+)\n\ndiff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt\nindex 656f909eb3..5a9ab969db 100644\n--- a/Documentation/merge-config.txt\n+++ b/Documentation/merge-config.txt\n@@ -1,3 +1,7 @@\n+merge.aggressive::\n+\tPasses \"aggressive\" to read-tree which makes the command resolve\n+\ta few more cases internally. See \"--aggressive\" in linkgit:git-read-tree[1].\n+\n merge.conflictStyle::\n \tSpecify the style in which conflicted hunks are written out to\n \tworking tree files upon merge. The default is \"merge\", which\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex cd5367e890..0ca84e4b82 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -355,6 +355,7 @@ static int git_merge_trees(struct merge_options *o,\n \to->unpack_opts.fn = threeway_merge;\n \to->unpack_opts.src_index = &the_index;\n \to->unpack_opts.dst_index = &the_index;\n+\tgit_config_get_bool(\"merge.aggressive\", (int *)&o->unpack_opts.aggressive);\n \tsetup_unpack_trees_porcelain(&o->unpack_opts, \"merge\");\n \n \tinit_tree_desc_from_tree(t+0, common);\n-- \n2.17.0.windows.1\n\n"},{"id":"345235","messageId":"CABPp-BFANBs=tOhS5BFfTMkdQsNYbUDExWK8QB0V=qD9YwZyWw@mail.gmail.com","threadId":"48335","inReplyTo":"20180420133632.17580-2-benpeart@microsoft.com","subject":"Re: [PATCH v1 1/2] merge: Add merge.renames config setting","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-04-20T17:02:30Z","receivedAt":"2018-04-20T17:02:35Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Apr 20, 2018 at 6:36 AM, Ben Peart wrote:\n> --- a/Documentation/merge-config.txt\n> +++ b/Documentation/merge-config.txt\n> @@ -37,6 +37,11 @@ merge.renameLimit::\n> during a merge; if not specified, defaults to the value of\n> diff.renameLimit.\n>\n> +merge.renames::\n> + Whether and how Git detects renames. If set to \"false\",\n> + rename detection is disabled. If set to \"true\", basic rename\n> + detection is enabled. This is the default.\n\nOne can already control o->detect_rename via the -Xno-renames and\n-Xfind-renames options. I think the documentation should mention that\n\"false\" is the same as passing -Xno-renames, and \"true\" is the same as\npassing -Xfind-renames. However, find-renames does take similarity\nthreshold as a parameter, so there's a question whether this option\nshould provide some way to do the same. I'm not sure the answer to\nthat; it may be that we'd want a separate config option for that, and\nwe can wait to add it until someone actually wants it.\n\n> merge.renormalize::\n> Tell Git that canonical representation of files in the\n> repository has changed over time (e.g. earlier commits record\n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index 9c05eb7f70..cd5367e890 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -3256,6 +3256,7 @@ static void merge_recursive_config(struct merge_options *o)\n> git_config_get_int(\"merge.verbosity\", &o->verbosity);\n> git_config_get_int(\"diff.renamelimit\", &o->diff_rename_limit);\n> git_config_get_int(\"merge.renamelimit\", &o->merge_rename_limit);\n> + git_config_get_bool(\"merge.renames\", &o->detect_rename);\n> git_config(git_xmerge_config, NULL);\n> }\n\nI would expect an explicitly passed -Xno-renames or -Xfind-renames to\noverride the config setting. Could you check if that's the case?\n\nAlso, if someone sets merge.renameLimit (to anything) and sets\nmerge.renames to false, then they've got a contradictory setup. Does\nit make sense to check and warn about that anywhere?\n"},{"id":"345237","messageId":"CABPp-BFXwbZfFe0bZYMwWxz_Qxw=KQ6XE5SEBmgiE+TzaSycuQ@mail.gmail.com","threadId":"48335","inReplyTo":"20180420133632.17580-3-benpeart@microsoft.com","subject":"Re: [PATCH v1 2/2] merge: Add merge.aggressive config setting","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-04-20T17:22:49Z","receivedAt":"2018-04-20T17:22:54Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Apr 20, 2018 at 6:36 AM, Ben Peart wrote:\n> Add the ability to control the aggressive flag passed to read-tree via a config setting.\n\nThis feels like a workaround to the performance problems with index\nupdates in merge-recursive.c. That said, it makes sense to me to do\nthis when rename detection is turned off. In fact, I think you'd\nautomatically want to set aggressive to true whenever rename detection\nis turned off (whether by your merge.renames option or the\n-Xno-renames flag).\n\nI can't think of any reason this setting would be useful separate from\nturning rename detection off, and it'd actively harm rename detection\nperformance improvements I have in the pipeline. I'd really prefer to\nnot add this option, and instead combine the setting of aggressive\nwith the other flag. Do you have an independent reason for wanting\nthis?\n\nThanks,\nElijah\n"},{"id":"345238","messageId":"CABPp-BEhvLVTL3+0scUucAp9ZMBiiT_0VG3eeKm9qRnHG=y+tw@mail.gmail.com","threadId":"48335","inReplyTo":"CABPp-BFANBs=tOhS5BFfTMkdQsNYbUDExWK8QB0V=qD9YwZyWw@mail.gmail.com","subject":"Re: [PATCH v1 1/2] merge: Add merge.renames config setting","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-04-20T17:26:45Z","receivedAt":"2018-04-20T17:26:49Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Apr 20, 2018 at 10:02 AM, Elijah Newren wrote:\n> On Fri, Apr 20, 2018 at 6:36 AM, Ben Peart wrote:\n>> --- a/Documentation/merge-config.txt\n>> +++ b/Documentation/merge-config.txt\n>> @@ -37,6 +37,11 @@ merge.renameLimit::\n>> during a merge; if not specified, defaults to the value of\n>> diff.renameLimit.\n>>\n>> +merge.renames::\n>> + Whether and how Git detects renames. If set to \"false\",\n>> + rename detection is disabled. If set to \"true\", basic rename\n>> + detection is enabled. This is the default.\n>\n> One can already control o->detect_rename via the -Xno-renames and\n> -Xfind-renames options. I think the documentation should mention that\n> \"false\" is the same as passing -Xno-renames, and \"true\" is the same as\n> passing -Xfind-renames. However, find-renames does take similarity\n> threshold as a parameter, so there's a question whether this option\n> should provide some way to do the same. I'm not sure the answer to\n> that; it may be that we'd want a separate config option for that, and\n> we can wait to add it until someone actually wants it.\n\nI just realized another issue, though it also affects -Xno-renames.\nEven if rename detection is turned off for the merge, it is\nunconditionally turned on for the diffstat. In builtin/merge.c,\nfunction finish(), there is the code:\n\n if (new_head && show_diffstat) {\n ...\n opts.detect_rename = DIFF_DETECT_RENAME;\n\nIt seems that this option should affect that line as well. (Do you\nhave diffstat turned off by chance? If not, you may be able to\nimprove your performance even more...)\n"},{"id":"345240","messageId":"CABPp-BEwwn+NwOEtWOKOdUKxoXfq6YwWeoH6OwkPjSwVtTm5=Q@mail.gmail.com","threadId":"48335","inReplyTo":"20180420133632.17580-1-benpeart@microsoft.com","subject":"Re: [PATCH v1 0/2] add additional config settings for merge","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-04-20T17:34:48Z","receivedAt":"2018-04-20T17:34:55Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Apr 20, 2018 at 6:36 AM, Ben Peart wrote:\n> This enables the user to set a couple of additional options for merge.\n>\n> 1. merge.aggressive - this is to try to resolve a few more trivial\n> merge cases. It is documented in read-tree and is not something you\n> can pass into merge itself.\n>\n> 2. merge.renames - this is to save git from having to go through the entire\n> 3 trees to see if there were any renames that happened.\n>\n> For the work item repro that I have been using this drops the merge time\n> from ~1 hour to ~5 minutes and the unmerged entries goes down from\n> ~40,000 to 1.\n\nOoh, this is *very* interesting. Is there any chance I could also get\nyou to test performing the same merge with the version of git at\nhttps://github.com/newren/git/tree/big-repo-small-cherry-pick and\nreport on your timings?\n\nThe 'big-repo-small-cherry-pick' name could be improved, but that\nbranch has a number of performance fixes for really poor rename\ndetection performance during merges. From your description, I'm\npretty sure it'll apply to your case. For my specific testcase, I\ngot a speedup factor of 30. Someone else on the list saw a factor of\n24[1]. Results are highly dependent on the specific repo, but it's\ncertainly possible that it gets much of your factor of 12 speedup that\nyou saw with these new config settings you added.\n\nHowever, what makes this case even more interesting to me is that my\nbranch may not be quite as effective as your workarounds. There are\nother other performance issues in merge that I am aware of, but for\nwhich I haven't had the time to write the patches yet (I've been\nwaiting for the directory rename detection stuff to land and settle\ndown before working more on the performance aspects). I do not know\nhow big a factor those other performance issues are, but your\nworkarounds (namely the aggressive setting) may get around some of\nthose other issues as well, so I'm very interested to see how my\ncurrent branch compares to the speedups you got with these settings.\n\nThanks,\nElijah\n\n\n[1] https://public-inbox.org/git/alpine.DEB.2.00.1711211303290.20686@ds9.cixit.se/\n"},{"id":"345243","messageId":"cd49481c-9665-124a-5f94-791f1a16657d@gmail.com","threadId":"48335","inReplyTo":"CABPp-BFANBs=tOhS5BFfTMkdQsNYbUDExWK8QB0V=qD9YwZyWw@mail.gmail.com","subject":"Re: [PATCH v1 1/2] merge: Add merge.renames config setting","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-04-20T17:59:31Z","receivedAt":"2018-04-20T17:59:59Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 4/20/2018 1:02 PM, Elijah Newren wrote:\n> On Fri, Apr 20, 2018 at 6:36 AM, Ben Peart wrote:\n>> --- a/Documentation/merge-config.txt\n>> +++ b/Documentation/merge-config.txt\n>> @@ -37,6 +37,11 @@ merge.renameLimit::\n>> during a merge; if not specified, defaults to the value of\n>> diff.renameLimit.\n>>\n>> +merge.renames::\n>> + Whether and how Git detects renames. If set to \"false\",\n>> + rename detection is disabled. If set to \"true\", basic rename\n>> + detection is enabled. This is the default.\n> \n> One can already control o->detect_rename via the -Xno-renames and\n> -Xfind-renames options. \n\nYes, but that requires people to know they need to do that and then \nremember to pass it on the command line every time. We've found that \ndoesn't typically happen, we just get someone complaining about slow \nmerges. :)\n\nThat is why we added them as config options which change the default. \nThat way we can then set them on the repo and the default behavior gives \nthem better performance. They can still always override the config \nsetting with the command line options.\n\nI think the documentation should mention that\n> \"false\" is the same as passing -Xno-renames, and \"true\" is the same as\n> passing -Xfind-renames. However, find-renames does take similarity\n> threshold as a parameter, so there's a question whether this option\n> should provide some way to do the same. I'm not sure the answer to\n> that; it may be that we'd want a separate config option for that, and\n> we can wait to add it until someone actually wants it.\n\nI'm of the opinion that we shouldn't bother adding features that we \naren't sure someone will want/use. If it comes up, we can certainly add \nit at a later date.\n\n> \n>> merge.renormalize::\n>> Tell Git that canonical representation of files in the\n>> repository has changed over time (e.g. earlier commits record\n>> diff --git a/merge-recursive.c b/merge-recursive.c\n>> index 9c05eb7f70..cd5367e890 100644\n>> --- a/merge-recursive.c\n>> +++ b/merge-recursive.c\n>> @@ -3256,6 +3256,7 @@ static void merge_recursive_config(struct merge_options *o)\n>> git_config_get_int(\"merge.verbosity\", &o->verbosity);\n>> git_config_get_int(\"diff.renamelimit\", &o->diff_rename_limit);\n>> git_config_get_int(\"merge.renamelimit\", &o->merge_rename_limit);\n>> + git_config_get_bool(\"merge.renames\", &o->detect_rename);\n>> git_config(git_xmerge_config, NULL);\n>> }\n> \n> I would expect an explicitly passed -Xno-renames or -Xfind-renames to\n> override the config setting. Could you check if that's the case?\n> \n\nYes, command line options override the config settings. You can see \nthat in the code where the call to init_merge_options() which loads the \nconfig settings is followed by parse_merge_opt() which loads the command \nline options. I've also verified the behavior in the debugger (it's on \nby default in the code, the config setting turns it off, then the \ncommand line option turns it back on).\n\n> Also, if someone sets merge.renameLimit (to anything) and sets\n> merge.renames to false, then they've got a contradictory setup. Does\n> it make sense to check and warn about that anywhere?\n> \n\nI don't think we need to. The merge.renameLimit is only used if \ndetect_rename it turned on no matter how that gets turned on (default, \nconfig setting, command line option) so there isn't really a change in \nbehavior here.\n"},{"id":"345245","messageId":"e580712c-b375-c07e-a02e-5bb63a914611@gmail.com","threadId":"48335","inReplyTo":"CABPp-BEwwn+NwOEtWOKOdUKxoXfq6YwWeoH6OwkPjSwVtTm5=Q@mail.gmail.com","subject":"Re: [PATCH v1 0/2] add additional config settings for merge","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-04-20T18:19:11Z","receivedAt":"2018-04-20T18:19:16Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 4/20/2018 1:34 PM, Elijah Newren wrote:\n> On Fri, Apr 20, 2018 at 6:36 AM, Ben Peart wrote:\n>> This enables the user to set a couple of additional options for merge.\n>>\n>> 1. merge.aggressive - this is to try to resolve a few more trivial\n>> merge cases. It is documented in read-tree and is not something you\n>> can pass into merge itself.\n>>\n>> 2. merge.renames - this is to save git from having to go through the entire\n>> 3 trees to see if there were any renames that happened.\n>>\n>> For the work item repro that I have been using this drops the merge time\n>> from ~1 hour to ~5 minutes and the unmerged entries goes down from\n>> ~40,000 to 1.\n> \n> Ooh, this is *very* interesting. Is there any chance I could also get\n> you to test performing the same merge with the version of git at\n> https://github.com/newren/git/tree/big-repo-small-cherry-pick and\n> report on your timings?\n> \n\nUnfortunately, it isn't quite that simple. My repo is _really_ big \n(3.2M files and ~100K commits per week) and requires me to use a custom \nfork of git that works with our GVFS solution for it to work at all.\n\nI've been watching your work in this area and am hoping it pays off for \nus if/when we have users that want to do rename detection and override \nour defaults.\n\n> The 'big-repo-small-cherry-pick' name could be improved, but that\n> branch has a number of performance fixes for really poor rename\n> detection performance during merges. From your description, I'm\n> pretty sure it'll apply to your case. For my specific testcase, I\n> got a speedup factor of 30. Someone else on the list saw a factor of\n> 24[1]. Results are highly dependent on the specific repo, but it's\n> certainly possible that it gets much of your factor of 12 speedup that\n> you saw with these new config settings you added.\n> \n> However, what makes this case even more interesting to me is that my\n> branch may not be quite as effective as your workarounds. There are\n> other other performance issues in merge that I am aware of, but for\n> which I haven't had the time to write the patches yet (I've been\n> waiting for the directory rename detection stuff to land and settle\n> down before working more on the performance aspects). I do not know\n> how big a factor those other performance issues are, but your\n> workarounds (namely the aggressive setting) may get around some of\n> those other issues as well, so I'm very interested to see how my\n> current branch compares to the speedups you got with these settings.\n> \n> Thanks,\n> Elijah\n> \n> \n> [1] https://public-inbox.org/git/alpine.DEB.2.00.1711211303290.20686@ds9.cixit.se/\n> \n"},{"id":"345248","messageId":"CABPp-BFqj2TFiHUDsysafq0NHC4MV-QYZVxOZe1TNRrXMOQfng@mail.gmail.com","threadId":"48335","inReplyTo":"cd49481c-9665-124a-5f94-791f1a16657d@gmail.com","subject":"Re: [PATCH v1 1/2] merge: Add merge.renames config setting","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-04-20T18:34:25Z","receivedAt":"2018-04-20T18:34:30Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi Ben,\n\nOn Fri, Apr 20, 2018 at 10:59 AM, Ben Peart wrote:\n>\n> On 4/20/2018 1:02 PM, Elijah Newren wrote:\n>>\n>> On Fri, Apr 20, 2018 at 6:36 AM, Ben Peart \n>> wrote:\n>>>\n>>> --- a/Documentation/merge-config.txt\n>>> +++ b/Documentation/merge-config.txt\n>>> @@ -37,6 +37,11 @@ merge.renameLimit::\n>>> during a merge; if not specified, defaults to the value of\n>>> diff.renameLimit.\n>>>\n>>> +merge.renames::\n>>> + Whether and how Git detects renames. If set to \"false\",\n>>> + rename detection is disabled. If set to \"true\", basic rename\n>>> + detection is enabled. This is the default.\n>>\n>>\n>> One can already control o->detect_rename via the -Xno-renames and\n>> -Xfind-renames options.\n\nThis statement wasn't meant to be independent of the sentence that\nfollowed it...\n\n> Yes, but that requires people to know they need to do that and then remember\n> to pass it on the command line every time. We've found that doesn't\n> typically happen, we just get someone complaining about slow merges. :)\n>\n> That is why we added them as config options which change the default. That\n> way we can then set them on the repo and the default behavior gives them\n> better performance. They can still always override the config setting with\n> the command line options.\n\nSorry, I think I wasn't being clear. The documentation for the config\noptions for e.g. diff.renameLimit, fetch.prune, log.abbrevCommit, and\nmerge.ff all mention the equivalent command line parameters. Your\npatch doesn't do that for merge.renames, but I think it would be\nhelpful if it did.\n\nAlso, a link in the documentation the other way, from\nDocumentation/merge-strategies.txt under the entries for -Xno-renames\nand -Xfind-renames should probably mention this new merge.renames\nconfig setting (much like the -Xno-renormalize flag mentions the\nmerge.renomralize config option).\n\n(In general, I think having this as a configuration option makes\nsense, though I hope my other performance patches would be enough to\nmake people consider switching back to the defaults and use rename\ndetection again.)\n\n\n> I'm of the opinion that we shouldn't bother adding features that we aren't\n> sure someone will want/use. If it comes up, we can certainly add it at a\n> later date.\n\nWorks for me; I was mostly throwing it out there for thought.\n\n> Yes, command line options override the config settings.\n\nGood. :-)\n\n>> Also, if someone sets merge.renameLimit (to anything) and sets\n>> merge.renames to false, then they've got a contradictory setup. Does\n>> it make sense to check and warn about that anywhere?\n>\n> I don't think we need to. The merge.renameLimit is only used if\n> detect_rename it turned on no matter how that gets turned on (default,\n> config setting, command line option) so there isn't really a change in\n> behavior here.\n\nI agree that's the pre-existing behavior, but prior to this patch\nturning off rename detection could only be done manually with every\ninvocation. I'm slightly concerned that users might be confused if\nmerge.renames was set to false somewhere -- perhaps even in a global\n/etc/gitconfig that they had no knowledge of or control over -- and in\nan attempt to get rename detection to work they started passing larger\nand larger values for renameLimit all to no avail.\n\nThe easy fix here may just be documenting the diff.renameLimit and\nmerge.renameLimit options that they have no effect if rename detection\nis turned off.\n\nOr maybe I'm just worrying too much, but we (folks at $dayjob) were\nbit pretty hard by renameLimit silently being capped at a value less\nthan the user specified and in a way that wasn't documented anywhere.\n"},{"id":"345307","messageId":"xmqqwox19ohw.fsf@gitster-ct.c.googlers.com","threadId":"48335","inReplyTo":"CABPp-BFqj2TFiHUDsysafq0NHC4MV-QYZVxOZe1TNRrXMOQfng@mail.gmail.com","subject":"Re: [PATCH v1 1/2] merge: Add merge.renames config setting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-04-21T04:23:23Z","receivedAt":"2018-04-21T04:23:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n>>>> +merge.renames::\n>>>> + Whether and how Git detects renames. If set to \"false\",\n>>>> + rename detection is disabled. If set to \"true\", basic rename\n>>>> + detection is enabled. This is the default.\n>>>\n>>>\n>>> One can already control o->detect_rename via the -Xno-renames and\n>>> -Xfind-renames options.\n> ...\n> Sorry, I think I wasn't being clear. The documentation for the config\n> options for e.g. diff.renameLimit, fetch.prune, log.abbrevCommit, and\n> merge.ff all mention the equivalent command line parameters. Your\n> patch doesn't do that for merge.renames, but I think it would be\n> helpful if it did.\n\nYes, and if we are adding a new configuration, we should do so in\nsuch a way that we do not have to come back and extend it when we\nknow what the command line option does and the configuration being\nproposed is less capable already. I wonder if we can just add a\nsingle configuration whose value can be \"never\" to pretend as if\n\"--Xno-renames\" were given, and some similarity score like \"50\" to\npretend as if \"--Xfind-renames=50\" were given. \n\nThat is, merge.renames does not have to a simple \"yes-no to control\nthe --Xno-renames option\". And it would of course be better to\ndocument it.\n\nI also had to wonder how \"merge -s resolve\" faired, if the project\nis not interested in renamed paths at all.\n\nThanks.\n"},{"id":"345387","messageId":"20180422120718.GA29956@esm","threadId":"48335","inReplyTo":"CABPp-BFqj2TFiHUDsysafq0NHC4MV-QYZVxOZe1TNRrXMOQfng@mail.gmail.com","subject":"Re: [PATCH v1 1/2] merge: Add merge.renames config setting","fromName":"Eckhard Maaß","fromEmail":"eckhard.s.maass@googlemail.com","sentAt":"2018-04-22T12:07:18Z","receivedAt":"2018-04-22T12:07:24Z","isPatch":true,"sender":{"key":"eckhard.s.maass@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/21984134?v=4"},"body":"On Fri, Apr 20, 2018 at 11:34:25AM -0700, Elijah Newren wrote:\n> Sorry, I think I wasn't being clear. The documentation for the config\n> options for e.g. diff.renameLimit, fetch.prune, log.abbrevCommit, and\n> merge.ff all mention the equivalent command line parameters. Your\n> patch doesn't do that for merge.renames, but I think it would be\n> helpful if it did.\n\nI wonder here what the relation to the diff.* options should be in this\nregard anyway. There is already diff.renames. Naively, I would assume\nthat these options are in sync, that is you control the behavior of both\nthe normal diff family like git show and git merge. The reasoning, at\nleast for me, is to keep consistency between the outcome of rename\ndetection while merging and a later simple \"git show MERGE_BASE..HEAD\".\nI would expect those to give me the same style of rename detection.\n\nHence, I would like to use diff.renames and maybe enhance this option to\nalso carry the score in backward compatible way (or introduce a second\nconfiguration option?). Is this idea going in a good direction? If yes,\nI will try to submit a patch for this.\n\nAh, by the way: for people that have not touched diff.renames there will\nbe no visible change in how Git behaves - the default for diff.renames\nis a rename with 50% score with is the same for merge. So it will only\nchange if one has tweaked diff.renames already. But I wonder if one does\nthat and expect the merge to use a different rename detection anyway.\n\nGreetings,\nEckhard\n"},{"id":"345457","messageId":"82d8c76d-b8fa-4b72-5ebd-25b650f89980@gmail.com","threadId":"48335","inReplyTo":"CABPp-BEhvLVTL3+0scUucAp9ZMBiiT_0VG3eeKm9qRnHG=y+tw@mail.gmail.com","subject":"Re: [PATCH v1 1/2] merge: Add merge.renames config setting","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-04-23T12:57:24Z","receivedAt":"2018-04-23T12:57:34Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 4/20/2018 1:26 PM, Elijah Newren wrote:\n> On Fri, Apr 20, 2018 at 10:02 AM, Elijah Newren wrote:\n>> On Fri, Apr 20, 2018 at 6:36 AM, Ben Peart wrote:\n>>> --- a/Documentation/merge-config.txt\n>>> +++ b/Documentation/merge-config.txt\n>>> @@ -37,6 +37,11 @@ merge.renameLimit::\n>>> during a merge; if not specified, defaults to the value of\n>>> diff.renameLimit.\n>>>\n>>> +merge.renames::\n>>> + Whether and how Git detects renames. If set to \"false\",\n>>> + rename detection is disabled. If set to \"true\", basic rename\n>>> + detection is enabled. This is the default.\n>>\n>> One can already control o->detect_rename via the -Xno-renames and\n>> -Xfind-renames options. I think the documentation should mention that\n>> \"false\" is the same as passing -Xno-renames, and \"true\" is the same as\n>> passing -Xfind-renames. However, find-renames does take similarity\n>> threshold as a parameter, so there's a question whether this option\n>> should provide some way to do the same. I'm not sure the answer to\n>> that; it may be that we'd want a separate config option for that, and\n>> we can wait to add it until someone actually wants it.\n> \n> I just realized another issue, though it also affects -Xno-renames.\n> Even if rename detection is turned off for the merge, it is\n> unconditionally turned on for the diffstat. In builtin/merge.c,\n> function finish(), there is the code:\n> \n> if (new_head && show_diffstat) {\n> ...\n> opts.detect_rename = DIFF_DETECT_RENAME;\n> \n> It seems that this option should affect that line as well. (Do you\n> have diffstat turned off by chance? If not, you may be able to\n> improve your performance even more...)\n> \n\nSeems reasonable to me. I'll update the patch to do that.\n"},{"id":"345458","messageId":"0eea1726-d511-6818-aa29-add6c13900da@gmail.com","threadId":"48335","inReplyTo":"20180422120718.GA29956@esm","subject":"Re: [PATCH v1 1/2] merge: Add merge.renames config setting","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-04-23T13:15:09Z","receivedAt":"2018-04-23T13:15:14Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 4/22/2018 8:07 AM, Eckhard Maaß wrote:\n> On Fri, Apr 20, 2018 at 11:34:25AM -0700, Elijah Newren wrote:\n>> Sorry, I think I wasn't being clear. The documentation for the config\n>> options for e.g. diff.renameLimit, fetch.prune, log.abbrevCommit, and\n>> merge.ff all mention the equivalent command line parameters. Your\n>> patch doesn't do that for merge.renames, but I think it would be\n>> helpful if it did.\n> \n> I wonder here what the relation to the diff.* options should be in this\n> regard anyway. There is already diff.renames. Naively, I would assume\n> that these options are in sync, that is you control the behavior of both\n> the normal diff family like git show and git merge. The reasoning, at\n> least for me, is to keep consistency between the outcome of rename\n> detection while merging and a later simple \"git show MERGE_BASE..HEAD\".\n> I would expect those to give me the same style of rename detection.\n> \n> Hence, I would like to use diff.renames and maybe enhance this option to\n> also carry the score in backward compatible way (or introduce a second\n> configuration option?). Is this idea going in a good direction? If yes,\n> I will try to submit a patch for this.\n\nIt's a fair question. If you look at all the options in \nDocumentation/merge-config.txt, you will see many merge specific \nsettings. I think the ability to control these settings separately is \npretty well established.\n\nIn commit 2a2ac926547 when merge.renamelimit was added, it was decided \nto have separate settings for merge and diff to give users the ability \nto control that behavior. In this particular case, it will default to \nthe value of diff.renamelimit when it isn't set. That isn't consistent \nwith the other merge settings.\n\nChanging that behavior across the rest of the merge settings is outside \nthe scope of this patch. I don't have a strong opinion as to whether \nthat is a good or bad thing.\n\n> \n> Ah, by the way: for people that have not touched diff.renames there will\n> be no visible change in how Git behaves - the default for diff.renames\n> is a rename with 50% score with is the same for merge. So it will only\n> change if one has tweaked diff.renames already. But I wonder if one does\n> that and expect the merge to use a different rename detection anyway.\n> \n> Greetings,\n> Eckhard\n> \n"},{"id":"345459","messageId":"1e57021a-2a99-c3a7-f203-e3c9b0546876@gmail.com","threadId":"48335","inReplyTo":"CABPp-BFqj2TFiHUDsysafq0NHC4MV-QYZVxOZe1TNRrXMOQfng@mail.gmail.com","subject":"Re: [PATCH v1 1/2] merge: Add merge.renames config setting","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-04-23T13:22:42Z","receivedAt":"2018-04-23T13:22:46Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 4/20/2018 2:34 PM, Elijah Newren wrote:\n> Hi Ben,\n> \n> On Fri, Apr 20, 2018 at 10:59 AM, Ben Peart wrote:\n>>\n>> On 4/20/2018 1:02 PM, Elijah Newren wrote:\n>>>\n>>> On Fri, Apr 20, 2018 at 6:36 AM, Ben Peart \n>>> wrote:\n>>>>\n>>>> --- a/Documentation/merge-config.txt\n>>>> +++ b/Documentation/merge-config.txt\n>>>> @@ -37,6 +37,11 @@ merge.renameLimit::\n>>>> during a merge; if not specified, defaults to the value of\n>>>> diff.renameLimit.\n>>>>\n>>>> +merge.renames::\n>>>> + Whether and how Git detects renames. If set to \"false\",\n>>>> + rename detection is disabled. If set to \"true\", basic rename\n>>>> + detection is enabled. This is the default.\n>>>\n>>>\n>>> One can already control o->detect_rename via the -Xno-renames and\n>>> -Xfind-renames options.\n> \n> This statement wasn't meant to be independent of the sentence that\n> followed it...\n> \n>> Yes, but that requires people to know they need to do that and then remember\n>> to pass it on the command line every time. We've found that doesn't\n>> typically happen, we just get someone complaining about slow merges. :)\n>>\n>> That is why we added them as config options which change the default. That\n>> way we can then set them on the repo and the default behavior gives them\n>> better performance. They can still always override the config setting with\n>> the command line options.\n> \n> Sorry, I think I wasn't being clear. The documentation for the config\n> options for e.g. diff.renameLimit, fetch.prune, log.abbrevCommit, and\n> merge.ff all mention the equivalent command line parameters. Your\n> patch doesn't do that for merge.renames, but I think it would be\n> helpful if it did.\n> \n> Also, a link in the documentation the other way, from\n> Documentation/merge-strategies.txt under the entries for -Xno-renames\n> and -Xfind-renames should probably mention this new merge.renames\n> config setting (much like the -Xno-renormalize flag mentions the\n> merge.renomralize config option).\n> \n\nI'm all in favor of having more information in the documentation. I'm \nof the opinion that if someone has made the effort to actually _read_ \nthe documentation, we should be as descriptive and complete as possible.\n\nI'll take a cut at adding the things you have pointed out would be helpful.\n\n> I agree that's the pre-existing behavior, but prior to this patch\n> turning off rename detection could only be done manually with every\n> invocation. I'm slightly concerned that users might be confused if\n> merge.renames was set to false somewhere -- perhaps even in a global\n> /etc/gitconfig that they had no knowledge of or control over -- and in\n> an attempt to get rename detection to work they started passing larger\n> and larger values for renameLimit all to no avail.\n> \n> The easy fix here may just be documenting the diff.renameLimit and\n> merge.renameLimit options that they have no effect if rename detection\n> is turned off.\n\nI can add this additional documentation as well. While some might think \nit is stating the obvious, I'm sure someone will benefit from it being \nexplicitly called out.\n\n> \n> Or maybe I'm just worrying too much, but we (folks at $dayjob) were\n> bit pretty hard by renameLimit silently being capped at a value less\n> than the user specified and in a way that wasn't documented anywhere.\n> \n"},{"id":"345474","messageId":"1fb11850-4c20-5327-a63a-6d1f5aa18ea4@gmail.com","threadId":"48335","inReplyTo":"xmqqwox19ohw.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v1 1/2] merge: Add merge.renames config setting","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-04-23T16:00:49Z","receivedAt":"2018-04-23T16:00:54Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 4/21/2018 12:23 AM, Junio C Hamano wrote:\n> Elijah Newren writes:\n> \n>>>>> +merge.renames::\n>>>>> + Whether and how Git detects renames. If set to \"false\",\n>>>>> + rename detection is disabled. If set to \"true\", basic rename\n>>>>> + detection is enabled. This is the default.\n>>>>\n>>>>\n>>>> One can already control o->detect_rename via the -Xno-renames and\n>>>> -Xfind-renames options.\n>> ...\n>> Sorry, I think I wasn't being clear. The documentation for the config\n>> options for e.g. diff.renameLimit, fetch.prune, log.abbrevCommit, and\n>> merge.ff all mention the equivalent command line parameters. Your\n>> patch doesn't do that for merge.renames, but I think it would be\n>> helpful if it did.\n> \n> Yes, and if we are adding a new configuration, we should do so in\n> such a way that we do not have to come back and extend it when we\n> know what the command line option does and the configuration being\n> proposed is less capable already.\n\nBetween all the different command line options, config settings, merge \nstrategies and the interactions between the diff and merge versions, I\nwas trying to keep things as simple and consistent as possible. To that \nend 'merge.renames' was modeled after the existing 'diff.renames.'\n\nI wonder if we can just add a\n> single configuration whose value can be \"never\" to pretend as if\n> \"--Xno-renames\" were given, and some similarity score like \"50\" to\n> pretend as if \"--Xfind-renames=50\" were given.\n> \n> That is, merge.renames does not have to a simple \"yes-no to control\n> the --Xno-renames option\". And it would of course be better to\n> document it.\n> \n\nWith the existing differences in how these options are passed on the \ncommand line, I'm hesitant to add yet another pattern in the config \nsettings that combines 'renames' and '--find-renames[=]'.\n\nI _have_ wondered why this all isn't configured via find-renames with \nfind-renames=0 meaning renames=false (instead of mapping 0 to 32K). I \nthink that could have eliminated the need for splitting rename across \ntwo different settings (which is what I think you are proposing above). \nI'd then want the config setting and command line option to be the same \nsyntax and behavior.\n\nMoving the existing settings to this model and updating the config and \ncommand line options to be consistent without breaking backwards \ncompatibility is outside the intended scope of this patch.\n\n> I also had to wonder how \"merge -s resolve\" faired, if the project\n> is not interested in renamed paths at all.\n> \n\nTo be clear, it isn't that we're not interested in detecting renamed \nfiles and paths. We're just opposed to it taking an hour to figure that \nout!\n\n> Thanks.\n> \n"},{"id":"345513","messageId":"20180423213228.GA20391@esm","threadId":"48335","inReplyTo":"0eea1726-d511-6818-aa29-add6c13900da@gmail.com","subject":"Re: [PATCH v1 1/2] merge: Add merge.renames config setting","fromName":"Eckhard Maaß","fromEmail":"eckhard.s.maass@googlemail.com","sentAt":"2018-04-23T21:32:28Z","receivedAt":"2018-04-23T21:32:35Z","isPatch":true,"sender":{"key":"eckhard.s.maass@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/21984134?v=4"},"body":"On Mon, Apr 23, 2018 at 09:15:09AM -0400, Ben Peart wrote:\n> In commit 2a2ac926547 when merge.renamelimit was added, it was decided to\n> have separate settings for merge and diff to give users the ability to\n> control that behavior. In this particular case, it will default to the\n> value of diff.renamelimit when it isn't set. That isn't consistent with the\n> other merge settings.\n\nHowever, it seems like a desirable way to do it.\n\nMaybe let me throw in some code for discussion (test and documentation\nis missing, mainly to form an idea what the change in options should\nbe). I admit the patch below is concerned only with diff.renames, but\nwhatever we come up with for merge should be reflected there, too,\ndoesn't it?\n\nGreetings,\nEckhard\n\n-- >8 --\n\nFrom e8a88111f2aaf338a4c19e83251c7178f7152129 Mon Sep 17 00:00:00 2001\nFrom: =?UTF-8?q?Eckhard=20S=2E=20Maa=C3=9F?= \nDate: Sun, 22 Apr 2018 23:29:08 +0200\nSubject: [PATCH] diff: enhance diff.renames to be able to set rename score\nMIME-Version: 1.0\nContent-Type: text/plain; charset=UTF-8\nContent-Transfer-Encoding: 8bit\n\nSigned-off-by: Eckhard S. Maaß \n---\n diff.c | 35 ++++++++++++++++++++++++++++-------\n 1 file changed, 28 insertions(+), 7 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 1289df4b1f..a3cedad5cf 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -30,6 +30,7 @@\n #endif\n \n static int diff_detect_rename_default;\n+static int diff_rename_score_default;\n static int diff_indent_heuristic = 1;\n static int diff_rename_limit_default = 400;\n static int diff_suppress_blank_empty;\n@@ -177,13 +178,33 @@ static int parse_submodule_params(struct diff_options *options, const char *valu\n \treturn 0;\n }\n \n+int parse_rename_score(const char **cp_p);\n+\n+static int git_config_rename_score(const char *value)\n+{\n+\tint parsed_rename_score = parse_rename_score(&value);\n+\tif (parsed_rename_score == -1)\n+\t\treturn error(\"invalid argument to diff.renamescore: %s\", value);\n+\tdiff_rename_score_default = parsed_rename_score;\n+\treturn 0;\n+}\n+\n static int git_config_rename(const char *var, const char *value)\n {\n-\tif (!value)\n-\t\treturn DIFF_DETECT_RENAME;\n-\tif (!strcasecmp(value, \"copies\") || !strcasecmp(value, \"copy\"))\n-\t\treturn DIFF_DETECT_COPY;\n-\treturn git_config_bool(var,value) ? DIFF_DETECT_RENAME : 0;\n+\tif (!value) {\n+\t\tdiff_detect_rename_default = DIFF_DETECT_RENAME;\n+\t\treturn 0;\n+\t}\n+\tif (skip_to_optional_arg(value, \"copies\", &value) || skip_to_optional_arg(value, \"copy\", &value)) {\n+\t\tdiff_detect_rename_default = DIFF_DETECT_COPY;\n+\t\treturn git_config_rename_score(value);\n+\t}\n+\tif (skip_to_optional_arg(value, \"renames\", &value) || skip_to_optional_arg(value, \"rename\", &value)) {\n+\t\tdiff_detect_rename_default = DIFF_DETECT_RENAME;\n+\t\treturn git_config_rename_score(value);\n+\t}\n+\tdiff_detect_rename_default = git_config_bool(var,value) ? DIFF_DETECT_RENAME : 0;\n+\treturn 0;\n }\n \n long parse_algorithm_value(const char *value)\n@@ -307,8 +328,7 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \tif (!strcmp(var, \"diff.renames\")) {\n-\t\tdiff_detect_rename_default = git_config_rename(var, value);\n-\t\treturn 0;\n+\t\treturn git_config_rename(var, value);\n \t}\n \tif (!strcmp(var, \"diff.autorefreshindex\")) {\n \t\tdiff_auto_refresh_index = git_config_bool(var, value);\n@@ -4116,6 +4136,7 @@ void diff_setup(struct diff_options *options)\n \toptions->add_remove = diff_addremove;\n \toptions->use_color = diff_use_color_default;\n \toptions->detect_rename = diff_detect_rename_default;\n+\toptions->rename_score = diff_rename_score_default;\n \toptions->xdl_opts |= diff_algorithm;\n \tif (diff_indent_heuristic)\n \t\tDIFF_XDL_SET(options, INDENT_HEURISTIC);\n-- \n2.17.0.252.gfe0a9eaf31\n\n"},{"id":"345521","messageId":"xmqqy3hd8q2k.fsf@gitster-ct.c.googlers.com","threadId":"48335","inReplyTo":"1fb11850-4c20-5327-a63a-6d1f5aa18ea4@gmail.com","subject":"Re: [PATCH v1 1/2] merge: Add merge.renames config setting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-04-23T23:23:47Z","receivedAt":"2018-04-23T23:23:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ben Peart writes:\n\n>> I also had to wonder how \"merge -s resolve\" faired, if the project\n>> is not interested in renamed paths at all.\n>>\n>\n> To be clear, it isn't that we're not interested in detecting renamed\n> files and paths. We're just opposed to it taking an hour to figure\n> that out!\n\nYeah, but as opposed to passing \"oh, let's see if we can get a\nreasonable result without rename detection just this time\" from the\ncommand line, configuring merge.renames=false in would mean exactly\nthat: \"we don't need rename detection, just want to skip the cycles\nspent for it\". That is why I wondered how well the resolve strategy\nwould have fit your needs.\n\n\n"},{"id":"345642","messageId":"nycvar.QRO.7.76.6.1804241354520.64@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz","threadId":"48335","inReplyTo":"xmqqy3hd8q2k.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v1 1/2] merge: Add merge.renames config setting","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-04-24T11:58:46Z","receivedAt":"2018-04-24T11:59:28Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 24 Apr 2018, Junio C Hamano wrote:\n\n> Ben Peart writes:\n> \n> >> I also had to wonder how \"merge -s resolve\" faired, if the project\n> >> is not interested in renamed paths at all.\n> >>\n> >\n> > To be clear, it isn't that we're not interested in detecting renamed\n> > files and paths. We're just opposed to it taking an hour to figure\n> > that out!\n> \n> Yeah, but as opposed to passing \"oh, let's see if we can get a\n> reasonable result without rename detection just this time\" from the\n> command line, configuring merge.renames=false in would mean exactly\n> that: \"we don't need rename detection, just want to skip the cycles\n> spent for it\". That is why I wondered how well the resolve strategy\n> would have fit your needs.\n\nPlease do not forget that the context is GVFS, where you would cause a lot\nof pain and suffering by letting users forget to specify that command-line\noption all the time, resulting in several gigabytes of objects having to\nbe downloaded just for the sake of rename detection.\n\nSo there is a pretty good point in doing this as a config option.\n\nCiao,\nDscho\n"},{"id":"345649","messageId":"a34144ff-b91e-6f00-93e8-b472ad5887d0@gmail.com","threadId":"48335","inReplyTo":"CABPp-BFXwbZfFe0bZYMwWxz_Qxw=KQ6XE5SEBmgiE+TzaSycuQ@mail.gmail.com","subject":"Re: [PATCH v1 2/2] merge: Add merge.aggressive config setting","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-04-24T16:45:00Z","receivedAt":"2018-04-24T16:45:04Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 4/20/2018 1:22 PM, Elijah Newren wrote:\n> On Fri, Apr 20, 2018 at 6:36 AM, Ben Peart wrote:\n>> Add the ability to control the aggressive flag passed to read-tree via a config setting.\n> \n> This feels like a workaround to the performance problems with index\n> updates in merge-recursive.c. \n\nThis change wasn't done to solve performance problems. We turned it on \nbecause it reduced the number of unmerged entries (from 40K to 1) in the \nparticular merge we were looking at. The additional 3 scenarios that \n--aggressive resolves made that much difference.\n\nThat said, it makes sense to me to do\n> this when rename detection is turned off. In fact, I think you'd\n> automatically want to set aggressive to true whenever rename detection\n> is turned off (whether by your merge.renames option or the\n> -Xno-renames flag).\n> > I can't think of any reason this setting would be useful separate from\n> turning rename detection off, and it'd actively harm rename detection\n> performance improvements I have in the pipeline. I'd really prefer to\n> not add this option, and instead combine the setting of aggressive\n> with the other flag. Do you have an independent reason for wanting\n> this?\n> \n\nWhile combining them would work for our specific use scenario (since we \nturn both on already along with turning off merge.stat), I really \nhesitate to tie these two different flags and code paths together with a \nsingle config setting.\n\nWhile I don't want to needlessly complicate your optimizations in this \narea (they are already complex enough!) I believe we need to keep the \noption to turn on --aggressive without turning off rename detection as a \nviable option. Perhaps if that is the case, your optimizations have \nless impact or don't apply but the user should be able to make that \nchoice for their specific situation.\n\n> Thanks,\n> Elijah\n> \n"},{"id":"345650","messageId":"ccb85580-e0cc-f1e6-6667-ea89ac90106f@gmail.com","threadId":"48335","inReplyTo":"20180423213228.GA20391@esm","subject":"Re: [PATCH v1 1/2] merge: Add merge.renames config setting","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-04-24T16:53:47Z","receivedAt":"2018-04-24T16:53:52Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 4/23/2018 5:32 PM, Eckhard Maaß wrote:\n> On Mon, Apr 23, 2018 at 09:15:09AM -0400, Ben Peart wrote:\n>> In commit 2a2ac926547 when merge.renamelimit was added, it was decided to\n>> have separate settings for merge and diff to give users the ability to\n>> control that behavior. In this particular case, it will default to the\n>> value of diff.renamelimit when it isn't set. That isn't consistent with the\n>> other merge settings.\n> \n> However, it seems like a desirable way to do it.\n\nI'm just one opinion among many but I personally believe the cascading \nsettings are complicated enough just with the various config files and \ncommand line options and which overwrite the others. I'd rather not \ncomplicate them further by having settings inherited from one feature \n(diff) to another (merge).\n\nThere are currently ~15 merge specific config settings and only \nmerge.renamelimit currently does this inheritance. That said, at least \none other person thought it was a good idea. :)\n\n> \n> Maybe let me throw in some code for discussion (test and documentation\n> is missing, mainly to form an idea what the change in options should\n> be). I admit the patch below is concerned only with diff.renames, but\n> whatever we come up with for merge should be reflected there, too,\n> doesn't it >\n> Greetings,\n> Eckhard\n> \n> -- >8 --\n> \n> From e8a88111f2aaf338a4c19e83251c7178f7152129 Mon Sep 17 00:00:00 2001\n> From: =?UTF-8?q?Eckhard=20S=2E=20Maa=C3=9F?= \n> Date: Sun, 22 Apr 2018 23:29:08 +0200\n> Subject: [PATCH] diff: enhance diff.renames to be able to set rename score\n> MIME-Version: 1.0\n> Content-Type: text/plain; charset=UTF-8\n> Content-Transfer-Encoding: 8bit\n> \n> Signed-off-by: Eckhard S. Maaß \n> ---\n> diff.c | 35 ++++++++++++++++++++++++++++-------\n> 1 file changed, 28 insertions(+), 7 deletions(-)\n> \n> diff --git a/diff.c b/diff.c\n> index 1289df4b1f..a3cedad5cf 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -30,6 +30,7 @@\n> #endif\n> \n> static int diff_detect_rename_default;\n> +static int diff_rename_score_default;\n> static int diff_indent_heuristic = 1;\n> static int diff_rename_limit_default = 400;\n> static int diff_suppress_blank_empty;\n> @@ -177,13 +178,33 @@ static int parse_submodule_params(struct diff_options *options, const char *valu\n> \treturn 0;\n> }\n> \n> +int parse_rename_score(const char **cp_p);\n> +\n> +static int git_config_rename_score(const char *value)\n> +{\n> +\tint parsed_rename_score = parse_rename_score(&value);\n> +\tif (parsed_rename_score == -1)\n> +\t\treturn error(\"invalid argument to diff.renamescore: %s\", value);\n> +\tdiff_rename_score_default = parsed_rename_score;\n> +\treturn 0;\n> +}\n> +\n> static int git_config_rename(const char *var, const char *value)\n> {\n> -\tif (!value)\n> -\t\treturn DIFF_DETECT_RENAME;\n> -\tif (!strcasecmp(value, \"copies\") || !strcasecmp(value, \"copy\"))\n> -\t\treturn DIFF_DETECT_COPY;\n> -\treturn git_config_bool(var,value) ? DIFF_DETECT_RENAME : 0;\n> +\tif (!value) {\n> +\t\tdiff_detect_rename_default = DIFF_DETECT_RENAME;\n> +\t\treturn 0;\n> +\t}\n> +\tif (skip_to_optional_arg(value, \"copies\", &value) || skip_to_optional_arg(value, \"copy\", &value)) {\n> +\t\tdiff_detect_rename_default = DIFF_DETECT_COPY;\n> +\t\treturn git_config_rename_score(value);\n> +\t}\n> +\tif (skip_to_optional_arg(value, \"renames\", &value) || skip_to_optional_arg(value, \"rename\", &value)) {\n> +\t\tdiff_detect_rename_default = DIFF_DETECT_RENAME;\n> +\t\treturn git_config_rename_score(value);\n> +\t}\n> +\tdiff_detect_rename_default = git_config_bool(var,value) ? DIFF_DETECT_RENAME : 0;\n> +\treturn 0;\n> }\n> \n> long parse_algorithm_value(const char *value)\n> @@ -307,8 +328,7 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)\n> \t\treturn 0;\n> \t}\n> \tif (!strcmp(var, \"diff.renames\")) {\n> -\t\tdiff_detect_rename_default = git_config_rename(var, value);\n> -\t\treturn 0;\n> +\t\treturn git_config_rename(var, value);\n> \t}\n> \tif (!strcmp(var, \"diff.autorefreshindex\")) {\n> \t\tdiff_auto_refresh_index = git_config_bool(var, value);\n> @@ -4116,6 +4136,7 @@ void diff_setup(struct diff_options *options)\n> \toptions->add_remove = diff_addremove;\n> \toptions->use_color = diff_use_color_default;\n> \toptions->detect_rename = diff_detect_rename_default;\n> +\toptions->rename_score = diff_rename_score_default;\n> \toptions->xdl_opts |= diff_algorithm;\n> \tif (diff_indent_heuristic)\n> \t\tDIFF_XDL_SET(options, INDENT_HEURISTIC);\n> \n"},{"id":"345652","messageId":"20180424171124.12064-1-benpeart@microsoft.com","threadId":"48335","inReplyTo":"20180420133632.17580-1-benpeart@microsoft.com","subject":"[PATCH v2 0/2] add additional config settings for merge","fromName":"Ben Peart","fromEmail":"ben.peart@microsoft.com","sentAt":"2018-04-24T17:11:39Z","receivedAt":"2018-04-24T17:11:46Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"Updated in response to feedback. Mostly documentation changes but the diffstat\nat the end of the merge (if on) now honors the new merge.rename setting as well.\n\nBase Ref: master\nWeb-Diff: https://github.com/benpeart/git/commit/653bfe6e01\nCheckout: git fetch https://github.com/benpeart/git merge-options-v2 && git checkout 653bfe6e01\n\n\n### Interdiff (v1..v2):\n\ndiff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt\nindex 5ca942ab5e..77caa66c2f 100644\n--- a/Documentation/diff-config.txt\n+++ b/Documentation/diff-config.txt\n@@ -112,7 +112,8 @@ diff.orderFile::\n \n diff.renameLimit::\n \tThe number of files to consider when performing the copy/rename\n-\tdetection; equivalent to the 'git diff' option `-l`.\n+\tdetection; equivalent to the 'git diff' option `-l`. This setting\n+\thas no effect if rename detection is turned off.\n \n diff.renames::\n \tWhether and how Git detects renames. If set to \"false\",\ndiff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt\nindex 5a9ab969db..38492bcb98 100644\n--- a/Documentation/merge-config.txt\n+++ b/Documentation/merge-config.txt\n@@ -39,7 +39,8 @@ include::fmt-merge-msg-config.txt[]\n merge.renameLimit::\n \tThe number of files to consider when performing rename detection\n \tduring a merge; if not specified, defaults to the value of\n-\tdiff.renameLimit.\n+\tdiff.renameLimit. This setting has no effect if rename detection\n+\tis turned off.\n \n merge.renames::\n \tWhether and how Git detects renames. If set to \"false\",\ndiff --git a/Documentation/merge-strategies.txt b/Documentation/merge-strategies.txt\nindex 4a58aad4b8..1e0728aa12 100644\n--- a/Documentation/merge-strategies.txt\n+++ b/Documentation/merge-strategies.txt\n@@ -84,12 +84,14 @@ no-renormalize;;\n \t`merge.renormalize` configuration variable.\n \n no-renames;;\n-\tTurn off rename detection.\n+\tTurn off rename detection. This overrides the `merge.renames`\n+\tconfiguration variable.\n \tSee also linkgit:git-diff[1] `--no-renames`.\n \n find-renames[=];;\n \tTurn on rename detection, optionally setting the similarity\n-\tthreshold. This is the default.\n+\tthreshold. This is the default. This overrides the\n+\t'merge.renames' configuration variable.\n \tSee also linkgit:git-diff[1] `--find-renames`.\n \n rename-threshold=;;\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 8746c5e3e8..3be52cd316 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -424,6 +424,7 @@ static void finish(struct commit *head_commit,\n \t\topts.output_format |=\n \t\t\tDIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;\n \t\topts.detect_rename = DIFF_DETECT_RENAME;\n+\t\tgit_config_get_bool(\"merge.renames\", &opts.detect_rename);\n \t\tdiff_setup_done(&opts);\n \t\tdiff_tree_oid(head, new_head, \"\", &opts);\n \t\tdiffcore_std(&opts);\n\n\n### Patches\n\nBen Peart (2):\n merge: Add merge.renames config setting\n merge: Add merge.aggressive config setting\n\n Documentation/diff-config.txt | 3 ++-\n Documentation/merge-config.txt | 12 +++++++++++-\n Documentation/merge-strategies.txt | 6 ++++--\n builtin/merge.c | 1 +\n merge-recursive.c | 2 ++\n 5 files changed, 20 insertions(+), 4 deletions(-)\n\n\nbase-commit: 0b0cc9f86731f894cff8dd25299a9b38c254569e\n-- \n2.17.0.windows.1\n\n\n"},{"id":"345653","messageId":"20180424171124.12064-2-benpeart@microsoft.com","threadId":"48335","inReplyTo":"20180424171124.12064-1-benpeart@microsoft.com","subject":"[PATCH v2 1/2] merge: Add merge.renames config setting","fromName":"Ben Peart","fromEmail":"ben.peart@microsoft.com","sentAt":"2018-04-24T17:11:41Z","receivedAt":"2018-04-24T17:11:49Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"Add the ability to control rename detection for merge via a config setting.\n\nReviewed-by: Johannes Schindelin \nSigned-off-by: Ben Peart \n---\n Documentation/diff-config.txt | 3 ++-\n Documentation/merge-config.txt | 8 +++++++-\n Documentation/merge-strategies.txt | 6 ++++--\n builtin/merge.c | 1 +\n merge-recursive.c | 1 +\n 5 files changed, 15 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt\nindex 5ca942ab5e..77caa66c2f 100644\n--- a/Documentation/diff-config.txt\n+++ b/Documentation/diff-config.txt\n@@ -112,7 +112,8 @@ diff.orderFile::\n \n diff.renameLimit::\n \tThe number of files to consider when performing the copy/rename\n-\tdetection; equivalent to the 'git diff' option `-l`.\n+\tdetection; equivalent to the 'git diff' option `-l`. This setting\n+\thas no effect if rename detection is turned off.\n \n diff.renames::\n \tWhether and how Git detects renames. If set to \"false\",\ndiff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt\nindex 12b6bbf591..0540c44e23 100644\n--- a/Documentation/merge-config.txt\n+++ b/Documentation/merge-config.txt\n@@ -35,7 +35,13 @@ include::fmt-merge-msg-config.txt[]\n merge.renameLimit::\n \tThe number of files to consider when performing rename detection\n \tduring a merge; if not specified, defaults to the value of\n-\tdiff.renameLimit.\n+\tdiff.renameLimit. This setting has no effect if rename detection\n+\tis turned off.\n+\n+merge.renames::\n+\tWhether and how Git detects renames. If set to \"false\",\n+\trename detection is disabled. If set to \"true\", basic rename\n+\tdetection is enabled. This is the default.\n \n merge.renormalize::\n \tTell Git that canonical representation of files in the\ndiff --git a/Documentation/merge-strategies.txt b/Documentation/merge-strategies.txt\nindex 4a58aad4b8..1e0728aa12 100644\n--- a/Documentation/merge-strategies.txt\n+++ b/Documentation/merge-strategies.txt\n@@ -84,12 +84,14 @@ no-renormalize;;\n \t`merge.renormalize` configuration variable.\n \n no-renames;;\n-\tTurn off rename detection.\n+\tTurn off rename detection. This overrides the `merge.renames`\n+\tconfiguration variable.\n \tSee also linkgit:git-diff[1] `--no-renames`.\n \n find-renames[=];;\n \tTurn on rename detection, optionally setting the similarity\n-\tthreshold. This is the default.\n+\tthreshold. This is the default. This overrides the\n+\t'merge.renames' configuration variable.\n \tSee also linkgit:git-diff[1] `--find-renames`.\n \n rename-threshold=;;\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 8746c5e3e8..3be52cd316 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -424,6 +424,7 @@ static void finish(struct commit *head_commit,\n \t\topts.output_format |=\n \t\t\tDIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;\n \t\topts.detect_rename = DIFF_DETECT_RENAME;\n+\t\tgit_config_get_bool(\"merge.renames\", &opts.detect_rename);\n \t\tdiff_setup_done(&opts);\n \t\tdiff_tree_oid(head, new_head, \"\", &opts);\n \t\tdiffcore_std(&opts);\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 9c05eb7f70..cd5367e890 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -3256,6 +3256,7 @@ static void merge_recursive_config(struct merge_options *o)\n \tgit_config_get_int(\"merge.verbosity\", &o->verbosity);\n \tgit_config_get_int(\"diff.renamelimit\", &o->diff_rename_limit);\n \tgit_config_get_int(\"merge.renamelimit\", &o->merge_rename_limit);\n+\tgit_config_get_bool(\"merge.renames\", &o->detect_rename);\n \tgit_config(git_xmerge_config, NULL);\n }\n \n-- \n2.17.0.windows.1\n\n"},{"id":"345654","messageId":"20180424171124.12064-3-benpeart@microsoft.com","threadId":"48335","inReplyTo":"20180424171124.12064-1-benpeart@microsoft.com","subject":"[PATCH v2 2/2] merge: Add merge.aggressive config setting","fromName":"Ben Peart","fromEmail":"ben.peart@microsoft.com","sentAt":"2018-04-24T17:11:42Z","receivedAt":"2018-04-24T17:11:50Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"Add the ability to control the aggressive flag passed to read-tree via a config setting.\n\nReviewed-by: Johannes Schindelin \nSigned-off-by: Ben Peart \n---\n Documentation/merge-config.txt | 4 ++++\n merge-recursive.c | 1 +\n 2 files changed, 5 insertions(+)\n\ndiff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt\nindex 0540c44e23..38492bcb98 100644\n--- a/Documentation/merge-config.txt\n+++ b/Documentation/merge-config.txt\n@@ -1,3 +1,7 @@\n+merge.aggressive::\n+\tPasses \"aggressive\" to read-tree which makes the command resolve\n+\ta few more cases internally. See \"--aggressive\" in linkgit:git-read-tree[1].\n+\n merge.conflictStyle::\n \tSpecify the style in which conflicted hunks are written out to\n \tworking tree files upon merge. The default is \"merge\", which\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex cd5367e890..0ca84e4b82 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -355,6 +355,7 @@ static int git_merge_trees(struct merge_options *o,\n \to->unpack_opts.fn = threeway_merge;\n \to->unpack_opts.src_index = &the_index;\n \to->unpack_opts.dst_index = &the_index;\n+\tgit_config_get_bool(\"merge.aggressive\", (int *)&o->unpack_opts.aggressive);\n \tsetup_unpack_trees_porcelain(&o->unpack_opts, \"merge\");\n \n \tinit_tree_desc_from_tree(t+0, common);\n-- \n2.17.0.windows.1\n\n"},{"id":"345655","messageId":"CABPp-BHYrxyg1W0+144M=Bstunuw36ZCtRJhvu5G_kCm1g7e4w@mail.gmail.com","threadId":"48335","inReplyTo":"a34144ff-b91e-6f00-93e8-b472ad5887d0@gmail.com","subject":"Re: [PATCH v1 2/2] merge: Add merge.aggressive config setting","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-04-24T17:36:23Z","receivedAt":"2018-04-24T17:36:34Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi Ben,\n\nOn Tue, Apr 24, 2018 at 9:45 AM, Ben Peart wrote:\n> On 4/20/2018 1:22 PM, Elijah Newren wrote:\n>> On Fri, Apr 20, 2018 at 6:36 AM, Ben Peart \n>> wrote:\n>>>\n>>> Add the ability to control the aggressive flag passed to read-tree via a\n>>> config setting.\n>>\n>> This feels like a workaround to the performance problems with index\n>> updates in merge-recursive.c.\n>\n> This change wasn't done to solve performance problems. We turned it on\n> because it reduced the number of unmerged entries (from 40K to 1) in the\n> particular merge we were looking at. The additional 3 scenarios that\n> --aggressive resolves made that much difference.\n>\n> That said, it makes sense to me to do\n\nUm...color me perplexed here. aggressive exists just to do some\nresolutions that higher-level strategies can and totally ought to be\nable to handle easily (the rules are almost trivially\nstraight-forward), but deferring allows the higher level strategies\n(either merge-recursive or resolve's git-merge-one-file) to handle\nslightly differently (e.g. by detecting renames). merge-recursive\nshould be able to resolve anything that the unpack_trees aggressive\nsetting handles. If it can't, it sounds like there's a horrible bug\nsomewhere.\n\nPerhaps fixing that bug is the real problem?\n\nIs there any chance you can dig out more details about any of these\nconflicts or come up with a simple testcase where running 'git merge\n-X no-renames' gives a merge conflict but running with this option\nwould run to completion?\n\n>> this when rename detection is turned off. In fact, I think you'd\n>> automatically want to set aggressive to true whenever rename detection\n>> is turned off (whether by your merge.renames option or the\n>> -Xno-renames flag).\n>> > I can't think of any reason this setting would be useful separate from\n>> turning rename detection off, and it'd actively harm rename detection\n>> performance improvements I have in the pipeline. I'd really prefer to\n>> not add this option, and instead combine the setting of aggressive\n>> with the other flag. Do you have an independent reason for wanting\n>> this?\n>>\n>\n> While combining them would work for our specific use scenario (since we turn\n> both on already along with turning off merge.stat), I really hesitate to tie\n> these two different flags and code paths together with a single config\n> setting.\n>\n> While I don't want to needlessly complicate your optimizations in this area\n> (they are already complex enough!) I believe we need to keep the option to\n> turn on --aggressive without turning off rename detection as a viable\n> option. Perhaps if that is the case, your optimizations have less impact or\n> don't apply but the user should be able to make that choice for their\n> specific situation.\n\nI totally buy that you need at least one option to avoid waiting for\n(current) rename detection in some fashion, and that you don't want\nlots of spurious conflicts. But I don't understand why you believe\nthat we need to keep the option to turn on the aggressive flag\nindependently. What's the usecase? It wasn't possible before in the\ncode, no one else has asked for it, and even you say you don't need it\nas a separate option. Is it a concern that turning on aggressive\nwhenever rename-detection is turned off will break something? The\nonly reason I can see to keep the aggressive codepath in unpack_trees\nbehind a branch instead of it always running unconditionally for every\nsingle caller throughout the codebase is because of renames. So the\nfact that you're turning renames off, to me, suggests that aggressive\nflag should automatically be turned on. I'd even call pre-existing\ncode (e.g. the -X no-renames option in merge-recursive) that doesn't\nturn on the aggressive flag buggy (even if the only result is\nsuboptimal-performance).\n\nI don't see how an option to turn on the aggressive flag independently\nis possibly useful to anyone. Further, we have strong reason to\nbelieve it will soon be actively harmful. So...why? It's totally\npossible I'm just missing something. If there's a good reason for it,\nproviding some kind of benefit that the user could weigh in a\ntradeoff, then I can get on board with providing it as an option, but\nright now I just don't see it.\n"},{"id":"345656","messageId":"CABPp-BFwN7nkA1ss0Ca+TSgrqsJLrBscWCOn7Eu0ZS5TmnopvA@mail.gmail.com","threadId":"48335","inReplyTo":"nycvar.QRO.7.76.6.1804241354520.64@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz","subject":"Re: [PATCH v1 1/2] merge: Add merge.renames config setting","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-04-24T17:47:29Z","receivedAt":"2018-04-24T17:47:37Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi Dscho,\n\nOn Tue, Apr 24, 2018 at 4:58 AM, Johannes Schindelin\n wrote:\n> Hi Junio,\n>\n> On Tue, 24 Apr 2018, Junio C Hamano wrote:\n>\n>> Yeah, but as opposed to passing \"oh, let's see if we can get a\n>> reasonable result without rename detection just this time\" from the\n>> command line, configuring merge.renames=false in would mean exactly\n>> that: \"we don't need rename detection, just want to skip the cycles\n>> spent for it\". That is why I wondered how well the resolve strategy\n>> would have fit your needs.\n>\n> Please do not forget that the context is GVFS, where you would cause a lot\n> of pain and suffering by letting users forget to specify that command-line\n> option all the time, resulting in several gigabytes of objects having to\n> be downloaded just for the sake of rename detection.\n>\n> So there is a pretty good point in doing this as a config option.\n\nI agree you need a config option, but I think Junio has a good point\nthat it's worth at least checking out the possibility of a different\none. In particular, you could add a merge.defaultStrategy (or maybe\nmerge.twohead to be similar to pull.twohead??) that is set to\n'resolve', and use that to avoid rename detection.\n\nPerhaps performance considerations rule out the resolve strategy and\nfavor recursive, or maybe you need the 'recursive' part of the\nrecursive strategy (rather than the rename part), or perhaps there's\nsome other special reason you need to go this route, but since you are\navoiding renames right now it's at least worth considering the resolve\nstrategy.\n\nElijah\n"},{"id":"345658","messageId":"CABPp-BGDS4ocBbjqW4FqosPvOe11crzK2G2pZa+9Q3hgGXsPfQ@mail.gmail.com","threadId":"48335","inReplyTo":"20180424171124.12064-2-benpeart@microsoft.com","subject":"Re: [PATCH v2 1/2] merge: Add merge.renames config setting","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-04-24T18:11:41Z","receivedAt":"2018-04-24T18:11:45Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Apr 24, 2018 at 10:11 AM, Ben Peart wrote:\n> Add the ability to control rename detection for merge via a config setting.\n\nSweet, thanks for including the documentation updates.\n\nI lean towards the side of the argument that says that since\nmerge.renameLimit inherits from diff.renameLimit, merge.renames should\ninherit default value from diff.renames (allow people to not have to\nrepeat themselves as much if they want to use the same rename settings\nfor all cases). Sounds like you and Johannes disagree. I don't feel\nsuper strongly about this item, but it'd probably be good to get some\nother git folks' opinions on this particular point.\n\nOther than that unresolved question, and the separate one about\nwhether to go with a different option instead (e.g.\nmerge.defaultStrategy), as being discussed elsewhere in this thread,\nthe patch looks good to me.\n"},{"id":"345668","messageId":"CABPp-BFTywvVFV3Wx1jv9RyoFk_cE7XE8x1neuLVt4qwyw0EMw@mail.gmail.com","threadId":"48335","inReplyTo":"20180424171124.12064-2-benpeart@microsoft.com","subject":"Re: [PATCH v2 1/2] merge: Add merge.renames config setting","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-04-24T18:59:46Z","receivedAt":"2018-04-24T18:59:52Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Sorry, I noticed something else I missed on my last reading...\n\nOn Tue, Apr 24, 2018 at 10:11 AM, Ben Peart wrote:\n> diff --git a/builtin/merge.c b/builtin/merge.c\n> index 8746c5e3e8..3be52cd316 100644\n> --- a/builtin/merge.c\n> +++ b/builtin/merge.c\n> @@ -424,6 +424,7 @@ static void finish(struct commit *head_commit,\n> opts.output_format |=\n> DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;\n> opts.detect_rename = DIFF_DETECT_RENAME;\n> + git_config_get_bool(\"merge.renames\", &opts.detect_rename);\n> diff_setup_done(&opts);\n> diff_tree_oid(head, new_head, \"\", &opts);\n> diffcore_std(&opts);\n\nShouldn't this also be turned off if either (a) merge.renames is unset\nand diff.renames is false, or (b) the user specifies -Xno-renames?\n"},{"id":"345676","messageId":"68fa18c0-1dac-f6dc-0c41-fa5722c2c227@gmail.com","threadId":"48335","inReplyTo":"CABPp-BFTywvVFV3Wx1jv9RyoFk_cE7XE8x1neuLVt4qwyw0EMw@mail.gmail.com","subject":"Re: [PATCH v2 1/2] merge: Add merge.renames config setting","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-04-24T20:31:30Z","receivedAt":"2018-04-24T20:31:34Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 4/24/2018 2:59 PM, Elijah Newren wrote:\n> Sorry, I noticed something else I missed on my last reading...\n> \n> On Tue, Apr 24, 2018 at 10:11 AM, Ben Peart wrote:\n>> diff --git a/builtin/merge.c b/builtin/merge.c\n>> index 8746c5e3e8..3be52cd316 100644\n>> --- a/builtin/merge.c\n>> +++ b/builtin/merge.c\n>> @@ -424,6 +424,7 @@ static void finish(struct commit *head_commit,\n>> opts.output_format |=\n>> DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;\n>> opts.detect_rename = DIFF_DETECT_RENAME;\n>> + git_config_get_bool(\"merge.renames\", &opts.detect_rename);\n>> diff_setup_done(&opts);\n>> diff_tree_oid(head, new_head, \"\", &opts);\n>> diffcore_std(&opts);\n> \n> Shouldn't this also be turned off if either (a) merge.renames is unset\n> and diff.renames is false, or (b) the user specifies -Xno-renames?\n> \n\nThis makes me think that I should probably remove the line that \noverrides the detect_rename setting with the merge config setting. As I \nlook at the code, none of the other merge options are reflected in the \ndiffstat; instead, all the settings are pretty much hard coded. Perhaps \nI shouldn't rock that boat.\n"},{"id":"345717","messageId":"xmqqlgdc5fa2.fsf@gitster-ct.c.googlers.com","threadId":"48335","inReplyTo":"a34144ff-b91e-6f00-93e8-b472ad5887d0@gmail.com","subject":"Re: [PATCH v1 2/2] merge: Add merge.aggressive config setting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-04-24T23:57:25Z","receivedAt":"2018-04-24T23:57:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ben Peart writes:\n\n> That said, it makes sense to me to do\n>> this when rename detection is turned off. In fact, I think you'd\n>> automatically want to set aggressive to true whenever rename detection\n>> is turned off (whether by your merge.renames option or the\n>> -Xno-renames flag).\n>> ...\n>\n> While combining them would work for our specific use scenario (since\n> we turn both on already along with turning off merge.stat), I really\n> hesitate to tie these two different flags and code paths together with\n> a single config setting.\n\nThe cases that non-agressive variant leaves unmerged are not\nauto-resolved only because marking them as merged will rob the\nchance from the rename detection logic to notice which ones are\n\"new\" paths that could be matched with \"deleted\" ones to turn into\nrenames. If rename deteciton is not done, there is no reason to\nleave it non aggressive, as \"#1 = missing, #2 = something and #3 =\nmissing\" entry (just one example that is not auto-resolved by\nnon-agressive, but the principle is the same) left unmerged in the\nindex will get resolved to keep the current entry by the post\nprocessing logic anyway.\n\nIn fact, checking git-merge-resolve would tell us that we already\nuse \"aggresive\" variant there unconditionally.\n\nSo, I think Elijah is correct---there is no reason not to enable\nthis setting when the other one to refuse rename detection is in\neffect.\n"},{"id":"345720","messageId":"xmqqd0yo5ejb.fsf@gitster-ct.c.googlers.com","threadId":"48335","inReplyTo":"20180424171124.12064-1-benpeart@microsoft.com","subject":"Re: [PATCH v2 0/2] add additional config settings for merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-04-25T00:13:28Z","receivedAt":"2018-04-25T00:13:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ben Peart writes:\n\n> diff.renameLimit::\n> \tThe number of files to consider when performing the copy/rename\n> -\tdetection; equivalent to the 'git diff' option `-l`.\n> +\tdetection; equivalent to the 'git diff' option `-l`. This setting\n> +\thas no effect if rename detection is turned off.\n\nYou mean \"turned off via diff.renames\"?\n\nThis is not meant as a suggestion to rewrite this paragraph\nfurther---but if the answer is \"no\", then that might be an\nindication that the sentence is inviting a misunderstanding.\n\n> diff.renames::\n> \tWhether and how Git detects renames. If set to \"false\",\n> diff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt\n> index 5a9ab969db..38492bcb98 100644\n> --- a/Documentation/merge-config.txt\n> +++ b/Documentation/merge-config.txt\n> @@ -39,7 +39,8 @@ include::fmt-merge-msg-config.txt[]\n> merge.renameLimit::\n> \tThe number of files to consider when performing rename detection\n> \tduring a merge; if not specified, defaults to the value of\n> -\tdiff.renameLimit.\n> +\tdiff.renameLimit. This setting has no effect if rename detection\n> +\tis turned off.\n\nDitto. If your design is to make the merge machinery completely\nignore diff.renames and only pay attention to merge.renames [*1*],\nthen it probably is a good idea to be more specific here, by saying\n\"... is turned off via ...\", though.\n\n> merge.renames::\n> \tWhether and how Git detects renames. If set to \"false\",\n\n[Footnote]\n\n*1* ...which I do not think is such a good idea, by the way. I'd\npersonally expect merge.renames to allow overriding and falling back\nto diff.renames, just like the {merge,diff}.renameLimit pair does.\n"},{"id":"345742","messageId":"nycvar.QRO.7.76.6.1804251019070.4978@tvgsbejvaqbjf.bet","threadId":"48335","inReplyTo":"CABPp-BFwN7nkA1ss0Ca+TSgrqsJLrBscWCOn7Eu0ZS5TmnopvA@mail.gmail.com","subject":"Re: [PATCH v1 1/2] merge: Add merge.renames config setting","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-04-25T08:20:10Z","receivedAt":"2018-04-25T08:20:44Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Elijah,\n\nOn Tue, 24 Apr 2018, Elijah Newren wrote:\n\n> On Tue, Apr 24, 2018 at 4:58 AM, Johannes Schindelin\n> wrote:\n> >\n> > On Tue, 24 Apr 2018, Junio C Hamano wrote:\n> >\n> >> Yeah, but as opposed to passing \"oh, let's see if we can get a\n> >> reasonable result without rename detection just this time\" from the\n> >> command line, configuring merge.renames=false in would mean exactly\n> >> that: \"we don't need rename detection, just want to skip the cycles\n> >> spent for it\". That is why I wondered how well the resolve strategy\n> >> would have fit your needs.\n> >\n> > Please do not forget that the context is GVFS, where you would cause a lot\n> > of pain and suffering by letting users forget to specify that command-line\n> > option all the time, resulting in several gigabytes of objects having to\n> > be downloaded just for the sake of rename detection.\n> >\n> > So there is a pretty good point in doing this as a config option.\n> \n> I agree you need a config option, but I think Junio has a good point\n> that it's worth at least checking out the possibility of a different\n> one. In particular, you could add a merge.defaultStrategy (or maybe\n> merge.twohead to be similar to pull.twohead??) that is set to\n> 'resolve', and use that to avoid rename detection.\n> \n> Perhaps performance considerations rule out the resolve strategy and\n> favor recursive, or maybe you need the 'recursive' part of the\n> recursive strategy (rather than the rename part), or perhaps there's\n> some other special reason you need to go this route, but since you are\n> avoiding renames right now it's at least worth considering the resolve\n> strategy.\n\nI would really hesitate to go to a different merge strategy. The recursive\nstrategy really has the best track record in general, and we have to use\nall kinds of branching models (including heavily criss-crossed ones) with\nGVFS Git.\n\nCiao,\nDscho\n"},{"id":"345802","messageId":"87cb6954-05ee-1e9c-43ec-30157b6e08f5@gmail.com","threadId":"48335","inReplyTo":"xmqqlgdc5fa2.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v1 2/2] merge: Add merge.aggressive config setting","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-04-25T14:47:11Z","receivedAt":"2018-04-25T14:47:18Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 4/24/2018 7:57 PM, Junio C Hamano wrote:\n> Ben Peart writes:\n> \n>> That said, it makes sense to me to do\n>>> this when rename detection is turned off. In fact, I think you'd\n>>> automatically want to set aggressive to true whenever rename detection\n>>> is turned off (whether by your merge.renames option or the\n>>> -Xno-renames flag).\n>>> ...\n>>\n>> While combining them would work for our specific use scenario (since\n>> we turn both on already along with turning off merge.stat), I really\n>> hesitate to tie these two different flags and code paths together with\n>> a single config setting.\n> \n> The cases that non-agressive variant leaves unmerged are not\n> auto-resolved only because marking them as merged will rob the\n> chance from the rename detection logic to notice which ones are\n> \"new\" paths that could be matched with \"deleted\" ones to turn into\n> renames. If rename deteciton is not done, there is no reason to\n> leave it non aggressive, as \"#1 = missing, #2 = something and #3 =\n> missing\" entry (just one example that is not auto-resolved by\n> non-agressive, but the principle is the same) left unmerged in the\n> index will get resolved to keep the current entry by the post\n> processing logic anyway.\n> \n> In fact, checking git-merge-resolve would tell us that we already\n> use \"aggresive\" variant there unconditionally.\n> \n> So, I think Elijah is correct---there is no reason not to enable\n> this setting when the other one to refuse rename detection is in\n> effect.\n> \n\nThank you. I understand this description and it make sense to me. I'm \nunfamiliar with the merge code so have to rely on you who are the \nexperts for a sanity check.\n\nI will remove the separate merge.aggressive config setting and instead, \nset the aggressive flag when the renames is turned off (whether by the \nnew merge.renames setting or by the -Xno-renames flag) as Elijah suggests.\n"},{"id":"345807","messageId":"365838dc-d988-b72c-ef29-20369a7f54a2@gmail.com","threadId":"48335","inReplyTo":"xmqqd0yo5ejb.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 0/2] add additional config settings for merge","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-04-25T15:22:33Z","receivedAt":"2018-04-25T15:22:40Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 4/24/2018 8:13 PM, Junio C Hamano wrote:\n> Ben Peart writes:\n> \n>> diff.renameLimit::\n>> \tThe number of files to consider when performing the copy/rename\n>> -\tdetection; equivalent to the 'git diff' option `-l`.\n>> +\tdetection; equivalent to the 'git diff' option `-l`. This setting\n>> +\thas no effect if rename detection is turned off.\n> \n> You mean \"turned off via diff.renames\"?\n> \n> This is not meant as a suggestion to rewrite this paragraph\n> further---but if the answer is \"no\", then that might be an\n> indication that the sentence is inviting a misunderstanding.\n> \n\nYes, this is referring to turned off via the config setting \n\"diff.renames\" but it could also be turned off by passing \"--no-renames\" \non the command line.\n\nTo be clear, this documentation change isn't trying to document any \nchanges to the code or behavior - it is just an attempt to clarify what \nthe existing behavior is. If it isn't helping, I can remove it.\n\n>> diff.renames::\n>> \tWhether and how Git detects renames. If set to \"false\",\n>> diff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt\n>> index 5a9ab969db..38492bcb98 100644\n>> --- a/Documentation/merge-config.txt\n>> +++ b/Documentation/merge-config.txt\n>> @@ -39,7 +39,8 @@ include::fmt-merge-msg-config.txt[]\n>> merge.renameLimit::\n>> \tThe number of files to consider when performing rename detection\n>> \tduring a merge; if not specified, defaults to the value of\n>> -\tdiff.renameLimit.\n>> +\tdiff.renameLimit. This setting has no effect if rename detection\n>> +\tis turned off.\n> \n> Ditto. If your design is to make the merge machinery completely\n> ignore diff.renames and only pay attention to merge.renames [*1*],\n> then it probably is a good idea to be more specific here, by saying\n> \"... is turned off via ...\", though.\n> \n>> merge.renames::\n>> \tWhether and how Git detects renames. If set to \"false\",\n> \n> [Footnote]\n> \n> *1* ...which I do not think is such a good idea, by the way. I'd\n> personally expect merge.renames to allow overriding and falling back\n> to diff.renames, just like the {merge,diff}.renameLimit pair does.\n> \n\nIt looks like I'm in the minority on whether the merge settings should \ninherit from the corresponding diff settings so I will submit a new \npatch series that does it the same way as the {merge,diff}.renameLimit \npair works.\n\nI'll leave it as an exercise for someone else [1] to change any other \nmerge settings that should behave that way.\n\n[1] \nhttps://public-inbox.org/git/20180420133632.17580-1-benpeart@microsoft.com/T/#m52a3dbd0945360bfb873fd3b553472558ef3b796\n"},{"id":"345808","messageId":"CABPp-BGGtLGKVGY_ry8=sdi=s=EDphzTADt+0UR1F4NJpSqmFw@mail.gmail.com","threadId":"48335","inReplyTo":"68fa18c0-1dac-f6dc-0c41-fa5722c2c227@gmail.com","subject":"Re: [PATCH v2 1/2] merge: Add merge.renames config setting","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-04-25T16:01:21Z","receivedAt":"2018-04-25T16:01:33Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Apr 24, 2018 at 1:31 PM, Ben Peart wrote:\n> On 4/24/2018 2:59 PM, Elijah Newren wrote:\n>> On Tue, Apr 24, 2018 at 10:11 AM, Ben Peart \n>> wrote:\n>>>\n>>> diff --git a/builtin/merge.c b/builtin/merge.c\n>>> index 8746c5e3e8..3be52cd316 100644\n>>> --- a/builtin/merge.c\n>>> +++ b/builtin/merge.c\n>>> @@ -424,6 +424,7 @@ static void finish(struct commit *head_commit,\n>>> opts.output_format |=\n>>> DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;\n>>> opts.detect_rename = DIFF_DETECT_RENAME;\n>>> + git_config_get_bool(\"merge.renames\",\n>>> &opts.detect_rename);\n>>> diff_setup_done(&opts);\n>>> diff_tree_oid(head, new_head, \"\", &opts);\n>>> diffcore_std(&opts);\n>>\n>>\n>> Shouldn't this also be turned off if either (a) merge.renames is unset\n>> and diff.renames is false, or (b) the user specifies -Xno-renames?\n>>\n>\n> This makes me think that I should probably remove the line that overrides\n> the detect_rename setting with the merge config setting. As I look at the\n> code, none of the other merge options are reflected in the diffstat;\n> instead, all the settings are pretty much hard coded. Perhaps I shouldn't\n> rock that boat.\n\nActually, stat_graph_width respects the diff.statGraphWidth config\noption, even though it's slightly hidden due to the magic value of -1,\nand being handled from diff.c.\n\nHowever, trying to get this suggestion of mine hooked up, particularly\nwith -Xno-renames and -Xfind-renames (the latter because it might need\nto override a merge.renames or diff.renames config setting), might be\nslightly tricky because the -X options are only passed down to a\nsingle merge strategy but this code is outside of the merge\nstrategies. So making it a separate patch, or even a separate patch\nseries may make sense. I'm still interested in this change if you\naren't, but I'm fine with it not being part of your series if you\ndon't want to tackle it.\n"},{"id":"345851","messageId":"xmqqa7tq4u1x.fsf@gitster-ct.c.googlers.com","threadId":"48335","inReplyTo":"365838dc-d988-b72c-ef29-20369a7f54a2@gmail.com","subject":"Re: [PATCH v2 0/2] add additional config settings for merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-04-26T01:48:10Z","receivedAt":"2018-04-26T01:48:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ben Peart writes:\n\n> To be clear, this documentation change isn't trying to document any\n> changes to the code or behavior - it is just an attempt to clarify\n> what the existing behavior is.\n\nYeah, I know, and I do think the new text is a good first step in\nthe right direction; I merely was trying to help in the clarifying\neffort ;-)\n\n"},{"id":"345913","messageId":"20180426205202.23056-2-benpeart@microsoft.com","threadId":"48335","inReplyTo":"20180426205202.23056-1-benpeart@microsoft.com","subject":"[PATCH v3 1/3] merge: update documentation for {merge,diff}.renameLimit","fromName":"Ben Peart","fromEmail":"ben.peart@microsoft.com","sentAt":"2018-04-26T20:52:19Z","receivedAt":"2018-04-26T20:52:24Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"Update the documentation to better indicate that the renameLimit setting is\nignored if rename detection is turned off via command line options or config\nsettings.\n\nSigned-off-by: Ben Peart \n---\n Documentation/diff-config.txt | 3 ++-\n Documentation/merge-config.txt | 3 ++-\n 2 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt\nindex 5ca942ab5e..77caa66c2f 100644\n--- a/Documentation/diff-config.txt\n+++ b/Documentation/diff-config.txt\n@@ -112,7 +112,8 @@ diff.orderFile::\n \n diff.renameLimit::\n \tThe number of files to consider when performing the copy/rename\n-\tdetection; equivalent to the 'git diff' option `-l`.\n+\tdetection; equivalent to the 'git diff' option `-l`. This setting\n+\thas no effect if rename detection is turned off.\n \n diff.renames::\n \tWhether and how Git detects renames. If set to \"false\",\ndiff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt\nindex 12b6bbf591..48ee3bce77 100644\n--- a/Documentation/merge-config.txt\n+++ b/Documentation/merge-config.txt\n@@ -35,7 +35,8 @@ include::fmt-merge-msg-config.txt[]\n merge.renameLimit::\n \tThe number of files to consider when performing rename detection\n \tduring a merge; if not specified, defaults to the value of\n-\tdiff.renameLimit.\n+\tdiff.renameLimit. This setting has no effect if rename detection\n+\tis turned off.\n \n merge.renormalize::\n \tTell Git that canonical representation of files in the\n-- \n2.17.0.windows.1\n\n"},{"id":"345914","messageId":"20180426205202.23056-1-benpeart@microsoft.com","threadId":"48335","inReplyTo":"20180420133632.17580-1-benpeart@microsoft.com","subject":"[PATCH v3 0/3] add merge.renames config setting","fromName":"Ben Peart","fromEmail":"ben.peart@microsoft.com","sentAt":"2018-04-26T20:52:18Z","receivedAt":"2018-04-26T20:52:28Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"This is a complete rewrite based on the feedback from earlier patches.\n\nUpdate the documentation to better indicate command line options that override\nvarious config settings related to merge.\n\nAdd a new config merge.renames setting to to control the rename detection\nbehavior of merge. This setting will default to the value of diff.renames.\n\nAlso adds logic so that when rename detection is turned off, the aggressive\nflag is passed to read_tree() so that it can auto resolve more cases that would\nhave been handled by rename detection.\n\nFor the repro that I have been using this drops the merge time from ~1 hour to\n~5 minutes and the unmerged entries goes down from ~40,000 to 1.\n\nHelped-by: Kevin Willford \nReviewed-by: Johannes Schindelin \nSigned-off-by: Ben Peart \n\nBase Ref: master\nWeb-Diff: https://github.com/benpeart/git/commit/6a8372d517\nCheckout: git fetch https://github.com/benpeart/git merge-options-v3 && git checkout 6a8372d517\n\n### Patches\n\nBen Peart (3):\n merge: update documentation for {merge,diff}.renameLimit\n merge: Add merge.renames config setting\n merge: pass aggressive when rename detection is turned off\n\n Documentation/diff-config.txt | 3 ++-\n Documentation/merge-config.txt | 9 +++++++-\n Documentation/merge-strategies.txt | 6 +++--\n diff.c | 2 +-\n diff.h | 2 ++\n merge-recursive.c | 27 +++++++++++++++++------\n merge-recursive.h | 8 ++++++-\n t/t3034-merge-recursive-rename-options.sh | 18 +++++++++++++++\n 8 files changed, 62 insertions(+), 13 deletions(-)\n\n\nbase-commit: 1f1cddd558b54bb0ce19c8ace353fd07b758510d\n-- \n2.17.0.windows.1\n\n\n"},{"id":"345915","messageId":"20180426205202.23056-3-benpeart@microsoft.com","threadId":"48335","inReplyTo":"20180426205202.23056-1-benpeart@microsoft.com","subject":"[PATCH v3 2/3] merge: Add merge.renames config setting","fromName":"Ben Peart","fromEmail":"ben.peart@microsoft.com","sentAt":"2018-04-26T20:52:21Z","receivedAt":"2018-04-26T20:52:31Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"Add the ability to control rename detection for merge via a config setting.\nThis setting behaves the same and defaults to the value of diff.renames but only\napplies to merge.\n\nReviewed-by: Johannes Schindelin \nSigned-off-by: Ben Peart \n---\n Documentation/merge-config.txt | 6 ++++++\n Documentation/merge-strategies.txt | 6 ++++--\n diff.c | 2 +-\n diff.h | 2 ++\n merge-recursive.c | 23 +++++++++++++++++------\n merge-recursive.h | 8 +++++++-\n t/t3034-merge-recursive-rename-options.sh | 18 ++++++++++++++++++\n 7 files changed, 55 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt\nindex 48ee3bce77..59848e5634 100644\n--- a/Documentation/merge-config.txt\n+++ b/Documentation/merge-config.txt\n@@ -38,6 +38,12 @@ merge.renameLimit::\n \tdiff.renameLimit. This setting has no effect if rename detection\n \tis turned off.\n \n+merge.renames::\n+\tWhether and how Git detects renames. If set to \"false\",\n+\trename detection is disabled. If set to \"true\", basic rename\n+\tdetection is enabled. If set to \"copies\" or \"copy\", Git will\n+\tdetect copies, as well. Defaults to the value of diff.renames.\n+\n merge.renormalize::\n \tTell Git that canonical representation of files in the\n \trepository has changed over time (e.g. earlier commits record\ndiff --git a/Documentation/merge-strategies.txt b/Documentation/merge-strategies.txt\nindex 4a58aad4b8..1e0728aa12 100644\n--- a/Documentation/merge-strategies.txt\n+++ b/Documentation/merge-strategies.txt\n@@ -84,12 +84,14 @@ no-renormalize;;\n \t`merge.renormalize` configuration variable.\n \n no-renames;;\n-\tTurn off rename detection.\n+\tTurn off rename detection. This overrides the `merge.renames`\n+\tconfiguration variable.\n \tSee also linkgit:git-diff[1] `--no-renames`.\n \n find-renames[=];;\n \tTurn on rename detection, optionally setting the similarity\n-\tthreshold. This is the default.\n+\tthreshold. This is the default. This overrides the\n+\t'merge.renames' configuration variable.\n \tSee also linkgit:git-diff[1] `--find-renames`.\n \n rename-threshold=;;\ndiff --git a/diff.c b/diff.c\nindex 1289df4b1f..5dfc24aa6d 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -177,7 +177,7 @@ static int parse_submodule_params(struct diff_options *options, const char *valu\n \treturn 0;\n }\n \n-static int git_config_rename(const char *var, const char *value)\n+int git_config_rename(const char *var, const char *value)\n {\n \tif (!value)\n \t\treturn DIFF_DETECT_RENAME;\ndiff --git a/diff.h b/diff.h\nindex d29560f822..806faee2b3 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -324,6 +324,8 @@ extern int git_diff_ui_config(const char *var, const char *value, void *cb);\n extern void diff_setup(struct diff_options *);\n extern int diff_opt_parse(struct diff_options *, const char **, int, const char *);\n extern void diff_setup_done(struct diff_options *);\n+extern int git_config_rename(const char *var, const char *value);\n+\n \n #define DIFF_DETECT_RENAME\t1\n #define DIFF_DETECT_COPY\t2\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 0c0d48624d..2637d34d87 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -555,13 +555,13 @@ static struct string_list *get_renames(struct merge_options *o,\n \tstruct diff_options opts;\n \n \trenames = xcalloc(1, sizeof(struct string_list));\n-\tif (!o->detect_rename)\n+\tif (!merge_detect_rename(o))\n \t\treturn renames;\n \n \tdiff_setup(&opts);\n \topts.flags.recursive = 1;\n \topts.flags.rename_empty = 0;\n-\topts.detect_rename = DIFF_DETECT_RENAME;\n+\topts.detect_rename = merge_detect_rename(o);\n \topts.rename_limit = o->merge_rename_limit >= 0 ? o->merge_rename_limit :\n \t\t\t o->diff_rename_limit >= 0 ? o->diff_rename_limit :\n \t\t\t 1000;\n@@ -2232,9 +2232,18 @@ int merge_recursive_generic(struct merge_options *o,\n \n static void merge_recursive_config(struct merge_options *o)\n {\n+\tchar *value = NULL;\n \tgit_config_get_int(\"merge.verbosity\", &o->verbosity);\n \tgit_config_get_int(\"diff.renamelimit\", &o->diff_rename_limit);\n \tgit_config_get_int(\"merge.renamelimit\", &o->merge_rename_limit);\n+\tif (!git_config_get_string(\"diff.renames\", &value)) {\n+\t\to->diff_detect_rename = git_config_rename(\"diff.renames\", value);\n+\t\tfree(value);\n+\t}\n+\tif (!git_config_get_string(\"merge.renames\", &value)) {\n+\t\to->merge_detect_rename = git_config_rename(\"merge.renames\", value);\n+\t\tfree(value);\n+\t}\n \tgit_config(git_xmerge_config, NULL);\n }\n \n@@ -2244,10 +2253,11 @@ void init_merge_options(struct merge_options *o)\n \tmemset(o, 0, sizeof(struct merge_options));\n \to->verbosity = 2;\n \to->buffer_output = 1;\n+\to->diff_detect_rename = -1;\n+\to->merge_detect_rename = -1;\n \to->diff_rename_limit = -1;\n \to->merge_rename_limit = -1;\n \to->renormalize = 0;\n-\to->detect_rename = 1;\n \tmerge_recursive_config(o);\n \tmerge_verbosity = getenv(\"GIT_MERGE_VERBOSITY\");\n \tif (merge_verbosity)\n@@ -2298,18 +2308,19 @@ int parse_merge_opt(struct merge_options *o, const char *s)\n \telse if (!strcmp(s, \"no-renormalize\"))\n \t\to->renormalize = 0;\n \telse if (!strcmp(s, \"no-renames\"))\n-\t\to->detect_rename = 0;\n+\t\to->merge_detect_rename = 0;\n \telse if (!strcmp(s, \"find-renames\")) {\n-\t\to->detect_rename = 1;\n+\t\to->merge_detect_rename = 1;\n \t\to->rename_score = 0;\n \t}\n \telse if (skip_prefix(s, \"find-renames=\", &arg) ||\n \t\t skip_prefix(s, \"rename-threshold=\", &arg)) {\n \t\tif ((o->rename_score = parse_rename_score(&arg)) == -1 || *arg != 0)\n \t\t\treturn -1;\n-\t\to->detect_rename = 1;\n+\t\to->merge_detect_rename = 1;\n \t}\n \telse\n \t\treturn -1;\n+\n \treturn 0;\n }\ndiff --git a/merge-recursive.h b/merge-recursive.h\nindex 80d69d1401..0c5f7eff98 100644\n--- a/merge-recursive.h\n+++ b/merge-recursive.h\n@@ -17,7 +17,8 @@ struct merge_options {\n \tunsigned renormalize : 1;\n \tlong xdl_opts;\n \tint verbosity;\n-\tint detect_rename;\n+\tint diff_detect_rename;\n+\tint merge_detect_rename;\n \tint diff_rename_limit;\n \tint merge_rename_limit;\n \tint rename_score;\n@@ -28,6 +29,11 @@ struct merge_options {\n \tstruct hashmap current_file_dir_set;\n \tstruct string_list df_conflict_file_set;\n };\n+inline int merge_detect_rename(struct merge_options *o)\n+{\n+\treturn o->merge_detect_rename >= 0 ? o->merge_detect_rename :\n+\t\to->diff_detect_rename >= 0 ? o->diff_detect_rename : 1;\n+}\n \n /* merge_trees() but with recursive ancestor consolidation */\n int merge_recursive(struct merge_options *o,\ndiff --git a/t/t3034-merge-recursive-rename-options.sh b/t/t3034-merge-recursive-rename-options.sh\nindex b9c4028496..3d9fae68c4 100755\n--- a/t/t3034-merge-recursive-rename-options.sh\n+++ b/t/t3034-merge-recursive-rename-options.sh\n@@ -309,4 +309,22 @@ test_expect_success 'last wins in --find-renames= --rename-threshold=' '\n \tcheck_threshold_0\n '\n \n+test_expect_success 'merge.renames disables rename detection' '\n+\tgit read-tree --reset -u HEAD &&\n+\tgit -c merge.renames=false merge-recursive $tail &&\n+\tcheck_no_renames\n+'\n+\n+test_expect_success 'merge.renames defaults to diff.renames' '\n+\tgit read-tree --reset -u HEAD &&\n+\tgit -c diff.renames=false merge-recursive $tail &&\n+\tcheck_no_renames\n+'\n+\n+test_expect_success 'merge.renames overrides diff.renames' '\n+\tgit read-tree --reset -u HEAD &&\n+\ttest_must_fail git -c diff.renames=false -c merge.renames=true merge-recursive $tail &&\n+\t$check_50\n+'\n+\n test_done\n-- \n2.17.0.windows.1\n\n"},{"id":"345916","messageId":"20180426205202.23056-4-benpeart@microsoft.com","threadId":"48335","inReplyTo":"20180426205202.23056-1-benpeart@microsoft.com","subject":"[PATCH v3 3/3] merge: pass aggressive when rename detection is turned off","fromName":"Ben Peart","fromEmail":"ben.peart@microsoft.com","sentAt":"2018-04-26T20:52:22Z","receivedAt":"2018-04-26T20:52:35Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"Set aggressive flag in git_merge_trees() when rename detection is turned off.\nThis allows read_tree() to auto resolve more cases that would have otherwise\nbeen handled by the rename detection.\n\nReviewed-by: Johannes Schindelin \nSigned-off-by: Ben Peart \n---\n merge-recursive.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 2637d34d87..6cc4404144 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -276,6 +276,7 @@ static void init_tree_desc_from_tree(struct tree_desc *desc, struct tree *tree)\n }\n \n static int git_merge_trees(int index_only,\n+\t\t\t int aggressive,\n \t\t\t struct tree *common,\n \t\t\t struct tree *head,\n \t\t\t struct tree *merge)\n@@ -294,6 +295,7 @@ static int git_merge_trees(int index_only,\n \topts.fn = threeway_merge;\n \topts.src_index = &the_index;\n \topts.dst_index = &the_index;\n+\topts.aggressive = aggressive;\n \tsetup_unpack_trees_porcelain(&opts, \"merge\");\n \n \tinit_tree_desc_from_tree(t+0, common);\n@@ -1993,7 +1995,7 @@ int merge_trees(struct merge_options *o,\n \t\treturn 1;\n \t}\n \n-\tcode = git_merge_trees(o->call_depth, common, head, merge);\n+\tcode = git_merge_trees(o->call_depth, !merge_detect_rename(o), common, head, merge);\n \n \tif (code != 0) {\n \t\tif (show(o, 4) || o->call_depth)\n-- \n2.17.0.windows.1\n\n"},{"id":"345918","messageId":"CABPp-BFh=gL6RnbST2bgtynkij1Z5TMgAr1Via5_VyteF5eBMg@mail.gmail.com","threadId":"48335","inReplyTo":"20180426205202.23056-1-benpeart@microsoft.com","subject":"Re: [PATCH v3 0/3] add merge.renames config setting","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-04-26T22:08:07Z","receivedAt":"2018-04-26T22:08:12Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi Ben,\n\nOn Thu, Apr 26, 2018 at 1:52 PM, Ben Peart wrote:\n> This is a complete rewrite based on the feedback from earlier patches.\n\nThanks for pushing forward on this.\n\n> Update the documentation to better indicate command line options that override\n> various config settings related to merge.\n>\n> Add a new config merge.renames setting to to control the rename detection\n> behavior of merge. This setting will default to the value of diff.renames.\n>\n> Also adds logic so that when rename detection is turned off, the aggressive\n> flag is passed to read_tree() so that it can auto resolve more cases that would\n> have been handled by rename detection.\n>\n> For the repro that I have been using this drops the merge time from ~1 hour to\n> ~5 minutes and the unmerged entries goes down from ~40,000 to 1.\n>\n> Helped-by: Kevin Willford \n> Reviewed-by: Johannes Schindelin \n> Signed-off-by: Ben Peart \n>\n> Base Ref: master\n\nWe may need to figure out how to coordinate amongst a few topics.\nLooking over your patches, there are going to be a few conflicts with\nen/rename-directory-detection-reboot, so this won't apply to pu.\nMartin's series to introduce clear_unpack_trees_porcelain()[1], which\nhe was waiting to submit until mine went through, will also conflict\nwith this, if he uses the changes I suggested for the handling in\nmerge-recursive[2]. These aren't major conflicts, but I'm just\nflagging it.\n\n[1] https://public-inbox.org/git/cover.1524545557.git.martin.agren@gmail.com/\n[2] https://public-inbox.org/git/20180424162939.20956-1-newren@gmail.com/\n"},{"id":"345921","messageId":"CABPp-BE29rwZCDqFHH-nzrDub6MMdtoiorj0jv3K6B6cmfcaLA@mail.gmail.com","threadId":"48335","inReplyTo":"20180426205202.23056-3-benpeart@microsoft.com","subject":"Re: [PATCH v3 2/3] merge: Add merge.renames config setting","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-04-26T22:52:18Z","receivedAt":"2018-04-26T22:52:24Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Apr 26, 2018 at 1:52 PM, Ben Peart wrote:\n\n> +merge.renames::\n> + Whether and how Git detects renames. If set to \"false\",\n> + rename detection is disabled. If set to \"true\", basic rename\n> + detection is enabled. If set to \"copies\" or \"copy\", Git will\n> + detect copies, as well. Defaults to the value of diff.renames.\n> +\n\nWe shouldn't allow users to force copy detection on for merges The\ndiff side of the code will detect them correctly but the code in\nmerge-recursive will mishandle the copy pairs. I think fixing it is\nsomewhere between big can of worms and\nit's-a-configuration-that-doesn't-even-make-sense, but it's been a\nwhile since I thought about it.\n\n> diff --git a/merge-recursive.h b/merge-recursive.h\n> index 80d69d1401..0c5f7eff98 100644\n> --- a/merge-recursive.h\n> +++ b/merge-recursive.h\n> @@ -17,7 +17,8 @@ struct merge_options {\n> unsigned renormalize : 1;\n> long xdl_opts;\n> int verbosity;\n> - int detect_rename;\n> + int diff_detect_rename;\n> + int merge_detect_rename;\n> int diff_rename_limit;\n> int merge_rename_limit;\n> int rename_score;\n> @@ -28,6 +29,11 @@ struct merge_options {\n> struct hashmap current_file_dir_set;\n> struct string_list df_conflict_file_set;\n> };\n> +inline int merge_detect_rename(struct merge_options *o)\n> +{\n> + return o->merge_detect_rename >= 0 ? o->merge_detect_rename :\n> + o->diff_detect_rename >= 0 ? o->diff_detect_rename : 1;\n> +}\n\nWhy did you split o->detect_rename into two fields? You then\nrecombine them in merge_detect_rename(), and after initial setup only\never access them through that function. Having two fields worries me\nthat people will accidentally introduce bugs by using one of them\ninstead of the merge_detect_rename() function. Is there a reason you\ndecided against having the initial setup just set a single value and\nthen use it directly?\n"},{"id":"345922","messageId":"CABPp-BHg++tvbd+Y8xCCNmi+fAv_4azXCkCWDucFLDj1sXeWAw@mail.gmail.com","threadId":"48335","inReplyTo":"20180426205202.23056-4-benpeart@microsoft.com","subject":"Re: [PATCH v3 3/3] merge: pass aggressive when rename detection is turned off","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-04-26T23:00:00Z","receivedAt":"2018-04-26T23:00:09Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Apr 26, 2018 at 1:52 PM, Ben Peart wrote:\n> Set aggressive flag in git_merge_trees() when rename detection is turned off.\n> This allows read_tree() to auto resolve more cases that would have otherwise\n> been handled by the rename detection.\n>\n> Reviewed-by: Johannes Schindelin \n> Signed-off-by: Ben Peart \n> ---\n> merge-recursive.c | 4 +++-\n> 1 file changed, 3 insertions(+), 1 deletion(-)\n>\n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index 2637d34d87..6cc4404144 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -276,6 +276,7 @@ static void init_tree_desc_from_tree(struct tree_desc *desc, struct tree *tree)\n> }\n>\n> static int git_merge_trees(int index_only,\n> + int aggressive,\n> struct tree *common,\n> struct tree *head,\n> struct tree *merge)\n> @@ -294,6 +295,7 @@ static int git_merge_trees(int index_only,\n> opts.fn = threeway_merge;\n> opts.src_index = &the_index;\n> opts.dst_index = &the_index;\n> + opts.aggressive = aggressive;\n> setup_unpack_trees_porcelain(&opts, \"merge\");\n>\n> init_tree_desc_from_tree(t+0, common);\n> @@ -1993,7 +1995,7 @@ int merge_trees(struct merge_options *o,\n> return 1;\n> }\n>\n> - code = git_merge_trees(o->call_depth, common, head, merge);\n> + code = git_merge_trees(o->call_depth, !merge_detect_rename(o), common, head, merge);\n>\n> if (code != 0) {\n> if (show(o, 4) || o->call_depth)\n> --\n> 2.17.0.windows.1\n\nPatch looks fine but as a heads up -- since merge_options is a\nparameter in git_merge_trees after the\nen/rename-directory-detection-reboot lands, we'll be able to switch\nthis patch to set opts.aggressive directly instead of needing to pass\nit in as a parameter.\n"},{"id":"345923","messageId":"CABPp-BEa2EDdeDfcXxRERKAuOPUYTsBGZB8XyTXDYN1JpHsbXA@mail.gmail.com","threadId":"48335","inReplyTo":"20180426205202.23056-2-benpeart@microsoft.com","subject":"Re: [PATCH v3 1/3] merge: update documentation for {merge,diff}.renameLimit","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-04-26T23:11:50Z","receivedAt":"2018-04-26T23:11:55Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Apr 26, 2018 at 1:52 PM, Ben Peart wrote:\n> Update the documentation to better indicate that the renameLimit setting is\n> ignored if rename detection is turned off via command line options or config\n> settings.\n>\n> Signed-off-by: Ben Peart \n> ---\n> Documentation/diff-config.txt | 3 ++-\n> Documentation/merge-config.txt | 3 ++-\n> 2 files changed, 4 insertions(+), 2 deletions(-)\n>\n> diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt\n> index 5ca942ab5e..77caa66c2f 100644\n> --- a/Documentation/diff-config.txt\n> +++ b/Documentation/diff-config.txt\n> @@ -112,7 +112,8 @@ diff.orderFile::\n>\n> diff.renameLimit::\n> The number of files to consider when performing the copy/rename\n> - detection; equivalent to the 'git diff' option `-l`.\n> + detection; equivalent to the 'git diff' option `-l`. This setting\n> + has no effect if rename detection is turned off.\n>\n> diff.renames::\n> Whether and how Git detects renames. If set to \"false\",\n> diff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt\n> index 12b6bbf591..48ee3bce77 100644\n> --- a/Documentation/merge-config.txt\n> +++ b/Documentation/merge-config.txt\n> @@ -35,7 +35,8 @@ include::fmt-merge-msg-config.txt[]\n> merge.renameLimit::\n> The number of files to consider when performing rename detection\n> during a merge; if not specified, defaults to the value of\n> - diff.renameLimit.\n> + diff.renameLimit. This setting has no effect if rename detection\n> + is turned off.\n>\n> merge.renormalize::\n> Tell Git that canonical representation of files in the\n> --\n> 2.17.0.windows.1\n\nPatch looks fine, but it's hard for me not to notice a separate issue\nin this area independent of your series: I'm curious if we should\ndocument that the value of 0 is special here (as per Jonathan Tan's\ncommit 89973554b52c (\"diffcore-rename: make diff-tree -l0 mean\n-l\", 2017-11-29)), and doesn't actually drop the limit to 0.\ncc'ing Jonathan Tan for his thoughts.\n"},{"id":"345924","messageId":"20180426162339.db6b4855fedb5e5244ba7dd1@google.com","threadId":"48335","inReplyTo":"CABPp-BEa2EDdeDfcXxRERKAuOPUYTsBGZB8XyTXDYN1JpHsbXA@mail.gmail.com","subject":"Re: [PATCH v3 1/3] merge: update documentation for {merge,diff}.renameLimit","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-04-26T23:23:39Z","receivedAt":"2018-04-26T23:23:45Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On Thu, 26 Apr 2018 16:11:50 -0700\nElijah Newren wrote:\n\n> Patch looks fine, but it's hard for me not to notice a separate issue\n> in this area independent of your series: I'm curious if we should\n> document that the value of 0 is special here (as per Jonathan Tan's\n> commit 89973554b52c (\"diffcore-rename: make diff-tree -l0 mean\n> -l\", 2017-11-29)), and doesn't actually drop the limit to 0.\n> cc'ing Jonathan Tan for his thoughts.\n\nDocumenting that the value of 0 is special does make sense to me. I\nthink this patch can go in as-is, though - it is already an improvement.\n"},{"id":"345930","messageId":"7de8f144-8a37-e471-48e8-0b6f17a7bf29@gmail.com","threadId":"48335","inReplyTo":"CABPp-BE29rwZCDqFHH-nzrDub6MMdtoiorj0jv3K6B6cmfcaLA@mail.gmail.com","subject":"Re: [PATCH v3 2/3] merge: Add merge.renames config setting","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-04-27T00:54:20Z","receivedAt":"2018-04-27T00:54:24Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 4/26/2018 6:52 PM, Elijah Newren wrote:\n> On Thu, Apr 26, 2018 at 1:52 PM, Ben Peart wrote:\n> \n>> +merge.renames::\n>> + Whether and how Git detects renames. If set to \"false\",\n>> + rename detection is disabled. If set to \"true\", basic rename\n>> + detection is enabled. If set to \"copies\" or \"copy\", Git will\n>> + detect copies, as well. Defaults to the value of diff.renames.\n>> +\n> \n> We shouldn't allow users to force copy detection on for merges The\n> diff side of the code will detect them correctly but the code in\n> merge-recursive will mishandle the copy pairs. I think fixing it is\n> somewhere between big can of worms and\n> it's-a-configuration-that-doesn't-even-make-sense, but it's been a\n> while since I thought about it.\n\nColor me puzzled. :) The consensus was that the default value for \nmerge.renames come from diff.renames. diff.renames supports copy \ndetection which means that merge.renames will inherit that value. My \nassumption was that is what was intended so when I reimplemented it, I \nfully implemented it that way.\n\nAre you now requesting to only use diff.renames as the default if the \nvalue is true or false but not if it is copy? What should happen if \ndiff.renames is actually set to copy? Should merge silently change that \nto true, display a warning, error out, or something else? Do you have \nsome other behavior for how to handle copy being inherited from \ndiff.renames you'd like to see?\n\nCan you write the documentation that clearly explains the exact behavior \nyou want? That would kill two birds with one stone... :)\n\n> \n>> diff --git a/merge-recursive.h b/merge-recursive.h\n>> index 80d69d1401..0c5f7eff98 100644\n>> --- a/merge-recursive.h\n>> +++ b/merge-recursive.h\n>> @@ -17,7 +17,8 @@ struct merge_options {\n>> unsigned renormalize : 1;\n>> long xdl_opts;\n>> int verbosity;\n>> - int detect_rename;\n>> + int diff_detect_rename;\n>> + int merge_detect_rename;\n>> int diff_rename_limit;\n>> int merge_rename_limit;\n>> int rename_score;\n>> @@ -28,6 +29,11 @@ struct merge_options {\n>> struct hashmap current_file_dir_set;\n>> struct string_list df_conflict_file_set;\n>> };\n>> +inline int merge_detect_rename(struct merge_options *o)\n>> +{\n>> + return o->merge_detect_rename >= 0 ? o->merge_detect_rename :\n>> + o->diff_detect_rename >= 0 ? o->diff_detect_rename : 1;\n>> +}\n> \n> Why did you split o->detect_rename into two fields? You then\n> recombine them in merge_detect_rename(), and after initial setup only\n> ever access them through that function. Having two fields worries me\n> that people will accidentally introduce bugs by using one of them\n> instead of the merge_detect_rename() function. Is there a reason you\n> decided against having the initial setup just set a single value and\n> then use it directly?\n> \n\nThe setup of this value is split into 3 places that may or may not all \nget called. The initial values, the values that come from the config \nsettings and then any values passed on the command line.\n\nBecause the merge value can now inherit from the diff value, you only \nknow the final value after you have received all possible inputs. That \nmakes it necessary to be a calculated value.\n\nIf you look at diff_rename_limit/merge_rename_limit, detect_rename \nfollow the same pattern for the same reasons. It turns out \ndetect_rename was a little more complex because it is used in 3 \ndifferent locations (vs just one) which is why I wrapped the inheritance \nlogic into the helper function merge_detect_rename().\n"},{"id":"345933","messageId":"xmqqy3h9z8sj.fsf@gitster-ct.c.googlers.com","threadId":"48335","inReplyTo":"7de8f144-8a37-e471-48e8-0b6f17a7bf29@gmail.com","subject":"Re: [PATCH v3 2/3] merge: Add merge.renames config setting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-04-27T02:23:56Z","receivedAt":"2018-04-27T02:24:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ben Peart writes:\n\n> Color me puzzled. :) The consensus was that the default value for\n> merge.renames come from diff.renames. diff.renames supports copy\n> detection which means that merge.renames will inherit that value. My\n> assumption was that is what was intended so when I reimplemented it, I\n> fully implemented it that way.\n>\n> Are you now requesting to only use diff.renames as the default if the\n> value is true or false but not if it is copy? What should happen if\n> diff.renames is actually set to copy? Should merge silently change\n> that to true, display a warning, error out, or something else? Do you\n> have some other behavior for how to handle copy being inherited from\n> diff.renames you'd like to see?\n>\n> Can you write the documentation that clearly explains the exact\n> behavior you want? That would kill two birds with one stone... :)\n\nI think demoting from copy to rename-only is a good idea, at least\nfor now, because I do not believe we have figured out what we want\nto happen when we detect copied files are involved in a merge.\n\nBut I am not sure if we even want to fail merge.renames=copy as an\ninvalid configuration. So my gut feeling of the best solution to\nthe above is to do something like:\n\n - whether the configuration comes from diff.renames or\n merge.renames, turn *.renames=copy to true inside the merge\n recursive machinery.\n\n - document the fact in \"git merge-recursive\" documentation (or \"git\n merge\" documentation) to say \"_currently_ asking for rename\n detection to find copies and renames will do the same\n thing---copies are ignored\", impliying \"this might change in the\n future\", in the BUGS section.\n\n"},{"id":"345935","messageId":"CABPp-BERgc9EZ=hw4CepgXptO283mW3O30_pHrj4jtz3QSCFjQ@mail.gmail.com","threadId":"48335","inReplyTo":"xmqqy3h9z8sj.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3 2/3] merge: Add merge.renames config setting","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-04-27T03:28:13Z","receivedAt":"2018-04-27T03:28:18Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Apr 26, 2018 at 7:23 PM, Junio C Hamano wrote:\n> Ben Peart writes:\n>\n>> Color me puzzled. :) The consensus was that the default value for\n>> merge.renames come from diff.renames. diff.renames supports copy\n>> detection which means that merge.renames will inherit that value. My\n>> assumption was that is what was intended so when I reimplemented it, I\n>> fully implemented it that way.\n>>\n>> Are you now requesting to only use diff.renames as the default if the\n>> value is true or false but not if it is copy? What should happen if\n>> diff.renames is actually set to copy? Should merge silently change\n>> that to true, display a warning, error out, or something else? Do you\n>> have some other behavior for how to handle copy being inherited from\n>> diff.renames you'd like to see?\n>>\n>> Can you write the documentation that clearly explains the exact\n>> behavior you want? That would kill two birds with one stone... :)\n>\n> I think demoting from copy to rename-only is a good idea, at least\n> for now, because I do not believe we have figured out what we want\n> to happen when we detect copied files are involved in a merge.\n>\n> But I am not sure if we even want to fail merge.renames=copy as an\n> invalid configuration. So my gut feeling of the best solution to\n> the above is to do something like:\n>\n> - whether the configuration comes from diff.renames or\n> merge.renames, turn *.renames=copy to true inside the merge\n> recursive machinery.\n>\n> - document the fact in \"git merge-recursive\" documentation (or \"git\n> merge\" documentation) to say \"_currently_ asking for rename\n> detection to find copies and renames will do the same\n> thing---copies are ignored\", impliying \"this might change in the\n> future\", in the BUGS section.\n\nYes, I agree. One more thing:\n\n - It may be best to avoid advertising \"copies\" as a vaild option for\nmerge.renames since it doesn't have any current practical use\nanywhere. (Remove the sentence 'If set to \"copies\" or \"copy\", Git\nwill detect copies, as well.' from the documentation)\n\nMy rationale for translating \"copy\" to \"true\" is a little different\nthan Junio's, though:\n\n1) The reason we have configuration options around renames and copies\nis primarily because they are expensive to compute. So we let some\nusers specify that they don't want them, other users are willing to\npay for rename detection, and others are willing to pay for both\nrename and copy detection.\n2) If rename/copy detection were cheap, every part of git would just\ncompute whatever level of detection was relevant and use it.\n3) The resolve and octopus merge strategies ignores diff.renames and\nmerge.renames, because they don't have logic to use any rename\ninformation. diff and log can use both renames and copies. And the\nrecursive merge machinery is code which can use renames but not\ncopies.\n4) Therefore, translating from \"copy\" to \"true\" inside the merge\nrecursive machinery is fine and not an error because we are using as\nmuch detection information as is relevant to the algorithm and which\nthe user is willing to pay for.\n\nTo throw one more wrinkle in here, merge.renames could actually be set\nto \"copy\" and make sense, because we compute diffs multiple times.\nTwice within the recursive merge machinery (for which we'd want to\ntranslate \"copy\" to \"true\"), and once for the diffstat at the end\n(which comes from builtin/merge.c, and for which it could make sense\nto detect copies).\n\n(Kind of curious whether Junio agrees with my rationale or thinks I'm\nout in left field with it...)\n"},{"id":"345937","messageId":"CABPp-BFDVQiUtytQ0TJu6inzsd93RD96XkazLT5BZJqeM2jX8Q@mail.gmail.com","threadId":"48335","inReplyTo":"7de8f144-8a37-e471-48e8-0b6f17a7bf29@gmail.com","subject":"Re: [PATCH v3 2/3] merge: Add merge.renames config setting","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-04-27T04:17:51Z","receivedAt":"2018-04-27T04:17:56Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Apr 26, 2018 at 5:54 PM, Ben Peart wrote:\n> On 4/26/2018 6:52 PM, Elijah Newren wrote:\n>> On Thu, Apr 26, 2018 at 1:52 PM, Ben Peart \n>> wrote:\n>>\n>>> diff --git a/merge-recursive.h b/merge-recursive.h\n>>> index 80d69d1401..0c5f7eff98 100644\n>>> --- a/merge-recursive.h\n>>> +++ b/merge-recursive.h\n>>> @@ -17,7 +17,8 @@ struct merge_options {\n>>> unsigned renormalize : 1;\n>>> long xdl_opts;\n>>> int verbosity;\n>>> - int detect_rename;\n>>> + int diff_detect_rename;\n>>> + int merge_detect_rename;\n>>> int diff_rename_limit;\n>>> int merge_rename_limit;\n>>> int rename_score;\n>>> @@ -28,6 +29,11 @@ struct merge_options {\n>>> struct hashmap current_file_dir_set;\n>>> struct string_list df_conflict_file_set;\n>>> };\n>>> +inline int merge_detect_rename(struct merge_options *o)\n>>> +{\n>>> + return o->merge_detect_rename >= 0 ? o->merge_detect_rename :\n>>> + o->diff_detect_rename >= 0 ? o->diff_detect_rename : 1;\n>>> +}\n>>\n>>\n>> Why did you split o->detect_rename into two fields? You then\n>> recombine them in merge_detect_rename(), and after initial setup only\n>> ever access them through that function. Having two fields worries me\n>> that people will accidentally introduce bugs by using one of them\n>> instead of the merge_detect_rename() function. Is there a reason you\n>> decided against having the initial setup just set a single value and\n>> then use it directly?\n>>\n> The setup of this value is split into 3 places that may or may not all get\n> called. The initial values, the values that come from the config settings\n> and then any values passed on the command line.\n>\n> Because the merge value can now inherit from the diff value, you only know\n> the final value after you have received all possible inputs. That makes it\n> necessary to be a calculated value.\n>\n> If you look at diff_rename_limit/merge_rename_limit, detect_rename follow\n> the same pattern for the same reasons. It turns out detect_rename was a\n> little more complex because it is used in 3 different locations (vs just\n> one) which is why I wrapped the inheritance logic into the helper function\n> merge_detect_rename().\n\nAh, you're following the precedent set by\ndiff_rename_limit/merge_rename_limit; that makes sense. Thanks for\nthe explanation. I believe another possibility here is that for both\nthe {merge,diff}_rename_limit pair of variables and the\n{diff,merge}_renames pair of variables, since the code parses all\ninputs before ever using the result, we could calculate the result\nonce and store it rather than storing the constituent pieces of the\ncalculation. That would also prevent people from trying to use one of\nthe pieces of the calculation instead of treating it as a coherent\nwhole. However, while I would have preferred that the rename_limit\npair of variables also went away in favor of just one field which is\nupdated as it parses each input option, what you have is fine for this\nseries.\n"},{"id":"345942","messageId":"nycvar.QRO.7.76.6.1804270918430.72@tvgsbejvaqbjf.bet","threadId":"48335","inReplyTo":"CABPp-BERgc9EZ=hw4CepgXptO283mW3O30_pHrj4jtz3QSCFjQ@mail.gmail.com","subject":"Re: [PATCH v3 2/3] merge: Add merge.renames config setting","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-04-27T07:23:59Z","receivedAt":"2018-04-27T07:24:45Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 26 Apr 2018, Elijah Newren wrote:\n\n> On Thu, Apr 26, 2018 at 7:23 PM, Junio C Hamano wrote:\n> > Ben Peart writes:\n> >\n> >> Color me puzzled. :) The consensus was that the default value for\n> >> merge.renames come from diff.renames. diff.renames supports copy\n> >> detection which means that merge.renames will inherit that value. My\n> >> assumption was that is what was intended so when I reimplemented it, I\n> >> fully implemented it that way.\n> >>\n> >> Are you now requesting to only use diff.renames as the default if the\n> >> value is true or false but not if it is copy? What should happen if\n> >> diff.renames is actually set to copy? Should merge silently change\n> >> that to true, display a warning, error out, or something else? Do you\n> >> have some other behavior for how to handle copy being inherited from\n> >> diff.renames you'd like to see?\n> >>\n> >> Can you write the documentation that clearly explains the exact\n> >> behavior you want? That would kill two birds with one stone... :)\n> >\n> > I think demoting from copy to rename-only is a good idea, at least\n> > for now, because I do not believe we have figured out what we want\n> > to happen when we detect copied files are involved in a merge.\n> >\n> > But I am not sure if we even want to fail merge.renames=copy as an\n> > invalid configuration. So my gut feeling of the best solution to\n> > the above is to do something like:\n> >\n> > - whether the configuration comes from diff.renames or\n> > merge.renames, turn *.renames=copy to true inside the merge\n> > recursive machinery.\n> >\n> > - document the fact in \"git merge-recursive\" documentation (or \"git\n> > merge\" documentation) to say \"_currently_ asking for rename\n> > detection to find copies and renames will do the same\n> > thing---copies are ignored\", impliying \"this might change in the\n> > future\", in the BUGS section.\n> \n> Yes, I agree.\n\nGuys, you argued long and hard that one config setting (diff.renames)\nshould magically imply another one (merge.renames), on the basis that they\nessentially do the same.\n\nAnd now you are suggesting that they *cannot* be essentially the same? Are\nyou agreeing on such a violation of the Law of Least Surprise?\n\nPlease, make up your mind. Inheriting merge.renames from diff.renames is\n\"magic\" enough to be puzzling. Introducing that auto-demoting is just\nsimply creating a bad user experience. Please don't.\n\nIt is fine, of course, to admit at this stage that the whole magic idea of\nletting merge.renames inherit from diff.renames was only half thought\nthrough (we're in reviewing stage right now for the purpose of weighing\nalternative approaches and trying to come up with the best solution after\nall) and that we should go with Ben's original approach, which has the\nrather huge and dramatic advantage of being very, very clear and easy to\nunderstand to the user. We could even document that (and why) the 'copy'\nvalue in merge.renames is not allowed for the time being.\n\nCiao,\nDscho\n"},{"id":"345949","messageId":"CABPp-BEoY3bLQVNzfcxKqfXktdpfq0m7yaExM0f3-Ad+Mi0GBQ@mail.gmail.com","threadId":"48335","inReplyTo":"nycvar.QRO.7.76.6.1804270918430.72@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v3 2/3] merge: Add merge.renames config setting","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-04-27T14:32:56Z","receivedAt":"2018-04-27T14:33:03Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi Dsco,\n\nOn Fri, Apr 27, 2018 at 12:23 AM, Johannes Schindelin\n wrote:\n> Hi,\n\n>\n> Guys, you argued long and hard that one config setting (diff.renames)\n> should magically imply another one (merge.renames), on the basis that they\n> essentially do the same.\n\nI apologize for any frustration; I've probably caused a good deal of\nit by repeatedly catching or noticing things after one review and then\nbringing them up later. I just didn't catch it all at first.\n\nYour restatement of the basis of the argument doesn't match my\nrationale, though. My reasoning was that (1) the configuration\noptions exist to allow the user to specify how much of a performance\ncost they're willing to pay, (2) the two options are separate because\nsome users might want to configure one more loosely or tightly than\nthe other, but (3) many users will want to just specify once how much\nthey are willing to pay without being forced to repeat themselves for\ndifferent parts of git (diff, merge, possible future commands).\n\nI'll add to my former basis by stating that I think the diffstat at\nthe end of the merge should be controlled by these variables, even if\nthat's not implemented as part of Ben's series. And both of those\nconfiguration options clearly feed into whether that diffstat should\nbe doing rename or copy or no detection. While they could be kept\nseparate options, forcing us to somehow decide which one overrides\nwhich, I think it's much more natural if we already have merge.renames\ninheriting from and overriding diff.renames.\n\n> And now you are suggesting that they *cannot* be essentially the same? Are\n> you agreeing on such a violation of the Law of Least Surprise?\n>\n> Please, make up your mind. Inheriting merge.renames from diff.renames is\n> \"magic\" enough to be puzzling. Introducing that auto-demoting is just\n> simply creating a bad user experience. Please don't.\n\nFrom my view, resolve and octopus merges auto-demote diff.renames and\nmerge.renames to false. In fact, they optimize the work involved in\nthat demotion by not even checking the value. I think demoting \"copy\"\nto \"true\" in the recursive merge machinery is no more surprising than\nthat is. You can find more details to this argument in the portion of\nmy email that you responded to but snipped out.\n\nBut to add to that argument, if auto-demotion is a violation of the\nLaw of Least Surprise, as you claim, then naming this option\nmerge.renames is also a violation of the Law of Least Surprise. You\nwould instead need to rename it to something like\nmerge.recursive.renames. (And then when a new merge strategy comes\nalong that also does renames, we'll need to add a merge.ort.renames.)\n\n> It is fine, of course, to admit at this stage that the whole magic idea of\n> letting merge.renames inherit from diff.renames was only half thought\n> through (we're in reviewing stage right now for the purpose of weighing\n> alternative approaches and trying to come up with the best solution after\n> all) and that we should go with Ben's original approach, which has the\n> rather huge and dramatic advantage of being very, very clear and easy to\n> understand to the user. We could even document that (and why) the 'copy'\n> value in merge.renames is not allowed for the time being.\n\nIt was only half thought through, yes, at least by me. But the more I\nthink through it, the more I like the inheritance personally. I see\nno problem with the demotion and think the inheritance has the\nadvantage of being easier to understand, because I see your proposal\nas causing questions like:\n - \"Why does merge.renameLimit inherit from diff.renameLimit but\nmerge.renames doesn't from diff.renames?\"\n - \"Why can't I just specify in one place that I {am, am not} willing\nto pay for {full, both copy and} rename detection wherever it makes\nsense?\"\n - \"How do I control the detection for the diffstat at the end of the merge?\"\n\n\nHope that helps,\nElijah\n"},{"id":"345970","messageId":"20180427181937.7607-1-newren@palantir.com","threadId":"48335","inReplyTo":"7de8f144-8a37-e471-48e8-0b6f17a7bf29@gmail.com","subject":"","fromName":"Elijah Newren","fromEmail":"newren@palantir.com","sentAt":"2018-04-27T18:19:37Z","receivedAt":"2018-04-27T18:34:48Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nOn Thu, Apr 26, 2018 at 5:54 PM, Ben Peart wrote:\n\n> Can you write the documentation that clearly explains the exact behavior you\n> want? That would kill two birds with one stone... :)\n\nSure, something like the following is what I envision, and I've tried to\ninclude the suggestion from Junio to document the copy behavior in the\nmerge-recursive documentation.\n\n-- 8< --\nSubject: [PATCH] fixup! merge: Add merge.renames config setting\n\n---\n Documentation/merge-config.txt | 3 +--\n Documentation/merge-strategies.txt | 5 +++--\n merge-recursive.c | 8 ++++++++\n 3 files changed, 12 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt\nindex 59848e5634..662c2713ca 100644\n--- a/Documentation/merge-config.txt\n+++ b/Documentation/merge-config.txt\n@@ -41,8 +41,7 @@ merge.renameLimit::\n merge.renames::\n \tWhether and how Git detects renames. If set to \"false\",\n \trename detection is disabled. If set to \"true\", basic rename\n-\tdetection is enabled. If set to \"copies\" or \"copy\", Git will\n-\tdetect copies, as well. Defaults to the value of diff.renames.\n+\tdetection is enabled. Defaults to the value of diff.renames.\n \n merge.renormalize::\n \tTell Git that canonical representation of files in the\ndiff --git a/Documentation/merge-strategies.txt b/Documentation/merge-strategies.txt\nindex 1e0728aa12..aa66cbe41e 100644\n--- a/Documentation/merge-strategies.txt\n+++ b/Documentation/merge-strategies.txt\n@@ -23,8 +23,9 @@ recursive::\n \tcausing mismerges by tests done on actual merge commits\n \ttaken from Linux 2.6 kernel development history.\n \tAdditionally this can detect and handle merges involving\n-\trenames. This is the default merge strategy when\n-\tpulling or merging one branch.\n+\trenames, but currently cannot make use of detected\n+\tcopies. This is the default merge strategy when pulling\n+\tor merging one branch.\n +\n The 'recursive' strategy can take the following options:\n \ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 6cc4404144..b618f134d2 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -564,6 +564,14 @@ static struct string_list *get_renames(struct merge_options *o,\n \topts.flags.recursive = 1;\n \topts.flags.rename_empty = 0;\n \topts.detect_rename = merge_detect_rename(o);\n+\t/*\n+\t * We do not have logic to handle the detection of copies. In\n+\t * fact, it may not even make sense to add such logic: would we\n+\t * really want a change to a base file to be propagated through\n+\t * multiple other files by a merge?\n+\t */\n+\tif (opts.detect_rename > DIFF_DETECT_RENAME)\n+\t\topts.detect_rename = DIFF_DETECT_RENAME;\n \topts.rename_limit = o->merge_rename_limit >= 0 ? o->merge_rename_limit :\n \t\t\t o->diff_rename_limit >= 0 ? o->diff_rename_limit :\n \t\t\t 1000;\n-- \n2.17.0.294.g8e3b83c0e3\n\n"},{"id":"345971","messageId":"20180427183752.GA2799@esm","threadId":"48335","inReplyTo":"xmqqy3h9z8sj.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3 2/3] merge: Add merge.renames config setting","fromName":"Eckhard Maaß","fromEmail":"eckhard.s.maass@googlemail.com","sentAt":"2018-04-27T18:37:52Z","receivedAt":"2018-04-27T18:38:03Z","isPatch":true,"sender":{"key":"eckhard.s.maass@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/21984134?v=4"},"body":"On Fri, Apr 27, 2018 at 11:23:56AM +0900, Junio C Hamano wrote:\n> I think demoting from copy to rename-only is a good idea, at least\n> for now, because I do not believe we have figured out what we want\n> to happen when we detect copied files are involved in a merge.\n\nDoes anyone know some threads concerning that topic? I tried to search\nfor it, but somehow \"merge copy detection\" did not find me useful\nthreads so far. I would be interested in that topic anyway, so I would\nlike to know what the ideas are that floated so far.\n\nGreetings,\nEckhard\n"},{"id":"345985","messageId":"CABPp-BHwM1jx2+VTxt7hga7v-E6gvHuxVNPqm-MPRXYe5CDVtA@mail.gmail.com","threadId":"48335","inReplyTo":"20180427183752.GA2799@esm","subject":"Re: [PATCH v3 2/3] merge: Add merge.renames config setting","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-04-27T20:23:20Z","receivedAt":"2018-04-27T20:23:25Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Apr 27, 2018 at 11:37 AM, Eckhard Maaß\n wrote:\n> On Fri, Apr 27, 2018 at 11:23:56AM +0900, Junio C Hamano wrote:\n>> I think demoting from copy to rename-only is a good idea, at least\n>> for now, because I do not believe we have figured out what we want\n>> to happen when we detect copied files are involved in a merge.\n>\n> Does anyone know some threads concerning that topic? I tried to search\n> for it, but somehow \"merge copy detection\" did not find me useful\n> threads so far. I would be interested in that topic anyway, so I would\n> like to know what the ideas are that floated so far.\n>\n\nI doubt it has ever been discussed before this thread. But, if you're\ncurious, I'll try to dump a few thoughts. Let's say we have branches\nA and B, and:\n A: modifies file z\n B: copies z to y\n\nShould the modifications to z done in A propagate to both z and y? If\nnot, what good is copy detection? If so, then there are several\nramifications...\n\n- If B not only copied z but also first modified it, then do we have\n potential conflicts with both z and y -- possibly the exact same\n conflicts, making the user resolve them repeatedly?\n\n- What if A copied z to x? Do changes to z propagate to all three of\n z and x and y? Do changes to either x or y affect z? Do they\n affect each other?\n\n- If A deleted z, does that give us a copy/delete conflict for y? Do\n we also have to worry about copy/add conflicts? copy/add/delete?\n rename/copy (multiple variants)? copy/copy?\n\n- Extra degrees of freedom may mean new conflict types:\n\n - The extra degrees of freedom from renames introduced multiple new\n conflict types (e.g. rename/add, rename/rename(1to2),\n rename/rename(2to1)).\n\n - Directory rename detection added more (rename/rename/rename(1to3),\n rename/rename(Nto1), add/add/add, directory/file/file, n-fold\n transitive rename, etc.), -- and forced us to specify various\n rules to limit the possibility space so that conflicts could be\n representable in the index and understandable by the user.\n\n - I suspect adding copies would add new types of conflicts we\n haven't thought of yet.\n\nThe more I think about it, the more I think that attempting to detect\ncopies in a merge algorithm just doesn't make sense. Anything I can\nthink of that someone might attempt to use detected copies for would\njust surprise users in a bad way...and it'd open up a big can of edge\nand corner case problems as well.\n"},{"id":"346126","messageId":"20180430080341.GA28348@esm","threadId":"48335","inReplyTo":"CABPp-BHwM1jx2+VTxt7hga7v-E6gvHuxVNPqm-MPRXYe5CDVtA@mail.gmail.com","subject":"Re: [PATCH v3 2/3] merge: Add merge.renames config setting","fromName":"Eckhard Maaß","fromEmail":"eckhard.s.maass@googlemail.com","sentAt":"2018-04-30T08:03:41Z","receivedAt":"2018-04-30T08:03:48Z","isPatch":true,"sender":{"key":"eckhard.s.maass@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/21984134?v=4"},"body":"On Fri, Apr 27, 2018 at 01:23:20PM -0700, Elijah Newren wrote:\n> I doubt it has ever been discussed before this thread. But, if you're\n> curious, I'll try to dump a few thoughts.\n\nThank you, I try to dump some of mine, too. Maybe let me first stress\nthat for me copy detection without --find-copies-harder is much more a\n\"find content extracted\" (like methods being factored out). In a way\nthis is nearer to a rename than to a real copy.\n\n\n> [...] Let's say we have branches\n> A and B, and:\n> A: modifies file z\n> B: copies z to y\n> \n> Should the modifications to z done in A propagate to both z and y? If\n> not, what good is copy detection? If so, then there are several\n> ramifications...\n\nIf one just assumes the most likely outcome is that something from z wad\nfactored out to y, it might just be sufficient to see whether the\nmodifications of the two branches apply cleanly - if A touched the parts\nof B that have been factored out there would be a normal merge conflict\n(where one could be nice and give a hint that some content was copied to\ny on the B branch), if A did not touched the parts touched (or moved) by\nB, then there is no problem. If A exactly deleted the content moved by\nB, there will be no conflict - but this is seems to be strange anyway.\n\nI admit that a \"real\" copy would get unnoticed that way. But the\nsemantics of such a copy isn't too clear for me either - did I copy the\nother part to make it independent of the other or did I just employ a\ncopy and paste tactic? The former does not want the changes, the later\ndoes. But I am happy catering to the former here.\n\nTo sum up:\n- fail as before for conflicting merges, but give a hint that one has\n copied to quicken up resolution.\n\n\n> - If B not only copied z but also first modified it, then do we have\n> potential conflicts with both z and y -- possibly the exact same\n> conflicts, making the user resolve them repeatedly?\n\nWith the above suggestion, if there are conflicts, you fail and give a\nhint.\n\n> - What if A copied z to x? Do changes to z propagate to all three of\n> z and x and y? Do changes to either x or y affect z? Do they\n> affect each other?\n\nA copy on branch to x and one another to y seems strange even if z\nmerges cleanly. Did both sides try to factor the same thing out to\ndifferent files? Or did they try to make something independent, but\nmanaged to make it to different files? For this I would be inclined to\njust suggest fail with a copy/copy(somewhere else). But this is a real\ncorner case after all. Has anyone seen just thing in practice?\n\n> - If A deleted z, does that give us a copy/delete conflict for y? Do\n> we also have to worry about copy/add conflicts? copy/add/delete?\n> rename/copy (multiple variants)? copy/copy?\n\nWe do have the modified/deleted conflict where we could hint that\ncontent also has been copied and then not try to do more.\n\n> - Extra degrees of freedom may mean new conflict types:\n> \n> - The extra degrees of freedom from renames introduced multiple new\n> conflict types (e.g. rename/add, rename/rename(1to2),\n> rename/rename(2to1)).\n\nFor renaming one side and coping the other, I would think doing the same\nas above is sensible enough: if there are conflicts one can give an\nadditional hint of the one part having been copied, but not change the\nkind of conflicts much.\n\n> The more I think about it, the more I think that attempting to detect\n> copies in a merge algorithm just doesn't make sense. Anything I can\n> think of that someone might attempt to use detected copies for would\n> just surprise users in a bad way...\n\nHm, it didn't sound like that. Would you think that users would be\nsurprised by my suggestions? Or are they all too corner casey to be\nworth implementing anyway?\n\nGreetings,\nEckhard\n"},{"id":"346145","messageId":"e753d8fd-5329-b819-0076-0ff4659dabf1@gmail.com","threadId":"48335","inReplyTo":"20180427181937.7607-1-newren@palantir.com","subject":"Re:","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-04-30T13:11:40Z","receivedAt":"2018-04-30T13:11:44Z","isPatch":false,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 4/27/2018 2:19 PM, Elijah Newren wrote:\n> From: Elijah Newren \n> \n> On Thu, Apr 26, 2018 at 5:54 PM, Ben Peart wrote:\n> \n>> Can you write the documentation that clearly explains the exact behavior you\n>> want? That would kill two birds with one stone... :)\n> \n> Sure, something like the following is what I envision, and I've tried to\n> include the suggestion from Junio to document the copy behavior in the\n> merge-recursive documentation.\n> \n> -- 8< --\n> Subject: [PATCH] fixup! merge: Add merge.renames config setting\n> \n> ---\n> Documentation/merge-config.txt | 3 +--\n> Documentation/merge-strategies.txt | 5 +++--\n> merge-recursive.c | 8 ++++++++\n> 3 files changed, 12 insertions(+), 4 deletions(-)\n> \n> diff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt\n> index 59848e5634..662c2713ca 100644\n> --- a/Documentation/merge-config.txt\n> +++ b/Documentation/merge-config.txt\n> @@ -41,8 +41,7 @@ merge.renameLimit::\n> merge.renames::\n> \tWhether and how Git detects renames. If set to \"false\",\n> \trename detection is disabled. If set to \"true\", basic rename\n> -\tdetection is enabled. If set to \"copies\" or \"copy\", Git will\n> -\tdetect copies, as well. Defaults to the value of diff.renames.\n> +\tdetection is enabled. Defaults to the value of diff.renames.\n> \n> merge.renormalize::\n> \tTell Git that canonical representation of files in the\n> diff --git a/Documentation/merge-strategies.txt b/Documentation/merge-strategies.txt\n> index 1e0728aa12..aa66cbe41e 100644\n> --- a/Documentation/merge-strategies.txt\n> +++ b/Documentation/merge-strategies.txt\n> @@ -23,8 +23,9 @@ recursive::\n> \tcausing mismerges by tests done on actual merge commits\n> \ttaken from Linux 2.6 kernel development history.\n> \tAdditionally this can detect and handle merges involving\n> -\trenames. This is the default merge strategy when\n> -\tpulling or merging one branch.\n> +\trenames, but currently cannot make use of detected\n> +\tcopies. This is the default merge strategy when pulling\n> +\tor merging one branch.\n> +\n> The 'recursive' strategy can take the following options:\n> \n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index 6cc4404144..b618f134d2 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -564,6 +564,14 @@ static struct string_list *get_renames(struct merge_options *o,\n> \topts.flags.recursive = 1;\n> \topts.flags.rename_empty = 0;\n> \topts.detect_rename = merge_detect_rename(o);\n> +\t/*\n> +\t * We do not have logic to handle the detection of copies. In\n> +\t * fact, it may not even make sense to add such logic: would we\n> +\t * really want a change to a base file to be propagated through\n> +\t * multiple other files by a merge?\n> +\t */\n> +\tif (opts.detect_rename > DIFF_DETECT_RENAME)\n> +\t\topts.detect_rename = DIFF_DETECT_RENAME;\n> \topts.rename_limit = o->merge_rename_limit >= 0 ? o->merge_rename_limit :\n> \t\t\t o->diff_rename_limit >= 0 ? o->diff_rename_limit :\n> \t\t\t 1000;\n> \n\nThanks Elijah. I've applied this patch and reviewed and tested it. It \nworks and addresses the concerns around the settings inheritance from \ndiff.renames. I still _prefer_ the simpler model that doesn't do the \npartial inheritance but I can use this model as well.\n\nI'm unsure on the protocol here. Should I incorporate this patch and \nsubmit a reroll or can it just be applied as is?\n"},{"id":"346172","messageId":"CABPp-BEC2cnpdvDsMPFodvNR06G5E434Hpdmaex+6+zHpYm_QQ@mail.gmail.com","threadId":"48335","inReplyTo":"e753d8fd-5329-b819-0076-0ff4659dabf1@gmail.com","subject":"Re:","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-04-30T16:12:35Z","receivedAt":"2018-04-30T16:12:40Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Apr 30, 2018 at 6:11 AM, Ben Peart wrote:\n> On 4/27/2018 2:19 PM, Elijah Newren wrote:\n>>\n>> From: Elijah Newren \n>>\n>> On Thu, Apr 26, 2018 at 5:54 PM, Ben Peart wrote:\n>>\n>>> Can you write the documentation that clearly explains the exact behavior\n>>> you\n>>> want? That would kill two birds with one stone... :)\n>>\n>>\n>> Sure, something like the following is what I envision, and I've tried to\n>> include the suggestion from Junio to document the copy behavior in the\n>> merge-recursive documentation.\n>>\n\n>\n> Thanks Elijah. I've applied this patch and reviewed and tested it. It works\n> and addresses the concerns around the settings inheritance from\n> diff.renames. I still _prefer_ the simpler model that doesn't do the\n> partial inheritance but I can use this model as well.\n>\n> I'm unsure on the protocol here. Should I incorporate this patch and submit\n> a reroll or can it just be applied as is?\n\nI suspect you'll want to re-roll anyway, to base your series on\nen/rename-directory-detection-reboot instead of on master. (Junio\nplans to merge it down to next, and your series has four different\nmerge conflicts with it.)\n\nThere are two other loose ends with this series that Junio will need\nto weigh in on:\n\n- I'm obviously a strong proponent of the inherited setting, but Junio\nmay change his mind after reading Dscho's arguments against it (or\nafter reading my arguments for it).\n\n- I like the setting as-is, and think we could allow a \"copy\" setting\nfor merge.renames to specify that the post-merge diffstat should\ndetect copies (not part of your series, but a useful addition I'd like\nto tackle afterwards). However, Junio had comments in\nxmqqwox19ohw.fsf@gitster-ct.c.googlers.com about merge.renames\nhandling the scoring as well, like -Xfind-renames. Those sound\nincompatible to me for a single setting, and I'm unsure if Junio would\nresolve them the way I do or still feels strongly about the scoring.\n"},{"id":"346178","messageId":"CABPp-BF3_gkuyS+9QOjoKHidKp=S6LzzyF7MJ06fKah6hE=pDw@mail.gmail.com","threadId":"48335","inReplyTo":"20180430080341.GA28348@esm","subject":"Re: [PATCH v3 2/3] merge: Add merge.renames config setting","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-04-30T16:54:10Z","receivedAt":"2018-04-30T16:54:15Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi Eckhard,\n\nOn Mon, Apr 30, 2018 at 1:03 AM, Eckhard Maaß\n wrote:\n> On Fri, Apr 27, 2018 at 01:23:20PM -0700, Elijah Newren wrote:\n>> I doubt it has ever been discussed before this thread. But, if you're\n>> curious, I'll try to dump a few thoughts.\n>\n> Thank you, I try to dump some of mine, too. Maybe let me first stress\n> that for me copy detection without --find-copies-harder is much more a\n> \"find content extracted\" (like methods being factored out). In a way\n> this is nearer to a rename than to a real copy.\n\nOoh, if you wanted to detect movement of code between files (as blame\ndoes, I think searches for PICKAXE_BLAME_MOVE would point you in the\nright direction) and then try to use that during merge to allow\napplied changes to move with the code, that would be awesome.\nExpensive, and might be a lot of work to wire it all up, but it'd be\nvery interesting. I was only discussing DIFF_DETECT_COPY in my\nprevious email, which was all about duplicating entire files; that's\nsomething I don't see utility in right now for resolving merges.\n\n\n> I admit that a \"real\" copy would get unnoticed that way. But the\n> semantics of such a copy isn't too clear for me either - did I copy the\n> other part to make it independent of the other or did I just employ a\n> copy and paste tactic? The former does not want the changes, the later\n> does. But I am happy catering to the former here.\n\nRight, if you have to assume that the copy was made to make the code\nindependent, then there's no value for merge resolution to having\ndetected the copy in the first place. That has the advantage of\nside-stepping the possible new edge and corner cases I mentioned in\nthe rest of my email, but it means we shouldn't even spend time\ndetecting copies -- whether whole file (via DIFF_DETECT_COPY) or\nindividual lines (via PICKAXE_BLAME_COPY and variants).\n\n\nElijah\n"},{"id":"346454","messageId":"c5c262f7-c24e-46bf-e9c8-24b322543711@gmail.com","threadId":"48335","inReplyTo":"CABPp-BEC2cnpdvDsMPFodvNR06G5E434Hpdmaex+6+zHpYm_QQ@mail.gmail.com","subject":"Re:","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-05-02T14:33:09Z","receivedAt":"2018-05-02T14:33:13Z","isPatch":false,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 4/30/2018 12:12 PM, Elijah Newren wrote:\n> On Mon, Apr 30, 2018 at 6:11 AM, Ben Peart wrote:\n>> On 4/27/2018 2:19 PM, Elijah Newren wrote:\n>>>\n>>> From: Elijah Newren \n>>>\n>>> On Thu, Apr 26, 2018 at 5:54 PM, Ben Peart wrote:\n>>>\n>>>> Can you write the documentation that clearly explains the exact behavior\n>>>> you\n>>>> want? That would kill two birds with one stone... :)\n>>>\n>>>\n>>> Sure, something like the following is what I envision, and I've tried to\n>>> include the suggestion from Junio to document the copy behavior in the\n>>> merge-recursive documentation.\n>>>\n> \n>>\n>> Thanks Elijah. I've applied this patch and reviewed and tested it. It works\n>> and addresses the concerns around the settings inheritance from\n>> diff.renames. I still _prefer_ the simpler model that doesn't do the\n>> partial inheritance but I can use this model as well.\n>>\n>> I'm unsure on the protocol here. Should I incorporate this patch and submit\n>> a reroll or can it just be applied as is?\n> \n> I suspect you'll want to re-roll anyway, to base your series on\n> en/rename-directory-detection-reboot instead of on master. (Junio\n> plans to merge it down to next, and your series has four different\n> merge conflicts with it.)\n> \n> There are two other loose ends with this series that Junio will need\n> to weigh in on:\n> \n> - I'm obviously a strong proponent of the inherited setting, but Junio\n> may change his mind after reading Dscho's arguments against it (or\n> after reading my arguments for it).\n> \n> - I like the setting as-is, and think we could allow a \"copy\" setting\n> for merge.renames to specify that the post-merge diffstat should\n> detect copies (not part of your series, but a useful addition I'd like\n> to tackle afterwards). However, Junio had comments in\n> xmqqwox19ohw.fsf@gitster-ct.c.googlers.com about merge.renames\n> handling the scoring as well, like -Xfind-renames. Those sound\n> incompatible to me for a single setting, and I'm unsure if Junio would\n> resolve them the way I do or still feels strongly about the scoring.\n> \n\nI think this patch series (including Elijah's fixup!) improves the \nsituation from where we were and it provides the necessary functionality \nto solve the problem I started out to solve. While there are other \nchanges that could be made, I think they should be done in separate \nfollow up patches.\n\nI'm happy to reroll this incorporating the fixup! so that we can make \nprogress. Junio, would you prefer I reroll this based on \nen/rename-directory-detection-reboot or master?\n"},{"id":"346461","messageId":"20180502160056.5836-1-benpeart@microsoft.com","threadId":"48335","inReplyTo":"20180420133632.17580-1-benpeart@microsoft.com","subject":"[PATCH v4 0/3] add additional config settings for merge","fromName":"Ben Peart","fromEmail":"ben.peart@microsoft.com","sentAt":"2018-05-02T16:01:11Z","receivedAt":"2018-05-02T16:01:17Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"This version incorporates Elijah's fixup patch and is now based on\nen/rename-directory-detection in next.\n\nBase Ref: en/rename-directory-detection\nWeb-Diff: https://github.com/benpeart/git/commit/16b175c25f\nCheckout: git fetch https://github.com/benpeart/git merge-renames-v4 && git checkout 16b175c25f\n\n### Patches\n\nBen Peart (3):\n merge: update documentation for {merge,diff}.renameLimit\n merge: Add merge.renames config setting\n merge: pass aggressive when rename detection is turned off\n\n Documentation/diff-config.txt | 3 ++-\n Documentation/merge-config.txt | 8 +++++-\n Documentation/merge-strategies.txt | 11 +++++---\n diff.c | 2 +-\n diff.h | 1 +\n merge-recursive.c | 31 ++++++++++++++++++-----\n merge-recursive.h | 8 +++++-\n t/t3034-merge-recursive-rename-options.sh | 18 +++++++++++++\n 8 files changed, 68 insertions(+), 14 deletions(-)\n\n\nbase-commit: c5b761fb2711542073cf1906c0e86a34616b79ae\n-- \n2.17.0.windows.1\n\n\n"},{"id":"346462","messageId":"20180502160056.5836-2-benpeart@microsoft.com","threadId":"48335","inReplyTo":"20180502160056.5836-1-benpeart@microsoft.com","subject":"[PATCH v4 1/3] merge: update documentation for {merge,diff}.renameLimit","fromName":"Ben Peart","fromEmail":"ben.peart@microsoft.com","sentAt":"2018-05-02T16:01:13Z","receivedAt":"2018-05-02T16:01:20Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"Update the documentation to better indicate that the renameLimit setting is\nignored if rename detection is turned off via command line options or config\nsettings.\n\nSigned-off-by: Ben Peart \n---\n Documentation/diff-config.txt | 3 ++-\n Documentation/merge-config.txt | 3 ++-\n 2 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt\nindex 5ca942ab5e..77caa66c2f 100644\n--- a/Documentation/diff-config.txt\n+++ b/Documentation/diff-config.txt\n@@ -112,7 +112,8 @@ diff.orderFile::\n \n diff.renameLimit::\n \tThe number of files to consider when performing the copy/rename\n-\tdetection; equivalent to the 'git diff' option `-l`.\n+\tdetection; equivalent to the 'git diff' option `-l`. This setting\n+\thas no effect if rename detection is turned off.\n \n diff.renames::\n \tWhether and how Git detects renames. If set to \"false\",\ndiff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt\nindex 12b6bbf591..48ee3bce77 100644\n--- a/Documentation/merge-config.txt\n+++ b/Documentation/merge-config.txt\n@@ -35,7 +35,8 @@ include::fmt-merge-msg-config.txt[]\n merge.renameLimit::\n \tThe number of files to consider when performing rename detection\n \tduring a merge; if not specified, defaults to the value of\n-\tdiff.renameLimit.\n+\tdiff.renameLimit. This setting has no effect if rename detection\n+\tis turned off.\n \n merge.renormalize::\n \tTell Git that canonical representation of files in the\n-- \n2.17.0.windows.1\n\n"},{"id":"346463","messageId":"20180502160056.5836-3-benpeart@microsoft.com","threadId":"48335","inReplyTo":"20180502160056.5836-1-benpeart@microsoft.com","subject":"[PATCH v4 2/3] merge: Add merge.renames config setting","fromName":"Ben Peart","fromEmail":"ben.peart@microsoft.com","sentAt":"2018-05-02T16:01:14Z","receivedAt":"2018-05-02T16:01:24Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"Add the ability to control rename detection for merge via a config setting.\nThis setting behaves the same and defaults to the value of diff.renames but only\napplies to merge.\n\nReviewed-by: Johannes Schindelin \nHelped-by: Elijah Newren \nSigned-off-by: Ben Peart \n---\n Documentation/merge-config.txt | 5 ++++\n Documentation/merge-strategies.txt | 11 ++++++---\n diff.c | 2 +-\n diff.h | 1 +\n merge-recursive.c | 30 ++++++++++++++++++-----\n merge-recursive.h | 8 +++++-\n t/t3034-merge-recursive-rename-options.sh | 18 ++++++++++++++\n 7 files changed, 63 insertions(+), 12 deletions(-)\n\ndiff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt\nindex 48ee3bce77..662c2713ca 100644\n--- a/Documentation/merge-config.txt\n+++ b/Documentation/merge-config.txt\n@@ -38,6 +38,11 @@ merge.renameLimit::\n \tdiff.renameLimit. This setting has no effect if rename detection\n \tis turned off.\n \n+merge.renames::\n+\tWhether and how Git detects renames. If set to \"false\",\n+\trename detection is disabled. If set to \"true\", basic rename\n+\tdetection is enabled. Defaults to the value of diff.renames.\n+\n merge.renormalize::\n \tTell Git that canonical representation of files in the\n \trepository has changed over time (e.g. earlier commits record\ndiff --git a/Documentation/merge-strategies.txt b/Documentation/merge-strategies.txt\nindex fd5d748d1b..30acc99232 100644\n--- a/Documentation/merge-strategies.txt\n+++ b/Documentation/merge-strategies.txt\n@@ -23,8 +23,9 @@ recursive::\n \tcausing mismerges by tests done on actual merge commits\n \ttaken from Linux 2.6 kernel development history.\n \tAdditionally this can detect and handle merges involving\n-\trenames. This is the default merge strategy when\n-\tpulling or merging one branch.\n+\trenames, but currently cannot make use of detected\n+\tcopies. This is the default merge strategy when pulling\n+\tor merging one branch.\n +\n The 'recursive' strategy can take the following options:\n \n@@ -84,12 +85,14 @@ no-renormalize;;\n \t`merge.renormalize` configuration variable.\n \n no-renames;;\n-\tTurn off rename detection.\n+\tTurn off rename detection. This overrides the `merge.renames`\n+\tconfiguration variable.\n \tSee also linkgit:git-diff[1] `--no-renames`.\n \n find-renames[=];;\n \tTurn on rename detection, optionally setting the similarity\n-\tthreshold. This is the default.\n+\tthreshold. This is the default. This overrides the\n+\t'merge.renames' configuration variable.\n \tSee also linkgit:git-diff[1] `--find-renames`.\n \n rename-threshold=;;\ndiff --git a/diff.c b/diff.c\nindex fb22b19f09..e744b35cdc 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -177,7 +177,7 @@ static int parse_submodule_params(struct diff_options *options, const char *valu\n \treturn 0;\n }\n \n-static int git_config_rename(const char *var, const char *value)\n+int git_config_rename(const char *var, const char *value)\n {\n \tif (!value)\n \t\treturn DIFF_DETECT_RENAME;\ndiff --git a/diff.h b/diff.h\nindex 7cf276f077..966fc8fce6 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -321,6 +321,7 @@ extern int git_diff_ui_config(const char *var, const char *value, void *cb);\n extern void diff_setup(struct diff_options *);\n extern int diff_opt_parse(struct diff_options *, const char **, int, const char *);\n extern void diff_setup_done(struct diff_options *);\n+extern int git_config_rename(const char *var, const char *value);\n \n #define DIFF_DETECT_RENAME\t1\n #define DIFF_DETECT_COPY\t2\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 5f42c677d5..372ffbbacc 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1524,7 +1524,15 @@ static struct diff_queue_struct *get_diffpairs(struct merge_options *o,\n \tdiff_setup(&opts);\n \topts.flags.recursive = 1;\n \topts.flags.rename_empty = 0;\n-\topts.detect_rename = DIFF_DETECT_RENAME;\n+\topts.detect_rename = merge_detect_rename(o);\n+\t/*\n+\t * We do not have logic to handle the detection of copies. In\n+\t * fact, it may not even make sense to add such logic: would we\n+\t * really want a change to a base file to be propagated through\n+\t * multiple other files by a merge?\n+\t */\n+\tif (opts.detect_rename > DIFF_DETECT_RENAME)\n+\t\topts.detect_rename = DIFF_DETECT_RENAME;\n \topts.rename_limit = o->merge_rename_limit >= 0 ? o->merge_rename_limit :\n \t\t\t o->diff_rename_limit >= 0 ? o->diff_rename_limit :\n \t\t\t 1000;\n@@ -2564,7 +2572,7 @@ static int handle_renames(struct merge_options *o,\n \tri->head_renames = NULL;\n \tri->merge_renames = NULL;\n \n-\tif (!o->detect_rename)\n+\tif (!merge_detect_rename(o))\n \t\treturn 1;\n \n \thead_pairs = get_diffpairs(o, common, head);\n@@ -3241,9 +3249,18 @@ int merge_recursive_generic(struct merge_options *o,\n \n static void merge_recursive_config(struct merge_options *o)\n {\n+\tchar *value = NULL;\n \tgit_config_get_int(\"merge.verbosity\", &o->verbosity);\n \tgit_config_get_int(\"diff.renamelimit\", &o->diff_rename_limit);\n \tgit_config_get_int(\"merge.renamelimit\", &o->merge_rename_limit);\n+\tif (!git_config_get_string(\"diff.renames\", &value)) {\n+\t\to->diff_detect_rename = git_config_rename(\"diff.renames\", value);\n+\t\tfree(value);\n+\t}\n+\tif (!git_config_get_string(\"merge.renames\", &value)) {\n+\t\to->merge_detect_rename = git_config_rename(\"merge.renames\", value);\n+\t\tfree(value);\n+\t}\n \tgit_config(git_xmerge_config, NULL);\n }\n \n@@ -3256,7 +3273,8 @@ void init_merge_options(struct merge_options *o)\n \to->diff_rename_limit = -1;\n \to->merge_rename_limit = -1;\n \to->renormalize = 0;\n-\to->detect_rename = 1;\n+\to->diff_detect_rename = -1;\n+\to->merge_detect_rename = -1;\n \tmerge_recursive_config(o);\n \tmerge_verbosity = getenv(\"GIT_MERGE_VERBOSITY\");\n \tif (merge_verbosity)\n@@ -3307,16 +3325,16 @@ int parse_merge_opt(struct merge_options *o, const char *s)\n \telse if (!strcmp(s, \"no-renormalize\"))\n \t\to->renormalize = 0;\n \telse if (!strcmp(s, \"no-renames\"))\n-\t\to->detect_rename = 0;\n+\t\to->merge_detect_rename = 0;\n \telse if (!strcmp(s, \"find-renames\")) {\n-\t\to->detect_rename = 1;\n+\t\to->merge_detect_rename = 1;\n \t\to->rename_score = 0;\n \t}\n \telse if (skip_prefix(s, \"find-renames=\", &arg) ||\n \t\t skip_prefix(s, \"rename-threshold=\", &arg)) {\n \t\tif ((o->rename_score = parse_rename_score(&arg)) == -1 || *arg != 0)\n \t\t\treturn -1;\n-\t\to->detect_rename = 1;\n+\t\to->merge_detect_rename = 1;\n \t}\n \telse\n \t\treturn -1;\ndiff --git a/merge-recursive.h b/merge-recursive.h\nindex d863cf8867..c1d9b5b3d9 100644\n--- a/merge-recursive.h\n+++ b/merge-recursive.h\n@@ -18,7 +18,8 @@ struct merge_options {\n \tunsigned renormalize : 1;\n \tlong xdl_opts;\n \tint verbosity;\n-\tint detect_rename;\n+\tint diff_detect_rename;\n+\tint merge_detect_rename;\n \tint diff_rename_limit;\n \tint merge_rename_limit;\n \tint rename_score;\n@@ -55,6 +56,11 @@ struct collision_entry {\n \tstruct string_list source_files;\n \tunsigned reported_already:1;\n };\n+inline int merge_detect_rename(struct merge_options *o)\n+{\n+\treturn o->merge_detect_rename >= 0 ? o->merge_detect_rename :\n+\t\to->diff_detect_rename >= 0 ? o->diff_detect_rename : 1;\n+}\n \n /* merge_trees() but with recursive ancestor consolidation */\n int merge_recursive(struct merge_options *o,\ndiff --git a/t/t3034-merge-recursive-rename-options.sh b/t/t3034-merge-recursive-rename-options.sh\nindex b9c4028496..3d9fae68c4 100755\n--- a/t/t3034-merge-recursive-rename-options.sh\n+++ b/t/t3034-merge-recursive-rename-options.sh\n@@ -309,4 +309,22 @@ test_expect_success 'last wins in --find-renames= --rename-threshold=' '\n \tcheck_threshold_0\n '\n \n+test_expect_success 'merge.renames disables rename detection' '\n+\tgit read-tree --reset -u HEAD &&\n+\tgit -c merge.renames=false merge-recursive $tail &&\n+\tcheck_no_renames\n+'\n+\n+test_expect_success 'merge.renames defaults to diff.renames' '\n+\tgit read-tree --reset -u HEAD &&\n+\tgit -c diff.renames=false merge-recursive $tail &&\n+\tcheck_no_renames\n+'\n+\n+test_expect_success 'merge.renames overrides diff.renames' '\n+\tgit read-tree --reset -u HEAD &&\n+\ttest_must_fail git -c diff.renames=false -c merge.renames=true merge-recursive $tail &&\n+\t$check_50\n+'\n+\n test_done\n-- \n2.17.0.windows.1\n\n"},{"id":"346464","messageId":"20180502160056.5836-4-benpeart@microsoft.com","threadId":"48335","inReplyTo":"20180502160056.5836-1-benpeart@microsoft.com","subject":"[PATCH v4 3/3] merge: pass aggressive when rename detection is turned off","fromName":"Ben Peart","fromEmail":"ben.peart@microsoft.com","sentAt":"2018-05-02T16:01:16Z","receivedAt":"2018-05-02T16:01:27Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"Set aggressive flag in git_merge_trees() when rename detection is turned off.\nThis allows read_tree() to auto resolve more cases that would have otherwise\nbeen handled by the rename detection.\n\nReviewed-by: Johannes Schindelin \nSigned-off-by: Ben Peart \n---\n merge-recursive.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 372ffbbacc..cea054cfd4 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -355,6 +355,7 @@ static int git_merge_trees(struct merge_options *o,\n \to->unpack_opts.fn = threeway_merge;\n \to->unpack_opts.src_index = &the_index;\n \to->unpack_opts.dst_index = &the_index;\n+\to->unpack_opts.aggressive = !merge_detect_rename(o);\n \tsetup_unpack_trees_porcelain(&o->unpack_opts, \"merge\");\n \n \tinit_tree_desc_from_tree(t+0, common);\n-- \n2.17.0.windows.1\n\n"},{"id":"346468","messageId":"CABPp-BEvJdLt-GvGYCo-XrafHDDTm0Sb5AWfrbffqo8vF=8Y0Q@mail.gmail.com","threadId":"48335","inReplyTo":"20180502160056.5836-1-benpeart@microsoft.com","subject":"Re: [PATCH v4 0/3] add additional config settings for merge","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-05-02T17:20:07Z","receivedAt":"2018-05-02T17:20:11Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi Ben,\n\nThanks for your persistence.\n\nOn Wed, May 2, 2018 at 9:01 AM, Ben Peart wrote:\n> This version incorporates Elijah's fixup patch and is now based on\n> en/rename-directory-detection in next.\n\nen/rename-directory-detection was in master but reverted.\nen/rename-directory-detection-reboot is the new series, and isn't\nquite in next yet; Junio just said in the last \"What's cooking\" that\nhe intends to merge it down. However, the two series are obviously\nsimilar, so your rebasing even onto the old one does cut down on the\nconflicts considerably. Rebasing this latest series onto\nen/rename-directory-detection-reboot leaves just one minor conflict.\n\nAnyway, I've looked over this latest re-roll and it looks good to me:\nReviewed-by: Elijah Newren \n"},{"id":"346618","messageId":"xmqqefist8xr.fsf@gitster-ct.c.googlers.com","threadId":"48335","inReplyTo":"20180502160056.5836-3-benpeart@microsoft.com","subject":"Re: [PATCH v4 2/3] merge: Add merge.renames config setting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-05-04T03:07:44Z","receivedAt":"2018-05-04T03:07:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ben Peart writes:\n\nI'd downcase the verb on the subject.\n\n> Add the ability to control rename detection for merge via a config setting.\n> This setting behaves the same and defaults to the value of diff.renames but only\n> applies to merge.\n>\n> Reviewed-by: Johannes Schindelin \n> Helped-by: Elijah Newren \n> Signed-off-by: Ben Peart \n> ...\n> diff --git a/merge-recursive.h b/merge-recursive.h\n> index d863cf8867..c1d9b5b3d9 100644\n> --- a/merge-recursive.h\n> +++ b/merge-recursive.h\n> @@ -55,6 +56,11 @@ struct collision_entry {\n> \tstruct string_list source_files;\n> \tunsigned reported_already:1;\n> };\n> +inline int merge_detect_rename(struct merge_options *o)\n> +{\n> +\treturn o->merge_detect_rename >= 0 ? o->merge_detect_rename :\n> +\t\to->diff_detect_rename >= 0 ? o->diff_detect_rename : 1;\n> +}\n\nI'll tweak the above to leave a blank before the function, and make\nit \"static inline\", to ensure that the output from\n\n $ git grep -e '\\' --and --not -e 'static inline' -- \\*.h\n\nis empty.\n"}]}