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 ;-)

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(-)

Currently handleLine() assumes that a commit author name will always start with a capital letter. It's possible that the author name is [email protected] 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.

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

index 22570f5..02454d8 100644
@@ -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

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

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?

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

Jakub Narebski
Poland

2009/11/19 Jakub Narebski <[email protected]>:

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

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.

---
 gitweb/gitweb.js |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

index 02454d8..30597dd 100644
@@ -31,12 +31,12 @@
 
 /**
  * pad number N with nonbreakable spaces on the left, to WIDTH characters
- * example: padLeftStr(12, 3, ' ') == ' 12'
- *          (' ' 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. ' '
+ * @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, ' ') + '%)';
+			' (' + padLeftStr(percentage, 3, '\u00A0') + '%)';
 	}
 
 	if (div_progress_bar) {

1.6.5.3.433.g11067

Errr... why are you avoiding writing &nbsp; or "&nbsp;" here?

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

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.

Jakub Narebski
Poland

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

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.

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.

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.

---

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

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

index 02454d8..9214497 100644
@@ -31,19 +31,19 @@
 
 /**
  * pad number N with nonbreakable spaces on the left, to WIDTH characters
- * example: padLeftStr(12, 3, ' ') == ' 12'
- *          (' ' 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. ' '
+ * @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, ' ') + '%)';
+			' (' + padLeftStr(percentage, 3, '\u00A0') + '%)';
 	}
 
 	if (div_progress_bar) {

1.6.6.rc0

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

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 &nbsp; 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 &nbsp; 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

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

-- 8< --

index 200ec5a..c1e425c 100644
@@ -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 --

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

Jakub Narebski
Poland

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.

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

index 200ec5a..c1e425c 100644
@@ -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

Quoting Jakub Narebski <[email protected]>

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

Quoting Stephen Boyd <[email protected]>

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

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

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

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

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

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.

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.

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

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.

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.

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

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.

---
 gitweb/gitweb.perl |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

index a80cbd3..0ab47e1 100755
@@ -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

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

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: <[email protected]> $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

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 <[email protected]>
Subject: [RFC/PATCH] gitweb: Make linking to actions requiring JavaScript a feature
Date: Thu, 26 Nov 2009 21:12:15 +0100
Message-ID: <[email protected]>

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.

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

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.

---
 gitweb/gitweb.perl |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

index a80cbd3..bb2d29c 100755
@@ -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

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

---
 gitweb/gitweb.perl |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

index bb2d29c..ca36761 100755
@@ -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

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.

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

index 2a25b7c..b936635 100644
@@ -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

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.

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

index 2a25b7c..9c66928 100644
@@ -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

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

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

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.

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

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