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

[Jeff King <peff@peff.net>]

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

> 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