threads / patch / 41420

patchmerge-recursive: option to disable renames

Subject: [PATCH] merge-recursive: option to disable renames

## tl;dr

5 messages between Feb 16, 2016 and Feb 16, 2016. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

Felipe Gonçalves Assis· Feb 16, 2016, 00:50 UTC · lore

The recursive strategy turns on rename detection by default. Add a strategy option to disable rename detection even for exact renames.

Signed-off-by: Felipe Gonçalves Assis <felipegassis@gmail.com>
---

Hi, this is a patch relative to the "Custom merge driver with no rename detection" thread, based on suggestions by Junio C Hamano.

 Documentation/merge-strategies.txt |  4 ++++
 merge-recursive.c                  | 16 +++++++++++++---
 merge-recursive.h                  |  1 +
 3 files changed, 18 insertions(+), 3 deletions(-)

Show changes to 3 files +18 −3

Documentation/merge-strategies.txt, merge-recursive.c, merge-recursive.h

diff --git a/Documentation/merge-strategies.txt b/Documentation/merge-strategies.txt
index 7bbd19b..0528d85 100644
--- a/Documentation/merge-strategies.txt
+++ b/Documentation/merge-strategies.txt
@@ -81,6 +81,10 @@ no-renormalize;;
 	Disables the `renormalize` option.  This overrides the
 	`merge.renormalize` configuration variable.
 
+no-renames;;
+	Turn off rename detection.
+	See also linkgit:git-diff[1] `--no-renames`.
+
 rename-threshold=<n>;;
 	Controls the similarity threshold used for rename detection.
 	See also linkgit:git-diff[1] `-M`.
