[PATCH] rationalize diffcore-rename options and their doc

[PATCH] rationalize diffcore-rename options and their doc

[Yann Dirson]

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.

[PATCH 1/2] [RFC] Use --find- instead of --detect- as prefix for long forms of -M and -C.

[Yann Dirson]

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

Signed-off-by: Yann Dirson <ydirson@altern.org>
---
 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

[PATCH 2/2] Keep together options controlling the behaviour of diffcore-rename.

[Yann Dirson]

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 <ydirson@altern.org>
---
 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

Re: [PATCH 1/2] [RFC] Use --find- instead of --detect- as prefix for long forms of -M and -C.

[Thomas Rast]

Yann Dirson wrote:
> Rationale: this is both shorter to spell and consistent with
> --find-copies-harder.
[...]
>  -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

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

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

Re: [PATCH 1/2] [RFC] Use --find- instead of --detect- as prefix for long forms of -M and -C.

[Yann Dirson]

On Thu, Nov 11, 2010 at 11:47:04AM +0100, Thomas Rast wrote:
> Yann Dirson wrote:
> > Rationale: this is both shorter to spell and consistent with
> > --find-copies-harder.
> [...]
> >  -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
> 
> > -	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

Re: [PATCH 1/2] [RFC] Use --find- instead of --detect- as prefix for long forms of -M and -C.

[Thomas Rast]

Yann Dirson wrote:
> On Thu, Nov 11, 2010 at 11:47:04AM +0100, Thomas Rast wrote:
> > Yann Dirson wrote:
> > > Rationale: this is both shorter to spell and consistent with
> > > --find-copies-harder.
> > [...]
> > >  -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
> > 
> > > -	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

Re: [PATCH 1/2] [RFC] Use --find- instead of --detect- as prefix for long forms of -M and -C.

[Kevin Ballard]

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

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

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

Re: [PATCH 1/2] [RFC] Use --find- instead of --detect- as prefix for long forms of -M and -C.

[Miles Bader]

Kevin Ballard <kevin@sb.org> writes:
> 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.

I agree.  The current names are better than the proposed names.

[For the reasons you state.]

-Miles

-- 
The automobile has not merely taken over the street, it has dissolved the
living tissue of the city.  Its appetite for space is absolutely insatiable;
moving and parked, it devours urban land, leaving the buildings as mere
islands of habitable space in a sea of dangerous and ugly traffic.
[James Marston Fitch, New York Times, 1 May 1960]

Re: [PATCH 1/2] [RFC] Use --find- instead of --detect- as prefix for long forms of -M and -C.

[Yann Dirson]

On Thu, Nov 11, 2010 at 11:24:57PM +0100, Thomas Rast wrote:
> Yann Dirson wrote:
> > On Thu, Nov 11, 2010 at 11:47:04AM +0100, Thomas Rast wrote:
> > > Yann Dirson wrote:
> > > > Rationale: this is both shorter to spell and consistent with
> > > > --find-copies-harder.
> > > [...]
> > > >  -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
> > > 
> > > > -	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 :)

Re: [PATCH 1/2] [RFC] Use --find- instead of --detect- as prefix for long forms of -M and -C.

[Yann Dirson]

On Thu, Nov 11, 2010 at 07:00:05PM -0800, Kevin Ballard wrote:
> On Nov 10, 2010, at 12:27 PM, Yann Dirson wrote:
> 
> > -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

Re: [PATCH 2/2] Keep together options controlling the behaviour of diffcore-rename.

[Junio C Hamano]

Yann Dirson <ydirson@altern.org> writes:

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

Makes sense; thanks.

Re: [PATCH 1/2] [RFC] Use --find- instead of --detect- as prefix for long forms of -M and -C.

[Yann Dirson]

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

Re: [PATCH 1/2] [RFC] Use --find- instead of --detect- as prefix for long forms of -M and -C.

[Junio C Hamano]

Kevin Ballard <kevin@sb.org> writes:

> On Nov 10, 2010, at 12:27 PM, Yann Dirson wrote:
>
>> -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.
>
> 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.

Hmm, but your "harder is different" comes from the knowledge of how it
works (namely, the set of paths the frontend feeds to diffcore is made
larger), which does not concern the end user.  Also the same logic of
yours can be applied to argue for renaming "detect renames" to "find
renames", as it _does_ tell diffcore to activate the rename finding
machinery, i.e. it is modifying the action of computing the differences.

So I think using the same verb would make sense, either by introducing a
new synonym "detect-copies-harder", or by giving longer "find-copies" and
"find-renames" options to not-so-hard ones.

Re: [PATCH 1/2] [RFC] Use --find- instead of --detect- as prefix for long forms of -M and -C.

[Junio C Hamano]

Yann Dirson <ydirson@free.fr> writes:

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

I was hoping that I didn't have to make any official decision ;-)

For what I think, please see my other message.

Re: [PATCH 1/2] [RFC] Use --find- instead of --detect- as prefix for long forms of -M and -C.

[Kevin Ballard]

