| Message ID | 20200813052531.GC2514880@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | blame: fix bug in coalescing non-adjacent "-L" chunks | expand |
Jeff King <peff@peff.net> writes: > diff --git a/blame.c b/blame.c > index 82fa16d658..1be1cd82a2 100644 > --- a/blame.c > +++ b/blame.c > @@ -1184,6 +1184,7 @@ void blame_coalesce(struct blame_scoreboard *sb) > for (ent = sb->ent; ent && (next = ent->next); ent = next) { > if (ent->suspect == next->suspect && > ent->s_lno + ent->num_lines == next->s_lno && > + ent->lno + ent->num_lines == next->lno && > ent->ignored == next->ignored && > ent->unblamable == next->unblamable) { > ent->num_lines += next->num_lines; When laid out like this, the correctness of the fix is quite obvious and it is surprising that this bug has survived for all these years, as it dates back to cee7f245 (git-pickaxe: blame rewritten., 2006-10-19), the inception of the current "git blame". Thanks. > diff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh > index 7f39a84289..ba8013b002 100755 > --- a/t/t8003-blame-corner-cases.sh > +++ b/t/t8003-blame-corner-cases.sh > @@ -310,4 +310,13 @@ test_expect_success 'blame coalesce' ' > test_cmp expect actual > ' > > +test_expect_success 'blame does not coalesce non-adjacent result lines' ' > + cat >expect <<-EOF && > + $orig 1) ABC > + $orig 3) DEF > + EOF > + git blame --no-abbrev -s -L1,1 -L3,3 $split giraffe >actual && > + test_cmp expect actual > +' > + > test_done
Jeff King <peff@peff.net> writes: > +test_expect_success 'blame does not coalesce non-adjacent result lines' ' > + cat >expect <<-EOF && > + $orig 1) ABC > + $orig 3) DEF > + EOF > + git blame --no-abbrev -s -L1,1 -L3,3 $split giraffe >actual && I like this much better than "git -c core.abbrev=40", too. Good. > + test_cmp expect actual > +' > + > test_done
diff --git a/blame.c b/blame.c index 82fa16d658..1be1cd82a2 100644 --- a/blame.c +++ b/blame.c @@ -1184,6 +1184,7 @@ void blame_coalesce(struct blame_scoreboard *sb) for (ent = sb->ent; ent && (next = ent->next); ent = next) { if (ent->suspect == next->suspect && ent->s_lno + ent->num_lines == next->s_lno && + ent->lno + ent->num_lines == next->lno && ent->ignored == next->ignored && ent->unblamable == next->unblamable) { ent->num_lines += next->num_lines; diff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh index 7f39a84289..ba8013b002 100755 --- a/t/t8003-blame-corner-cases.sh +++ b/t/t8003-blame-corner-cases.sh @@ -310,4 +310,13 @@ test_expect_success 'blame coalesce' ' test_cmp expect actual ' +test_expect_success 'blame does not coalesce non-adjacent result lines' ' + cat >expect <<-EOF && + $orig 1) ABC + $orig 3) DEF + EOF + git blame --no-abbrev -s -L1,1 -L3,3 $split giraffe >actual && + test_cmp expect actual +' + test_done
After blame has finished but before we produce any output, we coalesce groups of lines that were adjacent in the original suspect (which may have been split apart by lines in intermediate commits which went away). However, this can cause incorrect output if the lines are not also adjacent in the result. For instance, the case in t8003 has: ABC DEF which becomes ABC SPLIT DEF Blaming only lines 1 and 3 in the result yields two blame groups (one for each line) that were adjacent in the original. That's enough for us to coalesce them into a single group, but that loses information: our output routines assume they're adjacent in the result as well, and we output: <oid> 1) ABC <oid> 2) SPLIT This is nonsense for two reasons: - we were asked about line 3, not line 2; we should not output the SPLIT line at all - commit <oid> did not touch the SPLIT line at all! We found the correct blame for line 3, but the bug is actually in the output stage, which is showing the wrong line number and content from the final file. We can fix this by only coalescing when both the suspect and result lines are adjacent. That fixes this bug, but keeps coalescing in cases where want it (e.g., the existing test in t8003 where SPLIT goes away, and the lines really are adjacent in the result). Reported-by: Nuthan Munaiah <nm6061@rit.edu> Signed-off-by: Jeff King <peff@peff.net> --- blame.c | 1 + t/t8003-blame-corner-cases.sh | 9 +++++++++ 2 files changed, 10 insertions(+)