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

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

[Ramkumar Ramachandra]

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').
+
+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
+where a the small string being looked for is moved verbatim from one
+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

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

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

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

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

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

[Phil Hord]

On Tue, May 14, 2013 at 10:12 AM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> 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>::

It looks like you have deleted the -S and -G references here.  Am I
reading this right?

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

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.

> ++
> +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').

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

> +
> +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?

> +where a the small string being looked for is moved verbatim from one

s/a the/the/

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

Is it worth mentioning that something in the documentation is "worth
mentioning"?

> +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. Does
this seem clearer to you?

    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.

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

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

[Phil Hord]

On Tue, May 14, 2013 at 1:44 PM, Phil Hord <phil.hord@gmail.com> wrote:
> 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?

Doy!  Yes, I was reading it wrong.  Sorry for the noise there.

Phil

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

[Ramkumar Ramachandra]

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?

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

[Ramkumar Ramachandra]

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?

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

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

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.

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

[Ramkumar Ramachandra]

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?

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

[Junio C Hamano]

Phil Hord <phil.hord@gmail.com> writes:

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

I find this very readable, even though "diff output" might be
somewhat misleading (it is not "output for the end user", but is
"passing to the next stage in the pipeline").

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

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> What can we do to improve the interface?

Write a better shell?  Teach "git gui blame" to blame on arbitrary
regions instead of single lines?  I'm not sure what you're asking,
mostly because I'm not sure who "we" is.

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

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

I think that is one of the reasons why "If you moved a string from
file A to file B, log -S will flag that change as worth inspecting"
does not seem to logically follow and made Phil find your
description confusing.

Finding such a change indeed is a feature [*1*]; we need to flag
such a change as worth inspecting to find where the code came from
in order to dig deeper, so at least this "cannot omit" should be
"does not omit".


[Footnote]

*1* I suspect that your confusion may stem from not understanding
    what pickaxe was invented for. It is _not_ about finding the
    final answer, but is about stopping at a commit that is worth
    investigating further.  

    It may help to read
    http://article.gmane.org/gmane.comp.version-control.git/217 and
    then its follow-up http://gitster.livejournal.com/35628.html

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

[Phil Hord]

On Tue, May 14, 2013 at 2:44 PM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> 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.

She may have been led here by some other help topic, too.  Maybe the
git log examples belong in the git-log documentation.

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

S and G are abstractions.

DIFF_PICKAXE_KIND_G is an implementer's distinction.

I agree this is a tricky subject.  When writing technical
documentation you must be precise and clear.  It is easy to forget
both.  It is common to forget to be either clear or precise.  It is
very difficult to be both clear and precise.

So, my suggestion is to use some meaningful names for the action of -S
and -G.  Relating these names to -S and -G in a clear way is likely to
be difficult.

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

This is precision at the expense of clarity.  Omission is not expected
for the user who wants to ask this question:  "Where is that commit
that added 'foo'?"  To Git she wants to ask "Show me the commit that
added or removed 'foo'."  You and I both know that Git does this in
reverse.  Git translates this question into "Show all the commits, but
hide the ones which do not add or remove 'foo'."

And so we say Git 'cannot omit' commits


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

I think I do.  The concepts in here are complicated enough without
loading the language with excess verbiage.  I am chronically afflicted
with this disease where I am always adding extra decorations to my
sentences.  I work hard to combat that, especially when dealing with
technical writings.  This makes it easy for me to recognize in other
technical writing.

I didn't look closely, but these unnecessary introductory clause
appeared to be the only change in this paragraph.  I do not think it
adds anything, which is why I mentioned it.

I could have been more clear and less flippant about it, though.  I'm
sorry if my manner was offensive. This is another chronic problem I
seem to have.


>>> +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?

Clarity.

What is a 'filepair' when I am getting a short-log?  Internally there
was a diff, and the diff involves pairs of files.  But it's a tedious
detail to the user which might send her off needlessly trying to
understand the meaning of the term.

But you are right; gitdiffcore.txt is about precision and
implementation.  The goal is to address the technical user who is
trying to understand these operations in general, not just for log.
'filepairs' is a useful concept to lean on.

The clearer description probably belongs in git-log.

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

Yes, it was clear enough to me in git-log.  Perhaps it is not worth
mentioning here.

Phil

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

[Ramkumar Ramachandra]

Jonathan Nieder wrote:
> Write a better shell?

Shell, editor.  Both are very hard problems, and the successful
projects last many years (emacs, zsh are over 20 years old).

> Teach "git gui blame" to blame on arbitrary
> regions instead of single lines?

Or in my case: get magit to do log -S.

Should we mention in the -S documentation that temporary shell script
is the way to get multi-line input?

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

[Jonathan Nieder]

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.

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

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

Heh, I seem to have just found a volunteer to finish the Linus's
dream by following up on http://gitster.livejournal.com/35628.html
(see section #5; I do not do GUI, neither Linus, so this has been a
four-year old dream).

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

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

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

Or document it as "for scripter's use", which it is.

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

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> 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.

Yes, I'm exactly implying that.  But I don't want to lose meaning: in
the previous sentence, I talk about filepairs.  I want to point out
that rename detection working at the filepair level is a perfectly
normal thing.

> I think that is one of the reasons why "If you moved a string from
> file A to file B, log -S will flag that change as worth inspecting"
> does not seem to logically follow and made Phil find your
> description confusing.

Sure, we can elaborate a bit more.

> Finding such a change indeed is a feature [*1*]; we need to flag
> such a change as worth inspecting to find where the code came from
> in order to dig deeper, so at least this "cannot omit" should be
> "does not omit".

What I was trying to say is that it's an accidental feature: the
reason this "feature" exists is because diffcore is tied to filepairs
(and rename detection works at the filepair level).  You may argue
that there's nothing wrong with this design, but consider what happens
if you rebase on top of a big code move: it's completely broken.  If
git were a true content tracker, and file boundaries really did not
matter, isn't this feature actually a deficiency?

Ofcourse, the user doesn't need to know all this, and "does not omit"
is a good suggestion.

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> What I was trying to say is that it's an accidental feature

There is nothing accidental about it.  It was a very conscious
design decision.

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.

If you do not understand it, then you really should re-read
$gmane/217 and then its explanation I wrote 4 years ago (both of
which I already gave you URLs to in this thread).

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

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> 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.

Makes sense.  I wasn't looking at it from a macro perspective.

Yeah, I constantly re-read 217 and your follow-up.

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

[Phil Hord]

On Tue, May 14, 2013 at 2:57 PM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> 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?

Don't use tabs in your code?

P