gitweb: Fix an ancient typo in v1.7.7-rc1-1-g0866786

Subject: [PATCH 2/3] gitweb: Link to 7-character SHA1SUMS in commit messages

Change the minimum length of a commit we'll link to from 8 to 7.

This arbitrary minimum length of 8 was introduced in v1.4.4.2-151-gbfe2191, but as seen in e.g. v1.7.4-1-gdce9648 the default abbreviation length is 7.

It's still possible to reference SHA1s down to 4 characters in length, see v1.7.4-1-gdce9648's MINIMUM_ABBREV, but I can't see how to make git actually produce that, so I doubt anyone is putting that into log messages in practice, but people definitely do put 7 character SHA1s into log messages.

I think it's fairly dubious to link to things matching [0-9a-fA-F] here as opposed to just [0-9a-f], that dates back to the initial version of gitweb from 161332a. Git will accept all-caps SHA1s, but didn't ever produce them as far as I can tell.

---
 gitweb/gitweb.perl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

gitweb/gitweb.perl +0 -0

index 9473daf..101dbc0 100755

P.S. I have reworking of commit message parsing and enhancement in my long, long and dated gitweb TODO list :-(

P.P.S. Kay Sievers no longer works on gitweb, and I think no longer works at SuSE but at RedHat.

Quoted 3 lines

P.S. I have reworking of commit message parsing and enhancement in my long, long and dated gitweb TODO list :-(

Anything specific you could share?

One thing that would be a lot faster in Perl is if we didn't have to pass the log around as split-up lines and could just operate on it as one big string.

It would make some code like git_print_log() a bit more complex / fragile, since it would have to work on multi-line strings, but anything that needed to do a regex match / replacement would be much faster.

But OTOH I think perhaps we're worrying about nothing when it comes to the performance. I haven't been able to make gitweb display more than a 100 or so commits at a time (haven't found where exactly in the code these limits are), any munging we do on the log messages would have to be pretty damn slow to matter.

W dniu 21.09.2016 o 20:04, Ævar Arnfjörð Bjarmason pisze:

Quoted 4 lines

It would make some code like git_print_log() a bit more complex / fragile, since it would have to work on multi-line strings, but anything that needed to do a regex match / replacement would be much faster.

Would it? Did you perform any synthetic micro-benchmark?

Quoted 6 lines

But OTOH I think perhaps we're worrying about nothing when it comes to the performance. I haven't been able to make gitweb display more than a 100 or so commits at a time (haven't found where exactly in the code these limits are), any munging we do on the log messages would have to be pretty damn slow to matter.

sub git_log_generic {

        # [...]

        my @commitlist =
                parse_commits($commit_hash, 101, (100 * $page),
                              defined $file_name ? ($file_name, "--full-history") : ());

Here you have it (it probably should be a constant; this number can be found in a few other places).

There's surely room for improvement here, but I just wanted to address the very common case of sticking "git describe" output into commit messages without trying to link to all possible refnames, that's going to be a rather futile exercise given that this is free text, and it would be prohibitively expensive to look up whether the references in question exist in our repository.

---
 gitweb/gitweb.perl | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

gitweb/gitweb.perl +16 -2

index 101dbc0..3a52bc7 100755
@@ -2036,10 +2036,24 @@ sub format_log_line_html {
 	my $line = shift;
 
 	$line = esc_html($line, -nbsp=>1);
-	$line =~ s{\b([0-9a-fA-F]{7,40})\b}{
+	$line =~ s{
+        \b
+        (
+            # The output of "git describe", e.g. v2.10.0-297-gf6727b0
+            # or hadoop-20160921-113441-20-g094fb7d
+            (?<!-) # see strbuf_check_tag_ref(). Tags can't start with -
+            [A-Za-z0-9.-]+
+            (?!\.) # refs can't end with ".", see check_refname_format()
+            -g[0-9a-fA-F]{7,40}
+            |
+            # Just a normal looking Git SHA1
+            [0-9a-fA-F]{7,40}
+        )
+        \b
+    }{
 		$cgi->a({-href => href(action=>"object", hash=>$1),
 					-class => "text"}, $1);
-	}eg;
+	}egx;
 
 	return $line;
 }

Quoted 6 lines · Ævar Arnfjörð Bjarmason

There's surely room for improvement here, but I just wanted to address the very common case of sticking "git describe" output into commit messages without trying to link to all possible refnames, that's going to be a rather futile exercise given that this is free text, and it would be prohibitively expensive to look up whether the references in question exist in our repository.

When I saw 2/3 I wondered about one thing and 3/3 shares the same, which is that we only use regex match and do not validate for a false match. Would it be too expensive to pick up what _looks_ like a rev (e.g. hex or g(refname regexp)-hex) then validate it with "rev-parse --verify --quiet" to make sure it is a rev, before actually making it a link? Even if are we trying to account for people referring to commits that do not exist in this repository (e.g. some other project, in a submodule repository, or just an earlier incarnation of rebasing that has since been lost), it seems to me that it does not help to mark them with a link that won't resolve.

Quoted 3 lines

When I saw 2/3 I wondered about one thing and 3/3 shares the same, which is that we only use regex match and do not validate for a false match. Would it be too expensive...

It's a matter of balance between false positives (and unresolving links) and performance...

Change the log formatting function to know about "git describe" output like v2.8.0-4-g867ad08 in addition to just plain 867ad08.

This also fixes a micro-regression in my change of the minimum SHA1 length from 8 to 7, which is that dated tags like hadoop-20160921-113441-20-g094fb7d would start thinking the "20160921" part was a commit.

There are still many valid refnames that we don't link to e.g. v2.10.0-rc1~2^2~1 is also a valid way to refer to v2.8.0-4-g867ad08, but I'm not supporting that with this commit, similarly it's trivially possible to create some refnames like "æ/var-gf6727b0" or whatever which won't be picked up by this regex.

There's surely room for improvement here, but I just wanted to address the very common case of sticking "git describe" output into commit messages without trying to link to all possible refnames, that's going to be a rather futile exercise given that this is free text, and it would be prohibitively expensive to look up whether the references in question exist in our repository.

---
 gitweb/gitweb.perl | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

gitweb/gitweb.perl +0 -0

index 101dbc0..3a52bc7 100755

gitweb/gitweb.perl +3 -0

+            |
+            # Just a normal looking Git SHA1
+            [0-9a-fA-F]{7,40}

Quoted 2 lines · Ævar Arnfjörð Bjarmason

Change the log formatting function to know about "git describe" output like v2.8.0-4-g867ad08 in addition to just plain 867ad08.

All right, that is a good plan.

Quoted 5 lines

This also fixes a micro-regression in my change of the minimum SHA1 length from 8 to 7, which is that dated tags like hadoop-20160921-113441-20-g094fb7d would start thinking the "20160921" part was a commit.

Actually 20160921 is 8 characters, so assuming that '-' is treated as word boundary by Perl, it is not a regression; this false positive was there. The new feature would help, instead of linking false match it links whole git-describe output.

So this paragraph needs to be changed wrt. the above.

Note that there are quite a bit of shortened SHA-1 that are composed entirely from digits, without a-f characters.

Quoted 6 lines

There are still many valid refnames that we don't link to e.g. v2.10.0-rc1~2^2~1 is also a valid way to refer to v2.8.0-4-g867ad08, but I'm not supporting that with this commit, similarly it's trivially possible to create some refnames like "æ/var-gf6727b0" or whatever which won't be picked up by this regex.

Hopefully hierarchical tags are rare. We need to reduce false positives.

Quoted 7 lines

There's surely room for improvement here, but I just wanted to address the very common case of sticking "git describe" output into commit messages without trying to link to all possible refnames, that's going to be a rather futile exercise given that this is free text, and it would be prohibitively expensive to look up whether the references in question exist in our repository.

Note that we do not ask Git at the time of displaying commit message if the link is valid for performance reasons; we link it, and the link may be invalid if it was a false positive.

Note that recommended way to refer to other commit in commit mesages is (see Documentation/SubmittingPatches):

If you want to reference a previous commit in the history of a stable branch, use the format "abbreviated sha1 (subject, date)", with the subject enclosed in a pair of double-quotes, like this:

Commit f86a374 ("pack-bitmap.c: fix a memleak", 2015-03-30) noticed that ...

Hmmm... this makes previous commit even more important.

Quoted 9 lines

---
 gitweb/gitweb.perl | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

gitweb/gitweb.perl +0 -0

index 101dbc0..3a52bc7 100755

gitweb/gitweb.perl sub format_log_line_html { · line 2043

Quoted 9 lines

gitweb/gitweb.perl +5 -1

@@ -2036,10 +2036,24 @@ sub format_log_line_html {
      my $line = shift;
 
      $line = esc_html($line, -nbsp=>1);
-     $line =~ s{\b([0-9a-fA-F]{7,40})\b}{
+     $line =~ s{
+        \b
+        (
+            # The output of "git describe", e.g. v2.10.0-297-gf6727b0
+            # or hadoop-20160921-113441-20-g094fb7d

All right, for more complex regular expressions using in-line comments (extended regexp in Perl) is a good idea.

gitweb/gitweb.perl sub format_log_line_html { · line 2046

Quoted 3 lines

gitweb/gitweb.perl +3 -0

+            (?<!-) # see strbuf_check_tag_ref(). Tags can't start with -
+            [A-Za-z0-9.-]+
+            (?!\.) # refs can't end with ".", see check_refname_format()

If we can assume that tag name is at least two characters (instead of at least one character), we could get rid of those extended regexp lookaround assertions:

(?<!pattern) - zero-width negative lookbehind assertion (?!pattern) - zero-width negative lookahead assertion

That is:

  +            [A-Za-z0-9.]   # see strbuf_check_tag_ref(). Tags can't start with -
  +            [A-Za-z0-9.-]*
  +            [A-Za-z0-9-]   # refs can't end with ".", see check_refname_format()

Also, the canonical documentation for what is allowed in refnames is git-check-ref-format(1)... though it does not look like it includes "tags cannot start with '-'".

Anyway, perhaps 'is it valid refname' could be passed to a subroutine, or a named regexp (which might be more involved, like disallowing two consecutive dots, e.g. "(?!.*\.{2})" at beginning).

gitweb/gitweb.perl sub format_log_line_html { · line 2047

Quoted 1 line

gitweb/gitweb.perl +1 -0

+            -g[0-9a-fA-F]{7,40}

If we are limiting to git-describe output, we can get rid of A-F here.

Quoted 11 lines

gitweb/gitweb.perl +7 -1

+            |
+            # Just a normal looking Git SHA1
+            [0-9a-fA-F]{7,40}
+        )
+        \b
+    }{
              $cgi->a({-href => href(action=>"object", hash=>$1),
                                      -class => "text"}, $1);
-     }eg;
+     }egx;
 

gitweb/gitweb.perl sub format_log_line_html { · line 2059

Quoted 3 lines

gitweb/gitweb.perl +0 -0

      return $line;
 }

Good work.

I assume that you are using git-describe output in commit messages a lot, isn't it?

Quoted 15 lines

gitweb/gitweb.perl sub format_log_line_html { · line 2046

Quoted 3 lines · Ævar Arnfjörð Bjarmason

gitweb/gitweb.perl +3 -0

+            (?<!-) # see strbuf_check_tag_ref(). Tags can't start with -
+            [A-Za-z0-9.-]+
+            (?!\.) # refs can't end with ".", see check_refname_format()

If we can assume that tag name is at least two characters (instead of at least one character), we could get rid of those extended regexp lookaround assertions:

(?<!pattern) - zero-width negative lookbehind assertion (?!pattern) - zero-width negative lookahead assertion

That is:

Quoted 3 lines

  +            [A-Za-z0-9.]   # see strbuf_check_tag_ref(). Tags can't start with -
  +            [A-Za-z0-9.-]*
  +            [A-Za-z0-9-]   # refs can't end with ".", see check_refname_format()

Why get rid of them? I'm all for improving the regex, there's bound to be lots of bugs in it, but since it's perl we can freely use its extended features.

Quoted 4 lines

Also, the canonical documentation for what is allowed in refnames is git-check-ref-format(1)... though it does not look like it includes "tags cannot start with '-'".

Yeah, looks like that manpage needs to be patched.

Quoted 4 lines

Anyway, perhaps 'is it valid refname' could be passed to a subroutine, or a named regexp (which might be more involved, like disallowing two consecutive dots, e.g. "(?!.*\.{2})" at beginning).

Subject: [PATCH 1/3] gitweb: Fix an ancient typo in v1.7.7-rc1-1-g0866786

The Content-Type is application/xhtml+xml, not application/xhtm+xml.

---
 gitweb/gitweb.perl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

gitweb/gitweb.perl +0 -0

index 33d701d..9473daf 100755

gitweb/gitweb.perl +0 -0

 sub sanitize {
 	my $str = shift;
 

Quoted 1 line · Ævar Arnfjörð Bjarmason

Subject: [PATCH 1/3] gitweb: Fix an ancient typo in v1.7.7-rc1-1-g0866786

It is more "ancient typo from v1.7.7-rc1-1-g0866786", but perhaps more important is "ancient typo in a comment"

Quoted 2 lines

The Content-Type is application/xhtml+xml, not application/xhtm+xml.

Right. Thanks for the patch.

Signoff?

Quoted 12 lines

---
 gitweb/gitweb.perl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

gitweb/gitweb.perl +0 -0

index 33d701d..9473daf 100755
@@ -1616,7 +1616,7 @@ sub esc_path {
      return $str;
 }
 

gitweb/gitweb.perl sub esc_path { · line 1619

Quoted 2 lines

gitweb/gitweb.perl +1 -1

-# Sanitize for use in XHTML + application/xml+xhtm (valid XML 1.0)
+# Sanitize for use in XHTML + application/xml+xhtml (valid XML 1.0)

Nb. I wonder how common is use of XHTML nowadays, with HTML5 as standard...

Quoted 4 lines

gitweb/gitweb.perl +0 -0

 sub sanitize {
      my $str = shift;
 

Quoted 1 line · Jakub Narębski

W dniu 21.09.2016 o 13:44, Ævar Arnfjörð Bjarmason napisał:

Quoted 4 lines

gitweb/gitweb.perl sub esc_path { · line 1619

Quoted 2 lines

gitweb/gitweb.perl +1 -1

-# Sanitize for use in XHTML + application/xml+xhtm (valid XML 1.0)
+# Sanitize for use in XHTML + application/xml+xhtml (valid XML 1.0)

Nb. I wonder how common is use of XHTML nowadays, with HTML5 as standard...

It's sent to modern browsers, I noticed it because when doing the rest of the patches in the series the slightest mistake in the HTML syntax would cause the page not to render in Chrome, because application/xml+xhtml activates its anal parsing mode.

Quoted 2 lines · Ævar Arnfjörð Bjarmason

Quoted 1 line · Jakub Narębski

W dniu 21.09.2016 o 13:44, Ævar Arnfjörð Bjarmason napisał:

[...]

Quoted 5 lines

Quoted 4 lines

gitweb/gitweb.perl sub esc_path { · line 1619

Quoted 2 lines

gitweb/gitweb.perl +1 -1

-# Sanitize for use in XHTML + application/xml+xhtm (valid XML 1.0)
+# Sanitize for use in XHTML + application/xml+xhtml (valid XML 1.0)

Nb. I wonder how common is use of XHTML nowadays, with HTML5 as standard...

Quoted 4 lines

It's sent to modern browsers, I noticed it because when doing the rest of the patches in the series the slightest mistake in the HTML syntax would cause the page not to render in Chrome, because application/xml+xhtml activates its anal parsing mode.

What I wanted to say is if we should support XHTML mimetype at all; the future is HTML5 and perhaps gitweb should always use 'text/html'.