From: Jakub Narebski <jnareb@gmail.com>
Date: Thu, 11 Dec 2008 17:28:09 GMT
Subject: Re: [RFC/PATCH 4/3] gitweb: Incremental blame (proof of concept)
Message-ID: <m3r64ehba7.fsf@localhost.localdomain>
In-Reply-To: <20081210200908.16899.36727.stgit@localhost.localdomain>

Jakub Narebski <jnareb@gmail.com> writes:

> This is tweaked up version of Petr Baudis <pasky@suse.cz> patch, which
> in turn was tweaked up version of Fredrik Kuivinen <frekui@gmail.com>'s
> proof of concept patch.  It adds 'blame_incremental' view, which
> incrementally displays line data in blame view using JavaScript (AJAX).

[...]
> Patch by Petr Baudis this one is based on:
>   http://permalink.gmane.org/gmane.comp.version-control.git/56657
> 
> Original patch by Fredrik Kuivinen:
>   http://article.gmane.org/gmane.comp.version-control.git/41361
> 
> Snippet adding 'generated in' to gitweb, by Petr Baudis:
>   http://article.gmane.org/gmane.comp.version-control.git/83306
> 
> Should I post interdiff to Petr Baudis patch, and comments about
> difference between them? [...]

Here is the list of differences between Petr Baudis patch and the one
I have just send.  No interdiff, as it is artificially large because
previous patch was based on much older version, so ranges does not
match.

Bugs I have made:
 * I forgot to make some changes for git-instaweb.sh to have support
   for incremental blame, namely dependency of 'git-instaweb' target
   in Makefile on gitweb/blame.js, and lack of the following line in
   git-instaweb.sh:
        gitweb_blamejs $GIT_DIR/gitweb/blame.js

 * Pasky's patch added support for href(...,-partial_query=>1) extra
   parameter, which ensured that gitweb link had '?' in it, and used
   it to generate 'baseUrl' parameter for startBlame.  I have
   misunderstood what baseUrl is about, and used $my_url there, while
   it is partial URL for blame links: it is projectUrl.

   Therefore links in blame table current 'blame_incremental' would
   not work. I'm sorry about that, I thought I have checked it...

Intentionally omitted features:
 * In patch this one is based on there was fixBlameLinks() JavaScript
   function (put directly in the HTML head inside <script> element),
   which was used in body.onLoad event to change 'a=blame' to
   'a=blame_incremental' in links marked with class="blamelink".

   First, this IMHO should be put as separate patch; you can test
   'blame_incremental' view by hand-crafting gitweb URL.  Second, it
   would be not enough in current gitweb, as action can be put in
   path_info. So either fixBlameLinks() should be made work in both
   cases, or it should be done in different way, for example adding
   'js=1' for all links, or doing JavaScript redirect from 'blame'
   view (although this way we won't be able to view ordinary 'blame'
   view without turning off JavaScript).

Differences in coding of the same features:
 * In Pasky's patch git_blame (then named git_blame2) and
   git_blame_incremental were just wrappers around git_blame_common;
   in this patch git_blame_data is also wrapper (to avoid duplicating
   permissions and parameter checking code).

 * Instead of calculating title string for "Commit" column cell, and
   necessary data for each row, we now calculate it once per commit
   and save (cache) in 'commits' associative array (hash).

   This is a bit of performance improvement, similar to the one in 
     "[PATCH 2/3] gitweb: Cache $parent_commit info in git_blame()"
   for 'blame' view in gitweb. It is just using Date() and string
   concatenation, and not extra fork.

 * Variables holding manipulated elements are named a bit differently,
   and calculated upfront:
      td_sha1   instead of  tr.firstChild
      a_sha1    instead of  ahsAnchor
      a_linenr  instead of  lineAnchor

 * There were a few of style changes in gitweb/blame.js; for example
   it is used the following style of function definition

      function functionName(arg1, arg2) {

   thorough the code.

Fixes for bugs in Pasky's patch, and changes related to changes in
ordinary 'blame' output: 
 * handleResponse function was called only from XMLHttpRequest
   onreadystatechange event. Not all browsers call onreadystatechange
   each time the server flushes output (Firefox does that), so we use
   a timer to check (poll) for changes.

   See http://ajaxpatterns.org/HTTP_Streaming#Refactoring_Illustration

 * The 'linenr' link was to line number commit.srcline, while it
   should be to (commit.srcline + i), as commit.srcline is _start_
   line in group, and not the line equivalent to current line in
   source.

   This might be thought a (mis)feature, and not a bug, though.

 * Currently 'blame' view uses single cell (with rowspan="N") spanning
   the whole group of lines blaming the same commit, instead of using
   empty cells for subsequent lines in group.  Therefore instead of
   setting "shaAnchor.innerHTML = '';" to make subsequent cells in
   "Commit" ('sha1') column empty (or rather to make link element <a>
   in cell empty), we use "tr.removeChild(td_sha1);"

   This change was made necessary by changes in the 'blame' view.
   This also meant that the code checking if lines are in the same
   group has to be changes; it was refactored into startOfGroup(tr)
   function.

 * The title for cells in "Commit" column used UTC (GMT) date and time
      'Kay Sievers, 2005-08-07 19:49:46'
   instead of the localtime format used currently by 'blame' view:
      'Kay Sievers, 2005-08-07 21:49:46 +0200'
   Current code uses neither, but 'commit'-like format:
      'Kay Sievers, 2005-08-07 19:49:46 +0000 (21:49:46 +0200)'

New features (in short):
 * 3-coloring of lines with blame data during incremental blaming
 * Adding author initials below shortened SHA-1 of a commit
   (if there is place for it, i.e. if group has more than 1 row)
 * progress indicator: progress info and progress bar
 * information about how long it took to run 'blame_data',
   and how long it took to run JavaScript script

-- 
Jakub Narebski
Poland
ShadeHawk on #git