[PATCHv6 00/10] gitweb: remote_heads feature

[PATCHv6 00/10] gitweb: remote_heads feature

[Giuseppe Bilotta]

In this 6th version of the remote heads feature for gitweb, much of the
internal API has been cleaned up thanks to the suggestion from Jakub and
others. There were also some other stylistic changes, and a few patches were
reordered/merged.

The first 4 patches are rather straightforward and can probably go
straight in. The 5th patch is a bugfix for something that is only
triggered by the name manipulation done with the remote heads, but can
probably be useful even without the rest of the series.

Patch 7 provides 'single remote view', depending on patch 6 for
improved visuals of the page header.

Patches 8 and 9 provide some infrastructure for the grouped remote heads
display introduced in the last patch.

As usual, you can see it live @ http://git.oblomov.eu (look at the rbot
project in particular for the "lots of remotes" case), comments welcome.

Giuseppe Bilotta (10):
  gitweb: introduce remote_heads feature
  gitweb: git_get_heads_list accepts an optional list of refs.
  gitweb: separate heads and remotes lists
  gitweb: nagivation menu for tags, heads and remotes
  gitweb: use fullname as hash_base in heads link
  gitweb: allow action specialization in page header
  gitweb: remotes view for a single remote
  gitweb: refactor repository URL printing
  gitweb: provide a routine to display (sub)sections
  gitweb: group remote heads by remote

 gitweb/gitweb.perl       |  283 ++++++++++++++++++++++++++++++++++++++++++++--
 gitweb/static/gitweb.css |    6 +
 2 files changed, 279 insertions(+), 10 deletions(-)

-- 
1.7.3.68.g6ec8

[PATCHv6 01/10] gitweb: introduce remote_heads feature

[Giuseppe Bilotta]

With this feature enabled, remote heads 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 |   18 ++++++++++++++++--
 1 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 253f41a..0e71749 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -493,6 +493,18 @@ our %feature = (
 		'sub' => sub { feature_bool('highlight', @_) },
 		'override' => 0,
 		'default' => [0]},
+
+	# Enable displaying of remote heads 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 {
@@ -3160,10 +3172,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;
@@ -3174,7 +3188,7 @@ 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{'name'}  = $name;
 		$ref_item{'id'}    = $hash;
-- 
1.7.3.68.g6ec8

