[PATCH 0/2] jn/gitweb-blame fixes

[PATCH 0/2] jn/gitweb-blame fixes

[Stephen Boyd]

This series is based on next's jn/gitweb-blame branch.

I was trying out the incremental blame and noticed it didn't work each 
time. The first patch fixes the crashing I experience when blaming
gitweb.perl (ironic ;-)

The second patch fixes a visible bug I see in Firefox. Although I assume
on other browsers it's not a problem? I haven't tested it on others so I
can't be sure.

Stephen Boyd (2):
  gitweb.js: fix null object exception in initials calculation
  gitweb.js: use unicode encoding for nbsp instead of html entity

 gitweb/gitweb.js |   15 +++++++++------
 1 files changed, 9 insertions(+), 6 deletions(-)

[PATCH 1/2] gitweb.js: fix null object exception in initials calculation

[Stephen Boyd]

Currently handleLine() assumes that a commit author name will always
start with a capital letter. It's possible that the author name is
user@example.com and therefore calling a match() on the name will fail
to return any matches. Subsequently joining these matches will cause an
exception. Fix by checking that we have a match before trying to join
the results into a set of initials for the author.

Signed-off-by: Stephen Boyd <bebarino@gmail.com>
---
 gitweb/gitweb.js |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/gitweb/gitweb.js b/gitweb/gitweb.js
