[PATCH 0/7] gitweb: allheads feature

[PATCH 0/7] gitweb: allheads feature

[Giuseppe Bilotta]

This is a rehash of an old patchset of mine that got stalled waiting for
other independent patches to go in first, and then for me to get the
time to work on it again.

The first 4 patches are IMO ready for inclusing in gitweb, and their
purpose is to introduce a new view (and a new summary block) that
display all the remote heads (assuming the feature is enabled).
Somebody suggested via email that this could even the basis for some
kind of 'social graph' for gitweb repositories, in a way similar to what
is found on sites like github or gitorious, but for me the feature in
itself can already be useful.

The last three patches are more of the RFC side, in particular the last
one. The idea is to group remote heads 'by remote' instead of just
listing them serially. So I first introduce code and styling to have
'blocks of stuff' in gitweb, and then use this concept to group together
remote heads belonging to the same remote.

The final result is rather curious and you can see it in action at
<http://git.oblomov.eu/rbot/remotes>, although it would be nice to find
a way to layout the blocks in a smarter way. What I really don't like
(at the moment) is the way things come out in summary view instead.

The issue there is that we only gather 16 remote heads, so some remotes
might have no branches displayed, but it becomes difficult to detect and
indicate when remotes have incomplete information being displayed. A
possible solution would be to call show-ref N times (N being the number
of remotes) with a limit of 16/N heads, but that can be a lot of calls.
So I'm open to suggestions on how to improve this part (maybe just show
a flat view in the remotes section of summary view?)

Giuseppe Bilotta (7):
  gitweb: introduce remote_heads feature
  gitweb: git_get_heads_list accepts an optional list of refs.
  gitweb: separate heads and remotes lists
  gitweb: link heads and remotes view
  gitweb: auxiliary functions to group data
  gitweb: group styling
  gitweb: group remote heads

 gitweb/gitweb.perl       |  100 ++++++++++++++++++++++++++++++++++++++++++---
 gitweb/static/gitweb.css |    6 +++
 2 files changed, 99 insertions(+), 7 deletions(-)

-- 
1.7.3.rc1.230.g8b572

[PATCH 1/7] gitweb: introduce remote_heads feature

[Giuseppe Bilotta]

With this feature enabled, remotes are retrieved (and displayed)
when getting (and displaying) the heads list. Typical usage would be for
local repository browsing, e.g. by using git-instaweb (or even a more
permanent gitweb setup), to check the repository status and the relation
between tracking branches and the originating remotes.

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index a85e2f6..7116c26 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -486,6 +486,18 @@ our %feature = (
 		'sub' => sub { feature_bool('highlight', @_) },
 		'override' => 0,
 		'default' => [0]},
