patchdiffcore-pickaxe doc: document -S and -G properly

>  -S<string>::
> -	Look for differences that introduce or remove an instance of
> -	<string>. Note that this is different than the string simply
> -	appearing in diff output; see the 'pickaxe' entry in
> -	linkgit:gitdiffcore[7] for more details.
> +	Look for commits where the specified string was added or
> +	removed.  More precisely, find commits that change the number
> +	of occurrences of the specified string.

>> ++
>> +It is often useful when you're looking for an exact string (like a
>> +function prototype), and want to know the history of that string since
>> +it first came into being.
>
> I think you should remind that the most useful case (and indeed the
> intended one) is for "an exact string" to be a multi-line "block of
> text".  People often get a (wrong) impression from the word "string"
> that it is meant to be used with a single-liner.

> Junio C Hamano wrote:
>> Any time you say "This means that", "More precisely", etc. please
>> check if you can rewrite it to lose everything before them (i.e. a
>> vague sentence that needs to be clarified may not have to be there
>> at all).
>
> Right.  I thought both are necessary in this case: the first sentence
> gives easy information to a first-timer.  For someone who has played
> with it a bit, and wants to know more: the second line.
>
>>> ++
>>> +It is often useful when you're looking for an exact string (like a
>>> +function prototype), and want to know the history of that string since
>>> +it first came into being.
>>
>> I think you should remind that the most useful case (and indeed the
>> intended one) is for "an exact string" to be a multi-line "block of
>> text".  People often get a (wrong) impression from the word "string"
>> that it is meant to be used with a single-liner.
>
> Yes, I've been meaning to discuss that.  I've been having some trouble
> with multi-line strings: zsh doesn't insert a TAB in the next line.
> The workaround I have is to write a shell script and execute that.
> How do you do it?

> Ramkumar Ramachandra wrote:
>
>> Should we mention in the -S documentation that temporary shell script
>> is the way to get multi-line input?
>
> No, because for almost everyone it isn't.
>
> An example in the EXAMPLES section including an aside that you might
> have to hit ^V to enter a tab could be useful, though.

> Junio C Hamano wrote:
>> I do not use zsh but with bash+readline the old tradition lnext can
>> be used (see "stty -a" output and it typically is set to ^V), i.e.
>> \C-v followed by \C-i should give you a literal HT.
>
> Just looked it up: zsh has quoted-insert (^V) after which I can TAB.
> Still doesn't solve my problem though: I don't type out structs; I
> paste them the X clipboard (Emacs).  And that doesn't work either on
> bash or zsh.
>
> What can we do to improve the interface?

> Junio C Hamano wrote:
>> I do not use zsh but with bash+readline the old tradition lnext can
>> be used (see "stty -a" output and it typically is set to ^V), i.e.
>> \C-v followed by \C-i should give you a literal HT.
>
> Just looked it up: zsh has quoted-insert (^V) after which I can TAB.
> Still doesn't solve my problem though: I don't type out structs; I
> paste them the X clipboard (Emacs).  And that doesn't work either on
> bash or zsh.
>
> What can we do to improve the interface?

> The documentation of -S and -G is very sketchy.  Completely rewrite the
> sections in Documentation/diff-options.txt and
> Documentation/gitdiffcore.txt.
>
> References:
> 52e9578 ([PATCH] Introducing software archaeologist's tool "pickaxe".)
> f506b8e (git log/diff: add -G<regexp> that greps in the patch text)
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  I spent some time reading the code and history to figure out what -S
>  and -G really do.  I hope I've done justice.
>
>  Documentation/diff-options.txt | 35 +++++++++++++++++++++++++++-------
>  Documentation/gitdiffcore.txt  | 43 +++++++++++++++++++++++-------------------
>  2 files changed, 52 insertions(+), 26 deletions(-)
>
> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
> index 104579d..765abc5 100644
> --- a/Documentation/diff-options.txt
> +++ b/Documentation/diff-options.txt
> @@ -383,14 +383,35 @@ ifndef::git-format-patch[]
>         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
> -       appearing in diff output; see the 'pickaxe' entry in
> -       linkgit:gitdiffcore[7] for more details.
> +       Look for commits where the specified string was added or
> +       removed.  More precisely, find commits that change the number
> +       of occurrences of the specified string.
> ++
> +It is often useful when you're looking for an exact string (like a
> +function prototype), and want to know the history of that string since
> +it first came into being.
>
>  -G<regex>::

