{"thread":{"id":"41420","subject":"[PATCH] merge-recursive: option to disable renames","startedAt":"2016-02-16T00:50:34Z","lastAt":"2016-02-16T23:43:32Z","messageCount":5,"participants":["Felipe Gonçalves Assis","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"278252","messageId":"1455583834-4796-1-git-send-email-felipegassis@gmail.com","threadId":"41420","inReplyTo":null,"subject":"[PATCH] merge-recursive: option to disable renames","fromName":"Felipe Gonçalves Assis","fromEmail":"felipeg.assis@gmail.com","sentAt":"2016-02-16T00:50:34Z","receivedAt":"2016-02-16T00:50:34Z","isPatch":true,"sender":{"key":"felipeg.assis@gmail.com","avatar":null},"body":"The recursive strategy turns on rename detection by default. Add a\nstrategy option to disable rename detection even for exact renames.\n\nSigned-off-by: Felipe Gonçalves Assis \n---\n\nHi, this is a patch relative to the \"Custom merge driver with no rename\ndetection\" thread, based on suggestions by Junio C Hamano.\n\n Documentation/merge-strategies.txt | 4 ++++\n merge-recursive.c | 16 +++++++++++++---\n merge-recursive.h | 1 +\n 3 files changed, 18 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/merge-strategies.txt b/Documentation/merge-strategies.txt\nindex 7bbd19b..0528d85 100644\n--- a/Documentation/merge-strategies.txt\n+++ b/Documentation/merge-strategies.txt\n@@ -81,6 +81,10 @@ no-renormalize;;\n \tDisables the `renormalize` option. This overrides the\n \t`merge.renormalize` configuration variable.\n \n+no-renames;;\n+\tTurn off rename detection.\n+\tSee also linkgit:git-diff[1] `--no-renames`.\n+\n rename-threshold=;;\n \tControls the similarity threshold used for rename detection.\n \tSee also linkgit:git-diff[1] `-M`.\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 8eabde2..ca67805 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1839,9 +1839,16 @@ int merge_trees(struct merge_options *o,\n \n \t\tentries = get_unmerged();\n \t\trecord_df_conflict_files(o, entries);\n-\t\tre_head = get_renames(o, head, common, head, merge, entries);\n-\t\tre_merge = get_renames(o, merge, common, head, merge, entries);\n-\t\tclean = process_renames(o, re_head, re_merge);\n+\t\tif (o->detect_rename) {\n+\t\t\tre_head = get_renames(o, head, common, head, merge, entries);\n+\t\t\tre_merge = get_renames(o, merge, common, head, merge, entries);\n+\t\t\tclean = process_renames(o, re_head, re_merge);\n+\t\t}\n+\t\telse {\n+\t\t\tre_head = xcalloc(1, sizeof(struct string_list));\n+\t\t\tre_merge = xcalloc(1, sizeof(struct string_list));\n+\t\t\tclean = 1;\n+\t\t}\n \t\tfor (i = entries->nr-1; 0 <= i; i--) {\n \t\t\tconst char *path = entries->items[i].string;\n \t\t\tstruct stage_data *e = entries->items[i].util;\n@@ -2039,6 +2046,7 @@ 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 = DIFF_DETECT_RENAME;\n \tmerge_recursive_config(o);\n \tif (getenv(\"GIT_MERGE_VERBOSITY\"))\n \t\to->verbosity =\n@@ -2088,6 +2096,8 @@ int parse_merge_opt(struct merge_options *o, const char *s)\n \t\to->renormalize = 1;\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 \telse if (skip_prefix(s, \"rename-threshold=\", &arg)) {\n \t\tif ((o->rename_score = parse_rename_score(&arg)) == -1 || *arg != 0)\n \t\t\treturn -1;\ndiff --git a/merge-recursive.h b/merge-recursive.h\nindex 9e090a3..52f0201 100644\n--- a/merge-recursive.h\n+++ b/merge-recursive.h\n@@ -17,6 +17,7 @@ struct merge_options {\n \tunsigned renormalize : 1;\n \tlong xdl_opts;\n \tint verbosity;\n+\tint detect_rename;\n \tint diff_rename_limit;\n \tint merge_rename_limit;\n \tint rename_score;\n-- \n2.7.1.291.gd6478c6\n"},{"id":"278393","messageId":"xmqqegccmikn.fsf@gitster.mtv.corp.google.com","threadId":"41420","inReplyTo":"1455583834-4796-1-git-send-email-felipegassis@gmail.com","subject":"Re: [PATCH] merge-recursive: option to disable renames","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-16T21:49:44Z","receivedAt":"2016-02-16T21:49:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Felipe Gonçalves Assis\" writes:\n\n> +no-renames;;\n> +\tTurn off rename detection.\n> +\tSee also linkgit:git-diff[1] `--no-renames`.\n\nEven though by default for merge-recursive the rename detection is\non, if we are adding an option to control this aspect of the\nbehaviour from the command line, it should follow the usual pattern,\ni.e.\n\n (1) the code to parse options would allow an earlier \"--no-renames\"\n on the command line to be overridden with a later \"--renames\"; and\n\n (2) the description in the documentation would be headed by\n \"--[no-]renames\", describes which one is the default, etc.\n\n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index 8eabde2..ca67805 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -1839,9 +1839,16 @@ int merge_trees(struct merge_options *o,\n> \n> \t\tentries = get_unmerged();\n> \t\trecord_df_conflict_files(o, entries);\n> -\t\tre_head = get_renames(o, head, common, head, merge, entries);\n> -\t\tre_merge = get_renames(o, merge, common, head, merge, entries);\n> -\t\tclean = process_renames(o, re_head, re_merge);\n> +\t\tif (o->detect_rename) {\n> +\t\t\tre_head = get_renames(o, head, common, head, merge, entries);\n> +\t\t\tre_merge = get_renames(o, merge, common, head, merge, entries);\n> +\t\t\tclean = process_renames(o, re_head, re_merge);\n> +\t\t}\n> +\t\telse {\n> +\t\t\tre_head = xcalloc(1, sizeof(struct string_list));\n> +\t\t\tre_merge = xcalloc(1, sizeof(struct string_list));\n> +\t\t\tclean = 1;\n> +\t\t}\n\nYup, this is much nicer than butchering diffcore-rename.c for no\ngood reason ;-).\n\nEven if you _know_ that process_renames() currently does not do\nanything other than returning 1, it is a bad idea to hardcode that\nknowledge on the caller's side. Perhaps\n\n> \t\tentries = get_unmerged();\n> \t\trecord_df_conflict_files(o, entries);\n> -\t\tre_head = get_renames(o, head, common, head, merge, entries);\n> -\t\tre_merge = get_renames(o, merge, common, head, merge, entries);\n> +\t\tif (o->detect_rename) {\n> +\t\t\tre_head = get_renames(o, head, common, head, merge, entries);\n> +\t\t\tre_merge = get_renames(o, merge, common, head, merge, entries);\n> +\t\t} else {\n> +\t\t\tre_head = xcalloc(1, sizeof(struct string_list));\n> +\t\t\tre_merge = xcalloc(1, sizeof(struct string_list));\n> +\t\t}\n> \t\tclean = process_renames(o, re_head, re_merge);\n\nPreparing a new empty string_list instance with xcalloc() without\ndoing string_list_init() is probably \"caller knows too much about\nimplementation detail of the API\", but get_renames() seems to do so\nalready, so I'll let it pass.\n"},{"id":"278397","messageId":"xmqq60xomhlq.fsf@gitster.mtv.corp.google.com","threadId":"41420","inReplyTo":"xmqqegccmikn.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] merge-recursive: option to disable renames","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-16T22:10:41Z","receivedAt":"2016-02-16T22:10:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> \"Felipe Gonçalves Assis\" writes:\n>\n>> +no-renames;;\n>> +\tTurn off rename detection.\n>> +\tSee also linkgit:git-diff[1] `--no-renames`.\n>\n> Even though by default for merge-recursive the rename detection is\n> on, if we are adding an option to control this aspect of the\n> behaviour from the command line, it should follow the usual pattern,\n> i.e.\n>\n> (1) the code to parse options would allow an earlier \"--no-renames\"\n> on the command line to be overridden with a later \"--renames\"; and\n>\n> (2) the description in the documentation would be headed by\n> \"--[no-]renames\", describes which one is the default, etc.\n>\n>> diff --git a/merge-recursive.c b/merge-recursive.c\n>> index 8eabde2..ca67805 100644\n>> --- a/merge-recursive.c\n>> +++ b/merge-recursive.c\n>> @@ -1839,9 +1839,16 @@ int merge_trees(struct merge_options *o,\n>> \n>> \t\tentries = get_unmerged();\n>> \t\trecord_df_conflict_files(o, entries);\n>> -\t\tre_head = get_renames(o, head, common, head, merge, entries);\n>> -\t\tre_merge = get_renames(o, merge, common, head, merge, entries);\n>> -\t\tclean = process_renames(o, re_head, re_merge);\n>> +\t\tif (o->detect_rename) {\n>> +\t\t\tre_head = get_renames(o, head, common, head, merge, entries);\n>> +\t\t\tre_merge = get_renames(o, merge, common, head, merge, entries);\n>> +\t\t\tclean = process_renames(o, re_head, re_merge);\n>> +\t\t}\n>> +\t\telse {\n>> +\t\t\tre_head = xcalloc(1, sizeof(struct string_list));\n>> +\t\t\tre_merge = xcalloc(1, sizeof(struct string_list));\n>> +\t\t\tclean = 1;\n>> +\t\t}\n>\n> Yup, this is much nicer than butchering diffcore-rename.c for no\n> good reason ;-).\n\nThinking about this a bit deeper, you are already passing o to\nget_renames, so I think the right place to decide \"Oh, I can just\nreturn an empty list\" is inside that function. That is, the meat of\nthe change in this patch should be just these three lines instead, I\nthink. You'd of course need to have addition of that new field,\noption parsing, and documentation update in addition to that.\n\nThanks.\n\n\n merge-recursive.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 8eabde2..69fb947 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -482,6 +482,9 @@ 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+\t\treturn renames;\n+\n \tdiff_setup(&opts);\n \tDIFF_OPT_SET(&opts, RECURSIVE);\n \tDIFF_OPT_CLR(&opts, RENAME_EMPTY);\n"},{"id":"278402","messageId":"CALMa68q+TgAXS0=xc1CpZ2nbzGb+TWafWCUXZhGrWPzFL5_AQQ@mail.gmail.com","threadId":"41420","inReplyTo":"xmqqegccmikn.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] merge-recursive: option to disable renames","fromName":"Felipe Gonçalves Assis","fromEmail":"felipeg.assis@gmail.com","sentAt":"2016-02-16T22:42:05Z","receivedAt":"2016-02-16T22:42:05Z","isPatch":true,"sender":{"key":"felipeg.assis@gmail.com","avatar":null},"body":"On 16 February 2016 at 19:49, Junio C Hamano wrote:\n> \"Felipe Gonçalves Assis\" writes:\n>\n>> +no-renames;;\n>> + Turn off rename detection.\n>> + See also linkgit:git-diff[1] `--no-renames`.\n>\n> Even though by default for merge-recursive the rename detection is\n> on, if we are adding an option to control this aspect of the\n> behaviour from the command line, it should follow the usual pattern,\n> i.e.\n>\n> (1) the code to parse options would allow an earlier \"--no-renames\"\n> on the command line to be overridden with a later \"--renames\"; and\n>\n> (2) the description in the documentation would be headed by\n> \"--[no-]renames\", describes which one is the default, etc.\n>\n\nJust a note: In git-diff, there is no \"--renames\". Instead, renames\nare enabled by the \"--find-renames[=]\" option.\n\n1. Do you think \"rename-threshold=\" should enable renames?\n2. Should we have an option \"find-renames\" in merge?\n3. Should git-diff have a \"--renames\" option?\n\nI personally like your suggestion as it is. Just wanted to make sure\nyou considered that while I work on the next patch.\n\nThanks,\nFelipe\n"},{"id":"278415","messageId":"xmqqoabgkyqj.fsf@gitster.mtv.corp.google.com","threadId":"41420","inReplyTo":"CALMa68q+TgAXS0=xc1CpZ2nbzGb+TWafWCUXZhGrWPzFL5_AQQ@mail.gmail.com","subject":"Re: [PATCH] merge-recursive: option to disable renames","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-16T23:43:32Z","receivedAt":"2016-02-16T23:43:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Gonçalves Assis writes:\n\n> Just a note: In git-diff, there is no \"--renames\". Instead, renames\n> are enabled by the \"--find-renames[=]\" option.\n>\n> 1. Do you think \"rename-threshold=\" should enable renames?\n\nI do not think it matters in practice, but if you have\n\n\tgit merge-recursive --no-renames --rename-threshold=50%\n\nin mind, yes, I think it is OK for the latter to flip \"do we do the\nrename detection?\" switch on. Afger all, that is what happens in\n\n\tgit diff --no-renames --find-renames=50%\n\n> 2. Should we have an option \"find-renames\" in merge?\n> 3. Should git-diff have a \"--renames\" option?\n\nGood questions. I think the existing command line options to\nmerge-recursive are misdesigned. It should not have invented\nrename-threshold at all and instead used --find-renames=.\n\nWe could declare that \"--rename-threshold=\" that merge-recursive\nhas is a synonym to \"--find-renames=\" that exists for hysterical\nraisins, and start supporting \"--find-renames=\" as the \"kosher\"\nversion. That would align the two commands better. But I view it\nas a much lower priority than adding \"--no-renames\".\n\n> I personally like your suggestion as it is. Just wanted to make sure\n> you considered that while I work on the next patch.\n\nI didn't actually check what longhand \"-M\" has in the diff land. It\nwas a long time ago I wrote that code ;-)\n\nFor now, I would think the following is sensible:\n\n * Add \"--no-renames\" (but not \"--renames\") to merge-recursive;\n\n * Teach \"--rename-threshold=\" in merge-recursive to enable\n rename detection if it was disabled earlier.\n\nOptionally:\n\n * Teach merge-recursive to take \"--find-renames=\" as well, and\n behave the same way as \"--rename-threshold=\" does, after\n doing the two things above.\n\nThanks.\n"}]}