git log -Sfoo ignores indentation (whitespace?) changes...

git log -Sfoo ignores indentation (whitespace?) changes...

["Peter Valdemar Mørch (Lists)"]

Commits where only the indentation of 'foo' is changed are not shown 
with "git log -Sfoo". Is there any way to force showing them along with 
other changes involving foo? (E.g. for python, indentation matters!)

Why doesn't the second commit show up in the following?

$ git init

# Create text containing 'line' without whitespace
$ echo 'line' > text
$ git add text
$ git commit -m "first" text

# Here, I add one space of indentation in front of 'line'
$ echo ' line' > text
$ git commit -m "second" text

# git log -Sline shows only the first commit, not the second,
# where the indentation changed.
$ git log -Sline
commit c4481e4b38bb521d91583e5c5a3b2b98f08b7ec0
Author: pvm <pvm@change_me.dk>
Date:   Tue Mar 3 14:45:38 2009 +0100

     first

# But clearly, the second commit *also* has changes
# containing 'line':
$ git log -p HEAD~..HEAD
commit 6cb883409aa9ccda00d7720ee9bf4fa59918c5fd
Author: pvm <pvm@change_me.dk>
Date:   Tue Mar 3 14:45:39 2009 +0100

     second

diff --git a/text b/text
index a999a0c..d650980 100644
--- a/text
+++ b/text
@@ -1 +1 @@
-line
+ line

I would like to see both "first" and "second" somehow - can I do that?

-- 
Peter Valdemar Mørch
http://www.morch.com

Re: git log -Sfoo ignores indentation (whitespace?) changes...

[Jeff King]

On Tue, Mar 03, 2009 at 03:28:08PM +0100, "Peter Valdemar Mørch (Lists)" wrote:

> Commits where only the indentation of 'foo' is changed are not shown with 
> "git log -Sfoo". Is there any way to force showing them along with other 
> changes involving foo? (E.g. for python, indentation matters!)
>
> Why doesn't the second commit show up in the following?

Because you misunderstand how "-S" works (but don't worry, it's not your
fault -- the documentation is somewhat misleading). The documentation says:

  -S<string>
    Look for differences that contain the change in <string>.

but what it actually does is find changes where the string was introduced
or removed. So it literally counts the number of occurences before and
after the commit, and the commit is interesting if they are not equal.

> # Create text containing 'line' without whitespace
> $ echo 'line' > text
> $ git add text
> $ git commit -m "first" text
>
> # Here, I add one space of indentation in front of 'line'
> $ echo ' line' > text
> $ git commit -m "second" text

So "line" wasn't actually changed. It just happens to be on a line which
_did_ change.

> I would like to see both "first" and "second" somehow - can I do that?

I don't think there's an easy way to do this right now; you would need
to do "git log -p" and search through the output to get what you want (I
often do this just using the pager's search function).

-Peff

[PATCH] doc: clarify how -S works

[Jeff King]

The existing text was very vague about what exactly it means
for difference to "contain" a change. This seems to cause
confusion on the mailing list every month or two.

To fix it we:

  1. use "introduce or remove an instance of" instead of
     "contain"

  2. point the user to gitdiffcore(7), which contains a more
     complete explanation

Signed-off-by: Jeff King <peff@peff.net>
---
I wonder if "gitdiffcore" is a little scary for new people who just want
to use "-S", but hopefully point (1) above will get rid of most of the
confusion, and those who follow the link want to learn all about diff.

 Documentation/diff-options.txt |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 813a7b1..9276fae 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -176,7 +176,10 @@ override configuration settings.
 	number.
 
 -S<string>::
-	Look for differences that contain the change in <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.
 
 --pickaxe-all::
 	When -S finds a change, show all the changes in that
-- 
1.6.2.rc2.330.gba39e

Re: [PATCH] doc: clarify how -S works

[John Tapsell]

2009/3/3 Jeff King <peff@peff.net>:
> The existing text was very vague about what exactly it means
> for difference to "contain" a change. This seems to cause
> confusion on the mailing list every month or two.
>
> To fix it we:
>
>  1. use "introduce or remove an instance of" instead of
>     "contain"

I would read this to mean that it doesn't include modifying a line
containing that string.  But I also know that underneath the hood, a
change is a remove then an addition, so I would be confused :)

What about saying "modifies" rather than "contain" ?

