gitweb: Be smarter about linking to SHA1s in log messages

Change a typo'd MIME type in a comment. The Content-Type is application/xhtml+xml, not application/xhtm+xml.

Fixes up code originally added in 53c4031 ("gitweb: Strip non-printable characters from syntax highlighter output", 2011-09-16).

Signed-off-by Ævar Arnfjörð Bjarmason <[email protected]>

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

gitweb/gitweb.perl +1 -1

index 44094f4..cba7405 100755
@@ -1616,7 +1616,7 @@ sub esc_path {
 	return $str;
 }
 
-# Sanitize for use in XHTML + application/xml+xhtm (valid XML 1.0)
+# Sanitize for use in XHTML + application/xml+xhtml (valid XML 1.0)
 sub sanitize {
 	my $str = shift;
 

Change the minimum length of an abbreviated object identifier in the commit message gitweb tries to turn into link from 8 hexchars to 7.

This arbitrary minimum length of 8 was introduced in bfe2191 ("gitweb:
SHA-1 in commit log message links to "object" view", 2006-12-10), but the default abbreviation length is 7, and has been for a long time.

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 ("first working version", 2005-08-07). Git will accept all-caps SHA1s, but didn't ever produce them as far as I can tell.

Signed-off-by Ævar Arnfjörð Bjarmason <[email protected]>

---

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

gitweb/gitweb.perl +1 -1

index cba7405..92b5e91 100755
@@ -2036,7 +2036,7 @@ sub format_log_line_html {
 	my $line = shift;
 
 	$line = esc_html($line, -nbsp=>1);
-	$line =~ s{\b([0-9a-fA-F]{8,40})\b}{
+	$line =~ s{\b([0-9a-fA-F]{7,40})\b}{
 		$cgi->a({-href => href(action=>"object", hash=>$1),
 					-class => "text"}, $1);
 	}eg;

Change the minimum length of an abbreviated object identifier in the commit message gitweb tries to turn into link from 8 hexchars to 7.

This arbitrary minimum length of 8 was introduced in bfe2191 ("gitweb:
SHA-1 in commit log message links to "object" view", 2006-12-10), but the default abbreviation length is 7, and has been for a long time.

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 ("first working version", 2005-08-07). Git will accept all-caps SHA1s, but didn't ever produce them as far as I can tell.

Signed-off-by Ævar Arnfjörð Bjarmason <[email protected]>

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

gitweb/gitweb.perl +0 -0

index cba7405..92b5e91 100755

gitweb/gitweb.perl +0 -0

 		$cgi->a({-href => href(action=>"object", hash=>$1),
 					-class => "text"}, $1);
 	}eg;

s/SHA1/SHA-1/g in above paragraph (for correctness and consistency).

Quoted 6 lines

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 ("first working version", 2005-08-07). Git will accept all-caps SHA1s, but didn't ever produce them as far as I can tell.

All right. If we decide to be more strict in what we accept, we can do it in a separate commit.

Quoted 2 lines

Signed-off-by Ævar Arnfjörð Bjarmason <[email protected]>

Acked-by Jakub Narębski <[email protected]>

Quoted 10 lines

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

gitweb/gitweb.perl +0 -0

index cba7405..92b5e91 100755

gitweb/gitweb.perl sub format_log_line_html { · line 2039

Quoted 5 lines

gitweb/gitweb.perl +1 -1

@@ -2036,7 +2036,7 @@ sub format_log_line_html {
 	my $line = shift;
 
 	$line = esc_html($line, -nbsp=>1);
-	$line =~ s{\b([0-9a-fA-F]{8,40})\b}{
+	$line =~ s{\b([0-9a-fA-F]{7,40})\b}{

By the way, it is quite long commit message for one character change. Not that it is a bad thing...

Quoted 4 lines

gitweb/gitweb.perl +0 -0

 		$cgi->a({-href => href(action=>"object", hash=>$1),
 					-class => "text"}, $1);
 	}eg;

Quoted 12 lines · Jakub Narębski

s/SHA1/SHA-1/g in above paragraph (for correctness and consistency).

Quoted 6 lines

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 ("first working version", 2005-08-07). Git will accept all-caps SHA1s, but didn't ever produce them as far as I can tell.

All right. If we decide to be more strict in what we accept, we can do it in a separate commit.

Quoted 1 line

Quoted 3 lines

Quoted 1 line

Signed-off-by Ævar Arnfjörð Bjarmason <[email protected]>

Acked-by Jakub Narębski <[email protected]>

Thanks for a review. As the topic is not yet in 'next', I'll squish in your Acked-by: to them. I saw them only for 1 & 2/3; would another for 3/3 be coming soon?

Quoted 23 lines

Quoted 10 lines

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

gitweb/gitweb.perl +0 -0

index cba7405..92b5e91 100755

gitweb/gitweb.perl sub format_log_line_html { · line 2039

Quoted 5 lines

gitweb/gitweb.perl +1 -1

@@ -2036,7 +2036,7 @@ sub format_log_line_html {
     my $line = shift;
 
     $line = esc_html($line, -nbsp=>1);
-    $line =~ s{\b([0-9a-fA-F]{8,40})\b}{
+    $line =~ s{\b([0-9a-fA-F]{7,40})\b}{

By the way, it is quite long commit message for one character change. Not that it is a bad thing...

Quoted 4 lines

gitweb/gitweb.perl +0 -0

             $cgi->a({-href => href(action=>"object", hash=>$1),
                                     -class => "text"}, $1);
     }eg;

As far as I can tell the only outstanding "change this" is your s/SHA1/SHA-1/ in <[email protected]>, do you want to fix that up or should I submit another series?

Quoted 3 lines · Ævar Arnfjörð Bjarmason

As far as I can tell the only outstanding "change this" is your s/SHA1/SHA-1/ in <[email protected]>, do you want to fix that up or should I submit another series?

I think I did that already myself while queuing. Could you fetch what I queued on 'pu' to double check?

I think the diff between what was posted and what is queued (I just checked) looks like this:

(quoted diff) +6 -4

-gitweb: Link to 7-char+ SHA1s, not only 8-char+
+gitweb: link to 7-char+ SHA-1s, not only 8-char+
 
 Change the minimum length of an abbreviated object identifier in the
 commit message gitweb tries to turn into link from 8 hexchars to 7.

@@ -5,16 +12,18 @@
 SHA-1 in commit log message links to "object" view", 2006-12-10), but
 the default abbreviation length is 7, and has been for a long time.
 
-It's still possible to reference SHA1s down to 4 characters in length,
+It's still possible to reference SHA-1s 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
+messages in practice, but people definitely do put 7 character SHA-1s
 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 ("first working version",
-2005-08-07). Git will accept all-caps SHA1s, but didn't ever produce
+2005-08-07). Git will accept all-caps SHA-1s, but didn't ever produce
 them as far as I can tell.
 
 Signed-off-by: Ævar Arnfjörð Bjarmason <[email protected]>
+Acked-by: Jakub Narębski <[email protected]>
+Signed-off-by: Junio C Hamano <[email protected]>

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

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

There was on-list discussion about how we could do better than this patch. Junio suggested to update parse_commits() to call a new "gitweb--helper" command which would pass each of the revision candidates through "rev-parse --verify --quiet". That would cut down on our false positives (e.g. we'll link to "deadbeef"), and also allow us to be more aggressive in selecting candidate revisions.

That may be too expensive to work in practice, or it may not. Investigating that would be a good follow-up to this patch.

Signed-off-by Ævar Arnfjörð Bjarmason <[email protected]>

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

gitweb/gitweb.perl +16 -2

index 92b5e91..7cf68f0 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 8 lines · Ævar Arnfjörð Bjarmason

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

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 which won't be picked up by this regex.

Not a serious counter-proposal or suggestion, and certainly not an objection to the patch I am responding to, but I wonder if it is so bad if we made the 867ad08 into link when showing v2.8.0-4-g867ad08.

IOW, in addition to \b followed by [0-9a-f]+ followed by \b, if we allowed an optional 'g' in front of the hex, e.g.

(quoted diff) +1 -1

-       $line =~ s{\b([0-9a-fA-F]{7,40})\b}{
+       $line =~ s{\bg?([0-9a-fA-F]{7,40})\b}{

wouldn't that be much simpler, covers more cases and sufficient?

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

There was on-list discussion about how we could do better than this patch. Junio suggested to update parse_commits() to call a new "gitweb--helper" command which would pass each of the revision candidates through "rev-parse --verify --quiet". That would cut down on our false positives (e.g. we'll link to "deadbeef"), and also allow us to be more aggressive in selecting candidate revisions.

That may be too expensive to work in practice, or it may not. Investigating that would be a good follow-up to this patch.

Signed-off-by Ævar Arnfjörð Bjarmason <[email protected]>

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

gitweb/gitweb.perl +16 -2

index 92b5e91..7cf68f0 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 18 lines · Junio C Hamano

Quoted 8 lines · Ævar Arnfjörð Bjarmason

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

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 which won't be picked up by this regex.

Not a serious counter-proposal or suggestion, and certainly not an objection to the patch I am responding to, but I wonder if it is so bad if we made the 867ad08 into link when showing v2.8.0-4-g867ad08.

IOW, in addition to \b followed by [0-9a-f]+ followed by \b, if we allowed an optional 'g' in front of the hex, e.g.

Quoted 4 lines

(quoted diff) +1 -1

-       $line =~ s{\b([0-9a-fA-F]{7,40})\b}{
+       $line =~ s{\bg?([0-9a-fA-F]{7,40})\b}{

wouldn't that be much simpler, covers more cases and sufficient?

It would be more simpler, maybe that's the right approach. I have a preference for making the entire reference a link. I think it makes more sense for the UI.

Quoted 53 lines

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

There was on-list discussion about how we could do better than this patch. Junio suggested to update parse_commits() to call a new "gitweb--helper" command which would pass each of the revision candidates through "rev-parse --verify --quiet". That would cut down on our false positives (e.g. we'll link to "deadbeef"), and also allow us to be more aggressive in selecting candidate revisions.

That may be too expensive to work in practice, or it may not. Investigating that would be a good follow-up to this patch.

Signed-off-by Ævar Arnfjörð Bjarmason <[email protected]>

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

gitweb/gitweb.perl +16 -2

index 92b5e91..7cf68f0 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;
 }

I just ran into an example of a better reason for doing it like my patch is doing, which is that if you have some tag like:

deployment-20160928-171914-16-g42e13d8

With my patch the whole thing will be a link to the 42e13d8 commit, but with this suggestion both 20160928 and 42e13d8 would become commit links, the former one would be broken.

Of course we could have some code that would detect that the whole \S+ is part of one thing that ends in g<commit>,...

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

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

There was on-list discussion about how we could do better than this patch. Junio suggested to update parse_commits() to call a new "gitweb--helper" command which would pass each of the revision candidates through "rev-parse --verify --quiet". That would cut down on our false positives (e.g. we'll link to "deadbeef"), and also allow us to be more aggressive in selecting candidate revisions.

That may be too expensive to work in practice, or it may not. Investigating that would be a good follow-up to this patch.

Signed-off-by Ævar Arnfjörð Bjarmason <[email protected]>

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

gitweb/gitweb.perl +4 -1

index 92b5e91..7cf68f0 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

gitweb/gitweb.perl +2 -1

+    }{
 		$cgi->a({-href => href(action=>"object", hash=>$1),
 					-class => "text"}, $1);
-	}eg;
+	}egx;
 
 	return $line;
 }