> -       Look for differences whose added or removed line matches
> -       the given <regex>.
> +       Grep through the patch text of commits for added/removed lines
> +       that match <regex>.  `--pickaxe-regex` is implied in this
> +       mode.
> ++
> +To illustrate the difference between `-S<regex> --pickaxe-regex` and
> +`-G<regex>`, consider a commit with the following diff in the same
> +file:
> ++
> +----
> ++    return !regexec(regexp, two->ptr, 1, &regmatch, 0);
> +...
> +-    hit = !regexec(regexp, mf2.ptr, 1, &regmatch, 0);
> +----
> ++
> +While `git log -G"regexec\(regexp"` will show this commit, `git log
> +-S"regexec\(regexp" --pickaxe-regex` will not (because the number of
> +occurrences of that string didn't change).

> ++
> +See the 'pickaxe' entry in linkgit:gitdiffcore[7] for more
> +information.
>
>  --pickaxe-all::
>         When `-S` or `-G` finds a change, show all the changes in that
> @@ -399,7 +420,7 @@ ifndef::git-format-patch[]
>
>  --pickaxe-regex::
>         Make the <string> not a plain string but an extended POSIX
> -       regex to match.
> +       regex to match.  Implied when using `-G`.
>  endif::git-format-patch[]
>
>  -O<orderfile>::
> diff --git a/Documentation/gitdiffcore.txt b/Documentation/gitdiffcore.txt
> index 568d757..39b9c51 100644
> --- a/Documentation/gitdiffcore.txt
> +++ b/Documentation/gitdiffcore.txt
> @@ -222,25 +222,30 @@ version prefixed with '+'.
>  diffcore-pickaxe: For Detecting Addition/Deletion of Specified String
>  ---------------------------------------------------------------------
>
> -This transformation is used to find filepairs that represent
> -changes that touch a specified string, and is controlled by the
> --S option and the `--pickaxe-all` option to the 'git diff-*'
> -commands.
> -
> -When diffcore-pickaxe is in use, it checks if there are
> -filepairs whose "result" side and whose "origin" side have
> -different number of specified string.  Such a filepair represents
> -"the string appeared in this changeset".  It also checks for the
> -opposite case that loses the specified string.
> -
> -When `--pickaxe-all` is not in effect, diffcore-pickaxe leaves
> -only such filepairs that touch the specified string in its
> -output.  When `--pickaxe-all` is used, diffcore-pickaxe leaves all
> -filepairs intact if there is such a filepair, or makes the
> -output empty otherwise.  The latter behaviour is designed to
> -make reviewing of the changes in the context of the whole
> -changeset easier.
> -
> +There are two kinds of pickaxe: the S kind (corresponding to 'git log
> +-S') and the G kind (corresponding to 'git log -G').

> +file to another (since the number of occurrences of that string
> +changed in each of those two filepairs). The implementation
> +essentially runs a count, and is significantly cheaper than the G
> +kind.
> +
> +The G kind detects filepairs whose patch text has an added or a
> +deleted line that matches the given regexp.  This means that it can
> +detect in-file (or what rename-detection considers the same file)
> +moves.  The implementation of 'git log -G' runs diff twice and greps,
> +and this can be quite expensive.
> +
> +A diffcore-pickaxe option worth mentioning: `--pickaxe-all`.  When not

> +in effect, diffcore-pickaxe leaves only such filepairs that touch the
> +specified string in its output.  When in effect, diffcore-pickaxe
> +leaves all filepairs intact if there is such a filepair, or makes the
> +output empty otherwise.  The latter behavior is designed to make
> +reviewing of the changes in the context of the whole changeset easier.

>
>  diffcore-order: For Sorting the Output Based on Filenames
>  ---------------------------------------------------------
> --
> 1.8.3.rc1.61.g2cacfff
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

> On Tue, May 14, 2013 at 10:12 AM, Ramkumar Ramachandra
> <artagnon@gmail.com> wrote:
>>
>>  -S<string>::
>> -       Look for differences that introduce or remove an instance of
>> -       <string>. Note that this is different than the string simply
>> -       appearing in diff output; see the 'pickaxe' entry in
>> -       linkgit:gitdiffcore[7] for more details.
>> +       Look for commits where the specified string was added or
>> +       removed.  More precisely, find commits that change the number
>> +       of occurrences of the specified string.
>> ++
>> +It is often useful when you're looking for an exact string (like a
>> +function prototype), and want to know the history of that string since
>> +it first came into being.
>>
>>  -G<regex>::
>
> It looks like you have deleted the -S and -G references here.  Am I
> reading this right?

> References to git-log seem out of place to me here in git-diffcore.  I
> know it's only an example, but it seems that Git normally describes
> these 'reference selectors' more generically.  The generic description
> may be more confusing to new users, but this patch is not the place to
> consider whether it should change.

>> +
>> +The S kind detects filepairs whose "result" side and "origin" side
>> +have different number of occurrences of specified string.  While
>> +rename detection works as usual, 'git log -S' cannot omit commits
>
> The "cannot omit" feels like a confusing double-negative.  How about
> "includes" instead?

>> +in effect, diffcore-pickaxe leaves only such filepairs that touch the
>> +specified string in its output.  When in effect, diffcore-pickaxe
>> +leaves all filepairs intact if there is such a filepair, or makes the
>> +output empty otherwise.  The latter behavior is designed to make
>> +reviewing of the changes in the context of the whole changeset easier.
>
> I find this description (from the original code, not from this commit)
> somewhat confusing.  It is written from the implementer's POV.

>>> +The S kind detects filepairs whose "result" side and "origin" side
>>> +have different number of occurrences of specified string.  While
>>> +rename detection works as usual, 'git log -S' cannot omit commits
>>
>> The "cannot omit" feels like a confusing double-negative.  How about
>> "includes" instead?
>
> Intended.  Omission is expected.

> I think what makes this paragraph unnecessarily hard to read is the
> "While rename works".
>
> With that, you are implying "if you rename a file as a whole without
> changing the block of text you identify with the -S parameter, then
> such a change is not interesting as far as pickaxe is concerned".
> while that statement is logically correct, normal people are not
> that generous to read that much between your lines.

> When a commit moves a file wholesale without affecting the block of
> code you are interested in, you know that whole block came from the
> file in the old tree at pre-rename location without looking at
> anywhere else.  That is why renamed but pickaxe-uninteresting
> filepairs are dropped.
>
> When a commit moves (some lines of) the block of code you are
> interested in from one file to another, it may have been a single
> instance moving to another place, but it may well have been multiple
> copies consolidated into one (the new copy, pickaxe digging from
> future to past may see "disappearing").  That is a significant event
> worth digging into further by first stopping there and then
> inspecting the whole change with --pickaxe-all to see what changes
> that are similar to the change in question exist in the other parts
> (notice the multiple) of the tree.

> Phil Hord wrote:
>> References to git-log seem out of place to me here in git-diffcore.  I
>> know it's only an example, but it seems that Git normally describes
>> these 'reference selectors' more generically.  The generic description
>> may be more confusing to new users, but this patch is not the place to
>> consider whether it should change.
>
> It's not for new users at all.  The most useful application of -S and
> -G is in log.  The translation from a log -G to a diffcore -G is not
> obvious at all, and warrants an explanation.
>
> Oh, and for the user.  No user is going to type out `man gitdiffcore`
> out of the blue: she's most probably led there from log, and we're
> connecting the dots for her.

>> While the switches are called -S and -G, I do not think it is helpful
>> to name the two pickaxe options as "the S kind" and "the G kind".
>
> How do you describe something precisely without loss of meaning?  You
> stop abstracting unnecessarily.  Read the sources: you will literally
> see DIFF_PICKAXE_KIND_G there.

>>> +
>>> +The S kind detects filepairs whose "result" side and "origin" side
>>> +have different number of occurrences of specified string.  While
>>> +rename detection works as usual, 'git log -S' cannot omit commits
>>
>> The "cannot omit" feels like a confusing double-negative.  How about
>> "includes" instead?
>
> Intended.  Omission is expected.

>
>> Is it worth mentioning that something in the documentation is "worth
>> mentioning"?
>
> You don't have to nitpick style.  We can allow this much creative
> freedom in documentation.

>>> +in effect, diffcore-pickaxe leaves only such filepairs that touch the
>>> +specified string in its output.  When in effect, diffcore-pickaxe
>>> +leaves all filepairs intact if there is such a filepair, or makes the
>>> +output empty otherwise.  The latter behavior is designed to make
>>> +reviewing of the changes in the context of the whole changeset easier.
>>
>> I find this description (from the original code, not from this commit)
>> somewhat confusing.  It is written from the implementer's POV.
>
> I explained the entire -S and -G thing in terms of filepairs (and yes,
> that's implementation detail).  Why would I want to explain this in
> any other terms?

>> Does
>> this seem clearer to you?
>> [...]
>
> From diff-options.txt (the more end-user side):
>
> When -S or -G finds a change, show all the changes in that changeset,
> not just the files that contain the change in <string>.
>
> Not clear enough?

>     Normally the pickaxe options limit the diff output to those files which
>     contained the changes being searched; that is, those files which
>     had modifications including the search string.  With the --pickaxe-all
>     option, the diff of the entire commit will be shown including files
>     which did not have modifications including the search string.  This
>     is designed to make it easier to review the changes in the context
>     of the whole commit.