Re: [PATCH 2/2] diffcore-pickaxe doc: document -S and -G properly

[Ramkumar Ramachandra <artagnon@gmail.com>]

Junio C Hamano wrote:
> [...]

I agree with the other comments, and have made suitable changes.
Let's review your block now.

>         This transformation is used to find filepairs that represent
>         two kinds of changes, and is controlled by the -S, -G and
>         --pickaxe-all options.

Why do you call this a "transformation"?  Is git log --author="foo" a
transformation on the git-log output?  Then how is git log -Sfoo a
transformation?

Two kinds of changes controlled by three different options?  Isn't the
original much clearer?

The title says diffcore-pickaxe, and the first paragraph says:

There are two kinds of pickaxe: the S kind (corresponding to 'git log
-S') and the G kind (mnemonic: grep; corresponding to 'git log -G').

>         The "-S<block of text>" option tells Git to consider that a
>         filepair has differences only if the number of occurrences
>         of the specified block of text is different between its
>         preimage and its postimage, and treat other filepairs as if
>         they did not have any change.

I'll rewrite this without the trailing "and treat other filepairs as
if they did not have any change" (which I'm not fond of).

>         This is meant to be used with
>         a block of text that is unique enough to occur only once (so
>         expected the number of occurences is 1 vs 0 or 0 vs 1) to
>         use with "git log" to find a commit that touched the block
>         of text the last time.

You're saying how you think it's "meant" to be used, but in doing so
you've failed to describe its operation faithfully.  I've already
described how it's meant to be used in diff-options (digging a block
of text iteratively) and this is the place to explain what it is doing
faithfully.  Hence my previous writeup on changes in number of
occurrences and rename detection: I _had_ to read the code to
understand it properly, and your writeup is not helping by telling me
about idiomatic usage.

Also, you've dropped computational expense which was there in the original.

>         When used with the "--pickaxe-regex"
>         option, the <block of text> is used as a POSIX extended
>         regular expression to match, instead of a literal string.

Better.

>         The "-G<regular expression>" option tells Git to consider
>         that a filepair has differences only if a textual diff
>         between its preimage and postimage would indicate a line
>         that matches the given regular expression is changed, and
>         treat other filepairs as if they did not have any change.

"would indicate"?  Really?  I'll rewrite this without the trailing
"and treat other filepairs ..".

You've once again dropped what it means in the context of in-file
moves (rename detection), and computational expense from the original.

>         When -S or -G option is used without "--pickaxe-all" option,
>         only filepairs that match their respective criterion are
>         kept in the output.

Much better.

>         When `--pickaxe-all` is used, all
>         filepairs intact if there is such a filepair, or makes the
>         output empty otherwise.

-ENOPARSE.  I didn't particularly like the original, and this isn't
better.  I'll rewrite it.

>         This behaviour is designed to make
>         reviewing of the changes in the context of the whole
>         changeset easier.

Same as original.  Okay.

Thanks.