diff --git a/merge-recursive.c b/merge-recursive.c
index 8eabde2..ca67805 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1839,9 +1839,16 @@ int merge_trees(struct merge_options *o,
 
 		entries = get_unmerged();
 		record_df_conflict_files(o, entries);
-		re_head  = get_renames(o, head, common, head, merge, entries);
-		re_merge = get_renames(o, merge, common, head, merge, entries);
-		clean = process_renames(o, re_head, re_merge);
+		if (o->detect_rename) {
+			re_head  = get_renames(o, head, common, head, merge, entries);
+			re_merge = get_renames(o, merge, common, head, merge, entries);
+			clean = process_renames(o, re_head, re_merge);
+		}
+		else {
+			re_head  = xcalloc(1, sizeof(struct string_list));
+			re_merge = xcalloc(1, sizeof(struct string_list));
+			clean = 1;
+		}
 		for (i = entries->nr-1; 0 <= i; i--) {
 			const char *path = entries->items[i].string;
 			struct stage_data *e = entries->items[i].util;
@@ -2039,6 +2046,7 @@ void init_merge_options(struct merge_options *o)
 	o->diff_rename_limit = -1;
 	o->merge_rename_limit = -1;
 	o->renormalize = 0;
+	o->detect_rename = DIFF_DETECT_RENAME;
 	merge_recursive_config(o);
 	if (getenv("GIT_MERGE_VERBOSITY"))
 		o->verbosity =
@@ -2088,6 +2096,8 @@ int parse_merge_opt(struct merge_options *o, const char *s)
 		o->renormalize = 1;
 	else if (!strcmp(s, "no-renormalize"))
 		o->renormalize = 0;
+	else if (!strcmp(s, "no-renames"))
+		o->detect_rename = 0;
 	else if (skip_prefix(s, "rename-threshold=", &arg)) {
 		if ((o->rename_score = parse_rename_score(&arg)) == -1 || *arg != 0)
 			return -1;
diff --git a/merge-recursive.h b/merge-recursive.h
index 9e090a3..52f0201 100644
--- a/merge-recursive.h
+++ b/merge-recursive.h
@@ -17,6 +17,7 @@ struct merge_options {
 	unsigned renormalize : 1;
 	long xdl_opts;
 	int verbosity;
+	int detect_rename;
 	int diff_rename_limit;
 	int merge_rename_limit;
 	int rename_score;

-- 
2.7.1.291.gd6478c6

Junio C Hamano· Feb 16, 2016, 21:49 UTC · re: Felipe Gonçalves Assis · lore

Re: [PATCH] merge-recursive: option to disable renames

"Felipe Gonçalves Assis"  <felipeg.assis@gmail.com> writes:

> +no-renames;;
> +	Turn off rename detection.
> +	See also linkgit:git-diff[1] `--no-renames`.

Even though by default for merge-recursive the rename detection is on, if we are adding an option to control this aspect of the behaviour from the command line, it should follow the usual pattern, i.e.

 (1) the code to parse options would allow an earlier "--no-renames"
     on the command line to be overridden with a later "--renames"; and

 (2) the description in the documentation would be headed by
     "--[no-]renames", describes which one is the default, etc.

Show 21 quoted lines

> diff --git a/merge-recursive.c b/merge-recursive.c
> index 8eabde2..ca67805 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -1839,9 +1839,16 @@ int merge_trees(struct merge_options *o,
>  
>  		entries = get_unmerged();
>  		record_df_conflict_files(o, entries);
> -		re_head  = get_renames(o, head, common, head, merge, entries);
> -		re_merge = get_renames(o, merge, common, head, merge, entries);
> -		clean = process_renames(o, re_head, re_merge);
> +		if (o->detect_rename) {
> +			re_head  = get_renames(o, head, common, head, merge, entries);
> +			re_merge = get_renames(o, merge, common, head, merge, entries);
> +			clean = process_renames(o, re_head, re_merge);
> +		}
> +		else {
> +			re_head  = xcalloc(1, sizeof(struct string_list));
> +			re_merge = xcalloc(1, sizeof(struct string_list));
> +			clean = 1;
> +		}

Yup, this is much nicer than butchering diffcore-rename.c for no good reason ;-).

Even if you _know_ that process_renames() currently does not do anything other than returning 1, it is a bad idea to hardcode that knowledge on the caller's side. Perhaps

Show 12 quoted lines

>  		entries = get_unmerged();
>  		record_df_conflict_files(o, entries);
> -		re_head  = get_renames(o, head, common, head, merge, entries);
> -		re_merge = get_renames(o, merge, common, head, merge, entries);
> +		if (o->detect_rename) {
> +			re_head  = get_renames(o, head, common, head, merge, entries);
> +			re_merge = get_renames(o, merge, common, head, merge, entries);
> +		} else {
> +			re_head  = xcalloc(1, sizeof(struct string_list));
> +			re_merge = xcalloc(1, sizeof(struct string_list));
> +		}
> 		clean = process_renames(o, re_head, re_merge);

Preparing a new empty string_list instance with xcalloc() without doing string_list_init() is probably "caller knows too much about implementation detail of the API", but get_renames() seems to do so already, so I'll let it pass.

Junio C Hamano· Feb 16, 2016, 22:10 UTC · re: Junio C Hamano · lore

Re: [PATCH] merge-recursive: option to disable renames

Junio C Hamano <gitster@pobox.com> writes:

Show 41 quoted lines

> "Felipe Gonçalves Assis"  <felipeg.assis@gmail.com> writes:
>
>> +no-renames;;
>> +	Turn off rename detection.
>> +	See also linkgit:git-diff[1] `--no-renames`.
>
> Even though by default for merge-recursive the rename detection is
> on, if we are adding an option to control this aspect of the
> behaviour from the command line, it should follow the usual pattern,
> i.e.
>
>  (1) the code to parse options would allow an earlier "--no-renames"
>      on the command line to be overridden with a later "--renames"; and
>
>  (2) the description in the documentation would be headed by
>      "--[no-]renames", describes which one is the default, etc.
>
>> diff --git a/merge-recursive.c b/merge-recursive.c
>> index 8eabde2..ca67805 100644
>> --- a/merge-recursive.c
>> +++ b/merge-recursive.c
>> @@ -1839,9 +1839,16 @@ int merge_trees(struct merge_options *o,
>>  
>>  		entries = get_unmerged();
>>  		record_df_conflict_files(o, entries);
>> -		re_head  = get_renames(o, head, common, head, merge, entries);
>> -		re_merge = get_renames(o, merge, common, head, merge, entries);
>> -		clean = process_renames(o, re_head, re_merge);
>> +		if (o->detect_rename) {
>> +			re_head  = get_renames(o, head, common, head, merge, entries);
>> +			re_merge = get_renames(o, merge, common, head, merge, entries);
>> +			clean = process_renames(o, re_head, re_merge);
>> +		}
>> +		else {
>> +			re_head  = xcalloc(1, sizeof(struct string_list));
>> +			re_merge = xcalloc(1, sizeof(struct string_list));
>> +			clean = 1;
>> +		}
>
> Yup, this is much nicer than butchering diffcore-rename.c for no
> good reason ;-).

Thinking about this a bit deeper, you are already passing o to get_renames, so I think the right place to decide "Oh, I can just return an empty list" is inside that function. That is, the meat of the change in this patch should be just these three lines instead, I think. You'd of course need to have addition of that new field, option parsing, and documentation update in addition to that.

Thanks.

 merge-recursive.c | 3 +++
 1 file changed, 3 insertions(+)

Show changes to merge-recursive.c +3 −0

diff --git a/merge-recursive.c b/merge-recursive.c
index 8eabde2..69fb947 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -482,6 +482,9 @@ static struct string_list *get_renames(struct merge_options *o,
 	struct diff_options opts;
 
 	renames = xcalloc(1, sizeof(struct string_list));
+	if (!o->detect_rename)
+		return renames;
+
 	diff_setup(&opts);
 	DIFF_OPT_SET(&opts, RECURSIVE);
 	DIFF_OPT_CLR(&opts, RENAME_EMPTY);

Felipe Gonçalves Assis· Feb 16, 2016, 22:42 UTC · re: Junio C Hamano · lore

Re: [PATCH] merge-recursive: option to disable renames

On 16 February 2016 at 19:49, Junio C Hamano <gitster@pobox.com> wrote:

Show 17 quoted lines

> "Felipe Gonçalves Assis"  <felipeg.assis@gmail.com> writes:
>
>> +no-renames;;
>> +     Turn off rename detection.
>> +     See also linkgit:git-diff[1] `--no-renames`.
>
> Even though by default for merge-recursive the rename detection is
> on, if we are adding an option to control this aspect of the
> behaviour from the command line, it should follow the usual pattern,
> i.e.
>
>  (1) the code to parse options would allow an earlier "--no-renames"
>      on the command line to be overridden with a later "--renames"; and
>
>  (2) the description in the documentation would be headed by
>      "--[no-]renames", describes which one is the default, etc.
>

Just a note: In git-diff, there is no "--renames". Instead, renames are enabled by the "--find-renames[=<n>]" option.

1. Do you think "rename-threshold=<n>" should enable renames?
2. Should we have an option "find-renames" in merge?
3. Should git-diff have a "--renames" option?

I personally like your suggestion as it is. Just wanted to make sure you considered that while I work on the next patch.

Thanks, Felipe

Junio C Hamano· Feb 16, 2016, 23:43 UTC · re: Felipe Gonçalves Assis · lore

Re: [PATCH] merge-recursive: option to disable renames

Felipe Gonçalves Assis <felipeg.assis@gmail.com> writes:

> Just a note: In git-diff, there is no "--renames". Instead, renames
> are enabled by the "--find-renames[=<n>]" option.
>
> 1. Do you think "rename-threshold=<n>" should enable renames?

I do not think it matters in practice, but if you have

	git merge-recursive --no-renames --rename-threshold=50%

in mind, yes, I think it is OK for the latter to flip "do we do the rename detection?" switch on. Afger all, that is what happens in

	git diff --no-renames --find-renames=50%

> 2. Should we have an option "find-renames" in merge?
> 3. Should git-diff have a "--renames" option?

Good questions. I think the existing command line options to merge-recursive are misdesigned. It should not have invented rename-threshold at all and instead used --find-renames=<n>.

We could declare that "--rename-threshold=<n>" that merge-recursive has is a synonym to "--find-renames=<n>" that exists for hysterical raisins, and start supporting "--find-renames=<n>" as the "kosher" version. That would align the two commands better. But I view it as a much lower priority than adding "--no-renames".

> I personally like your suggestion as it is. Just wanted to make sure
> you considered that while I work on the next patch.

I didn't actually check what longhand "-M" has in the diff land. It was a long time ago I wrote that code ;-)

For now, I would think the following is sensible:

   * Add "--no-renames" (but not "--renames") to merge-recursive;

   * Teach "--rename-threshold=<n>" in merge-recursive to enable
     rename detection if it was disabled earlier.

Optionally:

   * Teach merge-recursive to take "--find-renames=<n>" as well, and
     behave the same way as "--rename-threshold=<n>" does, after
     doing the two things above.

Thanks.