{"thread":{"id":"28501","subject":"[PATCH] gitweb: Add js=1 before an URI fragment to fix line number links","startedAt":"2011-09-26T18:10:42Z","lastAt":"2011-09-28T01:31:01Z","messageCount":14,"participants":["Peter Stuge","Junio C Hamano","Jakub Narebski","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"176241","messageId":"1317060642-25488-1-git-send-email-peter@stuge.se","threadId":"28501","inReplyTo":null,"subject":"[PATCH] gitweb: Add js=1 before an URI fragment to fix line number links","fromName":"Peter Stuge","fromEmail":"peter@stuge.se","sentAt":"2011-09-26T18:10:42Z","receivedAt":"2011-09-26T18:10:42Z","isPatch":true,"sender":{"key":"peter@stuge.se","avatar":null},"body":"Signed-off-by: Peter Stuge \n---\n gitweb/static/js/javascript-detection.js | 8 ++++----\n 1 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/gitweb/static/js/javascript-detection.js b/gitweb/static/js/javascript-detection.js\nindex 93dd2bd..003acd1 100644\n--- a/gitweb/static/js/javascript-detection.js\n+++ b/gitweb/static/js/javascript-detection.js\n@@ -16,7 +16,7 @@\n * and other reasons to not add 'js=1' param at the end of link\n * @constant\n */\n-var jsExceptionsRe = /[;?]js=[01]$/;\n+var jsExceptionsRe = /[;?]js=[01](#.*)?$/;\n \n /**\n * Add '?js=1' or ';js=1' to the end of every link in the document\n@@ -33,9 +33,9 @@ function fixLinks() {\n \tvar allLinks = document.getElementsByTagName(\"a\") || document.links;\n \tfor (var i = 0, len = allLinks.length; i < len; i++) {\n \t\tvar link = allLinks[i];\n-\t\tif (!jsExceptionsRe.test(link)) { // =~ /[;?]js=[01]$/;\n-\t\t\tlink.href +=\n-\t\t\t\t(link.href.indexOf('?') === -1 ? '?' : ';') + 'js=1';\n+\t\tif (!jsExceptionsRe.test(link)) { // =~ /[;?]js=[01](#.*)?$/;\n+\t\t\tlink.href = link.href.replace(/(#|$)/,\n+\t\t\t\t(link.href.indexOf('?') === -1 ? '?' : ';') + 'js=1$1');\n \t\t}\n \t}\n }\n-- \n1.7.4.1.343.ga91df.dirty\n"},{"id":"176249","messageId":"7v62kf2jf4.fsf@alter.siamese.dyndns.org","threadId":"28501","inReplyTo":"1317060642-25488-1-git-send-email-peter@stuge.se","subject":"Re: [PATCH] gitweb: Add js=1 before an URI fragment to fix line number links","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-26T19:27:43Z","receivedAt":"2011-09-26T19:27:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Peter Stuge writes:\n\n> Signed-off-by: Peter Stuge \n> ---\n\nCare to elaborate a bit more please? Explanation of what you are fixing\nis totally lacking. What happens with the current code, why it is wrong,\nand how the updated pattern improves the result in what way?\n\n> gitweb/static/js/javascript-detection.js | 8 ++++----\n> 1 files changed, 4 insertions(+), 4 deletions(-)\n>\n> diff --git a/gitweb/static/js/javascript-detection.js b/gitweb/static/js/javascript-detection.js\n> index 93dd2bd..003acd1 100644\n> --- a/gitweb/static/js/javascript-detection.js\n> +++ b/gitweb/static/js/javascript-detection.js\n> @@ -16,7 +16,7 @@\n> * and other reasons to not add 'js=1' param at the end of link\n> * @constant\n> */\n> -var jsExceptionsRe = /[;?]js=[01]$/;\n> +var jsExceptionsRe = /[;?]js=[01](#.*)?$/;\n> \n> /**\n> * Add '?js=1' or ';js=1' to the end of every link in the document\n> @@ -33,9 +33,9 @@ function fixLinks() {\n> \tvar allLinks = document.getElementsByTagName(\"a\") || document.links;\n> \tfor (var i = 0, len = allLinks.length; i < len; i++) {\n> \t\tvar link = allLinks[i];\n> -\t\tif (!jsExceptionsRe.test(link)) { // =~ /[;?]js=[01]$/;\n> -\t\t\tlink.href +=\n> -\t\t\t\t(link.href.indexOf('?') === -1 ? '?' : ';') + 'js=1';\n> +\t\tif (!jsExceptionsRe.test(link)) { // =~ /[;?]js=[01](#.*)?$/;\n> +\t\t\tlink.href = link.href.replace(/(#|$)/,\n> +\t\t\t\t(link.href.indexOf('?') === -1 ? '?' : ';') + 'js=1$1');\n> \t\t}\n> \t}\n> }\n"},{"id":"176252","messageId":"20110926194639.25339.qmail@stuge.se","threadId":"28501","inReplyTo":"7v62kf2jf4.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] gitweb: Add js=1 before an URI fragment to fix line number links","fromName":"Peter Stuge","fromEmail":"peter@stuge.se","sentAt":"2011-09-26T19:46:39Z","receivedAt":"2011-09-26T19:46:39Z","isPatch":true,"sender":{"key":"peter@stuge.se","avatar":null},"body":"Junio C Hamano wrote:\n> > Signed-off-by: Peter Stuge \n> \n> Care to elaborate a bit more please?\n\nOkey. I thought subject together with change would be clear enough. :)\n\n\n> Explanation of what you are fixing is totally lacking.\n\nThe subject sums it up, if briefly.\n\n\n> What happens with the current code, why it is wrong, and how the\n> updated pattern improves the result in what way?\n\nCurrent code generates links to line numbers like ../file.c#l1234;js=1\n\nIt is wrong because a URI fragment always goes at the end.\n\nThe updated pattern improves this by treating # like end of string,\nto detect js=1 also before # and not only at end of string.\n\nThe updated code improves this by injecting [?;]js=1 before # if\none exists, or at end of string (like before) otherwise.\n\n\n> > -var jsExceptionsRe = /[;?]js=[01]$/;\n> > +var jsExceptionsRe = /[;?]js=[01](#.*)?$/;\n..\n> > -\t\tif (!jsExceptionsRe.test(link)) { // =~ /[;?]js=[01]$/;\n> > -\t\t\tlink.href +=\n> > -\t\t\t\t(link.href.indexOf('?') === -1 ? '?' : ';') + 'js=1';\n> > +\t\tif (!jsExceptionsRe.test(link)) { // =~ /[;?]js=[01](#.*)?$/;\n> > +\t\t\tlink.href = link.href.replace(/(#|$)/,\n> > +\t\t\t\t(link.href.indexOf('?') === -1 ? '?' : ';') + 'js=1$1');\n\nTo test how this works you can try it on\n\nhttp://git.libusb.org/?p=libusb.git;a=blob;f=COPYING\n\nwhere the change is in production. Compare the line number links with\nthose generated by another gitweb with javascript-actions enabled.\n\n\n//Peter\n"},{"id":"176260","messageId":"7vipof0zx0.fsf@alter.siamese.dyndns.org","threadId":"28501","inReplyTo":"20110926194639.25339.qmail@stuge.se","subject":"Re: [PATCH] gitweb: Add js=1 before an URI fragment to fix line number links","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-26T21:14:19Z","receivedAt":"2011-09-26T21:14:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Peter Stuge writes:\n\n> Okey. I thought subject together with change would be clear enough. :)\n>\n>\n>> Explanation of what you are fixing is totally lacking.\n>\n> The subject sums it up, if briefly.\n\n... Sorry, that is not what I meant.\n\nYou don't have to explain these to *me* specifically as a response to this\nthread. What I meant was that your patch should have these necessary\ndescriptions in its proposed commit log message.\n"},{"id":"176271","messageId":"20110926222801.14985.qmail@stuge.se","threadId":"28501","inReplyTo":"7vipof0zx0.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] gitweb: Add js=1 before an URI fragment to fix line number links","fromName":"Peter Stuge","fromEmail":"peter@stuge.se","sentAt":"2011-09-26T22:28:01Z","receivedAt":"2011-09-26T22:28:01Z","isPatch":true,"sender":{"key":"peter@stuge.se","avatar":null},"body":"Junio C Hamano wrote:\n> > I thought subject together with change would be clear enough. :)\n> >\n> >\n> >> Explanation of what you are fixing is totally lacking.\n> >\n> > The subject sums it up, if briefly.\n> \n> ... Sorry, that is not what I meant.\n> \n> You don't have to explain these to *me* specifically as a response\n> to this thread. What I meant was that your patch should have these\n> necessary descriptions in its proposed commit log message.\n\nIMO not so neccessary if one knows a little web and javascript, which\nis probably likely for a gitweb change..\n\nIt's a simple fix of links broken by manual URI manipulation that\ndidn't consider fragments. Is the subject description really not\nenough?\n\n\n//Peter\n"},{"id":"176276","messageId":"7vaa9q29ag.fsf@alter.siamese.dyndns.org","threadId":"28501","inReplyTo":"1317060642-25488-1-git-send-email-peter@stuge.se","subject":"Re: [PATCH] gitweb: Add js=1 before an URI fragment to fix line number links","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-26T23:06:31Z","receivedAt":"2011-09-26T23:06:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Peter Stuge writes:\n\n> -var jsExceptionsRe = /[;?]js=[01]$/;\n> +var jsExceptionsRe = /[;?]js=[01](#.*)?$/;\n> \n> /**\n> * Add '?js=1' or ';js=1' to the end of every link in the document\n> @@ -33,9 +33,9 @@ function fixLinks() {\n> \tvar allLinks = document.getElementsByTagName(\"a\") || document.links;\n> \tfor (var i = 0, len = allLinks.length; i < len; i++) {\n> \t\tvar link = allLinks[i];\n> -\t\tif (!jsExceptionsRe.test(link)) { // =~ /[;?]js=[01]$/;\n> -\t\t\tlink.href +=\n> -\t\t\t\t(link.href.indexOf('?') === -1 ? '?' : ';') + 'js=1';\n> +\t\tif (!jsExceptionsRe.test(link)) { // =~ /[;?]js=[01](#.*)?$/;\n\nLet's not repeat the regexp in the comment (badness you inherited from the\noriginal).\n\nRegarding the \"we already have the js=0 or js=1 in the URL\" check here...\n\n> +var jsExceptionsRe = /[;?]js=[01](#.*)?$/;\n\n... I am wondering who guarantees that this js=[01] is the last parameter\nbefore the fragment identifier. The answer obviously is \"the way the\ncurrent code is written using replace() method on link.href\", but that is\nsomewhat disturbing, because it is not clear what should happen, short of\ntotal rewrite of the code around this, when somebody needs to include\nanother variable, say xx=[01], just like the js=[01] you are fixing here,\nin the resulting URL. In other words, this fixLinks() logic does not seem\nto scale and also looks brittle.\n\nThe patch itself looks correct as a short-term fix, though.\n\nThanks.\n"},{"id":"176280","messageId":"20110926231703.21696.qmail@stuge.se","threadId":"28501","inReplyTo":"7vaa9q29ag.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] gitweb: Add js=1 before an URI fragment to fix line number links","fromName":"Peter Stuge","fromEmail":"peter@stuge.se","sentAt":"2011-09-26T23:17:03Z","receivedAt":"2011-09-26T23:17:03Z","isPatch":true,"sender":{"key":"peter@stuge.se","avatar":null},"body":"Junio C Hamano wrote:\n> > \t\tvar link = allLinks[i];\n> > -\t\tif (!jsExceptionsRe.test(link)) { // =~ /[;?]js=[01]$/;\n> > -\t\t\tlink.href +=\n> > -\t\t\t\t(link.href.indexOf('?') === -1 ? '?' : ';') + 'js=1';\n> > +\t\tif (!jsExceptionsRe.test(link)) { // =~ /[;?]js=[01](#.*)?$/;\n> \n> Let's not repeat the regexp in the comment (badness you inherited\n> from the original).\n\nI agree. I chose to follow style.\n\n\n> Regarding the \"we already have the js=0 or js=1 in the URL\" check here...\n> \n> > +var jsExceptionsRe = /[;?]js=[01](#.*)?$/;\n> \n> ... I am wondering who guarantees that this js=[01] is the last parameter\n> before the fragment identifier. The answer obviously is \"the way the\n> current code is written using replace() method on link.href\", but that is\n> somewhat disturbing, because it is not clear what should happen, short of\n> total rewrite of the code around this, when somebody needs to include\n> another variable, say xx=[01], just like the js=[01] you are fixing here,\n> in the resulting URL. In other words, this fixLinks() logic does not seem\n> to scale and also looks brittle.\n\nAt first glance indeed. But I think it might be fine, because the\npurpose of this function is only to use javascript in order to\nindicate to the server that the client supports javascript. This is\nsomewhat an odd case, the link rewriting is pretty much needed only\nfor this one value so it doesn't really have to scale. But I have\nnothing against say removing (#.*)?$ and thus only matching\n[?;]js=[01] in link.href.\n\n\n> The patch itself looks correct as a short-term fix, though.\n\nThanks, I figured if it was OK before then at least it would be\nbetter now.\n\n\n//Peter\n"},{"id":"176281","messageId":"m339fiq4dx.fsf@localhost.localdomain","threadId":"28501","inReplyTo":"1317060642-25488-1-git-send-email-peter@stuge.se","subject":"Re: [PATCH] gitweb: Add js=1 before an URI fragment to fix line number links","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2011-09-26T23:19:25Z","receivedAt":"2011-09-26T23:19:25Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Peter Stuge writes:\n\nIt really needs a proper commit message. Perhaps something like this:\n\n The fixLinks() function in javascript-detection.js is supposed to\n add 'js' query parameter with a value of 1 to each link that does\n not have 'js' query parameter already.\n\n However it didn't take into account the fact that URI can have\n 'fragment' part. It meant that:\n\n 1. URIs with fragment and 'js' query parameter, like e.g.\n\n ...foo?js=0#l199\n\n were not recognized as having 'js' query parameter already.\n \n 2. The 'js' query parameter, in the form of either '?js=1' or ';js=1'\n was appended at the end of URI, even if it included a fragment\n (had a hash part). This lead to the incorrect links like this\n\n ...foo#l199?js=1\n\n instead of adding query parameter as last part of query, but\n before the fragment part, i.e.\n\n ...foo?js=1#l199\n\n> Signed-off-by: Peter Stuge \n\nFor what it is worth it\n\nAcked-by: Jakub Narebski \n\n> ---\n> gitweb/static/js/javascript-detection.js | 8 ++++----\n> 1 files changed, 4 insertions(+), 4 deletions(-)\n> \n> diff --git a/gitweb/static/js/javascript-detection.js b/gitweb/static/js/javascript-detection.js\n> index 93dd2bd..003acd1 100644\n> --- a/gitweb/static/js/javascript-detection.js\n> +++ b/gitweb/static/js/javascript-detection.js\n> @@ -16,7 +16,7 @@\n> * and other reasons to not add 'js=1' param at the end of link\n> * @constant\n> */\n> -var jsExceptionsRe = /[;?]js=[01]$/;\n> +var jsExceptionsRe = /[;?]js=[01](#.*)?$/;\n> \n> /**\n> * Add '?js=1' or ';js=1' to the end of every link in the document\n> @@ -33,9 +33,9 @@ function fixLinks() {\n> \tvar allLinks = document.getElementsByTagName(\"a\") || document.links;\n> \tfor (var i = 0, len = allLinks.length; i < len; i++) {\n> \t\tvar link = allLinks[i];\n> -\t\tif (!jsExceptionsRe.test(link)) { // =~ /[;?]js=[01]$/;\n> -\t\t\tlink.href +=\n> -\t\t\t\t(link.href.indexOf('?') === -1 ? '?' : ';') + 'js=1';\n> +\t\tif (!jsExceptionsRe.test(link)) { // =~ /[;?]js=[01](#.*)?$/;\n> +\t\t\tlink.href = link.href.replace(/(#|$)/,\n> +\t\t\t\t(link.href.indexOf('?') === -1 ? '?' : ';') + 'js=1$1');\n> \t\t}\n> \t}\n> }\n> -- \n> 1.7.4.1.343.ga91df.dirty\n> \n\n-- \nJakub Narębski mailto:jnareb@fuw.edu.pl\n ZTHiL IFT UW http://info.fuw.edu.pl/~jnareb/\n"},{"id":"176309","messageId":"4E8170B3.8040205@viscovery.net","threadId":"28501","inReplyTo":"20110926222801.14985.qmail@stuge.se","subject":"Re: [PATCH] gitweb: Add js=1 before an URI fragment to fix line number links","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2011-09-27T06:44:03Z","receivedAt":"2011-09-27T06:44:03Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 9/27/2011 0:28, schrieb Peter Stuge:\n> Junio C Hamano wrote:\n>> You don't have to explain these to *me* specifically as a response\n>> to this thread. What I meant was that your patch should have these\n>> necessary descriptions in its proposed commit log message.\n> \n> IMO not so neccessary if one knows a little web and javascript, which\n> is probably likely for a gitweb change..\n> \n> It's a simple fix of links broken by manual URI manipulation that\n> didn't consider fragments. Is the subject description really not\n> enough?\n\nNo, it is not. The target audience of a commit message are people like I\nam. I do know a bit of Perl, and a bit of Javascript; I know how an URL is\nstructured; I would find my way through the gitweb code if the need\narises. But I am not an expert in any of these areas.\n\nThe subject alone is not sufficient because I do not know for sure what an\n\"URI fragment\" is or what role line numbers in gitweb's links play. The\nexplanations and examples you gave in a later email were very\nenlightening, and they would be very helpful if *I* am forced to hack\ngitweb, and if I need to understand why this particular change was good.\n\nFinding the right balance between verbosity and terseness needs practice,\nbut to write *no* justification is practically always wrong.\n\n-- Hannes\n"},{"id":"176323","messageId":"20110927094947.10955.qmail@stuge.se","threadId":"28501","inReplyTo":"4E8170B3.8040205@viscovery.net","subject":"Re: [PATCH] gitweb: Add js=1 before an URI fragment to fix line number links","fromName":"Peter Stuge","fromEmail":"peter@stuge.se","sentAt":"2011-09-27T09:49:47Z","receivedAt":"2011-09-27T09:49:47Z","isPatch":true,"sender":{"key":"peter@stuge.se","avatar":null},"body":"Hey!\n\nJohannes Sixt wrote:\n> > It's a simple fix of links broken by manual URI manipulation that\n> > didn't consider fragments. Is the subject description really not\n> > enough?\n> \n> No, it is not. The target audience of a commit message are people like I\n> am. I do know a bit of Perl, and a bit of Javascript; I know how an URL\n> is structured; I would find my way through the gitweb code if the need\n> arises. But I am not an expert in any of these areas.\n> \n> The subject alone is not sufficient because I do not know for sure what\n> an \"URI fragment\" is or what role line numbers in gitweb's links play.\n\nI shall continue playing advocatus diaboli only a little longer.\n\n\n> The explanations and examples you gave in a later email were very\n> enlightening, and they would be very helpful if *I* am forced to hack\n> gitweb, and if I need to understand why this particular change was good.\n\nOn the other hand you're just one quick google search on uri fragment\naway from the same enlightenment, and relying on terminology saves on\nunneccessary redundance.\n\nLorelei: That's repetitive\nRory: ..and redundant.\nLorelei: That's repetitive\nRory: ..and redundant.\n\n(SCNR the pop culture reference! :)\n\n\n> Finding the right balance between verbosity and terseness needs\n> practice,\n\nI disagree, but I agree with you if we qualify that a little. The\nright balance is a matter of subjective review, so the only way it\ncan be practiced with relevance is by actually working with the same\nreviewers for a while, to learn what they consider right.\n\nIt can absolutely not be practiced out of context, ie. with different\npeers. No later than the day before I sent this patch I wrote a\nwelcome mail in another open source project, to a new contributor,\nwhere one bit was about commit messages.\n\nhttp://marc.info/?l=openocd-development&m=131698532523018\n\n\"* Write a top quality commit message, technically and logically\n\n...\nAs for the logical quality, it is important to write the first line\ndescription of the change so that it makes sense for someone who\nknows nothing at all about the code, since this is used in list\nviews, and since the background for this code and for why this change\nwas done the way it was done comes only in the later lines, which may\nnot be available from where that list view is. ... Keep it\nhigh level, clear and simple. Writing this one line is not\nneccessarily easy.\"\n\nI of course also try to practise exactly this, but it's difficult to\nknow what reviewers expect to be fed, or how much verbosity they\nprefer. :) I tend to prefer as much useful information as possible in\nthe first line, while keeping it ideally <60 chars. Many times I find\nit to be enough.\n\n\n> but to write *no* justification is practically always wrong.\n\nI disagree strongly that I wrote no justification. I agree that it\nwas not verbose. I'm sorry that this is a problem. I'm personally\nallergic to redundancy such as the commit message Jakub wrote, I\nthink it's not only reasonable but also desirable to avoid that.\nMaybe gitweb is a special case in git.git though, I don't know, but\nI'm a little surprised. :)\n\nAnyway, I'm more than happy to write a more verbose message for you!\n\n\n//Peter\n"},{"id":"176324","messageId":"1317117060-3955-1-git-send-email-peter@stuge.se","threadId":"28501","inReplyTo":"20110927094947.10955.qmail@stuge.se","subject":"[PATCH] gitweb: Fix links to lines in blobs when javascript-actions are enabled","fromName":"Peter Stuge","fromEmail":"peter@stuge.se","sentAt":"2011-09-27T09:51:00Z","receivedAt":"2011-09-27T09:51:00Z","isPatch":true,"sender":{"key":"peter@stuge.se","avatar":null},"body":"Some javascript code will run in the browser onLoad and signals back to\ngitweb that the browser can actually do javascript.\n\nThe code adds [?;]js=1 into the URL of all links on the page. The code\nalways added [?;]js=1 to the end of links, which is wrong when links\ncontain a URI fragment, such as links directly to a line in a blob:\n..?p=repo.git;a=blob;f=file#l123\n\nIn this case, [?;]js=1 must be added before the hashmark.\n\nSigned-off-by: Peter Stuge \n---\n gitweb/static/js/javascript-detection.js | 8 ++++----\n 1 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/gitweb/static/js/javascript-detection.js b/gitweb/static/js/javascript-detection.js\nindex 93dd2bd..fa2596f 100644\n--- a/gitweb/static/js/javascript-detection.js\n+++ b/gitweb/static/js/javascript-detection.js\n@@ -16,7 +16,7 @@\n * and other reasons to not add 'js=1' param at the end of link\n * @constant\n */\n-var jsExceptionsRe = /[;?]js=[01]$/;\n+var jsExceptionsRe = /[;?]js=[01](#.*)?$/;\n \n /**\n * Add '?js=1' or ';js=1' to the end of every link in the document\n@@ -33,9 +33,9 @@ function fixLinks() {\n \tvar allLinks = document.getElementsByTagName(\"a\") || document.links;\n \tfor (var i = 0, len = allLinks.length; i < len; i++) {\n \t\tvar link = allLinks[i];\n-\t\tif (!jsExceptionsRe.test(link)) { // =~ /[;?]js=[01]$/;\n-\t\t\tlink.href +=\n-\t\t\t\t(link.href.indexOf('?') === -1 ? '?' : ';') + 'js=1';\n+\t\tif (!jsExceptionsRe.test(link)) {\n+\t\t\tlink.href = link.href.replace(/(#|$)/,\n+\t\t\t\t(link.href.indexOf('?') === -1 ? '?' : ';') + 'js=1$1');\n \t\t}\n \t}\n }\n-- \n1.7.4.1.343.ga91df.dirty\n"},{"id":"176346","messageId":"7v7h4tykf3.fsf@alter.siamese.dyndns.org","threadId":"28501","inReplyTo":"1317117060-3955-1-git-send-email-peter@stuge.se","subject":"Re: [PATCH] gitweb: Fix links to lines in blobs when javascript-actions are enabled","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-27T17:17:20Z","receivedAt":"2011-09-27T17:17:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Peter Stuge writes:\n\n> Some javascript code will run in the browser onLoad and signals back to\n> gitweb that the browser can actually do javascript.\n>\n> The code adds [?;]js=1 into the URL of all links on the page. The code\n> always added [?;]js=1 to the end of links, which is wrong when links\n> contain a URI fragment, such as links directly to a line in a blob:\n> ..?p=repo.git;a=blob;f=file#l123\n>\n> In this case, [?;]js=1 must be added before the hashmark.\n>\n> Signed-off-by: Peter Stuge \n\nNicely done. You forgot to mention another bug you fixed (see Jakub's\nsample in the thread), so I'll amend in a few sentences to describe it as\nwell.\n\nThanks.\n"},{"id":"176348","messageId":"7vy5x9x4ss.fsf@alter.siamese.dyndns.org","threadId":"28501","inReplyTo":"20110927094947.10955.qmail@stuge.se","subject":"Re: [PATCH] gitweb: Add js=1 before an URI fragment to fix line number links","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-27T17:40:03Z","receivedAt":"2011-09-27T17:40:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Peter Stuge writes:\n\n> I disagree, but I agree with you if we qualify that a little. The\n> right balance is a matter of subjective review, so the only way it\n> can be practiced with relevance is by actually working with the same\n> reviewers for a while, to learn what they consider right.\n>\n> It can absolutely not be practiced out of context,...\n\nYou are right that you need to practice in the context of working in the\nGit project to explain your change with the right amount of details when\npreparing a change for the Git project, and the same goes for the openocd\nproject.\n\nWe aim to write our commit log messages primarily for future developers\nwho want to read \"git log -p\" output and understand why these changes had\nto be made, to help them avoid intentionally breaking what the old commit\nwanted to achieve when they want to modify the existing code 6 months down\nthe road. Your reviewers aim to make sure that your log message gives\nsufficient information to such future developers, as opposed to themselves\nwhile reviewing the patch and the issue is still fresh in their head.\n\nI suspect that the target audience of log message may even be different\ndepending on the project, and openocd may not work that way, and that is\nperfectly fine.\n"},{"id":"176376","messageId":"20110928013101.9699.qmail@stuge.se","threadId":"28501","inReplyTo":"7v7h4tykf3.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] gitweb: Fix links to lines in blobs when javascript-actions are enabled","fromName":"Peter Stuge","fromEmail":"peter@stuge.se","sentAt":"2011-09-28T01:31:01Z","receivedAt":"2011-09-28T01:31:01Z","isPatch":true,"sender":{"key":"peter@stuge.se","avatar":null},"body":"Junio C Hamano wrote:\n> Nicely done.\n\nThanks.\n\n> You forgot to mention another bug you fixed\n\nWell.. I'm not sure that it is worth mentioning, since it may never\nactually have been triggered by the way fixLinks() works.\n\n\n> I'll amend in a few sentences to describe it as well.\n\nGo for it.\n\n\n//Peter\n"}]}