+
+	# Make gitweb show remotes too in the heads list
+
+	# To enable system wide have in $GITWEB_CONFIG
+	# $feature{'remote_heads'}{'default'} = [1];
+	# To have project specific config enable override in $GITWEB_CONFIG
+	# $feature{'remote_heads'}{'override'} = 1;
+	# and in project config gitweb.remote_heads = 0|1;
+	'remote_heads' => {
+		'sub' => sub { feature_bool('remote_heads', @_) },
+		'override' => 0,
+		'default' => [0]},
 );
 
 sub gitweb_get_feature {
@@ -3146,10 +3158,12 @@ sub git_get_heads_list {
 	my $limit = shift;
 	my @headslist;
 
+	my $remote_heads = gitweb_check_feature('remote_heads');
+
 	open my $fd, '-|', git_cmd(), 'for-each-ref',
 		($limit ? '--count='.($limit+1) : ()), '--sort=-committerdate',
 		'--format=%(objectname) %(refname) %(subject)%00%(committer)',
-		'refs/heads'
+		'refs/heads', ( $remote_heads ? 'refs/remotes' : '')
 		or return;
 	while (my $line = <$fd>) {
 		my %ref_item;
@@ -3160,8 +3174,9 @@ sub git_get_heads_list {
 		my ($committer, $epoch, $tz) =
 			($committerinfo =~ /^(.*) ([0-9]+) (.*)$/);
 		$ref_item{'fullname'}  = $name;
-		$name =~ s!^refs/heads/!!;
+		$name =~ s!^refs/(head|remote)s/!!;
 
+		$ref_item{'class'} = $1;
 		$ref_item{'name'}  = $name;
 		$ref_item{'id'}    = $hash;
 		$ref_item{'title'} = $title || '(no commit message)';
-- 
1.7.3.rc1.230.g8b572

[PATCH 2/7] gitweb: git_get_heads_list accepts an optional list of refs.

[Giuseppe Bilotta]

git_get_heads_list(limit, class1, class2, ...) can now be used to retrieve
refs/class1, refs/class2 etc. Defaults to ('heads') or ('heads', 'remotes')
depending on the remote_heads option.

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 7116c26..21e83bb 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3155,15 +3155,18 @@ sub parse_from_to_diffinfo {
 ## parse to array of hashes functions
 
 sub git_get_heads_list {
-	my $limit = shift;
+	my ($limit, @class) = @_;
+	unless (defined @class) {
+		my $remote_heads = gitweb_check_feature('remote_heads');
+		@class = ('heads', $remote_heads ? 'remotes' : undef);
+	}
+	my @refs = map { "refs/$_" } @class;
 	my @headslist;
 
-	my $remote_heads = gitweb_check_feature('remote_heads');
-
 	open my $fd, '-|', git_cmd(), 'for-each-ref',
 		($limit ? '--count='.($limit+1) : ()), '--sort=-committerdate',
 		'--format=%(objectname) %(refname) %(subject)%00%(committer)',
-		'refs/heads', ( $remote_heads ? 'refs/remotes' : '')
+		@refs
 		or return;
 	while (my $line = <$fd>) {
 		my %ref_item;
-- 
1.7.3.rc1.230.g8b572

[PATCH 3/7] gitweb: separate heads and remotes lists

[Giuseppe Bilotta]

We specialize the 'heads' action to only display local branches, and
introduce a 'remotes' action to display the remote branches (only
available when the remotes_head feature is enabled).

Mirroring this, we also split the heads list in summary view into
local and remote lists, each linking to the appropriate action.

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 21e83bb..0118739 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -712,6 +712,7 @@ our %actions = (
 	"log" => \&git_log,
 	"patch" => \&git_patch,
 	"patches" => \&git_patches,
+	"remotes" => \&git_remotes,
 	"rss" => \&git_rss,
 	"atom" => \&git_atom,
 	"search" => \&git_search,
@@ -5112,6 +5113,7 @@ sub git_summary {
 	my %co = parse_commit("HEAD");
 	my %cd = %co ? parse_date($co{'committer_epoch'}, $co{'committer_tz'}) : ();
 	my $head = $co{'id'};
+	my $remote_heads = gitweb_check_feature('remote_heads');
 
 	my $owner = git_get_project_owner($project);
 
@@ -5119,7 +5121,8 @@ sub git_summary {
 	# These get_*_list functions return one more to allow us to see if
 	# there are more ...
 	my @taglist  = git_get_tags_list(16);
-	my @headlist = git_get_heads_list(16);
+	my @headlist = git_get_heads_list(16, 'heads');
+	my @remotelist = $remote_heads ? git_get_heads_list(16, 'remotes') : ();
 	my @forklist;
 	my $check_forks = gitweb_check_feature('forks');
 
@@ -5197,6 +5200,13 @@ sub git_summary {
 		               $cgi->a({-href => href(action=>"heads")}, "..."));
 	}
 
+	if (@remotelist) {
+		git_print_header_div('remotes');
+		git_heads_body(\@remotelist, $head, 0, 15,
+		               $#remotelist <= 15 ? undef :
+		               $cgi->a({-href => href(action=>"remotes")}, "..."));
+	}
+
 	if (@forklist) {
 		git_print_header_div('forks');
 		git_project_list_body(\@forklist, 'age', 0, 15,
@@ -5504,13 +5514,29 @@ sub git_heads {
 	git_print_page_nav('','', $head,undef,$head);
 	git_print_header_div('summary', $project);
 
-	my @headslist = git_get_heads_list();
+	my @headslist = git_get_heads_list(undef, 'heads');
 	if (@headslist) {
 		git_heads_body(\@headslist, $head);
 	}
 	git_footer_html();
 }
 
+sub git_remotes {
+	gitweb_check_feature('remote_heads')
+		or die_error(403, "Remote heads view is disabled");
+
+	my $head = git_get_head_hash($project);
+	git_header_html();
+	git_print_page_nav('','', $head,undef,$head);
+	git_print_header_div('summary', $project);
+
+	my @remotelist = git_get_heads_list(undef, 'remotes');
+	if (@remotelist) {
+		git_heads_body(\@remotelist, $head);
+	}
+	git_footer_html();
+}
+
 sub git_blob_plain {
 	my $type = shift;
 	my $expires;
-- 
1.7.3.rc1.230.g8b572

[PATCH 4/7] gitweb: link heads and remotes view

[Giuseppe Bilotta]

Add a link in heads view to remotes view (if the feature is
enabled), and conversely from remotes to heads.

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 0118739..6138c6e 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -5511,7 +5511,10 @@ sub git_tags {
 sub git_heads {
 	my $head = git_get_head_hash($project);
 	git_header_html();
-	git_print_page_nav('','', $head,undef,$head);
+	my $heads_nav = gitweb_check_feature('remote_heads') ?
+		$cgi->a({-href => href(action=>"remotes", -replay=>1)},
+		        "remotes") : undef;
+	git_print_page_nav('','', $head,undef,$head, $heads_nav);
 	git_print_header_div('summary', $project);
 
 	my @headslist = git_get_heads_list(undef, 'heads');
@@ -5527,7 +5530,10 @@ sub git_remotes {
 
 	my $head = git_get_head_hash($project);
 	git_header_html();
-	git_print_page_nav('','', $head,undef,$head);
+	my $heads_nav =
+		$cgi->a({-href => href(action=>"heads", -replay=>1)},
+		        "heads");
+	git_print_page_nav('','', $head,undef,$head, $heads_nav);
 	git_print_header_div('summary', $project);
 
 	my @remotelist = git_get_heads_list(undef, 'remotes');
-- 
1.7.3.rc1.230.g8b572

[PATCH 5/7] gitweb: auxiliary functions to group data

[Giuseppe Bilotta]

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 6138c6e..92551e4 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3765,6 +3765,21 @@ sub git_print_header_div {
 	      "\n</div>\n";
 }
 
+sub git_begin_group {
+	my ($class, $id, @rest) = @_;
+
+	$class = ' class="' . join(' ', 'group', $class) . '"';
+
+	$id = ' id="' . $id . '"' if $id;
+
+	print "<div$class$id>\n";
+	git_print_header_div(@rest);
+}
+
+sub git_end_group {
+	print "</div>\n"
+}
+
 sub print_local_time {
 	print format_local_time(@_);
 }
-- 
1.7.3.rc1.230.g8b572

[PATCH 6/7] gitweb: group styling

[Giuseppe Bilotta]

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

diff --git a/gitweb/static/gitweb.css b/gitweb/static/gitweb.css
index 4132aab..4594155 100644
--- a/gitweb/static/gitweb.css
+++ b/gitweb/static/gitweb.css
@@ -573,6 +573,12 @@ div.binary {
 	font-style: italic;
 }
 
+div.group {
+	margin: .5em;
+	border: 1px solid #d9d8d1;
+	display: inline-block;
+}
+
 /* Style definition generated by highlight 2.4.5, http://www.andre-simon.de/ */
 
 /* Highlighting theme definition: */
-- 
1.7.3.rc1.230.g8b572

[PATCH 7/7] gitweb: group remote heads

[Giuseppe Bilotta]

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 92551e4..66b5400 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2758,6 +2758,16 @@ sub git_get_last_activity {
 	return (undef, undef);
 }
 
+sub git_get_remotes {
+	my ($limit) = @_;
+	open my $fd, '-|' , git_cmd(), 'remote';
+	return () unless $fd;
+	my @remotes = map { chomp ; $_ } <$fd>;
+	close $fd or return ();
+	my @remoteheads = git_get_heads_list($limit, 'remotes');
+	return (\@remotes, \@remoteheads);
+}
+
 sub git_get_references {
 	my $type = shift || "";
 	my %refs;
@@ -4979,7 +4989,7 @@ sub git_heads_body {
 		      "<td class=\"link\">" .
 		      $cgi->a({-href => href(action=>"shortlog", hash=>$ref{'fullname'})}, "shortlog") . " | " .
 		      $cgi->a({-href => href(action=>"log", hash=>$ref{'fullname'})}, "log") . " | " .
-		      $cgi->a({-href => href(action=>"tree", hash=>$ref{'fullname'}, hash_base=>$ref{'name'})}, "tree") .
+		      $cgi->a({-href => href(action=>"tree", hash=>$ref{'fullname'}, hash_base=>$ref{'fullname'})}, "tree") .
 		      "</td>\n" .
 		      "</tr>";
 	}
@@ -4991,6 +5001,19 @@ sub git_heads_body {
 	print "</table>\n";
 }
 
+sub git_remotes_body {
+	my ($remotedata, $head) = @_;
+	my @remotenames = @{$remotedata->[0]};
+	my @allheads = @{$remotedata->[1]};
+	foreach my $remote (@remotenames) {
+		my @remoteheads = grep { $_->{'name'} =~ s!^\Q$remote\E/!! } @allheads;
+		git_begin_group("remotes", $remote, "remotes/$remote",$remote);
+		git_heads_body(\@remoteheads, $head);
+		git_end_group();
+	}
+
+}
+
 sub git_search_grep_body {
 	my ($commitlist, $from, $to, $extra) = @_;
 	$from = 0 unless defined $from;
@@ -5137,7 +5160,7 @@ sub git_summary {
 	# there are more ...
 	my @taglist  = git_get_tags_list(16);
 	my @headlist = git_get_heads_list(16, 'heads');
-	my @remotelist = $remote_heads ? git_get_heads_list(16, 'remotes') : ();
+	my @remotelist = $remote_heads ? git_get_remotes(16) : ();
 	my @forklist;
 	my $check_forks = gitweb_check_feature('forks');
 
@@ -5217,9 +5240,7 @@ sub git_summary {
 
 	if (@remotelist) {
 		git_print_header_div('remotes');
-		git_heads_body(\@remotelist, $head, 0, 15,
-		               $#remotelist <= 15 ? undef :
-		               $cgi->a({-href => href(action=>"remotes")}, "..."));
+		git_remotes_body(\@remotelist, $head);
 	}
 
 	if (@forklist) {
@@ -5551,9 +5572,9 @@ sub git_remotes {
 	git_print_page_nav('','', $head,undef,$head, $heads_nav);
 	git_print_header_div('summary', $project);
 
-	my @remotelist = git_get_heads_list(undef, 'remotes');
+	my @remotelist = git_get_remotes();
 	if (@remotelist) {
-		git_heads_body(\@remotelist, $head);
+		git_remotes_body(\@remotelist, $head);
 	}
 	git_footer_html();
 }
-- 
1.7.3.rc1.230.g8b572

Re: [PATCH 3/7] gitweb: separate heads and remotes lists

[Ævar Arnfjörð Bjarmason]

On Thu, Sep 16, 2010 at 09:31, Giuseppe Bilotta
<giuseppe.bilotta@gmail.com> wrote:

> +       if (@remotelist) {
> +               git_print_header_div('remotes');
> +               git_heads_body(\@remotelist, $head, 0, 15,
> +                              $#remotelist <= 15 ? undef :
> +                              $cgi->a({-href => href(action=>"remotes")}, "..."));
> +       }

Nit: The $# syntax is pseudo-deprecated, and since you use 16 as a
constant above this would be clearer anyway:

    @remotelist <= 16 ? undef : ...

Re: [PATCH 5/7] gitweb: auxiliary functions to group data

[Ævar Arnfjörð Bjarmason]

On Thu, Sep 16, 2010 at 09:31, Giuseppe Bilotta
<giuseppe.bilotta@gmail.com> wrote:
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
> ---
>  gitweb/gitweb.perl |   15 +++++++++++++++
>  1 files changed, 15 insertions(+), 0 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 6138c6e..92551e4 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -3765,6 +3765,21 @@ sub git_print_header_div {
>              "\n</div>\n";
>  }
>
> +sub git_begin_group {
> +       my ($class, $id, @rest) = @_;
> +
> +       $class = ' class="' . join(' ', 'group', $class) . '"';
> +
> +       $id = ' id="' . $id . '"' if $id;
> +
> +       print "<div$class$id>\n";

Hrm, this would be better:

     $cgi->div( { class => "group $class", $id ? (id => $id) : () } )

> +       git_print_header_div(@rest);
> +}
> +
> +sub git_end_group {
> +       print "</div>\n"
> +}
> +

But obviously that won't fit if the closing element is being printed
separately.

Re: [PATCH 7/7] gitweb: group remote heads

[Ævar Arnfjörð Bjarmason]

On Thu, Sep 16, 2010 at 09:31, Giuseppe Bilotta
<giuseppe.bilotta@gmail.com> wrote:

> +sub git_get_remotes {
> +       my ($limit) = @_;
> +       open my $fd, '-|' , git_cmd(), 'remote';
> +       return () unless $fd;
> +       my @remotes = map { chomp ; $_ } <$fd>;
> +       close $fd or return ();

    return unless $fd;
    chomp(my @remotes = <$fd>);
    close $fd or return;

Re: [PATCH 3/7] gitweb: separate heads and remotes lists

[Giuseppe Bilotta]

On Thu, Sep 16, 2010 at 12:19 PM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> On Thu, Sep 16, 2010 at 09:31, Giuseppe Bilotta
> <giuseppe.bilotta@gmail.com> wrote:
>
>> +       if (@remotelist) {
>> +               git_print_header_div('remotes');
>> +               git_heads_body(\@remotelist, $head, 0, 15,
>> +                              $#remotelist <= 15 ? undef :
>> +                              $cgi->a({-href => href(action=>"remotes")}, "..."));
>> +       }
>
> Nit: The $# syntax is pseudo-deprecated, and since you use 16 as a
> constant above this would be clearer anyway:
>
>    @remotelist <= 16 ? undef : ...

I think following the gitweb coding style is better. OTOH, I can
probably prepare a (separate) patch to replace $# with @ where
possible.

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCH 7/7] gitweb: group remote heads

[Giuseppe Bilotta]

On Thu, Sep 16, 2010 at 12:29 PM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> On Thu, Sep 16, 2010 at 09:31, Giuseppe Bilotta
> <giuseppe.bilotta@gmail.com> wrote:
>
>> +sub git_get_remotes {
>> +       my ($limit) = @_;
>> +       open my $fd, '-|' , git_cmd(), 'remote';
>> +       return () unless $fd;
>> +       my @remotes = map { chomp ; $_ } <$fd>;
>> +       close $fd or return ();
>
>    return unless $fd;
>    chomp(my @remotes = <$fd>);
>    close $fd or return;

Thanks a lot. I guess this shows pretty much how Perl is not exactly
my primary language 8-)


-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCH 0/7] gitweb: allheads feature

[Jakub Narebski]

On Thu, 16 Sep 2010, Giuseppe Bilotta wrote:

> This is a rehash of an old patchset of mine that got stalled waiting for
> other independent patches to go in first, and then for me to get the
> time to work on it again.
> 
> The first 4 patches are IMO ready for inclusing in gitweb, and their
> purpose is to introduce a new view (and a new summary block) that
> display all the remote heads (assuming the feature is enabled).
> Somebody suggested via email that this could even the basis for some
> kind of 'social graph' for gitweb repositories, in a way similar to what
> is found on sites like github or gitorious, but for me the feature in
> itself can already be useful.

We might want to make git-instaweb enable this feature (and probably
other disabled-by-default features).  Otherwise some of information
about git repository you examine is hidden.  So I agree that this 
feature is useful by itself.
 
> The last three patches are more of the RFC side, in particular the last
> one. The idea is to group remote heads 'by remote' instead of just
> listing them serially. So I first introduce code and styling to have
> 'blocks of stuff' in gitweb, and then use this concept to group together
> remote heads belonging to the same remote.

This is a good idea in itself; I'd take a look at implementation and
styling when examining individual patches.

> The final result is rather curious and you can see it in action at
> <http://git.oblomov.eu/rbot/remotes>, although it would be nice to find
> a way to layout the blocks in a smarter way. 

Thanks for providing demo site.

Note that clicking on header for remote block, which should lead to
displaying of only single remote displays all remotes, see e.g.
http://git.oblomov.eu/rbot/remotes/a3li.  Moreover when I tried to
handcraft URL i.e. http://git.oblomov.eu/rbot/remote/a3li (with
'remote' rather than 'remotes' action) I get an empty list of heads.


About layout of 'remotes' view: to have remotes information aligned
we would have to either put everything in one single table (with remotes
headers being "colspan"), or style them with minimum width (which could
be good idea anyway).
 
> What I really don't like (at the moment) is the way things come out
> in summary view instead. 

What you don't like about it?  Should it be smarter and display only
list of remotes, perhaps even limited to 15 elements, if there are many
remote-tracking branches?  Or is it something else?

> The issue there is that we only gather 16 remote heads, so some remotes
> might have no branches displayed, but it becomes difficult to detect and
> indicate when remotes have incomplete information being displayed. A
> possible solution would be to call show-ref N times (N being the number
> of remotes) with a limit of 16/N heads, but that can be a lot of calls.
> So I'm open to suggestions on how to improve this part (maybe just show
> a flat view in the remotes section of summary view?)

Ah, I see...

Alternatively we could use smart limiting on the gitweb side.

-- 
Jakub Narebski
Poland

Re: [PATCH 1/7] gitweb: introduce remote_heads feature

[Jakub Narebski]

On Thu, 16 Sep 2010, Giuseppe Bilotta wrote:

> With this feature enabled, remotes are retrieved (and displayed)
> when getting (and displaying) the heads list. Typical usage would be for
> local repository browsing, e.g. by using git-instaweb (or even a more
> permanent gitweb setup), to check the repository status and the relation
> between tracking branches and the originating remotes.

Good idea.

> 
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
> ---
>  gitweb/gitweb.perl |   19 +++++++++++++++++--
>  1 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index a85e2f6..7116c26 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -486,6 +486,18 @@ our %feature = (
>  		'sub' => sub { feature_bool('highlight', @_) },
>  		'override' => 0,
>  		'default' => [0]},
> +
> +	# Make gitweb show remotes too in the heads list
> +
> +	# To enable system wide have in $GITWEB_CONFIG
> +	# $feature{'remote_heads'}{'default'} = [1];
> +	# To have project specific config enable override in $GITWEB_CONFIG
> +	# $feature{'remote_heads'}{'override'} = 1;
> +	# and in project config gitweb.remote_heads = 0|1;
> +	'remote_heads' => {
> +		'sub' => sub { feature_bool('remote_heads', @_) },
> +		'override' => 0,
> +		'default' => [0]},

I agree both with this feature being turned off by default, and with
it being per-project overridable.

>  );
>  
>  sub gitweb_get_feature {
> @@ -3146,10 +3158,12 @@ sub git_get_heads_list {
>  	my $limit = shift;
>  	my @headslist;
>  
> +	my $remote_heads = gitweb_check_feature('remote_heads');
> +
>  	open my $fd, '-|', git_cmd(), 'for-each-ref',
>  		($limit ? '--count='.($limit+1) : ()), '--sort=-committerdate',
>  		'--format=%(objectname) %(refname) %(subject)%00%(committer)',
> -		'refs/heads'
> +		'refs/heads', ( $remote_heads ? 'refs/remotes' : '')

The usual way for optionally providing extra arguments to git commands
in gitweb is to use empty list "()" and not empty argument "''", i.e.
it would be:

  +		'refs/heads', ( $remote_heads ? 'refs/remotes' : ())

See for example git_get_references, parse_commits,... and evem the line
with "($limit ? ...)" above in git_get_heads_list.

>  		or return;
>  	while (my $line = <$fd>) {
>  		my %ref_item;
> @@ -3160,8 +3174,9 @@ sub git_get_heads_list {
>  		my ($committer, $epoch, $tz) =
>  			($committerinfo =~ /^(.*) ([0-9]+) (.*)$/);
>  		$ref_item{'fullname'}  = $name;
> -		$name =~ s!^refs/heads/!!;
> +		$name =~ s!^refs/(head|remote)s/!!;
>  
> +		$ref_item{'class'} = $1;

Is it used anywhere, or is it left to be used by a further commit in
the series?  If it is the latter, perhaps it would be worth mentioning
in the commit message?

>  		$ref_item{'name'}  = $name;
>  		$ref_item{'id'}    = $hash;
>  		$ref_item{'title'} = $title || '(no commit message)';
> -- 
> 1.7.3.rc1.230.g8b572
> 
> 

-- 
Jakub Narebski
Poland

Re: [PATCH 2/7] gitweb: git_get_heads_list accepts an optional list of refs.

[Jakub Narebski]

On Thu, 16 Sep 2010, Giuseppe Bilotta wrote:

> git_get_heads_list(limit, class1, class2, ...) can now be used to retrieve
> refs/class1, refs/class2 etc. Defaults to ('heads') or ('heads', 'remotes')
> depending on the remote_heads option.

I like this API very much.
 
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
> ---
>  gitweb/gitweb.perl |   11 +++++++----
>  1 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 7116c26..21e83bb 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -3155,15 +3155,18 @@ sub parse_from_to_diffinfo {
>  ## parse to array of hashes functions
>  
>  sub git_get_heads_list {
> -	my $limit = shift;
> +	my ($limit, @class) = @_;

Nitpick: @class or @classes.

> +	unless (defined @class) {
> +		my $remote_heads = gitweb_check_feature('remote_heads');
> +		@class = ('heads', $remote_heads ? 'remotes' : undef);

Same comment as for previous patch:

  +		@class = ('heads', $remote_heads ? 'remotes' : ());

Or alternative solution:

  +		@class = 'heads';
  +		push @class, 'remotes' if gitweb_check_feature('remote_heads');


> +	}
> +	my @refs = map { "refs/$_" } @class;

... or you would get 

  "Use of uninitialized value in concatenation (.) or string [...]"

warning when 'remote_heads" feature is disabled, isn't it?

>  	my @headslist;
>  
> -	my $remote_heads = gitweb_check_feature('remote_heads');
> -
>  	open my $fd, '-|', git_cmd(), 'for-each-ref',
>  		($limit ? '--count='.($limit+1) : ()), '--sort=-committerdate',
>  		'--format=%(objectname) %(refname) %(subject)%00%(committer)',
> -		'refs/heads', ( $remote_heads ? 'refs/remotes' : '')
> +		@refs

Nitpick: it is called '<pattern>...' in git-for-each-ref manpage...

>  		or return;
>  	while (my $line = <$fd>) {
>  		my %ref_item;
> -- 
> 1.7.3.rc1.230.g8b572
> 
> 

-- 
Jakub Narebski
Poland

Re: [PATCH 3/7] gitweb: separate heads and remotes lists

[Jakub Narebski]

On Thu, 16 Sep 2010, Ævar Arnfjörð Bjarmason wrote:
> On Thu, Sep 16, 2010 at 09:31, Giuseppe Bilotta
> <giuseppe.bilotta@gmail.com> wrote:
> 
> > +       if (@remotelist) {
> > +               git_print_header_div('remotes');
> > +               git_heads_body(\@remotelist, $head, 0, 15,
> > +                              $#remotelist <= 15 ? undef :
> > +                              $cgi->a({-href => href(action=>"remotes")}, "..."));
> > +       }
> 
> Nit: The $# syntax is pseudo-deprecated, and since you use 16 as a
> constant above this would be clearer anyway:
> 
>     @remotelist <= 16 ? undef : ...

Actually gitweb uses *15* as a constant above... 

-- 
Jakub Narebski
Poland

Re: [PATCH 3/7] gitweb: separate heads and remotes lists

[Jakub Narebski]

On Thu, 16 Sep 2010, Giuseppe Bilotta wrote:

> We specialize the 'heads' action to only display local branches, and
> introduce a 'remotes' action to display the remote branches (only
> available when the remotes_head feature is enabled).
> 
> Mirroring this, we also split the heads list in summary view into
> local and remote lists, each linking to the appropriate action.
> 
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>

Very nice.

ACK.

-- 
Jakub Narebski
Poland

Re: [PATCH 3/7] gitweb: separate heads and remotes lists

[Ævar Arnfjörð Bjarmason]

On Thu, Sep 16, 2010 at 22:30, Jakub Narebski <jnareb@gmail.com> wrote:
> On Thu, 16 Sep 2010, Ævar Arnfjörð Bjarmason wrote:
>> On Thu, Sep 16, 2010 at 09:31, Giuseppe Bilotta
>> <giuseppe.bilotta@gmail.com> wrote:
>>
>> > +       if (@remotelist) {
>> > +               git_print_header_div('remotes');
>> > +               git_heads_body(\@remotelist, $head, 0, 15,
>> > +                              $#remotelist <= 15 ? undef :
>> > +                              $cgi->a({-href => href(action=>"remotes")}, "..."));
>> > +       }
>>
>> Nit: The $# syntax is pseudo-deprecated, and since you use 16 as a
>> constant above this would be clearer anyway:
>>
>>     @remotelist <= 16 ? undef : ...
>
> Actually gitweb uses *15* as a constant above...

I mean above that snippet, i.e. this:

    +       my @remotelist = $remote_heads ? git_get_heads_list(16,
'remotes') : ()

I think that's a bit clearer, you're not left wondering why it's 16
there and 15 here, and don't have to recall that `$#remotelist <= 15`
equals `@remotelist <= 16` (unless $[ != 0). But like I said, it's a
nit.

Re: [PATCH 4/7] gitweb: link heads and remotes view

[Jakub Narebski]

On Thu, 16 Sep 2010, Giuseppe Bilotta wrote:

> Add a link in heads view to remotes view (if the feature is
> enabled), and conversely from remotes to heads.

Good idea... but this commit message doesn't tell us *where* this link
do appear.  It is in lower part (the action specific part) of page
navigation menu.

> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
> ---
>  gitweb/gitweb.perl |   10 ++++++++--
>  1 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 0118739..6138c6e 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -5511,7 +5511,10 @@ sub git_tags {
>  sub git_heads {
>  	my $head = git_get_head_hash($project);
>  	git_header_html();
> -	git_print_page_nav('','', $head,undef,$head);
> +	my $heads_nav = gitweb_check_feature('remote_heads') ?
> +		$cgi->a({-href => href(action=>"remotes", -replay=>1)},
> +		        "remotes") : undef;

I think it would be more readable here to use 'if' statement instead
of conditional operator.

> +	git_print_page_nav('','', $head,undef,$head, $heads_nav);
>  	git_print_header_div('summary', $project);
>  
>  	my @headslist = git_get_heads_list(undef, 'heads');
> @@ -5527,7 +5530,10 @@ sub git_remotes {
>  
>  	my $head = git_get_head_hash($project);
>  	git_header_html();
> -	git_print_page_nav('','', $head,undef,$head);
> +	my $heads_nav =
> +		$cgi->a({-href => href(action=>"heads", -replay=>1)},
> +		        "heads");
> +	git_print_page_nav('','', $head,undef,$head, $heads_nav);
>  	git_print_header_div('summary', $project);
>  
>  	my @remotelist = git_get_heads_list(undef, 'remotes');
> -- 
> 1.7.3.rc1.230.g8b572
> 
> 

-- 
Jakub Narebski
Poland

Re: [PATCH 5/7] gitweb: auxiliary functions to group data

[Jakub Narebski]

On Thu, 16 Sep 2010, Giuseppe Bilotta wrote:

> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 6138c6e..92551e4 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -3765,6 +3765,21 @@ sub git_print_header_div {
>  	      "\n</div>\n";
>  }
>  
> +sub git_begin_group {
> +	my ($class, $id, @rest) = @_;
> +
> +	$class = ' class="' . join(' ', 'group', $class) . '"';
> +
> +	$id = ' id="' . $id . '"' if $id;
> +
> +	print "<div$class$id>\n";

I agree with Ævar that it would be better to use HTML generation
subroutines from CGI.pm, even start_div and end_div...

> +	git_print_header_div(@rest);
> +}
> +
> +sub git_end_group {
> +	print "</div>\n"
> +}

... but I think that having separate subroutines for opening and
closing tags is a bad design / bad API (except in some rare cases).
It is begging for unbalanced HTML.

It would be better if it was a single subroutine wrapping 'div' around
contents given either as a string, or via callback (subroutine reference),
in my opinion.

> +
>  sub print_local_time {
>  	print format_local_time(@_);
>  }
> -- 
> 1.7.3.rc1.230.g8b572
> 
> 

-- 
Jakub Narebski
Poland

Re: [PATCH 5/7] gitweb: auxiliary functions to group data

[Giuseppe Bilotta]

On Fri, Sep 17, 2010 at 3:24 AM, Jakub Narebski <jnareb@gmail.com> wrote:
>
> ... but I think that having separate subroutines for opening and
> closing tags is a bad design / bad API (except in some rare cases).
> It is begging for unbalanced HTML.
>
> It would be better if it was a single subroutine wrapping 'div' around
> contents given either as a string, or via callback (subroutine reference),
> in my opinion.

I'm not sure that in this case the string or callback approach would
be any cleaner. I'll see if perl supports closures or something like
that.

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCH 0/7] gitweb: allheads feature

[Giuseppe Bilotta]

2010/9/16 Jakub Narebski <jnareb@gmail.com>:
> On Thu, 16 Sep 2010, Giuseppe Bilotta wrote:
>
>> This is a rehash of an old patchset of mine that got stalled waiting for
>> other independent patches to go in first, and then for me to get the
>> time to work on it again.
>>
>> The first 4 patches are IMO ready for inclusing in gitweb, and their
>> purpose is to introduce a new view (and a new summary block) that
>> display all the remote heads (assuming the feature is enabled).
>> Somebody suggested via email that this could even the basis for some
>> kind of 'social graph' for gitweb repositories, in a way similar to what
>> is found on sites like github or gitorious, but for me the feature in
>> itself can already be useful.
>
> We might want to make git-instaweb enable this feature (and probably
> other disabled-by-default features).  Otherwise some of information
> about git repository you examine is hidden.  So I agree that this
> feature is useful by itself.

I'll add a patch to that effect in the next iteration.

>> The final result is rather curious and you can see it in action at
>> <http://git.oblomov.eu/rbot/remotes>, although it would be nice to find
>> a way to layout the blocks in a smarter way.
>
> Thanks for providing demo site.
>
> Note that clicking on header for remote block, which should lead to
> displaying of only single remote displays all remotes, see e.g.
> http://git.oblomov.eu/rbot/remotes/a3li.  Moreover when I tried to
> handcraft URL i.e. http://git.oblomov.eu/rbot/remote/a3li (with
> 'remote' rather than 'remotes' action) I get an empty list of heads.

The /remote/<thing> is just the standard behavior of git when any
random string is passed as PATH_INFO (try
http://git.oblomov.eu/rbot/whatever), so I cannot fix that unless I
map remote to remotes. The remotes/<headname> is a TODO

(This led me to think: do we want heads/<headname> to point to the
shortlog of that head? It currently does nothing)

> About layout of 'remotes' view: to have remotes information aligned
> we would have to either put everything in one single table (with remotes
> headers being "colspan"), or style them with minimum width (which could
> be good idea anyway).

I don't like the 'one single table' approach because, in contrast to
the current approach it doesn't allow for automatic block
rearrangement based on the window width.
The minimum width helps align the blocks vertically, but it doesn't
align the contents of the tables in it, so it's not really that big an
improvement (actually, I find it even funnier that way because of the
illusion of alignment provided by the blocks).

>> What I really don't like (at the moment) is the way things come out
>> in summary view instead.
>
> What you don't like about it?  Should it be smarter and display only
> list of remotes, perhaps even limited to 15 elements, if there are many
> remote-tracking branches?  Or is it something else?
>
>> The issue there is that we only gather 16 remote heads, so some remotes
>> might have no branches displayed, but it becomes difficult to detect and
>> indicate when remotes have incomplete information being displayed. A
>> possible solution would be to call show-ref N times (N being the number
>> of remotes) with a limit of 16/N heads, but that can be a lot of calls.
>> So I'm open to suggestions on how to improve this part (maybe just show
>> a flat view in the remotes section of summary view?)
>
> Ah, I see...
>
> Alternatively we could use smart limiting on the gitweb side.

That was an option I was considering. I thought the limiting to 16
refs and tags in summary view was for performance reasons, so I
thought that grabbing all remote heads would kind of kill the remote
view, but for sure it's more efficient than doing a ref retrieval for
each remote. OTOH, once we get all the refs, why would we only display
some of them?

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCH 1/7] gitweb: introduce remote_heads feature

[Giuseppe Bilotta]

2010/9/16 Jakub Narebski <jnareb@gmail.com>:
> On Thu, 16 Sep 2010, Giuseppe Bilotta wrote:
>>               or return;
>>       while (my $line = <$fd>) {
>>               my %ref_item;
>> @@ -3160,8 +3174,9 @@ sub git_get_heads_list {
>>               my ($committer, $epoch, $tz) =
>>                       ($committerinfo =~ /^(.*) ([0-9]+) (.*)$/);
>>               $ref_item{'fullname'}  = $name;
>> -             $name =~ s!^refs/heads/!!;
>> +             $name =~ s!^refs/(head|remote)s/!!;
>>
>> +             $ref_item{'class'} = $1;
>
> Is it used anywhere, or is it left to be used by a further commit in
> the series?  If it is the latter, perhaps it would be worth mentioning
> in the commit message?

Actually, I think this might be a leftover from some previous
iteration (IIRC my first version of this patchset listed all heads
under 'heads' grouping them by the class defined here, whereas now we
have the two separate sections). I should probably scratch it (even
though a similar mechanism is already in place in format_ref_marker
and it might be useful to coalesce this when parsing refs)

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCH 2/7] gitweb: git_get_heads_list accepts an optional list of refs.

[Giuseppe Bilotta]

2010/9/17 Jakub Narebski <jnareb@gmail.com>:
> On Thu, 16 Sep 2010, Giuseppe Bilotta wrote:
>
>> git_get_heads_list(limit, class1, class2, ...) can now be used to retrieve
>> refs/class1, refs/class2 etc. Defaults to ('heads') or ('heads', 'remotes')
>> depending on the remote_heads option.
>
> I like this API very much.

Thank you.

>> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
>> ---
>>  gitweb/gitweb.perl |   11 +++++++----
>>  1 files changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
>> index 7116c26..21e83bb 100755
>> --- a/gitweb/gitweb.perl
>> +++ b/gitweb/gitweb.perl
>> @@ -3155,15 +3155,18 @@ sub parse_from_to_diffinfo {
>>  ## parse to array of hashes functions
>>
>>  sub git_get_heads_list {
>> -     my $limit = shift;
>> +     my ($limit, @class) = @_;
>
> Nitpick: @class or @classes.

Right, the general gitweb style is for plural for arrays.

>> +     unless (defined @class) {
>> +             my $remote_heads = gitweb_check_feature('remote_heads');
>> +             @class = ('heads', $remote_heads ? 'remotes' : undef);
>
> Same comment as for previous patch:
>
>  +             @class = ('heads', $remote_heads ? 'remotes' : ());
>
> Or alternative solution:
>
>  +             @class = 'heads';
>  +             push @class, 'remotes' if gitweb_check_feature('remote_heads');
>

I like the () solution better. Less verbosity

>>       my @headslist;
>>
>> -     my $remote_heads = gitweb_check_feature('remote_heads');
>> -
>>       open my $fd, '-|', git_cmd(), 'for-each-ref',
>>               ($limit ? '--count='.($limit+1) : ()), '--sort=-committerdate',
>>               '--format=%(objectname) %(refname) %(subject)%00%(committer)',
>> -             'refs/heads', ( $remote_heads ? 'refs/remotes' : '')
>> +             @refs
>
> Nitpick: it is called '<pattern>...' in git-for-each-ref manpage...

I'm not particularly enamoured with @refs, so @patterns it is (if I
get what you mean here)

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCH 4/7] gitweb: link heads and remotes view

[Giuseppe Bilotta]

2010/9/17 Jakub Narebski <jnareb@gmail.com>:
> On Thu, 16 Sep 2010, Giuseppe Bilotta wrote:
>
>> Add a link in heads view to remotes view (if the feature is
>> enabled), and conversely from remotes to heads.
>
> Good idea... but this commit message doesn't tell us *where* this link
> do appear.  It is in lower part (the action specific part) of page
> navigation menu.

I will clarify it in the next rehash of this patchset.

>> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
>> ---
>>  gitweb/gitweb.perl |   10 ++++++++--
>>  1 files changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
>> index 0118739..6138c6e 100755
>> --- a/gitweb/gitweb.perl
>> +++ b/gitweb/gitweb.perl
>> @@ -5511,7 +5511,10 @@ sub git_tags {
>>  sub git_heads {
>>       my $head = git_get_head_hash($project);
>>       git_header_html();
>> -     git_print_page_nav('','', $head,undef,$head);
>> +     my $heads_nav = gitweb_check_feature('remote_heads') ?
>> +             $cgi->a({-href => href(action=>"remotes", -replay=>1)},
>> +                     "remotes") : undef;
>
> I think it would be more readable here to use 'if' statement instead
> of conditional operator.

You're right. I'm actually thinking about putting 'tags' in that nav
menu too, as it makes sense to link to all refs commands there.

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCH 5/7] gitweb: auxiliary functions to group data

[Jakub Narebski]

Giuseppe Bilotta wrote:
> On Fri, Sep 17, 2010 at 3:24 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> >
> > ... but I think that having separate subroutines for opening and
> > closing tags is a bad design / bad API (except in some rare cases).
> > It is begging for unbalanced HTML.
> >
> > It would be better if it was a single subroutine wrapping 'div' around
> > contents given either as a string, or via callback (subroutine reference),
> > in my opinion.
> 
> I'm not sure that in this case the string or callback approach would
> be any cleaner. I'll see if perl supports closures or something like
> that.

Perl supports closures (thanks to anonymous subroutines 'sub { ... }'
and lexical variables 'my $var'), see perlsub and "Function Templates"
in perlref.

I also recommend free ebook "Higher-Order Perl" http://hop.perl.plover.com/
-- 
Jakub Narebski
Poland

Re: [PATCH 6/7] gitweb: group styling

[Jakub Narebski]

On Thu, 16 Sep 2010, Giuseppe Bilotta wrote:

> +div.group {
> +	margin: .5em;
> +	border: 1px solid #d9d8d1;
> +	display: inline-block;

Is this 'display: inline-block;' really necessary?  I think we can do
without it (and I've heard that there are problems with it, but those
might not matter in layout used by gitweb).

Otherwise nice.

> +}

-- 
Jakub Narebski
Poland

Re: [PATCH 5/7] gitweb: auxiliary functions to group data

[Giuseppe Bilotta]

On Fri, Sep 17, 2010 at 6:06 PM, Jakub Narebski <jnareb@gmail.com> wrote:
> Giuseppe Bilotta wrote:
>> On Fri, Sep 17, 2010 at 3:24 AM, Jakub Narebski <jnareb@gmail.com> wrote:
>> >
>> > ... but I think that having separate subroutines for opening and
>> > closing tags is a bad design / bad API (except in some rare cases).
>> > It is begging for unbalanced HTML.
>> >
>> > It would be better if it was a single subroutine wrapping 'div' around
>> > contents given either as a string, or via callback (subroutine reference),
>> > in my opinion.
>>
>> I'm not sure that in this case the string or callback approach would
>> be any cleaner. I'll see if perl supports closures or something like
>> that.
>
> Perl supports closures (thanks to anonymous subroutines 'sub { ... }'
> and lexical variables 'my $var'), see perlsub and "Function Templates"
> in perlref.
>
> I also recommend free ebook "Higher-Order Perl" http://hop.perl.plover.com/

Thanks for the suggestion. I'm still not convinced that such an
implementation would be better though. Aside from the general
aesthetical suckiness of passing closures around (and the experience
is not any more pleasurable in Perl), there's also the matter of the
calling convention to use. I can think of two options:

(1) we make the function callable as git_do_group($class, $id, sub {
<closure that prints the content> }, <paramters that go to
git_print_header_div>), which is somewhat illogical since we're
specifying the content before the header, or

(2) we make it like git_do_group($class, $id, sub { <closure that
prints header div> }, sub {<closure that prints the content>}), which
is even more horrible since the header div is just a
git_print_header_div

Overall, I still get the impression that the current API is
considerably cleaner, even with the small counterweight of the risk of
leaving groups open (which is not something so dramatic anyway, IMO).

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCH 6/7] gitweb: group styling

[Giuseppe Bilotta]

2010/9/17 Jakub Narebski <jnareb@gmail.com>:
> On Thu, 16 Sep 2010, Giuseppe Bilotta wrote:
>
>> +div.group {
>> +     margin: .5em;
>> +     border: 1px solid #d9d8d1;
>> +     display: inline-block;
>
> Is this 'display: inline-block;' really necessary?  I think we can do
> without it (and I've heard that there are problems with it, but those
> might not matter in layout used by gitweb).
>
> Otherwise nice.
>
>> +}

I'm not aware of problems with inline-block (I can check
quirksmode.org though if necessary), but without it the divs will be
as large as the _containing_ block, meaning that each div will be
ultimately as large as the page. By using inline-block, instead, they
become as large as the _contained_ stuff, thus fitting the inner
table.

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCH 7/7] gitweb: group remote heads

[Jakub Narebski]

On Thu, 16 Sep 2010, Giuseppe Bilotta wrote:

> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 92551e4..66b5400 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -2758,6 +2758,16 @@ sub git_get_last_activity {
>  	return (undef, undef);
>  }
>  
> +sub git_get_remotes {
> +	my ($limit) = @_;

Probably more Perl-ish way would be to use

  +	my $limit = shift;

but this version is also all right.

> +	open my $fd, '-|' , git_cmd(), 'remote';
> +	return () unless $fd;

Gitweb usualy uses

  +	open my $fd, '-|' , git_cmd(), 'remote';
  +		or return;

> +	my @remotes = map { chomp ; $_ } <$fd>;

About Ævar comment: while

  +	chomp(my @remotes = <$fd>);

might be more Perl-ish, the above syntax is used thorough gitweb code.
So I'd leave it as it is now as the matter of code consistency.

> +	close $fd or return ();

  +	close $fd or return;

> +	my @remoteheads = git_get_heads_list($limit, 'remotes');
> +	return (\@remotes, \@remoteheads);

Why do you want for git_get_remotes() to also return remote-tracking
branches (refs/remotes/)?  Those are separate issues, and I think it
would be better API for git_get_remotes() to provide only list of
remotes, i.e.

  +	return @remotes;

Especially that we might want in the summary view to only list remotes,
without listing remote-tracking branches.

That would require more changes to the code.

> +}
> +
>  sub git_get_references {
>  	my $type = shift || "";
>  	my %refs;
> @@ -4979,7 +4989,7 @@ sub git_heads_body {
>  		      "<td class=\"link\">" .
>  		      $cgi->a({-href => href(action=>"shortlog", hash=>$ref{'fullname'})}, "shortlog") . " | " .
>  		      $cgi->a({-href => href(action=>"log", hash=>$ref{'fullname'})}, "log") . " | " .
> -		      $cgi->a({-href => href(action=>"tree", hash=>$ref{'fullname'}, hash_base=>$ref{'name'})}, "tree") .
> +		      $cgi->a({-href => href(action=>"tree", hash=>$ref{'fullname'}, hash_base=>$ref{'fullname'})}, "tree") .

This is independent change, and should be in a separate commit, isn't it?

>  		      "</td>\n" .
>  		      "</tr>";
>  	}
> @@ -4991,6 +5001,19 @@ sub git_heads_body {
>  	print "</table>\n";
>  }
>  
> +sub git_remotes_body {
> +	my ($remotedata, $head) = @_;
> +	my @remotenames = @{$remotedata->[0]};
> +	my @allheads = @{$remotedata->[1]};

Why not

  +	my ($remotenames, $allheads, $head) = @_;

Beside, isn't it $remote_heads and not $allheads?

> +	foreach my $remote (@remotenames) {

It would be then

  +	foreach my $remote (@$remotenames) {

> +		my @remoteheads = grep { $_->{'name'} =~ s!^\Q$remote\E/!! } @allheads;

Should we display remote even if it doesn't have any remote heads
associated with it?

> +		git_begin_group("remotes", $remote, "remotes/$remote",$remote);
> +		git_heads_body(\@remoteheads, $head);
> +		git_end_group();

This would have to be modified with change to git_begin_group() / 
/ git_end_group().

BTW isn't it premature generalization?  It is only place AFAIKS that 
uses git_*_group() subroutines.

> +	}
> +
> +}
> +
>  sub git_search_grep_body {
>  	my ($commitlist, $from, $to, $extra) = @_;
>  	$from = 0 unless defined $from;
> @@ -5137,7 +5160,7 @@ sub git_summary {
>  	# there are more ...
>  	my @taglist  = git_get_tags_list(16);
>  	my @headlist = git_get_heads_list(16, 'heads');
> -	my @remotelist = $remote_heads ? git_get_heads_list(16, 'remotes') : ();
> +	my @remotelist = $remote_heads ? git_get_remotes(16) : ();

No change of git_get_remotes() does only one thing: returning list
of remotes.

>  	my @forklist;
>  	my $check_forks = gitweb_check_feature('forks');
>  
> @@ -5217,9 +5240,7 @@ sub git_summary {
>  
>  	if (@remotelist) {
>  		git_print_header_div('remotes');
> -		git_heads_body(\@remotelist, $head, 0, 15,
> -		               $#remotelist <= 15 ? undef :
> -		               $cgi->a({-href => href(action=>"remotes")}, "..."));
> +		git_remotes_body(\@remotelist, $head);

Calling convention would change with my proposed change.

We might want to print only list of remotes (perhaps with number of
tracked branches) in the 'summary' view.  Just an idea...

>  	}
>  
>  	if (@forklist) {
> @@ -5551,9 +5572,9 @@ sub git_remotes {
>  	git_print_page_nav('','', $head,undef,$head, $heads_nav);
>  	git_print_header_div('summary', $project);
>  
> -	my @remotelist = git_get_heads_list(undef, 'remotes');
> +	my @remotelist = git_get_remotes();
>  	if (@remotelist) {
> -		git_heads_body(\@remotelist, $head);
> +		git_remotes_body(\@remotelist, $head);

Same here.

>  	}
>  	git_footer_html();
>  }
> -- 
> 1.7.3.rc1.230.g8b572
> 
> 

-- 
Jakub Narebski
Poland

Re: [PATCH 5/7] gitweb: auxiliary functions to group data

[Jakub Narebski]

On Fri, 17 Sep 2010, Giuseppe Bilotta wrote:
> On Fri, Sep 17, 2010 at 6:06 PM, Jakub Narebski <jnareb@gmail.com> wrote:
>> Giuseppe Bilotta wrote:
>>> On Fri, Sep 17, 2010 at 3:24 AM, Jakub Narebski <jnareb@gmail.com> wrote:
>>>>
>>>> ... but I think that having separate subroutines for opening and
>>>> closing tags is a bad design / bad API (except in some rare cases).
>>>> It is begging for unbalanced HTML.
>>>>
>>>> It would be better if it was a single subroutine wrapping 'div' around
>>>> contents given either as a string, or via callback (subroutine reference),
>>>> in my opinion.
>>>
>>> I'm not sure that in this case the string or callback approach would
>>> be any cleaner. I'll see if perl supports closures or something like
>>> that.
>>
>> Perl supports closures (thanks to anonymous subroutines 'sub { ... }'
>> and lexical variables 'my $var'), see perlsub and "Function Templates"
>> in perlref.
>>
>> I also recommend free ebook "Higher-Order Perl" http://hop.perl.plover.com/
> 
> Thanks for the suggestion. I'm still not convinced that such an
> implementation would be better though. Aside from the general
> aesthetical suckiness of passing closures around (and the experience
> is not any more pleasurable in Perl), there's also the matter of the
> calling convention to use. I can think of two options:
> 
> (1) we make the function callable as git_do_group($class, $id, sub {
> <closure that prints the content> }, <paramters that go to
> git_print_header_div>), which is somewhat illogical since we're
> specifying the content before the header, 

Why not

  git_do_group($class, $id, <print_header_div parameters>, sub { ... })

or even use subroutine prototypes?  We can use 'pop @_' to get last
argument of a subroutine.

[...]
> Overall, I still get the impression that the current API is
> considerably cleaner, even with the small counterweight of the risk of
> leaving groups open (which is not something so dramatic anyway, IMO).

Might be.

But as currently git_*group() is used in only one place, isn't it
premature generalization ;-) ?
-- 
Jakub Narebski
Poland

Re: [PATCH 6/7] gitweb: group styling

[Jakub Narebski]

Giuseppe Bilotta wrote:
> 2010/9/17 Jakub Narebski <jnareb@gmail.com>:
>> On Thu, 16 Sep 2010, Giuseppe Bilotta wrote:
>>
>>> +div.group {
>>> +     margin: .5em;
>>> +     border: 1px solid #d9d8d1;
>>> +     display: inline-block;
>>
>> Is this 'display: inline-block;' really necessary?  I think we can do
>> without it (and I've heard that there are problems with it, but those
>> might not matter in layout used by gitweb).
>>
>> Otherwise nice.
> 
> I'm not aware of problems with inline-block (I can check
> quirksmode.org though if necessary), but without it the divs will be
> as large as the _containing_ block, meaning that each div will be
> ultimately as large as the page. By using inline-block, instead, they
> become as large as the _contained_ stuff, thus fitting the inner
> table.

Errr... I am using old web browser that doesn't support 'inline-block',
so I don't know what it should look like.  I'll check later.

But graceful degradation to 'block' doesn't look too bad.
-- 
Jakub Narebski
Poland

Re: [PATCH 7/7] gitweb: group remote heads

[Jakub Narebski]

On Fri, 17 Sep 2010, Jakub Narebski wrote:
> On Thu, 16 Sep 2010, Giuseppe Bilotta wrote:

> > +		my @remoteheads = grep { $_->{'name'} =~ s!^\Q$remote\E/!! } @allheads;
> 
> Should we display remote even if it doesn't have any remote heads
> associated with it?

By the way, it would be place where we could limit number of 
remote-tracking branches displayed in each remote block.

-- 
Jakub Narebski
Poland

Re: [PATCH 5/7] gitweb: auxiliary functions to group data

[Giuseppe Bilotta]

On Fri, Sep 17, 2010 at 7:17 PM, Jakub Narebski <jnareb@gmail.com> wrote:
> On Fri, 17 Sep 2010, Giuseppe Bilotta wrote:
>> On Fri, Sep 17, 2010 at 6:06 PM, Jakub Narebski <jnareb@gmail.com> wrote:
>>> Giuseppe Bilotta wrote:
>>>> On Fri, Sep 17, 2010 at 3:24 AM, Jakub Narebski <jnareb@gmail.com> wrote:
>>>>>
>>>>> ... but I think that having separate subroutines for opening and
>>>>> closing tags is a bad design / bad API (except in some rare cases).
>>>>> It is begging for unbalanced HTML.
>>>>>
>>>>> It would be better if it was a single subroutine wrapping 'div' around
>>>>> contents given either as a string, or via callback (subroutine reference),
>>>>> in my opinion.
>>>>
>>>> I'm not sure that in this case the string or callback approach would
>>>> be any cleaner. I'll see if perl supports closures or something like
>>>> that.
>>>
>>> Perl supports closures (thanks to anonymous subroutines 'sub { ... }'
>>> and lexical variables 'my $var'), see perlsub and "Function Templates"
>>> in perlref.
>>>
>>> I also recommend free ebook "Higher-Order Perl" http://hop.perl.plover.com/
>>
>> Thanks for the suggestion. I'm still not convinced that such an
>> implementation would be better though. Aside from the general
>> aesthetical suckiness of passing closures around (and the experience
>> is not any more pleasurable in Perl), there's also the matter of the
>> calling convention to use. I can think of two options:
>>
>> (1) we make the function callable as git_do_group($class, $id, sub {
>> <closure that prints the content> }, <paramters that go to
>> git_print_header_div>), which is somewhat illogical since we're
>> specifying the content before the header,
>
> Why not
>
>  git_do_group($class, $id, <print_header_div parameters>, sub { ... })
>
> or even use subroutine prototypes?  We can use 'pop @_' to get last
> argument of a subroutine.

Ah, I hadn't thought about pop to get the last parameter. I think it
makes sense. Ok, I'll give this syntax a try,

> [...]
>> Overall, I still get the impression that the current API is
>> considerably cleaner, even with the small counterweight of the risk of
>> leaving groups open (which is not something so dramatic anyway, IMO).
>
> Might be.
>
> But as currently git_*group() is used in only one place, isn't it
> premature generalization ;-) ?

I noticed the same comment to patch 7/7, still writing the reply for
that patch. I believe the feature is useful enough to deem the
refactoring, and since it's written already why go back? ;-) (it also
makes the code cleaner IMO).


-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCH 7/7] gitweb: group remote heads

[Giuseppe Bilotta]

Hello,

as I mentioned, this patch was the one I had most doubts about. I will
therefore skip over the stylistic suggestions (which I _am_ following
for the next release of this patchset) and only reply to the more
technical remarks.

On Fri, Sep 17, 2010 at 6:54 PM, Jakub Narebski <jnareb@gmail.com> wrote:
> On Thu, 16 Sep 2010, Giuseppe Bilotta wrote:
>
>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
>> index 92551e4..66b5400 100755
>> --- a/gitweb/gitweb.perl
>> +++ b/gitweb/gitweb.perl
>> @@ -2758,6 +2758,16 @@ sub git_get_last_activity {
>>       return (undef, undef);
>>  }
>>
>> +sub git_get_remotes {

[snip]

>> +     my @remoteheads = git_get_heads_list($limit, 'remotes');
>> +     return (\@remotes, \@remoteheads);
>
> Why do you want for git_get_remotes() to also return remote-tracking
> branches (refs/remotes/)?  Those are separate issues, and I think it
> would be better API for git_get_remotes() to provide only list of
> remotes, i.e.
>
>  +     return @remotes;
>
> Especially that we might want in the summary view to only list remotes,
> without listing remote-tracking branches.
>
> That would require more changes to the code.

This is kind of the main issue with this patch. What do we want to do
with the remotes list in summary view and the remotes view? We
basically have three possibilities:

(1) we can make the remotes list in summary view be a 'reduced
remotes' view: this would make it behave the most similarly to the
other components of summary view
(2) we can make the remotes list be much more stripped down, by only
listing the remotes and possibly some summarizing property such as the
number of branches in it or when it was last updated
(3) we can make the remotes list be just a copy of the full remotes view.

The third option is surely the easiest to implement. The second option
with _only_ a list of remotes (no extra info) is also very easy to
implement _and_ fast to render. The second option with extra info, or
the first option, on the other hand, require the retrieval of some
additional data which, maybe due to my limited knowledge of git,
essentially means retrieving _all_ the remote heads and then doing the
filtering in gitweb. But once we're getting all the information, why
not display it all? isn't it faster to just display all of it, in
which case we go back to option 3?

If we go with option 3, it does make sense to get all remote names and
all remote branches at once, and thus to make the git_get_remotes()
call do all of the work.

>> +}
>> +
>>  sub git_get_references {
>>       my $type = shift || "";
>>       my %refs;
>> @@ -4979,7 +4989,7 @@ sub git_heads_body {
>>                     "<td class=\"link\">" .
>>                     $cgi->a({-href => href(action=>"shortlog", hash=>$ref{'fullname'})}, "shortlog") . " | " .
>>                     $cgi->a({-href => href(action=>"log", hash=>$ref{'fullname'})}, "log") . " | " .
>> -                   $cgi->a({-href => href(action=>"tree", hash=>$ref{'fullname'}, hash_base=>$ref{'name'})}, "tree") .
>> +                   $cgi->a({-href => href(action=>"tree", hash=>$ref{'fullname'}, hash_base=>$ref{'fullname'})}, "tree") .
>
> This is independent change, and should be in a separate commit, isn't it?

Probably yes, with an explanation of the why.

>>                     "</td>\n" .
>>                     "</tr>";
>>       }
>> @@ -4991,6 +5001,19 @@ sub git_heads_body {
>>       print "</table>\n";
>>  }
>>
>> +sub git_remotes_body {
>> +     my ($remotedata, $head) = @_;
>> +     my @remotenames = @{$remotedata->[0]};
>> +     my @allheads = @{$remotedata->[1]};
>
> Why not
>
>  +     my ($remotenames, $allheads, $head) = @_;
>
> Beside, isn't it $remote_heads and not $allheads?

I think it's a leftover name choice from the first version of the
patch. Can change.

>> +     foreach my $remote (@remotenames) {
>
> It would be then
>
>  +     foreach my $remote (@$remotenames) {
>
>> +             my @remoteheads = grep { $_->{'name'} =~ s!^\Q$remote\E/!! } @allheads;
>
> Should we display remote even if it doesn't have any remote heads
> associated with it?
>
> By the way, it would be place where we could limit number of
> remote-tracking branches displayed in each remote block.

But does it make sense to reduce the number of displayed branches
after we got the information about all of them? I think it depends on
what summary view is intended to do exactly.

>> +             git_begin_group("remotes", $remote, "remotes/$remote",$remote);
>> +             git_heads_body(\@remoteheads, $head);
>> +             git_end_group();
>
> This would have to be modified with change to git_begin_group() /
> / git_end_group().

Of course.

> BTW isn't it premature generalization?  It is only place AFAIKS that
> uses git_*_group() subroutines.

It's the only current use but I believe that, since it's factored out
now already and since it may be used in other views too (think:
grouping heads or tags by prefix) it might make sense to keep it this
way.

>> +     }
>> +
>> +}
>> +
>>  sub git_search_grep_body {
>>       my ($commitlist, $from, $to, $extra) = @_;
>>       $from = 0 unless defined $from;
>> @@ -5137,7 +5160,7 @@ sub git_summary {
>>       # there are more ...
>>       my @taglist  = git_get_tags_list(16);
>>       my @headlist = git_get_heads_list(16, 'heads');
>> -     my @remotelist = $remote_heads ? git_get_heads_list(16, 'remotes') : ();
>> +     my @remotelist = $remote_heads ? git_get_remotes(16) : ();
>
> No change of git_get_remotes() does only one thing: returning list
> of remotes.

See above about what should git_get_remotes() do. Even better, I was
thinking about git_get_remotes() returning a hash (mapping remote
names to the heads from that remote)

So the big question (which essentially determines the functionality
provided by this last patch in the set) is: what do we want to do in
summary view?

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCH 7/7] gitweb: group remote heads

[Jakub Narebski]

On Sun, 19 Sep 2010, Giuseppe Bilotta wrote:
> On Fri, Sep 17, 2010 at 6:54 PM, Jakub Narebski <jnareb@gmail.com> wrote:
>> On Thu, 16 Sep 2010, Giuseppe Bilotta wrote:
>>
>>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
>>> index 92551e4..66b5400 100755
>>> --- a/gitweb/gitweb.perl
>>> +++ b/gitweb/gitweb.perl
>>> @@ -2758,6 +2758,16 @@ sub git_get_last_activity {
>>>       return (undef, undef);
>>>  }
>>>
>>> +sub git_get_remotes {
> 
> [snip]
> 
>>> +     my @remoteheads = git_get_heads_list($limit, 'remotes');
>>> +     return (\@remotes, \@remoteheads);
>>
>> Why do you want for git_get_remotes() to also return remote-tracking
>> branches (refs/remotes/)?  Those are separate issues, and I think it
>> would be better API for git_get_remotes() to provide only list of
>> remotes, i.e.
>>
>>  +     return @remotes;
>>
>> Especially that we might want in the summary view to only list remotes,
>> without listing remote-tracking branches.
>>
>> That would require more changes to the code.
> 
> This is kind of the main issue with this patch. What do we want to do
> with the remotes list in summary view and the remotes view? We
> basically have three possibilities:
> 
> (1) we can make the remotes list in summary view be a 'reduced
> remotes' view: this would make it behave the most similarly to the
> other components of summary view
> (2) we can make the remotes list be much more stripped down, by only
> listing the remotes and possibly some summarizing property such as the
> number of branches in it or when it was last updated
> (3) we can make the remotes list be just a copy of the full remotes view.
> 
> The third option is surely the easiest to implement. The second option
> with _only_ a list of remotes (no extra info) is also very easy to
> implement _and_ fast to render. The second option with extra info, or
> the first option, on the other hand, require the retrieval of some
> additional data which, maybe due to my limited knowledge of git,
> essentially means retrieving _all_ the remote heads and then doing the
> filtering in gitweb. But once we're getting all the information, why
> not display it all? isn't it faster to just display all of it, in
> which case we go back to option 3?

When thinking about how display information about remotes and
remote-tracking branches in 'summary' view, we have to consider that
this view is meant to be what it says: a *summary*.  That is why both
the 'heads' and the 'tags' section displays only 15 items.

Without grouping remote heads the natural solution was to simply repeat
what we used for 'heads' subsection, namely limit displaying to 15
remote-tracking branches in 'remotes' subsection of the 'summary' view.

With grouping it is more complicated.  We can limit number of remote
head *per remote*, we can limit number of remotes... but what makes
IMVHO least sense is limit number of remote heads *then* do grouping.


The solution (1) i.e. limiting number of remote heads per remote, with
or without limiting number of remotes behaves, as you wrote, most
similarly to other components of 'summary' view.  On the other hand
with large number of remotes, and large number of remote heads in those
remotes it might be too large for a *summary* view.

The solution (3) i.e. displaying only list of remotes (perhaps limited
to 15 remotes) is simple and fast to render.  On the other hand it offers
least information and might be too little in the case of single remote.

> 
> If we go with option 3, it does make sense to get all remote names and
> all remote branches at once, and thus to make the git_get_remotes()
> call do all of the work.

Not if subroutine is called git_get_remotes(), IMVHO.  Perhaps
git_group_remotes()?
 
>>> @@ -4991,6 +5001,19 @@ sub git_heads_body {
>>>       print "</table>\n";
>>>  }
>>>
>>> +sub git_remotes_body {
>>> +     my ($remotedata, $head) = @_;
>>> +     my @remotenames = @{$remotedata->[0]};
>>> +     my @allheads = @{$remotedata->[1]};
>>
>> Why not
>>
>>  +     my ($remotenames, $allheads, $head) = @_;
>>
>> Beside, isn't it $remote_heads and not $allheads?
> 
> I think it's a leftover name choice from the first version of the
> patch. Can change.

O.K.

>>> +     foreach my $remote (@remotenames) {
>>
>> It would be then
>>
>>  +     foreach my $remote (@$remotenames) {
>>
>>> +             my @remoteheads = grep { $_->{'name'} =~ s!^\Q$remote\E/!! } @allheads;
>>
>> Should we display remote even if it doesn't have any remote heads
>> associated with it?
>>
>> By the way, it would be place where we could limit number of
>> remote-tracking branches displayed in each remote block.
> 
> But does it make sense to reduce the number of displayed branches
> after we got the information about all of them? I think it depends on
> what summary view is intended to do exactly.

Right.  But I think limit then group is not a very good choice from
all other available.
 
>>> +             git_begin_group("remotes", $remote, "remotes/$remote",$remote);
>>> +             git_heads_body(\@remoteheads, $head);
>>> +             git_end_group();
>>
>> This would have to be modified with change to git_begin_group() /
>> / git_end_group().
> 
> Of course.
> 
>> BTW isn't it premature generalization?  It is only place AFAIKS that
>> uses git_*_group() subroutines.
> 
> It's the only current use but I believe that, since it's factored out
> now already and since it may be used in other views too (think:
> grouping heads or tags by prefix) it might make sense to keep it this
> way.

If it could be used for other blocks (like 'readme', or 'heads',
or 'tags') itself it would be even better.
 
>>> +     }
>>> +
>>> +}
>>> +
>>>  sub git_search_grep_body {
>>>       my ($commitlist, $from, $to, $extra) = @_;
>>>       $from = 0 unless defined $from;
>>> @@ -5137,7 +5160,7 @@ sub git_summary {
>>>       # there are more ...
>>>       my @taglist  = git_get_tags_list(16);
>>>       my @headlist = git_get_heads_list(16, 'heads');
>>> -     my @remotelist = $remote_heads ? git_get_heads_list(16, 'remotes') : ();
>>> +     my @remotelist = $remote_heads ? git_get_remotes(16) : ();
>>
>> No change of git_get_remotes() does only one thing: returning list
>> of remotes.
> 
> See above about what should git_get_remotes() do. Even better, I was
> thinking about git_get_remotes() returning a hash (mapping remote
> names to the heads from that remote)

Wouldn't it be then named git_group_remotes() or something like that?

If remote doesn't have remote-tracking branches associated with it
(e.g. push-only remote, or remote predating separate remotes layout)
the value would be undef for given remote as key in hash.
 
> So the big question (which essentially determines the functionality
> provided by this last patch in the set) is: what do we want to do in
> summary view?

-- 
Jakub Narebski
Poland

Re: [PATCH 7/7] gitweb: group remote heads

[Giuseppe Bilotta]

On Mon, Sep 20, 2010 at 1:02 AM, Jakub Narebski <jnareb@gmail.com> wrote:
>
> When thinking about how display information about remotes and
> remote-tracking branches in 'summary' view, we have to consider that
> this view is meant to be what it says: a *summary*.  That is why both
> the 'heads' and the 'tags' section displays only 15 items.
>
> Without grouping remote heads the natural solution was to simply repeat
> what we used for 'heads' subsection, namely limit displaying to 15
> remote-tracking branches in 'remotes' subsection of the 'summary' view.
>
> With grouping it is more complicated.  We can limit number of remote
> head *per remote*, we can limit number of remotes... but what makes
> IMVHO least sense is limit number of remote heads *then* do grouping.

This is something I absolutely agree with, BTW. It is the way it's
implemented currently because it was the quickest way to ship a
prototype out for discussion.

> The solution (1) i.e. limiting number of remote heads per remote, with
> or without limiting number of remotes behaves, as you wrote, most
> similarly to other components of 'summary' view.  On the other hand
> with large number of remotes, and large number of remote heads in those
> remotes it might be too large for a *summary* view.

So you maintain that limiting the amount of data in summary view
should be primary wrt to limiting the amount of time?

> The solution (3) i.e. displaying only list of remotes (perhaps limited
> to 15 remotes) is simple and fast to render.  On the other hand it offers
> least information and might be too little in the case of single remote.

If time spent processing is not an issue, we can retrieve the number
of heads for each remote and display that, for example. Or even play
with some more dynamic stuff like making each group collapsible,
starting with it collapsed and then display the content when the user
hovers it with the mouse, for example.

>> If we go with option 3, it does make sense to get all remote names and
>> all remote branches at once, and thus to make the git_get_remotes()
>> call do all of the work.
>
> Not if subroutine is called git_get_remotes(), IMVHO.  Perhaps
> git_group_remotes()?

That I can do.

>> It's the only current use but I believe that, since it's factored out
>> now already and since it may be used in other views too (think:
>> grouping heads or tags by prefix) it might make sense to keep it this
>> way.
>
> If it could be used for other blocks (like 'readme', or 'heads',
> or 'tags') itself it would be even better.

That's a possibility, although it would change the layout some (which
is not necessarily a bad thing)

> If remote doesn't have remote-tracking branches associated with it
> (e.g. push-only remote, or remote predating separate remotes layout)
> the value would be undef for given remote as key in hash.

Yes, this is something I have to take into consideration. Skip
displaying them is probably the best idea (unless we have other ways
to gather information about them).

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCH 7/7] gitweb: group remote heads

[Jakub Narebski]

Giuseppe Bilotta wrote:
> On Mon, Sep 20, 2010 at 1:02 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> >
> > When thinking about how display information about remotes and
> > remote-tracking branches in 'summary' view, we have to consider that
> > this view is meant to be what it says: a *summary*.  That is why both
> > the 'heads' and the 'tags' section displays only 15 items.
> >
> > Without grouping remote heads the natural solution was to simply repeat
> > what we used for 'heads' subsection, namely limit displaying to 15
> > remote-tracking branches in 'remotes' subsection of the 'summary' view.
> >
> > With grouping it is more complicated.  We can limit number of remote
> > head *per remote*, we can limit number of remotes... but what makes
> > IMVHO least sense is limit number of remote heads *then* do grouping.
> 
> This is something I absolutely agree with, BTW. It is the way it's
> implemented currently because it was the quickest way to ship a
> prototype out for discussion.

All right.  Prototyping is good.

> > The solution (1) i.e. limiting number of remote heads per remote, with
> > or without limiting number of remotes behaves, as you wrote, most
> > similarly to other components of 'summary' view.  On the other hand
> > with large number of remotes, and large number of remote heads in those
> > remotes it might be too large for a *summary* view.
> 
> So you maintain that limiting the amount of data in summary view
> should be primary wrt to limiting the amount of time?

Well, what really affect gitweb performance is calling git commands, both
because of fork overhead, and because it means disk access (and gitweb
performance from what I have heard is affected mainly by IO, and not CPU).
With grouping (displaying remotes) the difference between displaying
remote-tracking branches (or information from them) and not displaying
them is an argument to git-for-each-ref.  So I don't think it would 
affect performance much.

> > The solution (3) i.e. displaying only list of remotes (perhaps limited
> > to 15 remotes) is simple and fast to render.  On the other hand it offers
> > least information and might be too little in the case of single remote.
> 
> If time spent processing is not an issue, we can retrieve the number
> of heads for each remote and display that, for example. Or even play
> with some more dynamic stuff like making each group collapsible,
> starting with it collapsed and then display the content when the user
> hovers it with the mouse, for example.

The dynamic stuff is IMHO a good idea... provided we can either do it
without JavaScript, or we can ensure that browser supports JavaScript
(see current hack used for turning 'blame' into 'blame_incremental'
view in gitweb).

Yet another solution would be to display only abbreviated list of remotes
if its more of them than some threshold, and list remotes with abbreviated
list of remote-tracking branches if there are only a few remotes.
 
> > > If we go with option 3, it does make sense to get all remote names and
> > > all remote branches at once, and thus to make the git_get_remotes()
> > > call do all of the work.
> >
> > Not if subroutine is called git_get_remotes(), IMVHO.  Perhaps
> > git_group_remotes()?
> 
> That I can do.

> > If remote doesn't have remote-tracking branches associated with it
> > (e.g. push-only remote, or remote predating separate remotes layout)
> > the value would be undef for given remote as key in hash.
> 
> Yes, this is something I have to take into consideration. Skip
> displaying them is probably the best idea (unless we have other ways
> to gather information about them).

Right.

P.S. It is not necessary for this series, but I think we should think
about "single remote" view... also because your code currently links
to such views, which do not exist yet (remotes/<remote> in path_info:
how it would be represented in CGI query format?).

-- 
Jakub Narebski
Poland

Re: [PATCH 7/7] gitweb: group remote heads

[Giuseppe Bilotta]

On Mon, Sep 20, 2010 at 10:59 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> Giuseppe Bilotta wrote:
>> On Mon, Sep 20, 2010 at 1:02 AM, Jakub Narebski <jnareb@gmail.com> wrote:
>> > The solution (1) i.e. limiting number of remote heads per remote, with
>> > or without limiting number of remotes behaves, as you wrote, most
>> > similarly to other components of 'summary' view.  On the other hand
>> > with large number of remotes, and large number of remote heads in those
>> > remotes it might be too large for a *summary* view.
>>
>> So you maintain that limiting the amount of data in summary view
>> should be primary wrt to limiting the amount of time?
>
> Well, what really affect gitweb performance is calling git commands, both
> because of fork overhead, and because it means disk access (and gitweb
> performance from what I have heard is affected mainly by IO, and not CPU).
> With grouping (displaying remotes) the difference between displaying
> remote-tracking branches (or information from them) and not displaying
> them is an argument to git-for-each-ref.  So I don't think it would
> affect performance much.

Getting the list of remote branches is, I would say, the most
IO-intensive operation. I'm not sure how much I/O it would do though,
even with a large number of remotes and heads. So maybe always gather
all the information is the way to go.

>> > The solution (3) i.e. displaying only list of remotes (perhaps limited
>> > to 15 remotes) is simple and fast to render.  On the other hand it offers
>> > least information and might be too little in the case of single remote.
>>
>> If time spent processing is not an issue, we can retrieve the number
>> of heads for each remote and display that, for example. Or even play
>> with some more dynamic stuff like making each group collapsible,
>> starting with it collapsed and then display the content when the user
>> hovers it with the mouse, for example.
>
> The dynamic stuff is IMHO a good idea... provided we can either do it
> without JavaScript, or we can ensure that browser supports JavaScript
> (see current hack used for turning 'blame' into 'blame_incremental'
> view in gitweb).

What I had in mind was something that is very easy to implement with CSS only.

> Yet another solution would be to display only abbreviated list of remotes
> if its more of them than some threshold, and list remotes with abbreviated
> list of remote-tracking branches if there are only a few remotes.

So something like this:

(1) if there are more than N remotes, only show N remote _names_ (no heads)
(2) if there are no more than N remotes, show all remote names, each
with no more than M heads

(with N and M to be decided, e.g. the usual 16)

>> Yes, this is something I have to take into consideration. Skip
>> displaying them is probably the best idea (unless we have other ways
>> to gather information about them).
>
> Right.

For this, it would be nice to have `git remote show`, but even if I
sent a patch to this effect gitweb should probably be left able to
cope with older git versions not supporting it ...

> P.S. It is not necessary for this series, but I think we should think
> about "single remote" view... also because your code currently links
> to such views, which do not exist yet (remotes/<remote> in path_info:
> how it would be represented in CGI query format?).

Maybe pass the remote name as head parameter?

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCH 7/7] gitweb: group remote heads

[Jakub Narebski]

On Mon, Sep 20, 2010, Giuseppe Bilotta wrote:
> On Mon, Sep 20, 2010 at 10:59 AM, Jakub Narebski <jnareb@gmail.com> wrote:
>> Giuseppe Bilotta wrote:
>>> On Mon, Sep 20, 2010 at 1:02 AM, Jakub Narebski <jnareb@gmail.com> wrote:

>>>> The solution (1) i.e. limiting number of remote heads per remote, with
>>>> or without limiting number of remotes behaves, as you wrote, most
>>>> similarly to other components of 'summary' view.  On the other hand
>>>> with large number of remotes, and large number of remote heads in those
>>>> remotes it might be too large for a *summary* view.
>>>
>>> So you maintain that limiting the amount of data in summary view
>>> should be primary wrt to limiting the amount of time?
>>
>> Well, what really affect gitweb performance is calling git commands, both
>> because of fork overhead, and because it means disk access (and gitweb
>> performance from what I have heard is affected mainly by IO, and not CPU).
>> With grouping (displaying remotes) the difference between displaying
>> remote-tracking branches (or information from them) and not displaying
>> them is an argument to git-for-each-ref.  So I don't think it would
>> affect performance much.
> 
> Getting the list of remote branches is, I would say, the most
> IO-intensive operation. I'm not sure how much I/O it would do though,
> even with a large number of remotes and heads. So maybe always gather
> all the information is the way to go.

Well, that depends on the number of remote-tracking branches, and whether
those refs are packed (see git-pack-refs).  But I agree that getting list
of all remote branches might be I/O hit... though I am not sure if it
would be large enough to visibly affect gitweb performance.
 
By the way, besides the solution described below (list only remotes if
there are many of them), we could make 'remote_heads' be not simply
boolean, but to be multiple-choice feature, configuring how 'remotes'
view and remotes section in 'summary' page looks like.  But that might
be overengineering.

>>> If time spent processing is not an issue, we can retrieve the number
>>> of heads for each remote and display that, for example. Or even play
>>> with some more dynamic stuff like making each group collapsible,
>>> starting with it collapsed and then display the content when the user
>>> hovers it with the mouse, for example.
>>
>> The dynamic stuff is IMHO a good idea... provided we can either do it
>> without JavaScript, or we can ensure that browser supports JavaScript
>> (see current hack used for turning 'blame' into 'blame_incremental'
>> view in gitweb).
> 
> What I had in mind was something that is very easy to implement with
> CSS only. 

Do I understand correctly that you would utilize ':hover' pseudoclass
and 'display: none;' style?  How ergonomic would this solution be?
 
>> Yet another solution would be to display only abbreviated list of remotes
>> if its more of them than some threshold, and list remotes with abbreviated
>> list of remote-tracking branches if there are only a few remotes.
> 
> So something like this:
> 
> (1) if there are more than N remotes, only show N remote _names_ (no heads)
> (2) if there are no more than N remotes, show all remote names, each
> with no more than M heads
> 
> (with N and M to be decided, e.g. the usual 16)

Right.

>>> Yes, this is something I have to take into consideration. Skip
>>> displaying them is probably the best idea (unless we have other ways
>>> to gather information about them).
>>
>> Right.
> 
> For this, it would be nice to have `git remote show`, but even if I
> sent a patch to this effect gitweb should probably be left able to
> cope with older git versions not supporting it ...
> 
>> P.S. It is not necessary for this series, but I think we should think
>> about "single remote" view... also because your code currently links
>> to such views, which do not exist yet (remotes/<remote> in path_info:
>> how it would be represented in CGI query format?).
> 
> Maybe pass the remote name as head parameter?

In the meantime, while we don't have 'remote' view for a single remote
(something like "git remote show"), it would be good if the header for
individual remotes didn't lead to "'remotes/<remote>' action".

-- 
Jakub Narebski
Poland

Re: [PATCH 7/7] gitweb: group remote heads

[Giuseppe Bilotta]

On Wed, Sep 22, 2010 at 10:34 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> On Mon, Sep 20, 2010, Giuseppe Bilotta wrote:
>> On Mon, Sep 20, 2010 at 10:59 AM, Jakub Narebski <jnareb@gmail.com> wrote:
>>> Giuseppe Bilotta wrote:
>>>> On Mon, Sep 20, 2010 at 1:02 AM, Jakub Narebski <jnareb@gmail.com> wrote:
>
>>>>> The solution (1) i.e. limiting number of remote heads per remote, with
>>>>> or without limiting number of remotes behaves, as you wrote, most
>>>>> similarly to other components of 'summary' view.  On the other hand
>>>>> with large number of remotes, and large number of remote heads in those
>>>>> remotes it might be too large for a *summary* view.
>>>>
>>>> So you maintain that limiting the amount of data in summary view
>>>> should be primary wrt to limiting the amount of time?
>>>
>>> Well, what really affect gitweb performance is calling git commands, both
>>> because of fork overhead, and because it means disk access (and gitweb
>>> performance from what I have heard is affected mainly by IO, and not CPU).
>>> With grouping (displaying remotes) the difference between displaying
>>> remote-tracking branches (or information from them) and not displaying
>>> them is an argument to git-for-each-ref.  So I don't think it would
>>> affect performance much.
>>
>> Getting the list of remote branches is, I would say, the most
>> IO-intensive operation. I'm not sure how much I/O it would do though,
>> even with a large number of remotes and heads. So maybe always gather
>> all the information is the way to go.
>
> Well, that depends on the number of remote-tracking branches, and whether
> those refs are packed (see git-pack-refs).  But I agree that getting list
> of all remote branches might be I/O hit... though I am not sure if it
> would be large enough to visibly affect gitweb performance.
>
> By the way, besides the solution described below (list only remotes if
> there are many of them), we could make 'remote_heads' be not simply
> boolean, but to be multiple-choice feature, configuring how 'remotes'
> view and remotes section in 'summary' page looks like.  But that might
> be overengineering.

A config option for maximum number of remotes and maximum number of
heads per remotes (in summary) with sane defaults (16/16?) maybe?

>> What I had in mind was something that is very easy to implement with
>> CSS only.
>
> Do I understand correctly that you would utilize ':hover' pseudoclass
> and 'display: none;' style?  How ergonomic would this solution be?

That was my idea, yes. It _can_ get a little confusing without CSS3
transitions, due to the 'flash roll down' of the remote heads list. I
will try to cook up a preview sometime this week.

>>> P.S. It is not necessary for this series, but I think we should think
>>> about "single remote" view... also because your code currently links
>>> to such views, which do not exist yet (remotes/<remote> in path_info:
>>> how it would be represented in CGI query format?).
>>
>> Maybe pass the remote name as head parameter?
>
> In the meantime, while we don't have 'remote' view for a single remote
> (something like "git remote show"), it would be good if the header for
> individual remotes didn't lead to "'remotes/<remote>' action".

True. I'll fix that for the next rehash.

-- 
Giuseppe "Oblomov" Bilotta