{"thread":{"id":"44124","subject":"[PATCH 1/3] gitweb: Fix an ancient typo in v1.7.7-rc1-1-g0866786","startedAt":"2016-09-21T11:44:46Z","lastAt":"2016-09-21T20:10:25Z","messageCount":17,"participants":["Ævar Arnfjörð Bjarmason","Jakub Narębski","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"302263","messageId":"20160921114428.28664-1-avarab@gmail.com","threadId":"44124","inReplyTo":null,"subject":"[PATCH 1/3] gitweb: Fix an ancient typo in v1.7.7-rc1-1-g0866786","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2016-09-21T11:44:26Z","receivedAt":"2016-09-21T11:44:46Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The Content-Type is application/xhtml+xml, not application/xhtm+xml.\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 33d701d..9473daf 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.1.3\n\n"},{"id":"302264","messageId":"20160921114428.28664-2-avarab@gmail.com","threadId":"44124","inReplyTo":"20160921114428.28664-1-avarab@gmail.com","subject":"[PATCH 2/3] gitweb: Link to 7-character SHA1SUMS in commit messages","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2016-09-21T11:44:27Z","receivedAt":"2016-09-21T11:44:49Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the minimum length of a commit we'll link to from 8 to 7.\n\nThis arbitrary minimum length of 8 was introduced in\nv1.4.4.2-151-gbfe2191, but as seen in e.g. v1.7.4-1-gdce9648 the\ndefault abbreviation length is 7.\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. Git will accept all-caps SHA1s, but\ndidn't ever produce them as far as I can tell.\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 9473daf..101dbc0 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.1.3\n\n"},{"id":"302265","messageId":"20160921114428.28664-3-avarab@gmail.com","threadId":"44124","inReplyTo":"20160921114428.28664-1-avarab@gmail.com","subject":"[PATCH 3/3] gitweb: Link to \"git describe\"'d commits in log messages","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2016-09-21T11:44:28Z","receivedAt":"2016-09-21T11:44:52Z","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\nlike v2.8.0-4-g867ad08 in addition to just plain 867ad08.\n\nThis also fixes a micro-regression in my change of the minimum SHA1\nlength from 8 to 7, which is that dated tags like\nhadoop-20160921-113441-20-g094fb7d would start thinking the \"20160921\"\npart was a commit.\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 whatever 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---\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 101dbc0..3a52bc7 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+            (?<!-) # see strbuf_check_tag_ref(). Tags can't start with -\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-- \n2.1.3\n\n"},{"id":"302270","messageId":"d047a76d-8f13-5c90-3946-6821957d9278@gmail.com","threadId":"44124","inReplyTo":"20160921114428.28664-1-avarab@gmail.com","subject":"Re: [PATCH 1/3] gitweb: Fix an ancient typo in v1.7.7-rc1-1-g0866786","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2016-09-21T13:33:53Z","receivedAt":"2016-09-21T13:34:16Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"W dniu 21.09.2016 o 13:44, Ævar Arnfjörð Bjarmason napisał:\n\n> Subject: [PATCH 1/3] gitweb: Fix an ancient typo in v1.7.7-rc1-1-g0866786\n\nIt is more \"ancient typo from v1.7.7-rc1-1-g0866786\", but perhaps more\nimportant is \"ancient typo in a comment\"\n\n>\n> The Content-Type is application/xhtml+xml, not application/xhtm+xml.\n\nRight.  Thanks for the patch.\n\nSignoff?\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 33d701d..9473daf 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\nNb. I wonder how common is use of XHTML nowadays, with HTML5 as standard...\n\n>  sub sanitize {\n>  \tmy $str = shift;\n>  \n> \n\n"},{"id":"302272","messageId":"CACBZZX4QDTNYDpOF+GsK3tMQamoXQhJoXuPTgB3O+iEW3jBxag@mail.gmail.com","threadId":"44124","inReplyTo":"d047a76d-8f13-5c90-3946-6821957d9278@gmail.com","subject":"Re: [PATCH 1/3] gitweb: Fix an ancient typo in v1.7.7-rc1-1-g0866786","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2016-09-21T14:17:11Z","receivedAt":"2016-09-21T14:17:40Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Wed, Sep 21, 2016 at 3:33 PM, Jakub Narębski <jnareb@gmail.com> wrote:\n> W dniu 21.09.2016 o 13:44, Ævar Arnfjörð Bjarmason napisał:\n>\n>> Subject: [PATCH 1/3] gitweb: Fix an ancient typo in v1.7.7-rc1-1-g0866786\n>\n> It is more \"ancient typo from v1.7.7-rc1-1-g0866786\", but perhaps more\n> important is \"ancient typo in a comment\"\n\nYeah, will rephrase.\n\n>>\n>> The Content-Type is application/xhtml+xml, not application/xhtm+xml.\n>\n> Right.  Thanks for the patch.\n>\n> Signoff?\n\nBlast! I forgot that for these 3x patches. I'll re-submit pending\nfurther comments on the rest of the code changes in the series.\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 33d701d..9473daf 100755\n>> --- a/gitweb/gitweb.perl\n>> +++ b/gitweb/gitweb.perl\n>> @@ -1616,7 +1616,7 @@ sub esc_path {\n>>       return $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>\n> Nb. I wonder how common is use of XHTML nowadays, with HTML5 as standard...\n\nIt's sent to modern browsers, I noticed it because when doing the rest\nof the patches in the series the slightest mistake in the HTML syntax\nwould cause the page not to render in Chrome, because\napplication/xml+xhtm activates its anal parsing mode.\n\n>>  sub sanitize {\n>>       my $str = shift;\n>>\n>>\n>\n"},{"id":"302287","messageId":"64389bcb-c6e4-1d19-54a1-650868b9acb5@gmail.com","threadId":"44124","inReplyTo":"20160921114428.28664-2-avarab@gmail.com","subject":"Re: [PATCH 2/3] gitweb: Link to 7-character SHA1SUMS in commit messages","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2016-09-21T16:26:17Z","receivedAt":"2016-09-21T16:26:59Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"W dniu 21.09.2016 o 13:44, Ævar Arnfjörð Bjarmason napisał:\n\n> Subject: [PATCH 2/3] gitweb: Link to 7-character SHA1SUMS in commit messages\n\nThis is modification of a feature, not a new feature it sounds like.\nI think the following title / subject would be better:\n\n  Subject: [PATCH 2/3] gitweb: Link to 7-char+ SHA1s, not only 8-char+\n\n>\n> Change the minimum length of a commit we'll link to from 8 to 7.\n\nI think it would read better as:\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> \n> This arbitrary minimum length of 8 was introduced in\n> v1.4.4.2-151-gbfe2191, but as seen in e.g. v1.7.4-1-gdce9648 the\n> default abbreviation length is 7.\n\nRight. I wonder why it was 8 in gitweb...\n\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\nThere is an additional problem: the shorter SHA1 abbrev we try to\nmatch, the more possibility of false positives, words that only look\nlike (shortened SHA-1).\n\nFor 7 characters there is at last one word that can be mistaken\nfor SHA1 abbrev, namely 'deedeed' (hopefully rare in commit messages).\nFor 6 characters we have 'accede', 'beaded', 'decade' (!), 'deface',\n'facade' (!!), and possibly more (and of course all 7 character\nhexdigit words).\n\nAlso, the number of digits provided as an optional parameter to\n--abbrev or --abbrev-commit options is only a minimal number of \nhexdigits: Git would use as many as needed for the abbreviated SHA-1\nto be unambiguous, at current time.\n\n\nI think allowing 7-character shortened SHA-1, which is what Git\nproduces for smaller repositories by default is (might be?) a good\nidea.  Thanks for the patch.\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. Git will accept all-caps SHA1s, but\n> didn't ever produce them as far as I can tell.\n\nAll right, thanks for reminder.\n\nSignoff?\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 9473daf..101dbc0 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> \n\nNice and simple.\n\nP.S. I have reworking of commit message parsing and enhancement in my\nlong, long and dated gitweb TODO list :-(\n\nP.P.S. Kay Sievers no longer works on gitweb, and I think no longer\nworks at SuSE but at RedHat.\n\nBest,\n-- \nJakub Narębski\n"},{"id":"302291","messageId":"xmqqvaxp9oyp.fsf@gitster.mtv.corp.google.com","threadId":"44124","inReplyTo":"20160921114428.28664-3-avarab@gmail.com","subject":"Re: [PATCH 3/3] gitweb: Link to \"git describe\"'d commits in log messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-21T16:50:38Z","receivedAt":"2016-09-21T16:50:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\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\nWhen I saw 2/3 I wondered about one thing and 3/3 shares the same,\nwhich is that we only use regex match and do not validate for a\nfalse match.  Would it be too expensive to pick up what _looks_ like\na rev (e.g. hex or g(refname regexp)-hex) then validate it with\n\"rev-parse --verify --quiet\" to make sure it is a rev, before\nactually making it a link?  Even if are we trying to account for\npeople referring to commits that do not exist in this repository\n(e.g. some other project, in a submodule repository, or just an\nearlier incarnation of rebasing that has since been lost), it seems\nto me that it does not help to mark them with a link that won't\nresolve.\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 101dbc0..3a52bc7 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> +            (?<!-) # see strbuf_check_tag_ref(). Tags can't start with -\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":"302293","messageId":"fadd75f3-3737-1eaf-30f3-46a2ef132b27@gmail.com","threadId":"44124","inReplyTo":"20160921114428.28664-3-avarab@gmail.com","subject":"Re: [PATCH 3/3] gitweb: Link to \"git describe\"'d commits in log messages","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2016-09-21T17:09:27Z","receivedAt":"2016-09-21T17:09:52Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"W dniu 21.09.2016 o 13:44, Ævar Arnfjörð Bjarmason napisał:\n\n> Change the log formatting function to know about \"git describe\" output\n> like v2.8.0-4-g867ad08 in addition to just plain 867ad08.\n\nAll right, that is a good plan.\n\n> \n> This also fixes a micro-regression in my change of the minimum SHA1\n> length from 8 to 7, which is that dated tags like\n> hadoop-20160921-113441-20-g094fb7d would start thinking the \"20160921\"\n> part was a commit.\n\nActually 20160921 is 8 characters, so assuming that '-' is treated\nas word boundary by Perl, it is not a regression; this false positive\nwas there.  The new feature would help, instead of linking false match\nit links whole git-describe output.\n\nSo this paragraph needs to be changed wrt. the above.\n\nNote that there are quite a bit of shortened SHA-1 that are composed\nentirely from digits, without a-f characters.\n\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 whatever which won't be picked up by this regex.\n\nHopefully hierarchical tags are rare.  We need to reduce false\npositives.\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\nNote that we do not ask Git at the time of displaying commit message\nif the link is valid for performance reasons; we link it, and the link\nmay be invalid if it was a false positive.\n\nNote that recommended way to refer to other commit in commit mesages\nis (see Documentation/SubmittingPatches):\n\n  If you want to reference a previous commit in the history of a stable\n  branch, use the format \"abbreviated sha1 (subject, date)\",\n  with the subject enclosed in a pair of double-quotes, like this:\n\n     Commit f86a374 (\"pack-bitmap.c: fix a memleak\", 2015-03-30)\n     noticed that ...\n\nHmmm... this makes previous commit even more important.\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 101dbc0..3a52bc7 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\nAll right, for more complex regular expressions using in-line comments\n(extended regexp in Perl) is a good idea.\n\n> +            (?<!-) # see strbuf_check_tag_ref(). Tags can't start with -\n> +            [A-Za-z0-9.-]+\n> +            (?!\\.) # refs can't end with \".\", see check_refname_format()\n\nIf we can assume that tag name is at least two characters (instead of\nat least one character), we could get rid of those extended regexp\nlookaround assertions:\n\n  (?<!pattern) - zero-width negative lookbehind assertion\n  (?!pattern)  - zero-width negative lookahead  assertion\n\nThat is:\n\n  +            [A-Za-z0-9.]   # see strbuf_check_tag_ref(). Tags can't start with -\n  +            [A-Za-z0-9.-]*\n  +            [A-Za-z0-9-]   # refs can't end with \".\", see check_refname_format()\n\nAlso, the canonical documentation for what is allowed in refnames\nis git-check-ref-format(1)... though it does not look like it includes\n\"tags cannot start with '-'\".\n\nAnyway, perhaps 'is it valid refname' could be passed to a subroutine,\nor a named regexp (which might be more involved, like disallowing two\nconsecutive dots, e.g. \"(?!.*\\.{2})\" at beginning).\n\n> +            -g[0-9a-fA-F]{7,40}\n\nIf we are limiting to git-describe output, we can get rid of A-F here.\n\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> \n\nGood work. \n\nI assume that you are using git-describe output in commit messages\na lot, isn't it?\n-- \nJakub Narębski\n\n"},{"id":"302295","messageId":"8b43ba22-9634-e2b8-8de7-d70c258f4034@gmail.com","threadId":"44124","inReplyTo":"CACBZZX4QDTNYDpOF+GsK3tMQamoXQhJoXuPTgB3O+iEW3jBxag@mail.gmail.com","subject":"Re: [PATCH 1/3] gitweb: Fix an ancient typo in v1.7.7-rc1-1-g0866786","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2016-09-21T17:14:58Z","receivedAt":"2016-09-21T17:15:27Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"W dniu 21.09.2016 o 16:17, Ævar Arnfjörð Bjarmason napisał:\n> On Wed, Sep 21, 2016 at 3:33 PM, Jakub Narębski <jnareb@gmail.com> wrote:\n>> W dniu 21.09.2016 o 13:44, Ævar Arnfjörð Bjarmason napisał:\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>>\n>> Nb. I wonder how common is use of XHTML nowadays, with HTML5 as standard...\n> \n> It's sent to modern browsers, I noticed it because when doing the rest\n> of the patches in the series the slightest mistake in the HTML syntax\n> would cause the page not to render in Chrome, because\n> application/xml+xhtml activates its anal parsing mode.\n\nWhat I wanted to say is if we should support XHTML mimetype at all;\nthe future is HTML5 and perhaps gitweb should always use 'text/html'.\n\nBut this is unrelated to this change...\n-- \nJakub Narębski\n\n"},{"id":"302296","messageId":"CACBZZX73N4A0WBOG0G6DUg2fbn0mCmfuApcC6gG=6b-6-UrW7Q@mail.gmail.com","threadId":"44124","inReplyTo":"8b43ba22-9634-e2b8-8de7-d70c258f4034@gmail.com","subject":"Re: [PATCH 1/3] gitweb: Fix an ancient typo in v1.7.7-rc1-1-g0866786","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2016-09-21T17:17:48Z","receivedAt":"2016-09-21T17:18:15Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Wed, Sep 21, 2016 at 7:14 PM, Jakub Narębski <jnareb@gmail.com> wrote:\n> W dniu 21.09.2016 o 16:17, Ævar Arnfjörð Bjarmason napisał:\n>> On Wed, Sep 21, 2016 at 3:33 PM, Jakub Narębski <jnareb@gmail.com> wrote:\n>>> W dniu 21.09.2016 o 13:44, Ævar Arnfjörð Bjarmason napisał:\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>>>\n>>> Nb. I wonder how common is use of XHTML nowadays, with HTML5 as standard...\n>>\n>> It's sent to modern browsers, I noticed it because when doing the rest\n>> of the patches in the series the slightest mistake in the HTML syntax\n>> would cause the page not to render in Chrome, because\n>> application/xml+xhtml activates its anal parsing mode.\n>\n> What I wanted to say is if we should support XHTML mimetype at all;\n> the future is HTML5 and perhaps gitweb should always use 'text/html'.\n\nRegardless of what MIME type we'd normally use, as long as browsers\nsupport application/xml+xhtml developing with it is very handy,\nbecause you get the instant equivalent of compile errors for your\nHTML, as opposed to the usual behavior of \"oh this doesn't parse, but\nlet's try to make sense of it anyway\", which often leads to fruitless\ndebugging sessions just because you forgot to close some tag or\nquotation.\n"},{"id":"302301","messageId":"ba9d9c82-4b5e-4d62-95f1-a1c56cfc70af@gmail.com","threadId":"44124","inReplyTo":"xmqqvaxp9oyp.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/3] gitweb: Link to \"git describe\"'d commits in log messages","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2016-09-21T17:49:23Z","receivedAt":"2016-09-21T17:49:46Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"W dniu 21.09.2016 o 18:50, Junio C Hamano pisze:\n> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\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> When I saw 2/3 I wondered about one thing and 3/3 shares the same,\n> which is that we only use regex match and do not validate for a\n> false match.  Would it be too expensive to pick up what _looks_ like\n> a rev (e.g. hex or g(refname regexp)-hex) then validate it with\n> \"rev-parse --verify --quiet\" to make sure it is a rev, before\n> actually making it a link?  Even if are we trying to account for\n> people referring to commits that do not exist in this repository\n> (e.g. some other project, in a submodule repository, or just an\n> earlier incarnation of rebasing that has since been lost), it seems\n> to me that it does not help to mark them with a link that won't\n> resolve.\n\nI think it could be a good *option*, but revision verification\ncould be costly, for example in the 'log' view with multiple commits\nand multiple revision-like looking candidates, even if we were able\nto do it with one command.\n\nAlso, \"git rev-parse --verify [--quiet]\" can verify only one\nrevision at once, isn't it? Maybe something like 'git cat-file\n--batch-check' would be better (one fork)?\n\nIt's a matter of balance between false positives (and unresolving\nlinks) and performance...\n\nBest,\n-- \nJakub Narębski\n\n"},{"id":"302305","messageId":"CACBZZX5QF7FztmU-mqOEx40kC-PpQ9SqcL8+3HtuntcRZ1tWzA@mail.gmail.com","threadId":"44124","inReplyTo":"fadd75f3-3737-1eaf-30f3-46a2ef132b27@gmail.com","subject":"Re: [PATCH 3/3] gitweb: Link to \"git describe\"'d commits in log messages","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2016-09-21T17:58:18Z","receivedAt":"2016-09-21T17:58:43Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Wed, Sep 21, 2016 at 7:09 PM, Jakub Narębski <jnareb@gmail.com> wrote:\n> W dniu 21.09.2016 o 13:44, Ævar Arnfjörð Bjarmason napisał:\n>\n>> Change the log formatting function to know about \"git describe\" output\n>> like v2.8.0-4-g867ad08 in addition to just plain 867ad08.\n>\n> All right, that is a good plan.\n>\n>>\n>> This also fixes a micro-regression in my change of the minimum SHA1\n>> length from 8 to 7, which is that dated tags like\n>> hadoop-20160921-113441-20-g094fb7d would start thinking the \"20160921\"\n>> part was a commit.\n>\n> Actually 20160921 is 8 characters, so assuming that '-' is treated\n> as word boundary by Perl, it is not a regression; this false positive\n> was there.  The new feature would help, instead of linking false match\n> it links whole git-describe output.\n>\n> So this paragraph needs to be changed wrt. the above.\n\nYeah I just miscounted there, will change that.\n\n> Note that there are quite a bit of shortened SHA-1 that are composed\n> entirely from digits, without a-f characters.\n\n*nod*\n\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 whatever which won't be picked up by this regex.\n>\n> Hopefully hierarchical tags are rare.  We need to reduce false\n> positives.\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> Note that we do not ask Git at the time of displaying commit message\n> if the link is valid for performance reasons; we link it, and the link\n> may be invalid if it was a false positive.\n>\n> Note that recommended way to refer to other commit in commit mesages\n> is (see Documentation/SubmittingPatches):\n>\n>   If you want to reference a previous commit in the history of a stable\n>   branch, use the format \"abbreviated sha1 (subject, date)\",\n>   with the subject enclosed in a pair of double-quotes, like this:\n>\n>      Commit f86a374 (\"pack-bitmap.c: fix a memleak\", 2015-03-30)\n>      noticed that ...\n>\n> Hmmm... this makes previous commit even more important.\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 101dbc0..3a52bc7 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>\n> All right, for more complex regular expressions using in-line comments\n> (extended regexp in Perl) is a good idea.\n>\n>> +            (?<!-) # see strbuf_check_tag_ref(). Tags can't start with -\n>> +            [A-Za-z0-9.-]+\n>> +            (?!\\.) # refs can't end with \".\", see check_refname_format()\n>\n> If we can assume that tag name is at least two characters (instead of\n> at least one character), we could get rid of those extended regexp\n> lookaround assertions:\n>\n>   (?<!pattern) - zero-width negative lookbehind assertion\n>   (?!pattern)  - zero-width negative lookahead  assertion\n>\n> That is:\n>\n>   +            [A-Za-z0-9.]   # see strbuf_check_tag_ref(). Tags can't start with -\n>   +            [A-Za-z0-9.-]*\n>   +            [A-Za-z0-9-]   # refs can't end with \".\", see check_refname_format()\n\nWhy get rid of them? I'm all for improving the regex, there's bound to\nbe lots of bugs in it, but since it's perl we can freely use its\nextended features.\n\n> Also, the canonical documentation for what is allowed in refnames\n> is git-check-ref-format(1)... though it does not look like it includes\n> \"tags cannot start with '-'\".\n\nYeah, looks like that manpage needs to be patched.\n\n> Anyway, perhaps 'is it valid refname' could be passed to a subroutine,\n> or a named regexp (which might be more involved, like disallowing two\n> consecutive dots, e.g. \"(?!.*\\.{2})\" at beginning).\n>\n>> +            -g[0-9a-fA-F]{7,40}\n>\n> If we are limiting to git-describe output, we can get rid of A-F here.\n\nIndeed.\n\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>>\n>\n> Good work.\n>\n> I assume that you are using git-describe output in commit messages\n> a lot, isn't it?\n\nYeah, and I got tired of gitweb not linking to any of the commits I\nwas referencing.\n"},{"id":"302306","messageId":"xmqqy42l874n.fsf@gitster.mtv.corp.google.com","threadId":"44124","inReplyTo":"ba9d9c82-4b5e-4d62-95f1-a1c56cfc70af@gmail.com","subject":"Re: [PATCH 3/3] gitweb: Link to \"git describe\"'d commits in log messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-21T18:01:12Z","receivedAt":"2016-09-21T18:01:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narębski <jnareb@gmail.com> writes:\n\n>> When I saw 2/3 I wondered about one thing and 3/3 shares the same,\n>> which is that we only use regex match and do not validate for a\n>> false match.  Would it be too expensive...\n>\n> It's a matter of balance between false positives (and unresolving\n> links) and performance...\n\nYes, and that is why I asked a simple yes-or-no question.  Would it\nbe too expensive?  Your answer seems to be yes.\n\nHave we measured?  Is that really a bottleneck?  Would it help to\nupdate parse_commits to call a new command \"gitweb--helper\" that\nproduces the result of what git_print_log would have done to its\n$log argument, for example?\n\n"},{"id":"302307","messageId":"CACBZZX5mYuDUMHfurKEbCqodZkhYygQz+-G6VmKW+AMnCycm9g@mail.gmail.com","threadId":"44124","inReplyTo":"64389bcb-c6e4-1d19-54a1-650868b9acb5@gmail.com","subject":"Re: [PATCH 2/3] gitweb: Link to 7-character SHA1SUMS in commit messages","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2016-09-21T18:04:21Z","receivedAt":"2016-09-21T18:04:48Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Wed, Sep 21, 2016 at 6:26 PM, Jakub Narębski <jnareb@gmail.com> wrote:\n\n> P.S. I have reworking of commit message parsing and enhancement in my\n> long, long and dated gitweb TODO list :-(\n\nAnything specific you could share?\n\nOne thing that would be a lot faster in Perl is if we didn't have to\npass the log around as split-up lines and could just operate on it as\none big string.\n\nIt would make some code like git_print_log() a bit more complex /\nfragile, since it would have to work on multi-line strings, but\nanything that needed to do a regex match / replacement would be much\nfaster.\n\nBut OTOH I think perhaps we're worrying about nothing when it comes to\nthe performance. I haven't been able to make gitweb display more than\na 100 or so commits at a time (haven't found where exactly in the code\nthese limits are), any munging we do on the log messages would have to\nbe pretty damn slow to matter.\n\n> P.P.S. Kay Sievers no longer works on gitweb, and I think no longer\n> works at SuSE but at RedHat.\n\nYup, been getting bounces from his address.\n"},{"id":"302317","messageId":"88635d93-5288-c933-273a-003e5df824af@gmail.com","threadId":"44124","inReplyTo":"CACBZZX5mYuDUMHfurKEbCqodZkhYygQz+-G6VmKW+AMnCycm9g@mail.gmail.com","subject":"Re: [PATCH 2/3] gitweb: Link to 7-character SHA1SUMS in commit messages","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2016-09-21T18:28:25Z","receivedAt":"2016-09-21T18:28:49Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"W dniu 21.09.2016 o 20:04, Ævar Arnfjörð Bjarmason pisze:\n> On Wed, Sep 21, 2016 at 6:26 PM, Jakub Narębski <jnareb@gmail.com> wrote:\n> \n>> P.S. I have reworking of commit message parsing and enhancement in my\n>> long, long and dated gitweb TODO list :-(\n> \n> Anything specific you could share?\n\nSome of TODO I would have to bring from backups, as the computer on\nwhich I did majority of gitweb development has since died (from old\nage).\n\nThe list includes:\n- implement caching of gitweb output\n- revamp handling of encoding (UTF-8 with fallback encoding)\n- split gitweb into modules, while maintaining ease of install\n- refactor handling of diffs\n- better handling of config files\n- document URI structure, perhaps revamp URI parsing and generation\n- make commit message transformation generic\n  (see below)\n\n> \n> One thing that would be a lot faster in Perl is if we didn't have to\n> pass the log around as split-up lines and could just operate on it as\n> one big string.\n\nWell, there are a few transformations that commit message undergoes\nin gitweb, including linking SHA1, optional linking of bug numbers\nto bug tracker, and syntax highlighting of signoff lines (trailer\nlines).  \n\nI would like to have this cleaned up, and refactored.  With all\nthose transformations we would need to keep account which parts\nare HTML, and which not and need escaping (note: URI escape !=\nHTML escape).\n\n> \n> It would make some code like git_print_log() a bit more complex /\n> fragile, since it would have to work on multi-line strings, but\n> anything that needed to do a regex match / replacement would be much\n> faster.\n\nWould it?  Did you perform any synthetic micro-benchmark?\n\n> \n> But OTOH I think perhaps we're worrying about nothing when it comes to\n> the performance. I haven't been able to make gitweb display more than\n> a 100 or so commits at a time (haven't found where exactly in the code\n> these limits are), any munging we do on the log messages would have to\n> be pretty damn slow to matter.\n\nsub git_log_generic {\n\n\t# [...]\n\n\tmy @commitlist =\n\t\tparse_commits($commit_hash, 101, (100 * $page),\n\t\t              defined $file_name ? ($file_name, \"--full-history\") : ());\n\nHere you have it (it probably should be a constant; this number can be\nfound in a few other places).\n\nBest,\n-- \nJakub Narębski\n\n"},{"id":"302320","messageId":"f6cceb0a-9224-70ef-4592-ce34fe8704fd@gmail.com","threadId":"44124","inReplyTo":"CACBZZX5QF7FztmU-mqOEx40kC-PpQ9SqcL8+3HtuntcRZ1tWzA@mail.gmail.com","subject":"Re: [PATCH 3/3] gitweb: Link to \"git describe\"'d commits in log messages","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2016-09-21T19:13:06Z","receivedAt":"2016-09-21T19:13:29Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"W dniu 21.09.2016 o 19:58, Ævar Arnfjörð Bjarmason pisze:\n> On Wed, Sep 21, 2016 at 7:09 PM, Jakub Narębski <jnareb@gmail.com> wrote:\n>> W dniu 21.09.2016 o 13:44, Ævar Arnfjörð Bjarmason napisał:\n\n>>> +            (?<!-) # see strbuf_check_tag_ref(). Tags can't start with -\n>>> +            [A-Za-z0-9.-]+\n>>> +            (?!\\.) # refs can't end with \".\", see check_refname_format()\n>>\n>> If we can assume that tag name is at least two characters (instead of\n>> at least one character), we could get rid of those extended regexp\n>> lookaround assertions:\n>>\n>>   (?<!pattern) - zero-width negative lookbehind assertion\n>>   (?!pattern)  - zero-width negative lookahead  assertion\n>>\n>> That is:\n>>\n>>   +            [A-Za-z0-9.]   # see strbuf_check_tag_ref(). Tags can't start with -\n>>   +            [A-Za-z0-9.-]*\n>>   +            [A-Za-z0-9-]   # refs can't end with \".\", see check_refname_format()\n> \n> Why get rid of them? I'm all for improving the regex, there's bound to\n> be lots of bugs in it, but since it's perl we can freely use its\n> extended features.\n\nAh, all right. I was wondering how zero width assertions / patterns\ninteract with each other, but zero-width negative lookaround assertions\nare really quite simple.\n\n> \n>> Also, the canonical documentation for what is allowed in refnames\n>> is git-check-ref-format(1)... though it does not look like it includes\n>> \"tags cannot start with '-'\".\n> \n> Yeah, looks like that manpage needs to be patched.\n\nRight.\n\n> \n>> Anyway, perhaps 'is it valid refname' could be passed to a subroutine,\n>> or a named regexp (which might be more involved, like disallowing two\n>> consecutive dots, e.g. \"(?!.*\\.{2})\" at beginning).\n\nI wonder if rules for valid tag name can be described in extended\nregexp, and if it is, how readable would it be.\n\nRegards,\n-- \nJakub Narębski\n\n"},{"id":"302325","messageId":"CACBZZX5VkviTtaQnAVdAbZQ2m4-tYiA+AVC_6sZa90O7Z-bgYA@mail.gmail.com","threadId":"44124","inReplyTo":"88635d93-5288-c933-273a-003e5df824af@gmail.com","subject":"Re: [PATCH 2/3] gitweb: Link to 7-character SHA1SUMS in commit messages","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2016-09-21T20:09:58Z","receivedAt":"2016-09-21T20:10:25Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Wed, Sep 21, 2016 at 8:28 PM, Jakub Narębski <jnareb@gmail.com> wrote:\n> W dniu 21.09.2016 o 20:04, Ævar Arnfjörð Bjarmason pisze:\n>> It would make some code like git_print_log() a bit more complex /\n>> fragile, since it would have to work on multi-line strings, but\n>> anything that needed to do a regex match / replacement would be much\n>> faster.\n>\n> Would it?  Did you perform any synthetic micro-benchmark?\n\nNo, just experience. With the caveat that this may not matter at all\nin this context, C-like code in Perl is slow, if you can offload\nthings to one big regex operation it's usually faster.\n\n>>\n>> But OTOH I think perhaps we're worrying about nothing when it comes to\n>> the performance. I haven't been able to make gitweb display more than\n>> a 100 or so commits at a time (haven't found where exactly in the code\n>> these limits are), any munging we do on the log messages would have to\n>> be pretty damn slow to matter.\n>\n> sub git_log_generic {\n>\n>         # [...]\n>\n>         my @commitlist =\n>                 parse_commits($commit_hash, 101, (100 * $page),\n>                               defined $file_name ? ($file_name, \"--full-history\") : ());\n>\n> Here you have it (it probably should be a constant; this number can be\n> found in a few other places).\n\nThanks!\n"}]}