{"thread":{"id":"45181","subject":"Inconsistent results of git blame --porcelain when detecting copies from other files","startedAt":"2017-02-20T17:59:16Z","lastAt":"2017-02-21T17:48:34Z","messageCount":6,"participants":["Sokolov, Konstantin","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"312156","messageId":"71BF70CE41AEE741896AF3A5450D86F11F4268FF@DEFTHW99EH3MSX.ww902.siemens.net","threadId":"45181","inReplyTo":null,"subject":"Inconsistent results of git blame --porcelain when detecting copies from other files","fromName":"Sokolov, Konstantin","fromEmail":"konstantin.sokolov.ext@siemens.com","sentAt":"2017-02-20T17:59:06Z","receivedAt":"2017-02-20T17:59:16Z","isPatch":false,"sender":{"key":"konstantin.sokolov.ext@siemens.com","avatar":null},"body":"Hi Folks!\n\nThe issue is best explained on an example. You can reproduce it using the Lucene repo https://github.com/apache/lucene-solr.git. Tested with the following versions:  1.8.1.6 (Ubuntu), 2.11.0.windows.1, 2.11.1.windows.1.\n\nFirst, let's produce the correct results without using --procelain:\n\n> git blame --show-name --show-number -s -w --abbrev=40 -C -C -C d1165b19726fa0cd13a539827a7cd43237a4feef..10ba9abeb208d37df985e95a742f756de067353f --not f5dba8b76709ff0ef8715b8b288a4b64d4993fa3 -- lucene/src/java/org/apache/lucene/index/DirectoryReader.java\n\nThe following excerpt shows lines 501-505 from the output. In particular we can see that lines 502-503 originate from IndexReader.java.\n\n10ba9abeb208d37df985e95a742f756de067353f lucene/src/java/org/apache/lucene/index/DirectoryReader.java 501 501)    * <p>This method\n^d1165b19726fa0cd13a539827a7cd43237a4fee lucene/src/java/org/apache/lucene/index/IndexReader.java     496 502)    * returns the version recorded in the commit that the\n^d1165b19726fa0cd13a539827a7cd43237a4fee lucene/src/java/org/apache/lucene/index/IndexReader.java     497 503)    * reader opened.  This version is advanced every time\n^d1165b19726fa0cd13a539827a7cd43237a4fee lucene/src/java/org/apache/lucene/index/IndexReader.java     498 504)    * a change is made with {@link IndexWriter}.</p>\n10ba9abeb208d37df985e95a742f756de067353f lucene/src/java/org/apache/lucene/index/DirectoryReader.java 505 505)    */\n\nThe same information can be obtained as well by using --line-porcelain:\n\n> git blame --show-name --show-number --line-porcelain -s -w --abbrev=40 -C -C -C d1165b19726fa0cd13a539827a7cd43237a4feef..10ba9abeb208d37df985e95a742f756de067353f --not f5dba8b76709ff0ef8715b8b288a4b64d4993fa3 -- lucene/src/java/org/apache/lucene/index/DirectoryReader.java\n\nHere is the output for line 502:\n\nd1165b19726fa0cd13a539827a7cd43237a4feef 496 502 3\nauthor Michael McCandless\nauthor-mail <mikemccand@apache.org>\nauthor-time 1327877325\nauthor-tz +0000\ncommitter Michael McCandless\ncommitter-mail <mikemccand@apache.org>\ncommitter-time 1327877325\ncommitter-tz +0000\nsummary LUCENE-3725: add optional packing to FSTs\nboundary\nfilename lucene/src/java/org/apache/lucene/index/IndexReader.java\n        * returns the version recorded in the commit that the\n\nHowever, when using --porcelain DirectoryReader.java is reported as the origin of lines 502-504:\n\n> git blame --show-name --show-number --porcelain -s -w --abbrev=40 -C -C -C d1165b19726fa0cd13a539827a7cd43237a4feef..10ba9abeb208d37df985e95a742f756de067353f --not f5dba8b76709ff0ef8715b8b288a4b64d4993fa3 -- lucene/src/java/org/apache/lucene/index/DirectoryReader.java\n\n10ba9abeb208d37df985e95a742f756de067353f 501 501 1\nauthor Uwe Schindler\nauthor-mail <uschindler@apache.org>\nauthor-time 1327879145\nauthor-tz +0000\ncommitter Uwe Schindler\ncommitter-mail <uschindler@apache.org>\ncommitter-time 1327879145\ncommitter-tz +0000\nsummary Reverse merged revision(s) from lucene/dev/trunk up to 1237502\nprevious f5dba8b76709ff0ef8715b8b288a4b64d4993fa3 lucene/src/java/org/apache/lucene/index/DirectoryReader.java\nfilename lucene/src/java/org/apache/lucene/index/DirectoryReader.java\n        * <p>This method\nd1165b19726fa0cd13a539827a7cd43237a4feef 496 502 3\n        * returns the version recorded in the commit that the\nd1165b19726fa0cd13a539827a7cd43237a4feef 497 503\n        * reader opened.  This version is advanced every time\nd1165b19726fa0cd13a539827a7cd43237a4feef 498 504\n\nThis is not only inconsistent with the other outputs but the output is also inconsistent in itself because lines 496 -498 do not even exist in a previous version of DirectoryReader.java.\n\nThanks for any feedback.\n\nKind Regards\nKonstantin Sokolov\n"},{"id":"312171","messageId":"xmqqd1ecim8a.fsf@gitster.mtv.corp.google.com","threadId":"45181","inReplyTo":"71BF70CE41AEE741896AF3A5450D86F11F4268FF@DEFTHW99EH3MSX.ww902.siemens.net","subject":"Re: Inconsistent results of git blame --porcelain when detecting copies from other files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-20T21:30:29Z","receivedAt":"2017-02-20T21:30:36Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Sokolov, Konstantin\" <konstantin.sokolov.ext@siemens.com> writes:\n\n> However, when using --porcelain DirectoryReader.java is reported as the origin of lines 502-504:\n> ...\n> This is not only inconsistent with the other outputs but the output is also inconsistent in itself because lines 496 -498 do not even exist in a previous version of DirectoryReader.java.\n\nHmph, this sounds vaguely familiar with\n\n    http://public-inbox.org/git/20170106042051.nwjiuyyp7ljhs4sr@sigill.intra.peff.net\n\nwhich is part of Git 2.12-rc0\n\n\n\n\n"},{"id":"312172","messageId":"20170220221540.6vemjdvyvwonpqyt@sigill.intra.peff.net","threadId":"45181","inReplyTo":"xmqqd1ecim8a.fsf@gitster.mtv.corp.google.com","subject":"Re: Inconsistent results of git blame --porcelain when detecting copies from other files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-20T22:15:40Z","receivedAt":"2017-02-20T22:15:48Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 20, 2017 at 01:30:29PM -0800, Junio C Hamano wrote:\n\n> \"Sokolov, Konstantin\" <konstantin.sokolov.ext@siemens.com> writes:\n> \n> > However, when using --porcelain DirectoryReader.java is reported as the origin of lines 502-504:\n> > ...\n> > This is not only inconsistent with the other outputs but the output is also inconsistent in itself because lines 496 -498 do not even exist in a previous version of DirectoryReader.java.\n> \n> Hmph, this sounds vaguely familiar with\n> \n>     http://public-inbox.org/git/20170106042051.nwjiuyyp7ljhs4sr@sigill.intra.peff.net\n> \n> which is part of Git 2.12-rc0\n\nYeah, I had the same thought while reading Konstantin's report.\n\nI'm not sure Git is wrong, though. I think it's just the way the\nporcelain output works.\n\nHere's a minimal reproduction; the interesting thing is when a commit is\nmentioned twice (as happens on lines 1 and 5 here):\n\n  git init repo\n  cd repo\n  \n  # use long lines to make sure we trigger content-movement detection\n  for i in $(seq 1 5); do\n  \techo this is really long line number $i\n  done >file\n  git add file\n  git commit -m initial\n  \n  sed 's/1/one/; s/5/five/' <file >renamed\n  git rm file\n  git add renamed\n  git commit -m 'rename and use english'\n  \n  git blame renamed\n  git blame --line-porcelain renamed\n  git blame --porcelain renamed\n\nThe first blame output looks something like this:\n\n  bab03701 renamed ... line number 1\n  ^dda1349 file    ... line number 2\n  ^dda1349 file    ... line number 3\n  ^dda1349 file    ... line number 4\n  bab03701 renamed ... line number 5\n\nso we can see it's the same case. The --line-porcelain similarly matches\nthe commits and filenames.\n\nBut the --porcelain output is:\n\n  bab037010dcabaf0509db27bf232d25659b180fa 1 1 1\n  ...\n  filename renamed\n          this is really long line number one\n  dda1349d41da859f4c37e018dbed714ba6c1aa18 2 2 3\n  ...\n  filename file\n          this is really long line number 2\n  dda1349d41da859f4c37e018dbed714ba6c1aa18 3 3\n          this is really long line number 3\n  dda1349d41da859f4c37e018dbed714ba6c1aa18 4 4\n          this is really long line number 4\n  bab037010dcabaf0509db27bf232d25659b180fa 5 5 1\n          this is really long line number five\n\nYou might be tempted to say that the fifth line comes from \"filename\nfile\", because that was the last \"filename\" entry we saw. But that's\n_not_ how the porcelain output works. That \"filename\" entry was\nassociated with dda1349, but the line comes from bab0370 here.\n\nThe simplest way (IMHO) to parse --porcelain output is:\n\n  - maintain a mapping of commit sha1s to the commit's details\n\n  - whenever you see a \"<sha1> <line_nr> <orig_nr> [<size-of-hunk>]\"\n    line, any key-value fields which follow impact _only_ that sha1, and\n    you should update the details for that map entry\n\n  - when you see the actual tab-indented line content, you have gotten\n    all of the key-value updates for that sha1. You can now safely do\n    what you like with the line entry.\n\nAnother way, if you don't want to update your mapping, is to actually\npay attention to the size-of-hunk headers. In this case the middle three\nlines come in their own hunk (which you can see from the \"2 2 3\" header\non the second line). The \"filename\" field we get applies to that hunk,\nbut once we switch to a different one, the filename field needs to be\nlooked up in the commit mapping.\n\nBut it's definitely not correct to blindly apply one \"filename\" field to\nsubsequent lines in other hunks.\n\nAnd yes, I do think this is probably more complex than it needs to be.\nI didn't write it. And I don't think it is worth the backwards\ncompatibility headache of trying to change it now. It's possible this\ncould be better documented (I didn't look at the documentation to write\nthat explanation; I happened to puzzle it out for somebody else\nrecently who had a similar case. That's what led to the bug-fix in the\nmessage you linked).\n\n-Peff\n"},{"id":"312189","messageId":"xmqqzihggjos.fsf@gitster.mtv.corp.google.com","threadId":"45181","inReplyTo":"20170220221540.6vemjdvyvwonpqyt@sigill.intra.peff.net","subject":"Re: Inconsistent results of git blame --porcelain when detecting copies from other files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-21T06:08:19Z","receivedAt":"2017-02-21T06:09:56Z","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> The simplest way (IMHO) to parse --porcelain output is:\n>\n>   - maintain a mapping of commit sha1s to the commit's details\n>\n>   - whenever you see a \"<sha1> <line_nr> <orig_nr> [<size-of-hunk>]\"\n>     line, any key-value fields which follow impact _only_ that sha1, and\n>     you should update the details for that map entry\n>\n>   - when you see the actual tab-indented line content, you have gotten\n>     all of the key-value updates for that sha1. You can now safely do\n>     what you like with the line entry.\n\nYup, that was how the output was meant to be read.  At least in the\nmind of the person who designed the output format ;-)\n\n"},{"id":"312199","messageId":"71BF70CE41AEE741896AF3A5450D86F11F42694F@DEFTHW99EH3MSX.ww902.siemens.net","threadId":"45181","inReplyTo":"20170220221540.6vemjdvyvwonpqyt@sigill.intra.peff.net","subject":"AW: Inconsistent results of git blame --porcelain when detecting copies from other files","fromName":"Sokolov, Konstantin","fromEmail":"konstantin.sokolov.ext@siemens.com","sentAt":"2017-02-21T12:25:37Z","receivedAt":"2017-02-21T12:25:58Z","isPatch":false,"sender":{"key":"konstantin.sokolov.ext@siemens.com","avatar":null},"body":"Thanks for going into the issue. As far as I understand 2.12 won't change the discussed behavior of --procelain. We will switch to --line-procelain. After the current discussion it seems to be less error prone, more future-proof and our current parser can handle it without any changes.\n\nRegards\nKonstantin\n\n-----Ursprüngliche Nachricht-----\nVon: Jeff King [mailto:peff@peff.net]\nGesendet: Montag, 20. Februar 2017 23:16\nAn: Junio C Hamano\nCc: Sokolov, Konstantin (ext) (CT RDA SSI ADM-DE); git@vger.kernel.org\nBetreff: Re: Inconsistent results of git blame --porcelain when detecting copies from other files\n\nOn Mon, Feb 20, 2017 at 01:30:29PM -0800, Junio C Hamano wrote:\n\n> \"Sokolov, Konstantin\" <konstantin.sokolov.ext@siemens.com> writes:\n>\n> > However, when using --porcelain DirectoryReader.java is reported as the origin of lines 502-504:\n> > ...\n> > This is not only inconsistent with the other outputs but the output is also inconsistent in itself because lines 496 -498 do not even exist in a previous version of DirectoryReader.java.\n>\n> Hmph, this sounds vaguely familiar with\n>\n>\n> http://public-inbox.org/git/20170106042051.nwjiuyyp7ljhs4sr@sigill.int\n> ra.peff.net\n>\n> which is part of Git 2.12-rc0\n\nYeah, I had the same thought while reading Konstantin's report.\n\nI'm not sure Git is wrong, though. I think it's just the way the porcelain output works.\n\nHere's a minimal reproduction; the interesting thing is when a commit is mentioned twice (as happens on lines 1 and 5 here):\n\n  git init repo\n  cd repo\n\n  # use long lines to make sure we trigger content-movement detection\n  for i in $(seq 1 5); do\n        echo this is really long line number $i\n  done >file\n  git add file\n  git commit -m initial\n\n  sed 's/1/one/; s/5/five/' <file >renamed\n  git rm file\n  git add renamed\n  git commit -m 'rename and use english'\n\n  git blame renamed\n  git blame --line-porcelain renamed\n  git blame --porcelain renamed\n\nThe first blame output looks something like this:\n\n  bab03701 renamed ... line number 1\n  ^dda1349 file    ... line number 2\n  ^dda1349 file    ... line number 3\n  ^dda1349 file    ... line number 4\n  bab03701 renamed ... line number 5\n\nso we can see it's the same case. The --line-porcelain similarly matches the commits and filenames.\n\nBut the --porcelain output is:\n\n  bab037010dcabaf0509db27bf232d25659b180fa 1 1 1\n  ...\n  filename renamed\n          this is really long line number one\n  dda1349d41da859f4c37e018dbed714ba6c1aa18 2 2 3\n  ...\n  filename file\n          this is really long line number 2\n  dda1349d41da859f4c37e018dbed714ba6c1aa18 3 3\n          this is really long line number 3\n  dda1349d41da859f4c37e018dbed714ba6c1aa18 4 4\n          this is really long line number 4\n  bab037010dcabaf0509db27bf232d25659b180fa 5 5 1\n          this is really long line number five\n\nYou might be tempted to say that the fifth line comes from \"filename file\", because that was the last \"filename\" entry we saw. But that's _not_ how the porcelain output works. That \"filename\" entry was associated with dda1349, but the line comes from bab0370 here.\n\nThe simplest way (IMHO) to parse --porcelain output is:\n\n  - maintain a mapping of commit sha1s to the commit's details\n\n  - whenever you see a \"<sha1> <line_nr> <orig_nr> [<size-of-hunk>]\"\n    line, any key-value fields which follow impact _only_ that sha1, and\n    you should update the details for that map entry\n\n  - when you see the actual tab-indented line content, you have gotten\n    all of the key-value updates for that sha1. You can now safely do\n    what you like with the line entry.\n\nAnother way, if you don't want to update your mapping, is to actually pay attention to the size-of-hunk headers. In this case the middle three lines come in their own hunk (which you can see from the \"2 2 3\" header on the second line). The \"filename\" field we get applies to that hunk, but once we switch to a different one, the filename field needs to be looked up in the commit mapping.\n\nBut it's definitely not correct to blindly apply one \"filename\" field to subsequent lines in other hunks.\n\nAnd yes, I do think this is probably more complex than it needs to be.\nI didn't write it. And I don't think it is worth the backwards compatibility headache of trying to change it now. It's possible this could be better documented (I didn't look at the documentation to write that explanation; I happened to puzzle it out for somebody else recently who had a similar case. That's what led to the bug-fix in the message you linked).\n\n-Peff\n"},{"id":"312209","messageId":"20170221174828.ddu3vjal4meafc7f@sigill.intra.peff.net","threadId":"45181","inReplyTo":"71BF70CE41AEE741896AF3A5450D86F11F42694F@DEFTHW99EH3MSX.ww902.siemens.net","subject":"Re: Inconsistent results of git blame --porcelain when detecting copies from other files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-21T17:48:28Z","receivedAt":"2017-02-21T17:48:34Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 21, 2017 at 12:25:37PM +0000, Sokolov, Konstantin wrote:\n\n> Thanks for going into the issue. As far as I understand 2.12 won't\n> change the discussed behavior of --procelain. We will switch to\n> --line-procelain. After the current discussion it seems to be less\n> error prone, more future-proof and our current parser can handle it\n> without any changes.\n\nRight, the 2.12 change is only fixing a case where the \"previous\"\nkey/value line was not repeated at the correct spots. The same parsing\nrules still hold.\n\n-Peff\n"}]}