[PATCH] merge-recursive: option to disable renames

[PATCH] merge-recursive: option to disable renames

[Felipe Gonçalves Assis]

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(-)

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

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

[Junio C Hamano]

"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 ;-).

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

>  		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.

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

[Junio C Hamano]

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

> "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(+)

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);

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

[Felipe Gonçalves Assis]

On 16 February 2016 at 19:49, Junio C Hamano <gitster@pobox.com> wrote:
> "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

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

[Junio C Hamano]

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.