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

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

[Peter Stuge]

Signed-off-by: Peter Stuge <peter@stuge.se>
---
 gitweb/static/js/javascript-detection.js |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/gitweb/static/js/javascript-detection.js b/gitweb/static/js/javascript-detection.js
index 93dd2bd..003acd1 100644
--- a/gitweb/static/js/javascript-detection.js
+++ b/gitweb/static/js/javascript-detection.js
@@ -16,7 +16,7 @@
  * and other reasons to not add 'js=1' param at the end of link
  * @constant
  */
-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](#.*)?$/;
+			link.href = link.href.replace(/(#|$)/,
+				(link.href.indexOf('?') === -1 ? '?' : ';') + 'js=1$1');
 		}
 	}
 }
-- 
1.7.4.1.343.ga91df.dirty

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

[Junio C Hamano]

Peter Stuge <peter@stuge.se> writes:

> Signed-off-by: Peter Stuge <peter@stuge.se>
> ---

Care to elaborate a bit more please?  Explanation of what you are fixing
is totally lacking.  What happens with the current code, why it is wrong,
and how the updated pattern improves the result in what way?

>  gitweb/static/js/javascript-detection.js |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/gitweb/static/js/javascript-detection.js b/gitweb/static/js/javascript-detection.js
> index 93dd2bd..003acd1 100644
> --- a/gitweb/static/js/javascript-detection.js
> +++ b/gitweb/static/js/javascript-detection.js
> @@ -16,7 +16,7 @@
>   * and other reasons to not add 'js=1' param at the end of link
>   * @constant
>   */
> -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](#.*)?$/;
> +			link.href = link.href.replace(/(#|$)/,
> +				(link.href.indexOf('?') === -1 ? '?' : ';') + 'js=1$1');
>  		}
>  	}
>  }

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

[Peter Stuge]

Junio C Hamano wrote:
> > Signed-off-by: Peter Stuge <peter@stuge.se>
> 
> Care to elaborate a bit more please?

Okey. I thought subject together with change would be clear enough. :)


> Explanation of what you are fixing is totally lacking.

The subject sums it up, if briefly.


> What happens with the current code, why it is wrong, and how the
> updated pattern improves the result in what way?

Current code generates links to line numbers like ../file.c#l1234;js=1

It is wrong because a URI fragment always goes at the end.

The updated pattern improves this by treating # like end of string,
to detect js=1 also before # and not only at end of string.

The updated code improves this by injecting [?;]js=1 before # if
one exists, or at end of string (like before) otherwise.


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

To test how this works you can try it on

http://git.libusb.org/?p=libusb.git;a=blob;f=COPYING

where the change is in production. Compare the line number links with
those generated by another gitweb with javascript-actions enabled.


//Peter

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

[Junio C Hamano]

Peter Stuge <peter@stuge.se> writes:

> Okey. I thought subject together with change would be clear enough. :)
>
>
>> Explanation of what you are fixing is totally lacking.
>
> The subject sums it up, if briefly.

... Sorry, that is not what I meant.

You don't have to explain these to *me* specifically as a response to this
thread. What I meant was that your patch should have these necessary
descriptions in its proposed commit log message.

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

[Peter Stuge]

Junio C Hamano wrote:
> > I thought subject together with change would be clear enough. :)
> >
> >
> >> Explanation of what you are fixing is totally lacking.
> >
> > The subject sums it up, if briefly.
> 
> ... Sorry, that is not what I meant.
> 
> You don't have to explain these to *me* specifically as a response
> to this thread. What I meant was that your patch should have these
> necessary descriptions in its proposed commit log message.

IMO not so neccessary if one knows a little web and javascript, which
is probably likely for a gitweb change..

It's a simple fix of links broken by manual URI manipulation that
didn't consider fragments. Is the subject description really not
enough?


//Peter

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

[Johannes Sixt]

Am 9/27/2011 0:28, schrieb Peter Stuge:
> Junio C Hamano wrote:
>> You don't have to explain these to *me* specifically as a response
>> to this thread. What I meant was that your patch should have these
>> necessary descriptions in its proposed commit log message.
> 
> IMO not so neccessary if one knows a little web and javascript, which
> is probably likely for a gitweb change..
> 
> It's a simple fix of links broken by manual URI manipulation that
> didn't consider fragments. Is the subject description really not
> enough?

