Junio C Hamano wrote:
> > 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...
>
> > +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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html