threads / patch / 25703
Subject: [PATCH] rationalize diffcore-rename options and their doc
Yann DirsonI 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.
Yann Dirson[PATCH 1/2] [RFC] Use --find- instead of --detect- as prefix for long forms of -M and -C.
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(-)
Documentation/diff-options.txt, diff.c
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
Thomas RastRe: [PATCH 1/2] [RFC] Use --find- instead of --detect- as prefix for long forms of -M and -C.
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.chRe: [PATCH 1/2] [RFC] Use --find- instead of --detect- as prefix for long forms of -M and -C.
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
Thomas RastRe: [PATCH 1/2] [RFC] Use --find- instead of --detect- as prefix for long forms of -M and -C.
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.chRe: [PATCH 1/2] [RFC] Use --find- instead of --detect- as prefix for long forms of -M and -C.
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 :)
Kevin BallardRe: [PATCH 1/2] [RFC] Use --find- instead of --detect- as prefix for long forms of -M and -C.
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
Miles BaderRe: [PATCH 1/2] [RFC] Use --find- instead of --detect- as prefix for long forms of -M and -C.
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.
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
Junio C HamanoRe: [PATCH 1/2] [RFC] Use --find- instead of --detect- as prefix for long forms of -M and -C.
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.
Kevin BallardRe: [PATCH 1/2] [RFC] Use --find- instead of --detect- as prefix for long forms of -M and -C.
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(-)
Documentation/diff-options.txt, diff.c
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
Junio C HamanoRe: [PATCH 1/2] [RFC] Use --find- instead of --detect- as prefix for long forms of -M and -C.
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
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
Re: [PATCH 1/2] [RFC] Use --find- instead of --detect- as prefix for long forms of -M and -C.
So is there an official decision that this idea was a bad one and should I drop this patch from my outq ?
Junio C HamanoRe: [PATCH 1/2] [RFC] Use --find- instead of --detect- as prefix for long forms of -M and -C.
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.
Yann Dirson[PATCH 2/2] Keep together options controlling the behaviour of diffcore-rename.
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
Junio C HamanoRe: [PATCH 2/2] Keep together options controlling the behaviour of diffcore-rename.
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.