>
>  2. point the user to gitdiffcore(7), which contains a more
>     complete explanation
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I wonder if "gitdiffcore" is a little scary for new people who just want
> to use "-S", but hopefully point (1) above will get rid of most of the
> confusion, and those who follow the link want to learn all about diff.
>
>  Documentation/diff-options.txt |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
> index 813a7b1..9276fae 100644
> --- a/Documentation/diff-options.txt
> +++ b/Documentation/diff-options.txt
> @@ -176,7 +176,10 @@ override configuration settings.
>        number.
>
>  -S<string>::
> -       Look for differences that contain the change in <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.
>
>  --pickaxe-all::
>        When -S finds a change, show all the changes in that
> --
> 1.6.2.rc2.330.gba39e
>
> --
> 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] doc: clarify how -S works

[Jeff King]

On Tue, Mar 03, 2009 at 04:12:30PM +0000, John Tapsell wrote:

> > To fix it we:
> >
> >  1. use "introduce or remove an instance of" instead of
> >     "contain"
> 
> I would read this to mean that it doesn't include modifying a line
> containing that string.  But I also know that underneath the hood, a
> change is a remove then an addition, so I would be confused :)
> 
> What about saying "modifies" rather than "contain" ?

I'm confused. It _doesn't_ include modifying a line containing the
string. In which case it has done its job. But your "but" after that
is what leaves me confused. You thought it would mean that, but you
don't due to some other knowledge, which is leading you down the wrong
path?

I was trying to get away with a short and sweet description. But the
behavior is basically (with a few optimizations):

  if count(a, string) != count(b, string) then
    it is interesting

which is unambiguous, but it takes a second to realize the implications.

-Peff

Re: [PATCH] doc: clarify how -S works

[John Tapsell]

2009/3/3 Jeff King <peff@peff.net>:
> On Tue, Mar 03, 2009 at 04:12:30PM +0000, John Tapsell wrote:
>
>> > To fix it we:
>> >
>> >  1. use "introduce or remove an instance of" instead of
>> >     "contain"
>>
>> I would read this to mean that it doesn't include modifying a line
>> containing that string.  But I also know that underneath the hood, a
>> change is a remove then an addition, so I would be confused :)
>>
>> What about saying "modifies" rather than "contain" ?
>
> I'm confused. It _doesn't_ include modifying a line containing the
> string. In which case it has done its job. But your "but" after that
> is what leaves me confused. You thought it would mean that, but you
> don't due to some other knowledge, which is leading you down the wrong
> path?

Yes, it would seem that I was also confused as to what -S means.  It
doesn't mean what I thought it meant :-)

> I was trying to get away with a short and sweet description. But the
> behavior is basically (with a few optimizations):
>
>  if count(a, string) != count(b, string) then
>    it is interesting
>
> which is unambiguous, but it takes a second to realize the implications.
>
> -Peff
>

Re: [PATCH] doc: clarify how -S works

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I wonder if "gitdiffcore" is a little scary for new people who just want
> to use "-S", but hopefully point (1) above will get rid of most of the
> confusion, and those who follow the link want to learn all about diff.

As I mentioned in the other message, what --pickaxe achieves is very
different from what people would naturally want from --search, an
option that does not exist.

I do not mind a patch that adds a diffcore transformation that internally
generates a diff and searches the string given by the user in it, and
triggers that with --search option.  The transformation should come just
after (or before) the pickaxe in the call sequence inside diffcore_std();
name it diffcore_search() or something.

In retrospect, because --pickaxe was designed primarily for Porcelain use,
it was a mistake for it to have taken a short-and-sweet -S synonym.

> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
> index 813a7b1..9276fae 100644
> --- a/Documentation/diff-options.txt
> +++ b/Documentation/diff-options.txt
> @@ -176,7 +176,10 @@ override configuration settings.
>  	number.
>  
>  -S<string>::
> -	Look for differences that contain the change in <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 differences that change the number of occurrences of <string>?

Re: [PATCH] doc: clarify how -S works

[Jeff King]

On Tue, Mar 03, 2009 at 08:42:12AM -0800, Junio C Hamano wrote:

> In retrospect, because --pickaxe was designed primarily for Porcelain use,
> it was a mistake for it to have taken a short-and-sweet -S synonym.

Hmm. I actually like the pickaxe behavior and find it useful for
searching. IOW, I consider it a porcelain feature, just perhaps not the
one that some people are expecting.

> >  -S<string>::
> > -	Look for differences that contain the change in <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 differences that change the number of occurrences of <string>?

Yes, that is technically correct. I was trying to find a wording that
was a little less "this is literally what it does" and more "this is
what you might find it useful for".

But I don't care overly much; I just think what was there was quite
misleading. And I've already provided my paint color, so feel free to
apply mine, use what you wrote above, or whatever. Just don't leave it
as-is. ;)

-Peff

Re: [PATCH] doc: clarify how -S works

[John Tapsell]

