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

[Junio C Hamano <gitster@pobox.com>]

Peter Stuge <peter@stuge.se> writes:

> -var jsExceptionsRe = /[;?]js=[01]$/;
> +var jsExceptionsRe = /[;?]js=[01](#.*)?$/;
>  
>  /**
>   * Add '?js=1' or ';js=1' to the end of every link in the document
> @@ -33,9 +33,9 @@ function fixLinks() {
>  	var allLinks = document.getElementsByTagName("a") || document.links;
>  	for (var i = 0, len = allLinks.length; i < len; i++) {
>  		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).

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.