Re: [PATCH] Avoid errors from git-rev-parse in gitweb blame

[Junio C Hamano <gitster@pobox.com>]

Jakub Narebski <jnareb@gmail.com> writes:

> On Wed, 4 Jun 2008, Junio C Hamano wrote:
>> Yes, but the current scheme breaks down in another way.  When $full_rev
>> added many lines to the file, and you are adding the link to for a line
>> near the end of the file and such a line may not exist.  This cannot be
>> cheaply done even inside blame itself.
>
> I think the scheme could be fixed by proposed belo git-blame porcelain
> format output extension.

For the line number information, I do not think so.

Luben's "continue from the same line number in the parent commit" is a
cute hack, but that strategy needs a qualifying comment "because hoping
that the same line number in the parent commit might have something
relevant would be better than stopping and giving up sometimes."  It
cannot reliably work (and it is not Luben's fault).

But the #l<lno> fragment is just a hint to scroll to that point after
restarting the blame from previous commit and jumping to the result, so it
may not be too big a deal.  Such a line may not exist in the resulting
blame page, but that's Ok.

>> Another breakage is even though $full_rev^ _may_ exist (iow, $full_rev
>> might not be the root commit), the file being blamed may not exist there
>> (iow $full_rev might have introduced the file).  Instead of running
>> "rev-parse $full_rev^", you would at least need to ask "rev-list -1
>> $full_rev^ -- $path" or something from the Porcelain layer, but
>> unfortunately this is rather expensive.
>
> Doesn't blame know revision graph for history of a given file already?

Not in the sense of "rev-list -2 $full_rev -- $path | sed -e 1d".  It
builds the graph as it digs deeper, and when it stops, it stopped digging,
so all it knows at that point without further computation is $full_rev^@,
and not "the previous commit that touched the path".

But as Luben explained (and you drew a simple strand of pearls history to
illustrate), immediate parent is just for the purpose of restarting the
computation.

>> Because blame already almost knows if the commit the final blame lies on
>> has a parent, it would be reasonably cheap to add that "parent or nothing"
>> information to its --porcelain (and its --incremental) format if we wanted
>> to.
>
> It would be easy to add 'parents' header, perhaps empty if we blame
> root commit, or a boundary commit (do we say 'boundary' then?) when
> doing revision limited blaming.

It shouldn't be too hard to say "parents of the blamed commit that has the
corresponding preimage of the file is this", and the history does not have
to be limited.  You need to also handle "the commit that introduced the
path" case just like "root" and "boundary" that we cannot dig further than
that point.

I'll follow this message up with two weatherballoon patches.