No, it is not. The target audience of a commit message are people like I
am. I do know a bit of Perl, and a bit of Javascript; I know how an URL is
structured; I would find my way through the gitweb code if the need
arises. But I am not an expert in any of these areas.

The subject alone is not sufficient because I do not know for sure what an
"URI fragment" is or what role line numbers in gitweb's links play. The
explanations and examples you gave in a later email were very
enlightening, and they would be very helpful if *I* am forced to hack
gitweb, and if I need to understand why this particular change was good.

Finding the right balance between verbosity and terseness needs practice,
but to write *no* justification is practically always wrong.

-- Hannes

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

[Peter Stuge]

Hey!

Johannes Sixt wrote:
> > It's a simple fix of links broken by manual URI manipulation that
> > didn't consider fragments. Is the subject description really not
> > enough?
> 
> No, it is not. The target audience of a commit message are people like I
> am. I do know a bit of Perl, and a bit of Javascript; I know how an URL
> is structured; I would find my way through the gitweb code if the need
> arises. But I am not an expert in any of these areas.
> 
> The subject alone is not sufficient because I do not know for sure what
> an "URI fragment" is or what role line numbers in gitweb's links play.

I shall continue playing advocatus diaboli only a little longer.


> The explanations and examples you gave in a later email were very
> enlightening, and they would be very helpful if *I* am forced to hack
> gitweb, and if I need to understand why this particular change was good.

On the other hand you're just one quick google search on uri fragment
away from the same enlightenment, and relying on terminology saves on
unneccessary redundance.

Lorelei: That's repetitive
Rory: ..and redundant.
Lorelei: That's repetitive
Rory: ..and redundant.

(SCNR the pop culture reference! :)


> Finding the right balance between verbosity and terseness needs
> practice,

I disagree, but I agree with you if we qualify that a little. The
right balance is a matter of subjective review, so the only way it
can be practiced with relevance is by actually working with the same
reviewers for a while, to learn what they consider right.

It can absolutely not be practiced out of context, ie. with different
peers. No later than the day before I sent this patch I wrote a
welcome mail in another open source project, to a new contributor,
where one bit was about commit messages.

http://marc.info/?l=openocd-development&m=131698532523018

"* Write a top quality commit message, technically and logically

...
As for the logical quality, it is important to write the first line
description of the change so that it makes sense for someone who
knows nothing at all about the code, since this is used in list
views, and since the background for this code and for why this change
was done the way it was done comes only in the later lines, which may
not be available from where that list view is. ... Keep it
high level, clear and simple. Writing this one line is not
neccessarily easy."

I of course also try to practise exactly this, but it's difficult to
know what reviewers expect to be fed, or how much verbosity they
prefer. :) I tend to prefer as much useful information as possible in
the first line, while keeping it ideally <60 chars. Many times I find
it to be enough.


> but to write *no* justification is practically always wrong.

I disagree strongly that I wrote no justification. I agree that it
was not verbose. I'm sorry that this is a problem. I'm personally
allergic to redundancy such as the commit message Jakub wrote, I
think it's not only reasonable but also desirable to avoid that.
Maybe gitweb is a special case in git.git though, I don't know, but
I'm a little surprised. :)

Anyway, I'm more than happy to write a more verbose message for you!


//Peter

[PATCH] gitweb: Fix links to lines in blobs when javascript-actions are enabled

[Peter Stuge]

Some javascript code will run in the browser onLoad and signals back to
gitweb that the browser can actually do javascript.

The code adds [?;]js=1 into the URL of all links on the page. The code
always added [?;]js=1 to the end of links, which is wrong when links
contain a URI fragment, such as links directly to a line in a blob:
..?p=repo.git;a=blob;f=file#l123

In this case, [?;]js=1 must be added before the hashmark.

Signed-off-by: Peter Stuge <peter@stuge.se>
---
 gitweb/static/js/javascript-detection.js |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/gitweb/static/js/javascript-detection.js b/gitweb/static/js/javascript-detection.js
index 93dd2bd..fa2596f 100644
--- a/gitweb/static/js/javascript-detection.js
+++ b/gitweb/static/js/javascript-detection.js
@@ -16,7 +16,7 @@
  * and other reasons to not add 'js=1' param at the end of link
  * @constant
  */
-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)) {
+			link.href = link.href.replace(/(#|$)/,
+				(link.href.indexOf('?') === -1 ? '?' : ';') + 'js=1$1');
 		}
 	}
 }
