[PATCH] rationalize diffcore-rename options and their doc

I am ready to take the blame for being the first to introduce
--detect-* options in diff-opts, with the directory-rename stuff.
However, since --find-copies-harder predates everything and is the
only one to be part of a release today, I'd think it would be much
more consistent to use --find- as a common prefix.  And, last but not
least, shorter long options do not hurt.

At the same time, I noticed the manpage could benefit from a small
improvement.

Rationale: this is both shorter to spell and consistent with
--find-copies-harder.

Signed-off-by: Yann Dirson <redacted>
---
 Documentation/diff-options.txt |    4 ++--
 diff.c                         |    8 ++++----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index bfd0b57..ed9c44e 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt

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

@@ -246,7 +246,7 @@ endif::git-log[]
 	hasn't changed.
 
 -C[<n>]::
---detect-copies[=<n>]::
+--find-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 d1c6b91..3837ffd 100644
--- a/diff.c
+++ b/diff.c

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

-- 
1.7.2.3

It makes little sense to have --diff-filter in the middle of them, and
even spares an ifndef::git-format-patch.

Signed-off-by: Yann Dirson <redacted>
---
 Documentation/diff-options.txt |   26 ++++++++++++--------------
 1 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index ed9c44e..c93124b 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt

@@ -250,20 +250,6 @@ endif::git-log[]
 	Detect copies as well as renames.  See also `--find-copies-harder`.
 	If `n` is specified, it has the same meaning as for `-M<n>`.
 
-ifndef::git-format-patch[]
---diff-filter=[(A|C|D|M|R|T|U|X|B)...[*]]::
-	Select only files that are Added (`A`), Copied (`C`),
-	Deleted (`D`), Modified (`M`), Renamed (`R`), have their
-	type (i.e. regular file, symlink, submodule, ...) changed (`T`),
-	are Unmerged (`U`), are
-	Unknown (`X`), or have had their pairing Broken (`B`).
-	Any combination of the filter characters (including none) can be used.
-	When `*` (All-or-none) is added to the combination, all
-	paths are selected if there is any file that matches
-	other criteria in the comparison; if there is no file
-	that matches other criteria, nothing is selected.
-endif::git-format-patch[]
-
 --find-copies-harder::
 	For performance reasons, by default, `-C` option finds copies only
 	if the original file of the copy was modified in the same

@@ -281,6 +267,18 @@ endif::git-format-patch[]
 	number.
 
 ifndef::git-format-patch[]
+--diff-filter=[(A|C|D|M|R|T|U|X|B)...[*]]::
+	Select only files that are Added (`A`), Copied (`C`),
+	Deleted (`D`), Modified (`M`), Renamed (`R`), have their
+	type (i.e. regular file, symlink, submodule, ...) changed (`T`),
+	are Unmerged (`U`), are
+	Unknown (`X`), or have had their pairing Broken (`B`).
+	Any combination of the filter characters (including none) can be used.
+	When `*` (All-or-none) is added to the combination, all
+	paths are selected if there is any file that matches
+	other criteria in the comparison; if there is no file
+	that matches other criteria, nothing is selected.
+
 -S<string>::
 	Look for differences that introduce or remove an instance of
 	<string>. Note that this is different than the string simply

-- 
1.7.2.3

Yann Dirson wrote:

Rationale: this is both shorter to spell and consistent with
--find-copies-harder.

[...]

Umm.  The reasoning seems ok for me, but the farthest you can go is
deprecating the options.  Removing them as in

would break backwards compatibility.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

On Thu, Nov 11, 2010 at 11:47:04AM +0100, Thomas Rast wrote:

Yann Dirson wrote:

quoted

Rationale: this is both shorter to spell and consistent with
--find-copies-harder.

[...]

quoted

 -M[<n>]::
---detect-renames[=<n>]::
+--find-renames[=<n>]::

Umm.  The reasoning seems ok for me, but the farthest you can go is
deprecating the options.  Removing them as in

quoted

