Re: `git blame` Line Number Off-by-one

From: Jeff King <peff@peff.net>
Date: Aug 7, 2020, 21:33 UTC
Message-ID: <20200807213349.GB1871940@coredump.intra.peff.net>
In-Reply-To: <20200807212159.GA1871940@coredump.intra.peff.net>

On Fri, Aug 07, 2020 at 05:21:59PM -0400, Jeff King wrote:

Show 16 quoted lines

> So we have two one-line entries at lines 21 and 23 ("lno"; note that
> internally we zero-index the lines), and we know that the second one is
> actually from 22 ("s_lno").
> 
> But then after blame_coalesce() returns, we have only one entry with
> both lines:
> 
>   (gdb) n
>   1148		if (!(output_option & (OUTPUT_COLOR_LINE | OUTPUT_SHOW_AGE_WITH_COLOR)))
>   (gdb) print *sb->ent
>   $46 = {next = 0x0, lno = 20, num_lines = 2, suspect = 0x555555999a30, s_lno = 20, score = 0, ignored = 0, 
>     unblamable = 0}
> 
> Presumably it saw the adjacent lines in the _source_ file and coalesced
> them, but that's not the right thing to do. They're distinct hunks in
> the output we're going to show, so they have to remain such.

I took a look at blame_coalesce(), wondering if there might be a bug in it (e.g., using source lines instead of result lines). But it seems to be intentionally joining lines based on the original source count. And thinking on it, we wouldn't need to join groups based no result lines, since those start as groups in the first place.

So I'm having trouble seeing how this coalescing could ever be helpful. It dates back to the original cee7f245dc (git-pickaxe: blame rewritten., 2006-10-19). Is it possible that this is just an ill-conceived idea that nobody ever noticed before (because most of the time it simply does nothing)?

Dropping it entirely, as below, doesn't break any tests. Junio, do you know of a case this is meant to improve?

-Peff

diff --git a/blame.c b/blame.c
index 82fa16d658..e4fd9a80ef 100644
--- a/blame.c
+++ b/blame.c
@@ -1172,33 +1172,6 @@ static void sanity_check_refcnt(struct blame_scoreboard *sb)
 		sb->on_sanity_fail(sb, baa);
 }
 
-/*
- * If two blame entries that are next to each other came from
- * contiguous lines in the same origin (i.e. <commit, path> pair),
- * merge them together.
- */
-void blame_coalesce(struct blame_scoreboard *sb)
-{
-	struct blame_entry *ent, *next;
-
-	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->ignored == next->ignored &&
-		    ent->unblamable == next->unblamable) {
-			ent->num_lines += next->num_lines;
-			ent->next = next->next;
-			blame_origin_decref(next->suspect);
-			free(next);
-			ent->score = 0;
-			next = ent; /* again */
-		}
-	}
-
-	if (sb->debug) /* sanity */
-		sanity_check_refcnt(sb);
-}
-
 /*
  * Merge the given sorted list of blames into a preexisting origin.
  * If there were no previous blames to that commit, it is entered into
diff --git a/blame.h b/blame.h
index b6bbee4147..1807253ef8 100644
--- a/blame.h
+++ b/blame.h
@@ -173,7 +173,6 @@ static inline struct blame_origin *blame_origin_incref(struct blame_origin *o)
 }
 void blame_origin_decref(struct blame_origin *o);
 
-void blame_coalesce(struct blame_scoreboard *sb);
 void blame_sort_final(struct blame_scoreboard *sb);
 unsigned blame_entry_score(struct blame_scoreboard *sb, struct blame_entry *e);
 void assign_blame(struct blame_scoreboard *sb, int opt);
diff --git a/builtin/blame.c b/builtin/blame.c
index 94ef57c1cc..ce564a5916 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1143,8 +1143,6 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 
 	blame_sort_final(&sb);
 
-	blame_coalesce(&sb);
-
 	if (!(output_option & (OUTPUT_COLOR_LINE | OUTPUT_SHOW_AGE_WITH_COLOR)))
 		output_option |= coloring_mode;