# [PATCH v3 0/2] merge-recursive: option to disable renames 8 messages from 2016-02-17 to 2016-02-17. Participants: Felipe Gonçalves Assis, Eric Sunshine. Thread: https://gitlist.dev/t/41429 ## Felipe Gonçalves Assis, 2016-02-17 01:11 Subject: [PATCH v3 0/2] merge-recursive: option to disable renames Message-ID: <1455671495-10908-1-git-send-email-felipegassis@gmail.com> URL: https://gitlist.dev/e/1455671495-10908-1-git-send-email-felipegassis%40gmail.com ``` 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[=]" 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, 2016-02-17 01:11 Subject: [PATCH v3 1/2] merge-recursive: option to disable renames Message-ID: <1455671495-10908-2-git-send-email-felipegassis@gmail.com> URL: https://gitlist.dev/e/1455671495-10908-2-git-send-email-felipegassis%40gmail.com In-Reply-To: <1455671495-10908-1-git-send-email-felipegassis@gmail.com> ``` 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 --- 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=;; 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[=];; 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 ``` ## Felipe Gonçalves Assis, 2016-02-17 01:11 Subject: [PATCH v3 2/2] merge-recursive: more consistent interface Message-ID: <1455671495-10908-3-git-send-email-felipegassis@gmail.com> URL: https://gitlist.dev/e/1455671495-10908-3-git-send-email-felipegassis%40gmail.com In-Reply-To: <1455671495-10908-1-git-send-email-felipegassis@gmail.com> ``` 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=;; +find-renames[=];; 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=;; + Synonym for `find-renames=`. Present for historical reasons. + New scripts should prefer the `find-renames=` syntax. subtree[=];; 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, 2016-02-17 01:37 Subject: Re: [PATCH v3 0/2] merge-recursive: option to disable renames Message-ID: URL: https://gitlist.dev/e/CAPig%2BcRhbCRJhrOFcjY2avFZ0rQgCP-JT-6RTCiihOQB8nWeMw%40mail.gmail.com In-Reply-To: <1455671495-10908-1-git-send-email-felipegassis@gmail.com> ``` On Tue, Feb 16, 2016 at 8:11 PM, Felipe Gonçalves Assis 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[=]" 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 ``` ## Eric Sunshine, 2016-02-17 01:41 Subject: Re: [PATCH v3 1/2] merge-recursive: option to disable renames Message-ID: URL: https://gitlist.dev/e/CAPig%2BcRCKQ8Vpr11XB-MSNsjDXjMHstaEAz333nMZxuKC8xmEg%40mail.gmail.com In-Reply-To: <1455671495-10908-2-git-send-email-felipegassis@gmail.com> ``` On Tue, Feb 16, 2016 at 8:11 PM, Felipe Gonçalves Assis 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 > --- > 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=;; > 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`. ``` ## Eric Sunshine, 2016-02-17 01:52 Subject: Re: [PATCH v3 2/2] merge-recursive: more consistent interface Message-ID: URL: https://gitlist.dev/e/CAPig%2BcQMutC1rDoLTZHu6zYi2i7s9jYkO8Nb29q8tJwEjq%2BADg%40mail.gmail.com In-Reply-To: <1455671495-10908-3-git-send-email-felipegassis@gmail.com> ``` On Tue, Feb 16, 2016 at 8:11 PM, Felipe Gonçalves Assis 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=;; > +find-renames[=];; > 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=;; > + Synonym for `find-renames=`. Present for historical reasons. > + New scripts should prefer the `find-renames=` syntax. It might be sufficient to say merely: Deprecated synonym for `--find-renames=`. which implies the bits about "historical reasons" and "new scripts should prefer...". > subtree[=];; > 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 ``` ## Felipe Gonçalves Assis, 2016-02-17 03:16 Subject: Re: [PATCH v3 1/2] merge-recursive: option to disable renames Message-ID: URL: https://gitlist.dev/e/CALMa68peGr3TsS8bTpsG3Xt5U5VtbLhKryhOSLEYaNZyjRuSgQ%40mail.gmail.com In-Reply-To: ``` On 16 February 2016 at 23:41, Eric Sunshine wrote: > On Tue, Feb 16, 2016 at 8:11 PM, Felipe Gonçalves Assis > 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 >> --- >> 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=;; >> 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, 2016-02-17 03:38 Subject: Re: [PATCH v3 0/2] merge-recursive: option to disable renames Message-ID: URL: https://gitlist.dev/e/CALMa68oy2i6GeiUS1MLodAJ__TsqDzbRWcFVY2xdZKcoONKvzA%40mail.gmail.com In-Reply-To: ``` On 16 February 2016 at 23:37, Eric Sunshine wrote: > On Tue, Feb 16, 2016 at 8:11 PM, Felipe Gonçalves Assis > 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 ```