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

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

On Fri, Aug 07, 2020 at 05:33:49PM -0400, Jeff King wrote:

Show 15 quoted lines

> Dropping it entirely, as below, doesn't break any tests. Junio, do you
> know of a case this is meant to improve?
> [...]
> 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;

I wondered also whether this sort was necessary (obviously it is for coalescing, but is it otherwise?). But there are definitely tests that break if it's removed (e.g. t8011-blame-split-file). And it's sorting on "lno", which implies that yes, in some cases we may end up with a contiguous block of final-result lines that was broken up and shuffled around.

I'm not sure if that implies a case where source-coalescing could help. Would you ever have such a case where they end up blaming to the same commit?

I'm not sure. If so, then it would probably make sense for blame_coalesce() to make sure that the ent->lno and next->lno blocks are contiguous (as well as the s_lno ones).

But even if that's true, I'm not sure that this coalescing is really all that beneficial. It doesn't impact the normal blame output at all (which is line oriented anyway, and repeats information). It doesn't make --porcelain more efficient, because we omit repeated commit details even for non-contiguous blocks. It would let a consumer of --porcelain see bigger blocks which could be useful for coloring, etc.

So I dunno. I'd still be curious to see a concrete case where this code triggers and does something useful.

-Peff