{"thread":{"id":"54006","subject":"`git blame` Line Number Off-by-one","startedAt":"2020-08-07T01:13:05Z","lastAt":"2020-08-13T18:42:03Z","messageCount":16,"participants":["Nuthan Munaiah","Jeff King","Junio C Hamano","Barret Rhoden"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"403109","messageId":"emc6590292-832a-4a35-8815-d5707731d605@sanctum","threadId":"54006","inReplyTo":null,"subject":"`git blame` Line Number Off-by-one","fromName":"Nuthan Munaiah","fromEmail":"nm6061@rit.edu","sentAt":"2020-08-07T01:05:51Z","receivedAt":"2020-08-07T01:13:05Z","isPatch":false,"sender":{"key":"nm6061@rit.edu","avatar":null},"body":"What did you do before the bug happened? (Steps to reproduce your issue)\n\n * Clone https://github.com/apache/tomcat\n * Run `git blame --root -leftw -L 21,21 -L 23,23 \n51844327d8613448bb0bf9667e1a61e462e2043c^ -- \nmodules/jdbc-pool/java/org/apache/tomcat/jdbc/pool/PoolProperties.java`\n\nWhat did you expect to happen? (Expected behavior)\n\n`git blame` shows the last commit that modified lines 21 and 23 of \n`modules/jdbc-pool/java/org/apache/tomcat/jdbc/pool/PoolProperties.java` \nstarting at the parent of `51844327d8613448bb0bf9667e1a61e462e2043c`.\n\n```\n$ git blame --root -leftw -L 21,21 -L 23,23 \n51844327d8613448bb0bf9667e1a61e462e2043c^ -- \nmodules/jdbc-pool/java/org/apache/tomcat/jdbc/pool/PoolProperties.java\nc65a429f06f4e4a025a306e377211863d9ff2a0c \nmodules/jdbc-pool/java/org/apache/tomcat/jdbc/pool/PoolProperties.java \n( 1226896977 +0000 21) import java.util.ArrayList;\nc65a429f06f4e4a025a306e377211863d9ff2a0c \nmodules/jdbc-pool/java/org/apache/tomcat/jdbc/pool/PoolProperties.java \n( 1226896977 +0000 23) import java.util.List;\n```\n\nWhat happened instead? (Actual behavior)\n\nLine 23 is not shown in the `git blame` output. Instead, line 22 is \nshown.\n\n```\n$ git blame --root -leftw -L 21,21 -L 23,23 \n51844327d8613448bb0bf9667e1a61e462e2043c^ -- \nmodules/jdbc-pool/java/org/apache/tomcat/jdbc/pool/PoolProperties.java\nc65a429f06f4e4a025a306e377211863d9ff2a0c \nmodules/jdbc-pool/java/org/apache/tomcat/jdbc/pool/PoolProperties.java \n( 1226896977 +0000 21) import java.util.ArrayList;\nc65a429f06f4e4a025a306e377211863d9ff2a0c \nmodules/jdbc-pool/java/org/apache/tomcat/jdbc/pool/PoolProperties.java \n( 1226896977 +0000 22) import java.util.HashMap;\n```\n\nWhat's different between what you expected and what actually happened?\n\nLine 23 is not shown in the `git blame` output. Instead line 22 is \nshown.\n\nAnything else you want to add:\n\n * The issue is reproducible on git versions 2.28.0 (on Ubuntu 18.04.4 \nLTS), 2.24.0 (on Ubuntu 18.04.3 LTS), 2.17.0 (on Ubuntu 18.04.4 LTS), \nand 2.27.0.windows.1 (on Microsoft Windows 10 Enterprise 10.0.19041 \nBuild 19041).\n * The addition of `--porcelain` does not resolve the issue.\n * The issue is not specific to a particular repository. For instance, \nthe output from `git blame -L 1466,1466 -L 1468,1468 \n35e69fc7cf9421ab04ffc9d52cb36d07fa12984a^ -- c/dec/decode.c` in a clone \nof https://github.com/google/brotli shows last commit that modified \nlines 1466 and 1467 (instead 1468).\n * The issue seems to present itself only when there is exactly one line \nin between two specifications of `-L`. For instance, `-L 20,20 -L 23,23` \ncorrectly blames lines 20 and 23, `-L 20,20 -L 21,21 -L 22,22 -L 23,23` \ncorrectly blames lines 20, 21, 22, and 23, `-L 23,23` correctly blames \nline 23, but `-L 21,21 -L 23,23` blames lines 21 and 22 (instead of 23).\n\n[System Info]\ngit version:\ngit version 2.28.0\ncpu: x86_64\nno commit associated with this build\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nuname: Linux 4.15.0-88-generic #88-Ubuntu SMP Tue Feb 11 20:11:34 UTC \n2020 x86_64\ncompiler info: gnuc: 7.5\nlibc info: glibc: 2.27\n$SHELL (typically, interactive shell): \n\n[Enabled Hooks]\n\n"},{"id":"403158","messageId":"20200807212159.GA1871940@coredump.intra.peff.net","threadId":"54006","inReplyTo":"emc6590292-832a-4a35-8815-d5707731d605@sanctum","subject":"Re: `git blame` Line Number Off-by-one","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-07T21:21:59Z","receivedAt":"2020-08-07T21:22:02Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 07, 2020 at 01:05:51AM +0000, Nuthan Munaiah wrote:\n\n> * Clone https://github.com/apache/tomcat\n> * Run `git blame --root -leftw -L 21,21 -L 23,23\n> 51844327d8613448bb0bf9667e1a61e462e2043c^ --\n> modules/jdbc-pool/java/org/apache/tomcat/jdbc/pool/PoolProperties.java`\n>\n> [...]\n> \n> `git blame` shows the last commit that modified lines 21 and 23 of\n> `modules/jdbc-pool/java/org/apache/tomcat/jdbc/pool/PoolProperties.java`\n> starting at the parent of `51844327d8613448bb0bf9667e1a61e462e2043c`.\n>\n> [...]\n>\n> Line 23 is not shown in the `git blame` output. Instead, line 22 is shown.\n\nThanks for providing an easy reproduction case.\n\nI think the issue is not in the -L input or in the blame algorithm\nitself, but in the hunk-coalescing at the end.\n\nAs you note, this shows up even with --porcelain:\n\n $ commit=51844327d8613448bb0bf9667e1a61e462e2043c^\n $ fn=modules/jdbc-pool/java/org/apache/tomcat/jdbc/pool/PoolProperties.java\n $ git blame --porcelain -L 21,21 -L 23,23 $commit -- $fn |\n egrep '^[0-9a-f]{40}'\n c65a429f06f4e4a025a306e377211863d9ff2a0c 21 21 2\n c65a429f06f4e4a025a306e377211863d9ff2a0c 22 22\n\nbut if we try --incremental:\n\n $ git blame --incremental -L 21,21 -L 23,23 $commit -- $fn |\n egrep '^[0-9a-f]{40}'\n c65a429f06f4e4a025a306e377211863d9ff2a0c 21 21 1\n c65a429f06f4e4a025a306e377211863d9ff2a0c 22 23 1\n\nSo we do know at the moment we find the line that it was at line 23 in\nthe final result, but line 22 in the earlier version at c65a429f06.\n\nAnd indeed, running a non-incremental blame in a debugger, right before\ncalling blame_coalesce() our entries look like this:\n\n cmd_blame (argc=3, argv=0x7fffffffe458, prefix=0x0) at builtin/blame.c:1146\n 1146\t\tblame_coalesce(&sb);\n (gdb) print *sb->ent \n $44 = {next = 0x55555596eda0, lno = 20, num_lines = 1, suspect = 0x555555999a30, s_lno = 20, score = 0, ignored = 0, \n unblamable = 0}\n (gdb) print *sb->ent->next\n $45 = {next = 0x0, lno = 22, num_lines = 1, suspect = 0x555555999a30, s_lno = 21, score = 0, ignored = 0, \n unblamable = 0}\n\nSo we have two one-line entries at lines 21 and 23 (\"lno\"; note that\ninternally we zero-index the lines), and we know that the second one is\nactually from 22 (\"s_lno\").\n\nBut then after blame_coalesce() returns, we have only one entry with\nboth lines:\n\n (gdb) n\n 1148\t\tif (!(output_option & (OUTPUT_COLOR_LINE | OUTPUT_SHOW_AGE_WITH_COLOR)))\n (gdb) print *sb->ent\n $46 = {next = 0x0, lno = 20, num_lines = 2, suspect = 0x555555999a30, s_lno = 20, score = 0, ignored = 0, \n unblamable = 0}\n\nPresumably it saw the adjacent lines in the _source_ file and coalesced\nthem, but that's not the right thing to do. They're distinct hunks in\nthe output we're going to show, so they have to remain such.\n\n-Peff\n"},{"id":"403159","messageId":"20200807213349.GB1871940@coredump.intra.peff.net","threadId":"54006","inReplyTo":"20200807212159.GA1871940@coredump.intra.peff.net","subject":"Re: `git blame` Line Number Off-by-one","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-07T21:33:49Z","receivedAt":"2020-08-07T21:33:52Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 07, 2020 at 05:21:59PM -0400, Jeff King wrote:\n\n> So we have two one-line entries at lines 21 and 23 (\"lno\"; note that\n> internally we zero-index the lines), and we know that the second one is\n> actually from 22 (\"s_lno\").\n> \n> But then after blame_coalesce() returns, we have only one entry with\n> both lines:\n> \n> (gdb) n\n> 1148\t\tif (!(output_option & (OUTPUT_COLOR_LINE | OUTPUT_SHOW_AGE_WITH_COLOR)))\n> (gdb) print *sb->ent\n> $46 = {next = 0x0, lno = 20, num_lines = 2, suspect = 0x555555999a30, s_lno = 20, score = 0, ignored = 0, \n> unblamable = 0}\n> \n> Presumably it saw the adjacent lines in the _source_ file and coalesced\n> them, but that's not the right thing to do. They're distinct hunks in\n> the output we're going to show, so they have to remain such.\n\nI took a look at blame_coalesce(), wondering if there might be a bug in\nit (e.g., using source lines instead of result lines). But it seems to\nbe intentionally joining lines based on the original source count. And\nthinking on it, we wouldn't need to join groups based no result lines,\nsince those start as groups in the first place.\n\nSo I'm having trouble seeing how this coalescing could ever be helpful.\nIt dates back to the original cee7f245dc (git-pickaxe: blame rewritten.,\n2006-10-19). Is it possible that this is just an ill-conceived idea that\nnobody ever noticed before (because most of the time it simply does\nnothing)?\n\nDropping it entirely, as below, doesn't break any tests. Junio, do you\nknow of a case this is meant to improve?\n\n-Peff\n\ndiff --git a/blame.c b/blame.c\nindex 82fa16d658..e4fd9a80ef 100644\n--- a/blame.c\n+++ b/blame.c\n@@ -1172,33 +1172,6 @@ static void sanity_check_refcnt(struct blame_scoreboard *sb)\n \t\tsb->on_sanity_fail(sb, baa);\n }\n \n-/*\n- * If two blame entries that are next to each other came from\n- * contiguous lines in the same origin (i.e. pair),\n- * merge them together.\n- */\n-void blame_coalesce(struct blame_scoreboard *sb)\n-{\n-\tstruct blame_entry *ent, *next;\n-\n-\tfor (ent = sb->ent; ent && (next = ent->next); ent = next) {\n-\t\tif (ent->suspect == next->suspect &&\n-\t\t ent->s_lno + ent->num_lines == next->s_lno &&\n-\t\t ent->ignored == next->ignored &&\n-\t\t ent->unblamable == next->unblamable) {\n-\t\t\tent->num_lines += next->num_lines;\n-\t\t\tent->next = next->next;\n-\t\t\tblame_origin_decref(next->suspect);\n-\t\t\tfree(next);\n-\t\t\tent->score = 0;\n-\t\t\tnext = ent; /* again */\n-\t\t}\n-\t}\n-\n-\tif (sb->debug) /* sanity */\n-\t\tsanity_check_refcnt(sb);\n-}\n-\n /*\n * Merge the given sorted list of blames into a preexisting origin.\n * If there were no previous blames to that commit, it is entered into\ndiff --git a/blame.h b/blame.h\nindex b6bbee4147..1807253ef8 100644\n--- a/blame.h\n+++ b/blame.h\n@@ -173,7 +173,6 @@ static inline struct blame_origin *blame_origin_incref(struct blame_origin *o)\n }\n void blame_origin_decref(struct blame_origin *o);\n \n-void blame_coalesce(struct blame_scoreboard *sb);\n void blame_sort_final(struct blame_scoreboard *sb);\n unsigned blame_entry_score(struct blame_scoreboard *sb, struct blame_entry *e);\n void assign_blame(struct blame_scoreboard *sb, int opt);\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 94ef57c1cc..ce564a5916 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -1143,8 +1143,6 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \n \tblame_sort_final(&sb);\n \n-\tblame_coalesce(&sb);\n-\n \tif (!(output_option & (OUTPUT_COLOR_LINE | OUTPUT_SHOW_AGE_WITH_COLOR)))\n \t\toutput_option |= coloring_mode;\n \n"},{"id":"403160","messageId":"20200807214533.GC1871940@coredump.intra.peff.net","threadId":"54006","inReplyTo":"20200807213349.GB1871940@coredump.intra.peff.net","subject":"Re: `git blame` Line Number Off-by-one","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-07T21:45:33Z","receivedAt":"2020-08-07T21:45:36Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 07, 2020 at 05:33:49PM -0400, Jeff King wrote:\n\n> Dropping it entirely, as below, doesn't break any tests. Junio, do you\n> know of a case this is meant to improve?\n> [...]\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index 94ef57c1cc..ce564a5916 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -1143,8 +1143,6 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n> \n> \tblame_sort_final(&sb);\n> \n> -\tblame_coalesce(&sb);\n> -\n> \tif (!(output_option & (OUTPUT_COLOR_LINE | OUTPUT_SHOW_AGE_WITH_COLOR)))\n> \t\toutput_option |= coloring_mode;\n\nI wondered also whether this sort was necessary (obviously it is for\ncoalescing, but is it otherwise?). But there are definitely tests that\nbreak if it's removed (e.g. t8011-blame-split-file). And it's sorting on\n\"lno\", which implies that yes, in some cases we may end up with a\ncontiguous block of final-result lines that was broken up and shuffled\naround.\n\nI'm not sure if that implies a case where source-coalescing could help.\nWould you ever have such a case where they end up blaming to the same\ncommit?\n\nI'm not sure. If so, then it would probably make sense for\nblame_coalesce() to make sure that the ent->lno and next->lno blocks are\ncontiguous (as well as the s_lno ones).\n\nBut even if that's true, I'm not sure that this coalescing is really all\nthat beneficial. It doesn't impact the normal blame output at all (which\nis line oriented anyway, and repeats information). It doesn't make\n--porcelain more efficient, because we omit repeated commit details even\nfor non-contiguous blocks. It would let a consumer of --porcelain see\nbigger blocks which could be useful for coloring, etc.\n\nSo I dunno. I'd still be curious to see a concrete case where this code\ntriggers and does something useful.\n\n-Peff\n"},{"id":"403169","messageId":"xmqq7duaf6xk.fsf@gitster.c.googlers.com","threadId":"54006","inReplyTo":"20200807213349.GB1871940@coredump.intra.peff.net","subject":"Re: `git blame` Line Number Off-by-one","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-07T22:05:27Z","receivedAt":"2020-08-07T22:05:33Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Dropping it entirely, as below, doesn't break any tests. Junio, do you\n> know of a case this is meant to improve?\n\nI think the only conceivable case is that in the middle of a single\nblock of text in an ancient version, another block of lines gets\ninserted during the evolution of the file, but in the end these\nintermediate edits all go away and the same original text remains.\n\nIn such a case, without coalescing, we would not treat the original\nsingle block of text as a single unit.\n\nIIRC, blame has some threshold that makes too small a block not\nsubject to move and copy detection, and it is most likely to avoid\nfragmenting the blocks too small.\n\n"},{"id":"403174","messageId":"20200807222630.GA3750245@coredump.intra.peff.net","threadId":"54006","inReplyTo":"xmqq7duaf6xk.fsf@gitster.c.googlers.com","subject":"Re: `git blame` Line Number Off-by-one","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-07T22:26:30Z","receivedAt":"2020-08-07T22:26:33Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 07, 2020 at 03:05:27PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Dropping it entirely, as below, doesn't break any tests. Junio, do you\n> > know of a case this is meant to improve?\n> \n> I think the only conceivable case is that in the middle of a single\n> block of text in an ancient version, another block of lines gets\n> inserted during the evolution of the file, but in the end these\n> intermediate edits all go away and the same original text remains.\n> \n> In such a case, without coalescing, we would not treat the original\n> single block of text as a single unit.\n\nYeah, that makes sense, and it should be possible to construct a case\nbased on that.\n\n> IIRC, blame has some threshold that makes too small a block not\n> subject to move and copy detection, and it is most likely to avoid\n> fragmenting the blocks too small.\n\nI don't know if that would matter here, though. This coalescing is\ntaking place after all of the blaming has happened, and is just\npreparing the final output.\n\n-Peff\n"},{"id":"403176","messageId":"20200807223522.GB3750245@coredump.intra.peff.net","threadId":"54006","inReplyTo":"20200807222630.GA3750245@coredump.intra.peff.net","subject":"Re: `git blame` Line Number Off-by-one","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-07T22:35:22Z","receivedAt":"2020-08-07T22:35:24Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 07, 2020 at 06:26:30PM -0400, Jeff King wrote:\n\n> On Fri, Aug 07, 2020 at 03:05:27PM -0700, Junio C Hamano wrote:\n> \n> > Jeff King writes:\n> > \n> > > Dropping it entirely, as below, doesn't break any tests. Junio, do you\n> > > know of a case this is meant to improve?\n> > \n> > I think the only conceivable case is that in the middle of a single\n> > block of text in an ancient version, another block of lines gets\n> > inserted during the evolution of the file, but in the end these\n> > intermediate edits all go away and the same original text remains.\n> > \n> > In such a case, without coalescing, we would not treat the original\n> > single block of text as a single unit.\n> \n> Yeah, that makes sense, and it should be possible to construct a case\n> based on that.\n\nIt was even easier than I expected. :)\n\nTry this:\n\n git init repo\n cd repo\n \n seq 20 >file\n git add file\n git commit -m base\n \n { seq 10; echo foo; seq 11 20; } >file\n git commit -am cruft\n \n seq 20 >file\n git commit -am revert\n\n git blame --root --porcelain file\n\nWith the coalescing, we get a single 20-line block:\n\n 25d93ad4c4b27570ccd4b6412ccaa549ae6f9ff7 1 1 20\n author Jeff King\n ...\n 1\n ...\n 25d93ad4c4b27570ccd4b6412ccaa549ae6f9ff7 11 11\n 11\n\nand without it, we get two 10-line blocks:\n\n 25d93ad4c4b27570ccd4b6412ccaa549ae6f9ff7 1 1 10\n author Jeff King\n ...\n 1\n ...\n 25d93ad4c4b27570ccd4b6412ccaa549ae6f9ff7 11 11 10\n 11\n\nI doubt it really matters all that much in practice, but it's possible\nsome consumer of --porcelain relies on us giving minimal blocks.\n\nSo that argues for fixing blame_coalesce() to make sure that we coalesce\nonly when the blocks are adjacent in both the source and the result.\nOtherwise we're losing information.\n\n-Peff\n"},{"id":"403526","messageId":"20200813052054.GA1962792@coredump.intra.peff.net","threadId":"54006","inReplyTo":"20200807223522.GB3750245@coredump.intra.peff.net","subject":"[PATCH 0/3] blame: fix bug in coalescing non-adjacent \"-L\" chunks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-13T05:20:54Z","receivedAt":"2020-08-13T05:20:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 07, 2020 at 06:35:22PM -0400, Jeff King wrote:\n\n> > > I think the only conceivable case is that in the middle of a single\n> > > block of text in an ancient version, another block of lines gets\n> > > inserted during the evolution of the file, but in the end these\n> > > intermediate edits all go away and the same original text remains.\n> > > \n> > > In such a case, without coalescing, we would not treat the original\n> > > single block of text as a single unit.\n> > \n> > Yeah, that makes sense, and it should be possible to construct a case\n> > based on that.\n\nI started to add a test for this, and it turns out we already had one!\nIt just wasn't checking the output as carefully as it could. :)\n\nSo here's a series which actually checks that blame_coalesce() is doing\nsomething useful, and then fixes Nuthan's bug on top (with a test case,\nbut I also confirmed it makes the original tomcat issue go away).\n\n [1/3]: t8003: check output of coalesced blame\n [2/3]: t8003: factor setup out of coalesce test\n [3/3]: blame: only coalesce lines that are adjacent in result\n\n blame.c | 1 +\n t/t8003-blame-corner-cases.sh | 28 +++++++++++++++++++---------\n 2 files changed, 20 insertions(+), 9 deletions(-)\n\n-Peff\n"},{"id":"403527","messageId":"20200813052305.GA2514880@coredump.intra.peff.net","threadId":"54006","inReplyTo":"20200813052054.GA1962792@coredump.intra.peff.net","subject":"[PATCH 1/3] t8003: check output of coalesced blame","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-13T05:23:05Z","receivedAt":"2020-08-13T05:23:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit f0cbe742f4 (blame: add a test to cover blame_coalesce(),\n2019-06-20) added a test case where blame can usefully coalesce two\ngroups of lines. But since it relies on the normal blame output, it only\nexercises the code and can't tell whether the lines were actually\njoined into a single group.\n\nHowever, by using --porcelain output, we can see how git-blame considers\nthe groupings (and likewise how the coalescing might have a real\nuser-visible impact for a tool that uses the porcelain-output\ngroupings). This lets us confirm that we are indeed coalescing correctly\n(and the fact that this test case requires coalescing can be verified by\ndropping the call to blame_coalesce(), causing the test to fail).\n\nSigned-off-by: Jeff King \n---\n t/t8003-blame-corner-cases.sh | 11 ++++-------\n 1 file changed, 4 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh\nindex b871dd4f86..1e89494ef6 100755\n--- a/t/t8003-blame-corner-cases.sh\n+++ b/t/t8003-blame-corner-cases.sh\n@@ -273,10 +273,6 @@ test_expect_success 'blame file with CRLF core.autocrlf=true' '\n \tgrep \"A U Thor\" actual\n '\n \n-# Tests the splitting and merging of blame entries in blame_coalesce().\n-# The output of blame is the same, regardless of whether blame_coalesce() runs\n-# or not, so we'd likely only notice a problem if blame crashes or assigned\n-# blame to the \"splitting\" commit ('SPLIT' below).\n test_expect_success 'blame coalesce' '\n \tcat >giraffe <<-\\EOF &&\n \tABC\n@@ -302,10 +298,11 @@ test_expect_success 'blame coalesce' '\n \tgit commit -m \"same contents as original\" &&\n \n \tcat >expect <<-EOF &&\n-\t$oid 1) ABC\n-\t$oid 2) DEF\n+\t$oid 1 1 2\n+\t$oid 2 2\n \tEOF\n-\tgit -c core.abbrev=$(test_oid hexsz) blame -s giraffe >actual &&\n+\tgit blame --porcelain giraffe >actual.raw &&\n+\tgrep \"^$oid\" actual.raw >actual &&\n \ttest_cmp expect actual\n '\n \n-- \n2.28.0.570.ge2c3ee08e4\n\n"},{"id":"403528","messageId":"20200813052341.GB2514880@coredump.intra.peff.net","threadId":"54006","inReplyTo":"20200813052054.GA1962792@coredump.intra.peff.net","subject":"[PATCH 2/3] t8003: factor setup out of coalesce test","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-13T05:23:41Z","receivedAt":"2020-08-13T05:23:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In preparation for adding more tests of blame's coalesce code, let's\nsplit the setup out from the first test, and give each of the commits\na more meaningful name:\n\n - $orig for the original source that added the lines\n\n - $split for the version where they are split apart\n\n - $final for the final version that re-joins them\n\nThat's not strictly necessary, but makes the follow-on tests less\nbrittle than relying on HEAD^, etc, to name the commits.\n\nSigned-off-by: Jeff King \n---\n t/t8003-blame-corner-cases.sh | 16 ++++++++++------\n 1 file changed, 10 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh\nindex 1e89494ef6..7f39a84289 100755\n--- a/t/t8003-blame-corner-cases.sh\n+++ b/t/t8003-blame-corner-cases.sh\n@@ -273,14 +273,14 @@ test_expect_success 'blame file with CRLF core.autocrlf=true' '\n \tgrep \"A U Thor\" actual\n '\n \n-test_expect_success 'blame coalesce' '\n+test_expect_success 'setup coalesce tests' '\n \tcat >giraffe <<-\\EOF &&\n \tABC\n \tDEF\n \tEOF\n \tgit add giraffe &&\n \tgit commit -m \"original file\" &&\n-\toid=$(git rev-parse HEAD) &&\n+\torig=$(git rev-parse HEAD) &&\n \n \tcat >giraffe <<-\\EOF &&\n \tABC\n@@ -289,20 +289,24 @@ test_expect_success 'blame coalesce' '\n \tEOF\n \tgit add giraffe &&\n \tgit commit -m \"interior SPLIT line\" &&\n+\tsplit=$(git rev-parse HEAD) &&\n \n \tcat >giraffe <<-\\EOF &&\n \tABC\n \tDEF\n \tEOF\n \tgit add giraffe &&\n \tgit commit -m \"same contents as original\" &&\n+\tfinal=$(git rev-parse HEAD)\n+'\n \n+test_expect_success 'blame coalesce' '\n \tcat >expect <<-EOF &&\n-\t$oid 1 1 2\n-\t$oid 2 2\n+\t$orig 1 1 2\n+\t$orig 2 2\n \tEOF\n-\tgit blame --porcelain giraffe >actual.raw &&\n-\tgrep \"^$oid\" actual.raw >actual &&\n+\tgit blame --porcelain $final giraffe >actual.raw &&\n+\tgrep \"^$orig\" actual.raw >actual &&\n \ttest_cmp expect actual\n '\n \n-- \n2.28.0.570.ge2c3ee08e4\n\n"},{"id":"403529","messageId":"20200813052531.GC2514880@coredump.intra.peff.net","threadId":"54006","inReplyTo":"20200813052054.GA1962792@coredump.intra.peff.net","subject":"[PATCH 3/3] blame: only coalesce lines that are adjacent in result","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-13T05:25:31Z","receivedAt":"2020-08-13T05:25:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"After blame has finished but before we produce any output, we coalesce\ngroups of lines that were adjacent in the original suspect (which may\nhave been split apart by lines in intermediate commits which went away).\nHowever, this can cause incorrect output if the lines are not also\nadjacent in the result. For instance, the case in t8003 has:\n\n ABC\n DEF\n\nwhich becomes\n\n ABC\n SPLIT\n DEF\n\nBlaming only lines 1 and 3 in the result yields two blame groups (one\nfor each line) that were adjacent in the original. That's enough for us\nto coalesce them into a single group, but that loses information: our\noutput routines assume they're adjacent in the result as well, and we\noutput:\n\n 1) ABC\n 2) SPLIT\n\nThis is nonsense for two reasons:\n\n - we were asked about line 3, not line 2; we should not output the\n SPLIT line at all\n\n - commit did not touch the SPLIT line at all! We found the\n correct blame for line 3, but the bug is actually in the output\n stage, which is showing the wrong line number and content from the\n final file.\n\nWe can fix this by only coalescing when both the suspect and result\nlines are adjacent. That fixes this bug, but keeps coalescing in cases\nwhere want it (e.g., the existing test in t8003 where SPLIT goes away,\nand the lines really are adjacent in the result).\n\nReported-by: Nuthan Munaiah \nSigned-off-by: Jeff King \n---\n blame.c | 1 +\n t/t8003-blame-corner-cases.sh | 9 +++++++++\n 2 files changed, 10 insertions(+)\n\ndiff --git a/blame.c b/blame.c\nindex 82fa16d658..1be1cd82a2 100644\n--- a/blame.c\n+++ b/blame.c\n@@ -1184,6 +1184,7 @@ void blame_coalesce(struct blame_scoreboard *sb)\n \tfor (ent = sb->ent; ent && (next = ent->next); ent = next) {\n \t\tif (ent->suspect == next->suspect &&\n \t\t ent->s_lno + ent->num_lines == next->s_lno &&\n+\t\t ent->lno + ent->num_lines == next->lno &&\n \t\t ent->ignored == next->ignored &&\n \t\t ent->unblamable == next->unblamable) {\n \t\t\tent->num_lines += next->num_lines;\ndiff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh\nindex 7f39a84289..ba8013b002 100755\n--- a/t/t8003-blame-corner-cases.sh\n+++ b/t/t8003-blame-corner-cases.sh\n@@ -310,4 +310,13 @@ test_expect_success 'blame coalesce' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'blame does not coalesce non-adjacent result lines' '\n+\tcat >expect <<-EOF &&\n+\t$orig 1) ABC\n+\t$orig 3) DEF\n+\tEOF\n+\tgit blame --no-abbrev -s -L1,1 -L3,3 $split giraffe >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.28.0.570.ge2c3ee08e4\n"},{"id":"403540","messageId":"e63239f2-f121-97c3-7229-2c48d6aefcd0@google.com","threadId":"54006","inReplyTo":"20200813052054.GA1962792@coredump.intra.peff.net","subject":"Re: [PATCH 0/3] blame: fix bug in coalescing non-adjacent \"-L\" chunks","fromName":"Barret Rhoden","fromEmail":"brho@google.com","sentAt":"2020-08-13T12:25:59Z","receivedAt":"2020-08-13T12:26:04Z","isPatch":true,"sender":{"key":"brho@google.com","avatar":null},"body":"On 8/13/20 1:20 AM, Jeff King wrote:\n> On Fri, Aug 07, 2020 at 06:35:22PM -0400, Jeff King wrote:\n> \n>>>> I think the only conceivable case is that in the middle of a single\n>>>> block of text in an ancient version, another block of lines gets\n>>>> inserted during the evolution of the file, but in the end these\n>>>> intermediate edits all go away and the same original text remains.\n>>>>\n>>>> In such a case, without coalescing, we would not treat the original\n>>>> single block of text as a single unit.\n>>>\n>>> Yeah, that makes sense, and it should be possible to construct a case\n>>> based on that.\n> \n> I started to add a test for this, and it turns out we already had one!\n> It just wasn't checking the output as carefully as it could. :)\n> \n> So here's a series which actually checks that blame_coalesce() is doing\n> something useful, and then fixes Nuthan's bug on top (with a test case,\n> but I also confirmed it makes the original tomcat issue go away).\n> \n> [1/3]: t8003: check output of coalesced blame\n> [2/3]: t8003: factor setup out of coalesce test\n> [3/3]: blame: only coalesce lines that are adjacent in result\n> \n\nthanks for the fixes, looks good to me.\n\n"},{"id":"403568","messageId":"xmqq8sei4h40.fsf@gitster.c.googlers.com","threadId":"54006","inReplyTo":"20200813052531.GC2514880@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] blame: only coalesce lines that are adjacent in result","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-13T16:59:11Z","receivedAt":"2020-08-13T16:59:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> diff --git a/blame.c b/blame.c\n> index 82fa16d658..1be1cd82a2 100644\n> --- a/blame.c\n> +++ b/blame.c\n> @@ -1184,6 +1184,7 @@ void blame_coalesce(struct blame_scoreboard *sb)\n> \tfor (ent = sb->ent; ent && (next = ent->next); ent = next) {\n> \t\tif (ent->suspect == next->suspect &&\n> \t\t ent->s_lno + ent->num_lines == next->s_lno &&\n> +\t\t ent->lno + ent->num_lines == next->lno &&\n> \t\t ent->ignored == next->ignored &&\n> \t\t ent->unblamable == next->unblamable) {\n> \t\t\tent->num_lines += next->num_lines;\n\nWhen laid out like this, the correctness of the fix is quite obvious\nand it is surprising that this bug has survived for all these years,\nas it dates back to cee7f245 (git-pickaxe: blame rewritten.,\n2006-10-19), the inception of the current \"git blame\".\n\nThanks.\n\n> diff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh\n> index 7f39a84289..ba8013b002 100755\n> --- a/t/t8003-blame-corner-cases.sh\n> +++ b/t/t8003-blame-corner-cases.sh\n> @@ -310,4 +310,13 @@ test_expect_success 'blame coalesce' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'blame does not coalesce non-adjacent result lines' '\n> +\tcat >expect <<-EOF &&\n> +\t$orig 1) ABC\n> +\t$orig 3) DEF\n> +\tEOF\n> +\tgit blame --no-abbrev -s -L1,1 -L3,3 $split giraffe >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_done\n"},{"id":"403570","messageId":"xmqq4kp64guk.fsf@gitster.c.googlers.com","threadId":"54006","inReplyTo":"20200813052305.GA2514880@coredump.intra.peff.net","subject":"Re: [PATCH 1/3] t8003: check output of coalesced blame","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-13T17:04:51Z","receivedAt":"2020-08-13T17:05:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Commit f0cbe742f4 (blame: add a test to cover blame_coalesce(),\n> 2019-06-20) added a test case where blame can usefully coalesce two\n> groups of lines. But since it relies on the normal blame output, it only\n> exercises the code and can't tell whether the lines were actually\n> joined into a single group.\n>\n> However, by using --porcelain output, we can see how git-blame considers\n> the groupings (and likewise how the coalescing might have a real\n> user-visible impact for a tool that uses the porcelain-output\n> groupings). This lets us confirm that we are indeed coalescing correctly\n> (and the fact that this test case requires coalescing can be verified by\n> dropping the call to blame_coalesce(), causing the test to fail).\n>\n> Signed-off-by: Jeff King \n> ---\n> t/t8003-blame-corner-cases.sh | 11 ++++-------\n> 1 file changed, 4 insertions(+), 7 deletions(-)\n>\n> diff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh\n> index b871dd4f86..1e89494ef6 100755\n> --- a/t/t8003-blame-corner-cases.sh\n> +++ b/t/t8003-blame-corner-cases.sh\n> @@ -273,10 +273,6 @@ test_expect_success 'blame file with CRLF core.autocrlf=true' '\n> \tgrep \"A U Thor\" actual\n> '\n> \n> -# Tests the splitting and merging of blame entries in blame_coalesce().\n> -# The output of blame is the same, regardless of whether blame_coalesce() runs\n> -# or not, so we'd likely only notice a problem if blame crashes or assigned\n> -# blame to the \"splitting\" commit ('SPLIT' below).\n> test_expect_success 'blame coalesce' '\n> \tcat >giraffe <<-\\EOF &&\n> \tABC\n> @@ -302,10 +298,11 @@ test_expect_success 'blame coalesce' '\n> \tgit commit -m \"same contents as original\" &&\n> \n> \tcat >expect <<-EOF &&\n> -\t$oid 1) ABC\n> -\t$oid 2) DEF\n> +\t$oid 1 1 2\n> +\t$oid 2 2\n> \tEOF\n\nIt has become a bit harder to grok, but for the purpose of the later\nsteps to see where things exactly came from (including their line\nnumbers), it is easier to see what is going on with the new format.\n\n> -\tgit -c core.abbrev=$(test_oid hexsz) blame -s giraffe >actual &&\n> +\tgit blame --porcelain giraffe >actual.raw &&\n\nThe original forced the abbrev length; by switching to the format\nfor porcelain-writers, we know we will get the full object name. OK.\n\n> +\tgrep \"^$oid\" actual.raw >actual &&\n> \ttest_cmp expect actual\n> '\n"},{"id":"403573","messageId":"20200813172206.GA1597339@coredump.intra.peff.net","threadId":"54006","inReplyTo":"xmqq4kp64guk.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 1/3] t8003: check output of coalesced blame","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-13T17:22:06Z","receivedAt":"2020-08-13T17:22:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 13, 2020 at 10:04:51AM -0700, Junio C Hamano wrote:\n\n> > \tcat >expect <<-EOF &&\n> > -\t$oid 1) ABC\n> > -\t$oid 2) DEF\n> > +\t$oid 1 1 2\n> > +\t$oid 2 2\n> > \tEOF\n> \n> It has become a bit harder to grok, but for the purpose of the later\n> steps to see where things exactly came from (including their line\n> numbers), it is easier to see what is going on with the new format.\n\nYeah, I agree the numbers are a bit more inscrutable. We could do\nsomething like:\n\ndiff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh\nindex ba8013b002..9486888e5a 100755\n--- a/t/t8003-blame-corner-cases.sh\n+++ b/t/t8003-blame-corner-cases.sh\n@@ -301,12 +301,14 @@ test_expect_success 'setup coalesce tests' '\n '\n \n test_expect_success 'blame coalesce' '\n-\tcat >expect <<-EOF &&\n+\tq_to_tab >expect <<-EOF &&\n \t$orig 1 1 2\n+\tQABC\n \t$orig 2 2\n+\tQDEF\n \tEOF\n \tgit blame --porcelain $final giraffe >actual.raw &&\n-\tgrep \"^$orig\" actual.raw >actual &&\n+\tegrep \"^($orig|\t)\" actual.raw >actual &&\n \ttest_cmp expect actual\n '\n\nbut IMHO that isn't much more readable (largely due to the\ntab-handling in the here-doc).\n\n-Peff\n"},{"id":"403588","messageId":"xmqqv9hm1j7u.fsf@gitster.c.googlers.com","threadId":"54006","inReplyTo":"20200813052531.GC2514880@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] blame: only coalesce lines that are adjacent in result","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-13T18:41:57Z","receivedAt":"2020-08-13T18:42:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> +test_expect_success 'blame does not coalesce non-adjacent result lines' '\n> +\tcat >expect <<-EOF &&\n> +\t$orig 1) ABC\n> +\t$orig 3) DEF\n> +\tEOF\n> +\tgit blame --no-abbrev -s -L1,1 -L3,3 $split giraffe >actual &&\n\nI like this much better than \"git -c core.abbrev=40\", too. Good.\n\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_done\n"}]}