Re: [PATCH] merge-recursive: option to specify rename threshold

Kevin Ballard [off-list ref] writes:

quoted

At the end-user level, this new option to merge-recursive has exactly the
same meaning as existing -M given to "diff" family; people would probably
want to see it made available as a synonym to "diff" family as well, no?

You mean so you can type `git diff --rename-score=50% foo`? A reasonable
suggestion, but then what do we do with -B and -C? It doesn't make much
sense to give a longer name to only one of the three options. This patch
was concerned with simply exposing the functionality to the merge
strategy and doesn't attempt to address the problem of providing long
names for this trio of options.

I would call them --break-threshold and --copy-threshold respectively.

I have been happy without long option names when we originally had only
short names, but some people seem to be able to be more explicit, so...

While we are at it, would it make sense to have "merge-recursive -M20" as
a shorthand as well?

On Sep 26, 2010, at 10:24 PM, Junio C Hamano wrote:

Kevin Ballard [off-list ref] writes:

quoted

quoted

At the end-user level, this new option to merge-recursive has exactly the
same meaning as existing -M given to "diff" family; people would probably
want to see it made available as a synonym to "diff" family as well, no?

You mean so you can type `git diff --rename-score=50% foo`? A reasonable
suggestion, but then what do we do with -B and -C? It doesn't make much
sense to give a longer name to only one of the three options. This patch
was concerned with simply exposing the functionality to the merge
strategy and doesn't attempt to address the problem of providing long
names for this trio of options.

I would call them --break-threshold and --copy-threshold respectively.

I have been happy without long option names when we originally had only
short names, but some people seem to be able to be more explicit, so...

Fair enough. Expect that naming in the next iteration of the patch.

While we are at it, would it make sense to have "merge-recursive -M20" as
a shorthand as well?

So it would be invoked like `git merge -s recursive -X M20 foo`? Looks a bit odd to me. I can add that if you think it's worthwhile though.

-Kevin Ballard

On Sep 26, 2010, at 10:24 PM, Junio C Hamano wrote:

Kevin Ballard [off-list ref] writes:

quoted

quoted

At the end-user level, this new option to merge-recursive has exactly the
same meaning as existing -M given to "diff" family; people would probably
want to see it made available as a synonym to "diff" family as well, no?

You mean so you can type `git diff --rename-score=50% foo`? A reasonable
suggestion, but then what do we do with -B and -C? It doesn't make much
sense to give a longer name to only one of the three options. This patch
was concerned with simply exposing the functionality to the merge
strategy and doesn't attempt to address the problem of providing long
names for this trio of options.

I would call them --break-threshold and --copy-threshold respectively.

I have been happy without long option names when we originally had only
short names, but some people seem to be able to be more explicit, so...

After taking a look at this, it raises another question. -B, -M, and -C all have optional arguments, but the long-form names don't seem to support that. `git diff --rename-threshold= foo` would work, but looks mighty odd, and if I make it support `git diff --rename-threshold foo` that would also work, but the name doesn't seem appropriate without the argument. Should I go ahead and support `git diff --rename-threshold foo` and just live with it looking weird, or do you have a better suggestion?

-Kevin Ballard

Kevin Ballard wrote:

After taking a look at this, it raises another question. -B, -M, and
-C all have optional arguments, but the long-form names don't seem
to support that.

[...]

                      if I make it support `git diff
--rename-threshold foo` that would also work, but the name doesn't
seem appropriate without the argument.

Right --- with merge-recursive the argument doesn't need to be
optional, but with git diff it does.

How about

	--detect-renames=<threshold>
	--detect-copies=<threshold>
	--detect-rewrites=<threshold>/<threshold>

?

Ciao,
Jonathan

The recursive merge strategy turns on rename detection but leaves the
rename threshold at the default. Add a strategy option to allow the user
to specify a rename threshold to use.

Signed-off-by: Kevin Ballard <redacted>
---
 Documentation/merge-strategies.txt |    4 ++++
 diff.c                             |    6 +++---
 diff.h                             |    2 ++
 merge-recursive.c                  |    6 ++++++
 merge-recursive.h                  |    1 +
 5 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/Documentation/merge-strategies.txt b/Documentation/merge-strategies.txt
index 91faba5..77f2606 100644
--- a/Documentation/merge-strategies.txt
+++ b/Documentation/merge-strategies.txt

@@ -74,6 +74,10 @@ no-renormalize;;
 	Disables the `renormalize` option.  This overrides the
 	`merge.renormalize` configuration variable.
 
+rename-threshold=<n>;;
+	Controls the similarity threshold used for rename detection.
+	See also linkgit:git-diff[1] `-M`.
+
 subtree[=path];;
 	This option is a more advanced form of 'subtree' strategy, where
 	the strategy makes a guess on how two trees must be shifted to