On Nov 29, 2010, at 2:42 PM, Junio C Hamano 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.
> 
> Hmm, but your "harder is different" comes from the knowledge of how it
> works (namely, the set of paths the frontend feeds to diffcore is made
> larger), which does not concern the end user.  Also the same logic of
> yours can be applied to argue for renaming "detect renames" to "find
> renames", as it _does_ tell diffcore to activate the rename finding
> machinery, i.e. it is modifying the action of computing the differences.

I will admit I am not an unbiased observer, but I still think
--find-copies-harder is less of a directive and more of a modifier than
--find-copies is.

> So I think using the same verb would make sense, either by introducing a
> new synonym "detect-copies-harder", or by giving longer "find-copies" and
> "find-renames" options to not-so-hard ones.

Given my druthers, I'd prefer to go with --detect-copies-harder, but at this
point I don't have as much of a strong preference anymore.

-Kevin Ballard

-- 8< --
Subject: [PATCH] diff: add --detect-copies-harder as a synonym for --find-copies-harder


Signed-off-by: Kevin Ballard <kevin@sb.org>
---
 Documentation/diff-options.txt |    1 +
 diff.c                         |    2 +-
 2 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index f3e9538..7246e10 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -251,6 +251,7 @@ endif::git-log[]
 	If `n` is specified, it has the same meaning as for `-M<n>`.
 
 --find-copies-harder::
+--detect-copies-harder::
 	For performance reasons, by default, `-C` option finds copies only
 	if the original file of the copy was modified in the same
 	changeset.  This flag makes the command
diff --git a/diff.c b/diff.c
index 6991ed4..faa8dc0 100644
--- a/diff.c
+++ b/diff.c
@@ -3198,7 +3198,7 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 		DIFF_OPT_SET(options, TEXT);
 	else if (!strcmp(arg, "-R"))
 		DIFF_OPT_SET(options, REVERSE_DIFF);
-	else if (!strcmp(arg, "--find-copies-harder"))
+	else if (!strcmp(arg, "--find-copies-harder") || !strcmp(arg, "--detect-copies-harder"))
 		DIFF_OPT_SET(options, FIND_COPIES_HARDER);
 	else if (!strcmp(arg, "--follow"))
 		DIFF_OPT_SET(options, FOLLOW_RENAMES);
-- 
1.7.3.2.615.g83f72.dirty

Re: [PATCH 1/2] [RFC] Use --find- instead of --detect- as prefix for long forms of -M and -C.

[Junio C Hamano]

Kevin Ballard <kevin@sb.org> writes:

>> So I think using the same verb would make sense, either by introducing a
>> new synonym "detect-copies-harder", or by giving longer "find-copies" and
>> "find-renames" options to not-so-hard ones.
>
> Given my druthers, I'd prefer to go with --detect-copies-harder, but at this
> point I don't have as much of a strong preference anymore.

The patch is smaller, and we often say "rename detection", not "rename
finding", so it may probably make more sense.

Thanks.

> -Kevin Ballard
>
> -- 8< --
> Subject: [PATCH] diff: add --detect-copies-harder as a synonym for --find-copies-harder
>
>
> Signed-off-by: Kevin Ballard <kevin@sb.org>
> ---
>  Documentation/diff-options.txt |    1 +
>  diff.c                         |    2 +-
>  2 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
> index f3e9538..7246e10 100644
> --- a/Documentation/diff-options.txt
> +++ b/Documentation/diff-options.txt
> @@ -251,6 +251,7 @@ endif::git-log[]
>  	If `n` is specified, it has the same meaning as for `-M<n>`.
>  
>  --find-copies-harder::
> +--detect-copies-harder::
>  	For performance reasons, by default, `-C` option finds copies only
>  	if the original file of the copy was modified in the same
>  	changeset.  This flag makes the command
> diff --git a/diff.c b/diff.c
> index 6991ed4..faa8dc0 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -3198,7 +3198,7 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
>  		DIFF_OPT_SET(options, TEXT);
>  	else if (!strcmp(arg, "-R"))
>  		DIFF_OPT_SET(options, REVERSE_DIFF);
> -	else if (!strcmp(arg, "--find-copies-harder"))
> +	else if (!strcmp(arg, "--find-copies-harder") || !strcmp(arg, "--detect-copies-harder"))
>  		DIFF_OPT_SET(options, FIND_COPIES_HARDER);
>  	else if (!strcmp(arg, "--follow"))
>  		DIFF_OPT_SET(options, FOLLOW_RENAMES);
> -- 
> 1.7.3.2.615.g83f72.dirty

Re: [PATCH 1/2] [RFC] Use --find- instead of --detect- as prefix for

[Yann Dirson]

Junio:
> Kevin wrote:
> > Given my druthers, I'd prefer to go with --detect-copies-harder, but
> > at this point I don't have as much of a strong preference anymore.
>
> The patch is smaller, and we often say "rename detection", not "rename
> finding", so it may probably make more sense.

OTOH:

* when cumulated to the still-recent patch adding the long forms, not
  using a synonym is shorter

* if it is just "adding a synonym" without deprecation, it only
  adds a new option for little gain; OTOH deprecating such an old
  option may not be such a good idea (not to mention it would make the
  patch longer ;)

* The --find-* forms are still shorter to spell

-- 
Yann Dirson - Bertin Technologies