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

From: Jakub Narebski <jnareb@gmail.com>
Date: Jun 4, 2008, 14:03 UTC
Message-ID: <200806041603.49555.jnareb@gmail.com>
In-Reply-To: <7v3ant213k.fsf@gitster.siamese.dyndns.org>

On Wed, 4 Jun 2008, Junio C Hamano wrote:

Show 15 quoted lines

> Jakub Narebski <jnareb@gmail.com> writes:
>> On Tue, 3 Jan 2008, Luben Tuikov wrote:
>>>
>>> The intention was that it shouldn't necessarily be the (strict) parent
>>> of the change (changed segment), since it may or may not have changed
>>> in the strict parent commit.  The intention was that it
>>> "starts"/"opens" the parent commit so that "git" would start from
>>> there and find the actual change/commit where that line/segment has
>>> changed.  And it has worked pretty fine for me when data-mining
>>> (something I do quite often) code evolution.
> 
> 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. Can it be done cheaply? I don't know, generating extended info as described below should be cheap if we have equivalent of textual (patch) diff between commit blamed for given line, and its parent; actually what we need is more of 'context' diff than of default 'unified' diff.

Show 6 quoted lines

> 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?

But even without it (i.e. ony 'parents' header showing true, not rewritten parents) what we need is some info about pre-image for blamed line. We would need line number of the line in pre-image (or NUL if the page was added in blamed commit), and pre-image filename.

I don't know if it could be done cheaply, and if it could be done simply; currently git-diff doesn't have "context diff" format output, and what I though about by pre-image line number requires finding if a line was added in a commit, or was modified in a commit. (If it was removed, it wouldn't be in final image and hence wouldn't be blamed; if it was moved, it wouldn't be blamed, as blame follows code movement).

> 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.

>From what you write it wouldn't be easy to add "history of a given

line begins here", or even "history of a given file begins there"...

-- 
Jakub Narebski
Poland