diff --git a/diff.c b/diff.c
index cc73061..d862234 100644
--- a/diff.c
+++ b/diff.c

@@ -3323,7 +3323,7 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 	return 1;
 }
 
-static int parse_num(const char **cp_p)
+int parse_rename_score(const char **cp_p)
 {
 	unsigned long num, scale;
 	int ch, dot;

@@ -3369,7 +3369,7 @@ static int diff_scoreopt_parse(const char *opt)
 	if (cmd != 'M' && cmd != 'C' && cmd != 'B')
 		return -1; /* that is not a -M, -C nor -B option */
 
-	opt1 = parse_num(&opt);
+	opt1 = parse_rename_score(&opt);
 	if (cmd != 'B')
 		opt2 = 0;
 	else {

@@ -3379,7 +3379,7 @@ static int diff_scoreopt_parse(const char *opt)
 			return -1; /* we expect -B80/99 or -B80 */
 		else {
 			opt++;
-			opt2 = parse_num(&opt);
+			opt2 = parse_rename_score(&opt);
 		}
 	}
 	if (*opt != 0)

diff --git a/diff.h b/diff.h
index 1fd44f5..0083d92 100644
--- a/diff.h
+++ b/diff.h

@@ -315,4 +315,6 @@ extern size_t fill_textconv(struct userdiff_driver *driver,
 
 extern struct userdiff_driver *get_textconv(struct diff_filespec *one);
 
+extern int parse_rename_score(const char **cp_p);
+
 #endif /* DIFF_H */

diff --git a/merge-recursive.c b/merge-recursive.c
index 325a97b..875859f 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c

@@ -334,6 +334,7 @@ static struct string_list *get_renames(struct merge_options *o,
 	opts.rename_limit = o->merge_rename_limit >= 0 ? o->merge_rename_limit :
 			    o->diff_rename_limit >= 0 ? o->diff_rename_limit :
 			    500;
+	opts.rename_score = o->rename_score;
 	opts.warn_on_too_large_rename = 1;
 	opts.output_format = DIFF_FORMAT_NO_OUTPUT;
 	if (diff_setup_done(&opts) < 0)

@@ -1576,6 +1577,11 @@ 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 (!prefixcmp(s, "rename-threshold=")) {
+		const char *score = s + strlen("rename-threshold=");
+		if ((o->rename_score = parse_rename_score(&score)) == -1 || *score != 0)
+			return -1;
+	}
 	else
 		return -1;
 	return 0;

diff --git a/merge-recursive.h b/merge-recursive.h
index 2eb5d1a..c8135b0 100644
--- a/merge-recursive.h
+++ b/merge-recursive.h

@@ -19,6 +19,7 @@ struct merge_options {
 	int verbosity;
 	int diff_rename_limit;
 	int merge_rename_limit;
+	int rename_score;
 	int call_depth;
 	struct strbuf obuf;
 	struct string_list current_file_set;

-- 
1.7.3.72.g8af0.dirty

Add new long-form options --detect-renames[=<n>], --detect-copies[=<n>],
and --break-rewrites[=[<n>][/<m>]] as synonyms for the -M, -C, and -B
options (respectively).

Signed-off-by: Kevin Ballard <redacted>
---
After thinking about it, I decided that --rename-threshold doesn't make sense
as a long-form option because it doesn't make its meaning obvious when you
don't specify the optional argument. I also figured it doesn't need to match
exactly with the rename-threshold merge strategy option as merge-recursive
already turns on rename detection and the option just controls the threshold.
However, the option to git-diff actually turns on rename detection instead of
just controlling the threshold.

 Documentation/diff-options.txt |    3 +++
 diff.c                         |   25 ++++++++++++++++++++++---
 2 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index f77a0f8..a511529 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt

@@ -207,6 +207,7 @@ endif::git-format-patch[]
 	digits can be specified with `--abbrev=<n>`.
 
 -B[<n>][/<m>]::
+--break-rewrites[=[<n>][/<m>]]::
 	Break complete rewrite changes into pairs of delete and
 	create. This serves two purposes:
 +

@@ -229,6 +230,7 @@ eligible for being picked up as a possible source of a rename to
 another file.
 
 -M[<n>]::
+--detect-renames[=<n>]::
 ifndef::git-log[]
 	Detect renames.
 endif::git-log[]

@@ -244,6 +246,7 @@ endif::git-log[]
 	hasn't changed.
 
 -C[<n>]::
+--detect-copies[=<n>]::
 	Detect copies as well as renames.  See also `--find-copies-harder`.
 	If `n` is specified, it has the same meaning as for `-M<n>`.
 

diff --git a/diff.c b/diff.c
index d862234..d8fcf06 100644
--- a/diff.c
+++ b/diff.c

@@ -3140,16 +3140,19 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 		return stat_opt(options, av);
 
 	/* renames options */
-	else if (!prefixcmp(arg, "-B")) {
+	else if (!prefixcmp(arg, "-B") || !prefixcmp(arg, "--break-rewrites=") ||
+		 !strcmp(arg, "--break-rewrites")) {
 		if ((options->break_opt = diff_scoreopt_parse(arg)) == -1)
 			return -1;
 	}
-	else if (!prefixcmp(arg, "-M")) {
+	else if (!prefixcmp(arg, "-M") || !prefixcmp(arg, "--detect-renames=") ||
+		 !strcmp(arg, "--detect-renames")) {
 		if ((options->rename_score = diff_scoreopt_parse(arg)) == -1)
 			return -1;
 		options->detect_rename = DIFF_DETECT_RENAME;
 	}
-	else if (!prefixcmp(arg, "-C")) {
+	else if (!prefixcmp(arg, "-C") || !prefixcmp(arg, "--detect-copies=") ||
+		 !strcmp(arg, "--detect-copies")) {
 		if (options->detect_rename == DIFF_DETECT_COPY)
 			DIFF_OPT_SET(options, FIND_COPIES_HARDER);
 		if ((options->rename_score = diff_scoreopt_parse(arg)) == -1)

@@ -3366,6 +3369,22 @@ static int diff_scoreopt_parse(const char *opt)
 	if (*opt++ != '-')
 		return -1;
 	cmd = *opt++;
+	if (cmd == '-') {
+		/* convert the long-form arguments into short-form versions */
+		if (!prefixcmp(opt, "break-rewrites")) {
+			opt += strlen("break-rewrites");
+			if (*opt == 0 || *opt++ == '=')
+				cmd = 'B';
+		} else if (!prefixcmp(opt, "detect-copies")) {
+			opt += strlen("detect-copies");
+			if (*opt == 0 || *opt++ == '=')
+				cmd = 'C';
+		} else if (!prefixcmp(opt, "detect-renames")) {
+			opt += strlen("detect-renames");
+			if (*opt == 0 || *opt++ == '=')
+				cmd = 'M';
+		}
+	}
 	if (cmd != 'M' && cmd != 'C' && cmd != 'B')
 		return -1; /* that is not a -M, -C nor -B option */
 

-- 
1.7.3.72.g8af0.dirty

On Sep 27, 2010, at 4:53 PM, Jonathan Nieder wrote:

Kevin Ballard wrote:

quoted

After taking a look at this, it raises another question. -B, -M, and
-C all have optional arguments, but the long-form names don't seem
to support that.

[...]

quoted

                     if I make it support `git diff
--rename-threshold foo` that would also work, but the name doesn't
seem appropriate without the argument.

Right --- with merge-recursive the argument doesn't need to be
optional, but with git diff it does.

How about

	--detect-renames=<threshold>
	--detect-copies=<threshold>
	--detect-rewrites=<threshold>/<threshold>

Good timing, I just sent out a patch that does almost exactly this, though I went with --break-rewrites instead of --detect-rewrites.

-Kevin Ballard

Kevin Ballard wrote:

Good timing, I just sent out a patch that does almost exactly this,
though I went with --break-rewrites instead of --detect-rewrites.

Both new patches look good to me, for what it's worth (though it
would be nicer to have tests, of course :)).

On Sep 27, 2010, at 5:08 PM, Jonathan Nieder wrote:

Kevin Ballard wrote:

quoted

Good timing, I just sent out a patch that does almost exactly this,
though I went with --break-rewrites instead of --detect-rewrites.

Both new patches look good to me, for what it's worth (though it
would be nicer to have tests, of course :)).

I considered tests, and I looked at the existing ones. I am unable to find any tests that actually test setting the rename/copy score to anything other than the default, and I was a bit hesitant to add tests that simply checked to make sure the long-form option was parsed correctly, as that would just be duplicating existing tests that use the short-form arguments.

-Kevin Ballard

Kevin Ballard wrote:

I looked at the existing ones. I am unable to find any tests that
actually test setting the rename/copy score to anything other than
the default

Yep, a quick grep shows there is none.  I think the precise meaning
of the scores is subject to change, but a test for 1% should be
reliable enough. :)  (Or 0%, except that that is a magic number
with the current code.)

I can look into it tomorrow if no one else gets around to it before
then.

'night,
Jonathan

Just wanted to throw support behind this patch as it is already helping 
our project to ease merging of a project that can't/won't modify some 
files that we have extensively altered.

Thanks for doing this Kevin!

Just to note:  We are using this applied to msysgit's devel branch by 
manually removing the diff.h chunk, applying the first patch, manually 
fixing diff.h, hen applying the second patch.  Which so far has worked 
just fine.

-- 
Thell