[PATCH] gitweb: ref markers link to named shortlogs

[PATCH] gitweb: ref markers link to named shortlogs

[Giuseppe Bilotta]

This patch turns ref markers for tags and heads into links to
shortlog/refname. Appropriate changes are made in the CSS to prevent ref
markers to be annoyingly blue and underlined.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 gitweb/gitweb.css  |    5 +++++
 gitweb/gitweb.perl |    5 ++++-
 2 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css
index aa0eeca..2b43eea 100644
--- a/gitweb/gitweb.css
+++ b/gitweb/gitweb.css
@@ -481,6 +481,11 @@ span.refs span {
 	border-color: #ffccff #ff00ee #ff00ee #ffccff;
 }
 
+span.refs span a {
+	text-decoration: none;
+	color: inherit;
+}
+
 span.refs span.ref {
 	background-color: #aaaaff;
 	border-color: #ccccff #0033cc #0033cc #ccccff;
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 90cd99b..7f391fa 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1093,6 +1093,7 @@ sub format_log_line_html {
 sub format_ref_marker {
 	my ($refs, $id) = @_;
 	my $markers = '';
+	my $htmltext;
 
 	if (defined $refs->{$id}) {
 		foreach my $ref (@{$refs->{$id}}) {
@@ -1101,13 +1102,15 @@ sub format_ref_marker {
 			if ($ref =~ m!^(.*?)s?/(.*)$!) {
 				$type = $1;
 				$name = $2;
+				$htmltext = $cgi->a({-href => href(action=>"shortlog", hash=>$name)}, $name);
 			} else {
 				$type = "ref";
 				$name = $ref;
+				$htmltext = esc_html($name);
 			}
 
 			$markers .= " <span class=\"$type\" title=\"$ref\">" .
-			            esc_html($name) . "</span>";
+			            $htmltext . "</span>";
 		}
 	}
 
-- 
1.5.6.3

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

[Petr Baudis]

On Sat, Aug 02, 2008 at 05:39:14PM +0200, Giuseppe Bilotta wrote:
> This patch turns ref markers for tags and heads into links to
> shortlog/refname. Appropriate changes are made in the CSS to prevent ref
> markers to be annoyingly blue and underlined.
> 
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>

I think this is good idea in principle, but

	(i) Why not do this for all the refs?

	(ii) I think you should decide on the type of the action based
on the object type of the ref; actually, any kind of object type can be
ref'd, and for tags you would rather want tag view, etc. (The tag view
actually sucks and should behave more like git show tag - i.e. append
the appropriate view after the tag info - but that is different matter.)

-- 
				Petr "Pasky" Baudis
The next generation of interesting software will be done
on the Macintosh, not the IBM PC.  -- Bill Gates

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

[Giuseppe Bilotta]

On Sun, Aug 3, 2008 at 2:03 PM, Petr Baudis <pasky@suse.cz> wrote:
> On Sat, Aug 02, 2008 at 05:39:14PM +0200, Giuseppe Bilotta wrote:
>> This patch turns ref markers for tags and heads into links to
>> shortlog/refname. Appropriate changes are made in the CSS to prevent ref
>> markers to be annoyingly blue and underlined.
>>
>> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
>
> I think this is good idea in principle, but
>
>        (i) Why not do this for all the refs?
>
>        (ii) I think you should decide on the type of the action based
> on the object type of the ref; actually, any kind of object type can be
> ref'd, and for tags you would rather want tag view, etc. (The tag view
> actually sucks and should behave more like git show tag - i.e. append
> the appropriate view after the tag info - but that is different matter.)

Funny that. My original plan was to have a different action depending
on tag (I tried shortlog for tag and commitdiff for branch). And since
I I had no idea what kind of action to use for 'generic' refs, I left
them out. Then I had second thoughts and started using shortlog for
both heads and tags, and collapsed the code but still kept the generic
refs out of the way. So maybe we can use shortlog as default action
and single out tags (and whatever else we'll find to need a different
action)?



-- 
Giuseppe "Oblomov" Bilotta

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

[Petr Baudis]

On Sun, Aug 03, 2008 at 03:14:09PM +0200, Giuseppe Bilotta wrote:
> On Sun, Aug 3, 2008 at 2:03 PM, Petr Baudis <pasky@suse.cz> wrote:
> >        (ii) I think you should decide on the type of the action based
> > on the object type of the ref; actually, any kind of object type can be
> > ref'd, and for tags you would rather want tag view, etc. (The tag view
> > actually sucks and should behave more like git show tag - i.e. append
> > the appropriate view after the tag info - but that is different matter.)
> 
> Funny that. My original plan was to have a different action depending
> on tag (I tried shortlog for tag and commitdiff for branch). And since
> I I had no idea what kind of action to use for 'generic' refs, I left
> them out. Then I had second thoughts and started using shortlog for
> both heads and tags, and collapsed the code but still kept the generic
> refs out of the way. So maybe we can use shortlog as default action
> and single out tags (and whatever else we'll find to need a different
> action)?

What's wrong with my proposed approach to choose actoin based on object
type of the ref?

-- 
				Petr "Pasky" Baudis
The next generation of interesting software will be done
on the Macintosh, not the IBM PC.  -- Bill Gates

[PATCHv2] gitweb: ref markers link to named shortlogs

[Giuseppe Bilotta]

This patch turns ref markers for tags and heads into links to
appropriate views for the ref name. Appropriate changes are made in the
CSS to prevent ref markers to be annoyingly blue and underlined.

For all git ref types it's assumed that the preferred view is named like
the ref type itself. For commits, we map the view to shortlog.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---

Following pasky's suggestion, the action used by the ref marker
is now determined from the ref type, default action to the ref type
itsef. Currently the only non-default action is shortlog for commit
refs, but the map can be expanded as needed.

 gitweb/gitweb.css  |    5 +++++
 gitweb/gitweb.perl |    7 ++++++-
 2 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css
index aa0eeca..2b43eea 100644
--- a/gitweb/gitweb.css
+++ b/gitweb/gitweb.css
@@ -481,6 +481,11 @@ span.refs span {
 	border-color: #ffccff #ff00ee #ff00ee #ffccff;
 }
 
+span.refs span a {
+	text-decoration: none;
+	color: inherit;
+}
+
 span.refs span.ref {
 	background-color: #aaaaff;
 	border-color: #ccccff #0033cc #0033cc #ccccff;
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 90cd99b..a12ce87 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1093,10 +1093,14 @@ sub format_log_line_html {
 sub format_ref_marker {
 	my ($refs, $id) = @_;
 	my $markers = '';
+	my %view = (
+		"commit" => "shortlog",
+	);
 
 	if (defined $refs->{$id}) {
 		foreach my $ref (@{$refs->{$id}}) {
 			my ($type, $name) = qw();
+			my $git_type = git_get_type($ref);
 			# e.g. tags/v2.6.11 or heads/next
 			if ($ref =~ m!^(.*?)s?/(.*)$!) {
 				$type = $1;
@@ -1107,7 +1111,8 @@ sub format_ref_marker {
 			}
 
 			$markers .= " <span class=\"$type\" title=\"$ref\">" .
-			            esc_html($name) . "</span>";
+				$cgi->a({-href => href(action=>$view{$git_type} || $git_type, hash=>$name)}, $name) .
+				"</span>";
 		}
 	}
 
-- 
1.5.6.3

[PATCH] gitweb: ref markers link to named shortlogs

[Giuseppe Bilotta]

This patch turns ref markers for tags and heads into links to
appropriate views for the ref name. Appropriate changes are made in the
CSS to prevent ref markers to be annoyingly blue and underlined.

For all git ref types it's assumed that the preferred view is named like
the ref type itself. For commits, we map the view to shortlog.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---

This is a resend of version 2 of the patch, as it seems to have
dropped into oblivion without ACKs or NACKs.

 gitweb/gitweb.css  |    5 +++++
 gitweb/gitweb.perl |    7 ++++++-
 2 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css
index aa0eeca..2b43eea 100644
--- a/gitweb/gitweb.css
+++ b/gitweb/gitweb.css
@@ -481,6 +481,11 @@ span.refs span {
 	border-color: #ffccff #ff00ee #ff00ee #ffccff;
 }
 
+span.refs span a {
+	text-decoration: none;
+	color: inherit;
+}
+
 span.refs span.ref {
 	background-color: #aaaaff;
 	border-color: #ccccff #0033cc #0033cc #ccccff;
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 90cd99b..a12ce87 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1093,10 +1093,14 @@ sub format_log_line_html {
 sub format_ref_marker {
 	my ($refs, $id) = @_;
 	my $markers = '';
+	my %view = (
+		"commit" => "shortlog",
+	);
 
 	if (defined $refs->{$id}) {
 		foreach my $ref (@{$refs->{$id}}) {
 			my ($type, $name) = qw();
+			my $git_type = git_get_type($ref);
 			# e.g. tags/v2.6.11 or heads/next
 			if ($ref =~ m!^(.*?)s?/(.*)$!) {
 				$type = $1;
@@ -1107,7 +1111,8 @@ sub format_ref_marker {
 			}
 
 			$markers .= " <span class=\"$type\" title=\"$ref\">" .
-			            esc_html($name) . "</span>";
+				$cgi->a({-href => href(action=>$view{$git_type} || $git_type, hash=>$name)}, $name) .
+				"</span>";
 		}
 	}
 
-- 
1.5.6.3

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

[Jakub Narebski]

On Thu, 21 Aug 2008, Giuseppe Bilotta wrote:

> This patch turns ref markers for tags and heads into links to
> appropriate views for the ref name. Appropriate changes are made in the
> CSS to prevent ref markers to be annoyingly blue and underlined.
> 
> For all git ref types it's assumed that the preferred view is named like
> the ref type itself. For commits, we map the view to shortlog.

NAK.

It is a good idea, but not so good solution.

> --- a/gitweb/gitweb.css
> +++ b/gitweb/gitweb.css

> +span.refs span a {
> +	text-decoration: none;
> +	color: inherit;
> +}

Possible improvement:

We would probably want to make this link discoverable, by adding
underline on :hover, like for other "hidden links" in gitweb (for
example in commitdiff view).

> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 90cd99b..a12ce87 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -1093,10 +1093,14 @@ sub format_log_line_html {
>  sub format_ref_marker {
>  	my ($refs, $id) = @_;
>  	my $markers = '';
> +	my %view = (
> +		"commit" => "shortlog",
> +	);
>  
>  	if (defined $refs->{$id}) {
>  		foreach my $ref (@{$refs->{$id}}) {
>  			my ($type, $name) = qw();
> +			my $git_type = git_get_type($ref);
>  			# e.g. tags/v2.6.11 or heads/next
>  			if ($ref =~ m!^(.*?)s?/(.*)$!) {
>  				$type = $1;

git_get_type calls 'git cat-file -t', so for each ref shown you make
*additional call* to git command (additional fork).  Not good, especially
that you can get information if a ref is a tag (indirect reference)
or not one can get from within git_get_references; which in turn
uses "git show-refs --dereference" and used to use either 
"git peek-remote ." or ".git/info/refs" file.  If there is <name>^{},
then <name> is indirect reference: is a tag.

As we display ref markers only for log-like views, marker can be tag
or can be "lightweight reference" and be only a commit (in theory
we could show ref markers also for tree and blob items, but it is not
important now).

-- 
Jakub Narebski
Poland

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

[Giuseppe Bilotta]

> It is a good idea, but not so good solution.

Ok, let's see if I can find a better way to do it 8-)

>> --- a/gitweb/gitweb.css
>> +++ b/gitweb/gitweb.css
>
>> +span.refs span a {
>> +     text-decoration: none;
>> +     color: inherit;
>> +}
>
> Possible improvement:
>
> We would probably want to make this link discoverable, by adding
> underline on :hover, like for other "hidden links" in gitweb (for
> example in commitdiff view).

Can do that.

>>                       my ($type, $name) = qw();
>> +                     my $git_type = git_get_type($ref);
>>                       # e.g. tags/v2.6.11 or heads/next
>>                       if ($ref =~ m!^(.*?)s?/(.*)$!) {
>>                               $type = $1;
>
> git_get_type calls 'git cat-file -t', so for each ref shown you make
> *additional call* to git command (additional fork).  Not good, especially
> that you can get information if a ref is a tag (indirect reference)
> or not one can get from within git_get_references; which in turn
> uses "git show-refs --dereference" and used to use either
> "git peek-remote ." or ".git/info/refs" file.  If there is <name>^{},
> then <name> is indirect reference: is a tag.
>
> As we display ref markers only for log-like views, marker can be tag
> or can be "lightweight reference" and be only a commit (in theory
> we could show ref markers also for tree and blob items, but it is not
> important now).

By looking at git_get_reference() what I see is basically the use of
the same field as $type in format_ref_marker(). I can probably use
that, although it means that any future extensions to ref marker
display will need to hack the routine too. (This would mean that the
patch would be more similar to my original patch
http://marc.info/?l=git&m=121769155017642&w=2 ).

If this is not what you're suggesting, then I'm afraid I don't fully
grasp your idea.


-- 
Giuseppe "Oblomov" Bilotta

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

[Jakub Narebski]

On Fri, 22 Aug 2008, Giuseppe Bilotta wrote:

>>> --- a/gitweb/gitweb.css
>>> +++ b/gitweb/gitweb.css
>>
>>> +span.refs span a {
>>> +     text-decoration: none;
>>> +     color: inherit;
>>> +}
>>
>> Possible improvement:
>>
>> We would probably want to make this link discoverable, by adding
>> underline on :hover, like for other "hidden links" in gitweb (for
>> example in commitdiff view).
> 
> Can do that.

Additional idea: it would be nice to know if clicking on ref marker
would lead us to 'shortlog' view, or to 'tag' view; so perhaps we should
distinguish somehow indirect refs, for example using bold font-weight.
 
>>>                       my ($type, $name) = qw();
>>> +                     my $git_type = git_get_type($ref);
>>>                       # e.g. tags/v2.6.11 or heads/next
>>>                       if ($ref =~ m!^(.*?)s?/(.*)$!) {
>>>                               $type = $1;
>>
>> git_get_type calls 'git cat-file -t', so for each ref shown you make
>> *additional call* to git command (additional fork).  Not good, especially
>> that you can get information if a ref is a tag (indirect reference)
>> or not one can get from within git_get_references; which in turn
>> uses "git show-refs --dereference" and used to use either
>> "git peek-remote ." or ".git/info/refs" file.  If there is <name>^{},
>> then <name> is indirect reference: is a tag.
>>
>> As we display ref markers only for log-like views, marker can be tag
>> or can be "lightweight reference" and be only a commit (in theory
>> we could show ref markers also for tree and blob items, but it is not
>> important now).
> 
> By looking at git_get_reference() what I see is basically the use of
> the same field as $type in format_ref_marker(). I can probably use
> that, although it means that any future extensions to ref marker
> display will need to hack the routine too. (This would mean that the
> patch would be more similar to my original patch
> http://marc.info/?l=git&m=121769155017642&w=2 ).
> 
> If this is not what you're suggesting, then I'm afraid I don't fully
> grasp your idea.

No, that is not what I was suggesting.

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

But that is just a proposal...
-- 
Jakub Narebski
Poland

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

[Jakub Narebski]

On Fri, 22 August 2008, Giuseppe Bilotta wrote:
> On Fri, Aug 22, 2008 at 10:49 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> >
> > 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

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

[Giuseppe Bilotta]

On Fri, Aug 22, 2008 at 12:56 PM, Jakub Narebski <jnareb@gmail.com> 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

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

[Lea Wiemann]

Giuseppe Bilotta wrote:
> +			my $git_type = git_get_type($ref);
> [...]
> +				$cgi->a({-href => href(action=>$view{$git_type} || $git_type, hash=>$name)}, $name) .

Since some of this thread seems to be about performance, you might just
make this a link to action => 'object' (and save the git_get_type call)
and let gitweb Do The Right Thing when the link is followed.

[Disclaimer: Haven't read the whole thread, and haven't checked if
action=object is actually doing the right thing here.]

-- Lea

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

[Giuseppe Bilotta]

On Sun, Aug 24, 2008 at 9:30 PM, Lea Wiemann <lewiemann@gmail.com> wrote:
> Giuseppe Bilotta wrote:
>> +                     my $git_type = git_get_type($ref);
>> [...]
>> +                             $cgi->a({-href => href(action=>$view{$git_type} || $git_type, hash=>$name)}, $name) .
>
> Since some of this thread seems to be about performance, you might just
> make this a link to action => 'object' (and save the git_get_type call)
> and let gitweb Do The Right Thing when the link is followed.
>
> [Disclaimer: Haven't read the whole thread, and haven't checked if
> action=object is actually doing the right thing here.]

The object would do the right thing if we wanted 'commit' to be the
default action for commits, but we actually want shortlogs in that
case.

However, this remark of yours makes me think of a different way to
approach the problem: create an 'objectview' action that acts just
like object, but actually maps those objects to an appropriate default
view. I'd like to hear Petr's and Jakub's opinion, and if they think
it's a better approach than the latest version of my patch
http://marc.info/?l=git&m=121941177812828&w=2 (which has not received
any comments yet), I'll implemented it that way.

-- 
Giuseppe "Oblomov" Bilotta

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

[Jakub Narebski]

Lea Wiemann wrote:
> Giuseppe Bilotta wrote:
> > +			my $git_type = git_get_type($ref);
> > [...]
> > +				$cgi->a({-href => href(action=>$view{$git_type} || $git_type, hash=>$name)}, $name) .
> 
> Since some of this thread seems to be about performance, you might just
> make this a link to action => 'object' (and save the git_get_type call)
> and let gitweb Do The Right Thing when the link is followed.
> 
> [Disclaimer: Haven't read the whole thread, and haven't checked if
> action=object is actually doing the right thing here.]

First, only the first patch (and perhaps second) called git_get_type;
v4 and v5 do not.  Second, link to 'object' action would not do the
right thing; we want either 'shortlog' or 'tag' view, not 'commit'
or 'tag' view.

What this patch does is making ref markers in the log-like views, and
in the commit subject line headers in other view be "hidden links" to
either 'shortlog' (in the case of ref being head/branch, or lightweight
tag), or to 'tag' view in the case of annotated tag.  We rely on the
fact that we know what type of object refs points to (currently it is
only 'commit', which might change, but the fact that we know type of
object for which we show marker would not change), and the fact that
tags point to given object only indirectly, and only tags can point
indirectly (^{} suffix in "git show-ref --dereference", and
"git ls-remote .", and $GIT_DIR/info/refs).


So you could have spared yourself this comment if you have read commit
and the rest of thread more carefully...
-- 
Jakub Narebski
Poland

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

[Giuseppe Bilotta]

On Sun, Aug 24, 2008 at 10:37 PM, Jakub Narebski <jnareb@gmail.com> wrote:
> Lea Wiemann wrote:
>> Giuseppe Bilotta wrote:
>> > +                   my $git_type = git_get_type($ref);
>> > [...]
>> > +                           $cgi->a({-href => href(action=>$view{$git_type} || $git_type, hash=>$name)}, $name) .
>>
>> Since some of this thread seems to be about performance, you might just
>> make this a link to action => 'object' (and save the git_get_type call)
>> and let gitweb Do The Right Thing when the link is followed.
>>
>> [Disclaimer: Haven't read the whole thread, and haven't checked if
>> action=object is actually doing the right thing here.]
>
> First, only the first patch (and perhaps second) called git_get_type;
> v4 and v5 do not.  Second, link to 'object' action would not do the
> right thing; we want either 'shortlog' or 'tag' view, not 'commit'
> or 'tag' view.
>
> What this patch does is making ref markers in the log-like views, and
> in the commit subject line headers in other view be "hidden links" to
> either 'shortlog' (in the case of ref being head/branch, or lightweight
> tag), or to 'tag' view in the case of annotated tag.  We rely on the
> fact that we know what type of object refs points to (currently it is
> only 'commit', which might change, but the fact that we know type of
> object for which we show marker would not change), and the fact that
> tags point to given object only indirectly, and only tags can point
> indirectly (^{} suffix in "git show-ref --dereference", and
> "git ls-remote .", and $GIT_DIR/info/refs).

However, Lea's idea has its own merit. I hacked up a patch series that
implements a git_marker_view() function (or fucntion as the shortlog
says 8-P) and *that* one is used for ref markers, you can see it (3
patches) here: http://git.oblomov.eu/git/shortlog/heads/gitweb/shortlog..heads/gitweb/refmark
[it's on top of other stuff but it's actually independent from the
other stuff].

The advantage of this approach is that you actually it's more flexible
in case future expansions lead to other differences in object vs
marker view (currently the only difference is commit => shortlog):
such differences just need to be added to the appropriate %views hash.

The disadvantage is that we don't care about the difference between
lightweight and annotated tags, so there is no more visual difference
about it, something which patch v5 does. This could of course be
addressed separately as needed.

So, should I resend v5 with the small change about refs canonical
form, or is somebody else doing it? Or is the new idea as implemented
in the above mentioned changeset preferrable? Should I send that
patches to the mailing list?

Let me know, I've got plenty more stuff ready for gitweb and I'm eager
to see them accepted upstream 8-D

-- 
Giuseppe "Oblomov" Bilotta

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

[Jakub Narebski]

On Tue, 26 August 2008, Giuseppe Bilotta wrote:
> On Sun, Aug 24, 2008 at 10:37 PM, Jakub Narebski <jnareb@gmail.com> wrote:
>> Lea Wiemann wrote:
>>> Giuseppe Bilotta wrote:
>>>> +                   my $git_type = git_get_type($ref);
>>>> [...]
>>>> +                           $cgi->a({-href => href(action=>$view{$git_type} || $git_type, hash=>$name)}, $name) .
>>>
>>> Since some of this thread seems to be about performance, you might just
>>> make this a link to action => 'object' (and save the git_get_type call)
>>> and let gitweb Do The Right Thing when the link is followed.
>>>
>>> [Disclaimer: Haven't read the whole thread, and haven't checked if
>>> action=object is actually doing the right thing here.]
>>
>> First, only the first patch (and perhaps second) called git_get_type;
>> v4 and v5 do not.  Second, link to 'object' action would not do the
>> right thing; we want either 'shortlog' or 'tag' view, not 'commit'
>> or 'tag' view.
>>
>> What this patch does is making ref markers in the log-like views, and
>> in the commit subject line headers in other view be "hidden links" to
>> either 'shortlog' (in the case of ref being head/branch, or lightweight
>> tag), or to 'tag' view in the case of annotated tag.  We rely on the
>> fact that we know what type of object refs points to (currently it is
>> only 'commit', which might change, but the fact that we know type of
>> object for which we show marker would not change), and the fact that
>> tags point to given object only indirectly, and only tags can point
>> indirectly (^{} suffix in "git show-ref --dereference", and
>> "git ls-remote .", and $GIT_DIR/info/refs).
> 
> However, Lea's idea has its own merit. I hacked up a patch series that
> implements a git_marker_view() function (or fucntion as the shortlog
> says 8-P) and *that* one is used for ref markers, you can see it (3
> patches) here: http://git.oblomov.eu/git/shortlog/heads/gitweb/shortlog..heads/gitweb/refmark
> [it's on top of other stuff but it's actually independent from the
> other stuff].
> 
> The advantage of this approach is that you actually it's more flexible
> in case future expansions lead to other differences in object vs
> marker view (currently the only difference is commit => shortlog):
> such differences just need to be added to the appropriate %views hash.
> 
> The disadvantage is that we don't care about the difference between
> lightweight and annotated tags, so there is no more visual difference
> about it, something which patch v5 does. This could of course be
> addressed separately as needed.

NAK. For me using 'objectview' action, i.e. deciding on action based
on the type of object, it is a bad idea. Let me explain in more detail.

First, I pretty much think that user would want to know if clicking
on ref marker would lead him/her to 'tag' view, or to 'shortlog' view.
So having visual difference is something that we want to have.  And
if we do that when generating link, why not go one extra step and
in addition to visual difference also use different action in link?


Second, for me the whole idea of deciding on action based on the type
of object, either via 'object' view/action or via actionless gitweb URL
is either necessary evil or a tradeoff.  We use it because of the
following issues:

 * Calculating proper action during link generation via git_get_type()
   is costly; it is additional fork (this can be avoided by using
   'reuse connection' get type from Lea gitweb caching work), extra
   CPU load, and extra I/O hit.  If it *cannot be avoided* (which is
   not the case of ref markers links) it is better to defer this cost
   till user actually follows the link.  This feature is used for
   sha-1 committags (turning something that looks like sha-1 into
   gitweb hyperlink) and for links to symbolic links targets in 'tree'
   view (where link target can not exist - dangling symlink, or can
   lead to file or directory).

   (That is necessary evil part).

 * Guessing action based on type of objects allows gitweb to be more
   robust, support URL editing/mangling more easily, and aid the
   creation of shortcuts to git repositories (e.g. in bugtracker)
   more easily (see commit 7f9778b19b07601ae81).  Currently it is
   done in dispatch phase, and does not do redirection.

   (That is tradeoff: more robust and extra feature for extra get type).

BTW. one thing that can be done is consolidation of "guessing action"
code: it is done by simply calculating what is to put in $action during
dispatch, or is done in 'object' view to calculate redirect URL with
proper action.  I have tried to bring them together, but patches were
I think lost in the noise.

> So, should I resend v5 with the small change about refs canonical
> form, or is somebody else doing it? Or is the new idea as implemented
> in the above mentioned changeset preferrable? Should I send that
> patches to the mailing list?

IMHO v5 with small change making refs canonical (hash=>"refs/$ref")
is preferred way to do this.  You can send v6 patch or I can send
it (I planned doing this today).
 
> Let me know, I've got plenty more stuff ready for gitweb and I'm eager
> to see them accepted upstream 8-D

First, the great problem with gitweb patches as of today is if Lea
Google Summer of Code 2008 work on gitweb caching would be accepted
(merged in) into git repository; I pretty much think that any gitweb
improvements would be "incompatibile" (read: causing conflicts) with
'gitweb caching' patch covering such large parts of code... but
I might be mistaken about that.

Second, I have planned on sending "gitweb TODO and wishlist", or 
"what's cooking in gitweb", with links to threads on git mailing
list, and sometimes explanation why some feature should wait on
infrastructure improvements, such as consolidating log-like views
code.


Thank you for contributing to gitweb...
-- 
Jakub Narebski
Poland

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

[Giuseppe Bilotta]

On Tue, Aug 26, 2008 at 10:15 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> BTW. one thing that can be done is consolidation of "guessing action"
> code: it is done by simply calculating what is to put in $action during
> dispatch, or is done in 'object' view to calculate redirect URL with
> proper action.  I have tried to bring them together, but patches were
> I think lost in the noise.

Ah, but the problem is that guessing the action based on the object
sometimes depends on the context: for some cases the 'commit' action
is the correct one for commits, in other cases shortlog is preferred.
This is why I introduced the %views hash in some of my patches.

> IMHO v5 with small change making refs canonical (hash=>"refs/$ref")
> is preferred way to do this.  You can send v6 patch or I can send
> it (I planned doing this today).

I can do it, no problem.

> First, the great problem with gitweb patches as of today is if Lea
> Google Summer of Code 2008 work on gitweb caching would be accepted
> (merged in) into git repository; I pretty much think that any gitweb
> improvements would be "incompatibile" (read: causing conflicts) with
> 'gitweb caching' patch covering such large parts of code... but
> I might be mistaken about that.

That's ok, I can wait for Lea's code to get into the repo, so I can
work on the conflicts myself. I'm not sure what parts of the code she
touches though, so for some things it might be easy (the pathinfo
stuff, for example). Other features such as my allheads thing would
probably need to be reworked for caching.

> Thank you for contributing to gitweb...

My pleasure.

-- 
Giuseppe "Oblomov" Bilotta

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

[Jakub Narebski]

On Tue, 26 Aug 2008 12:58, Giuseppe Bilotta wrote:
> On Tue, Aug 26, 2008 at 10:15 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> >
> > BTW. one thing that can be done is consolidation of "guessing action"
> > code: it is done by simply calculating what is to put in $action during
> > dispatch, or is done in 'object' view to calculate redirect URL with
> > proper action.  I have tried to bring them together, but patches were
> > I think lost in the noise.
> 
> Ah, but the problem is that guessing the action based on the object
> sometimes depends on the context: for some cases the 'commit' action
> is the correct one for commits, in other cases shortlog is preferred.
> This is why I introduced the %views hash in some of my patches.

By the way, this is argument *for* selecting action when generating
link, if it is possible without incurring unnecessary (if you don't
follow the link) performance penalty.  For example in the case of
ref markers links, you can link to 'log' in log view, 'shortlog'
in 'shortlog' view, perhaps even 'history' in 'history' and 'tree'/'blob'
views; or just 'shortlog' in all other views (for "commit subject"
line heading).

Just my 2 eurocents.
-- 
Jakub Narebski
Poland

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

[Giuseppe Bilotta]

On Tue, Aug 26, 2008 at 1:49 PM, Jakub Narebski <jnareb@gmail.com> wrote:
> By the way, this is argument *for* selecting action when generating
> link, if it is possible without incurring unnecessary (if you don't
> follow the link) performance penalty.  For example in the case of
> ref markers links, you can link to 'log' in log view, 'shortlog'
> in 'shortlog' view, perhaps even 'history' in 'history' and 'tree'/'blob'
> views; or just 'shortlog' in all other views (for "commit subject"
> line heading).

An interesting idea. I'll start experimenting with this
context-sensitive action selection for ref-markers. (In the mean time,
I've sent v6 with the simpler 'shortlog or tags' view. I guess an
eventual context-sensitive action can be applied as a subsequent
patch).

-- 
Giuseppe "Oblomov" Bilotta

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

[Giuseppe Bilotta]

On Tue, Aug 26, 2008 at 1:49 PM, Jakub Narebski <jnareb@gmail.com> wrote:
> On Tue, 26 Aug 2008 12:58, Giuseppe Bilotta wrote:
>> Ah, but the problem is that guessing the action based on the object
>> sometimes depends on the context: for some cases the 'commit' action
>> is the correct one for commits, in other cases shortlog is preferred.
>> This is why I introduced the %views hash in some of my patches.
>
> By the way, this is argument *for* selecting action when generating
> link, if it is possible without incurring unnecessary (if you don't
> follow the link) performance penalty.  For example in the case of
> ref markers links, you can link to 'log' in log view, 'shortlog'
> in 'shortlog' view, perhaps even 'history' in 'history' and 'tree'/'blob'
> views; or just 'shortlog' in all other views (for "commit subject"
> line heading).

So, I managed to get this feature into the latest version of the
refmark patch (v7). Hope it's really the final form 8-)

-- 
Giuseppe "Oblomov" Bilotta

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

[Lea Wiemann]

Jakub Narebski wrote:
> By the way, this is argument *for* selecting action when generating
> link, if it is possible without incurring unnecessary (if you don't
> follow the link) performance penalty.

I agree that it's much cleaner to select the action when generating the
page, rather than having an 'objectview' action or so.

Worrying about performance seems like premature optimization though --
my guesstimate is that the performance penalty for looking up the object
type is not practically noticeable (read: relevant), and with my patch
applied (even without caching activated) it should move below the
measurable range.  So don't complicate the code to gain another 0.01%
performance. ;-)

-- Lea

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

[Giuseppe Bilotta]

On Thu, Aug 28, 2008 at 3:43 AM, Lea Wiemann <lewiemann@gmail.com> wrote:
> Jakub Narebski wrote:
>> By the way, this is argument *for* selecting action when generating
>> link, if it is possible without incurring unnecessary (if you don't
>> follow the link) performance penalty.
>
> I agree that it's much cleaner to select the action when generating the
> page, rather than having an 'objectview' action or so.

In that case v7 of my patch is The Way (TM).

> Worrying about performance seems like premature optimization though --
> my guesstimate is that the performance penalty for looking up the object
> type is not practically noticeable (read: relevant), and with my patch
> applied (even without caching activated) it should move below the
> measurable range.  So don't complicate the code to gain another 0.01%
> performance. ;-)

Well, considering that after Jakub's suggestion we just get the target
type from the presence and absence of ^{} in the output of show-refs,
we're not making the code overly complex :)

BTW, any ETA on your caching changes landing in git? I'm really
curious to see how many of my changes are not compatible with it 8-D

-- 
Giuseppe "Oblomov" Bilotta

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

[Jakub Narebski]

Lea Wiemann wrote:
> Jakub Narebski wrote:
> >
> > By the way, this is argument *for* selecting action when generating
> > link, if it is possible without incurring unnecessary (if you don't
> > follow the link) performance penalty.
> 
> I agree that it's much cleaner to select the action when generating the
> page, rather than having an 'objectview' action or so.
> 
> Worrying about performance seems like premature optimization though --
> my guesstimate is that the performance penalty for looking up the object
> type is not practically noticeable (read: relevant), and with my patch
> applied (even without caching activated) it should move below the
> measurable range.  So don't complicate the code to gain another 0.01%
> performance. ;-)

First, without your 'git cat-file --batch-check' reuse-connection trick
it wouldn't be _one_ additional fork; it is one fork per ref marker,
which might be quite a lot in tag-heavy, branch-heavy, and using for
example StGIT (with its refs) environment.  Note that not all operating
systems have lightweight fork, and that even with "caching" it is IO hit,
and a bit of CPU hit.

Second, it isn't much more code than git_get_type solution, it is bit
larger change: leave ^{} alone, check if ^{} and strip it, as compared
to git_get_type.

-- 
Jakub Narebski
Poland