[PATCH v3 0/2] merge-recursive: option to disable renames

No more renames option. rename-threshold enables renames.

detect_rename is now a simple boolean. Its value is no longer linked to
DIFF_ANYTHING symbol.

A second optional patch is included, teaching merge-recursive to take
"find-renames[=<n>]" as well, for consistency.

Felipe Gonçalves Assis (2):
  merge-recursive: option to disable renames
  merge-recursive: more consistent interface

 Documentation/merge-strategies.txt | 14 ++++++++++++--
 merge-recursive.c                  | 12 +++++++++++-
 merge-recursive.h                  |  1 +
 3 files changed, 24 insertions(+), 3 deletions(-)

-- 
2.7.1.288.gfad33a8

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 <redacted>
---
 Documentation/merge-strategies.txt | 6 ++++++
 merge-recursive.c                  | 7 +++++++
 merge-recursive.h                  | 1 +
 3 files changed, 14 insertions(+)

diff --git a/Documentation/merge-strategies.txt b/Documentation/merge-strategies.txt
index 7bbd19b..1a5e197 100644
--- a/Documentation/merge-strategies.txt
+++ b/Documentation/merge-strategies.txt

@@ -81,8 +81,14 @@ 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.
+	Re-enables rename detection if disabled by a preceding
+	`no-renames`.
 	See also linkgit:git-diff[1] `-M`.
 
 subtree[=<path>];;

diff --git a/merge-recursive.c b/merge-recursive.c
index 8eabde2..6dd0a11 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);

@@ -2039,6 +2042,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 = 1;
 	merge_recursive_config(o);
 	if (getenv("GIT_MERGE_VERBOSITY"))
 		o->verbosity =

@@ -2088,9 +2092,12 @@ 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;
+		o->detect_rename = 1;
 	}
 	else
 		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.288.gfad33a8

Add strategy option find-renames, following git-diff interface. This
makes the option rename-threshold redundant.
---

A minor note
git diff --check complains about an indent with spaces here, but I think I did
the right thing: indented with tabs and aligned with spaces. If desired, I can
align with tabs to avoid this.

 Documentation/merge-strategies.txt | 8 ++++++--
 merge-recursive.c                  | 5 ++++-
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/Documentation/merge-strategies.txt b/Documentation/merge-strategies.txt
index 1a5e197..f8618c9 100644
--- a/Documentation/merge-strategies.txt
+++ b/Documentation/merge-strategies.txt

@@ -85,11 +85,15 @@ no-renames;;
 	Turn off rename detection.
 	See also linkgit:git-diff[1] `--no-renames`.
 
-rename-threshold=<n>;;
+find-renames[=<n>];;
 	Controls the similarity threshold used for rename detection.
 	Re-enables rename detection if disabled by a preceding
 	`no-renames`.
-	See also linkgit:git-diff[1] `-M`.
+	See also linkgit:git-diff[1] `--find-renames`.
+
+rename-threshold=<n>;;
+	Synonym for `find-renames=<n>`. Present for historical reasons.
+	New scripts should prefer the `find-renames=<n>` syntax.
 
 subtree[=<path>];;
 	This option is a more advanced form of 'subtree' strategy, where

diff --git a/merge-recursive.c b/merge-recursive.c
index 6dd0a11..700febd 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c

@@ -2094,7 +2094,10 @@ int parse_merge_opt(struct merge_options *o, const char *s)
 		o->renormalize = 0;
 	else if (!strcmp(s, "no-renames"))
 		o->detect_rename = 0;