index 22570f5..02454d8 100644
--- a/gitweb/gitweb.js
+++ b/gitweb/gitweb.js
@@ -532,8 +532,11 @@ function handleLine(commit, group) {
 			if (group.numlines >= 2) {
 				var fragment = document.createDocumentFragment();
 				var br   = document.createElement("br");
-				var text = document.createTextNode(
-					commit.author.match(/\b([A-Z])\B/g).join(''));
+				var match = commit.author.match(/\b([A-Z])\B/g);
+				if (match) {
+					var text = document.createTextNode(
+							match.join(''));
+				}
 				if (br && text) {
 					var elem = fragment || td_sha1;
 					elem.appendChild(br);
-- 
1.6.5.3.433.g11067

[PATCH 2/2] gitweb.js: use unicode encoding for nbsp instead of html entity

[Stephen Boyd]

It seems that in Firefox-3.5 inserting nbsp with javascript inserts the
literal nbsp; instead of a space. Fix this by inserting the unicode
representation for nbsp instead.

Signed-off-by: Stephen Boyd <bebarino@gmail.com>
---
 gitweb/gitweb.js |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/gitweb/gitweb.js b/gitweb/gitweb.js
index 02454d8..30597dd 100644
--- a/gitweb/gitweb.js
+++ b/gitweb/gitweb.js
@@ -31,12 +31,12 @@
 
 /**
  * pad number N with nonbreakable spaces on the left, to WIDTH characters
- * example: padLeftStr(12, 3, '&nbsp;') == '&nbsp;12'
- *          ('&nbsp;' is nonbreakable space)
+ * example: padLeftStr(12, 3, '\u00A0') == '\u00A012'
+ *          ('\u00A0' is nonbreakable space)
  *
  * @param {Number|String} input: number to pad
  * @param {Number} width: visible width of output
- * @param {String} str: string to prefix to string, e.g. '&nbsp;'
+ * @param {String} str: string to prefix to string, e.g. '\u00A0'
  * @returns {String} INPUT prefixed with (WIDTH - INPUT.length) x STR
  */
 function padLeftStr(input, width, str) {
@@ -158,7 +158,7 @@ function updateProgressInfo() {
 
 	if (div_progress_info) {
 		div_progress_info.firstChild.data  = blamedLines + ' / ' + totalLines +
-			' (' + padLeftStr(percentage, 3, '&nbsp;') + '%)';
+			' (' + padLeftStr(percentage, 3, '\u00A0') + '%)';
 	}
 
 	if (div_progress_bar) {
-- 
1.6.5.3.433.g11067

Re: [PATCH 1/2] gitweb.js: fix null object exception in initials calculation

[Jakub Narebski]

On Thu, 19 Nov 2009, Stephen Boyd wrote:

> Currently handleLine() assumes that a commit author name will always
> start with a capital letter. It's possible that the author name is
> user@example.com and therefore calling a match() on the name will fail
> to return any matches. Subsequently joining these matches will cause an
> exception. Fix by checking that we have a match before trying to join
> the results into a set of initials for the author.
> 
> Signed-off-by: Stephen Boyd <bebarino@gmail.com>

Acked-by: Jakub Narebski <jnareb@gmail.com>

> ---

[From "[PATCH 0/2] jn/gitweb-blame fixes"]
> This series is based on next's jn/gitweb-blame branch.
>
> I was trying out the incremental blame and noticed it didn't work
> each time. The first patch fixes the crashing I experience when
> blaming gitweb.perl (ironic ;-)

Hmmm... gitweb/gitweb.perl *was* one of files I have tested 
'blame_incremental' view on, but I have not experienced any
crashes.

Perhaps it was the matter of different JavaScript engine (Mozilla 1.7.12
with Gecko/20050923 engine, Konqueror 3.5.3-0.2.fc4), or different
starting point for blame.

I assume that crashing lead simply to not working blame, not to browser
crash?

>  gitweb/gitweb.js |    7 +++++--
>  1 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/gitweb/gitweb.js b/gitweb/gitweb.js
> index 22570f5..02454d8 100644
> --- a/gitweb/gitweb.js
> +++ b/gitweb/gitweb.js
> @@ -532,8 +532,11 @@ function handleLine(commit, group) {
>  			if (group.numlines >= 2) {
>  				var fragment = document.createDocumentFragment();
>  				var br   = document.createElement("br");
> -				var text = document.createTextNode(
> -					commit.author.match(/\b([A-Z])\B/g).join(''));
> +				var match = commit.author.match(/\b([A-Z])\B/g);
> +				if (match) {
> +					var text = document.createTextNode(
> +							match.join(''));
> +				}
>  				if (br && text) {
>  					var elem = fragment || td_sha1;
>  					elem.appendChild(br);

Thanks.  It now corresponds to what it is done for ordinary 'blame'
view, i.e.:

	my @author_initials = ($author =~ /\b([[:upper:]])\B/g);
	if (@author_initials) {
		print "<br />" .
		      esc_html(join('', @author_initials));
		#           or join('.', ...)
	}


P.S. Unfortunately unless we decide to generate JavaScript from Perl code,
using something like GWT for Java or Pylons for Python, e.g. CGI::Ajax
(which is not in Perl core), we would have to have such code duplication.

-- 
Jakub Narebski
Poland

Re: [PATCH 1/2] gitweb.js: fix null object exception in initials calculation

[Stephen Boyd]

2009/11/19 Jakub Narebski <jnareb@gmail.com>:
>
> Hmmm... gitweb/gitweb.perl *was* one of files I have tested
> 'blame_incremental' view on, but I have not experienced any
> crashes.
>
> Perhaps it was the matter of different JavaScript engine (Mozilla 1.7.12
> with Gecko/20050923 engine, Konqueror 3.5.3-0.2.fc4), or different
> starting point for blame.
>
> I assume that crashing lead simply to not working blame, not to browser
> crash?

Yes. The blame stops at 20% or something but the browser is fine.

Re: [PATCH 2/2] gitweb.js: use unicode encoding for nbsp instead of html entity

[Jakub Narebski]

On Thu, 19 Nov 2009, Stephen Boyd wrote:

> It seems that in Firefox-3.5 inserting nbsp with javascript inserts the
> literal nbsp; instead of a space. Fix this by inserting the unicode
> representation for nbsp instead.

Errr... why are you avoiding writing   or " " here?

> 
> Signed-off-by: Stephen Boyd <bebarino@gmail.com>
> ---

[From "[PATCH 0/2] jn/gitweb-blame fixes"]
> This series is based on next's jn/gitweb-blame branch.

> The second patch fixes a visible bug I see in Firefox. Although I
> assume on other browsers it's not a problem? I haven't tested it on
> others so I can't be sure.

Well, since I moved from elem.innerHTML (which is non-standard, and
does not work for some browsers in strict XHTML mode) to setting
elem.firstChild.data (which assumes that firstChild exists and it
is a text node) I have had damned *intermittent* bugs where sometimes
'&nbsp;' would be shown literally, and sometimes this entity would
be correctly rendered.

I suspect this is either bug in Firefox, or unspecified part of DOM.

As we need this only for progress report, I am not against this change,
if it doesn't make it worse in other browsers.

>  gitweb/gitweb.js |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/gitweb/gitweb.js b/gitweb/gitweb.js
> index 02454d8..30597dd 100644
> --- a/gitweb/gitweb.js
> +++ b/gitweb/gitweb.js
> @@ -31,12 +31,12 @@
>  
>  /**
>   * pad number N with nonbreakable spaces on the left, to WIDTH characters
> - * example: padLeftStr(12, 3, '&nbsp;') == '&nbsp;12'
> - *          ('&nbsp;' is nonbreakable space)
> + * example: padLeftStr(12, 3, '\u00A0') == '\u00A012'
> + *          ('\u00A0' is nonbreakable space)
>   *
>   * @param {Number|String} input: number to pad
>   * @param {Number} width: visible width of output
> - * @param {String} str: string to prefix to string, e.g. '&nbsp;'
> + * @param {String} str: string to prefix to string, e.g. '\u00A0'
>   * @returns {String} INPUT prefixed with (WIDTH - INPUT.length) x STR
>   */
>  function padLeftStr(input, width, str) {
> @@ -158,7 +158,7 @@ function updateProgressInfo() {
>  
>  	if (div_progress_info) {
>  		div_progress_info.firstChild.data  = blamedLines + ' / ' + totalLines +
> -			' (' + padLeftStr(percentage, 3, '&nbsp;') + '%)';
> +			' (' + padLeftStr(percentage, 3, '\u00A0') + '%)';
>  	}
>  
>  	if (div_progress_bar) {
> -- 
> 1.6.5.3.433.g11067
> 
> 

-- 
Jakub Narebski
Poland

Re: [PATCH 0/2] jn/gitweb-blame fixes

[Jakub Narebski]

On Thu, 19 Nov 2009, Stephen Boyd wrote:

> This series is based on next's jn/gitweb-blame branch.
> 
> I was trying out the incremental blame and noticed it didn't work each 
> time. The first patch fixes the crashing I experience when blaming
> gitweb.perl (ironic ;-)
> 
> The second patch fixes a visible bug I see in Firefox. Although I assume
> on other browsers it's not a problem? I haven't tested it on others so I
> can't be sure.

Thanks for working on this.  Also it is nice to have incremental blame
tested for another browser, beside Mozilla 1.17.2 and Konqueror 3.5.3

-- 
Jakub Narebski
Poland

Re: [PATCH 2/2] gitweb.js: use unicode encoding for nbsp instead of html entity

[Stephen Boyd]

Jakub Narebski wrote:
> On Thu, 19 Nov 2009, Stephen Boyd wrote
>> It seems that in Firefox-3.5 inserting nbsp with javascript inserts the
>> literal nbsp; instead of a space. Fix this by inserting the unicode
>> representation for nbsp instead.
>
> Errr... why are you avoiding writing   or " " here?

Sorry, this part was rushed out. I can fix it in a resend if this is 
actually the right thing to do.

> Well, since I moved from elem.innerHTML (which is non-standard, and
> does not work for some browsers in strict XHTML mode) to setting
> elem.firstChild.data (which assumes that firstChild exists and it
> is a text node) I have had damned *intermittent* bugs where sometimes
> ' ' would be shown literally, and sometimes this entity would
> be correctly rendered.
>
> I suspect this is either bug in Firefox, or unspecified part of DOM.
>
> As we need this only for progress report, I am not against this change,
> if it doesn't make it worse in other browsers.

I've tested this in Opera-10.10 now and it looks good. I'll try to get 
my hands on a Windows machine to test with IE, but no promises.

Re: [PATCH 0/2] jn/gitweb-blame fixes

[Stephen Boyd]

Jakub Narebski wrote:
>
> Thanks for working on this.  Also it is nice to have incremental blame
> tested for another browser, beside Mozilla 1.17.2 and Konqueror 3.5.3

For those following along, Opera-10.10 has been tested and works.

Re: [PATCH 0/2] jn/gitweb-blame fixes

[Stephen Boyd]

Stephen Boyd wrote:
> Jakub Narebski wrote:
>>
>> Thanks for working on this.  Also it is nice to have incremental blame
>> tested for another browser, beside Mozilla 1.17.2 and Konqueror 3.5.3
>
> For those following along, Opera-10.10 has been tested and works.

Ok. I tried using the version of incremental blame that's in next 
(e206d62 gitweb: Colorize 'blame_incremental' view during processing, 
2009-09-01) on IE8 with no success. The page loads and the file is shown 
with line numbers, but the progress is stuck at 0% (with the   
showing too).

I then tried with my two patches applied on top of e206d62 on IE8 and 
still no success. The page loads and the file is show with the line 
numbers but still stuck at 0%, but the   is gone at least.

Do you have access to IE8 to confirm?

Re: [PATCH 0/2] jn/gitweb-blame fixes

[Jakub Narebski]

On Fri, 20 Nov 2009, Stephen Boyd wrote:
> Stephen Boyd wrote:
> > Jakub Narebski wrote:
> > >
> > > Thanks for working on this.  Also it is nice to have incremental blame
> > > tested for another browser, beside Mozilla 1.17.2 and Konqueror 3.5.3
> >
> > For those following along, Opera-10.10 has been tested and works.
> 
> Ok. I tried using the version of incremental blame that's in next 
> (e206d62 gitweb: Colorize 'blame_incremental' view during processing, 
> 2009-09-01) on IE8 with no success. The page loads and the file is shown 
> with line numbers, but the progress is stuck at 0% (with the   
> showing too).
> 
> I then tried with my two patches applied on top of e206d62 on IE8 and 
> still no success. The page loads and the file is show with the line 
> numbers but still stuck at 0%, but the   is gone at least.
> 
> Do you have access to IE8 to confirm?

I have tested gitweb with both of your patches applied, serving gitweb
as CGI script using Apache 2.0.54 on Linux, and viewing from separate
computer on MS Windows XP, with the following results:

* For the following browsers blame_incremental view on gitweb/gitweb.perl
  file produces correct output, but for progress info which instead of
  (  1%) -> ( 29%) -> (100%) looks more like ( 1%) -> (29%) -> (100%)

  + Firefox 3.5.5 (rv:1.9.1.5 Gecko/20091102)
  + Opera 10.01
  + Google Chrome 3.0.195.33

* Testing it with IE8 (Internet Explorer 8.0.6001.18702) page loading stops
  at 0%, at the very beginning on startBlame() function

  IE8 shows that it finds the following errors:

  * "firstChild is null or not an object"
    line: 565, char:4

      a_sha1.firstChild.data = commit.sha1.substr(0, 8);

    It might be caused by the fact that firstChild for this case should be
    text node containing of pure whitespace:
       <a href=""> </a>
    Perhaps IE8 simplifies it in "compatability view" mode

 * "Unspecified error" (twice)
   line: 777, char:2

     if (xhr.readyState === 3 && xhr.status !== 200) {
     	return;
     }

   I don't know what might be the source of error here; I suspect that the
   error position mentioned by IE8 is bogus.

-- 
Jakub Narebski
Poland

Re: [PATCH 0/2] jn/gitweb-blame fixes

[Jakub Narebski]

On Sat, 21 Nov 2009, Jakub Narebski wrote:

> * Testing it with IE8 (Internet Explorer 8.0.6001.18702) page loading stops
>   at 0%, at the very beginning on startBlame() function
> 
>   IE8 shows that it finds the following errors:
> 
>   * "firstChild is null or not an object"
>     line: 565, char:4
> 
>       a_sha1.firstChild.data = commit.sha1.substr(0, 8);
> 
>     It might be caused by the fact that firstChild for this case should be
>     text node containing of pure whitespace:
>        <a href=""> </a>
>     Perhaps IE8 simplifies it in "compatibility view" mode

This bug (be it in gitweb.js or in IE8) is fixed by the following patch:

-- 8< --
diff --git i/gitweb/gitweb.js w/gitweb/gitweb.js
index 200ec5a..c1e425c 100644
--- i/gitweb/gitweb.js
+++ w/gitweb/gitweb.js
@@ -562,7 +562,12 @@ function handleLine(commit, group) {
 			td_sha1.rowSpan = group.numlines;
 
 			a_sha1.href = projectUrl + 'a=commit;h=' + commit.sha1;
-			a_sha1.firstChild.data = commit.sha1.substr(0, 8);
+			if (a_sha1.firstChild) {
+				a_sha1.firstChild.data = commit.sha1.substr(0, 8);
+			} else {
+				a_sha1.appendChild(
+					document.createTextNode(commit.sha1.substr(0, 8)));
+			}
 			if (group.numlines >= 2) {
 				var fragment = document.createDocumentFragment();
 				var br   = document.createElement("br");
-- >8 --

> 
>  * "Unspecified error" (twice)
>    line: 777, char:2
> 
>      if (xhr.readyState === 3 && xhr.status !== 200) {
>      	return;
>      }
> 
>    I don't know what might be the source of error here; I suspect that the
>    error position mentioned by IE8 is bogus.

But I have no idea how to fix this.  "Unspecified error" isn't very 
helpful...

-- 
Jakub Narebski
Poland

Re: [PATCH 0/2] jn/gitweb-blame fixes

[Stephen Boyd]

Jakub Narebski wrote:
> I have tested gitweb with both of your patches applied, serving gitweb
> as CGI script using Apache 2.0.54 on Linux, and viewing from separate
> computer on MS Windows XP, with the following results:
>
> * For the following browsers blame_incremental view on gitweb/gitweb.perl
>   file produces correct output, but for progress info which instead of
>   (  1%) -> ( 29%) -> (100%) looks more like ( 1%) -> (29%) -> (100%)

This is due to an off-by-one error in the while loop. This should fix 
it. I'll probably squash this into patch 2 and resend.

--->8----

diff --git a/gitweb/gitweb.js b/gitweb/gitweb.js
index 30597dd..9214497 100644
--- a/gitweb/gitweb.js
+++ b/gitweb/gitweb.js
@@ -43,7 +43,7 @@ function padLeftStr(input, width, str) {
        var prefix = '';
 
        width -= input.toString().length;
-       while (width > 1) {
+       while (width > 0) {
                prefix += str;
                width--;
        }

[PATCH] gitweb.js: Harden setting blamed commit info in incremental blame

[Jakub Narebski]

On Sat, 21 Nov 2009, Jakub Narebski wrote:
> On Sat, 21 Nov 2009, Jakub Narebski wrote:
> 
> > * Testing it with IE8 (Internet Explorer 8.0.6001.18702) page loading stops
> >   at 0%, at the very beginning on startBlame() function
> > 
> >   IE8 shows that it finds the following errors:
> > 
> >   * "firstChild is null or not an object"
> >     line: 565, char:4
> > 
> >       a_sha1.firstChild.data = commit.sha1.substr(0, 8);
> > 
> >     It might be caused by the fact that firstChild for this case should be
> >     text node containing of pure whitespace:
> >        <a href=""> </a>
> >     Perhaps IE8 simplifies it in "compatibility view" mode
> 
> This bug (be it in gitweb.js or in IE8) is fixed by the following patch:
> 
> -- 8< --
> diff --git i/gitweb/gitweb.js w/gitweb/gitweb.js
> index 200ec5a..c1e425c 100644
> --- i/gitweb/gitweb.js
> +++ w/gitweb/gitweb.js
> @@ -562,7 +562,12 @@ function handleLine(commit, group) {
>  			td_sha1.rowSpan = group.numlines;
>  
>  			a_sha1.href = projectUrl + 'a=commit;h=' + commit.sha1;
> -			a_sha1.firstChild.data = commit.sha1.substr(0, 8);
> +			if (a_sha1.firstChild) {
> +				a_sha1.firstChild.data = commit.sha1.substr(0, 8);
> +			} else {
> +				a_sha1.appendChild(
> +					document.createTextNode(commit.sha1.substr(0, 8)));
> +			}
>  			if (group.numlines >= 2) {
>  				var fragment = document.createDocumentFragment();
>  				var br   = document.createElement("br");
> -- >8 --

Below the same patch is in the form of a proper commit; although the title
(subject) of this commit could be better...

> >  * "Unspecified error" (twice)
> >    line: 777, char:2
> > 
> >      if (xhr.readyState === 3 && xhr.status !== 200) {
> >      	return;
> >      }
> > 
> >    I don't know what might be the source of error here; I suspect that the
> >    error position mentioned by IE8 is bogus.
> 
> But I have no idea how to fix this.  "Unspecified error" isn't very 
> helpful...

Debugging this is serious PITA.  After fix below it appears that this bug
is some intermittent bug, depending on XMLHttpRequest timing.  It more
often than not (at least when I tried to debug it using build-in IE8
debugger) works correctly for the folowing files: README, GIT-VERSION-GEN,
revision.c (once even it did fail when first running for given file, and
then running correctly when reloading from debugger; fun it is not).

It does consistently fail for gitweb/gitweb.perl... but when I tried
to debug it IE8 would hang up when trying to use debugger (with around
600MB available RAM).  Perhaps somebody else would have more luck...

-- >8 --
Subject: [PATCH] gitweb.js: Harden setting blamed commit info in incremental blame

Internet Explorer 8 stops at beginning of blame filling
with the following bug
  "firstChild is null or not an object"
at this line
  a_sha1.firstChild.data = commit.sha1.substr(0, 8);

It is (probably) caused by the fact that while a_sha1 element,
which looks like this
  <a href=""> </a>
has firstChild which is text node containing only whitespace (single
space character) in other web browsers (Firefox 3.5, Opera 10,
Google Chrome 3.0), IE8 simplifies DOM, removing trailing/leading
whitespace.

Protect against this bug by creating text element if it does not
exist.

Found-by: Stephen Boyd <bebarino@gmail.com>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
 gitweb/gitweb.js |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.js b/gitweb/gitweb.js
index 200ec5a..c1e425c 100644
--- a/gitweb/gitweb.js
+++ b/gitweb/gitweb.js
@@ -562,7 +562,12 @@ function handleLine(commit, group) {
 			td_sha1.rowSpan = group.numlines;
 
 			a_sha1.href = projectUrl + 'a=commit;h=' + commit.sha1;
-			a_sha1.firstChild.data = commit.sha1.substr(0, 8);
+			if (a_sha1.firstChild) {
+				a_sha1.firstChild.data = commit.sha1.substr(0, 8);
+			} else {
+				a_sha1.appendChild(
+					document.createTextNode(commit.sha1.substr(0, 8)));
+			}
 			if (group.numlines >= 2) {
 				var fragment = document.createDocumentFragment();
 				var br   = document.createElement("br");
-- 
1.6.5.3

Re: [PATCH] gitweb.js: Harden setting blamed commit info in incremental blame

[Nanako Shiraishi]

Quoting Jakub Narebski <jnareb@gmail.com>

> On Sat, 21 Nov 2009, Jakub Narebski wrote:
>
> Below the same patch is in the form of a proper commit; although the title
> (subject) of this commit could be better...

Does this replace the first of the previous two-patch series? Is Stephen's second patch still needed (with his fix)?

Quoting Stephen Boyd <bebarino@gmail.com>

> Jakub Narebski wrote:
>> I have tested gitweb with both of your patches applied, serving gitweb
>> as CGI script using Apache 2.0.54 on Linux, and viewing from separate
>> computer on MS Windows XP, with the following results:
>>
>> * For the following browsers blame_incremental view on gitweb/gitweb.perl
>>   file produces correct output, but for progress info which instead of
>>   (  1%) -> ( 29%) -> (100%) looks more like ( 1%) -> (29%) -> (100%)
>
> This is due to an off-by-one error in the while loop. This should fix
> it. I'll probably squash this into patch 2 and resend.
>
> --->8----
>
> diff --git a/gitweb/gitweb.js b/gitweb/gitweb.js
> index 30597dd..9214497 100644
> --- a/gitweb/gitweb.js
> +++ b/gitweb/gitweb.js
> @@ -43,7 +43,7 @@ function padLeftStr(input, width, str) {
>        var prefix = '';
>
>        width -= input.toString().length;
> -       while (width > 1) {
> +       while (width > 0) {
>                prefix += str;
>                width--;
>        }

-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/

Re: [PATCH] gitweb.js: Harden setting blamed commit info in incremental blame

[Jakub Narebski]

On Wed, 25 Nov 2009, Nanako Shiraishi wrote:
> Quoting Jakub Narebski <jnareb@gmail.com>
> 
> > On Sat, 21 Nov 2009, Jakub Narebski wrote:
> >
> > Below the same patch is in the form of a proper commit; although the title
> > (subject) of this commit could be better...
> 
> Does this replace the first of the previous two-patch series? Is
> Stephen's second patch still needed (with his fix)? 

No, it replaces neither first patch of Stephen's two patch series that
started this thread, namely "gitweb.js: fix null object exception in
initials calculation" (which is about initials of committer... if they
exist), nor the follow-up fix of off-by-one bug in padLeftStr.

This is fix of an independent issue (even if it occurs only in IE8).

-- 
Jakub Narebski
Poland

[PATCHv2 2/2] gitweb.js: fix padLeftStr() and its usage

[Stephen Boyd]

It seems that in Firefox-3.5 inserting   with javascript inserts the
literal   instead of a space. Fix this by inserting the unicode
representation for   instead.

Also fix the off-by-one error in the padding calculation that was
causing one less space to be inserted than was requested by the caller.

Signed-off-by: Stephen Boyd <bebarino@gmail.com>
Cc: Jakub Narebski <jnareb@gmail.com>
---

Fixed the commit message and squashed in the off-by-one bug.

 gitweb/gitweb.js |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/gitweb/gitweb.js b/gitweb/gitweb.js
index 02454d8..9214497 100644
--- a/gitweb/gitweb.js
+++ b/gitweb/gitweb.js
@@ -31,19 +31,19 @@
 
 /**
  * pad number N with nonbreakable spaces on the left, to WIDTH characters
- * example: padLeftStr(12, 3, '&nbsp;') == '&nbsp;12'
- *          ('&nbsp;' is nonbreakable space)
+ * example: padLeftStr(12, 3, '\u00A0') == '\u00A012'
+ *          ('\u00A0' is nonbreakable space)
  *
  * @param {Number|String} input: number to pad
  * @param {Number} width: visible width of output
- * @param {String} str: string to prefix to string, e.g. '&nbsp;'
+ * @param {String} str: string to prefix to string, e.g. '\u00A0'
  * @returns {String} INPUT prefixed with (WIDTH - INPUT.length) x STR
  */
 function padLeftStr(input, width, str) {
 	var prefix = '';
 
 	width -= input.toString().length;
-	while (width > 1) {
+	while (width > 0) {
 		prefix += str;
 		width--;
 	}
@@ -158,7 +158,7 @@ function updateProgressInfo() {
 
 	if (div_progress_info) {
 		div_progress_info.firstChild.data  = blamedLines + ' / ' + totalLines +
-			' (' + padLeftStr(percentage, 3, '&nbsp;') + '%)';
+			' (' + padLeftStr(percentage, 3, '\u00A0') + '%)';
 	}
 
 	if (div_progress_bar) {
-- 
1.6.6.rc0

Re: [PATCH] gitweb.js: Harden setting blamed commit info in incremental blame

[Stephen Boyd]

Jakub Narebski wrote:
>
> Debugging this is serious PITA.  After fix below it appears that this bug
> is some intermittent bug, depending on XMLHttpRequest timing.  It more
> often than not (at least when I tried to debug it using build-in IE8
> debugger) works correctly for the folowing files: README, GIT-VERSION-GEN,
> revision.c (once even it did fail when first running for given file, and
> then running correctly when reloading from debugger; fun it is not).
>
> It does consistently fail for gitweb/gitweb.perl... but when I tried
> to debug it IE8 would hang up when trying to use debugger (with around
> 600MB available RAM).  Perhaps somebody else would have more luck...

Interesting. I don't have time to look into this until early December, 
but if it's still around then I'll take a look. I wonder if IE6 or IE7 
works (I don't think everyone is on version 8 yet).

Re: [PATCH] gitweb.js: Harden setting blamed commit info in incremental blame

[Jakub Narebski]

On Wed, 25 Nov 2009 05:01, Stephen Boyd wrote:
> Jakub Narebski wrote:
> >
> > Debugging this is serious PITA.  After fix below it appears that this bug
> > is some intermittent bug, depending on XMLHttpRequest timing.  It more
> > often than not (at least when I tried to debug it using build-in IE8
> > debugger) works correctly for the folowing files: README, GIT-VERSION-GEN,
> > revision.c (once even it did fail when first running for given file, and
> > then running correctly when reloading from debugger; fun it is not).
> >
> > It does consistently fail for gitweb/gitweb.perl... but when I tried
> > to debug it IE8 would hang up when trying to use debugger (with around
> > 600MB available RAM).  Perhaps somebody else would have more luck...
> 
> Interesting. I don't have time to look into this until early December, 
> but if it's still around then I'll take a look. I wonder if IE6 or IE7 
> works (I don't think everyone is on version 8 yet).

Well, the one time I was able to run debugger (F12, select 'Script', select
'gitweb.js') with error occurring and without IE hanging (for README file)
it did show an error for the following line:

  if (xhr.readyState === 3 && xhr.status !== 200) {

When I checked 'xhr' object, it has "Unknown error" as contents of 
xhr.statusText field and as contents of xhr.status (sic!), which should
be a number: HTTP status code.

Unfortunately I had to take a break... and I was not able to reproduce this
(without hanging web browser: "program not responding") since then...

-- 
Jakub Narebski
Poland

Re: [PATCH] gitweb.js: Harden setting blamed commit info in incremental blame

[Jakub Narebski]

On Wed, 25 Nov 2009, Jakub Narebski wrote:
> On Wed, 25 Nov 2009 05:01, Stephen Boyd wrote:
> > Jakub Narebski wrote:
> > >
> > > Debugging this is serious PITA.  After fix below it appears that this bug
> > > is some intermittent bug, depending on XMLHttpRequest timing.  It more
> > > often than not (at least when I tried to debug it using build-in IE8
> > > debugger) works correctly for the folowing files: README, GIT-VERSION-GEN,
> > > revision.c (once even it did fail when first running for given file, and
> > > then running correctly when reloading from debugger; fun it is not).
> > >
> > > It does consistently fail for gitweb/gitweb.perl... but when I tried
> > > to debug it IE8 would hang up when trying to use debugger (with around
> > > 600MB available RAM).  Perhaps somebody else would have more luck...
> > 
> > Interesting. I don't have time to look into this until early December, 
> > but if it's still around then I'll take a look. I wonder if IE6 or IE7 
> > works (I don't think everyone is on version 8 yet).
> 
> Well, the one time I was able to run debugger (F12, select 'Script', select
> 'gitweb.js') with error occurring

The error was "Unspecified error", char:2 in the mentioned line

> and without IE hanging (for README file) it did show an error for the
> following line: 
> 
>   if (xhr.readyState === 3 && xhr.status !== 200) {
> 
> When I checked 'xhr' object, it has "Unknown error" as contents of 
> xhr.statusText field and as contents of xhr.status (sic!), which should
> be a number: HTTP status code.

It was 'Unspecified error.' shown in xhr watch.  Accessing xhr.status
causes an error.

This might be cause by the fact that xhr (XMLHttpRequest object, or as IE8
shows it in JScript debugger DispHTMLXMLHttpRequest object) is not fully
initialized, or something; gitweb.js calls handleResponse() also from
a timer, to work around the fact that some web browsers onreadystatechange
handler is called only once for each state, and not as soon as new data
is available from server.

Longer term solution would be to use onprogress handler if it is available;
if it is available we don't need trick with calling handleResponse from
timer, as XMLHttpRequest 2.0 proposed specification ensures calling callback
as soon as new data is available.

Short term solution would be to wrap access to xhr.status in try ... catch
block for IE8... although I am a bit reluctant to implement this workaround
bugs in IE8.

-- 
Jakub Narebski
Poland

Re: [PATCH] gitweb.js: Harden setting blamed commit info in incremental blame

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> It was 'Unspecified error.' shown in xhr watch.  Accessing xhr.status
> causes an error.

As to the topic, it does not seem to break _existing_ features; if that is
not the case, please let me know.

Otherwise I'm inclined to merge the entire series to 'master' by 1.6.6-rc1.

    6821dee gitweb.js: fix padLeftStr() and its usage
    6aa2de5 gitweb.js: Harden setting blamed commit info in incremental blame
    e42a05f gitweb.js: fix null object exception in initials calculation
    63267de gitweb: Minify gitweb.js if JSMIN is defined
    c4ccf61 gitweb: Create links leading to 'blame_incremental' using JavaScript
    e206d62 gitweb: Colorize 'blame_incremental' view during processing
    4af819d gitweb: Incremental blame (using JavaScript)
    aa7dd05 gitweb: Add optional "time to generate page" info in footer
    -aef3768 gitweb: Use light/dark for class names also in 'blame' view

and treat it as a new feature with known breakages, to give it wider
audience.  That way you will hopefully get more people who are willing to
help debug/fix things for you.

Re: [PATCH] gitweb.js: Harden setting blamed commit info in incremental blame

[Jakub Narebski]

On Wed, środa 25. listopada 2009 22:39, Junio C Hamano napisał:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
> > It was 'Unspecified error.' shown in xhr watch.  Accessing xhr.status
> > causes an error.
> 
> As to the topic, it does not seem to break _existing_ features; if that is
> not the case, please let me know.

With the single exception of "Create links leading to 'blame_incremental'
using JavaScript" all commits are about opt-in features: either need to
be enabled in gitweb config ('timed' feature), or you need to handcraft
URL ('blame_incremental' as action parameter).

The commit that makes 'blame' links to use 'blame_incremental' view if
JavaScript is enabled might break 'blame' view for IE8 users.

> Otherwise I'm inclined to merge the entire series to 'master' by 1.6.6-rc1.
> 
>     6821dee gitweb.js: fix padLeftStr() and its usage
>     6aa2de5 gitweb.js: Harden setting blamed commit info in incremental blame
>     e42a05f gitweb.js: fix null object exception in initials calculation
>     63267de gitweb: Minify gitweb.js if JSMIN is defined
>     c4ccf61 gitweb: Create links leading to 'blame_incremental' using JavaScript
>     e206d62 gitweb: Colorize 'blame_incremental' view during processing
>     4af819d gitweb: Incremental blame (using JavaScript)
>     aa7dd05 gitweb: Add optional "time to generate page" info in footer
>     -aef3768 gitweb: Use light/dark for class names also in 'blame' view
> 
> and treat it as a new feature with known breakages, to give it wider
> audience.  That way you will hopefully get more people who are willing to
> help debug/fix things for you.

Perhaps then it would be better to put commit that creates links to 
'blame_incremental' action last, and have it in 'next' and not in master?

Below there is request-pull with reordered series, unless you want me
to resend this series as a set of patches, as reply to this email.

-- >8 --
The following changes since commit b073b7a990deb1cb3425db45642fa18c8b3cb65c:
  Benjamin Kramer (1):
        Explicitly truncate bswap operand to uint32_t

are available in the git repository at:

  git://repo.or.cz/git/jnareb-git.git gitweb/web

  7bc7e38 gitweb: Create links leading to 'blame_incremental' using JavaScript
  aa1ff3d gitweb.js: Harden setting blamed commit info in incremental blame
  da51a3c gitweb.js: fix padLeftStr() and its usage
  a253a2c gitweb.js: fix null object exception in initials calculation
  fbebd29 gitweb: Minify gitweb.js if JSMIN is defined
  8ba6343 gitweb: Colorize 'blame_incremental' view during processing
  0e14278 gitweb: Incremental blame (using JavaScript)
  c4c6645 gitweb: Add optional "time to generate page" info in foot

Jakub Narebski (6):
      gitweb: Add optional "time to generate page" info in footer
      gitweb: Incremental blame (using JavaScript)
      gitweb: Colorize 'blame_incremental' view during processing
      gitweb: Minify gitweb.js if JSMIN is defined
      gitweb.js: Harden setting blamed commit info in incremental blame
      gitweb: Create links leading to 'blame_incremental' using JavaScript

Stephen Boyd (2):
      gitweb.js: fix null object exception in initials calculation
      gitweb.js: fix padLeftStr() and its usage

 Makefile           |   26 ++-
 git-instaweb.sh    |    7 +
 gitweb/README      |    5 +
 gitweb/gitweb.css  |   25 ++
 gitweb/gitweb.js   |  870 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 gitweb/gitweb.perl |  311 ++++++++++++++-----
 6 files changed, 1157 insertions(+), 87 deletions(-)
 create mode 100644 gitweb/gitweb.js

-- 
Jakub Narebski
Poland

Re: [PATCH] gitweb.js: Harden setting blamed commit info in incremental blame

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> Below there is request-pull with reordered series, unless you want me
> to resend this series as a set of patches, as reply to this email.

It is too late for that, as they are already in 'next'.  I do not think it
is necessary nor desirable, either.

With the "Create links" commit, you are _adding_ a new link that leads to
a new feature with known breakage, not _replacing_ a link that leads to an
existing working implementation with one that does not work for some
people, no?

Merging everything else but not merging that commit does not make any
sense.  It won't give any wider exposure to the feature, and is no better
than carrying the entire thing in 'next' (or 'pu') post 1.6.6.

A follow-up patch to add a gitweb configuration switch that disables the
non-working view by default but allows site owners to enable it in order
to help improving the feature would be a sensible thing to do.  As long as
that patch is solidly done we can merge the whole thing to 'master' in the
upcoming release.

Re: [PATCH] gitweb.js: Harden setting blamed commit info in incremental blame

[Jakub Narebski]

On Thu, 26 Nov 2009, Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
> > Below there is request-pull with reordered series, unless you want me
> > to resend this series as a set of patches, as reply to this email.
> 
> It is too late for that, as they are already in 'next'.  I do not think it
> is necessary nor desirable, either.
> 
> With the "Create links" commit, you are _adding_ a new link that leads to
> a new feature with known breakage, not _replacing_ a link that leads to an
> existing working implementation with one that does not work for some
> people, no?

No.  The "Create links" commits make existing 'blame' links, which till that
commit always lead to non-incremental 'blame' action, now lead to new
'blame_incremental' action (if JavaScript is enabled).  So the result is
that we _replace_ links to 'blame' view by links to 'blame_incremental' view.

> Merging everything else but not merging that commit does not make any
> sense.  It won't give any wider exposure to the feature, and is no better
> than carrying the entire thing in 'next' (or 'pu') post 1.6.6.
> 
> A follow-up patch to add a gitweb configuration switch that disables the
> non-working view by default but allows site owners to enable it in order
> to help improving the feature would be a sensible thing to do.  As long as
> that patch is solidly done we can merge the whole thing to 'master' in the
> upcoming release.

But if it is already in 'next', then I'll try to come up with patch which
makes JavaScript-ing links (replacing links with JavaScript to equivalent
actions utilizing JavaScript, currently only 'blame' -> 'blame_incremental')
configurable.

-- 
Jakub Narebski
Poland

[RFC/PATCH] gitweb: Make linking to actions requiring JavaScript a feature

[Jakub Narebski]

On Thu, 26 Nov 2009, Jakub Narebski wrote:
> On Thu, 26 Nov 2009, Junio C Hamano wrote:

> > A follow-up patch to add a gitweb configuration switch that disables the
> > non-working view by default but allows site owners to enable it in order
> > to help improving the feature would be a sensible thing to do.  As long as
> > that patch is solidly done we can merge the whole thing to 'master' in the
> > upcoming release.
> 
> But if it is already in 'next', then I'll try to come up with patch which
> makes JavaScript-ing links (replacing links with JavaScript to equivalent
> actions utilizing JavaScript, currently only 'blame' -> 'blame_incremental')
> configurable.

Here it is.  I am a bit ambiguous about *naming* of this feature (and
whether it should be overridable), that's why it is marked as RFC.

Also the subject of this commit could have been better, I think...

-- >8 --
Let gitweb turn some links (like 'blame' links) into linking to
actions which require JavaScript (like 'blame_incremental' action)
only if 'javascript-actions' feature is enabled.

This means that links to such actions would be present only if both
JavaScript is enabled and 'javascript-actions' feature is enabled.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
 gitweb/gitweb.perl |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index a80cbd3..0ab47e1 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -432,6 +432,13 @@ our %feature = (
 	'timed' => {
 		'override' => 0,
 		'default' => [0]},
+
+	# Enable turning some links into links to actions which require
+	# JavaScript to run (like 'blame_incremental').  Enabled by default.
+	# Project specific override is currently not supported.
+	'javascript-actions' => {
+		'override' => 0,
+		'default' => [1]},
 );
 
 sub gitweb_get_feature {
@@ -3326,7 +3333,7 @@ sub git_footer_html {
 		      qq!startBlame("!. href(action=>"blame_data", -replay=>1) .qq!",\n!.
 		      qq!           "!. href() .qq!");\n!.
 		      qq!</script>\n!;
-	} else {
+	} elsif (gitweb_check_feature('javascript-actions')) {
 		print qq!<script type="text/javascript">\n!.
 		      qq!window.onload = fixLinks;\n!.
 		      qq!</script>\n!;
-- 
1.6.5.3

Re: [RFC/PATCH] gitweb: Make linking to actions requiring JavaScript a feature

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> Let gitweb turn some links (like 'blame' links) into linking to
> actions which require JavaScript (like 'blame_incremental' action)
> only if 'javascript-actions' feature is enabled.

Hmm, instead of "fixLinks" that actually munges existing links to working
action with some other action, can that be "addLinks" that _adds_ new
links to features available only when JS can be used?

Re: [RFC/PATCH] gitweb: Make linking to actions requiring JavaScript a feature

[Jakub Narebski]

Dnia czwartek 26. listopada 2009 21:34, Junio C Hamano napisał:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
> > Let gitweb turn some links (like 'blame' links) into linking to
> > actions which require JavaScript (like 'blame_incremental' action)
> > only if 'javascript-actions' feature is enabled.
> 
> Hmm, instead of "fixLinks" that actually munges existing links to working
> action with some other action, can that be "addLinks" that _adds_ new
> links to features available only when JS can be used?

I am not sure if this would make sense.

Both 'blame' (running "git blame --porcelain") and 'blame_incremental'
(running "git blame --incremental") actions generate *the same* view,
modulo some very minor differences.  The idea behind 'blame_incremental'
is that it provides incremental updates to long-running action, and
hopefully also reduce server load, at least a bit.


It might be however good *interim* solution, so people would be able
to test 'blame_incremental' view without having to edit gitweb links.
Hmmm....

-- 
Jakub Narebski
Poland

Re: [RFC/PATCH] gitweb: Make linking to actions requiring JavaScript a feature

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> It might be however good *interim* solution, so people would be able
> to test 'blame_incremental' view without having to edit gitweb links.

Exactly.  I thought you were responding to my earlier "ship it as a new
feature with known breakage so that people can choose to enable to help
debugging and fixing".  If flipping on the new implementation makes an
alternative working implementation unavailable, that would be one reason
the site owners might consider _not_ enabling it.  By making them both
available, the result will have one less reason not to try for site
owners.

Re: [RFC/PATCH] gitweb: Make linking to actions requiring JavaScript a feature

[Jakub Narebski]

On Fri, 27 Nov 2009, Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
> > It might be however good *interim* solution, so people would be able
> > to test 'blame_incremental' view without having to edit gitweb links.
> 
> Exactly.  I thought you were responding to my earlier "ship it as a new
> feature with known breakage so that people can choose to enable to help
> debugging and fixing".  If flipping on the new implementation makes an
> alternative working implementation unavailable, that would be one reason
> the site owners might consider _not_ enabling it.  By making them both
> available, the result will have one less reason not to try for site
> owners.

Actually "addLinks" would be a bit harder, I guess, than current "fixLinks"
because "fixLinks" just adds ';js=1' to URL to denote that gitweb can use
JavaScript-requiring actions equivalents.  For "addLinks" selecting where
to add links would have to be in gitweb.js

I can "borrow" some code from Martin Koegler patch from April 2007
"[PATCH 5/7] gitweb: Prototyp for selecting diffs in JavaScript"
Message-Id: <1176669971921-git-send-email-mkoegler@auto.tuwien.ac.at>
$gmane/44517/focus=44523.  

Would turning

  "blame"

link ito pair of links

  "blame (incremental)"

be a good solution?  I'm trying to come up with good naming for extra link
to 'blame_incremental' action...

-- 
Jakub Narebski
Poland

Re: [RFC/PATCH] gitweb: Make linking to actions requiring JavaScript a feature

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> Would turning
>
>   "blame"
>
> link ito pair of links
>
>   "blame (incremental)"
>
> be a good solution?  I'm trying to come up with good naming for extra link
> to 'blame_incremental' action...

I had to step back a bit and ask myself what we are trying to achieve
here.  When the current blame and incremental one are both working
perfectly and well, will there be a reason for the end users to choose
between them when they click?

My answer is no.  If the incremental one gives nicer user experience in
all cases, it will be shown without the non-incremental one; if the
incremental one makes the server or network load too heavy, a site owner
may decide to show only the non-incremental one.

That makes my addLinks suggestion a change that would help _only_ while we
are working kinks out of the incremental one.

Let's not waste too much effort doing that.  Sorry for suggesting.

Letting the site owner choose if the site wants to set the "incremental if
possible" boolean would be more than adequate, I think.

Re: [RFC/PATCH] gitweb: Make linking to actions requiring JavaScript a feature

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I had to step back a bit and ask myself what we are trying to achieve
> here.  When the current blame and incremental one are both working
> perfectly and well, will there be a reason for the end users to choose
> between them when they click?
>
> My answer is no.  If the incremental one gives nicer user experience in
> all cases, it will be shown without the non-incremental one; if the
> incremental one makes the server or network load too heavy, a site owner
> may decide to show only the non-incremental one.
>
> That makes my addLinks suggestion a change that would help _only_ while we
> are working kinks out of the incremental one.
>
> Let's not waste too much effort doing that.  Sorry for suggesting.
>
> Letting the site owner choose if the site wants to set the "incremental if
> possible" boolean would be more than adequate, I think.

Sorry, but I guess I dropped the ball after this message.  If I understand
correctly, the conclusion is that I can apply the patch in this one

    From: Jakub Narebski <jnareb@gmail.com>
    Subject: [RFC/PATCH] gitweb: Make linking to actions requiring JavaScript a feature
    Date: Thu, 26 Nov 2009 21:12:15 +0100
    Message-ID: <200911262112.16280.jnareb@gmail.com>

and shipping 1.6.6 with it (perhaps setting 'default' to '[0]' instead)
would be both reasonably safe and allows easy experimentation by willing
site owners (or individual gitweb deployment), right?

Please advice if I am mistaken.

Thanks.

Re: [RFC/PATCH] gitweb: Make linking to actions requiring JavaScript a feature

[Jakub Narebski]

On Tue, 1 Dec 2009, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > I had to step back a bit and ask myself what we are trying to achieve
> > here.  When the current blame and incremental one are both working
> > perfectly and well, will there be a reason for the end users to choose
> > between them when they click?
> >
> > My answer is no.  If the incremental one gives nicer user experience in
> > all cases, it will be shown without the non-incremental one; if the
> > incremental one makes the server or network load too heavy, a site owner
> > may decide to show only the non-incremental one.
> >
> > That makes my addLinks suggestion a change that would help _only_ while we
> > are working kinks out of the incremental one.
> >
> > Let's not waste too much effort doing that.  Sorry for suggesting.
> >
> > Letting the site owner choose if the site wants to set the "incremental if
> > possible" boolean would be more than adequate, I think.
> 
> Sorry, but I guess I dropped the ball after this message.  If I understand
> correctly, the conclusion is that I can apply the patch in this one
> 
>     From: Jakub Narebski <jnareb@gmail.com>
>     Subject: [RFC/PATCH] gitweb: Make linking to actions requiring JavaScript a feature
>     Date: Thu, 26 Nov 2009 21:12:15 +0100
>     Message-ID: <200911262112.16280.jnareb@gmail.com>
> 
> and shipping 1.6.6 with it (perhaps setting 'default' to '[0]' instead)
> would be both reasonably safe and allows easy experimentation by willing
> site owners (or individual gitweb deployment), right?

Yes, I think it is right.

As a followup to this mail I have sent modified version of patch mentioned
above, only with default setting for 'javascript-actions' changed to '[0]'
(disabled), and new patch adding link to 'blame_incremental' action in an
ordinary 'blame' view and vice versa, so even if 'javascript-actions' is
turned off one can experiment with AJAX-y blame.

 gitweb/gitweb.perl |   21 +++++++++++++++++++--
 1 files changed, 19 insertions(+), 2 deletions(-)

-- 
Jakub Narebski
Poland

[PATCH 1/2] gitweb: Make linking to actions requiring JavaScript a feature

[Jakub Narebski]

Let gitweb turn some links (like 'blame' links) into linking to
actions which require JavaScript (like 'blame_incremental' action)
only if 'javascript-actions' feature is enabled.

This means that links to such actions would be present only if both
JavaScript is enabled, and 'javascript-actions' feature is enabled.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
 gitweb/gitweb.perl |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index a80cbd3..bb2d29c 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -432,6 +432,13 @@ our %feature = (
 	'timed' => {
 		'override' => 0,
 		'default' => [0]},
+
+	# Enable turning some links into links to actions which require
+	# JavaScript to run (like 'blame_incremental').  Disabled by default.
+	# Project specific override is currently not supported.
+	'javascript-actions' => {
+		'override' => 0,
+		'default' => [0]},
 );
 
 sub gitweb_get_feature {
@@ -3326,7 +3333,7 @@ sub git_footer_html {
 		      qq!startBlame("!. href(action=>"blame_data", -replay=>1) .qq!",\n!.
 		      qq!           "!. href() .qq!");\n!.
 		      qq!</script>\n!;
-	} else {
+	} elsif (gitweb_check_feature('javascript-actions')) {
 		print qq!<script type="text/javascript">\n!.
 		      qq!window.onload = fixLinks;\n!.
 		      qq!</script>\n!;
-- 
1.6.5.3

[PATCH 2/2] gitweb: Add link to other blame implementation in blame views

[Jakub Narebski]

Add link to 'blame_incremental' action (which requires JavaScript) in
'blame' view, and add link to 'blame' action in 'blame_incremental'
view.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
 gitweb/gitweb.perl |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index bb2d29c..ca36761 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -5006,6 +5006,17 @@ sub git_blame_common {
 	my $formats_nav =
 		$cgi->a({-href => href(action=>"blob", -replay=>1)},
 		        "blob") .
+		" | ";
+	if ($format eq 'incremental') {
+		$formats_nav .=
+			$cgi->a({-href => href(action=>"blame", javascript=>0, -replay=>1)},
+			        "blame") . " (non-incremental)";
+	} else {
+		$formats_nav .=
+			$cgi->a({-href => href(action=>"blame_incremental", -replay=>1)},
+			        "blame") . " (incremental)";
+	}
+	$formats_nav .=
 		" | " .
 		$cgi->a({-href => href(action=>"history", -replay=>1)},
 		        "history") .
-- 
1.6.5.3

Re: [PATCH] gitweb.js: Harden setting blamed commit info in incremental blame

[Stephen Boyd]

On Wed, 2009-11-25 at 15:36 +0100, Jakub Narebski wrote:
> Well, the one time I was able to run debugger (F12, select 'Script', select
> 'gitweb.js') with error occurring and without IE hanging (for README file)
> it did show an error for the following line:
> 
>   if (xhr.readyState === 3 && xhr.status !== 200) {
> 
> When I checked 'xhr' object, it has "Unknown error" as contents of 
> xhr.statusText field and as contents of xhr.status (sic!), which should
> be a number: HTTP status code.
> 
> Unfortunately I had to take a break... and I was not able to reproduce this
> (without hanging web browser: "program not responding") since then...
> 

Ok. It's December and I've had some more time to look into this.
Initializing 'xhr' to null seems to get rid of the "Unknown error"
problem (see patch below).

The responsiveness on IE8 is pretty poor. I load the page and then after
some waiting (usually 20-30 seconds including IE becoming a "Not
Responding" program) the whole page will load. There is nothing
incremental about it. I'm trying to figure out why it's slow now.

--->8----

Subject: [PATCH] gitweb.js: workaround IE8 "Unknown error"

Internet Explorer 8 complains about an "Unknown error on line 782, char 2".
That line is a reference to xhr and the status code. By initializing xhr
to null this error goes away.

Signed-off-by: Stephen Boyd <bebarino@gmail.com>
---
 gitweb/gitweb.js |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.js b/gitweb/gitweb.js
index 2a25b7c..b936635 100644
--- a/gitweb/gitweb.js
+++ b/gitweb/gitweb.js
@@ -126,7 +126,7 @@ function createRequestObject() {
 /* ============================================================ */
 /* utility/helper functions (and variables) */

-var xhr;        // XMLHttpRequest object
+var xhr = null;        // XMLHttpRequest object
 var projectUrl; // partial query + separator ('?' or ';')

 // 'commits' is an associative map. It maps SHA1s to Commit objects.
-- 
1.6.6.rc1.39.g9a42

Re: [PATCH] gitweb.js: Harden setting blamed commit info in incremental blame

[Stephen Boyd]

On Sun, 2009-12-06 at 17:04 -0800, Stephen Boyd wrote:
> 
> Ok. It's December and I've had some more time to look into this.
> Initializing 'xhr' to null seems to get rid of the "Unknown error"
> problem (see patch below).
> 

Ah sorry. Seems this doesn't work and I was just getting lucky.

PATCH/RFC] gitweb.js: Workaround for IE8 bug

[Jakub Narebski]

On Mon, 7 Dec 2009, Stephen Boyd wrote:
> On Sun, 2009-12-06 at 17:04 -0800, Stephen Boyd wrote:
> > 
> > Ok. It's December and I've had some more time to look into this.
> > Initializing 'xhr' to null seems to get rid of the "Unknown error"
> > problem (see patch below).
> > 
> 
> Ah sorry. Seems this doesn't work and I was just getting lucky.

Does the following fixes the issue for IE8 for you (it works for me)?

As for testing, try with blaming 'revision.c' and 'gitweb/gitweb.perl'
using 'blame_incremental' view.


With probably too long commit message, and not detailed enough title:

-- >8 --
Subject: [PATCH] gitweb.js: Workaround for IE8 bug

In Internet Explorer 8 (IE8) the 'blame_incremental' view, which uses
JavaScript to generate blame info using AJAX, sometimes hang at the
beginning (at 0%) of blaming, e.g. for larger files with long history
like git's own gitweb/gitweb.perl.

The error shown by JavaScript console is "Unspecified error" at char:2
of the following line in gitweb/gitweb.js:

  if (xhr.readyState === 3 && xhr.status !== 200) {

Debugging it using IE8 JScript debuger shown that the error occurs
when trying to access xhr.status (xhr is XMLHttpRequest object).
Watch for xhr object shows 'Unspecified error.' as "value" of
xhr.status, and trying to access xhr.status from console throws error.

This bug is some intermittent bug, depending on XMLHttpRequest timing,
as it doesn't occur in all cases.  It is probably caused by the fact
that handleResponse is called from timer (pollTimer), to work around
the fact that some browsers call onreadystatechange handler only once
for each state change, and not like required for 'blame_incremental'
as soon as new data is available from server.  It looks like xhr
object is not properly initialized; still it is a bug to throw an
error when accessing xhr.status (and not use 'null' or 'undefined' as
value).

Work around this bug in IE8 by using try-catch block when accessing
xhr.status.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
 gitweb/gitweb.js |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.js b/gitweb/gitweb.js
index 2a25b7c..9c66928 100644
--- a/gitweb/gitweb.js
+++ b/gitweb/gitweb.js
@@ -779,7 +779,12 @@ function handleResponse() {
 	}
 
 	// the server returned error
-	if (xhr.readyState === 3 && xhr.status !== 200) {
+	// try ... catch block is to work around bug in IE8
+	try {
+		if (xhr.readyState === 3 && xhr.status !== 200) {
+			return;
+		}
+	} catch (e) {
 		return;
 	}
 	if (xhr.readyState === 4 && xhr.status !== 200) {
-- 
1.6.5.3

Re: PATCH/RFC] gitweb.js: Workaround for IE8 bug

[Stephen Boyd]

On Tue, 2009-12-08 at 17:29 +0100, Jakub Narebski wrote: 
> 
> Does the following fixes the issue for IE8 for you (it works for me)?
> 

Yes, there is no longer an error but IE8 still locks up and takes about
30 seconds. It doesn't seem to be any more responsive. Isn't putting the
error in a try-catch just papering over the issue?

Re: PATCH/RFC] gitweb.js: Workaround for IE8 bug

[Jakub Narebski]

On Tue, 8 Dec 2009, Stephen Boyd wrote:
> On Tue, 2009-12-08 at 17:29 +0100, Jakub Narebski wrote: 
> > 
> > Does the following fixes the issue for IE8 for you (it works for me)?
> > 
> 
> Yes, there is no longer an error but IE8 still locks up and takes about
> 30 seconds. It doesn't seem to be any more responsive. Isn't putting the
> error in a try-catch just papering over the issue?

Well, I wrote it is *workaround* for IE8 bug, didn't I?

There are actually two separate issues.  First is 'blame_incremental'
freezing browser (making it non responsive), second is proper solution
to this bug.

The problem with 'blame_incremental' freezing is that JavaScript is
single-threaded, and that modifying DOM is not lightweight.  gitweb.js
should then use technique described in
  http://www.nczonline.net/blog/2009/08/11/timed-array-processing-in-javascript/
to avoid freezing browser, and perhaps also some technique to avoid
reflowing (if possible).

The proper solution for IE8 bug would be to use 'progress', 'error'
and 'load' events of XHR 2.0 (XMLHttpRequest specification level 2)
if they are available, instead of mucking with underspecified 
'readystatechange' event from XHR 1.0 and timer.  But it is a more
complicated solution.

-- 
Jakub Narebski
Poland

Re: PATCH/RFC] gitweb.js: Workaround for IE8 bug

[Jakub Narebski]

Dnia wtorek 8. grudnia 2009 22:56, Stephen Boyd napisał:
> On Tue, 2009-12-08 at 17:29 +0100, Jakub Narebski wrote: 
> > 
> > Does the following fixes the issue for IE8 for you (it works for me)?
> > 
> 
> Yes, there is no longer an error but IE8 still locks up and takes about
> 30 seconds. It doesn't seem to be any more responsive. Isn't putting the
> error in a try-catch just papering over the issue?

Does increasing interval in setInterval call at the end of startBlame
finction in gitweb.js from 1000 (1 second) to e.g. 2000 (2 seconds)
help there?

-- 
Jakub Narebski
Poland

Re: PATCH/RFC] gitweb.js: Workaround for IE8 bug

[Stephen Boyd]

On Tue, 2009-12-08 at 23:32 +0100, Jakub Narebski wrote:
> Dnia wtorek 8. grudnia 2009 22:56, Stephen Boyd napisał:
> > Yes, there is no longer an error but IE8 still locks up and takes about
> > 30 seconds. It doesn't seem to be any more responsive. Isn't putting the
> > error in a try-catch just papering over the issue?
> 
> Does increasing interval in setInterval call at the end of startBlame
> finction in gitweb.js from 1000 (1 second) to e.g. 2000 (2 seconds)
> help there?
> 

I tried numbers from 100 to 10000 and didn't see a difference. Files
with only a few hundred lines or less don't lock up though. I've been
using builtin-apply.c for testing.