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

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

[Felipe Gonçalves Assis]

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

[PATCH v3 1/2] 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>
---
 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

[PATCH v3 2/2] merge-recursive: more consistent interface

[Felipe Gonçalves Assis]

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

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

[Eric Sunshine]

On Tue, Feb 16, 2016 at 8:11 PM, Felipe Gonçalves Assis
<felipeg.assis@gmail.com> 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

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

[Eric Sunshine]

On Tue, Feb 16, 2016 at 8:11 PM, Felipe Gonçalves Assis
<felipeg.assis@gmail.com> wrote:
> 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>
> ---
> 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?

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

Re: [PATCH v3 2/2] merge-recursive: more consistent interface

[Eric Sunshine]

On Tue, Feb 16, 2016 at 8:11 PM, Felipe Gonçalves Assis
<felipeg.assis@gmail.com> wrote:
> Add strategy option find-renames, following git-diff interface. This
> makes the option rename-threshold redundant.

Missing sign-off.

> ---
> diff --git 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`.

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

>  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

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

[Felipe Gonçalves Assis]

On 16 February 2016 at 23:41, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Tue, Feb 16, 2016 at 8:11 PM, Felipe Gonçalves Assis
> <felipeg.assis@gmail.com> wrote:
>> 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>
>> ---
>> 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

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

[Felipe Gonçalves Assis]

On 16 February 2016 at 23:37, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Tue, Feb 16, 2016 at 8:11 PM, Felipe Gonçalves Assis
> <felipeg.assis@gmail.com> 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"
>

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