2009/3/3 Jeff King <peff@peff.net>:
> On Tue, Mar 03, 2009 at 08:42:12AM -0800, Junio C Hamano wrote:
>
>> In retrospect, because --pickaxe was designed primarily for Porcelain use,
>> it was a mistake for it to have taken a short-and-sweet -S synonym.
>
> Hmm. I actually like the pickaxe behavior and find it useful for
> searching. IOW, I consider it a porcelain feature, just perhaps not the
> one that some people are expecting.
>
>> >  -S<string>::
>> > -   Look for differences that contain the change in <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 differences that change the number of occurrences of <string>?
>
> Yes, that is technically correct. I was trying to find a wording that
> was a little less "this is literally what it does" and more "this is
> what you might find it useful for".

Is there any way to have an option to also match any line containing
the string?  That might be the best way to document it, as well as
being very useful:

-s<string>
   Look for any additions, removals or changes in any line containing <string>
-S<string>
   Look only for any additions or removals of the <string> in any line

John

Re: [PATCH] doc: clarify how -S works

[Jeff King]

On Tue, Mar 03, 2009 at 05:39:38PM +0000, John Tapsell wrote:

> > Yes, that is technically correct. I was trying to find a wording that
> > was a little less "this is literally what it does" and more "this is
> > what you might find it useful for".
> 
> Is there any way to have an option to also match any line containing
> the string?  That might be the best way to document it, as well as
> being very useful:
> 
> -s<string>
>    Look for any additions, removals or changes in any line containing <string>
> -S<string>
>    Look only for any additions or removals of the <string> in any line

Yes, that would be possible (though it is still not foolproof against
moves, as I mentioned elsewhere), and I think it would be simple to
explain the two together.  It just needs somebody to code it.

Unfortunately "-s" is already taken for "no output".

-Peff

Re: [PATCH] doc: clarify how -S works

[Junio C Hamano]

John Tapsell <johnflux@gmail.com> writes:

> 2009/3/3 Jeff King <peff@peff.net>:
>> On Tue, Mar 03, 2009 at 08:42:12AM -0800, Junio C Hamano wrote:
>>
>>> In retrospect, because --pickaxe was designed primarily for Porcelain use,
>>> it was a mistake for it to have taken a short-and-sweet -S synonym.
>>
>> Hmm. I actually like the pickaxe behavior and find it useful for
>> searching. IOW, I consider it a porcelain feature, just perhaps not the
>> one that some people are expecting.
>>
>>> >  -S<string>::
>>> > -   Look for differences that contain the change in <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 differences that change the number of occurrences of <string>?
>>
>> Yes, that is technically correct. I was trying to find a wording that
>> was a little less "this is literally what it does" and more "this is
>> what you might find it useful for".
>
> Is there any way to have an option to also match any line containing
> the string?

Patches welcome. I've already outlined what you need to do.

I think it can be called -G (short for --grep-diff), if --search cannot be
used because "-s" is unavailable.

Re: git log -Sfoo ignores indentation (whitespace?) changes...

["Peter Valdemar Mørch (Lists)"]

How sad... From "git log -Sfoo" looking like a really cool feature, it 
now for me goes into the "must be plumbing because I have no use for it" 
bin, as I can't rely on it:

If a commit removes mention of foo one place and just accidentally 
happens to add foo somewhere completely unrelated then it wouldn't show 
up in the output.

Would be neat with a feature that does what I thought -S did tho...

Thanks, Jeff, for both the answer and the documentation patch.

Peter
-- 
Peter Valdemar Mørch
http://www.morch.com

Re: git log -Sfoo ignores indentation (whitespace?) changes...

[Jeff King]

On Tue, Mar 03, 2009 at 04:48:52PM +0100, "Peter Valdemar Mørch (Lists)" wrote:

> If a commit removes mention of foo one place and just accidentally  
> happens to add foo somewhere completely unrelated then it wouldn't show  
> up in the output.

Right. But think for a minute about what it means to "move". If I have:

  foo
  bar

and then change it to:

  bar
  foo

Did "foo" move, or did "bar"?

So I'm not sure that what you're asking for is necessarily well-defined.

> Would be neat with a feature that does what I thought -S did tho...

You can do:

  git log -z -p | perl -0ne 'print if /^[-+].*string/m' | tr '\0' '\n'

which I think is what you want (show any commit that has changed lines
that contain the string). But of course you will lose colorizing and
automatic paging, and it's a lot slower. And note that whether it finds
the example above will depend on how the diff is generated: did "foo"
move or did "bar"? But in practice it will generally find what you are
looking for.

> Thanks, Jeff, for both the answer and the documentation patch.

You're welcome.

-Peff

