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

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

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

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.

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

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

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

2009/3/3 Jeff King <peff@peff.net>:

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

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

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

2009/3/3 Jeff King <peff@peff.net>:

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

Jeff King <peff@peff.net> writes:

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.

Look for differences that change the number of occurrences of <string>?

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

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.

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

2009/3/3 Jeff King <peff@peff.net>:

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

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

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

John Tapsell <johnflux@gmail.com> writes:

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.

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

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

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.

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.

You're welcome.

-Peff

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

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.

Jeff King venit, vidit, dixit 03.03.2009 16:23:

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?

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

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

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.

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

-Peff

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

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