Re: [PATCH] Avoid errors from git-rev-parse in gitweb blame
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 5, 2008, 06:07 UTC
- Message-ID
- <7vd4mw4dpp.fsf@gitster.siamese.dyndns.org>
- In-Reply-To
- <200806041603.49555.jnareb@gmail.com>
Jakub Narebski <jnareb@gmail.com> writes:
Show 8 quoted lines
> 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.
Show 8 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?
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.
Show 8 quoted lines
>> 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.