{"thread":{"id":"26299","subject":"gitk \"find commit adding/removing string\"/possible pickaxe bug?","startedAt":"2011-01-18T16:16:18Z","lastAt":"2011-01-31T20:00:03Z","messageCount":10,"participants":["Sebastian Hahn","Thomas Rast","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"159585","messageId":"514EB3AA-CD31-4BDB-B777-B7AAEEDF5663@sebastianhahn.net","threadId":"26299","inReplyTo":null,"subject":"gitk \"find commit adding/removing string\"/possible pickaxe bug?","fromName":"Sebastian Hahn","fromEmail":"mail@sebastianhahn.net","sentAt":"2011-01-18T16:16:18Z","receivedAt":"2011-01-18T16:16:18Z","isPatch":false,"sender":{"key":"mail@sebastianhahn.net","avatar":null},"body":"Hi,\n\nI quite like gitk and am a fan of the ability to easily locate commits\nwhere a specific string was added/removed. If the string in question\nwas added in a merge commit as part of a conflicted/otherwise\nchanged merge, gitk doesn't display it.\n\n(If you want to reproduce, the repository is git://git.torproject.org/ \ntor,\nthe string I'm looking for is \"< DIGEST\" and the first commit I hope to\nfind is ed87738ede789fb).\n\nI presented the issue to #git, and it was suggested that it is probably\na pickaxe bug in that it doesn't display changes in merge commits if\nthey add strings that neither of their parents has.\n\nDo you agree that this is a bug, or am I missing anything here?\n\nThanks for you consideration!\n\nSebastian\n"},{"id":"159587","messageId":"201101181744.18139.trast@student.ethz.ch","threadId":"26299","inReplyTo":"514EB3AA-CD31-4BDB-B777-B7AAEEDF5663@sebastianhahn.net","subject":"Re: gitk \"find commit adding/removing string\"/possible pickaxe bug?","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-01-18T16:44:17Z","receivedAt":"2011-01-18T16:44:17Z","isPatch":false,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Sebastian Hahn wrote:\n> \n> I quite like gitk and am a fan of the ability to easily locate commits\n> where a specific string was added/removed. If the string in question\n> was added in a merge commit as part of a conflicted/otherwise\n> changed merge, gitk doesn't display it.\n[...]\n> I presented the issue to #git, and it was suggested that it is probably\n> a pickaxe bug\n\nIn particular, in a history where\n\n  $ git show HEAD:foo\n  quux\n  $ git show HEAD^:foo\n  bar\n  $ git show HEAD^2:foo\n  baz\n\nthe behaviour is:\n\n  git log -Squux                  # empty\n  git log -Squux -p               # empty\n  git log -Squux --pickaxe-all    # empty\n\n  git log -Squux -c      \t  # shows merge, but no diff\n  git log -Squux --cc    \t  # shows merge, but no diff\n  git log -Squux -c -p   \t  # shows merge, but no diff\n  git log -Squux -c --pickaxe-all # shows merge, but no diff\n\n  git log -Squux --pickaxe-all -c -p  # shows merge & combined diff\n\nSo it only shows the diff with --pickaxe-all, even though the (only)\nhunk clearly introduced the string.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"159590","messageId":"7v8vyikqdg.fsf@alter.siamese.dyndns.org","threadId":"26299","inReplyTo":"201101181744.18139.trast@student.ethz.ch","subject":"Re: gitk \"find commit adding/removing string\"/possible pickaxe bug?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-01-18T18:39:55Z","receivedAt":"2011-01-18T18:39:55Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast <trast@student.ethz.ch> writes:\n\n>   git log -Squux --pickaxe-all -c -p  # shows merge & combined diff\n>\n> So it only shows the diff with --pickaxe-all, even though the (only)\n> hunk clearly introduced the string.\n\nIsn't it just because unless you ask for -m to get all the pairwise diff\nbetween each parent and the merged child the diff machinery won't kick in\nfor merge commits?  I didn't check the option defaulting logic, but there\nmay be something that changes the default setting of \"-m\" among your\nexamples.\n"},{"id":"159591","messageId":"20110118185027.GA10562@sigill.intra.peff.net","threadId":"26299","inReplyTo":"201101181744.18139.trast@student.ethz.ch","subject":"Re: gitk \"find commit adding/removing string\"/possible pickaxe bug?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-01-18T18:50:28Z","receivedAt":"2011-01-18T18:50:28Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 18, 2011 at 05:44:17PM +0100, Thomas Rast wrote:\n\n> In particular, in a history where\n> \n>   $ git show HEAD:foo\n>   quux\n>   $ git show HEAD^:foo\n>   bar\n>   $ git show HEAD^2:foo\n>   baz\n\nI created a similar repo with:\n\ncommit() {\n  echo $1 >file && git add file && git commit -m $1\n}\nmkdir repo && cd repo && git init\ncommit base\ncommit master\ngit checkout -b other HEAD^\ncommit other\ngit merge master\ncommit resolved\n\nwhich should be identical. But I get different results (see near the\nend):\n\n> the behaviour is:\n> \n>   git log -Squux                  # empty\n>   git log -Squux -p               # empty\n\nAll of which make sense to me. Pickaxe operates on diff filepairs, and\ngit by default doesn't seem to do merge diffing at all (but see below).\nSo those filepairs don't exist to consider.\n\n>   git log -Squux --pickaxe-all    # empty\n\nThis doesn't help. It just loosens the actual diff shown from \"just the\nthings that matched -S\" to \"everything in that commit\". It doesn't\nadd to the filepairs that make it to pickaxe.\n\n>   git log -Squux -c      \t  # shows merge, but no diff\n\nThis \"-c\" does what you want, because we start looking at merge\nfilepairs. Although one thing leaves me confused. If I do:\n\n  git log -p\n\nI get no diff for the merge commit. But in git-diff(1), it says:\n\n  COMBINED DIFF FORMAT\n    \"git-diff-tree\", \"git-diff-files\" and \"git-diff\" can take -c or --cc\n    option to produce combined diff. For showing a merge commit with\n    \"git log -p\", this is the default format; you can force showing full\n    diff with the -m option.\n\nwhich implies to me that \"-c\" should be on by default if we selected\n\"-p\" (or presumably -S).\n\nI didn't bisect, but I wonder if the doc is wrong, or if we accidentally\nlost this default at some point.\n\n>   git log -Squux --cc    \t  # shows merge, but no diff\n\nMakes sense again, since you didn't ask for a patch, no patch.\n\n>   git log -Squux -c -p   \t  # shows merge, but no diff\n\nWeird.  Here I get a nice combined diff, which is what I expect.\n\n>   git log -Squux -c --pickaxe-all # shows merge, but no diff\n\nYep, no \"-p\" again.\n\n>   git log -Squux --pickaxe-all -c -p  # shows merge & combined diff\n\nThe pickaxe-all shouldn't impact anything, at least not in my test repo,\nas there is only one file. But of course I do get the diff, as I did\nabove. Can you show the steps to create your repo? I'm wondering what is\ndifferent.\n\n-Peff\n"},{"id":"159595","messageId":"201101182139.28808.trast@student.ethz.ch","threadId":"26299","inReplyTo":"20110118185027.GA10562@sigill.intra.peff.net","subject":"Re: gitk \"find commit adding/removing string\"/possible pickaxe bug?","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-01-18T20:39:28Z","receivedAt":"2011-01-18T20:39:28Z","isPatch":false,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Jeff King wrote:\n> commit() {\n>   echo $1 >file && git add file && git commit -m $1\n> }\n> mkdir repo && cd repo && git init\n> commit base\n> commit master\n> git checkout -b other HEAD^\n> commit other\n> git merge master\n> commit resolved\n\nIndeed, my history looks just like that.\n\n> >   git log -Squux -c -p          # shows merge, but no diff\n> \n> Weird.  Here I get a nice combined diff, which is what I expect.\n\nTrue, I managed to confuse myself between looking for the resolution and\nlooking for one of the (deleted) merge sides.\n\nSo indeed\n\n  git log -Squux -c -p\n\ngives a combined diff.  But OTOH\n\n  git log -Sbar -c -p\n\ndoesn't; it only gives a diff for the commit that introduced 'bar'.  I\nguess this makes sense: -S notices that the number of 'bar's is\nactually the same as in *one* merge parent, hence the merge cannot be\nall that interesting.  OTOH it still shows the merge commit in the\nhistory, which is a bit strange.  --pickaxe-all does not make a\ndifference either;\n\n   git log -Sbar --cc -p --pickaxe-all\n\nstill shows the merge commit but no diff.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"159596","messageId":"20110118205040.GA20970@sigill.intra.peff.net","threadId":"26299","inReplyTo":"201101182139.28808.trast@student.ethz.ch","subject":"Re: gitk \"find commit adding/removing string\"/possible pickaxe bug?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-01-18T20:50:40Z","receivedAt":"2011-01-18T20:50:40Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 18, 2011 at 09:39:28PM +0100, Thomas Rast wrote:\n\n> So indeed\n> \n>   git log -Squux -c -p\n> \n> gives a combined diff.  But OTOH\n> \n>   git log -Sbar -c -p\n> \n> doesn't; it only gives a diff for the commit that introduced 'bar'.  I\n> guess this makes sense: -S notices that the number of 'bar's is\n> actually the same as in *one* merge parent, hence the merge cannot be\n> all that interesting.  OTOH it still shows the merge commit in the\n> history, which is a bit strange.  --pickaxe-all does not make a\n> difference either;\n\nHrm. What I expected[1] to happen would be for the diff machinery to\nlook at each filepair individually, one of them to trigger -S, which\nshows the commit, and then to fail to produce a combined diff because we\nthrew away the other uninteresting filepair. But in that case,\n--pickaxe-all _should_ show something, as its point is to keep all of\nthe filepairs.  And that's clearly not happening.\n\nSo now I don't know what's going on. I'll try to trace through the diff\nmachinery and see if that gives a clue.\n\n-Peff\n\n[1] That's what I expect, but not necessarily what I want. I think what\nI would want is for it to do a token count of the merge commit, and if\nit fails to match _every_ parent, then it it interesting. Otherwise, the\ncontent presumably came from that parent.\n"},{"id":"159598","messageId":"201101182216.01772.trast@student.ethz.ch","threadId":"26299","inReplyTo":"201101182139.28808.trast@student.ethz.ch","subject":"Re: gitk \"find commit adding/removing string\"/possible pickaxe bug?","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-01-18T21:16:01Z","receivedAt":"2011-01-18T21:16:01Z","isPatch":false,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Thomas Rast wrote:\n> So indeed\n> \n>   git log -Squux -c -p\n> \n> gives a combined diff.  But OTOH\n> \n>   git log -Sbar -c -p\n> \n> doesn't; it only gives a diff for the commit that introduced 'bar'.\n\nIt's actually even stranger.  -S does not seem to filter merges at\nall.  For example, in git.git\n\n  $ git log | grep -c ^Merge\n  4677\n  $ git log -Sthis_string_never_existed_anywhere -c | grep -c ^Merge\n  4677\n\nI think it should just filter all history, shouldn't it?  After all\n\n  $ git log -- this_file_never_existed\n\nalso comes up empty.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"159600","messageId":"7vsjwpkio8.fsf@alter.siamese.dyndns.org","threadId":"26299","inReplyTo":"20110118205040.GA20970@sigill.intra.peff.net","subject":"Re: gitk \"find commit adding/removing string\"/possible pickaxe bug?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-01-18T21:26:15Z","receivedAt":"2011-01-18T21:26:15Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> [1] That's what I expect, but not necessarily what I want. I think what\n> I would want is for it to do a token count of the merge commit, and if\n> it fails to match _every_ parent, then it it interesting. Otherwise, the\n> content presumably came from that parent.\n\nHonestly, my guess is that the interaction of -S with a merge commit is\n\"whatever the code happens to do\", as I didn't think nor design how they\nshould interact with each other when I wrote -c/--cc nor when I wrote -S.\nIf I recall correctly -S codepath predates -c/--cc by a wide margin, and I\nwouldn't be surprised at all if pickaxe doesn't work as expected (by\nanybody's definition of \"expectation\"), unless you are looking at \"-p -m\"\noutput, not a combined one.\n\nHaving said that, I tend to agree with your latter expectation (\"what I\nwant\").\n\nBy the way, you guys should really not be looking at the disused\nplumbing-helper -S but instead be advocating its newer and more human\nfriendly cousin -G.  1.7.4 is coming ;-).\n"},{"id":"159602","messageId":"20110118213309.GA933@sigill.intra.peff.net","threadId":"26299","inReplyTo":"7vsjwpkio8.fsf@alter.siamese.dyndns.org","subject":"Re: gitk \"find commit adding/removing string\"/possible pickaxe bug?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-01-18T21:33:09Z","receivedAt":"2011-01-18T21:33:09Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 18, 2011 at 01:26:15PM -0800, Junio C Hamano wrote:\n\n> Honestly, my guess is that the interaction of -S with a merge commit is\n> \"whatever the code happens to do\", as I didn't think nor design how they\n> should interact with each other when I wrote -c/--cc nor when I wrote -S.\n\nThat's kind of what I figured.\n\n> Having said that, I tend to agree with your latter expectation (\"what I\n> want\").\n\nI'll take a look and see how painful it will be to do that. I'm not even\nsure how the merge filepairs are represented by the diff code (since\nthey are not even pairs, but rather one child with many parents).\n\n> By the way, you guys should really not be looking at the disused\n> plumbing-helper -S but instead be advocating its newer and more human\n> friendly cousin -G.  1.7.4 is coming ;-).\n\nIt's _way_ slower for simple things:\n\n  $ time git log -Sfoo >/dev/null\n  real    0m11.550s\n  user    0m11.409s\n  sys     0m0.116s\n\n  $ time git log -Gfoo >/dev/null\n  real    0m25.722s\n  user    0m25.442s\n  sys     0m0.220s\n\nI for one really like the -S behavior, anyway, but I expect I am in the\nminority.\n\n-Peff\n"},{"id":"160106","messageId":"20110131200002.GA7859@sigill.intra.peff.net","threadId":"26299","inReplyTo":"20110118205040.GA20970@sigill.intra.peff.net","subject":"Re: gitk \"find commit adding/removing string\"/possible pickaxe bug?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-01-31T20:00:03Z","receivedAt":"2011-01-31T20:00:03Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 18, 2011 at 03:50:40PM -0500, Jeff King wrote:\n\n> > doesn't; it only gives a diff for the commit that introduced 'bar'.  I\n> > guess this makes sense: -S notices that the number of 'bar's is\n> > actually the same as in *one* merge parent, hence the merge cannot be\n> > all that interesting.  OTOH it still shows the merge commit in the\n> > history, which is a bit strange.  --pickaxe-all does not make a\n> > difference either;\n> \n> Hrm. What I expected[1] to happen would be for the diff machinery to\n> look at each filepair individually, one of them to trigger -S, which\n> shows the commit, and then to fail to produce a combined diff because we\n> threw away the other uninteresting filepair. But in that case,\n> --pickaxe-all _should_ show something, as its point is to keep all of\n> the filepairs.  And that's clearly not happening.\n> \n> So now I don't know what's going on. I'll try to trace through the diff\n> machinery and see if that gives a clue.\n> \n> -Peff\n> \n> [1] That's what I expect, but not necessarily what I want. I think what\n> I would want is for it to do a token count of the merge commit, and if\n> it fails to match _every_ parent, then it it interesting. Otherwise, the\n> content presumably came from that parent.\n\nI looked into this, and sadly the \"wanted\" behavior I described above is\nnot easy to do. It turns out that we never actually see the whole 3-way\ndiff as a single unit in diffcore-pickaxe. Instead, log-tree calls into\ndiff_tree_combined, which diffs each parent _individually_, including\nrunning diffcore magic on it. And then if one of those appears\ninteresting, we show the merge.\n\nSo diffcore-pickaxe never even knows that we are doing a combined diff.\nIt just sees the difference between M and M^, and then separately the\ndifference between M and M^2. This works OK in my example:\n\n  commit() {\n    echo $1 >file && git add file && git commit -m $1\n  }\n  commit base\n  commit master\n  git checkout -b other HEAD^\n  commit other\n  git merge master\n  commit resolved\n\nas doing \"git log -Sother -c\" will show both the commit \"other\" _and_\nthe merge commit (since it removed \"other\" in favor of \"resolved\"). But\nyou could also construct a case where it isn't true. For example,\nconsider a case where two sides add the same token, and the resolution\nis to keep both. E.g.:\n\n  echo base >file && git add file && git commit -m base\n  echo foo bar >file && git commit -a -m master\n  git checkout -b other HEAD^\n  echo foo baz >file && git commit -a -m other\n  git merge master\n  (echo foo bar; echo foo baz) >file && git commit -a -m resolved\n\nThat shows the merge commit, even though it didn't actually introduce or\ndelete that token at all. OTOH, it is part of a conflict region, so it\nis really difficult to say whether it is interesting or not. I dunno\nwhat the right semantics are (and note that the definition I gave in the\nabove email would also trigger on this case).\n\nI have the nagging feeling there is another less ambiguous corner case\nthat is wrong, but I'm having trouble constructing one.\n\n\nAnyway, the real point is that we can't do anything special to pickaxe\nmerge commits at the diffcore level without some pretty major diff\nsurgery. So where does that leave us? You can still get pretty\nreasonable results from turning on \"-c\". I was curious what the CPU cost\nwas of turning \"-c\" on by default, and was very surprised by the\nresults (in git.git):\n\n  $ time git log -Sfoo >/dev/null\n  real    0m11.532s\n  user    0m11.273s\n  sys     0m0.116s\n\n  $ time git log -c -Sfoo >/dev/null\n  real    3m7.530s\n  user    3m3.991s\n  sys     0m2.948s\n\nA 1700% slowdown? Wow. There are ~20000 non-merge commits in git.git and\n~4500 merge commits. Each merge commit has two parents (since we don't\ntend to octopus merge), each of which is diffed individually. So I'd\nexpect it to add about 9000 diffs, or roughly 50% on top of the\n11-second case.\n\nMy guess is that the subtree merges from gitk and git-gui are very\nexpensive to look at, since from one parent's perspective we will have\ncreated the entire git project from scratch. On every merge. Yikes.\n\n-Peff\n"}]}