threads / patch / 41429

v3, 2 partsmerge-recursive: option to disable renames

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

## tl;dr

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

replies: 7people: 2as markdown or json

Felipe Gonçalves Assis· Feb 17, 2016, 01:11 UTC · lore
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
Felipe Gonçalves Assis· Feb 17, 2016, 01:11 UTC · re: Felipe Gonçalves Assis · lore

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

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(+)
Show changes to 3 files +14 −0

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

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
Eric Sunshine· Feb 17, 2016, 01:41 UTC · re: Felipe Gonçalves Assis · lore

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

On Tue, Feb 16, 2016 at 8:11 PM, Felipe Gonçalves Assis <felipeg.assis@gmail.com> wrote:

Show 15 quoted lines
> 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`.
Felipe Gonçalves Assis· Feb 17, 2016, 03:16 UTC · re: Eric Sunshine · lore

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

On 16 February 2016 at 23:41, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 22 quoted lines
> 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

Felipe Gonçalves Assis· Feb 17, 2016, 01:11 UTC · re: Felipe Gonçalves Assis · lore

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

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(-)
Show changes to 2 files +10 −3

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

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
Eric Sunshine· Feb 17, 2016, 01:52 UTC · re: Felipe Gonçalves Assis · lore

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

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.
Show 11 quoted lines
> ---
> 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.

Show 6 quoted lines
> -       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...".

Show 20 quoted lines
>  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
Eric Sunshine· Feb 17, 2016, 01:37 UTC · re: Felipe Gonçalves Assis · lore

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

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.
Show 11 quoted lines
> 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
Felipe Gonçalves Assis· Feb 17, 2016, 03:38 UTC · re: Eric Sunshine · lore

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

On 16 February 2016 at 23:37, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 11 quoted lines
> 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

← back to recent threads