patchgitweb: Add js=1 before an URI fragment to fix line number links

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

Peter Stuge <peter@stuge.se> writes:

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?

Junio C Hamano wrote:

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

The subject sums it up, if briefly.

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.

..

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

Peter Stuge <peter@stuge.se> writes:

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

Junio C Hamano wrote:

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

Am 9/27/2011 0:28, schrieb Peter Stuge:

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

Hey!

Johannes Sixt wrote:

I shall continue playing advocatus diaboli only a little longer.

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! :)

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.

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

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(-)

Peter Stuge <peter@stuge.se> writes:

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.

Junio C Hamano wrote:

Thanks.

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

Go for it.

//Peter

Peter Stuge <peter@stuge.se> writes:

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.

Peter Stuge <peter@stuge.se> writes:

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

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

Junio C Hamano wrote:

I agree. I chose to follow style.

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.

Thanks, I figured if it was OK before then at least it would be better now.

//Peter

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

For what it is worth it

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