{"thread":{"id":"44231","subject":"[PATCH v2 0/3] gitweb: Be smarter about linking to SHA1s in log messages","startedAt":"2016-10-06T09:12:18Z","lastAt":"2016-10-17T19:47:07Z","messageCount":16,"participants":["Ævar Arnfjörð Bjarmason","Junio C Hamano","Jakub Narębski"],"isPatch":true,"patchVersion":2,"patchTotal":3},"messages":[{"id":"303463","messageId":"20161006091135.29590-1-avarab@gmail.com","threadId":"44231","inReplyTo":null,"subject":"[PATCH v2 0/3] gitweb: Be smarter about linking to SHA1s in log messages","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2016-10-06T09:11:32Z","receivedAt":"2016-10-06T09:12:18Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This is v2 of patches I sent on September 21st starting at\n<20160921114428.28664-1-avarab@gmail.com>. Jakub Narębski had a lot of\nfeedback for that series (thanks!). Which as far as I can tell I've\nincorporated entirely in this re-roll.\n\nÆvar Arnfjörð Bjarmason (3):\n gitweb: Fix a typo in a comment\n gitweb: Link to 7-char+ SHA1s, not only 8-char+\n gitweb: Link to \"git describe\"'d commits in log messages\n\n gitweb/gitweb.perl | 20 +++++++++++++++++---\n 1 file changed, 17 insertions(+), 3 deletions(-)\n\n-- \n2.9.3\n\n"},{"id":"303464","messageId":"20161006091135.29590-2-avarab@gmail.com","threadId":"44231","inReplyTo":"20161006091135.29590-1-avarab@gmail.com","subject":"[PATCH v2 1/3] gitweb: Fix a typo in a comment","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2016-10-06T09:11:33Z","receivedAt":"2016-10-06T09:12:40Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change a typo'd MIME type in a comment. The Content-Type is\napplication/xhtml+xml, not application/xhtm+xml.\n\nFixes up code originally added in 53c4031 (\"gitweb: Strip\nnon-printable characters from syntax highlighter output\", 2011-09-16).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n gitweb/gitweb.perl | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 44094f4..cba7405 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -1616,7 +1616,7 @@ sub esc_path {\n \treturn $str;\n }\n \n-# Sanitize for use in XHTML + application/xml+xhtm (valid XML 1.0)\n+# Sanitize for use in XHTML + application/xml+xhtml (valid XML 1.0)\n sub sanitize {\n \tmy $str = shift;\n \n-- \n2.9.3\n\n"},{"id":"303465","messageId":"20161006091135.29590-4-avarab@gmail.com","threadId":"44231","inReplyTo":"20161006091135.29590-1-avarab@gmail.com","subject":"[PATCH v2 3/3] gitweb: Link to \"git describe\"'d commits in log messages","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2016-10-06T09:11:35Z","receivedAt":"2016-10-06T09:12:44Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the log formatting function to know about \"git describe\" output\nsuch as \"v2.8.0-4-g867ad08\", in addition to just plain \"867ad08\".\n\nThere are still many valid refnames that we don't link to\ne.g. v2.10.0-rc1~2^2~1 is also a valid way to refer to\nv2.8.0-4-g867ad08, but I'm not supporting that with this commit,\nsimilarly it's trivially possible to create some refnames like\n\"æ/var-gf6727b0\" or which won't be picked up by this regex.\n\nThere's surely room for improvement here, but I just wanted to address\nthe very common case of sticking \"git describe\" output into commit\nmessages without trying to link to all possible refnames, that's going\nto be a rather futile exercise given that this is free text, and it\nwould be prohibitively expensive to look up whether the references in\nquestion exist in our repository.\n\nThere was on-list discussion about how we could do better than this\npatch. Junio suggested to update parse_commits() to call a new\n\"gitweb--helper\" command which would pass each of the revision\ncandidates through \"rev-parse --verify --quiet\". That would cut down\non our false positives (e.g. we'll link to \"deadbeef\"), and also allow\nus to be more aggressive in selecting candidate revisions.\n\nThat may be too expensive to work in practice, or it may\nnot. Investigating that would be a good follow-up to this patch.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n gitweb/gitweb.perl | 18 ++++++++++++++++--\n 1 file changed, 16 insertions(+), 2 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 92b5e91..7cf68f0 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -2036,10 +2036,24 @@ sub format_log_line_html {\n \tmy $line = shift;\n \n \t$line = esc_html($line, -nbsp=>1);\n-\t$line =~ s{\\b([0-9a-fA-F]{7,40})\\b}{\n+\t$line =~ s{\n+ \\b\n+ (\n+ # The output of \"git describe\", e.g. v2.10.0-297-gf6727b0\n+ # or hadoop-20160921-113441-20-g094fb7d\n+ (?a({-href => href(action=>\"object\", hash=>$1),\n \t\t\t\t\t-class => \"text\"}, $1);\n-\t}eg;\n+\t}egx;\n \n \treturn $line;\n }\n-- \n2.9.3\n\n"},{"id":"303466","messageId":"20161006091135.29590-3-avarab@gmail.com","threadId":"44231","inReplyTo":"20161006091135.29590-1-avarab@gmail.com","subject":"[PATCH v2 2/3] gitweb: Link to 7-char+ SHA1s, not only 8-char+","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2016-10-06T09:11:34Z","receivedAt":"2016-10-06T09:12:45Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the minimum length of an abbreviated object identifier in the\ncommit message gitweb tries to turn into link from 8 hexchars to 7.\n\nThis arbitrary minimum length of 8 was introduced in bfe2191 (\"gitweb:\nSHA-1 in commit log message links to \"object\" view\", 2006-12-10), but\nthe default abbreviation length is 7, and has been for a long time.\n\nIt's still possible to reference SHA1s down to 4 characters in length,\nsee v1.7.4-1-gdce9648's MINIMUM_ABBREV, but I can't see how to make\ngit actually produce that, so I doubt anyone is putting that into log\nmessages in practice, but people definitely do put 7 character SHA1s\ninto log messages.\n\nI think it's fairly dubious to link to things matching [0-9a-fA-F]\nhere as opposed to just [0-9a-f], that dates back to the initial\nversion of gitweb from 161332a (\"first working version\",\n2005-08-07). Git will accept all-caps SHA1s, but didn't ever produce\nthem as far as I can tell.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n gitweb/gitweb.perl | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex cba7405..92b5e91 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -2036,7 +2036,7 @@ sub format_log_line_html {\n \tmy $line = shift;\n \n \t$line = esc_html($line, -nbsp=>1);\n-\t$line =~ s{\\b([0-9a-fA-F]{8,40})\\b}{\n+\t$line =~ s{\\b([0-9a-fA-F]{7,40})\\b}{\n \t\t$cgi->a({-href => href(action=>\"object\", hash=>$1),\n \t\t\t\t\t-class => \"text\"}, $1);\n \t}eg;\n-- \n2.9.3\n\n"},{"id":"303528","messageId":"xmqq37k9jm86.fsf@gitster.mtv.corp.google.com","threadId":"44231","inReplyTo":"20161006091135.29590-3-avarab@gmail.com","subject":"Re: [PATCH v2 2/3] gitweb: Link to 7-char+ SHA1s, not only 8-char+","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-06T19:44:57Z","receivedAt":"2016-10-06T19:45:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Change the minimum length of an abbreviated object identifier in the\n> commit message gitweb tries to turn into link from 8 hexchars to 7.\n>\n> This arbitrary minimum length of 8 was introduced in bfe2191 (\"gitweb:\n> SHA-1 in commit log message links to \"object\" view\", 2006-12-10), but\n> the default abbreviation length is 7, and has been for a long time.\n>\n> It's still possible to reference SHA1s down to 4 characters in length,\n> see v1.7.4-1-gdce9648's MINIMUM_ABBREV, but I can't see how to make\n> git actually produce that, so I doubt anyone is putting that into log\n> messages in practice, but people definitely do put 7 character SHA1s\n> into log messages.\n>\n> I think it's fairly dubious to link to things matching [0-9a-fA-F]\n> here as opposed to just [0-9a-f], that dates back to the initial\n> version of gitweb from 161332a (\"first working version\",\n> 2005-08-07). Git will accept all-caps SHA1s, but didn't ever produce\n> them as far as I can tell.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n\ns/SHA1/SHA-1/g. I agree that A-F are dubious.\n\n> gitweb/gitweb.perl | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index cba7405..92b5e91 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -2036,7 +2036,7 @@ sub format_log_line_html {\n> \tmy $line = shift;\n> \n> \t$line = esc_html($line, -nbsp=>1);\n> -\t$line =~ s{\\b([0-9a-fA-F]{8,40})\\b}{\n> +\t$line =~ s{\\b([0-9a-fA-F]{7,40})\\b}{\n> \t\t$cgi->a({-href => href(action=>\"object\", hash=>$1),\n> \t\t\t\t\t-class => \"text\"}, $1);\n> \t}eg;\n"},{"id":"303529","messageId":"xmqqy421i7dg.fsf@gitster.mtv.corp.google.com","threadId":"44231","inReplyTo":"20161006091135.29590-4-avarab@gmail.com","subject":"Re: [PATCH v2 3/3] gitweb: Link to \"git describe\"'d commits in log messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-06T19:51:07Z","receivedAt":"2016-10-06T19:51:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Change the log formatting function to know about \"git describe\" output\n> such as \"v2.8.0-4-g867ad08\", in addition to just plain \"867ad08\".\n>\n> There are still many valid refnames that we don't link to\n> e.g. v2.10.0-rc1~2^2~1 is also a valid way to refer to\n> v2.8.0-4-g867ad08, but I'm not supporting that with this commit,\n> similarly it's trivially possible to create some refnames like\n> \"æ/var-gf6727b0\" or which won't be picked up by this regex.\n\nNot a serious counter-proposal or suggestion, and certainly not an\nobjection to the patch I am responding to, but I wonder if it is so\nbad if we made the 867ad08 into link when showing v2.8.0-4-g867ad08.\n\nIOW, in addition to \\b followed by [0-9a-f]+ followed by \\b, if we\nallowed an optional 'g' in front of the hex, e.g.\n\n-\t$line =~ s{\\b([0-9a-fA-F]{7,40})\\b}{\n+\t$line =~ s{\\bg?([0-9a-fA-F]{7,40})\\b}{\n\nwouldn't that be much simpler, covers more cases and sufficient?\n\n> There's surely room for improvement here, but I just wanted to address\n> the very common case of sticking \"git describe\" output into commit\n> messages without trying to link to all possible refnames, that's going\n> to be a rather futile exercise given that this is free text, and it\n> would be prohibitively expensive to look up whether the references in\n> question exist in our repository.\n>\n> There was on-list discussion about how we could do better than this\n> patch. Junio suggested to update parse_commits() to call a new\n> \"gitweb--helper\" command which would pass each of the revision\n> candidates through \"rev-parse --verify --quiet\". That would cut down\n> on our false positives (e.g. we'll link to \"deadbeef\"), and also allow\n> us to be more aggressive in selecting candidate revisions.\n>\n> That may be too expensive to work in practice, or it may\n> not. Investigating that would be a good follow-up to this patch.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> gitweb/gitweb.perl | 18 ++++++++++++++++--\n> 1 file changed, 16 insertions(+), 2 deletions(-)\n>\n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 92b5e91..7cf68f0 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -2036,10 +2036,24 @@ sub format_log_line_html {\n> \tmy $line = shift;\n> \n> \t$line = esc_html($line, -nbsp=>1);\n> -\t$line =~ s{\\b([0-9a-fA-F]{7,40})\\b}{\n> +\t$line =~ s{\n> + \\b\n> + (\n> + # The output of \"git describe\", e.g. v2.10.0-297-gf6727b0\n> + # or hadoop-20160921-113441-20-g094fb7d\n> + (? + [A-Za-z0-9.-]+\n> + (?!\\.) # refs can't end with \".\", see check_refname_format()\n> + -g[0-9a-fA-F]{7,40}\n> + |\n> + # Just a normal looking Git SHA1\n> + [0-9a-fA-F]{7,40}\n> + )\n> + \\b\n> + }{\n> \t\t$cgi->a({-href => href(action=>\"object\", hash=>$1),\n> \t\t\t\t\t-class => \"text\"}, $1);\n> -\t}eg;\n> +\t}egx;\n> \n> \treturn $line;\n> }\n"},{"id":"303669","messageId":"CACBZZX5mnCmzT3N35YKpr2t1LT-hh-H7eS-+XTjudguedJL5Zw@mail.gmail.com","threadId":"44231","inReplyTo":"xmqqy421i7dg.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 3/3] gitweb: Link to \"git describe\"'d commits in log messages","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2016-10-09T11:20:47Z","receivedAt":"2016-10-09T11:21:13Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Thu, Oct 6, 2016 at 9:51 PM, Junio C Hamano wrote:\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> Change the log formatting function to know about \"git describe\" output\n>> such as \"v2.8.0-4-g867ad08\", in addition to just plain \"867ad08\".\n>>\n>> There are still many valid refnames that we don't link to\n>> e.g. v2.10.0-rc1~2^2~1 is also a valid way to refer to\n>> v2.8.0-4-g867ad08, but I'm not supporting that with this commit,\n>> similarly it's trivially possible to create some refnames like\n>> \"æ/var-gf6727b0\" or which won't be picked up by this regex.\n>\n> Not a serious counter-proposal or suggestion, and certainly not an\n> objection to the patch I am responding to, but I wonder if it is so\n> bad if we made the 867ad08 into link when showing v2.8.0-4-g867ad08.\n>\n> IOW, in addition to \\b followed by [0-9a-f]+ followed by \\b, if we\n> allowed an optional 'g' in front of the hex, e.g.\n>\n> - $line =~ s{\\b([0-9a-fA-F]{7,40})\\b}{\n> + $line =~ s{\\bg?([0-9a-fA-F]{7,40})\\b}{\n>\n> wouldn't that be much simpler, covers more cases and sufficient?\n\nIt would be more simpler, maybe that's the right approach. I have a\npreference for making the entire reference a link. I think it makes\nmore sense for the UI.\n\n>> There's surely room for improvement here, but I just wanted to address\n>> the very common case of sticking \"git describe\" output into commit\n>> messages without trying to link to all possible refnames, that's going\n>> to be a rather futile exercise given that this is free text, and it\n>> would be prohibitively expensive to look up whether the references in\n>> question exist in our repository.\n>>\n>> There was on-list discussion about how we could do better than this\n>> patch. Junio suggested to update parse_commits() to call a new\n>> \"gitweb--helper\" command which would pass each of the revision\n>> candidates through \"rev-parse --verify --quiet\". That would cut down\n>> on our false positives (e.g. we'll link to \"deadbeef\"), and also allow\n>> us to be more aggressive in selecting candidate revisions.\n>>\n>> That may be too expensive to work in practice, or it may\n>> not. Investigating that would be a good follow-up to this patch.\n>>\n>> Signed-off-by: Ævar Arnfjörð Bjarmason \n>> ---\n>> gitweb/gitweb.perl | 18 ++++++++++++++++--\n>> 1 file changed, 16 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n>> index 92b5e91..7cf68f0 100755\n>> --- a/gitweb/gitweb.perl\n>> +++ b/gitweb/gitweb.perl\n>> @@ -2036,10 +2036,24 @@ sub format_log_line_html {\n>> my $line = shift;\n>>\n>> $line = esc_html($line, -nbsp=>1);\n>> - $line =~ s{\\b([0-9a-fA-F]{7,40})\\b}{\n>> + $line =~ s{\n>> + \\b\n>> + (\n>> + # The output of \"git describe\", e.g. v2.10.0-297-gf6727b0\n>> + # or hadoop-20160921-113441-20-g094fb7d\n>> + (?> + [A-Za-z0-9.-]+\n>> + (?!\\.) # refs can't end with \".\", see check_refname_format()\n>> + -g[0-9a-fA-F]{7,40}\n>> + |\n>> + # Just a normal looking Git SHA1\n>> + [0-9a-fA-F]{7,40}\n>> + )\n>> + \\b\n>> + }{\n>> $cgi->a({-href => href(action=>\"object\", hash=>$1),\n>> -class => \"text\"}, $1);\n>> - }eg;\n>> + }egx;\n>>\n>> return $line;\n>> }\n"},{"id":"304176","messageId":"51eb6aba-9cd2-47f4-a26e-6dbbabdaa282@gmail.com","threadId":"44231","inReplyTo":"20161006091135.29590-2-avarab@gmail.com","subject":"Re: [PATCH v2 1/3] gitweb: Fix a typo in a comment","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2016-10-14T17:34:04Z","receivedAt":"2016-10-14T17:34:22Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"W dniu 06.10.2016 o 11:11, Ævar Arnfjörð Bjarmason napisał:\n\n> Change a typo'd MIME type in a comment. The Content-Type is\n> application/xhtml+xml, not application/xhtm+xml.\n> \n> Fixes up code originally added in 53c4031 (\"gitweb: Strip\n> non-printable characters from syntax highlighter output\", 2011-09-16).\n> \n> Signed-off-by: Ævar Arnfjörð Bjarmason \n\nGood. Thanks for taking care of this.\nFor what is worth for such a trivial patch:\n\nAcked-by: Jakub Narębski \n\n> ---\n> gitweb/gitweb.perl | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 44094f4..cba7405 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -1616,7 +1616,7 @@ sub esc_path {\n> \treturn $str;\n> }\n> \n> -# Sanitize for use in XHTML + application/xml+xhtm (valid XML 1.0)\n> +# Sanitize for use in XHTML + application/xml+xhtml (valid XML 1.0)\n> sub sanitize {\n> \tmy $str = shift;\n> \n> \n\n"},{"id":"304185","messageId":"3fa9902f-3b01-5ec6-8129-34cff4c7cac9@gmail.com","threadId":"44231","inReplyTo":"20161006091135.29590-3-avarab@gmail.com","subject":"Re: [PATCH v2 2/3] gitweb: Link to 7-char+ SHA1s, not only 8-char+","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2016-10-14T17:45:12Z","receivedAt":"2016-10-14T17:45:30Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"W dniu 06.10.2016 o 11:11, Ævar Arnfjörð Bjarmason pisze:\n\n> Change the minimum length of an abbreviated object identifier in the\n> commit message gitweb tries to turn into link from 8 hexchars to 7.\n> \n> This arbitrary minimum length of 8 was introduced in bfe2191 (\"gitweb:\n> SHA-1 in commit log message links to \"object\" view\", 2006-12-10), but\n> the default abbreviation length is 7, and has been for a long time.\n> \n> It's still possible to reference SHA1s down to 4 characters in length,\n> see v1.7.4-1-gdce9648's MINIMUM_ABBREV, but I can't see how to make\n> git actually produce that, so I doubt anyone is putting that into log\n> messages in practice, but people definitely do put 7 character SHA1s\n> into log messages.\n\nThat's nice explanation why we want to support 7 character SHA-1\n(it is the default, and was seen in the wild), but don't need to\nsupport shorter lengths.\n\nAnother reason (just for completeness; you don't need to put it in\nthe commit message) to not go below 7 characters is that with \ndecreasing length there is more and more chance for false positives\n(like 'beef' or 'caffee' for 4-char or 5-char hexstring), as I wrote\npreviously.\n\ns/SHA1/SHA-1/g in above paragraph (for correctness and consistency).\n\n> \n> I think it's fairly dubious to link to things matching [0-9a-fA-F]\n> here as opposed to just [0-9a-f], that dates back to the initial\n> version of gitweb from 161332a (\"first working version\",\n> 2005-08-07). Git will accept all-caps SHA1s, but didn't ever produce\n> them as far as I can tell.\n\nAll right. If we decide to be more strict in what we accept, we can\ndo it in a separate commit.\n\n> \n> Signed-off-by: Ævar Arnfjörð Bjarmason \n\nAcked-by: Jakub Narębski \n\n> ---\n> gitweb/gitweb.perl | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index cba7405..92b5e91 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -2036,7 +2036,7 @@ sub format_log_line_html {\n> \tmy $line = shift;\n> \n> \t$line = esc_html($line, -nbsp=>1);\n> -\t$line =~ s{\\b([0-9a-fA-F]{8,40})\\b}{\n> +\t$line =~ s{\\b([0-9a-fA-F]{7,40})\\b}{\n\nBy the way, it is quite long commit message for one character change.\nNot that it is a bad thing...\n\n> \t\t$cgi->a({-href => href(action=>\"object\", hash=>$1),\n> \t\t\t\t\t-class => \"text\"}, $1);\n> \t}eg;\n> \n\n"},{"id":"304202","messageId":"d070f375-6d95-4050-b823-796fb38b19c0@gmail.com","threadId":"44231","inReplyTo":"20161006091135.29590-4-avarab@gmail.com","subject":"Re: [PATCH v2 3/3] gitweb: Link to \"git describe\"'d commits in log messages","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2016-10-14T20:06:26Z","receivedAt":"2016-10-16T00:34:00Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"W dniu 06.10.2016 o 11:11, Ævar Arnfjörð Bjarmason pisze:\n> Change the log formatting function to know about \"git describe\" output\n> such as \"v2.8.0-4-g867ad08\", in addition to just plain \"867ad08\".\n> \n> There are still many valid refnames that we don't link to\n> e.g. v2.10.0-rc1~2^2~1 is also a valid way to refer to\n> v2.8.0-4-g867ad08, but I'm not supporting that with this commit,\n> similarly it's trivially possible to create some refnames like\n> \"æ/var-gf6727b0\" or which won't be picked up by this regex.\n\nIt's important to cover most common cases occurring naturally in\npeople's repositories, while trying to avoid false positives (the latter\nis important more now, where gitweb doesn't check for rev name validity).\n\nI guess that most common is to use non-hierarchical tags with ASCII-only\nnames for describe output, so while \"æ/var-gf6727b0\" won't be picked,\nit is IMVHO also much less likely to occur.\n\n> \n> There's surely room for improvement here, but I just wanted to address\n> the very common case of sticking \"git describe\" output into commit\n> messages without trying to link to all possible refnames, that's going\n> to be a rather futile exercise given that this is free text, and it\n> would be prohibitively expensive to look up whether the references in\n> question exist in our repository.\n> \n> There was on-list discussion about how we could do better than this\n> patch. Junio suggested to update parse_commits() to call a new\n> \"gitweb--helper\" command which would pass each of the revision\n> candidates through \"rev-parse --verify --quiet\". That would cut down\n> on our false positives (e.g. we'll link to \"deadbeef\"), and also allow\n> us to be more aggressive in selecting candidate revisions.\n> \n> That may be too expensive to work in practice, or it may\n> not. Investigating that would be a good follow-up to this patch.\n\nAll right. That's good and enough for one patch.\n\n> \n> Signed-off-by: Ævar Arnfjörð Bjarmason \n\nAcked-by: Jakub Narębski \n\nBTW. to add to whole \"git describe\" output or not discussion: having link\nspan whole revision marker makes for easier to use UI: larger are to hit.\n\n> ---\n> gitweb/gitweb.perl | 18 ++++++++++++++++--\n> 1 file changed, 16 insertions(+), 2 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 92b5e91..7cf68f0 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -2036,10 +2036,24 @@ sub format_log_line_html {\n> \tmy $line = shift;\n> \n> \t$line = esc_html($line, -nbsp=>1);\n> -\t$line =~ s{\\b([0-9a-fA-F]{7,40})\\b}{\n> +\t$line =~ s{\n> + \\b\n> + (\n> + # The output of \"git describe\", e.g. v2.10.0-297-gf6727b0\n> + # or hadoop-20160921-113441-20-g094fb7d\n> + (? + [A-Za-z0-9.-]+\n> + (?!\\.) # refs can't end with \".\", see check_refname_format()\n> + -g[0-9a-fA-F]{7,40}\n> + |\n> + # Just a normal looking Git SHA1\n> + [0-9a-fA-F]{7,40}\n> + )\n> + \\b\n\nNice using of eXplained regexp. It is much easier to understand with\ncomments.\n\n> + }{\n> \t\t$cgi->a({-href => href(action=>\"object\", hash=>$1),\n> \t\t\t\t\t-class => \"text\"}, $1);\n> -\t}eg;\n> +\t}egx;\n> \n> \treturn $line;\n> }\n> \n\n"},{"id":"304204","messageId":"CACBZZX4FK+zrVyBTpbJAbSAxQ9LuCTXcExeEqZE6D3nwHbNxZA@mail.gmail.com","threadId":"44231","inReplyTo":"xmqq37jyzsdk.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 2/3] gitweb: Link to 7-char+ SHA1s, not only 8-char+","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2016-10-15T08:11:30Z","receivedAt":"2016-10-16T00:34:03Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Fri, Oct 14, 2016 at 8:40 PM, Junio C Hamano wrote:\n> Jakub Narębski writes:\n>\n>> s/SHA1/SHA-1/g in above paragraph (for correctness and consistency).\n>>>\n>>> I think it's fairly dubious to link to things matching [0-9a-fA-F]\n>>> here as opposed to just [0-9a-f], that dates back to the initial\n>>> version of gitweb from 161332a (\"first working version\",\n>>> 2005-08-07). Git will accept all-caps SHA1s, but didn't ever produce\n>>> them as far as I can tell.\n>>\n>> All right. If we decide to be more strict in what we accept, we can\n>> do it in a separate commit.\n>>\n>>>\n>>> Signed-off-by: Ævar Arnfjörð Bjarmason \n>>\n>> Acked-by: Jakub Narębski \n>\n> Thanks for a review. As the topic is not yet in 'next', I'll squish\n> in your Acked-by: to them. I saw them only for 1 & 2/3; would\n> another for 3/3 be coming soon?\n\nAs far as I can tell the only outstanding \"change this\" is your\ns/SHA1/SHA-1/ in , do\nyou want to fix that up or should I submit another series?\n\n>>\n>>> ---\n>>> gitweb/gitweb.perl | 2 +-\n>>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>>\n>>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n>>> index cba7405..92b5e91 100755\n>>> --- a/gitweb/gitweb.perl\n>>> +++ b/gitweb/gitweb.perl\n>>> @@ -2036,7 +2036,7 @@ sub format_log_line_html {\n>>> my $line = shift;\n>>>\n>>> $line = esc_html($line, -nbsp=>1);\n>>> - $line =~ s{\\b([0-9a-fA-F]{8,40})\\b}{\n>>> + $line =~ s{\\b([0-9a-fA-F]{7,40})\\b}{\n>>\n>> By the way, it is quite long commit message for one character change.\n>> Not that it is a bad thing...\n>>\n>>> $cgi->a({-href => href(action=>\"object\", hash=>$1),\n>>> -class => \"text\"}, $1);\n>>> }eg;\n>>>\n"},{"id":"304205","messageId":"xmqq37jyzsdk.fsf@gitster.mtv.corp.google.com","threadId":"44231","inReplyTo":"3fa9902f-3b01-5ec6-8129-34cff4c7cac9@gmail.com","subject":"Re: [PATCH v2 2/3] gitweb: Link to 7-char+ SHA1s, not only 8-char+","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-14T18:40:23Z","receivedAt":"2016-10-16T00:34:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narębski writes:\n\n> s/SHA1/SHA-1/g in above paragraph (for correctness and consistency).\n>> \n>> I think it's fairly dubious to link to things matching [0-9a-fA-F]\n>> here as opposed to just [0-9a-f], that dates back to the initial\n>> version of gitweb from 161332a (\"first working version\",\n>> 2005-08-07). Git will accept all-caps SHA1s, but didn't ever produce\n>> them as far as I can tell.\n>\n> All right. If we decide to be more strict in what we accept, we can\n> do it in a separate commit.\n>\n>> \n>> Signed-off-by: Ævar Arnfjörð Bjarmason \n>\n> Acked-by: Jakub Narębski \n\nThanks for a review. As the topic is not yet in 'next', I'll squish\nin your Acked-by: to them. I saw them only for 1 & 2/3; would\nanother for 3/3 be coming soon?\n\n>\n>> ---\n>> gitweb/gitweb.perl | 2 +-\n>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>> \n>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n>> index cba7405..92b5e91 100755\n>> --- a/gitweb/gitweb.perl\n>> +++ b/gitweb/gitweb.perl\n>> @@ -2036,7 +2036,7 @@ sub format_log_line_html {\n>> \tmy $line = shift;\n>> \n>> \t$line = esc_html($line, -nbsp=>1);\n>> -\t$line =~ s{\\b([0-9a-fA-F]{8,40})\\b}{\n>> +\t$line =~ s{\\b([0-9a-fA-F]{7,40})\\b}{\n>\n> By the way, it is quite long commit message for one character change.\n> Not that it is a bad thing...\n>\n>> \t\t$cgi->a({-href => href(action=>\"object\", hash=>$1),\n>> \t\t\t\t\t-class => \"text\"}, $1);\n>> \t}eg;\n>> \n"},{"id":"304207","messageId":"xmqqvawuycv0.fsf@gitster.mtv.corp.google.com","threadId":"44231","inReplyTo":"CACBZZX4HPci=n193WPm1RCes4PZfFXQtAdJuwxMwvTvF2NBVMA@mail.gmail.com","subject":"Re: [PATCH v2 3/3] gitweb: Link to \"git describe\"'d commits in log messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-14T19:00:51Z","receivedAt":"2016-10-16T00:34:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> I just ran into an example of a better reason for doing it like my\n> patch is doing, which is that if you have some tag like:\n>\n> deployment-20160928-171914-16-g42e13d8\n>\n> With my patch the whole thing will be a link to the 42e13d8 commit,\n> but with this suggestion both 20160928 and 42e13d8 would become commit\n> links, the former one would be broken.\n>\n> Of course we could have some code that would detect that the whole \\S+\n> is part of one thing that ends in g,...\n\nI think that this example shows a flaw not in the \"suffix that looks\nlike an object name\" approach, but more in the boundary regexp,\nnamely, the \\b part; it is probably too loose.\n\nAnd \\S+ is not the right cue, either, for that matter. IOW, \"we\nonly should take hexstring, optionally prefixed with 'g', that\nappears before the whitespace\" is too strict, as a sentence\n\n We broke the system with deployment-g42e13d8.\n\ndoes want to link to 42e13d8, even though full-stop at the end is\nnot whitespace, and the existing regexp uses \\b there as a rough\nequivalent to saying \"Here must be a whitespace or punctuation\".\n\nAn attempt to tighten \"what a punctuation is\" by excluding '-' may\nmake that \"timestamp is in the tagname\" example work, but is not a\ngood solution, either, because two sentences can be concatenated\nwith an em-dash that is often typed as two hyphen in plain text,\nresulting in something like\n\n We broke the system with deployment-g42e13d8--sigh.\n\nand we do want to treat the '-' after 42e13d8 as a punctuation after\na described object name.\n \nSo I agree 3/3 is good thing to do as-is.\n\nThanks.\n\n"},{"id":"304215","messageId":"CACBZZX4HPci=n193WPm1RCes4PZfFXQtAdJuwxMwvTvF2NBVMA@mail.gmail.com","threadId":"44231","inReplyTo":"CACBZZX5mnCmzT3N35YKpr2t1LT-hh-H7eS-+XTjudguedJL5Zw@mail.gmail.com","subject":"Re: [PATCH v2 3/3] gitweb: Link to \"git describe\"'d commits in log messages","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2016-10-14T17:50:46Z","receivedAt":"2016-10-16T00:34:25Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Sun, Oct 9, 2016 at 1:20 PM, Ævar Arnfjörð Bjarmason\n wrote:\n> On Thu, Oct 6, 2016 at 9:51 PM, Junio C Hamano wrote:\n>> Ævar Arnfjörð Bjarmason writes:\n>>\n>>> Change the log formatting function to know about \"git describe\" output\n>>> such as \"v2.8.0-4-g867ad08\", in addition to just plain \"867ad08\".\n>>>\n>>> There are still many valid refnames that we don't link to\n>>> e.g. v2.10.0-rc1~2^2~1 is also a valid way to refer to\n>>> v2.8.0-4-g867ad08, but I'm not supporting that with this commit,\n>>> similarly it's trivially possible to create some refnames like\n>>> \"æ/var-gf6727b0\" or which won't be picked up by this regex.\n>>\n>> Not a serious counter-proposal or suggestion, and certainly not an\n>> objection to the patch I am responding to, but I wonder if it is so\n>> bad if we made the 867ad08 into link when showing v2.8.0-4-g867ad08.\n>>\n>> IOW, in addition to \\b followed by [0-9a-f]+ followed by \\b, if we\n>> allowed an optional 'g' in front of the hex, e.g.\n>>\n>> - $line =~ s{\\b([0-9a-fA-F]{7,40})\\b}{\n>> + $line =~ s{\\bg?([0-9a-fA-F]{7,40})\\b}{\n>>\n>> wouldn't that be much simpler, covers more cases and sufficient?\n>\n> It would be more simpler, maybe that's the right approach. I have a\n> preference for making the entire reference a link. I think it makes\n> more sense for the UI.\n\nI just ran into an example of a better reason for doing it like my\npatch is doing, which is that if you have some tag like:\n\ndeployment-20160928-171914-16-g42e13d8\n\n With my patch the whole thing will be a link to the 42e13d8 commit,\nbut with this suggestion both 20160928 and 42e13d8 would become commit\nlinks, the former one would be broken.\n\nOf course we could have some code that would detect that the whole \\S+\nis part of one thing that ends in g, but the complexity in\ndoing that would be equivalent or more to doing what my patch is\ndoing.\n\n>>> There's surely room for improvement here, but I just wanted to address\n>>> the very common case of sticking \"git describe\" output into commit\n>>> messages without trying to link to all possible refnames, that's going\n>>> to be a rather futile exercise given that this is free text, and it\n>>> would be prohibitively expensive to look up whether the references in\n>>> question exist in our repository.\n>>>\n>>> There was on-list discussion about how we could do better than this\n>>> patch. Junio suggested to update parse_commits() to call a new\n>>> \"gitweb--helper\" command which would pass each of the revision\n>>> candidates through \"rev-parse --verify --quiet\". That would cut down\n>>> on our false positives (e.g. we'll link to \"deadbeef\"), and also allow\n>>> us to be more aggressive in selecting candidate revisions.\n>>>\n>>> That may be too expensive to work in practice, or it may\n>>> not. Investigating that would be a good follow-up to this patch.\n>>>\n>>> Signed-off-by: Ævar Arnfjörð Bjarmason \n>>> ---\n>>> gitweb/gitweb.perl | 18 ++++++++++++++++--\n>>> 1 file changed, 16 insertions(+), 2 deletions(-)\n>>>\n>>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n>>> index 92b5e91..7cf68f0 100755\n>>> --- a/gitweb/gitweb.perl\n>>> +++ b/gitweb/gitweb.perl\n>>> @@ -2036,10 +2036,24 @@ sub format_log_line_html {\n>>> my $line = shift;\n>>>\n>>> $line = esc_html($line, -nbsp=>1);\n>>> - $line =~ s{\\b([0-9a-fA-F]{7,40})\\b}{\n>>> + $line =~ s{\n>>> + \\b\n>>> + (\n>>> + # The output of \"git describe\", e.g. v2.10.0-297-gf6727b0\n>>> + # or hadoop-20160921-113441-20-g094fb7d\n>>> + (?>> + [A-Za-z0-9.-]+\n>>> + (?!\\.) # refs can't end with \".\", see check_refname_format()\n>>> + -g[0-9a-fA-F]{7,40}\n>>> + |\n>>> + # Just a normal looking Git SHA1\n>>> + [0-9a-fA-F]{7,40}\n>>> + )\n>>> + \\b\n>>> + }{\n>>> $cgi->a({-href => href(action=>\"object\", hash=>$1),\n>>> -class => \"text\"}, $1);\n>>> - }eg;\n>>> + }egx;\n>>>\n>>> return $line;\n>>> }\n"},{"id":"304280","messageId":"xmqqbmyiykzz.fsf@gitster.mtv.corp.google.com","threadId":"44231","inReplyTo":"CACBZZX4FK+zrVyBTpbJAbSAxQ9LuCTXcExeEqZE6D3nwHbNxZA@mail.gmail.com","subject":"Re: [PATCH v2 2/3] gitweb: Link to 7-char+ SHA1s, not only 8-char+","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-17T16:54:08Z","receivedAt":"2016-10-17T16:55:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> As far as I can tell the only outstanding \"change this\" is your\n> s/SHA1/SHA-1/ in , do\n> you want to fix that up or should I submit another series?\n\nI think I did that already myself while queuing. Could you fetch\nwhat I queued on 'pu' to double check?\n\nI think the diff between what was posted and what is queued (I just\nchecked) looks like this:\n\n-gitweb: Link to 7-char+ SHA1s, not only 8-char+\n+gitweb: link to 7-char+ SHA-1s, not only 8-char+\n\n Change the minimum length of an abbreviated object identifier in the\n commit message gitweb tries to turn into link from 8 hexchars to 7.\n \n@@ -5,16 +12,18 @@\n SHA-1 in commit log message links to \"object\" view\", 2006-12-10), but\n the default abbreviation length is 7, and has been for a long time.\n \n-It's still possible to reference SHA1s down to 4 characters in length,\n+It's still possible to reference SHA-1s down to 4 characters in length,\n see v1.7.4-1-gdce9648's MINIMUM_ABBREV, but I can't see how to make\n git actually produce that, so I doubt anyone is putting that into log\n-messages in practice, but people definitely do put 7 character SHA1s\n+messages in practice, but people definitely do put 7 character SHA-1s\n into log messages.\n \n I think it's fairly dubious to link to things matching [0-9a-fA-F]\n here as opposed to just [0-9a-f], that dates back to the initial\n version of gitweb from 161332a (\"first working version\",\n-2005-08-07). Git will accept all-caps SHA1s, but didn't ever produce\n+2005-08-07). Git will accept all-caps SHA-1s, but didn't ever produce\n them as far as I can tell.\n \n Signed-off-by: Ævar Arnfjörð Bjarmason \n+Acked-by: Jakub Narębski \n+Signed-off-by: Junio C Hamano \n"},{"id":"304302","messageId":"CACBZZX5v79g=xMEQiWHNm8563+OaQb-L_cmr6VnoxevapHqesg@mail.gmail.com","threadId":"44231","inReplyTo":"xmqqbmyiykzz.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 2/3] gitweb: Link to 7-char+ SHA1s, not only 8-char+","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2016-10-17T19:46:21Z","receivedAt":"2016-10-17T19:47:07Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Mon, Oct 17, 2016 at 6:54 PM, Junio C Hamano wrote:\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> As far as I can tell the only outstanding \"change this\" is your\n>> s/SHA1/SHA-1/ in , do\n>> you want to fix that up or should I submit another series?\n>\n> I think I did that already myself while queuing. Could you fetch\n> what I queued on 'pu' to double check?\n\nThanks, looked at it, looks good to me!\n> I think the diff between what was posted and what is queued (I just\n> checked) looks like this:\n>\n> -gitweb: Link to 7-char+ SHA1s, not only 8-char+\n> +gitweb: link to 7-char+ SHA-1s, not only 8-char+\n>\n> Change the minimum length of an abbreviated object identifier in the\n> commit message gitweb tries to turn into link from 8 hexchars to 7.\n>\n> @@ -5,16 +12,18 @@\n> SHA-1 in commit log message links to \"object\" view\", 2006-12-10), but\n> the default abbreviation length is 7, and has been for a long time.\n>\n> -It's still possible to reference SHA1s down to 4 characters in length,\n> +It's still possible to reference SHA-1s down to 4 characters in length,\n> see v1.7.4-1-gdce9648's MINIMUM_ABBREV, but I can't see how to make\n> git actually produce that, so I doubt anyone is putting that into log\n> -messages in practice, but people definitely do put 7 character SHA1s\n> +messages in practice, but people definitely do put 7 character SHA-1s\n> into log messages.\n>\n> I think it's fairly dubious to link to things matching [0-9a-fA-F]\n> here as opposed to just [0-9a-f], that dates back to the initial\n> version of gitweb from 161332a (\"first working version\",\n> -2005-08-07). Git will accept all-caps SHA1s, but didn't ever produce\n> +2005-08-07). Git will accept all-caps SHA-1s, but didn't ever produce\n> them as far as I can tell.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> +Acked-by: Jakub Narębski \n> +Signed-off-by: Junio C Hamano \n"}]}