Re: [PATCH] gitweb: ref markers link to named shortlogs

On Fri, 22 August 2008, Giuseppe Bilotta wrote:

On Fri, Aug 22, 2008 at 10:49 AM, Jakub Narebski [off-list ref] wrote:

quoted

What format_ref_marker() uses is not exactly 'type' of reference, but
more 'kind of' reference.  It is based on reference namespace, not
on type of object the reference is at (points to).  So code based
on this info (like your v3 patch) would fail on lightweight tag, i.e.
if there is ref in 'refs/tags' namespace which points directly to commit,
and not to tag object.

But 'git show-ref --dereference' _has_ information about whether
given reference points directly or indirectly to given object
($refs->{$id}), but currently we neither save it, nor use it.
For example we can have:

 781c1834f5419bdf81bb7f3750170ccd6b809174 refs/heads/maint
 ...
 124c62e8781a8f03ee0256bee78f7b392e3920af refs/stash
 ...
 89e6fcde639d65823e8113c307067441701ac74f refs/tags/Attic/gitweb/parse_rev_list
 b69a41a384d19fe253b9f4f34c9019ad96ca571d refs/tags/Attic/gitweb/patchset_body
 781c1834f5419bdf81bb7f3750170ccd6b809174 refs/tags/TEMP
 ...
 07cca3b30ee2b5d060e44e5b18d7c22929c63d1a refs/tags/v1.5.6.5
 781c1834f5419bdf81bb7f3750170ccd6b809174 refs/tags/v1.5.6.5^{}

Now in this example we have three refs pointing to commit object
781c1834: refs/heads/maint, refs/tags/TEMP and refs/tags/v1.5.6.5.
From those only refs/tags/v1.5.6.5 is (via) tag, even though TEMP
is in tags namespace.  Currently git_get_references() strips '^{}'
indirect reference marker from the output (from refname), and doesn't
make use of it.  One solution would be to not stip it in
git_get_references(), but leave it, and strip it and make use of
it (if ref ends with '^{}' it must be tag object) in format_ref_marker().

Ah, I see what you mean. If I understand correctly, this particular
situation is only a problem with tags, as they can be either
lighweight tags (that reference a commit) or actual tag objects (that
are indirect references to commits and direct references to
themselves), whereas everything else is just direct references to
object.

Yes, properly managed git repository should have refs pointing to
tag objects only in 'refs/tags' (tags) namespace.

Handling this requires a couple of extra info to be carried 
over in $refs, so I guess I'll have to experiment with it  a little
since it would require a more extensive change than I originally
planned.

This "couple of extra info" could be just '^{}' suffix.  So I don't
think it would be very complicated.

You could simply do not strip '^{}' suffix in git_get_references()
subroutine (so for example $refs->{$id} could be "tags/v1.6.0^{}",
and not simply "tags/v1.6.0" when $id is sha-1 of a _commit_ indirectly
referenced by v1.6.0, i.e. referenced by v1.6.0 _tag_), and strip
it and make use of it in format_ref_marker():

   if ($ref =~ s/\^\{\}$//) {
      # $ref is a tag
   } else {
      # $ref points directly to object
   }

HTH.

P.S. I have re-added git mailing list to Cc:.
-- 
Jakub Narebski
Poland

On Fri, Aug 22, 2008 at 12:56 PM, Jakub Narebski [off-list ref] wrote:

This "couple of extra info" could be just '^{}' suffix.  So I don't
think it would be very complicated.

You could simply do not strip '^{}' suffix in git_get_references()
subroutine (so for example $refs->{$id} could be "tags/v1.6.0^{}",
and not simply "tags/v1.6.0" when $id is sha-1 of a _commit_ indirectly
referenced by v1.6.0, i.e. referenced by v1.6.0 _tag_), and strip
it and make use of it in format_ref_marker():

  if ($ref =~ s/\^\{\}$//) {
     # $ref is a tag
  } else {
     # $ref points directly to object
  }

I've actually changed the format of $refs, making the values into
arrays whose first value is the name and the other is the ^{} marker,
if present. Patch incoming.

P.S. I have re-added git mailing list to Cc:.

Oops, didn't realized I had forgotten it.

-- 
Giuseppe "Oblomov" Bilotta

This patch turns ref markers for tags and heads into links to
appropriate views for the ref name. For annotated tags, we link to the
tag view, while shortlog is used for anything else.

Appropriate changes are made in the CSS to prevent ref markers from
being annoyingly blue and underlined, unless hovered. A visual
indication of the target view difference is also implemented by making
annotated tags show up in italic.

