Re: [RFC/PATCH 4/3] gitweb: Incremental blame (proof of concept)

From: Jakub Narebski <jnareb@gmail.com>
Date: Dec 11, 2008, 01:22 UTC
Message-ID: <200812110222.04663.jnareb@gmail.com>
In-Reply-To: <7v3agvy1v3.fsf@gitster.siamese.dyndns.org>

On Thu, 11 Dec 2008, Junio C Hamano wrote:

Show 9 quoted lines

> Jakub Narebski <jnareb@gmail.com> writes:
> 
> > NOTE: This patch is RFC proof of concept patch!: it should be split
> > onto many smaller patches for easy review (and bug finding) in version
> > meant to be applied.
> 
> Hmm, the comments an RFC requests for would certainly be based on reviews
> of the patch in question, so if the patch is known to be unsuitable for
> reviewing, what would that tell us, I wonder ;-)?

Well, you can apply patch and test how it works, for example if JavaScript code works in other browsers that have JavaScript and DOM support (Firefox, IE, Opera, Safari, Google Chrome)... Or what features or what interface one would like to have...

> Among the 700 lines added/deleted, 400 lines are from a single new file,
> so what may benefit from splitting would be the changes to gitweb.perl but
> it does not look so bad (I haven't really read the patch, though).

There are a few features which could be split in separate commits:
 * there are a few improvements to gitweb.css, independent of 
   incremental blame view, like td.warning -> .warning
 * adding to gitweb writing how long it took to generate page should
   be made into separate commit, probably made optional, use better
   HTML style, and have some fallback if there is no Time::HiRes

 * progress report could be made into separate commit; I needed it to
   debug code, to check if it progress nicely, but it is not strictly
   required (but it is nice to have visual indicator of progress)
 * 3-coloring of blamed lines during adding blame info was added for
   the fun of it, and should probably be in separate commit
 * adding author initials a'la "git gui blame" while nice could also
   be put in separate commit, probably adding this feature also to
   ordinary 'blame' output

[...]

Show 8 quoted lines

> > Differences between 'blame' and 'blame_incremental' output:
> 
> Hmm, are these by design in the sense that "when people are getting
> incremental blame output, the normal blame output format is unsuitable for
> such and such reasons and that is why there have to be these differences",
> or "the code happens to produce slightly different results because it is
> implemented differently; the differences are listed here as due
> diligence"?

Actually it is both. Some of differences are _currently_ not possible to resolve (parent commit 'lineno' links, split group of lines blamed by the same commit), some are coded differently (title attribute for sha1, rowspan="1", author initials feature), and some are impossible in incremental blame at least during generation (zebra table) or does not make sense in 'blame' view (progress indicators).

Show 5 quoted lines

> > P.P.S. What is the stance for copyrigth assesments in the files
> > for git code, like the ones in gitweb/gitweb.perl and gitweb/blame.js?
> 
> There is no copyright assignment.  Everybody retains the own copyright on
> their own work.

Errr... I'm sorry, I haven't made myself clear. I wanted to ask what is the best practices about copyright statement lines like

  // Copyright (C) 2007, Fredrik Kuivinen <frekui@gmail.com>

and other results of "git grep Copyright": should it be added for initial author, for main authors... I guess not for all authors.

> > P.P.P.S. Should I use Signed-off-by from Pasky and Fredrik if I based
> > my code on theirs, and if they all signed their patches?
> 
> I think that is in line with what Certificate of Origin asks you to do.

 
I was bit confused because Petr Baudis in his patch used Cc: and not
Signed-off-by: to Fredrik Kuivinen...

-- 
Jakub Narebski
Poland