Re: git log -Sfoo ignores indentation (whitespace?) changes...

[Junio C Hamano]

"Peter Valdemar Mørch (Lists)"  <4ux6as402@sneakemail.com> writes:

> How sad... From "git log -Sfoo" looking like a really cool feature,

It is a cool feature, but its coolness lies in the other parts of a
Porcelain that is still to be written.  See my other message.

Re: git log -Sfoo ignores indentation (whitespace?) changes...

[Michael J Gruber]

Jeff King venit, vidit, dixit 03.03.2009 16:23:
> On Tue, Mar 03, 2009 at 03:28:08PM +0100, "Peter Valdemar Mørch (Lists)" wrote:
> 
>> Commits where only the indentation of 'foo' is changed are not shown with 
>> "git log -Sfoo". Is there any way to force showing them along with other 
>> changes involving foo? (E.g. for python, indentation matters!)
>>
>> Why doesn't the second commit show up in the following?
> 
> Because you misunderstand how "-S" works (but don't worry, it's not your
> fault -- the documentation is somewhat misleading). The documentation says:
> 
>   -S<string>
>     Look for differences that contain the change in <string>.
> 
> but what it actually does is find changes where the string was introduced
> or removed. So it literally counts the number of occurences before and
> after the commit, and the commit is interesting if they are not equal.

Hmm. The diffcore doc sounds more like if the filepair is picked if
#before > 0 and #after = 0, but not if #after > 0.

In any case, the pickaxe can't detect moving around of strings, right?

>> # Create text containing 'line' without whitespace
>> $ echo 'line' > text
>> $ git add text
>> $ git commit -m "first" text
>>
>> # Here, I add one space of indentation in front of 'line'
>> $ echo ' line' > text
>> $ git commit -m "second" text
> 
> So "line" wasn't actually changed. It just happens to be on a line which
> _did_ change.
> 
>> I would like to see both "first" and "second" somehow - can I do that?
> 
> I don't think there's an easy way to do this right now; you would need
> to do "git log -p" and search through the output to get what you want (I
> often do this just using the pager's search function).

If you know what your are looking for you can do variations on

git log -S'line| line' --pickaxe-regex

which seems to be different from

git log -S'line' -S' line'

which was my first attempt...

Michael

Re: git log -Sfoo ignores indentation (whitespace?) changes...

[Jeff King]

On Tue, Mar 03, 2009 at 05:02:06PM +0100, Michael J Gruber wrote:

> > but what it actually does is find changes where the string was introduced
> > or removed. So it literally counts the number of occurences before and
> > after the commit, and the commit is interesting if they are not equal.
> 
> Hmm. The diffcore doc sounds more like if the filepair is picked if
> #before > 0 and #after = 0, but not if #after > 0.

Nope, the code is #before != #after. There are some special cases around
deleted and unmerged files, but the obvious one is:

    $ sed -n 89,92p diffcore-pickaxe.c
                    else if (!diff_unmodified_pair(p) &&
                             contains(p->one, needle, len, regexp) !=
                             contains(p->two, needle, len, regexp))
                            has_changes++;

So maybe the diffcore documentation needs to be clarified.

> In any case, the pickaxe can't detect moving around of strings, right?

No, it won't (and see my other mail elsewhere in the thread for why that is
actually hard to define).

-Peff

Re: git log -Sfoo ignores indentation (whitespace?) changes...

[Junio C Hamano]

"Peter Valdemar Mørch (Lists)"  <4ux6as402@sneakemail.com> writes:

> Commits where only the indentation of 'foo' is changed are not shown
> with "git log -Sfoo". Is there any way to force showing them along
> with other changes involving foo? (E.g. for python, indentation
> matters!)

No.  You should be running "git log -p" which spawns "less" and then type
/foo to jump to the occurrences of "foo".

The pickaxe -Sfoo looks for a filepair that contains different number
of substring "foo".  It was designed as a basic building block for a very
different kind of Porcelain, whose final shape would look like the tool
described in http://article.gmane.org/gmane.comp.version-control.git/217,
but not yet written by anybody yet.

And I think it might be the good topic for a SoC project.  From some GUI,
you let the user grab a block of text, feed it to "log -S<that multi-line
string> -1" to find where that block of text last changed, and inspect the
commit you have found very carefully using things like "git grep" to find
other places in the commit that could be related to the change of the
block of text, present all of them to the user.  From there you let the
user dig deeper in the history by choosing what to look for next, most
likely giving the default selection to the block of text that roughly
corresponds to the original selection.

To make the iteration fast for this use case, the pickaxe cannot afford to
actually run diff and then grep inside the diff output (which is what you
would be doing with "log -p | less" and looking for your string in it).