[v3,1/1] diff: --patch-{modifies,grep} arg names for -S and -G

Commit Message

Illia Bobyr Feb. 6, 2025, 1:43 a.m. UTC

Most arguments have both short and long versions.  Long versions are
easier to read, especially in scripts and command history.

Tests that check just the option parsing are duplicated to check both
short and long argument options.  But more complex tests are updated to
use the long argument in order to improve the test readability.
Assuming that the usage tests have already verified that both arguments
invoke the same underlying functionality.

Signed-off-by: Illia Bobyr <[e-mail]>
---
 Documentation/diff-options.txt |  36 +++++------
 Documentation/git-blame.txt    |   2 +-
 Documentation/gitdiffcore.txt  |  48 ++++++++-------
 diff.c                         |  18 +++---
 diff.h                         |  11 +++-
 t/t4062-diff-pickaxe.sh        |   8 +--
 t/t4209-log-pickaxe.sh         | 106 +++++++++++++++++++++++----------
 7 files changed, 142 insertions(+), 87 deletions(-)

Comments

Junio C Hamano Feb. 6, 2025, 8:59 p.m. UTC | #1

Illia Bobyr <[e-mail]> writes:

> Most arguments have both short and long versions.  Long versions are
> easier to read, especially in scripts and command history.
>
> Tests that check just the option parsing are duplicated to check both
> short and long argument options.  But more complex tests are updated to
> use the long argument in order to improve the test readability.

While checking both may be a prudent thing to do, because the "-S"
option and the "-G" option have been there with us almost since the
beginning of time, the swapping all existing use of them with the
longhand is rather unwelcome and needless churn, I would have to
say.

>  `-S<string>`::
> +`--patch-modifies=<string>`::

Good.  I am looking at 'git-commit.txt' as an example, and we seem
to give shorthand first and then longhand, which matches what we see
here.

> @@ -657,18 +658,19 @@ renamed entries cannot appear if detection for those types is disabled.
>  It is useful when you're looking for an exact block of code (like a
>  struct), and want to know the history of that block since it first
>  came into being: use the feature iteratively to feed the interesting
> -block in the preimage back into `-S`, and keep going until you get the
> -very first version of the block.
> +block in the preimage back into `--patch-modifies`, and keep going until
> +you get the very first version of the block.

If this paragraph _were_ written with the longhand from the
beginning, I would not have minded too much, but I personally find
it unnecessary to churn the existing document like this.

>  `-G<regex>`::
> +`--patch-grep=<regex>`::

Same two paragraphs from the above apply here, and ...

>  `--find-object=<object-id>`::
>  `--pickaxe-all`::
>  `--pickaxe-regex`::

... all of the above.

> diff --git a/Documentation/git-blame.txt b/Documentation/git-blame.txt

Ditto.

> diff --git a/Documentation/gitdiffcore.txt b/Documentation/gitdiffcore.txt
> index 642c5..e4b18 100644
> --- a/Documentation/gitdiffcore.txt
> +++ b/Documentation/gitdiffcore.txt
> @@ -245,33 +245,35 @@ diffcore-pickaxe: For Detecting Addition/Deletion of Specified String
>  
>  This transformation limits the set of filepairs to those that change
>  specified strings between the preimage and the postimage in a certain
> -way.  -S<block-of-text> and -G<regular-expression> options are used to
> +way.  --patch-modifies=<block-of-text> and
> +--patch-grep=<regular-expression> options are used to specify
> +different ways these strings are sought.

This is worse.  Here is the first part that describes the pickaxe,
so mentioning both may be more appropriate; showing only the
longhand nobody is familiar with (yet) does not make any sense.

    ... certain way.  `--patch-modifies=<block-of-text>`
    (`-S<block-of-text>` for short) and `--patch-grep=<regular-expression>`
    (`-G<regular-expression>` for short) are used to ...

Once establishing the equivalence between the longhand and the
shorthand for these two options, we do not have to churn the
existing text at all.

> diff --git a/diff.c b/diff.c
> index d28b41..09beb 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -4877,15 +4877,17 @@ void diff_setup_done(struct diff_options *options)
>  
>  	if (HAS_MULTI_BITS(options->pickaxe_opts & DIFF_PICKAXE_KINDS_MASK))
>  		die(_("options '%s', '%s', and '%s' cannot be used together"),
> -			"-G", "-S", "--find-object");
> +			"-G/--patch-grep", "-S/--patch-modifies", "--find-object");
>  
>  	if (HAS_MULTI_BITS(options->pickaxe_opts & DIFF_PICKAXE_KINDS_G_REGEX_MASK))
>  		die(_("options '%s' and '%s' cannot be used together, use '%s' with '%s'"),
> -			"-G", "--pickaxe-regex", "--pickaxe-regex", "-S");
> +			"-G/--patch-grep", "--pickaxe-regex",
> +                        "--pickaxe-regex", "-S/--patch-modifies");
>  
>  	if (HAS_MULTI_BITS(options->pickaxe_opts & DIFF_PICKAXE_KINDS_ALL_OBJFIND_MASK))
>  		die(_("options '%s' and '%s' cannot be used together, use '%s' with '%s' and '%s'"),
> -			"--pickaxe-all", "--find-object", "--pickaxe-all", "-G", "-S");
> +			"--pickaxe-all", "--find-object",
> +                        "--pickaxe-all", "-G/--patch-grep", "-S/--patch-modifies");

