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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 26, 2011, 23:06 UTC
Message-ID: <7vaa9q29ag.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1317060642-25488-1-git-send-email-peter@stuge.se>

Peter Stuge <peter@stuge.se> writes:

Show 13 quoted lines

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