Signed-off-by: Giuseppe Bilotta <redacted>
---

This version collects the suggestions made by Jakub Narebski, including an
enhancement to git_get_references() to preserve information on the
nature of a tag, and the exploitation of this extra information to
differentiate between shortlog an tag view. Tag objects are also
made visually different from lightweight tags by use of italics.

 gitweb/gitweb.css  |   13 +++++++++++++
 gitweb/gitweb.perl |   23 +++++++++++++++++------
 2 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css
index 5f4a4b8..3e50060 100644
--- a/gitweb/gitweb.css
+++ b/gitweb/gitweb.css

@@ -491,6 +491,19 @@ span.refs span {
 	border-color: #ffccff #ff00ee #ff00ee #ffccff;
 }
 
+span.refs span a {
+	text-decoration: none;
+	color: inherit;
+}
+
+span.refs span a:hover {
+	text-decoration: underline;
+}
+
+span.refs span.indirect {
+	font-style: italic;
+}
+
 span.refs span.ref {
 	background-color: #aaaaff;
 	border-color: #ccccff #0033cc #0033cc #ccccff;

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index a0d9272..5cb332f 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl

@@ -1201,7 +1201,12 @@ sub format_ref_marker {
 	my $markers = '';
 
 	if (defined $refs->{$id}) {
-		foreach my $ref (@{$refs->{$id}}) {
+		foreach my $aref (@{$refs->{$id}}) {
+			# this code exploits the fact that non-lightweight tags are the
+			# only indirect objects, and that they are the only objects for which
+			# we want to use tag instead of shortlog as action
+			my $ref = $aref->[0];
+			my $indirect = $aref->[1];
 			my ($type, $name) = qw();
 			# e.g. tags/v2.6.11 or heads/next
 			if ($ref =~ m!^(.*?)s?/(.*)$!) {

@@ -1212,8 +1217,14 @@ sub format_ref_marker {
 				$name = $ref;
 			}
 
-			$markers .= " <span class=\"$type\" title=\"$ref\">" .
-			            esc_html($name) . "</span>";
+			my $class = $type;
+			if ($indirect) {
+				$class .= " indirect";
+			}
+
+			$markers .= " <span class=\"$class\" title=\"$ref\">" .
+				$cgi->a({-href => href(action=>( $indirect ? "tag" : "shortlog"), hash=>$ref)}, $name) .
+				"</span>";
 		}
 	}
 

@@ -2035,11 +2046,11 @@ sub git_get_references {
 
 	while (my $line = <$fd>) {
 		chomp $line;
-		if ($line =~ m!^([0-9a-fA-F]{40})\srefs/($type/?[^^]+)!) {
+		if ($line =~ m!^([0-9a-fA-F]{40})\srefs/($type/?[^^]+)(\^\{\})?$!) {
 			if (defined $refs{$1}) {
-				push @{$refs{$1}}, $2;
+				push @{$refs{$1}}, [$2, $3];
 			} else {
-				$refs{$1} = [ $2 ];
+				$refs{$1} = [ [$2, $3] ];
 			}
 		}
 	}

-- 
1.5.6.3

On Fri, 22 August 2008, Giuseppe Bilotta wrote:

Seems overly complicated.  How about something like this, instead?
It simply moves stripping ^{} from refs to format_ref_marker(),
and uses "tags/v1.5.0^{}" instead of [ "tags/v1.5.0", 1 ].

NOT TESTED!

 gitweb/gitweb.perl |   22 ++++++++++++++--------
 1 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 4c104d2..261307f 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl

@@ -1096,18 +1096,24 @@ sub format_ref_marker {
 
 	if (defined $refs->{$id}) {
 		foreach my $ref (@{$refs->{$id}}) {
-			my ($type, $name) = qw();
+			my ($class, $name, $indirect) = qw();
+			# strip ^{} suffix from indirect refs, like tags/v2.6.11^{}
+			$indirect = ($ref =~ s/\^{}$//);
 			# e.g. tags/v2.6.11 or heads/next
 			if ($ref =~ m!^(.*?)s?/(.*)$!) {
-				$type = $1;
-				$name = $2;
+				$class = $1;
+				$name  = $2;
 			} else {
-				$type = "ref";
-				$name = $ref;
+				$class = "ref";
+				$name  = $ref;
 			}
+			$class .= " indirect" if $indirect;
 
-			$markers .= " <span class=\"$type\" title=\"$ref\">" .
-			            esc_html($name) . "</span>";
+			$markers .= " <span class=\"$class\" title=\"$ref\">" .
+			            esc_html($name) .
+			            $cgi->a({-href => href(action=>($indirect ? "tag" : "shortlog"),
+			                                   hash=>$ref)}, $name) .
+			            "</span>";
 		}
 	}
 

@@ -1959,7 +1965,7 @@ sub git_get_references {
 
 	while (my $line = <$fd>) {
 		chomp $line;
-		if ($line =~ m!^([0-9a-fA-F]{40})\srefs/($type/?[^^]+)!) {
+		if ($line =~ m!^([0-9a-fA-F]{40})\srefs/($type/?.+)$!) {
 			if (defined $refs{$1}) {
 				push @{$refs{$1}}, $2;
 			} else {

-- 
Jakub Narebski
Poland

On Fri, Aug 22, 2008 at 3:01 PM, Jakub Narebski [off-list ref] wrote:

Seems overly complicated.  How about something like this, instead?
It simply moves stripping ^{} from refs to format_ref_marker(),
and uses "tags/v1.5.0^{}" instead of [ "tags/v1.5.0", 1 ].

My thought was that if the refs stuff was being used for something
else than format_ref_marker, it would fail more spectacularly due to
the different kind of values, rather than introducing potentially
subtle bugs due to the additiona ^{} popping in unexpectedly, but it
wouldn't be a problem to do it the simple way. Let me clean the patch
up and resubmit it.

-- 
Giuseppe "Oblomov" Bilotta

This patch turns ref markers for tags and heads into links to
appropriate views for the ref name. For annotated tags, we link to the
tag view, while shortlog is used for anything else.

Appropriate changes are made in the CSS to prevent ref markers from
being annoyingly blue and underlined, unless hovered. A visual
indication of the target view difference is also implemented by making
annotated tags show up in italic.

Signed-off-by: Giuseppe Bilotta <redacted>
---

As suggested by Jakub Narebski, the git_get_references() format
gets extended to include the final ^{} for tag objects, and the
name cleanup and indirection detection is moved to format_ref_marker()

 gitweb/gitweb.css  |   13 +++++++++++++
 gitweb/gitweb.perl |   16 +++++++++++++---
 2 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css
index 5f4a4b8..3e50060 100644
--- a/gitweb/gitweb.css
+++ b/gitweb/gitweb.css

@@ -491,6 +491,19 @@ span.refs span {
 	border-color: #ffccff #ff00ee #ff00ee #ffccff;
 }
 
+span.refs span a {
+	text-decoration: none;
+	color: inherit;
+}
+
+span.refs span a:hover {
+	text-decoration: underline;
+}
+
+span.refs span.indirect {
+	font-style: italic;
+}
+
 span.refs span.ref {
 	background-color: #aaaaff;
 	border-color: #ccccff #0033cc #0033cc #ccccff;

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index a0d9272..def4136 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl

@@ -1202,7 +1202,11 @@ sub format_ref_marker {
 
 	if (defined $refs->{$id}) {
 		foreach my $ref (@{$refs->{$id}}) {
+			# this code exploits the fact that non-lightweight tags are the
+			# only indirect objects, and that they are the only objects for which
+			# we want to use tag instead of shortlog as action
 			my ($type, $name) = qw();
+			my $indirect = ($ref =~ s/\^\{\}$//);
 			# e.g. tags/v2.6.11 or heads/next
 			if ($ref =~ m!^(.*?)s?/(.*)$!) {
 				$type = $1;

@@ -1212,8 +1216,14 @@ sub format_ref_marker {
 				$name = $ref;
 			}
 
-			$markers .= " <span class=\"$type\" title=\"$ref\">" .
-			            esc_html($name) . "</span>";
+			my $class = $type;
+			if ($indirect) {
+				$class .= " indirect";
+			}
+
+			$markers .= " <span class=\"$class\" title=\"$ref\">" .
+				$cgi->a({-href => href(action=>( $indirect ? "tag" : "shortlog"), hash=>$ref)}, $name) .
+				"</span>";
 		}
 	}
 

@@ -2035,7 +2045,7 @@ sub git_get_references {
 
 	while (my $line = <$fd>) {
 		chomp $line;
-		if ($line =~ m!^([0-9a-fA-F]{40})\srefs/($type/?[^^]+)!) {
+		if ($line =~ m!^([0-9a-fA-F]{40})\srefs/($type.*)$!) {
 			if (defined $refs{$1}) {
 				push @{$refs{$1}}, $2;
 			} else {

-- 
1.5.6.3

Giuseppe Bilotta wrote:

On Fri, Aug 22, 2008 at 3:01 PM, Jakub Narebski [off-list ref] wrote:

quoted

Seems overly complicated.  How about something like this, instead?
It simply moves stripping ^{} from refs to format_ref_marker(),
and uses "tags/v1.5.0^{}" instead of [ "tags/v1.5.0", 1 ].

My thought was that if the refs stuff was being used for something
else than format_ref_marker, it would fail more spectacularly due to
the different kind of values, rather than introducing potentially
subtle bugs due to the additiona ^{} popping in unexpectedly, but it
wouldn't be a problem to do it the simple way. Let me clean the patch
up and resubmit it.

This might be good idea, but for the two following reasons: a) it makes
code more complicated, b) it is inconsistent, because ref type would
be saved in refname then stripped in format_ref_marker, while indirection
would be saved in git_get_references; consistent would be 
["tags", "v1.5.0", 1].

Thanks for your work on improving gitweb.

-- 
Jakub Narebski
Poland

On Fri, Aug 22, 2008 at 3:42 PM, Jakub Narebski [off-list ref] wrote:

This might be good idea, but for the two following reasons: a) it makes
code more complicated, b) it is inconsistent, because ref type would
be saved in refname then stripped in format_ref_marker, while indirection
would be saved in git_get_references; consistent would be
["tags", "v1.5.0", 1].

Ah, I'll keep that in mind for future enhancements.

Thanks for your work on improving gitweb.

My pleasure, I actually have a long list of patches in store and I
hope to be able to push them through for the next release :)


-- 
Giuseppe "Oblomov" Bilotta

On Fri, 22 Aug 2008, Giuseppe Bilotta wrote:

This patch turns ref markers for tags and heads into links to
appropriate views for the ref name. For annotated tags, we link to the
tag view, while shortlog is used for anything else.

Appropriate changes are made in the CSS to prevent ref markers from
being annoyingly blue and underlined, unless hovered. A visual
indication of the target view difference is also implemented by making
annotated tags show up in italic.

Nice. I like it (read: Ack), with the following caveat

+			$markers .= " <span class=\"$class\" title=\"$ref\">" .
+				$cgi->a({-href => href(action=>( $indirect ? "tag" : "shortlog"), hash=>$ref)}, $name) .
+				"</span>";

We strip leading "refs/" in git_get_references(), so $ref does not
contain it. I'm not sure of one has to use refs/heads/aaa and refs/tags/aaa
to distinguish between tag and head with the same name, or heads/aaa and
tags/aaa is enough.

Also, the above line is bit long.
-- 
Jakub Narebski
Poland

On Mon, Aug 25, 2008 at 01:53:30AM +0200, Jakub Narebski [off-list ref] wrote:

We strip leading "refs/" in git_get_references(), so $ref does not
contain it. I'm not sure of one has to use refs/heads/aaa and refs/tags/aaa
to distinguish between tag and head with the same name, or heads/aaa and
tags/aaa is enough.

You can have both heads/master and refs/heads/master, then heads/master
is ambiguous.

Given that git fsck will not barf on such a configuration, I think
gitweb should handle such a case as well.

Miklos Vajna wrote:

On Mon, Aug 25, 2008 at 01:53:30AM +0200, Jakub Narebski [off-list ref] wrote:

quoted

We strip leading "refs/" in git_get_references(), so $ref does not
contain it. I'm not sure of one has to use refs/heads/aaa and refs/tags/aaa
to distinguish between tag and head with the same name, or heads/aaa and
tags/aaa is enough.

You can have both heads/master and refs/heads/master, then heads/master
is ambiguous.

Given that git fsck will not barf on such a configuration, I think
gitweb should handle such a case as well.

What I wanted to say was that I am not sure if current

+                     $markers .= " <span class=\"$class\" title=\"$ref\">" .
+                             $cgi->a({-href => href(action=>( $indirect ? "tag" : "shortlog"),
+                                                    hash=>$ref)}, $name) .
+                             "</span>";

is enough, or should gitweb use

+                     $markers .= " <span class=\"$class\" title=\"$ref\">" .
+                             $cgi->a({-href => href(action=>( $indirect ? "tag" : "shortlog"),
+                                                    hash=>"refs/$ref")}, $name) .
+                             "</span>";

or equivalent (not stripping "refs/" in git_get_references).

P.S. We are interested _only_ in refs shown by git-show-ref.
-- 
Jakub Narebski
Poland