The message change looks fine; the indentation is broken.

.git/rebase-apply/patch:184: indent with spaces.
                        "--pickaxe-regex", "-S/--patch-modifies");
.git/rebase-apply/patch:190: indent with spaces.
                        "--pickaxe-all", "-G/--patch-grep", "-S/--patch-modifies");
warning: 2 lines applied after fixing whitespace errors.
Applying: diff: --patch-{modifies,grep} arg names for -S and -G

These alone do not require a new iteration, as "git am --whitespace=fix"
already corrected them.

> -		OPT_CALLBACK_F('S', NULL, options, N_("<string>"),
> +		OPT_CALLBACK_F('S', "patch-modifies", options, N_("<string>"),
> -		OPT_CALLBACK_F('G', NULL, options, N_("<regex>"),
> +		OPT_CALLBACK_F('G', "patch-grep", options, N_("<regex>"),

OK.  NOte that this says <regex>.  We may want to have a separate clean-up
patch so that Documentation/gitdifcore.txt that used <regular-expression>
and the placeholder used here match.

> -			       N_("look for differences that change the number of occurrences of the specified regex"),
> +			       N_("look for differences where a patch contains the specified regex"),

This is an unrelated change that should not be in this patch.  If
you want to modify it, please do it in a separate clean-up patch,
just like the above <regex> vs <regular-expression> change.

> -			  N_("show all changes in the changeset with -S or -G"),
> +			  N_("show all changes in the changeset with -S/--patch-modifies or -G/--patch-grep"),

This line is meant to be shown when the user requests list of
options and their meanings.  Growing the message from 47 columns or
so to 78 columns would make it wider than terminals when these
messages are indented.  Because earlier entries in this array have
already established the equivalence between the shorthand and the
longhand, I do not think the output is understandable without this
change.

>  		OPT_BIT_F(0, "pickaxe-regex", &options->pickaxe_opts,
> -			  N_("treat <string> in -S as extended POSIX regular expression"),
> +			  N_("treat <string> in -S/--patch-modifies as extended POSIX regular expression"),

Ditto.

Thanks.

Illia Bobyr Feb. 12, 2025, 3:26 a.m. UTC | #2

On 2/6/25 12:59, Junio C Hamano wrote:
 > Illia Bobyr <[e-mail]> writes:
 >
 >> Most arguments have both short and long versions.  Long versions are
 >> easier to read, especially in scripts and command history.
 >>
 >> Tests that check just the option parsing are duplicated to check both
 >> short and long argument options.  But more complex tests are updated to
 >> use the long argument in order to improve the test readability.
 >
 > While checking both may be a prudent thing to do, because the "-S"
 > option and the "-G" option have been there with us almost since the
 > beginning of time, the swapping all existing use of them with the
 > longhand is rather unwelcome and needless churn, I would have to
 > say.

My thinking is that as long version names improve readability, it also 
applies
to the test code.  When I see a short option, I often have to check the 
manual
to remember what exactly does it do.  Even for "-S"/"-G" I find myself 
sometimes
confused as to which of the two does what exactly.  While the "grep" 
mnemonic
helps, I do not always remember it.

But, I think, I understand your point of view as well.

In v5, patch 5 contains a relatively minimum amount of changes that add long
alternatives for "-S" and "-G" just to the command line parsing.

I do not have your experience with assessing the churn, but if my 
argument about
the readability changes your mind, I've moved the rest of the updates into
separate patches, at the end of the chain.  Patches 8 through 10. Making it
easier to discuss them in smaller chunks, if you wish so.  But also, I 
assume,
it should be easy for you to ignore those, if you do not want to include 
them?

 > [...]
 >
 >> diff --git a/Documentation/gitdiffcore.txt 
b/Documentation/gitdiffcore.txt
 >> index 642c5..e4b18 100644
 >> --- a/Documentation/gitdiffcore.txt
 >> +++ b/Documentation/gitdiffcore.txt
 >> @@ -245,33 +245,35 @@ diffcore-pickaxe: For Detecting 
Addition/Deletion of Specified String
 >>
 >>  This transformation limits the set of filepairs to those that change
 >>  specified strings between the preimage and the postimage in a certain
 >> -way.  -S<block-of-text> and -G<regular-expression> options are used to
 >> +way.  --patch-modifies=<block-of-text> and
 >> +--patch-grep=<regular-expression> options are used to specify
 >> +different ways these strings are sought.
 >
 > This is worse.  Here is the first part that describes the pickaxe,
 > so mentioning both may be more appropriate; showing only the
 > longhand nobody is familiar with (yet) does not make any sense.
 >
 >     ... certain way. `--patch-modifies=<block-of-text>`
 >     (`-S<block-of-text>` for short) and 
`--patch-grep=<regular-expression>`
 >     (`-G<regular-expression>` for short) are used to ...
 >
 > Once establishing the equivalence between the longhand and the
 > shorthand for these two options, we do not have to churn the
 > existing text at all.

Applied your suggestion.

I guess one difference in the way you look at it, is that you default to the
short version when you can.  While I default to the long one, as I 
assume it is
easier to understand.  Someone not that intimately familiar with git 
might need
to go to the previous paragraph to recall what "-S" and "-G" are, while 
if they
are spelled as "--patch-modifies" and "--patch-grep", it might be less
necessary.  So, the argument is that while we are reducing the diff, we 
might
also be reducing the improvement in readability.

Being the author, I could also be biased when assessing how much more 
readable
"--patch-modifies" and "--patch-grep" are compared to "-S" and "-G".

 >> diff --git a/diff.c b/diff.c
 >> index d28b41..09beb 100644
 >> --- a/diff.c
 >> +++ b/diff.c
 >> @@ -4877,15 +4877,17 @@ void diff_setup_done(struct diff_options 
*options)
 >>
 >>      if (HAS_MULTI_BITS(options->pickaxe_opts & 
DIFF_PICKAXE_KINDS_MASK))
 >>          die(_("options '%s', '%s', and '%s' cannot be used together"),
 >> -            "-G", "-S", "--find-object");
 >> +            "-G/--patch-grep", "-S/--patch-modifies", "--find-object");
 >>
 >>      if (HAS_MULTI_BITS(options->pickaxe_opts & 
DIFF_PICKAXE_KINDS_G_REGEX_MASK))
 >>          die(_("options '%s' and '%s' cannot be used together, use 
'%s' with '%s'"),
 >> -            "-G", "--pickaxe-regex", "--pickaxe-regex", "-S");
 >> +            "-G/--patch-grep", "--pickaxe-regex",
 >> +                        "--pickaxe-regex", "-S/--patch-modifies");
 >>
 >>      if (HAS_MULTI_BITS(options->pickaxe_opts & 
DIFF_PICKAXE_KINDS_ALL_OBJFIND_MASK))
 >>          die(_("options '%s' and '%s' cannot be used together, use 
'%s' with '%s' and '%s'"),
 >> -            "--pickaxe-all", "--find-object", "--pickaxe-all", 
"-G", "-S");
 >> +            "--pickaxe-all", "--find-object",
 >> +                        "--pickaxe-all", "-G/--patch-grep", 
"-S/--patch-modifies");
 >
 > The message change looks fine; the indentation is broken.
 >
 > .git/rebase-apply/patch:184: indent with spaces.
 >                         "--pickaxe-regex", "-S/--patch-modifies");
 > .git/rebase-apply/patch:190: indent with spaces.
 >                         "--pickaxe-all", "-G/--patch-grep", 
"-S/--patch-modifies");
 > warning: 2 lines applied after fixing whitespace errors.
 > Applying: diff: --patch-{modifies,grep} arg names for -S and -G
 >
 > These alone do not require a new iteration, as "git am --whitespace=fix"
 > already corrected them.

Sorry about this.  I did check the indentation manually, but did not use a
tool.  Reconfigured my editor to use tabs now.

 >> -        OPT_CALLBACK_F('S', NULL, options, N_("<string>"),
 >> +        OPT_CALLBACK_F('S', "patch-modifies", options, N_("<string>"),
 >> -        OPT_CALLBACK_F('G', NULL, options, N_("<regex>"),
 >> +        OPT_CALLBACK_F('G', "patch-grep", options, N_("<regex>"),
 >
 > OK.  NOte that this says <regex>.  We may want to have a separate 
clean-up
 > patch so that Documentation/gitdifcore.txt that used <regular-expression>
 > and the placeholder used here match.

Makes sense.
I've added this fix as patch 5 in v5.
I've reformatted paragraphs in gitdifcore.adoc that were affected.
Let me know if you do not want me to reformat it, and just keep shorter 
lines as
is.

 >> -                   N_("look for differences that change the number 
of occurrences of the specified regex"),
 >> +                   N_("look for differences where a patch contains 
the specified regex"),
 >
 > This is an unrelated change that should not be in this patch.  If
 > you want to modify it, please do it in a separate clean-up patch,
 > just like the above <regex> vs <regular-expression> change.

Split it into patch 2 in v5.

 >> -              N_("show all changes in the changeset with -S or -G"),
 >> +              N_("show all changes in the changeset with 
-S/--patch-modifies or -G/--patch-grep"),
 >
 > This line is meant to be shown when the user requests list of
 > options and their meanings.  Growing the message from 47 columns or
 > so to 78 columns would make it wider than terminals when these
 > messages are indented.  Because earlier entries in this array have
 > already established the equivalence between the shorthand and the
 > longhand, I do not think the output is understandable without this
 > change.

A description for "-S" is already 81 characters long:

N_("look for differences that change the number of occurrences of the 
specified regex"),

So I was assuming if I grow another description to 77 characters, it is 
still OK.
While one can find the correspondence between "-S" and "--patch-modifies" by
reading the "-S" description, in my mind, the same argument applies as 
to the
test readability - it just makes it a bit easier for the reader.

This change is now part of a much smaller patch 9 in v5, which is only about
adding longer alternatives to the CLI help messages that currently 
contain only
"-G" and "-S".  This way you can decide if you want it or not as a complete
unit.  Or if you want me to change it in some way, we can discuss it 
separately
from the rest of the changes.

By the way, I must admit I can not find a way to look at a help message
generated from these strings.  Running `git diff -h` shows a message from

`diff.h` and running `git diff --help` shows the man page.

Thank you.

Junio C Hamano Feb. 12, 2025, 5:08 p.m. UTC | #3

Illia Bobyr <[e-mail]> writes:

> My thinking is that as long version names improve readability, it also
> applies
> to the test code.  When I see a short option, I often have to check
> the manual
> to remember what exactly does it do.

But by now due to enough exposure, you have committed them in your
memory, no? ;-)

> But, I think, I understand your point of view as well.

Yup, if the options were introduced with long and short forms at the
same time and the tests were written at the same time or shortly
after their introduction, I'd agree that using longer form more may
be beneficial, since there is nobody who is already familier with
either of the forms.  But at this point after 20 years, swapping one
for the other is mostly unnecessary churn, I would have to say (and
I do not particularly want to having to repeat saying the same thing
again).

>> OK.  NOte that this says <regex>.  We may want to have a separate
>   clean-up
>> patch so that Documentation/gitdifcore.txt that used <regular-expression>
>> and the placeholder used here match.
>
> Makes sense.
> I've added this fix as patch 5 in v5.

I'd rather see these "fixes to existing anomalies" done totally
outside of this series.  IOW, I'd prefer to either (1) get the
series done with the minimally necessary changes first and then
after the dust settles from merging that to 'master', see these "oh
we noticed these issues while working on the other series that has
now completed" issues addressed, or (2) do the clean-up of existing
anomalies first as a separate series, and then after the dust
settles for the clean-up, do the proposed addition of longform as a
separate series.  I have slight preference to (1), simply because
nobody complained on these small anomalies for the past 20 years ;-)
but I can also go with "preliminary clean-up first" route.

>> This is an unrelated change that should not be in this patch.  If
>> you want to modify it, please do it in a separate clean-up patch,
>> just like the above <regex> vs <regular-expression> change.
>
> Split it into patch 2 in v5.

Again, when I said "unrelated", I meant that I want them to be
treated as unrelated changes, addressed outside of this series,
either in a preliminary clean-up, or after-the-dust-settles
clean-up.

Thanks.

Patch

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 640eb..c9f7c 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -650,6 +650,7 @@  Note that not all diffs can feature all types. For instance, copied and
 renamed entries cannot appear if detection for those types is disabled.
 
 `-S<string>`::
+`--patch-modifies=<string>`::
 	Look for differences that change the number of occurrences of
 	the specified _<string>_ (i.e. addition/deletion) in a file.
 	Intended for the scripter's use.
@@ -657,18 +658,19 @@  renamed entries cannot appear if detection for those types is disabled.
 It is useful when you're looking for an exact block of code (like a
 struct), and want to know the history of that block since it first
 came into being: use the feature iteratively to feed the interesting
-block in the preimage back into `-S`, and keep going until you get the
-very first version of the block.
+block in the preimage back into `--patch-modifies`, and keep going until
+you get the very first version of the block.
 +
 Binary files are searched as well.
 
 `-G<regex>`::
+`--patch-grep=<regex>`::
 	Look for differences whose patch text contains added/removed
 	lines that match _<regex>_.
 +
-To illustrate the difference between `-S<regex>` `--pickaxe-regex` and
-`-G<regex>`, consider a commit with the following diff in the same
-file:
+To illustrate the difference between `--patch-modifies=<regex>
+--pickaxe-regex` and `--patch-grep=<regex>`, consider a commit with the
+following diff in the same file:
 +
 ----
 +    return frotz(nitfol, two->ptr, 1, 0);
@@ -676,9 +678,9 @@  file:
 -    hit = frotz(nitfol, mf2.ptr, 1, 0);
 ----
 +
-While `git log -G"frotz\(nitfol"` will show this commit, `git log
--S"frotz\(nitfol" --pickaxe-regex` will not (because the number of
-occurrences of that string did not change).
+While `git log --patch-grep="frotz\(nitfol"` will show this commit, `git
+log --patch-modifies="frotz\(nitfol" --pickaxe-regex` will not (because the
+number of occurrences of that string did not change).
 +
 Unless `--text` is supplied patches of binary files without a textconv
 filter will be ignored.
@@ -687,22 +689,22 @@  See the 'pickaxe' entry in linkgit:gitdiffcore[7] for more
 information.
 
 `--find-object=<object-id>`::
-	Look for differences that change the number of occurrences of
-	the specified object. Similar to `-S`, just the argument is different
-	in that it doesn't search for a specific string but for a specific
-	object id.
+	Look for differences that change the number of occurrences of the
+	specified object. Similar to `--patch-modifies`, just the argument
+	is different in that it doesn't search for a specific string but
+	for a specific object id.
 +
 The object can be a blob or a submodule commit. It implies the `-t` option in
 `git-log` to also find trees.
 
 `--pickaxe-all`::
-	When `-S` or `-G` finds a change, show all the changes in that
-	changeset, not just the files that contain the change
-	in _<string>_.
+	When `--patch-modifies` or `--patch-grep` finds a change, show all
+	the changes in that changeset, not just the files that contain the
+	change in _<string>_.
 
 `--pickaxe-regex`::
-	Treat the _<string>_ given to `-S` as an extended POSIX regular
-	expression to match.
+	Treat the _<string>_ given to `--patch-modifies` as an extended
+	POSIX regular expression to match.
 
 endif::git-format-patch[]
 
diff --git a/Documentation/git-blame.txt b/Documentation/git-blame.txt
index b1d7fb..0f21d3 100644
--- a/Documentation/git-blame.txt
+++ b/Documentation/git-blame.txt
@@ -41,7 +41,7 @@  a text string in the diff. A small example of the pickaxe interface
 that searches for `blame_usage`:
 
 -----------------------------------------------------------------------------
-$ git log --pretty=oneline -S'blame_usage'
+$ git log --pretty=oneline --patch-modifies='blame_usage'
 5040f17eba15504bad66b14a645bddd9b015ebb7 blame -S <ancestry-file>
 ea4c7f9bf69e781dd0cd88d2bccb2bf5cc15c9a7 git-blame: Make the output
 -----------------------------------------------------------------------------
diff --git a/Documentation/gitdiffcore.txt b/Documentation/gitdiffcore.txt
index 642c5..e4b18 100644
--- a/Documentation/gitdiffcore.txt
+++ b/Documentation/gitdiffcore.txt
@@ -245,33 +245,35 @@  diffcore-pickaxe: For Detecting Addition/Deletion of Specified String
 
 This transformation limits the set of filepairs to those that change
 specified strings between the preimage and the postimage in a certain
-way.  -S<block-of-text> and -G<regular-expression> options are used to
-specify different ways these strings are sought.
-
-"-S<block-of-text>" detects filepairs whose preimage and postimage
-have different number of occurrences of the specified block of text.
-By definition, it will not detect in-file moves.  Also, when a
-changeset moves a file wholesale without affecting the interesting
-string, diffcore-rename kicks in as usual, and `-S` omits the filepair
-(since the number of occurrences of that string didn't change in that
+way.  --patch-modifies=<block-of-text> and
+--patch-grep=<regular-expression> options are used to specify
+different ways these strings are sought.
+
+"-S<block-of-text>", or "--patch-modifies=<block-of-text>" detects
+filepairs whose preimage and postimage have different number of
+occurrences of the specified block of text.  By definition, it will
+not detect in-file moves.  Also, when a changeset moves a file
+wholesale without affecting the interesting string, diffcore-rename
+kicks in as usual, and `--patch-modifies` omits the filepair (since
+the number of occurrences of that string didn't change in that
 rename-detected filepair).  When used with `--pickaxe-regex`, treat
 the <block-of-text> as an extended POSIX regular expression to match,
 instead of a literal string.
 
-"-G<regular-expression>" (mnemonic: grep) detects filepairs whose
-textual diff has an added or a deleted line that matches the given
-regular expression.  This means that it will detect in-file (or what
-rename-detection considers the same file) moves, which is noise.  The
-implementation runs diff twice and greps, and this can be quite
-expensive.  To speed things up, binary files without textconv filters
-will be ignored.
-
-When `-S` or `-G` are used without `--pickaxe-all`, only filepairs
-that match their respective criterion are kept in the output.  When
-`--pickaxe-all` is used, if even one filepair matches their respective
-criterion in a changeset, the entire changeset is kept.  This behavior
-is designed to make reviewing changes in the context of the whole
-changeset easier.
+"-G<regular-expression>", or "--patch-grep=<regular-expression>"
+(mnemonic: grep) detects filepairs whose textual diff has an added or
+a deleted line that matches the given regular expression.  This means
+that it will detect in-file (or what rename-detection considers the
+same file) moves, which is noise.  The implementation runs diff twice
+and greps, and this can be quite expensive.  To speed things up,
+binary files without textconv filters will be ignored.
+
+When `--patch-modifies` or `--patch-grep` are used without
+`--pickaxe-all`, only filepairs that match their respective criterion
+are kept in the output.  When `--pickaxe-all` is used, if even one
+filepair matches their respective criterion in a changeset, the entire
+changeset is kept.  This behavior is designed to make reviewing
+changes in the context of the whole changeset easier.
 
 diffcore-order: For Sorting the Output Based on Filenames
 ---------------------------------------------------------
diff --git a/diff.c b/diff.c
index d28b41..09beb 100644
--- a/diff.c
+++ b/diff.c
@@ -4877,15 +4877,17 @@  void diff_setup_done(struct diff_options *options)
 
 	if (HAS_MULTI_BITS(options->pickaxe_opts & DIFF_PICKAXE_KINDS_MASK))
 		die(_("options '%s', '%s', and '%s' cannot be used together"),
-			"-G", "-S", "--find-object");
+			"-G/--patch-grep", "-S/--patch-modifies", "--find-object");
 
 	if (HAS_MULTI_BITS(options->pickaxe_opts & DIFF_PICKAXE_KINDS_G_REGEX_MASK))
 		die(_("options '%s' and '%s' cannot be used together, use '%s' with '%s'"),
-			"-G", "--pickaxe-regex", "--pickaxe-regex", "-S");
+			"-G/--patch-grep", "--pickaxe-regex",
+                        "--pickaxe-regex", "-S/--patch-modifies");
 
 	if (HAS_MULTI_BITS(options->pickaxe_opts & DIFF_PICKAXE_KINDS_ALL_OBJFIND_MASK))
 		die(_("options '%s' and '%s' cannot be used together, use '%s' with '%s' and '%s'"),
-			"--pickaxe-all", "--find-object", "--pickaxe-all", "-G", "-S");
+			"--pickaxe-all", "--find-object",
+                        "--pickaxe-all", "-G/--patch-grep", "-S/--patch-modifies");
 
 	/*
 	 * Most of the time we can say "there are changes"
@@ -5862,17 +5864,17 @@  struct option *add_diff_options(const struct option *opts,
 		OPT_SET_INT_F(0, "ita-visible-in-index", &options->ita_invisible_in_index,
 			      N_("treat 'git add -N' entries as real in the index"),
 			      0, PARSE_OPT_NONEG),
-		OPT_CALLBACK_F('S', NULL, options, N_("<string>"),
+		OPT_CALLBACK_F('S', "patch-modifies", options, N_("<string>"),
 			       N_("look for differences that change the number of occurrences of the specified string"),
 			       0, diff_opt_pickaxe_string),
-		OPT_CALLBACK_F('G', NULL, options, N_("<regex>"),
-			       N_("look for differences that change the number of occurrences of the specified regex"),
+		OPT_CALLBACK_F('G', "patch-grep", options, N_("<regex>"),
+			       N_("look for differences where a patch contains the specified regex"),
 			       0, diff_opt_pickaxe_regex),
 		OPT_BIT_F(0, "pickaxe-all", &options->pickaxe_opts,
-			  N_("show all changes in the changeset with -S or -G"),
+			  N_("show all changes in the changeset with -S/--patch-modifies or -G/--patch-grep"),
 			  DIFF_PICKAXE_ALL, PARSE_OPT_NONEG),
 		OPT_BIT_F(0, "pickaxe-regex", &options->pickaxe_opts,
-			  N_("treat <string> in -S as extended POSIX regular expression"),
+			  N_("treat <string> in -S/--patch-modifies as extended POSIX regular expression"),
 			  DIFF_PICKAXE_REGEX, PARSE_OPT_NONEG),
 		OPT_FILENAME('O', NULL, &options->orderfile,
 			     N_("control the order in which files appear in the output")),
diff --git a/diff.h b/diff.h
index 6e6007..247ac 100644
--- a/diff.h
+++ b/diff.h
@@ -598,9 +598,16 @@  void diffcore_fix_diff_index(void);
 "                try unchanged files as candidate for copy detection.\n" \
 "  -l<n>         limit rename attempts up to <n> paths.\n" \
 "  -O<file>      reorder diffs according to the <file>.\n" \
-"  -S<string>    find filepair whose only one side contains the string.\n" \
+"  -G<regex>\n" \
+"  --patch-grep=<regex>\n" \
+"                find differences whose patch contains the regex.\n" \
+"  -S<string>\n" \
+"  --patch-modifies=<string>\n" \
+"                find filepair who differ in the number of occurrences of string.\n" \
+"  --pickaxe-grep\n" \
+"                treat <string> as regex in the -S/--patch-modifies argument.\n" \
 "  --pickaxe-all\n" \
-"                show all files diff when -S is used and hit is found.\n" \
+"                show all files diff for -G/--patch-grep and -S/--patch-modifies.\n" \
 "  -a  --text    treat all files as text.\n"
 
 int diff_queue_is_empty(struct diff_options *o);
diff --git a/t/t4062-diff-pickaxe.sh b/t/t4062-diff-pickaxe.sh
index 8ad3d7..805e0f 100755
--- a/t/t4062-diff-pickaxe.sh
+++ b/t/t4062-diff-pickaxe.sh
@@ -16,13 +16,13 @@  test_expect_success setup '
 '
 
 # OpenBSD only supports up to 255 repetitions, so repeat twice for 64*64=4096.
-test_expect_success '-G matches' '
-	git diff --name-only -G "^(0{64}){64}$" HEAD^ >out &&
+test_expect_success '--patch-grep matches' '
+	git diff --name-only --patch-grep "^(0{64}){64}$" HEAD^ >out &&
 	test 4096-zeroes.txt = "$(cat out)"
 '
 
-test_expect_success '-S --pickaxe-regex' '
-	git diff --name-only -S0 --pickaxe-regex HEAD^ >out &&
+test_expect_success '--patch-modifies --pickaxe-regex' '
+	git diff --name-only --patch-modifies 0 --pickaxe-regex HEAD^ >out &&
 	test 4096-zeroes.txt = "$(cat out)"
 '
 
diff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh
index a675ac..5f4d6 100755
--- a/t/t4209-log-pickaxe.sh
+++ b/t/t4209-log-pickaxe.sh
@@ -1,6 +1,6 @@ 
 #!/bin/sh
 
-test_description='log --grep/--author/--regexp-ignore-case/-S/-G'
+test_description='log --grep/--author/--regexp-ignore-case/--patch-{modifies,grep}'
 
 . ./test-lib.sh
 
@@ -60,24 +60,48 @@  test_expect_success 'usage' '
 	test_expect_code 129 git log -S 2>err &&
 	test_grep "switch.*requires a value" err &&
 
+	test_expect_code 129 git log --patch-modifies 2>err &&
+	test_grep "option.*requires a value" err &&
+
 	test_expect_code 129 git log -G 2>err &&
 	test_grep "switch.*requires a value" err &&
 
+	test_expect_code 129 git log --patch-grep 2>err &&
+	test_grep "option.*requires a value" err &&
+
 	test_expect_code 128 git log -Gregex -Sstring 2>err &&
 	grep "cannot be used together" err &&
 
+	test_expect_code 128 git log -Gregex --patch-modifies string 2>err &&
+	grep "cannot be used together" err &&
+
+	test_expect_code 128 git log --patch-grep regex -Sstring 2>err &&
+	grep "cannot be used together" err &&
+
+	test_expect_code 128 git log --patch-grep regex --patch-modifies string 2>err &&
+	grep "cannot be used together" err &&
+
 	test_expect_code 128 git log -Gregex --find-object=HEAD 2>err &&
 	grep "cannot be used together" err &&
 
+	test_expect_code 128 git log --patch-grep regex --find-object=HEAD 2>err &&
+	grep "cannot be used together" err &&
+
 	test_expect_code 128 git log -Sstring --find-object=HEAD 2>err &&
 	grep "cannot be used together" err &&
 
+	test_expect_code 128 git log --patch-modifies string --find-object=HEAD 2>err &&
+	grep "cannot be used together" err &&
+
 	test_expect_code 128 git log --pickaxe-all --find-object=HEAD 2>err &&
 	grep "cannot be used together" err
 '
 
 test_expect_success 'usage: --pickaxe-regex' '
 	test_expect_code 128 git log -Gregex --pickaxe-regex 2>err &&
+	grep "cannot be used together" err &&
+
+	test_expect_code 128 git log --patch-grep regex --pickaxe-regex 2>err &&
 	grep "cannot be used together" err
 '
 
@@ -89,7 +113,13 @@  test_expect_success 'usage: --no-pickaxe-regex' '
 	test_expect_code 128 git log -Sstring --no-pickaxe-regex 2>actual &&
 	test_cmp expect actual &&
 
-	test_expect_code 128 git log -Gstring --no-pickaxe-regex 2>err &&
+	test_expect_code 128 git log --patch-modifies string --no-pickaxe-regex 2>actual &&
+	test_cmp expect actual &&
+
+	test_expect_code 128 git log -Gregex --no-pickaxe-regex 2>err &&
+	test_cmp expect actual &&
+
+	test_expect_code 128 git log --patch-grep regex --no-pickaxe-regex 2>err &&
 	test_cmp expect actual
 '
 
@@ -104,47 +134,59 @@  test_log_icase	expect_second	--author person
 test_log_icase	expect_nomatch	--author spreon
 
 test_log	expect_nomatch	-G picked
+test_log	expect_nomatch	--patch-grep picked
 test_log	expect_second	-G Picked
+test_log	expect_second	--patch-grep Picked
 test_log_icase	expect_nomatch	-G pickle
+test_log_icase	expect_nomatch	--patch-grep pickle
 test_log_icase	expect_second	-G picked
+test_log_icase	expect_second	--patch-grep picked
 
-test_expect_success 'log -G --textconv (missing textconv tool)' '
+test_expect_success 'log --patch-grep --textconv (missing textconv tool)' '
 	echo "* diff=test" >.gitattributes &&
-	test_must_fail git -c diff.test.textconv=missing log -Gfoo &&
+	test_must_fail git -c diff.test.textconv=missing log --patch-grep foo &&
 	rm .gitattributes
 '
 
-test_expect_success 'log -G --no-textconv (missing textconv tool)' '
+test_expect_success 'log --patch-grep --no-textconv (missing textconv tool)' '
 	echo "* diff=test" >.gitattributes &&
-	git -c diff.test.textconv=missing log -Gfoo --no-textconv >actual &&
+	git -c diff.test.textconv=missing log --patch-grep foo --no-textconv >actual &&
 	test_cmp expect_nomatch actual &&
 	rm .gitattributes
 '
 
 test_log	expect_nomatch	-S picked
+test_log	expect_nomatch	--patch-modifies picked
 test_log	expect_second	-S Picked
+test_log	expect_second	--patch-modifies Picked
 test_log_icase	expect_second	-S picked
+test_log_icase	expect_second	--patch-modifies picked
 test_log_icase	expect_nomatch	-S pickle
+test_log_icase	expect_nomatch	--patch-modifies pickle
 
 test_log	expect_nomatch	-S p.cked --pickaxe-regex
+test_log	expect_nomatch	--patch-modifies p.cked --pickaxe-regex
 test_log	expect_second	-S P.cked --pickaxe-regex
+test_log	expect_second	--patch-modifies P.cked --pickaxe-regex
 test_log_icase	expect_second	-S p.cked --pickaxe-regex
+test_log_icase	expect_second	--patch-modifies p.cked --pickaxe-regex
 test_log_icase	expect_nomatch	-S p.ckle --pickaxe-regex
+test_log_icase	expect_nomatch	--patch-modifies p.ckle --pickaxe-regex
 
-test_expect_success 'log -S --textconv (missing textconv tool)' '
+test_expect_success 'log --patch-modifies --textconv (missing textconv tool)' '
 	echo "* diff=test" >.gitattributes &&
-	test_must_fail git -c diff.test.textconv=missing log -Sfoo &&
+	test_must_fail git -c diff.test.textconv=missing log --patch-modifies foo &&
 	rm .gitattributes
 '
 
-test_expect_success 'log -S --no-textconv (missing textconv tool)' '
+test_expect_success 'log --patch-modifies --no-textconv (missing textconv tool)' '
 	echo "* diff=test" >.gitattributes &&
-	git -c diff.test.textconv=missing log -Sfoo --no-textconv >actual &&
+	git -c diff.test.textconv=missing log --patch-modifies foo --no-textconv >actual &&
 	test_cmp expect_nomatch actual &&
 	rm .gitattributes
 '
 
-test_expect_success 'setup log -[GS] plain & regex' '
+test_expect_success 'setup log --patch{-modifies,-grep} plain & regex' '
 	test_create_repo GS-plain &&
 	test_commit -C GS-plain --append A data.txt "a" &&
 	test_commit -C GS-plain --append B data.txt "a a" &&
@@ -159,31 +201,31 @@  test_expect_success 'setup log -[GS] plain & regex' '
 	git -C GS-plain log >full-log
 '
 
-test_expect_success 'log -G trims diff new/old [-+]' '
-	git -C GS-plain log -G"[+-]a" >log &&
+test_expect_success 'log --patch-grep trims diff new/old [-+]' '
+	git -C GS-plain log --patch-grep "[+-]a" >log &&
 	test_must_be_empty log &&
-	git -C GS-plain log -G"^a" >log &&
+	git -C GS-plain log --patch-grep "^a" >log &&
 	test_cmp log A-to-B-then-E-log
 '
 
-test_expect_success 'log -S<pat> is not a regex, but -S<pat> --pickaxe-regex is' '
-	git -C GS-plain log -S"a" >log &&
+test_expect_success 'log --patch-modifies <pat> is not a regex, but --patch-modifies <pat> --pickaxe-regex is' '
+	git -C GS-plain log --patch-modifies "a" >log &&
 	test_cmp log A-to-B-then-E-log &&
 
-	git -C GS-plain log -S"[a]" >log &&
+	git -C GS-plain log --patch-modifies "[a]" >log &&
 	test_must_be_empty log &&
 
-	git -C GS-plain log -S"[a]" --pickaxe-regex >log &&
+	git -C GS-plain log --patch-modifies "[a]" --pickaxe-regex >log &&
 	test_cmp log A-to-B-then-E-log &&
 
-	git -C GS-plain log -S"[b]" >log &&
+	git -C GS-plain log --patch-modifies "[b]" >log &&
 	test_cmp log D-then-E-log &&
 
-	git -C GS-plain log -S"[b]" --pickaxe-regex >log &&
+	git -C GS-plain log --patch-modifies "[b]" --pickaxe-regex >log &&
 	test_cmp log C-to-D-then-E-log
 '
 
-test_expect_success 'setup log -[GS] binary & --text' '
+test_expect_success 'setup log --patch{-modifies,-grep} binary & --text' '
 	test_create_repo GS-bin-txt &&
 	test_commit -C GS-bin-txt --printf A data.bin "a\na\0a\n" &&
 	test_commit -C GS-bin-txt --append --printf B data.bin "a\na\0a\n" &&
@@ -191,36 +233,36 @@  test_expect_success 'setup log -[GS] binary & --text' '
 	git -C GS-bin-txt log >full-log
 '
 
-test_expect_success 'log -G ignores binary files' '
-	git -C GS-bin-txt log -Ga >log &&
+test_expect_success 'log --patch-grep ignores binary files' '
+	git -C GS-bin-txt log --patch-grep a >log &&
 	test_must_be_empty log
 '
 
-test_expect_success 'log -G looks into binary files with -a' '
-	git -C GS-bin-txt log -a -Ga >log &&
+test_expect_success 'log --patch-grep looks into binary files with -a' '
+	git -C GS-bin-txt log -a --patch-grep a >log &&
 	test_cmp log full-log
 '
 
-test_expect_success 'log -G looks into binary files with textconv filter' '
+test_expect_success 'log --patch-grep looks into binary files with textconv filter' '
 	test_when_finished "rm GS-bin-txt/.gitattributes" &&
 	(
 		cd GS-bin-txt &&
 		echo "* diff=bin" >.gitattributes &&
-		git -c diff.bin.textconv=cat log -Ga >../log
+		git -c diff.bin.textconv=cat log --patch-grep a >../log
 	) &&
 	test_cmp log full-log
 '
 
-test_expect_success 'log -S looks into binary files' '
-	git -C GS-bin-txt log -Sa >log &&
+test_expect_success 'log --patch-modifies looks into binary files' '
+	git -C GS-bin-txt log --patch-modifies a >log &&
 	test_cmp log full-log
 '
 
-test_expect_success 'log -S --pickaxe-regex looks into binary files' '
-	git -C GS-bin-txt log --pickaxe-regex -Sa >log &&
+test_expect_success 'log --patch-modifies --pickaxe-regex looks into binary files' '
+	git -C GS-bin-txt log --pickaxe-regex --patch-modifies a >log &&
 	test_cmp log full-log &&
 
-	git -C GS-bin-txt log --pickaxe-regex -S"[a]" >log &&
+	git -C GS-bin-txt log --pickaxe-regex --patch-modifies "[a]" >log &&
 	test_cmp log full-log
 '