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

From: Peter Stuge <peter@stuge.se>
Date: Sep 26, 2011, 23:17 UTC
Message-ID: <20110926231703.21696.qmail@stuge.se>
In-Reply-To: <7vaa9q29ag.fsf@alter.siamese.dyndns.org>

Junio C Hamano wrote:

Show 8 quoted lines

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

Show 12 quoted lines

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

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