-	else if (skip_prefix(s, "rename-threshold=", &arg)) {
+	else if (!strcmp(s, "find-renames"))
+		o->detect_rename = 1;
+	else if (skip_prefix(s, "find-renames=", &arg) ||
+	         skip_prefix(s, "rename-threshold=", &arg)) {
 		if ((o->rename_score = parse_rename_score(&arg)) == -1 || *arg != 0)
 			return -1;
 		o->detect_rename = 1;

-- 
2.7.1.288.gfad33a8

On Tue, Feb 16, 2016 at 8:11 PM, Felipe Gonçalves Assis
[off-list ref] wrote:

No more renames option. rename-threshold enables renames.

Can you add some tests? Off the top of my head, I'd expect to see at
least three new tests:

1. --no-renames works as expected
2. last wins in "--no-renames --rename-threshold=x"
3. last wins in "--rename-threshold=x --no-renames"

A second optional patch is included, teaching merge-recursive to take
"find-renames[=<n>]" as well, for consistency.

Tests should also accompany this change. A couple obvious ones:

1. --find-rename=x works as synonym for --rename-threshold=x
2. --fine-rename (without "=x") works as expected

Thanks.

Felipe Gonçalves Assis (2):
  merge-recursive: option to disable renames
  merge-recursive: more consistent interface

 Documentation/merge-strategies.txt | 14 ++++++++++++--
 merge-recursive.c                  | 12 +++++++++++-
 merge-recursive.h                  |  1 +
 3 files changed, 24 insertions(+), 3 deletions(-)

--
2.7.1.288.gfad33a8

On Tue, Feb 16, 2016 at 8:11 PM, Felipe Gonçalves Assis
[off-list ref] wrote:

I'm not sure that it is necessary to mention the "last one wins" rule
here, but if you do so, does --no-renames documentation deserve
similar treatment?

        See also linkgit:git-diff[1] `-M`.

On Tue, Feb 16, 2016 at 8:11 PM, Felipe Gonçalves Assis
[off-list ref] wrote:

Add strategy option find-renames, following git-diff interface. This
makes the option rename-threshold redundant.

Missing sign-off.

This may need some enhancement. It doesn't, for instance, talk about
what bare --find-renames (without "=n") means.

-       See also linkgit:git-diff[1] `-M`.
+       See also linkgit:git-diff[1] `--find-renames`.
+
+rename-threshold=<n>;;
+       Synonym for `find-renames=<n>`. Present for historical reasons.
+       New scripts should prefer the `find-renames=<n>` syntax.

It might be sufficient to say merely:

    Deprecated synonym for `--find-renames=<n>`.

which implies the bits about "historical reasons" and "new scripts
should prefer...".

On 16 February 2016 at 23:41, Eric Sunshine [off-list ref] wrote:

On Tue, Feb 16, 2016 at 8:11 PM, Felipe Gonçalves Assis
[off-list ref] wrote:

quoted

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

diff --git a/Documentation/merge-strategies.txt b/Documentation/merge-strategies.txt

@@ -81,8 +81,14 @@ no-renormalize;;
+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.
+       Re-enables rename detection if disabled by a preceding
+       `no-renames`.

I'm not sure that it is necessary to mention the "last one wins" rule
here, but if you do so, does --no-renames documentation deserve
similar treatment?

It is not so much about the "last one wins" rule, but about the fact
that, given the sub-optimal name, it is not obvious that this option
even enables renaming. The wording is just the best way I found to
explain that without creating additional confusion.

I will apply your suggestion on the second commit. Please see the next
patch proposal and tell what you think.

Thanks,
Felipe

On 16 February 2016 at 23:37, Eric Sunshine [off-list ref] wrote:

On Tue, Feb 16, 2016 at 8:11 PM, Felipe Gonçalves Assis
[off-list ref] wrote:

quoted

No more renames option. rename-threshold enables renames.

Can you add some tests? Off the top of my head, I'd expect to see at
least three new tests:

1. --no-renames works as expected
2. last wins in "--no-renames --rename-threshold=x"
3. last wins in "--rename-threshold=x --no-renames"

Sure.

Just a heads up: For the next three days I will have very little time,
so I will start tackling this on the weekend. I submitted a fourth
version of the patch without the tests, so that this part of the work
can proceed in parallel.

Thanks,
Felipe