-- 
1.7.4.1.343.ga91df.dirty

Re: [PATCH] gitweb: Fix links to lines in blobs when javascript-actions are enabled

[Junio C Hamano]

Peter Stuge <peter@stuge.se> writes:

> Some javascript code will run in the browser onLoad and signals back to
> gitweb that the browser can actually do javascript.
>
> The code adds [?;]js=1 into the URL of all links on the page. The code
> always added [?;]js=1 to the end of links, which is wrong when links
> contain a URI fragment, such as links directly to a line in a blob:
> ..?p=repo.git;a=blob;f=file#l123
>
> In this case, [?;]js=1 must be added before the hashmark.
>
> Signed-off-by: Peter Stuge <peter@stuge.se>

Nicely done. You forgot to mention another bug you fixed (see Jakub's
sample in the thread), so I'll amend in a few sentences to describe it as
well.

Thanks.

Re: [PATCH] gitweb: Fix links to lines in blobs when javascript-actions are enabled

[Peter Stuge]

Junio C Hamano wrote:
> Nicely done.

Thanks.

> You forgot to mention another bug you fixed

Well.. I'm not sure that it is worth mentioning, since it may never
actually have been triggered by the way fixLinks() works.


> I'll amend in a few sentences to describe it as well.

Go for it.


//Peter

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

[Junio C Hamano]

Peter Stuge <peter@stuge.se> writes:

> I disagree, but I agree with you if we qualify that a little. The
> right balance is a matter of subjective review, so the only way it
> can be practiced with relevance is by actually working with the same
> reviewers for a while, to learn what they consider right.
>
> It can absolutely not be practiced out of context,...

You are right that you need to practice in the context of working in the
Git project to explain your change with the right amount of details when
preparing a change for the Git project, and the same goes for the openocd
project.

We aim to write our commit log messages primarily for future developers
who want to read "git log -p" output and understand why these changes had
to be made, to help them avoid intentionally breaking what the old commit
wanted to achieve when they want to modify the existing code 6 months down
the road. Your reviewers aim to make sure that your log message gives
sufficient information to such future developers, as opposed to themselves
while reviewing the patch and the issue is still fresh in their head.

I suspect that the target audience of log message may even be different
depending on the project, and openocd may not work that way, and that is
perfectly fine.

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

[Junio C Hamano]

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.

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

[Peter Stuge]

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

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

[Jakub Narebski]

Peter Stuge <peter@stuge.se> writes:

It really needs a proper commit message.  Perhaps something like this:

  The fixLinks() function in javascript-detection.js is supposed to
  add 'js' query parameter with a value of 1 to each link that does
  not have 'js' query parameter already.

  However it didn't take into account the fact that URI can have
  'fragment' part.  It meant that:

  1. URIs with fragment and 'js' query parameter, like e.g.

        ...foo?js=0#l199

     were not recognized as having 'js' query parameter already.
     
  2. The 'js' query parameter, in the form of either '?js=1' or ';js=1'
     was appended at the end of URI, even if it included a fragment
     (had a hash part).  This lead to the incorrect links like this

        ...foo#l199?js=1

     instead of adding query parameter as last part of query, but
     before the fragment part, i.e.

        ...foo?js=1#l199

> Signed-off-by: Peter Stuge <peter@stuge.se>

For what it is worth it

Acked-by: Jakub Narebski <jnareb@gmail.com>

> ---
>  gitweb/static/js/javascript-detection.js |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/gitweb/static/js/javascript-detection.js b/gitweb/static/js/javascript-detection.js
> index 93dd2bd..003acd1 100644
> --- a/gitweb/static/js/javascript-detection.js
> +++ b/gitweb/static/js/javascript-detection.js
> @@ -16,7 +16,7 @@
>   * and other reasons to not add 'js=1' param at the end of link
>   * @constant
>   */
> -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](#.*)?$/;
> +			link.href = link.href.replace(/(#|$)/,
> +				(link.href.indexOf('?') === -1 ? '?' : ';') + 'js=1$1');
>  		}
>  	}
>  }
> -- 
> 1.7.4.1.343.ga91df.dirty
> 

-- 
Jakub Narębski                      mailto:jnareb@fuw.edu.pl
 ZTHiL IFT UW                       http://info.fuw.edu.pl/~jnareb/