Re: [PATCH] gitweb: Add js=1 before an URI fragment to fix line number links

Peter Stuge [off-list ref] writes:

Let's not repeat the regexp in the comment (badness you inherited from the
original).

Regarding the "we already have the js=0 or js=1 in the URL" check here...

+var jsExceptionsRe = /[;?]js=[01](#.*)?$/;

... I am wondering who guarantees that this js=[01] is the last parameter
before the fragment identifier. The answer obviously is "the way the
current code is written using replace() method on link.href", but that is
somewhat disturbing, because it is not clear what should happen, short of
total rewrite of the code around this, when somebody needs to include
another variable, say xx=[01], just like the js=[01] you are fixing here,
in the resulting URL. In other words, this fixLinks() logic does not seem
to scale and also looks brittle.

The patch itself looks correct as a short-term fix, though.

Thanks.

Junio C Hamano wrote:

quoted

 		var link = allLinks[i];
-		if (!jsExceptionsRe.test(link)) { // =~ /[;?]js=[01]$/;
-			link.href +=
-				(link.href.indexOf('?') === -1 ? '?' : ';') + 'js=1';
+		if (!jsExceptionsRe.test(link)) { // =~ /[;?]js=[01](#.*)?$/;

Let's not repeat the regexp in the comment (badness you inherited
from the original).

I agree. I chose to follow style.

Regarding the "we already have the js=0 or js=1 in the URL" check here...

quoted

+var jsExceptionsRe = /[;?]js=[01](#.*)?$/;

... I am wondering who guarantees that this js=[01] is the last parameter
before the fragment identifier. The answer obviously is "the way the
current code is written using replace() method on link.href", but that is
somewhat disturbing, because it is not clear what should happen, short of
total rewrite of the code around this, when somebody needs to include
another variable, say xx=[01], just like the js=[01] you are fixing here,
in the resulting URL. In other words, this fixLinks() logic does not seem
to scale and also looks brittle.

At first glance indeed. But I think it might be fine, because the
purpose of this function is only to use javascript in order to
indicate to the server that the client supports javascript. This is
somewhat an odd case, the link rewriting is pretty much needed only
for this one value so it doesn't really have to scale. But I have
nothing against say removing (#.*)?$ and thus only matching
[?;]js=[01] in link.href.

The patch itself looks correct as a short-term fix, though.

Thanks, I figured if it was OK before then at least it would be
better now.


//Peter