[PATCHv6 02/10] 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', 'remotes') or ('heads')
depending on whether the 'remote_heads' feature is enabled or not.

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 0e71749..e75d3f4 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3169,15 +3169,18 @@ sub parse_from_to_diffinfo {
 ## parse to array of hashes functions
 
 sub git_get_heads_list {
-	my $limit = shift;
+	my ($limit, @classes) = @_;
+	unless (defined @classes) {
+		my $remote_heads = gitweb_check_feature('remote_heads');
+		@classes = ('heads', $remote_heads ? 'remotes' : ());
+	}
+	my @patterns = map { "refs/$_" } @classes;
 	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' : ())
+		@patterns
 		or return;
 	while (my $line = <$fd>) {
 		my %ref_item;
-- 
1.7.3.68.g6ec8

[PATCHv6 03/10] 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 e75d3f4..667c11b 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -719,6 +719,7 @@ our %actions = (
 	"log" => \&git_log,
 	"patch" => \&git_patch,
 	"patches" => \&git_patches,
+	"remotes" => \&git_remotes,
 	"rss" => \&git_rss,
 	"atom" => \&git_atom,
 	"search" => \&git_search,
@@ -5126,6 +5127,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);
 
@@ -5133,7 +5135,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');
 
@@ -5211,6 +5214,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,
@@ -5518,13 +5528,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.68.g6ec8

[PATCHv6 04/10] gitweb: nagivation menu for tags, heads and remotes

[Giuseppe Bilotta]

tags, heads and remotes are all views that inspect a (particular class
of) refs, so allow the user to easily switch between them by adding
the appropriate navigation submenu to each view.

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 667c11b..f4592ad 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3736,6 +3736,19 @@ sub git_print_page_nav {
 	      "</div>\n";
 }
 
+# returns a submenu for the nagivation of the refs views (tags, heads,
+# remotes) with the current view disabled and the remotes view only
+# available if the feature is enabled
+sub format_ref_views {
+	my ($current) = @_;
+	my @ref_views = qw{tags heads};
+	push @ref_views, 'remotes' if gitweb_check_feature('remote_heads');
+	return join " | ", map {
+		$_ eq $current ? $_ :
+		$cgi->a({-href => href(action=>$_)}, $_)
+	} @ref_views
+}
+
 sub format_paging_nav {
 	my ($action, $page, $has_next_link) = @_;
 	my $paging_nav;
@@ -5512,7 +5525,7 @@ sub git_blame_data {
 sub git_tags {
 	my $head = git_get_head_hash($project);
 	git_header_html();
-	git_print_page_nav('','', $head,undef,$head);
+	git_print_page_nav('','', $head,undef,$head,format_ref_views('tags'));
 	git_print_header_div('summary', $project);
 
 	my @tagslist = git_get_tags_list();
@@ -5525,7 +5538,7 @@ sub git_tags {
 sub git_heads {
 	my $head = git_get_head_hash($project);
 	git_header_html();
-	git_print_page_nav('','', $head,undef,$head);
+	git_print_page_nav('','', $head,undef,$head,format_ref_views('heads'));
 	git_print_header_div('summary', $project);
 
 	my @headslist = git_get_heads_list(undef, 'heads');
@@ -5541,7 +5554,7 @@ sub git_remotes {
 
 	my $head = git_get_head_hash($project);
 	git_header_html();
-	git_print_page_nav('','', $head,undef,$head);
+	git_print_page_nav('','', $head,undef,$head,format_ref_views('remotes'));
 	git_print_header_div('summary', $project);
 
 	my @remotelist = git_get_heads_list(undef, 'remotes');
-- 
1.7.3.68.g6ec8

[PATCHv6 05/10] gitweb: use fullname as hash_base in heads link

[Giuseppe Bilotta]

Otherwise, if names are manipulated for display, the link will point to
the wrong head.

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index f4592ad..a381892 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -4991,7 +4991,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>";
 	}
-- 
1.7.3.68.g6ec8

[PATCHv6 06/10] gitweb: allow action specialization in page header

[Giuseppe Bilotta]

An optional -action_extra parameter is given to git_header_html() to
identify a variant of the action that is being displayed. For example,
this can be used to specify that the remotes view is being used for a
specific remote and not to display all remotes.

When -action_extra is provided, the action name in the header will be
turned into a link to the action without any arguments or parameters, to
provide a quick link to the non-specific variant of the action.

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index a381892..3612e63 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3529,7 +3529,15 @@ EOF
 	if (defined $project) {
 		print $cgi->a({-href => href(action=>"summary")}, esc_html($project));
 		if (defined $action) {
-			print " / $action";
+			my $action_print = $action ;
+			if (defined $opts{-action_extra}) {
+				$action_print = $cgi->a({-href => href(action=>$action)},
+					$action);
+			}
+			print " / $action_print";
+		}
+		if (defined $opts{-action_extra}) {
+			print " / $opts{-action_extra}";
 		}
 		print "\n";
 	}
-- 
1.7.3.68.g6ec8

[PATCHv6 07/10] gitweb: remotes view for a single remote

[Giuseppe Bilotta]

When 'remotes' view is passed the 'hash' parameter, interpret it as the
name of a remote and limit the view the the heads of that remote.

In single-remote view we let the user switch easily to the default
remotes view by specifying an -action_extra for the page header and by
enabling the 'remotes' link in the reference navigation submenu.

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 3612e63..aa80748 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -5561,14 +5561,35 @@ sub git_remotes {
 		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,format_ref_views('remotes'));
-	git_print_header_div('summary', $project);
+	my $remote = $input_params{'hash'};
+
+	my @remotelist;
+
+	if (defined $remote) {
+		# only display the heads in a given remote
+		@remotelist = map {
+			my $ref = $_ ;
+			$ref->{'name'} =~ s!^$remote/!!;
+			$ref
+		} git_get_heads_list(undef, "remotes/$remote");
+	} else {
+		@remotelist = git_get_heads_list(undef, 'remotes');
+	}
+
+	git_header_html(undef, undef, -action_extra => $remote);
+	git_print_page_nav('', '',  $head, undef, $head,
+		format_ref_views($remote ? '' : 'remotes'));
+
+	if (defined $remote) {
+		git_print_header_div('remotes', "$remote remote for $project");
+	} else {
+		git_print_header_div('summary', "$project remotes");
+	}
 
-	my @remotelist = git_get_heads_list(undef, 'remotes');
 	if (@remotelist) {
 		git_heads_body(\@remotelist, $head);
 	}
+
 	git_footer_html();
 }
 
-- 
1.7.3.68.g6ec8

[PATCHv6 08/10] gitweb: refactor repository URL printing

[Giuseppe Bilotta]

Factor out the code to display the repository URL(s) from summary view
into a format_rep_url() routine.

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index aa80748..ec2626d 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3800,6 +3800,11 @@ sub git_print_header_div {
 	      "\n</div>\n";
 }
 
+sub format_repo_url {
+	my ($name, $url) = @_;
+	return "<tr class=\"metadata_url\"><td>$name</td><td>$url</td></tr>\n";
+}
+
 sub print_local_time {
 	print format_local_time(@_);
 }
@@ -5183,7 +5188,7 @@ sub git_summary {
 	@url_list = map { "$_/$project" } @git_base_url_list unless @url_list;
 	foreach my $git_url (@url_list) {
 		next unless $git_url;
-		print "<tr class=\"metadata_url\"><td>$url_tag</td><td>$git_url</td></tr>\n";
+		print format_repo_url($url_tag, $git_url);
 		$url_tag = "";
 	}
 
-- 
1.7.3.68.g6ec8

[PATCHv6 09/10] gitweb: provide a routine to display (sub)sections

[Giuseppe Bilotta]

The routine puts the given contento into a DIV element, automatically
adding a header div. The content can be provided as a standard scalar
value (which is used as-is), as a scalar ref (which is HTML-escaped), as
a function reference to be executed, or as a file handle to be dumped.

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index ec2626d..feca8bc 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3805,6 +3805,44 @@ sub format_repo_url {
 	return "<tr class=\"metadata_url\"><td>$name</td><td>$url</td></tr>\n";
 }
 
+# Group output by placing it in a DIV element and adding a header.
+# Options for start_div() can be provided by passing a hash reference as the
+# first parameter to the function.
+# Options to git_print_header_div() can be provided by passing an array
+# reference. This must follow the options to start_div if they are present.
+# The content can be a scalar, which is output as-is, a scalar reference, which
+# is output after html escaping, an IO handle passed either as *handle or
+# *handle{IO}, or a function reference. In the latter case all following
+# parameters will be taken as argument to the content function call.
+sub git_print_section {
+	my ($div_args, $header_args, $content);
+	my $arg = shift;
+	if (ref($arg) eq 'HASH') {
+		$div_args = $arg;
+		$arg = shift;
+	}
+	if (ref($arg) eq 'ARRAY') {
+		$header_args = $arg;
+		$arg = shift;
+	}
+	$content = $arg;
+
+	print $cgi->start_div($div_args);
+	git_print_header_div(@$header_args);
+
+	if (ref($content) eq 'CODE') {
+		$content->(@_);
+	} elsif (ref($content) eq 'SCALAR') {
+		print esc_html($$content);
+	} elsif (ref($content) eq 'GLOB' or ref($content) eq 'IO::Handle') {
+		print <$content>;
+	} elsif (!ref($content) && defined($content)) {
+		print $content;
+	}
+
+	print $cgi->end_div;
+}
+
 sub print_local_time {
 	print format_local_time(@_);
 }
-- 
1.7.3.68.g6ec8

[PATCHv6 10/10] gitweb: group remote heads by remote

[Giuseppe Bilotta]

In remote and summary view, display a block for each remote, with the
fetch and push URL(s) as well as the list of the remote heads.

In summary view, if the number of remotes is higher than a prescribed
limit, only display the first <limit> remotes and their fetch and push
urls, without any heads information and without grouping.

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index feca8bc..5f08dcc 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2772,6 +2772,66 @@ sub git_get_last_activity {
 	return (undef, undef);
 }
 
+# Returns a hash ref mapping remote names to their fetch and push URLs.
+# We return a hash ref rather than a hash so that a simple check with defined
+# can be used to tell apart the "no remotes" case from other kinds of
+# failures.
+#
+# It is possible to limit the retrieved remotes either by number
+# (specifying a -limit parameter) or by name (-wanted parameter).
+#
+# When a single remote is wanted, we cannot use 'git remote show -n' because
+# that command always work (assuming it's a remote URL if it's not defined),
+# and we cannot use 'git remote show' because that would try to make a network
+# roundtrip. So the only way to find if that particular remote is defined is to
+# walk the list provided by 'git remote -v' and stop if and when we find what
+# we want.
+sub git_get_remotes_list {
+	my %params = @_;
+	my $limit = $params{-limit};
+	my $wanted = $params{-wanted};
+	my %remotes = ();
+
+	open my $fd, '-|' , git_cmd(), 'remote', '-v';
+	return unless $fd;
+	while (my $remote = <$fd>) {
+		chomp $remote;
+		$remote =~ s!\t(.*?)\s+\((\w+)\)$!!;
+		next if $wanted and not $remote eq $wanted;
+		my ($url, $key) = ($1, $2);
+
+		# a remote may appear more than once because of multiple URLs,
+		# so if this is a remote we know already, be sure to continue,
+		# lest we end up with a remote for which we get the fetch URL
+		# bot not the push URL, for example
+		my $more = exists $remotes{$remote};
+		$more ||= defined $limit ? (keys(%remotes) <= $limit) : 1;
+		if ($more) {
+			$remotes{$remote} ||= { 'heads' => () };
+			$remotes{$remote}{$key} = $url;
+		} else {
+			last;
+		}
+	}
+	close $fd or return;
+	return \%remotes;
+}
+
+# Takes a hash of remotes as first parameter and fills it by adding the
+# available remote heads for each of the indicated remotes.
+# A maximum number of heads can also be specified.
+sub git_get_remote_heads {
+	my ($remotes, $limit) = @_;
+	my @heads = map { "remotes/$_" } keys %$remotes;
+	my @remoteheads = git_get_heads_list($limit, @heads);
+	foreach (keys %$remotes) {
+		my $remote = $_;
+		$remotes->{$remote}{'heads'} = [ grep {
+			$_->{'name'} =~ s!^$remote/!!
+			} @remoteheads ];
+	}
+}
+
 sub git_get_references {
 	my $type = shift || "";
 	my %refs;
@@ -5054,6 +5114,100 @@ sub git_heads_body {
 	print "</table>\n";
 }
 
+# Display a single remote block
+sub git_remote_body {
+	my ($remote, $rdata, $limit, $head) = @_;
+
+	my $heads = $rdata->{'heads'};
+	my $fetch = $rdata->{'fetch'};
+	my $push = $rdata->{'push'};
+
+	my $urls = "<table class=\"projects_list\">\n" ;
+
+	if (defined $fetch) {
+		if ($fetch eq $push) {
+			$urls .= format_repo_url("URL", $fetch);
+		} else {
+			$urls .= format_repo_url("Fetch URL", $fetch);
+			$urls .= format_repo_url("Push URL", $push) if defined $push;
+		}
+	} elsif (defined $push) {
+		$urls .= format_repo_url("Push URL", $push);
+	} else {
+		$urls .= format_repo_url("", "No remote URL");
+	}
+
+	$urls .= "</table>\n";
+
+	my ($maxheads, $dots);
+	if (defined $limit) {
+		$maxheads = $limit - 1;
+		if ($#{$heads} > $maxheads) {
+			$dots = $cgi->a({-href => href(action=>"remotes", hash=>$remote)}, "...");
+		}
+	}
+
+	print $urls;
+	git_heads_body($heads, $head, 0, $maxheads, $dots);
+}
+
+# Display a list of remote names with the respective fetch and push URLs
+# This routine only gets called when there are more remotes than the given
+# limit, so we know that we have to append an ellipsis to the table and
+# that we have to pop one of the keys.
+sub git_remotes_list {
+	my ($remotedata) = @_;
+	print "<table class=\"heads\">\n";
+	my $alternate = 1;
+	my @keys = sort keys %$remotedata;
+	pop @keys;
+
+	while (my $remote = shift @keys) {
+		my $rdata = $remotedata->{$remote};
+		my $fetch = $rdata->{'fetch'};
+		my $push = $rdata->{'push'};
+		if ($alternate) {
+			print "<tr class=\"dark\">\n";
+		} else {
+			print "<tr class=\"light\">\n";
+		}
+		$alternate ^= 1;
+		print "<td>" .
+		      $cgi->a({-href=> href(action=>'remotes', hash=>$remote),
+			       -class=> "list name"},esc_html($remote)) . "</td>";
+		print "<td class=\"link\">" .
+		      (defined $fetch ? $cgi->a({-href=> $fetch}, "fetch") : "fetch") .
+		      " | " .
+		      (defined $push ? $cgi->a({-href=> $push}, "push") : "push") .
+		      "</td>";
+
+		print "</tr>\n";
+	}
+	print "<tr>\n" .
+	      "<td colspan=\"3\">" .
+	      $cgi->a({-href => href(action=>"remotes")}, "...") .
+	      "</td>\n" . "</tr>\n";
+	print "</table>";
+}
+
+# Display remote heads grouped by remote, unless there are too many
+# remotes ($have_all is false), in which case we only display the remote
+# names
+sub git_remotes_body {
+	my ($remotedata, $limit, $head) = @_;
+	if (not defined $limit or scalar keys %$remotedata <= $limit) {
+		git_get_remote_heads($remotedata, $limit);
+		while (my ($remote, $rdata) = each %$remotedata) {
+			git_print_section({-class=>"remote", -id=>$remote},
+				["remotes", $remote, $remote], sub {
+					git_remote_body($remote, $rdata, $limit, $head);
+				});
+		}
+	} else {
+		git_remotes_list($remotedata, $limit);
+	}
+}
+
 sub git_search_grep_body {
 	my ($commitlist, $from, $to, $extra) = @_;
 	$from = 0 unless defined $from;
@@ -5200,7 +5354,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 $remotedata = $remote_heads ? git_get_remotes_list(-limit => 16) : undef;
 	my @forklist;
 	my $check_forks = gitweb_check_feature('forks');
 
@@ -5278,11 +5432,9 @@ sub git_summary {
 		               $cgi->a({-href => href(action=>"heads")}, "..."));
 	}
 
-	if (@remotelist) {
+	if ($remotedata) {
 		git_print_header_div('remotes');
-		git_heads_body(\@remotelist, $head, 0, 15,
-		               $#remotelist <= 15 ? undef :
-		               $cgi->a({-href => href(action=>"remotes")}, "..."));
+		git_remotes_body($remotedata, 16, $head);
 	}
 
 	if (@forklist) {
@@ -5606,31 +5758,26 @@ sub git_remotes {
 	my $head = git_get_head_hash($project);
 	my $remote = $input_params{'hash'};
 
-	my @remotelist;
+	my $remotedata = git_get_remotes_list(-wanted => $remote);
+	die_error(500, "Unable to get remote information") unless defined $remotedata;
 
-	if (defined $remote) {
-		# only display the heads in a given remote
-		@remotelist = map {
-			my $ref = $_ ;
-			$ref->{'name'} =~ s!^$remote/!!;
-			$ref
-		} git_get_heads_list(undef, "remotes/$remote");
-	} else {
-		@remotelist = git_get_heads_list(undef, 'remotes');
+	if (keys(%$remotedata) == 0) {
+		die_error(404, defined $remote ?
+			"Remote $remote not found" :
+			"No remotes found");
 	}
 
 	git_header_html(undef, undef, -action_extra => $remote);
 	git_print_page_nav('', '',  $head, undef, $head,
 		format_ref_views($remote ? '' : 'remotes'));
 
+	git_get_remote_heads($remotedata, undef);
 	if (defined $remote) {
 		git_print_header_div('remotes', "$remote remote for $project");
+		git_remote_body($remote, $remotedata->{$remote}, undef, $head);
 	} else {
 		git_print_header_div('summary', "$project remotes");
-	}
-
-	if (@remotelist) {
-		git_heads_body(\@remotelist, $head);
+		git_remotes_body($remotedata, undef $head);
 	}
 
 	git_footer_html();
diff --git a/gitweb/static/gitweb.css b/gitweb/static/gitweb.css
index 4132aab..79d7eeb 100644
--- a/gitweb/static/gitweb.css
+++ b/gitweb/static/gitweb.css
@@ -573,6 +573,12 @@ div.binary {
 	font-style: italic;
 }
 
+div.remote {
+	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.68.g6ec8

Re: [PATCHv6 05/10] gitweb: use fullname as hash_base in heads link

[Jakub Narebski]

On Sun, 24 Oct 2010, Giuseppe Bilotta wrote:

> Otherwise, if names are manipulated for display, the link will point to
> the wrong head.
> 
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>

Shouldn't this patch be at beginning, as first patch of the series?

> ---
>  gitweb/gitweb.perl |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index f4592ad..a381892 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -4991,7 +4991,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>";
>  	}
> -- 
> 1.7.3.68.g6ec8
> 
> 

-- 
Jakub Narebski
Poland

Re: [PATCHv6 03/10] gitweb: separate heads and remotes lists

[Jakub Narebski]

On Sun, 24 Oct 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>

> -	my @headlist = git_get_heads_list(16);
> +	my @headlist = git_get_heads_list(16, 'heads');

> -	my @headslist = git_get_heads_list();
> +	my @headslist = git_get_heads_list(undef, 'heads');

Hmmm... I wonder if it wouldn't be better to simply change defaults
in git_get_heads_list() so that second argument defaults always to
'heads', even if 'remote_heads' feature is enabled...

-- 
Jakub Narebski
Poland

Re: [PATCHv6 05/10] gitweb: use fullname as hash_base in heads link

[Giuseppe Bilotta]

2010/10/25 Jakub Narebski <jnareb@gmail.com>:
> On Sun, 24 Oct 2010, Giuseppe Bilotta wrote:
>
>> Otherwise, if names are manipulated for display, the link will point to
>> the wrong head.
>>
>> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
>
> Shouldn't this patch be at beginning, as first patch of the series?

Given how independent it is from the rest of the series, that's
probably a good idea.

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCHv6 07/10] gitweb: remotes view for a single remote

[Jakub Narebski]

On Sun, 24 Oct 2010, Giuseppe Bilotta wrote:

> When 'remotes' view is passed the 'hash' parameter, interpret it as the
> name of a remote and limit the view the the heads of that remote.

I don't quite like (ab)using 'hash' parameter like that, but because
it allows us to use e.g. 'project.git/remotes/origin' path_info
without writing special code for that, I am all right with this hack.

> 
> In single-remote view we let the user switch easily to the default
> remotes view by specifying an -action_extra for the page header and by
> enabling the 'remotes' link in the reference navigation submenu.

Nice!

> +	if (defined $remote) {
> +		# only display the heads in a given remote

It also strips uninteresting '<remote>/' prefix, isn't it?
It would be nice, though not necessary, to have this in comment.

> +		@remotelist = map {
> +			my $ref = $_ ;
> +			$ref->{'name'} =~ s!^$remote/!!;
> +			$ref
> +		} git_get_heads_list(undef, "remotes/$remote");
> +	} else {

-- 
Jakub Narebski
Poland

Re: [PATCHv6 09/10] gitweb: provide a routine to display (sub)sections

[Jakub Narebski]

On Sun, 24 Oct 2010, Giuseppe Bilotta wrote:

[...]
> +# Group output by placing it in a DIV element and adding a header.
> +# Options for start_div() can be provided by passing a hash reference as the
> +# first parameter to the function.
> +# Options to git_print_header_div() can be provided by passing an array
> +# reference. This must follow the options to start_div if they are present.
> +# The content can be a scalar, which is output as-is, a scalar reference, which
> +# is output after html escaping, an IO handle passed either as *handle or
> +# *handle{IO}, or a function reference. In the latter case all following
> +# parameters will be taken as argument to the content function call.
> +sub git_print_section {

Very nice API.  Good work!

-- 
Jakub Narebski
Poland

Re: [PATCHv6 03/10] gitweb: separate heads and remotes lists

[Giuseppe Bilotta]

2010/10/25 Jakub Narebski <jnareb@gmail.com>:
> On Sun, 24 Oct 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>
>
>> -     my @headlist = git_get_heads_list(16);
>> +     my @headlist = git_get_heads_list(16, 'heads');
>
>> -     my @headslist = git_get_heads_list();
>> +     my @headslist = git_get_heads_list(undef, 'heads');
>
> Hmmm... I wonder if it wouldn't be better to simply change defaults
> in git_get_heads_list() so that second argument defaults always to
> 'heads', even if 'remote_heads' feature is enabled...

That's a good point. I'll look into that.


-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCHv6 07/10] gitweb: remotes view for a single remote

[Giuseppe Bilotta]

2010/10/25 Jakub Narebski <jnareb@gmail.com>:
> On Sun, 24 Oct 2010, Giuseppe Bilotta wrote:
>
>> When 'remotes' view is passed the 'hash' parameter, interpret it as the
>> name of a remote and limit the view the the heads of that remote.
>
> I don't quite like (ab)using 'hash' parameter like that, but because
> it allows us to use e.g. 'project.git/remotes/origin' path_info
> without writing special code for that, I am all right with this hack.

We could rename the 'hash' parameter to be more descriptive of the
fact that it includes this nature, but I really think it's not worth
it.

>> In single-remote view we let the user switch easily to the default
>> remotes view by specifying an -action_extra for the page header and by
>> enabling the 'remotes' link in the reference navigation submenu.
>
> Nice!
>
>> +     if (defined $remote) {
>> +             # only display the heads in a given remote
>
> It also strips uninteresting '<remote>/' prefix, isn't it?
> It would be nice, though not necessary, to have this in comment.

Good idea.
-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCHv6 09/10] gitweb: provide a routine to display (sub)sections

[Giuseppe Bilotta]

2010/10/25 Jakub Narebski <jnareb@gmail.com>:
> On Sun, 24 Oct 2010, Giuseppe Bilotta wrote:
>
> [...]
>> +# Group output by placing it in a DIV element and adding a header.
>> +# Options for start_div() can be provided by passing a hash reference as the
>> +# first parameter to the function.
>> +# Options to git_print_header_div() can be provided by passing an array
>> +# reference. This must follow the options to start_div if they are present.
>> +# The content can be a scalar, which is output as-is, a scalar reference, which
>> +# is output after html escaping, an IO handle passed either as *handle or
>> +# *handle{IO}, or a function reference. In the latter case all following
>> +# parameters will be taken as argument to the content function call.
>> +sub git_print_section {
>
> Very nice API.  Good work!

I'm glad you like it. And thank you very much for the reviews that
took me to design it properly. Shall I take this as an ack for the
series, modulo the minor suggestions for the other two patches? (Just
to know if I can include Junio in the next posting)

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCHv6 00/10] gitweb: remote_heads feature

[Jakub Narebski]

On Sun, 24 Oct 2010, Giuseppe Bilotta wrote:

> In this 6th version of the remote heads feature for gitweb, much of the
> internal API has been cleaned up thanks to the suggestion from Jakub and
> others. There were also some other stylistic changes, and a few patches were
> reordered/merged.
> 
> The first 4 patches are rather straightforward and can probably go
> straight in. The 5th patch is a bugfix for something that is only
> triggered by the name manipulation done with the remote heads, but can
> probably be useful even without the rest of the series.
> 
> Patch 7 provides 'single remote view', depending on patch 6 for
> improved visuals of the page header.
> 
> Patches 8 and 9 provide some infrastructure for the grouped remote heads
> display introduced in the last patch.

You can consider all patches, except the very last one (which is
undergoing review - I will send a comment shortly), can be considered
Acked-by.  Any comments are minor, and non-blocking.

-- 
Jakub Narebski
Poland

Re: [PATCHv6 02/10] gitweb: git_get_heads_list accepts an optional list of refs.

[Jakub Narebski]

On Sun, 24 Sep 2010, Giuseppe Bilotta wrote:

>  sub git_get_heads_list {
> -       my $limit = shift;
> +       my ($limit, @classes) = @_;
> +       unless (defined @classes) {
> +               my $remote_heads = gitweb_check_feature('remote_heads');
> +               @classes = ('heads', $remote_heads ? 'remotes' : ());
> +       }

defined(@array) is deprecated at t/../gitweb/gitweb.perl line 3221.

Should be simply 'unless (@classes)', or 'unless (scalar @classes)' but
conditionals provide boolean context, which is scalar context.

I'm sorry about missing it earlier.
-- 
Jakub Narebski
Poland

Re: [PATCHv6 02/10] gitweb: git_get_heads_list accepts an optional list of refs.

[Giuseppe Bilotta]

2010/10/25 Jakub Narebski <jnareb@gmail.com>:
> On Sun, 24 Sep 2010, Giuseppe Bilotta wrote:
>
>>  sub git_get_heads_list {
>> -       my $limit = shift;
>> +       my ($limit, @classes) = @_;
>> +       unless (defined @classes) {
>> +               my $remote_heads = gitweb_check_feature('remote_heads');
>> +               @classes = ('heads', $remote_heads ? 'remotes' : ());
>> +       }
>
> defined(@array) is deprecated at t/../gitweb/gitweb.perl line 3221.
>
> Should be simply 'unless (@classes)', or 'unless (scalar @classes)' but
> conditionals provide boolean context, which is scalar context.
>
> I'm sorry about missing it earlier.

No problem. Fixed for the next batch.

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCHv6 10/10] gitweb: group remote heads by remote

[Jakub Narebski]

On Sun, 24 Oct 2010, Giuseppe Bilotta wrote:

> In remote and summary view, display a block for each remote, with the
> fetch and push URL(s) as well as the list of the remote heads.
> 
> In summary view, if the number of remotes is higher than a prescribed
> limit, only display the first <limit> remotes and their fetch and push
> urls, without any heads information and without grouping.

I like this idea.

I guess that we can always implement more fancy limiting in the future
(e.g. taking into account total number of displayed remote heads).

> 
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
> ---
>  gitweb/gitweb.perl       |  185 +++++++++++++++++++++++++++++++++++++++++-----
>  gitweb/static/gitweb.css |    6 ++
>  2 files changed, 172 insertions(+), 19 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index feca8bc..5f08dcc 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -2772,6 +2772,66 @@ sub git_get_last_activity {
>  	return (undef, undef);
>  }
>  
> +# Returns a hash ref mapping remote names to their fetch and push URLs.
> +# We return a hash ref rather than a hash so that a simple check with defined
> +# can be used to tell apart the "no remotes" case from other kinds of
> +# failures.

Just an idea (it is not necessary to implement it; it has its own
drawbacks): we could implement here

  return wantsarray ? %remotes : \%remotes;

so the caller that wants hash, would get hash; and the one wanting
reference (to distinguish error from no remotes), would get hash
reference.

> +#
> +# It is possible to limit the retrieved remotes either by number
> +# (specifying a -limit parameter) or by name (-wanted parameter).

I don't quite like limiting when generating, and would prefer do limiting
on display, especially if not doing limiting would not affect performance
much (git command invoked doesn't do limiting, like in case of 
git_get_heads_list / git_get_tags_list or *most important* parse_commits).

Especially if it complicates code that much (see below).

Not doing limiting here, in git_get_remotes_list (or just git_get_remotes)
would also make API simpler; the single optional argument would be name of
remote we want to retrieve.

+# It is possible to limit the retrieved remotes to only single remote
+# by passing its name as a parameter to git_get_remotes_list.


> +#
> +# When a single remote is wanted, we cannot use 'git remote show -n' because
> +# that command always work (assuming it's a remote URL if it's not defined),
> +# and we cannot use 'git remote show' because that would try to make a network
> +# roundtrip. So the only way to find if that particular remote is defined is to
> +# walk the list provided by 'git remote -v' and stop if and when we find what
> +# we want.

I would add 'Implemetation note: ' here, which means start with
'Implementation note: When ..." -- but it is not necessary.

> +sub git_get_remotes_list {
> +	my %params = @_;
> +	my $limit = $params{-limit};
> +	my $wanted = $params{-wanted};
> +	my %remotes = ();
> +
> +	open my $fd, '-|' , git_cmd(), 'remote', '-v';
> +	return unless $fd;
> +	while (my $remote = <$fd>) {
> +		chomp $remote;
> +		$remote =~ s!\t(.*?)\s+\((\w+)\)$!!;
> +		next if $wanted and not $remote eq $wanted;
> +		my ($url, $key) = ($1, $2);
> +
> +		# a remote may appear more than once because of multiple URLs,
> +		# so if this is a remote we know already, be sure to continue,
> +		# lest we end up with a remote for which we get the fetch URL
> +		# bot not the push URL, for example
> +		my $more = exists $remotes{$remote};
> +		$more ||= defined $limit ? (keys(%remotes) <= $limit) : 1;
> +		if ($more) {
> +			$remotes{$remote} ||= { 'heads' => () };
> +			$remotes{$remote}{$key} = $url;
> +		} else {
> +			last;
> +		}
> +	}
> +	close $fd or return;
> +	return \%remotes;
> +}

Without implementing limiting in git_get_remotes_list(), but leaving it
for the display subroutine, it would reduce to:

+sub git_get_remotes_list {
+	my $wanted = shift;
+	my %remotes = ();
+
+	open my $fd, '-|' , git_cmd(), 'remote', '-v';
+	return unless $fd;
+	while (my $remote = <$fd>) {
+		chomp $remote;
+		next unless ($remote =~ s!\t(.*?)\s+\((\w+)\)$!!);
+		# $wanted remote might be present multiple times in output
+		next if ($wanted && $remote ne $wanted);
+		my ($url, $key) = ($1, $2);
+
+		$remotes{$remote}{$key} = $url;
+	}
+	close $fd or return;
+	return \%remotes;
+}

> +
> +# Takes a hash of remotes as first parameter and fills it by adding the
> +# available remote heads for each of the indicated remotes.
> +# A maximum number of heads can also be specified.

All git_get_* subroutines _return_ something; this looks more like fill_*
function for me.

> +sub git_get_remote_heads {
> +	my ($remotes, $limit) = @_;
> +	my @heads = map { "remotes/$_" } keys %$remotes;
> +	my @remoteheads = git_get_heads_list($limit, @heads);
> +	foreach (keys %$remotes) {
> +		my $remote = $_;
> +		$remotes->{$remote}{'heads'} = [ grep {
> +			$_->{'name'} =~ s!^$remote/!!
> +			} @remoteheads ];
> +	}
> +}

We could remove limiting number of heads shown per remote also from here,
but this time 1.) the $limit parameter is passed down to git_get_heads_list
which in turn uses $limit as parameter to git command  2.) it doesn't
simplify code almost at all:

+sub fill_remote_heads {
+	my $remotes = shift;
+
+	my @heads = map { "remotes/$_" } keys %$remotes;
+	## A QUESTION: perhaps it would be better to pass @remoteheads as parameter?
+	my @remoteheads = git_get_heads_list(undef, @heads);
+	foreach my $remote (keys %$remotes) {
+		$remotes->{$remote}{'heads'} =
+			[ grep { $_->{'name'} =~ s!^$remote/!! } @remoteheads ];
+	}
+}


A QUESTION: perhaps it would be better to pass @remoteheads as parameter?
I wonder if won't end up with calling git_get_heads_list multiple times...
well, the improvement can be left for later.  Answering this question
should not affect accepting this patch series.

> +
>  sub git_get_references {
>  	my $type = shift || "";
>  	my %refs;
> @@ -5054,6 +5114,100 @@ sub git_heads_body {
>  	print "</table>\n";
>  }
>  
> +# Display a single remote block

Note that you would end up with two very similarly named subrotines:
git_remote_body and git_remotes_body.  Perhaps it would be better to
call this one git_remote_block, or git_describe_remote?

> +sub git_remote_body {
> +	my ($remote, $rdata, $limit, $head) = @_;
> +
> +	my $heads = $rdata->{'heads'};
> +	my $fetch = $rdata->{'fetch'};
> +	my $push = $rdata->{'push'};

We could have used the following Perl shortcut

+	my ($heads, $fetch, $push) = @{$rdata}{qw(heads fetch push)};

but it isn't needed, and I guess it even could hurt readibility...

> +
> +	my $urls = "<table class=\"projects_list\">\n" ;
> +
> +	if (defined $fetch) {
> +		if ($fetch eq $push) {
> +			$urls .= format_repo_url("URL", $fetch);
> +		} else {
> +			$urls .= format_repo_url("Fetch URL", $fetch);
> +			$urls .= format_repo_url("Push URL", $push) if defined $push;
> +		}
> +	} elsif (defined $push) {
> +		$urls .= format_repo_url("Push URL", $push);
> +	} else {
> +		$urls .= format_repo_url("", "No remote URL");
> +	}
> +
> +	$urls .= "</table>\n";

I'm not sure about naming this variable $urls...

> +
> +	my ($maxheads, $dots);
> +	if (defined $limit) {
> +		$maxheads = $limit - 1;
> +		if ($#{$heads} > $maxheads) {
> +			$dots = $cgi->a({-href => href(action=>"remotes", hash=>$remote)}, "...");
> +		}
> +	}
> +
> +	print $urls;
> +	git_heads_body($heads, $head, 0, $maxheads, $dots);

Wouldn't this be simpler:

+	my $dots;
+	if (defined $limit && $limit < @$heads) {
+		$dots = $cgi->a({-href => href(action=>"remotes", hash=>$remote)}, "...");
+	}
+
+	print $urls;
+	git_heads_body($heads, $head, 0, $limit, $dots);

We would do similar trick as in other parts: request one item more than we
display to check if there is more data.

> +}
> +
> +# Display a list of remote names with the respective fetch and push URLs
> +# This routine only gets called when there are more remotes than the given
> +# limit, so we know that we have to append an ellipsis to the table and
> +# that we have to pop one of the keys.

I think it would be a better idea to make this subroutine generic: display
only list of remotes, limited to $limit remotes maximum.

+# Display a list of remote names with the respective fetch and push URLs,
+# without displaying remote heads (used when there are too many remotes).

> +sub git_remotes_list {
> +	my ($remotedata) = @_;
> +	print "<table class=\"heads\">\n";
> +	my $alternate = 1;
> +	my @keys = sort keys %$remotedata;
> +	pop @keys;
> +
> +	while (my $remote = shift @keys) {
> +		my $rdata = $remotedata->{$remote};
> +		my $fetch = $rdata->{'fetch'};
> +		my $push = $rdata->{'push'};

If we do explicit limiting (not assuming that $remotedata is already
limited on generation), it could look like the following:

+sub git_remotes_list {
+	my ($remotedata, $limit) = @_;
+
+	print "<table class=\"heads\">\n";
+	my $alternate = 1;
+	my @remotes = sort keys %$remotedata;
+	$limit = scalar @remotes
+		unless defined $limit;
+
+	for (my $i = 0; $i < $limit; $i++) {
+		my $remote = $remotes[$i];
+		my $rdata = $remotedata->{$remote};
+		my $fetch = $rdata->{'fetch'};
+		my $push  = $rdata->{'push'};

> +		if ($alternate) {
> +			print "<tr class=\"dark\">\n";
> +		} else {
> +			print "<tr class=\"light\">\n";
> +		}
> +		$alternate ^= 1;
> +		print "<td>" .
> +		      $cgi->a({-href=> href(action=>'remotes', hash=>$remote),
> +			       -class=> "list name"},esc_html($remote)) . "</td>";

Very, very minor nitpick: if "<td>" is on separate line, why "</td>"
isn't?

> +		print "<td class=\"link\">" .
> +		      (defined $fetch ? $cgi->a({-href=> $fetch}, "fetch") : "fetch") .
> +		      " | " .
> +		      (defined $push ? $cgi->a({-href=> $push}, "push") : "push") .
> +		      "</td>";
> +
> +		print "</tr>\n";
> +	}
> +	print "<tr>\n" .
> +	      "<td colspan=\"3\">" .
> +	      $cgi->a({-href => href(action=>"remotes")}, "...") .
> +	      "</td>\n" . "</tr>\n";

With explicit limiting this would be:

+	if ($limit < @remotes) {
+		print "<tr>\n" .
+		      "<td colspan=\"3\">" .
+		      $cgi->a({-href => href(action=>"remotes")}, "...") .
+		      "</td>\n" .
+		      "</tr>\n";
+	}

> +	print "</table>";
> +}
> +
> +# Display remote heads grouped by remote, unless there are too many
> +# remotes ($have_all is false), in which case we only display the remote
> +# names

This of course also would have to be changed if limiting is done at
display time, not at generation time (at least for limiting list of
remotes, if not for generating list of refs).

> +sub git_remotes_body {
> +	my ($remotedata, $limit, $head) = @_;
> +	if (not defined $limit or scalar keys %$remotedata <= $limit) {
> +		git_get_remote_heads($remotedata, $limit);
> +		while (my ($remote, $rdata) = each %$remotedata) {
> +			git_print_section({-class=>"remote", -id=>$remote},
> +				["remotes", $remote, $remote], sub {
> +					git_remote_body($remote, $rdata, $limit, $head);
> +				});
> +		}
> +	} else {
> +		git_remotes_list($remotedata, $limit);
> +	}

Minor issue: wouldn't it make for easier reading to have less code in
the 'if' part of if { ... } else { ... } clause, and more code in 'else'
part.

> +}

The rewritten function, which makes invoked display subroutines to do
the limiting by themselves, could look like this:

+# Display remote heads grouped by remote, unless there are too many
+# remotes, in which case we only display the remote names
+sub git_remotes_body {
+	my ($remotedata, $limit, $head) = @_;
+
+	if (defined $limit && $limit < scalar keys %$remotedata) {
+		# $limit limits number of displayed remotes
+		git_remotes_list($remotedata, $limit);
+
+	} else {
+		# $limit limits number of displayed heads in each remote
+		fill_remote_heads($remotedata); ## or fill_remote_heads($remotedata, $limit+1);
+		while (my ($remote, $rdata) = each %$remotedata) {
+			git_print_section(
+				{-class=>"remote", -id=>$remote},
+				["remotes", $remote, $remote],
+				sub { git_remote_block($remote, $rdata, $limit, $head) }
+			);
+		}
+	}
+}


> +
>  sub git_search_grep_body {
>  	my ($commitlist, $from, $to, $extra) = @_;
>  	$from = 0 unless defined $from;
> @@ -5200,7 +5354,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 $remotedata = $remote_heads ? git_get_remotes_list(-limit => 16) : undef;

If git_get_remotes_list wouldn't do limiting, it would simply be

-	my @remotelist = $remote_heads ? git_get_heads_list(16, 'remotes') : ();
+	my $remotedata = $remote_heads ? git_get_remotes_list() : undef;

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

This wouldn't change, and would be the same even if git_get_remotes_list
wouldn't do limiting.


Perhaps it would be worth adding comment about git_remotes:

@@ -5599,6 +5740,7 @@ sub git_heads {
 	git_footer_html();
 }
 
+# dual lived: used both for single remote view, and for list of all remotes
 sub git_remotes {
 	gitweb_check_feature('remote_heads')
 		or die_error(403, "Remote heads view is disabled");


Though I am not sure if it is good public API.  Perhaps it is...

> @@ -5606,31 +5758,26 @@ sub git_remotes {
>  	my $head = git_get_head_hash($project);
>  	my $remote = $input_params{'hash'};
>  
> -	my @remotelist;
> +	my $remotedata = git_get_remotes_list(-wanted => $remote);
> +	die_error(500, "Unable to get remote information") unless defined $remotedata;

It would be little simpler with git_get_remotes_list not doing the
limiting, because of which we can use slightly simpler API.

+	my $remotedata = git_get_remotes_list($remote);
+	die_error(500, "Unable to get remote information")
+		unless defined $remotedata;

>  
> -	if (defined $remote) {
> -		# only display the heads in a given remote
> -		@remotelist = map {
> -			my $ref = $_ ;
> -			$ref->{'name'} =~ s!^$remote/!!;
> -			$ref
> -		} git_get_heads_list(undef, "remotes/$remote");
> -	} else {
> -		@remotelist = git_get_heads_list(undef, 'remotes');
> +	if (keys(%$remotedata) == 0) {

You can write simply

+	unless (%$remotedata) {

>From perldata(1):  "If you evaluate a hash in scalar context, it returns
false if the hash is empty."

> +		die_error(404, defined $remote ?
> +			"Remote $remote not found" :
> +			"No remotes found");
>  	}
>  
>  	git_header_html(undef, undef, -action_extra => $remote);
>  	git_print_page_nav('', '',  $head, undef, $head,
>  		format_ref_views($remote ? '' : 'remotes'));
>  
> +	git_get_remote_heads($remotedata, undef);

+	fill_remote_heads($remotedata, undef);

>  	if (defined $remote) {
>  		git_print_header_div('remotes', "$remote remote for $project");
> +		git_remote_body($remote, $remotedata->{$remote}, undef, $head);
>  	} else {
>  		git_print_header_div('summary', "$project remotes");
> -	}
> -
> -	if (@remotelist) {
> -		git_heads_body(\@remotelist, $head);
> +		git_remotes_body($remotedata, undef $head);
>  	}

You are missing comma after 'undef' here

-	if (@remotelist) {
-		git_heads_body(\@remotelist, $head);
+		git_remotes_body($remotedata, undef, $head);
 	}

-- 
Jakub Narebski
ShadeHawk on #git

Re: [PATCHv6 10/10] gitweb: group remote heads by remote

[Jakub Narebski]

On Wed, 27 Oct 2010, Jakub Narebski wrote:
> On Sun, 24 Oct 2010, Giuseppe Bilotta wrote:

> > +# It is possible to limit the retrieved remotes either by number
> > +# (specifying a -limit parameter) or by name (-wanted parameter).
> 
> I don't quite like limiting when generating, and would prefer do limiting
> on display, especially if not doing limiting would not affect performance
> much (git command invoked doesn't do limiting, like in case of 
> git_get_heads_list / git_get_tags_list or *most important* parse_commits).
> 
> Especially if it complicates code that much (see below).
> 
> Not doing limiting here, in git_get_remotes_list (or just git_get_remotes)
> would also make API simpler; the single optional argument would be name of
> remote we want to retrieve.

Note that you can see the changes I have mentioned here:

  git://repo.or.cz/git/jnareb-git.git  gitweb/allheads-jn

  http://repo.or.cz/w/git/jnareb-git.git/shortlog/refs/heads/gitweb/allheads-jn
  http://github.com/jnareb/git/commits/gitweb%2Fallheads-jn
  

-- 
Jakub Narebski
Poland

Re: [PATCHv6 10/10] gitweb: group remote heads by remote

[Jakub Narebski]

On Sun, 24 Oct 2010, Giuseppe Bilotta wrote:

I forgot to mention this:

> +       foreach (keys %$remotes) {
> +               my $remote = $_;

Why not simply

  +       foreach my $remote (keys %$remotes) {

-- 
Jakub Narebski
Poland

Re: [PATCHv6 10/10] gitweb: group remote heads by remote

[Giuseppe Bilotta]

2010/10/27 Jakub Narebski <jnareb@gmail.com>:
> On Sun, 24 Oct 2010, Giuseppe Bilotta wrote:
>
>> In remote and summary view, display a block for each remote, with the
>> fetch and push URL(s) as well as the list of the remote heads.
>>
>> In summary view, if the number of remotes is higher than a prescribed
>> limit, only display the first <limit> remotes and their fetch and push
>> urls, without any heads information and without grouping.
>
> I like this idea.

Thanks.

> I guess that we can always implement more fancy limiting in the future
> (e.g. taking into account total number of displayed remote heads).

The limiting seems to be the most debatable part of this patch 8-)

>> +# Returns a hash ref mapping remote names to their fetch and push URLs.
>> +# We return a hash ref rather than a hash so that a simple check with defined
>> +# can be used to tell apart the "no remotes" case from other kinds of
>> +# failures.
>
> Just an idea (it is not necessary to implement it; it has its own
> drawbacks): we could implement here
>
>  return wantsarray ? %remotes : \%remotes;
>
> so the caller that wants hash, would get hash; and the one wanting
> reference (to distinguish error from no remotes), would get hash
> reference.

It does simplify the summary view case a little, good point.

>> +#
>> +# It is possible to limit the retrieved remotes either by number
>> +# (specifying a -limit parameter) or by name (-wanted parameter).
>
> I don't quite like limiting when generating, and would prefer do limiting
> on display, especially if not doing limiting would not affect performance
> much (git command invoked doesn't do limiting, like in case of
> git_get_heads_list / git_get_tags_list or *most important* parse_commits).
>
> Especially if it complicates code that much (see below).
>
> Not doing limiting here, in git_get_remotes_list (or just git_get_remotes)
> would also make API simpler; the single optional argument would be name of
> remote we want to retrieve.

Hm. By the same token, there would be no need to do the limiting even
when trying to get information about a single remote, meaning we could
make the sub totally parameter-less. OTOH, this would make the calling
routine quite more complex (since it would have to check if the key is
there, and then select that single key etc), much more so than
limiting the number of displayed heads. I'll take the numerical
limiting off the routine.

>> +#
>> +# When a single remote is wanted, we cannot use 'git remote show -n' because
>> +# that command always work (assuming it's a remote URL if it's not defined),
>> +# and we cannot use 'git remote show' because that would try to make a network
>> +# roundtrip. So the only way to find if that particular remote is defined is to
>> +# walk the list provided by 'git remote -v' and stop if and when we find what
>> +# we want.
>
> I would add 'Implemetation note: ' here, which means start with
> 'Implementation note: When ..." -- but it is not necessary.

Can do that.

>> +
>> +# Takes a hash of remotes as first parameter and fills it by adding the
>> +# available remote heads for each of the indicated remotes.
>> +# A maximum number of heads can also be specified.
>
> All git_get_* subroutines _return_ something; this looks more like fill_*
> function for me.

I'm not particularly enamored with _get_, fill will do

>
>> +sub git_get_remote_heads {
>> +     my ($remotes, $limit) = @_;
>> +     my @heads = map { "remotes/$_" } keys %$remotes;
>> +     my @remoteheads = git_get_heads_list($limit, @heads);
>> +     foreach (keys %$remotes) {
>> +             my $remote = $_;
>> +             $remotes->{$remote}{'heads'} = [ grep {
>> +                     $_->{'name'} =~ s!^$remote/!!
>> +                     } @remoteheads ];
>> +     }
>> +}
>
> We could remove limiting number of heads shown per remote also from here,
> but this time 1.) the $limit parameter is passed down to git_get_heads_list
> which in turn uses $limit as parameter to git command  2.) it doesn't
> simplify code almost at all:
>
> +sub fill_remote_heads {
> +       my $remotes = shift;
> +
> +       my @heads = map { "remotes/$_" } keys %$remotes;
> +       ## A QUESTION: perhaps it would be better to pass @remoteheads as parameter?
> +       my @remoteheads = git_get_heads_list(undef, @heads);
> +       foreach my $remote (keys %$remotes) {
> +               $remotes->{$remote}{'heads'} =
> +                       [ grep { $_->{'name'} =~ s!^$remote/!! } @remoteheads ];
> +       }
> +}
>
>
> A QUESTION: perhaps it would be better to pass @remoteheads as parameter?
> I wonder if won't end up with calling git_get_heads_list multiple times...
> well, the improvement can be left for later.  Answering this question
> should not affect accepting this patch series.

I'm not sure I actually understand the question. Who should we pass
@remoteheads to?

>> +
>>  sub git_get_references {
>>       my $type = shift || "";
>>       my %refs;
>> @@ -5054,6 +5114,100 @@ sub git_heads_body {
>>       print "</table>\n";
>>  }
>>
>> +# Display a single remote block
>
> Note that you would end up with two very similarly named subrotines:
> git_remote_body and git_remotes_body.  Perhaps it would be better to
> call this one git_remote_block, or git_describe_remote?

Makes sense. I'll go with git_remote_block for similarity with git_remotes_list

>> +sub git_remote_body {
>> +     my ($remote, $rdata, $limit, $head) = @_;
>> +
>> +     my $heads = $rdata->{'heads'};
>> +     my $fetch = $rdata->{'fetch'};
>> +     my $push = $rdata->{'push'};
>
> We could have used the following Perl shortcut
>
> +       my ($heads, $fetch, $push) = @{$rdata}{qw(heads fetch push)};
>
> but it isn't needed, and I guess it even could hurt readibility...

OTOH, if readibility was a priority, we wouldn't use Perl would we ;-)
Joking aside, I think I'll leave it as is, unless there's some
significant performance improvement in using the latter syntax.

>> +
>> +     my $urls = "<table class=\"projects_list\">\n" ;
>> +
>> +     if (defined $fetch) {
>> +             if ($fetch eq $push) {
>> +                     $urls .= format_repo_url("URL", $fetch);
>> +             } else {
>> +                     $urls .= format_repo_url("Fetch URL", $fetch);
>> +                     $urls .= format_repo_url("Push URL", $push) if defined $push;
>> +             }
>> +     } elsif (defined $push) {
>> +             $urls .= format_repo_url("Push URL", $push);
>> +     } else {
>> +             $urls .= format_repo_url("", "No remote URL");
>> +     }
>> +
>> +     $urls .= "</table>\n";
>
> I'm not sure about naming this variable $urls...

I'm open to suggestions. $urls_table ?

>> +
>> +     my ($maxheads, $dots);
>> +     if (defined $limit) {
>> +             $maxheads = $limit - 1;
>> +             if ($#{$heads} > $maxheads) {
>> +                     $dots = $cgi->a({-href => href(action=>"remotes", hash=>$remote)}, "...");
>> +             }
>> +     }
>> +
>> +     print $urls;
>> +     git_heads_body($heads, $head, 0, $maxheads, $dots);
>
> Wouldn't this be simpler:
>
> +       my $dots;
> +       if (defined $limit && $limit < @$heads) {
> +               $dots = $cgi->a({-href => href(action=>"remotes", hash=>$remote)}, "...");
> +       }
> +
> +       print $urls;
> +       git_heads_body($heads, $head, 0, $limit, $dots);
>
> We would do similar trick as in other parts: request one item more than we
> display to check if there is more data.

Right.

>> +}
>> +
>> +# Display a list of remote names with the respective fetch and push URLs
>> +# This routine only gets called when there are more remotes than the given
>> +# limit, so we know that we have to append an ellipsis to the table and
>> +# that we have to pop one of the keys.
>
> I think it would be a better idea to make this subroutine generic: display
> only list of remotes, limited to $limit remotes maximum.

That's probably a good idea. I've taken some suggestions from your
proposed code, but kept the while loop.

>> +             print "<td>" .
>> +                   $cgi->a({-href=> href(action=>'remotes', hash=>$remote),
>> +                            -class=> "list name"},esc_html($remote)) . "</td>";
>
> Very, very minor nitpick: if "<td>" is on separate line, why "</td>"
> isn't?

Good question.

>> +     print "</table>";
>> +}
>> +
>> +# Display remote heads grouped by remote, unless there are too many
>> +# remotes ($have_all is false), in which case we only display the remote
>> +# names
>
> This of course also would have to be changed if limiting is done at
> display time, not at generation time (at least for limiting list of
> remotes, if not for generating list of refs).

Actually, it doesn't need to change much, since $limit was still taken
into consideration.

>> +sub git_remotes_body {
>> +     my ($remotedata, $limit, $head) = @_;
>> +     if (not defined $limit or scalar keys %$remotedata <= $limit) {
>> +             git_get_remote_heads($remotedata, $limit);
>> +             while (my ($remote, $rdata) = each %$remotedata) {
>> +                     git_print_section({-class=>"remote", -id=>$remote},
>> +                             ["remotes", $remote, $remote], sub {
>> +                                     git_remote_body($remote, $rdata, $limit, $head);
>> +                             });
>> +             }
>> +     } else {
>> +             git_remotes_list($remotedata, $limit);
>> +     }
>
> Minor issue: wouldn't it make for easier reading to have less code in
> the 'if' part of if { ... } else { ... } clause, and more code in 'else'
> part.

I was just thinking about that. It also makes the conditional more obvious.

> Perhaps it would be worth adding comment about git_remotes:
>
> @@ -5599,6 +5740,7 @@ sub git_heads {
>        git_footer_html();
>  }
>
> +# dual lived: used both for single remote view, and for list of all remotes

Good idea.

> Though I am not sure if it is good public API.  Perhaps it is...

The alternative would be to have git_remote to handle the single
remote case, and possibly even have the action name be 'remote' rather
than 'remotes' in that case.

>> -     if (defined $remote) {
>> -             # only display the heads in a given remote
>> -             @remotelist = map {
>> -                     my $ref = $_ ;
>> -                     $ref->{'name'} =~ s!^$remote/!!;
>> -                     $ref
>> -             } git_get_heads_list(undef, "remotes/$remote");
>> -     } else {
>> -             @remotelist = git_get_heads_list(undef, 'remotes');
>> +     if (keys(%$remotedata) == 0) {
>
> You can write simply
>
> +       unless (%$remotedata) {
>
> From perldata(1):  "If you evaluate a hash in scalar context, it returns
> false if the hash is empty."

Right.

>> -     if (@remotelist) {
>> -             git_heads_body(\@remotelist, $head);
>> +             git_remotes_body($remotedata, undef $head);
>>       }
>
> You are missing comma after 'undef' here

Subtle bug. Thanks.

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCHv6 10/10] gitweb: group remote heads by remote

[Jakub Narebski]

On Tue, 2 Nov 2010, Giuseppe Bilotta wrote:
> 2010/10/27 Jakub Narebski <jnareb@gmail.com>:
>> On Sun, 24 Oct 2010, Giuseppe Bilotta wrote:
>>
>>> In remote and summary view, display a block for each remote, with the
>>> fetch and push URL(s) as well as the list of the remote heads.
>>>
>>> In summary view, if the number of remotes is higher than a prescribed
>>> limit, only display the first <limit> remotes and their fetch and push
>>> urls, without any heads information and without grouping.

>> I guess that we can always implement more fancy limiting in the future
>> (e.g. taking into account total number of displayed remote heads).
> 
> The limiting seems to be the most debatable part of this patch 8-)

I think that untill this feature gets into gitweb we can only guess
as to what kind of limiting would look and behave best on RL cases.
 
>>> +#
>>> +# It is possible to limit the retrieved remotes either by number
>>> +# (specifying a -limit parameter) or by name (-wanted parameter).
>>
>> I don't quite like limiting when generating, and would prefer do limiting
>> on display, especially if not doing limiting would not affect performance
>> much (git command invoked doesn't do limiting, like in case of
>> git_get_heads_list / git_get_tags_list or *most important* parse_commits).
>>
>> Especially if it complicates code that much (see below).
>>
>> Not doing limiting here, in git_get_remotes_list (or just git_get_remotes)
>> would also make API simpler; the single optional argument would be name of
>> remote we want to retrieve.
> 
> Hm. By the same token, there would be no need to do the limiting even
> when trying to get information about a single remote, meaning we could
> make the sub totally parameter-less. OTOH, this would make the calling
> routine quite more complex (since it would have to check if the key is
> there, and then select that single key etc), much more so than
> limiting the number of displayed heads. 

True, we have to balance complexity in generating function vs. complexity
in display code.

> I'll take the numerical limiting off the routine.

You meant here limiting number of remotes returned (except of case of
limiting to single remote), isn't it?

>>> +
>>> +# Takes a hash of remotes as first parameter and fills it by adding the
>>> +# available remote heads for each of the indicated remotes.
>>> +# A maximum number of heads can also be specified.
>>
>> All git_get_* subroutines _return_ something; this looks more like fill_*
>> function for me.
> 
> I'm not particularly enamored with _get_, fill will do

We have fill_from_file_info, fill_project_list_info; I think 
fill_remote_heads would be a good name.
 
>>
>>> +sub git_get_remote_heads {
>>> +     my ($remotes, $limit) = @_;
>>> +     my @heads = map { "remotes/$_" } keys %$remotes;
>>> +     my @remoteheads = git_get_heads_list($limit, @heads);
>>> +     foreach (keys %$remotes) {
>>> +             my $remote = $_;
>>> +             $remotes->{$remote}{'heads'} = [ grep {
>>> +                     $_->{'name'} =~ s!^$remote/!!
>>> +                     } @remoteheads ];
>>> +     }
>>> +}
>>
>> We could remove limiting number of heads shown per remote also from here,
>> but this time 1.) the $limit parameter is passed down to git_get_heads_list
>> which in turn uses $limit as parameter to git command  2.) it doesn't
>> simplify code almost at all:
>>
>> +sub fill_remote_heads {
>> +       my $remotes = shift;
>> +
>> +       my @heads = map { "remotes/$_" } keys %$remotes;
>> +       ## A QUESTION: perhaps it would be better to pass @remoteheads as parameter?
>> +       my @remoteheads = git_get_heads_list(undef, @heads);
>> +       foreach my $remote (keys %$remotes) {
>> +               $remotes->{$remote}{'heads'} =
>> +                       [ grep { $_->{'name'} =~ s!^$remote/!! } @remoteheads ];
>> +       }
>> +}

I now think that passing limit to fill_remote_heads (like you have) might
be a good solution (contrary to what I wrote earlier).

>> A QUESTION: perhaps it would be better to pass @remoteheads as parameter?
>> I wonder if won't end up with calling git_get_heads_list multiple times...
>> well, the improvement can be left for later.  Answering this question
>> should not affect accepting this patch series.
> 
> I'm not sure I actually understand the question. Who should we pass
> @remoteheads to?

I meant here passing @remoteheads or \@remoteheads, i.e. result of call
to git_get_heads_list, to fill_remote_heads (as one of parameters).  
I was worrying, perhaps unnecessarily, about calling git_get_heads_list
more than once... but I guess that we did it anyway.

>>> +
>>> +     my $urls = "<table class=\"projects_list\">\n" ;
>>> +
>>> +     if (defined $fetch) {
>>> +             if ($fetch eq $push) {
>>> +                     $urls .= format_repo_url("URL", $fetch);
>>> +             } else {
>>> +                     $urls .= format_repo_url("Fetch URL", $fetch);
>>> +                     $urls .= format_repo_url("Push URL", $push) if defined $push;
>>> +             }
>>> +     } elsif (defined $push) {
>>> +             $urls .= format_repo_url("Push URL", $push);
>>> +     } else {
>>> +             $urls .= format_repo_url("", "No remote URL");
>>> +     }
>>> +
>>> +     $urls .= "</table>\n";
>>
>> I'm not sure about naming this variable $urls...
> 
> I'm open to suggestions. $urls_table ?

It is better.
 
>> Though I am not sure if it is good public API.  Perhaps it is...
> 
> The alternative would be to have git_remote to handle the single
> remote case, and possibly even have the action name be 'remote' rather
> than 'remotes' in that case.

We might want to have 'remote' as alias to 'remotes' anyway.

Though what you propose, having separately 'remotes' for list of all
remotes, and 'remote' + name of remote, might be a good idea.
 

Thank you on diligently working on this feature.

-- 
Jakub Narebski
Poland

Re: [PATCHv6 10/10] gitweb: group remote heads by remote

[Giuseppe Bilotta]

On Wed, Nov 3, 2010 at 12:58 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> On Tue, 2 Nov 2010, Giuseppe Bilotta wrote:
>> The limiting seems to be the most debatable part of this patch 8-)
>
> I think that untill this feature gets into gitweb we can only guess
> as to what kind of limiting would look and behave best on RL cases.

I think that in most cases there won't be any need for limiting.
Public cases of lots of remotes with lots of branches are, I suspect,
rare.

>> I'll take the numerical limiting off the routine.
>
> You meant here limiting number of remotes returned (except of case of
> limiting to single remote), isn't it?

Yes.

> We have fill_from_file_info, fill_project_list_info; I think
> fill_remote_heads would be a good name.

That's what will be in the next rehash of the patchset

>>> +sub fill_remote_heads {
>>> +       my $remotes = shift;
>>> +
>>> +       my @heads = map { "remotes/$_" } keys %$remotes;
>>> +       ## A QUESTION: perhaps it would be better to pass @remoteheads as parameter?
>>> +       my @remoteheads = git_get_heads_list(undef, @heads);
>>> +       foreach my $remote (keys %$remotes) {
>>> +               $remotes->{$remote}{'heads'} =
>>> +                       [ grep { $_->{'name'} =~ s!^$remote/!! } @remoteheads ];
>>> +       }
>>> +}
>
> I now think that passing limit to fill_remote_heads (like you have) might
> be a good solution (contrary to what I wrote earlier).

I hadn't taken it out anyway, so that's good ;-)

>>> A QUESTION: perhaps it would be better to pass @remoteheads as parameter?
>>> I wonder if won't end up with calling git_get_heads_list multiple times...
>>> well, the improvement can be left for later.  Answering this question
>>> should not affect accepting this patch series.
>>
>> I'm not sure I actually understand the question. Who should we pass
>> @remoteheads to?
>
> I meant here passing @remoteheads or \@remoteheads, i.e. result of call
> to git_get_heads_list, to fill_remote_heads (as one of parameters).
> I was worrying, perhaps unnecessarily, about calling git_get_heads_list
> more than once... but I guess that we did it anyway.

I'm still not sure I follow. git_get_heads_list only gets called once,
with all the remotes/<remotename> pathspecs as a single array
argument. This _does_ mean that when the total number of remote heads
is greater than the limit some remotes will not display complete
information in summary view. The real issue here is, I think, that
there is no trivial way to tell which remotes have incomplete
information and which don't, meaning that in the subsequent
git_remote_block calls we'll have no way to provide visual feedback
(the ellipsis) when some heads are missing.

>>> I'm not sure about naming this variable $urls...
>>
>> I'm open to suggestions. $urls_table ?
>
> It is better.

Can do.

>>> Though I am not sure if it is good public API.  Perhaps it is...
>>
>> The alternative would be to have git_remote to handle the single
>> remote case, and possibly even have the action name be 'remote' rather
>> than 'remotes' in that case.
>
> We might want to have 'remote' as alias to 'remotes' anyway.
>
> Though what you propose, having separately 'remotes' for list of all
> remotes, and 'remote' + name of remote, might be a good idea.

I hope you don't mind if I opt to leave these refinements for later ;-)

> Thank you on diligently working on this feature.

Thank you for your review.

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCHv6 10/10] gitweb: group remote heads by remote

[Jakub Narebski]

On Wed, 3 Nov 2010, Giuseppe Bilotta wrote:
> On Wed, Nov 3, 2010 at 12:58 AM, Jakub Narebski <jnareb@gmail.com> wrote:
>> On Tue, 2 Nov 2010, Giuseppe Bilotta wrote:
>>> The limiting seems to be the most debatable part of this patch 8-)
>>
>> I think that untill this feature gets into gitweb we can only guess
>> as to what kind of limiting would look and behave best on RL cases.
> 
> I think that in most cases there won't be any need for limiting.
> Public cases of lots of remotes with lots of branches are, I suspect,
> rare.

It's not about _public_ cases; I think that it is in very rare cases
that public repository would want to display remotes and remote-tracking
branches.

I think remote_heads feature is more important for _local_ use, for
example browsing one own repository using git-instaweb.  In such cases
number of remotes and of remote-tracking branches might be large (I have
11 remotes, not all active, and 58 remote-tracking branches).

BTW. would next version of this series include patch to git-instaweb
enabling 'remote_heads' feature for it (gitweb_conf function)?

>>>> +sub fill_remote_heads {
>>>> +       my $remotes = shift;
>>>> +
>>>> +       my @heads = map { "remotes/$_" } keys %$remotes;
>>>> +       ## A QUESTION: perhaps it would be better to pass @remoteheads as parameter?
>>>> +       my @remoteheads = git_get_heads_list(undef, @heads);
>>>> +       foreach my $remote (keys %$remotes) {
>>>> +               $remotes->{$remote}{'heads'} =
>>>> +                       [ grep { $_->{'name'} =~ s!^$remote/!! } @remoteheads ];
>>>> +       }
>>>> +}
[...]
>>>> A QUESTION: perhaps it would be better to pass @remoteheads as parameter?
>>>> I wonder if won't end up with calling git_get_heads_list multiple times...
>>>> well, the improvement can be left for later.  Answering this question
>>>> should not affect accepting this patch series.
>>>
>>> I'm not sure I actually understand the question. Who should we pass
>>> @remoteheads to?
>>
>> I meant here passing @remoteheads or \@remoteheads, i.e. result of call
>> to git_get_heads_list, to fill_remote_heads (as one of parameters).
>> I was worrying, perhaps unnecessarily, about calling git_get_heads_list
>> more than once... but I guess that we did it anyway.
> 
> I'm still not sure I follow. git_get_heads_list only gets called once,

Ah, that's all right then.  I was worring that it is called more than
once in one request by gitweb.

> with all the remotes/<remotename> pathspecs as a single array
> argument. This _does_ mean that when the total number of remote heads
> is greater than the limit some remotes will not display complete
> information in summary view. The real issue here is, I think, that
> there is no trivial way to tell which remotes have incomplete
> information and which don't, meaning that in the subsequent
> git_remote_block calls we'll have no way to provide visual feedback
> (the ellipsis) when some heads are missing.

Errr... shouldn't we leave limiting number of heads to fill_remote_heads,
which can do limiting per remote (with each remote having up to $limit
remote-tracking branches / remote heads), instead of having 
git_get_heads_list do it?

Something like this:

+sub fill_remote_heads {
+       my ($remotes, $limit) = @_;
+
+       my @heads = map { "remotes/$_" } keys %$remotes;
+       my @remoteheads = git_get_heads_list(undef, @heads);
+       foreach my $remote (keys %$remotes) {
+               $remotes->{$remote}{'heads'} =
+                       [ grep { $_->{'name'} =~ s!^$remote/!! } @remoteheads ];
+		$remotes->{$remote}{'heads'} =
+			[ @{$remotes->{$remote}{'heads'}}[0..$limit-1] ]
+			if (@{$remotes->{$remote}{'heads'}} > $limit);
+       }
+}

Though perhaps it will be more clear with if as statement, not as modifier:

+sub fill_remote_heads {
+       my ($remotes, $limit) = @_;
+
+       my @heads = map { "remotes/$_" } keys %$remotes;
+       my @remoteheads = git_get_heads_list(undef, @heads);
+       foreach my $remote (keys %$remotes) {
+               $remotes->{$remote}{'heads'} =
+                       [ grep { $_->{'name'} =~ s!^$remote/!! } @remoteheads ];
+		if (@{$remotes->{$remote}{'heads'}} > $limit) {
+			$remotes->{$remote}{'heads'} =
+				[ @{$remotes->{$remote}{'heads'}}[0..$limit-1] ]
+		}	
+       }
+}


>>>> Though I am not sure if it is good public API.  Perhaps it is...
>>>
>>> The alternative would be to have git_remote to handle the single
>>> remote case, and possibly even have the action name be 'remote' rather
>>> than 'remotes' in that case.
>>
>> We might want to have 'remote' as alias to 'remotes' anyway.
>>
>> Though what you propose, having separately 'remotes' for list of all
>> remotes, and 'remote' + name of remote, might be a good idea.
> 
> I hope you don't mind if I opt to leave these refinements for later ;-)

Not a problem.  For backward compatibility we would have to accept
'remotes/<remote>' in addition to more correct 'remote/<remote>', but
this is not a problem.

-- 
Jakub Narebski
Poland

Re: [PATCHv6 10/10] gitweb: group remote heads by remote

[Giuseppe Bilotta]

On Thu, Nov 4, 2010 at 11:41 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> On Wed, 3 Nov 2010, Giuseppe Bilotta wrote:
>>
>> I think that in most cases there won't be any need for limiting.
>> Public cases of lots of remotes with lots of branches are, I suspect,
>> rare.
>
> It's not about _public_ cases; I think that it is in very rare cases
> that public repository would want to display remotes and remote-tracking
> branches.

That's a good point.

> I think remote_heads feature is more important for _local_ use, for
> example browsing one own repository using git-instaweb.  In such cases
> number of remotes and of remote-tracking branches might be large (I have
> 11 remotes, not all active, and 58 remote-tracking branches).
>
> BTW. would next version of this series include patch to git-instaweb
> enabling 'remote_heads' feature for it (gitweb_conf function)?

I will look into that.

>> with all the remotes/<remotename> pathspecs as a single array
>> argument. This _does_ mean that when the total number of remote heads
>> is greater than the limit some remotes will not display complete
>> information in summary view. The real issue here is, I think, that
>> there is no trivial way to tell which remotes have incomplete
>> information and which don't, meaning that in the subsequent
>> git_remote_block calls we'll have no way to provide visual feedback
>> (the ellipsis) when some heads are missing.
>
> Errr... shouldn't we leave limiting number of heads to fill_remote_heads,
> which can do limiting per remote (with each remote having up to $limit
> remote-tracking branches / remote heads), instead of having
> git_get_heads_list do it?
>
> Something like this:
>
> +sub fill_remote_heads {
> +       my ($remotes, $limit) = @_;
> +
> +       my @heads = map { "remotes/$_" } keys %$remotes;
> +       my @remoteheads = git_get_heads_list(undef, @heads);
> +       foreach my $remote (keys %$remotes) {
> +               $remotes->{$remote}{'heads'} =
> +                       [ grep { $_->{'name'} =~ s!^$remote/!! } @remoteheads ];
> +               $remotes->{$remote}{'heads'} =
> +                       [ @{$remotes->{$remote}{'heads'}}[0..$limit-1] ]
> +                       if (@{$remotes->{$remote}{'heads'}} > $limit);
> +       }
> +}
>
> Though perhaps it will be more clear with if as statement, not as modifier:
>
> +sub fill_remote_heads {
> +       my ($remotes, $limit) = @_;
> +
> +       my @heads = map { "remotes/$_" } keys %$remotes;
> +       my @remoteheads = git_get_heads_list(undef, @heads);
> +       foreach my $remote (keys %$remotes) {
> +               $remotes->{$remote}{'heads'} =
> +                       [ grep { $_->{'name'} =~ s!^$remote/!! } @remoteheads ];
> +               if (@{$remotes->{$remote}{'heads'}} > $limit) {
> +                       $remotes->{$remote}{'heads'} =
> +                               [ @{$remotes->{$remote}{'heads'}}[0..$limit-1] ]
> +               }
> +       }
> +}

Either solution is fine, but it would require grabbing all the remote
heads. The real issue here is, I think understanding what is the
purpose of limiting in gitweb. Is it to reduce runtime? is it to
reduce clutter on the screen? In the first case, the limiting should
be done as early as possible (i.e. during the git call that retrieves
the data); in the latter case, is it _really_ needed at all?


-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCHv6 10/10] gitweb: group remote heads by remote

[Jakub Narebski]

On Mon, 8 Nov 2010, Giuseppe Bilotta wrote:
> On Thu, Nov 4, 2010 at 11:41 AM, Jakub Narebski <jnareb@gmail.com> wrote:
>> On Wed, 3 Nov 2010, Giuseppe Bilotta wrote:

>> I think remote_heads feature is more important for _local_ use, for
>> example browsing one own repository using git-instaweb.  In such cases
>> number of remotes and of remote-tracking branches might be large (I have
>> 11 remotes, not all active, and 58 remote-tracking branches).
>>
>> BTW. would next version of this series include patch to git-instaweb
>> enabling 'remote_heads' feature for it (gitweb_conf function)?
> 
> I will look into that.

It can be as simple as

diff --git i/git-instaweb.sh w/git-instaweb.sh
index e6f6ecd..50f65b1 100755
--- i/git-instaweb.sh
+++ w/git-instaweb.sh
@@ -580,6 +580,8 @@ gitweb_conf() {
 our \$projectroot = "$(dirname "$fqgitdir")";
 our \$git_temp = "$fqgitdir/gitweb/tmp";
 our \$projects_list = \$projectroot;
+
+$feature{'remote_heads'}{'default'} = [1]
 EOF
 }
 

We might want to enable more features for git-instaweb, but I think
it would out of scope for planned commit (for 'remote heads' series).

>>> with all the remotes/<remotename> pathspecs as a single array
>>> argument. This _does_ mean that when the total number of remote heads
>>> is greater than the limit some remotes will not display complete
>>> information in summary view. The real issue here is, I think, that
>>> there is no trivial way to tell which remotes have incomplete
>>> information and which don't, meaning that in the subsequent
>>> git_remote_block calls we'll have no way to provide visual feedback
>>> (the ellipsis) when some heads are missing.
>>
>> Errr... shouldn't we leave limiting number of heads to fill_remote_heads,
>> which can do limiting per remote (with each remote having up to $limit
>> remote-tracking branches / remote heads), instead of having
>> git_get_heads_list do it?
>>
>> Something like this:
>>
>> +sub fill_remote_heads {
>> +       my ($remotes, $limit) = @_;
>> +
>> +       my @heads = map { "remotes/$_" } keys %$remotes;
>> +       my @remoteheads = git_get_heads_list(undef, @heads);
>> +       foreach my $remote (keys %$remotes) {
>> +               $remotes->{$remote}{'heads'} =
>> +                       [ grep { $_->{'name'} =~ s!^$remote/!! } @remoteheads ];
>> +               $remotes->{$remote}{'heads'} =
>> +                       [ @{$remotes->{$remote}{'heads'}}[0..$limit-1] ]
>> +                       if (@{$remotes->{$remote}{'heads'}}> $limit);
>> +       }
>> +}
>>
>> Though perhaps it will be more clear with if as statement, not as modifier:
>>
>> +sub fill_remote_heads {
>> +       my ($remotes, $limit) = @_;
>> +
>> +       my @heads = map { "remotes/$_" } keys %$remotes;
>> +       my @remoteheads = git_get_heads_list(undef, @heads);
>> +       foreach my $remote (keys %$remotes) {
>> +               $remotes->{$remote}{'heads'} =
>> +                       [ grep { $_->{'name'} =~ s!^$remote/!! } @remoteheads ];
>> +               if (@{$remotes->{$remote}{'heads'}}> $limit) {
>> +                       $remotes->{$remote}{'heads'} =
>> +                               [ @{$remotes->{$remote}{'heads'}}[0..$limit-1] ]
>> +               }
>> +       }
>> +}
> 
> Either solution is fine, but it would require grabbing all the remote
> heads. The real issue here is, I think understanding what is the
> purpose of limiting in gitweb. Is it to reduce runtime? is it to
> reduce clutter on the screen? In the first case, the limiting should
> be done as early as possible (i.e. during the git call that retrieves
> the data); in the latter case, is it _really_ needed at all?

It is to reduce clutter on the screen, or rather have 'summary' view
for a project (for a repository) to be really a _summary_.  That's why
there is limit of 15 commits in shortlog, of 15 branches in heads, of
15 tags.  This action is meant to present balanced overview of 
repository.


Regarding gitweb performance, it is quite important to pass limit to
git-log / git-rev-list needed also for 'summary' view; passing limit
to git command really matters here.

git_get_heads_list passes '--count='.($limit+1) to git-for-each-ref,
but I don't think that it improves performance in any measurable way.
Similar with saving a memory: it is negligible amount.  So if we can
do better at the cost of running git_get_heads_list without a limit,
I say go for it.

Note that the costly part of git_get_heads_list is forking git command,
so it makes absolutely no sense to run git_get_heads_list once per
remote instead of doing limiting per-remote in Perl.  The former would
affect performance badly, I can guess.

-- 
Jakub Narebski
Poland

Re: [PATCHv6 10/10] gitweb: group remote heads by remote

[Giuseppe Bilotta]

On Mon, Nov 8, 2010 at 12:05 PM, Jakub Narebski <jnareb@gmail.com> wrote:
> On Mon, 8 Nov 2010, Giuseppe Bilotta wrote:
>> On Thu, Nov 4, 2010 at 11:41 AM, Jakub Narebski <jnareb@gmail.com> wrote:
>>> On Wed, 3 Nov 2010, Giuseppe Bilotta wrote:
>
>>> I think remote_heads feature is more important for _local_ use, for
>>> example browsing one own repository using git-instaweb.  In such cases
>>> number of remotes and of remote-tracking branches might be large (I have
>>> 11 remotes, not all active, and 58 remote-tracking branches).
>>>
>>> BTW. would next version of this series include patch to git-instaweb
>>> enabling 'remote_heads' feature for it (gitweb_conf function)?
>>
>> I will look into that.
>
> It can be as simple as
>
> diff --git i/git-instaweb.sh w/git-instaweb.sh
> index e6f6ecd..50f65b1 100755
> --- i/git-instaweb.sh
> +++ w/git-instaweb.sh
> @@ -580,6 +580,8 @@ gitweb_conf() {
>  our \$projectroot = "$(dirname "$fqgitdir")";
>  our \$git_temp = "$fqgitdir/gitweb/tmp";
>  our \$projects_list = \$projectroot;
> +
> +$feature{'remote_heads'}{'default'} = [1]
>  EOF
>  }

Thanks.

> We might want to enable more features for git-instaweb, but I think
> it would out of scope for planned commit (for 'remote heads' series).

Definitely 8-)

>> Either solution is fine, but it would require grabbing all the remote
>> heads. The real issue here is, I think understanding what is the
>> purpose of limiting in gitweb. Is it to reduce runtime? is it to
>> reduce clutter on the screen? In the first case, the limiting should
>> be done as early as possible (i.e. during the git call that retrieves
>> the data); in the latter case, is it _really_ needed at all?
>
> It is to reduce clutter on the screen, or rather have 'summary' view
> for a project (for a repository) to be really a _summary_.  That's why
> there is limit of 15 commits in shortlog, of 15 branches in heads, of
> 15 tags.  This action is meant to present balanced overview of
> repository.

Ok.

> Regarding gitweb performance, it is quite important to pass limit to
> git-log / git-rev-list needed also for 'summary' view; passing limit
> to git command really matters here.
>
> git_get_heads_list passes '--count='.($limit+1) to git-for-each-ref,
> but I don't think that it improves performance in any measurable way.
> Similar with saving a memory: it is negligible amount.  So if we can
> do better at the cost of running git_get_heads_list without a limit,
> I say go for it.

The gain in performance is, I believe, related to the number of heads
and the number of remotes that are to be enumerated. 11 remotes with a
total of 58 remote branches (the case you mentioned, for example)
might not feel much of a difference between pre- and post-filtering,
but something bigger might start to feel the effect.

I think the strongest point in favour of post-filtering is that the
feature is intended for use mostly for local repositories anyway.

> Note that the costly part of git_get_heads_list is forking git command,
> so it makes absolutely no sense to run git_get_heads_list once per
> remote instead of doing limiting per-remote in Perl.  The former would
> affect performance badly, I can guess.

That is indeed the reason why I chose to go the single call way, even
though it meant having the limit end up being used somewhat
incorrectly.

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCHv6 10/10] gitweb: group remote heads by remote

[Jakub Narebski]

On Mon, Nov 8, 2010, Giuseppe Bilotta napisał:
> On Mon, Nov 8, 2010 at 12:05 PM, Jakub Narebski <jnareb@gmail.com> wrote:
>> On Mon, 8 Nov 2010, Giuseppe Bilotta wrote:
>>> On Thu, Nov 4, 2010 at 11:41 AM, Jakub Narebski <jnareb@gmail.com> wrote:
>>>> On Wed, 3 Nov 2010, Giuseppe Bilotta wrote:
[...]
>>>> BTW. would next version of this series include patch to git-instaweb
>>>> enabling 'remote_heads' feature for it (gitweb_conf function)?
>>>
>>> I will look into that.
>>
>> It can be as simple as
>>
>> diff --git i/git-instaweb.sh w/git-instaweb.sh
>> index e6f6ecd..50f65b1 100755
>> --- i/git-instaweb.sh
>> +++ w/git-instaweb.sh
>> @@ -580,6 +580,8 @@ gitweb_conf() {
>>  our \$projectroot = "$(dirname "$fqgitdir")";
>>  our \$git_temp = "$fqgitdir/gitweb/tmp";
>>  our \$projects_list = \$projectroot;
>> +
>> +$feature{'remote_heads'}{'default'} = [1]
>>  EOF
>>  }
> 
> Thanks.

I forgot about trailing semicolon.  It should be:

    +$feature{'remote_heads'}{'default'} = [1];

>>> Either solution is fine, but it would require grabbing all the remote
>>> heads. The real issue here is, I think understanding what is the
>>> purpose of limiting in gitweb. Is it to reduce runtime? is it to
>>> reduce clutter on the screen? In the first case, the limiting should
>>> be done as early as possible (i.e. during the git call that retrieves
>>> the data); in the latter case, is it _really_ needed at all?
[...]

>> Regarding gitweb performance, it is quite important to pass limit to
>> git-log / git-rev-list needed also for 'summary' view; passing limit
>> to git command really matters here.
>>
>> git_get_heads_list passes '--count='.($limit+1) to git-for-each-ref,
>> but I don't think that it improves performance in any measurable way.
>> Similar with saving a memory: it is negligible amount.  So if we can
>> do better at the cost of running git_get_heads_list without a limit,
>> I say go for it.
> 
> The gain in performance is, I believe, related to the number of heads
> and the number of remotes that are to be enumerated. 11 remotes with a
> total of 58 remote branches (the case you mentioned, for example)
> might not feel much of a difference between pre- and post-filtering,
> but something bigger might start to feel the effect.

Actually I would guess it would depend on what git-for-each-ref does
it.  I would guess that git-for-each-ref reads in all refs anyway,
the limit only matters if format contains fieldnames that need accessing
the object,... like e.g. '%(subject)' which git_get_heads_list requests,
but git_heads_body doesn't use.  Ehh...
 
> I think the strongest point in favour of post-filtering is that the
> feature is intended for use mostly for local repositories anyway.

True.
 
>> Note that the costly part of git_get_heads_list is forking git command,
>> so it makes absolutely no sense to run git_get_heads_list once per
>> remote instead of doing limiting per-remote in Perl.  The former would
>> affect performance badly, I can guess.
> 
> That is indeed the reason why I chose to go the single call way, even
> though it meant having the limit end up being used somewhat
> incorrectly.

I think that single call and post-filtering would be reasonable 
compromise: reasonable performance (single fork), and correct results.

-- 
Jakub Narebski
Poland