-	else if (!prefixcmp(arg, "-M") || !prefixcmp(arg, "--detect-renames=") ||
-		 !strcmp(arg, "--detect-renames")) {
+	else if (!prefixcmp(arg, "-M") || !prefixcmp(arg, "--find-renames=") ||
+		 !strcmp(arg, "--find-renames")) {

would break backwards compatibility.

I don't think we care with compatibility here, since those are not
part of any release.

-- 
Yann

Yann Dirson wrote:

On Thu, Nov 11, 2010 at 11:47:04AM +0100, Thomas Rast wrote:

quoted

Yann Dirson wrote:

quoted

Rationale: this is both shorter to spell and consistent with
--find-copies-harder.

[...]

quoted

 -M[<n>]::
---detect-renames[=<n>]::
+--find-renames[=<n>]::

Umm.  The reasoning seems ok for me, but the farthest you can go is
deprecating the options.  Removing them as in

quoted

-	else if (!prefixcmp(arg, "-M") || !prefixcmp(arg, "--detect-renames=") ||
-		 !strcmp(arg, "--detect-renames")) {
+	else if (!prefixcmp(arg, "-M") || !prefixcmp(arg, "--find-renames=") ||
+		 !strcmp(arg, "--find-renames")) {

would break backwards compatibility.

I don't think we care with compatibility here, since those are not
part of any release.

Ah well.  You're right of course, but you could have mentioned that
somewhere :-)

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

On Nov 10, 2010, at 12:27 PM, Yann Dirson wrote:

I'm not sure I like the wording --find-copies and --find-renames. Maybe I'm
just being silly, but it sounds like those are directives, saying "I want you
to find copies/renames", as opposed to just saying "while you're working you
should also detect copies/renames". The original flag --find-copies-harder
is a bit different, because it's modifying the action of finding copies
rather than making finding copies the prime directive.

On the other hand, --detect-copies and --detect-renames sounds to me like
you're just telling it that it should, well, detect copies/renames as it goes
about its business.

-Kevin Ballard

On Thu, Nov 11, 2010 at 11:24:57PM +0100, Thomas Rast wrote:

Yann Dirson wrote:

quoted

On Thu, Nov 11, 2010 at 11:47:04AM +0100, Thomas Rast wrote:

quoted

Yann Dirson wrote:

quoted

Rationale: this is both shorter to spell and consistent with
--find-copies-harder.

[...]

quoted

 -M[<n>]::
---detect-renames[=<n>]::
+--find-renames[=<n>]::

Umm.  The reasoning seems ok for me, but the farthest you can go is
deprecating the options.  Removing them as in

quoted

-	else if (!prefixcmp(arg, "-M") || !prefixcmp(arg, "--detect-renames=") ||
-		 !strcmp(arg, "--detect-renames")) {
+	else if (!prefixcmp(arg, "-M") || !prefixcmp(arg, "--find-renames=") ||
+		 !strcmp(arg, "--find-renames")) {

would break backwards compatibility.

I don't think we care with compatibility here, since those are not
part of any release.

Ah well.  You're right of course, but you could have mentioned that
somewhere :-)

Ah, I was sure I did, but apprently not :)

On Thu, Nov 11, 2010 at 07:00:05PM -0800, Kevin Ballard wrote:

On Nov 10, 2010, at 12:27 PM, Yann Dirson wrote:

quoted

-M[<n>]::
---detect-renames[=<n>]::
+--find-renames[=<n>]::
ifndef::git-log[]
	Detect renames.
endif::git-log[]

@@ -246,7 +246,7 @@ endif::git-log[]

	hasn't changed.

-C[<n>]::
---detect-copies[=<n>]::
+--find-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>`.

I'm not sure I like the wording --find-copies and --find-renames. Maybe I'm
just being silly, but it sounds like those are directives, saying "I want you
to find copies/renames", as opposed to just saying "while you're working you
should also detect copies/renames". The original flag --find-copies-harder
is a bit different, because it's modifying the action of finding copies
rather than making finding copies the prime directive.

Well, I don't see how --find-copies-harder is much different: it is
just a more powerful version of -C, as seen by the fact that it implies -C.

On the other hand, --detect-copies and --detect-renames sounds to me like
you're just telling it that it should, well, detect copies/renames as it goes
about its business.

I can understand this.  However, I feel that the fact they are just
options, as opposed to the explicit "diff/show/whatever" commands that
take them as modifiers, would be enough to balance the nuance in the
words.  That may just be a matter of taste, but the consistency with
--find-copies-harder may be important here.

-- 
Yann

So is there an official decision that this idea was a bad one and
should I drop this patch from my outq ?