[PATCHv5 00/12] gitweb: remote_heads feature

[PATCHv5 00/12] gitweb: remote_heads feature

[Giuseppe Bilotta]

New version of the remote heads feature for gitweb (v5 because the
previous rehash was actually v4, although I forgot to prefix the
patchset accordingly).

I've included all the comments received from the previous review (unless
I forgot about something), as well as the suggestions about how to
select and present remotes in summary view.

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 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-9 present a new routine for grouping data (will only be used
by the final patch in this set presently, but it has other prospective
uses too), patch 10 introduces refactors some code that is used in
summary view (patch 11) and in the final version of the remotes view
(patch 12).

Remotes view now displays the remote URL(s) together with the heads (or
only the URL(s) in summary view if there are too many remotes), although
I'm open to suggestions about the opportunity of displaying ssh URLs.

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 (12):
  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 extra text after action in page header
  gitweb: remotes view for a single remote
  gitweb: auxiliary function to group data
  gitweb: group styling
  gitweb: git_repo_url() routine
  gitweb: use git_repo_url() in summary
  gitweb: gather more remote data

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

-- 
1.7.3.68.g6ec8

[PATCHv5 01/12] gitweb: introduce remote_heads feature

[Giuseppe Bilotta]

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

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index a85e2f6..f09fcee 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -486,6 +486,18 @@ our %feature = (
 		'sub' => sub { feature_bool('highlight', @_) },
 		'override' => 0,
 		'default' => [0]},
+
+	# Make gitweb show remotes too in the heads list
+
+	# To enable system wide have in $GITWEB_CONFIG
+	# $feature{'remote_heads'}{'default'} = [1];
+	# To have project specific config enable override in $GITWEB_CONFIG
+	# $feature{'remote_heads'}{'override'} = 1;
+	# and in project config gitweb.remote_heads = 0|1;
+	'remote_heads' => {
+		'sub' => sub { feature_bool('remote_heads', @_) },
+		'override' => 0,
+		'default' => [0]},
 );
 
 sub gitweb_get_feature {
@@ -3146,10 +3158,12 @@ sub git_get_heads_list {
 	my $limit = shift;
 	my @headslist;
 
+	my $remote_heads = gitweb_check_feature('remote_heads');
+
 	open my $fd, '-|', git_cmd(), 'for-each-ref',
 		($limit ? '--count='.($limit+1) : ()), '--sort=-committerdate',
 		'--format=%(objectname) %(refname) %(subject)%00%(committer)',
-		'refs/heads'
+		'refs/heads', ($remote_heads ? 'refs/remotes' : ())
 		or return;
 	while (my $line = <$fd>) {
 		my %ref_item;
@@ -3160,7 +3174,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

[PATCHv5 02/12] gitweb: git_get_heads_list accepts an optional list of refs.

[Giuseppe Bilotta]

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

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index f09fcee..27c455e 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3155,15 +3155,18 @@ sub parse_from_to_diffinfo {
 ## parse to array of hashes functions
 
 sub git_get_heads_list {
-	my $limit = shift;
+	my ($limit, @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

[PATCHv5 03/12] 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 27c455e..fe9f73e 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -712,6 +712,7 @@ our %actions = (
 	"log" => \&git_log,
 	"patch" => \&git_patch,
 	"patches" => \&git_patches,
+	"remotes" => \&git_remotes,
 	"rss" => \&git_rss,
 	"atom" => \&git_atom,
 	"search" => \&git_search,
@@ -5111,6 +5112,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);
 
@@ -5118,7 +5120,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');
 
@@ -5196,6 +5199,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,
@@ -5503,13 +5513,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

[PATCHv5 04/12] 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 |   20 +++++++++++++++++---
 1 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index fe9f73e..c3ce7a3 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3721,6 +3721,20 @@ 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=>$_, -replay=>1)}, $_)
+	} @ref_views
+}
+
 sub format_paging_nav {
 	my ($action, $page, $has_next_link) = @_;
 	my $paging_nav;
@@ -5497,7 +5511,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();
@@ -5510,7 +5524,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');
@@ -5526,7 +5540,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

[PATCHv5 05/12] 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 c3ce7a3..e70897e 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -4977,7 +4977,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

[PATCHv5 06/12] gitweb: allow extra text after action in page header

[Giuseppe Bilotta]

This extra text is intended to 'specify' the action. Therefore, if it's
present, the action name in the header will be turned into a link to
the action itself but without specifying any parameter.

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 e70897e..76cf806 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3514,7 +3514,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{'header_extra'}) {
+				$action_print = $cgi->a({-href => href(action=>$action)},
+					esc_html($action));
+			}
+			print " / $action_print";
+		}
+		if (defined $opts{'header_extra'}) {
+			print " / $opts{'header_extra'}";
 		}
 		print "\n";
 	}
-- 
1.7.3.68.g6ec8

[PATCHv5 07/12] gitweb: remotes view for a single remote

[Giuseppe Bilotta]

If the hash parameter is passed to gitweb, remotes will interpret it as
the name of a remote and limit the view the the heads of that remote.

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 76cf806..7c62701 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -5547,13 +5547,28 @@ 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'));
+	my $remote = $input_params{'hash'};
+
+	git_header_html(undef, undef, 'header_extra' => $remote);
+	git_print_page_nav('', '',  $head, undef, $head,
+		format_ref_views($remote ? '' : 'remotes'));
 	git_print_header_div('summary', $project);
 
-	my @remotelist = git_get_heads_list(undef, 'remotes');
-	if (@remotelist) {
-		git_heads_body(\@remotelist, $head);
+	if (defined $remote) {
+		# only display the heads in a given remote
+		my @headslist = map {
+			my $ref = $_ ;
+			$ref->{'name'} =~ s!^$remote/!!;
+			$ref
+		} git_get_heads_list(undef, "remotes/$remote");
+		if (@headslist) {
+			git_heads_body(\@headslist, $head);
+		}
+	} else {
+		my @remotelist = git_get_heads_list(undef, 'remotes');
+		if (@remotelist) {
+			git_heads_body(\@remotelist, $head);
+		}
 	}
 	git_footer_html();
 }
-- 
1.7.3.68.g6ec8

[PATCHv5 08/12] gitweb: auxiliary function to group data

[Giuseppe Bilotta]

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 7c62701..8f11fb5 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3786,6 +3786,22 @@ sub git_print_header_div {
 	      "\n</div>\n";
 }
 
+sub git_group {
+	my ($class, $id, @rest) = @_;
+
+	my $content_func = pop @rest;
+
+	$class = join(' ', 'group', $class);
+
+	print $cgi->start_div({
+		-class => $class,
+		-id => $id,
+	});
+	git_print_header_div(@rest);
+	$content_func->() if defined $content_func;
+	print $cgi->end_div;
+}
+
 sub print_local_time {
 	print format_local_time(@_);
 }
-- 
1.7.3.68.g6ec8

[PATCHv5 09/12] gitweb: group styling

[Giuseppe Bilotta]

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

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

[PATCHv5 10/12] gitweb: git_repo_url() routine

[Giuseppe Bilotta]

The routine creates a table row with a name and a repository address,
like the one used at the top of summary view.

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 8f11fb5..2ab9327 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3802,6 +3802,11 @@ sub git_group {
 	print $cgi->end_div;
 }
 
+sub git_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(@_);
 }
-- 
1.7.3.68.g6ec8

[PATCHv5 11/12] gitweb: use git_repo_url() in summary

[Giuseppe Bilotta]

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 2ab9327..93017a4 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -5190,7 +5190,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 git_repo_url($url_tag, $git_url);
 		$url_tag = "";
 	}
 
-- 
1.7.3.68.g6ec8

[PATCHv5 12/12] gitweb: gather more remote data

[Giuseppe Bilotta]

Collect remote information by gathering the list of remotes, and then
the URL(s) and heads in each remote. In summary view, limit the number
of remotes for which we collect data, as well as the maximum number of
heads per remote that we display.

If the number of remotes is higher than the prescribed limit, do not
collect any heads information and just show the remotes names and the
links to the corresponding fetch and push URLs. Otherwise, create a
group for each remote and display all the information there.

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 93017a4..1e671ff 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2758,6 +2758,57 @@ sub git_get_last_activity {
 	return (undef, undef);
 }
 
+# Return an array with: a hash of remote names mapping to the corresponding
+# remote heads, the value of the $limit parameter, and a boolean indicating
+# whether we managed to get all the remotes or not.
+# If $limit is specified and the number of heads found is higher, the head
+# list is empy. Additional filtering on the number of heads can be done when
+# displaying the remotes.
+sub git_get_remotes_data {
+	my ($limit, $wanted) = @_;
+	my %remotes;
+	open my $fd, '-|' , git_cmd(), 'remote', '-v';
+	return unless $fd;
+	my $more = 1;
+	while (my $remote = <$fd> and $more) {
+		chomp $remote;
+		$remote =~ s!\t(.*?)\s+\((\w+)\)$!!;
+		next if $wanted and not $remote eq $wanted;
+		my $url = $1;
+		my $key = $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
+		$more = exists $remotes{$remote};
+		$more ||= defined $limit ? (keys(%remotes) < $limit) : 1;
+		if ($more) {
+			$remotes{$remote} ||= { 'heads' => () };
+			$remotes{$remote}{$key} = $url;
+		}
+	}
+	close $fd or return;
+
+	# if the while finished with $more being true, it means we ran
+	# out of remotes before we hit $limit; paradoxically, it being true out
+	# of the loop means there are 'no more' remotes.
+	# Rather than waste time renaming the variable, we just read it to
+	# answer the question: "did we get all remotes before we hit
+	# the limit?"
+	if ($more) {
+		my @heads = map { "remotes/$_" } keys %remotes;
+		my @remoteheads = git_get_heads_list(undef, @heads);
+		foreach (keys %remotes) {
+			my $remote = $_;
+			$remotes{$remote}{'heads'} = [ grep {
+				$_->{'name'} =~ s!^$remote/!!
+			} @remoteheads ];
+		}
+	}
+	return (\%remotes, $limit, $more);
+}
+
 sub git_get_references {
 	my $type = shift || "";
 	my %refs;
@@ -5018,6 +5069,93 @@ sub git_heads_body {
 	print "</table>\n";
 }
 
+# Display a single remote block
+sub git_remote_body {
+	my ($remote, $rdata, $limit, $head, $single) = @_;
+	my %rdata = %{$rdata};
+	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 .= git_repo_url("URL", $fetch);
+		} else {
+			$urls .= git_repo_url("Fetch URL", $fetch);
+			$urls .= git_repo_url("Push URL", $push) if defined $push;
+		}
+	} elsif (defined $push) {
+		$urls .= git_repo_url("Push URL", $push);
+	} else {
+		$urls .= git_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)}, "...");
+		}
+	}
+
+	my $content = sub {
+		print $urls;
+		git_heads_body($heads, $head, 0, $maxheads, $dots);
+	};
+
+	if (defined $single and $single) {
+		$content->();
+	} else {
+		git_group("remotes", $remote, "remotes", $remote, $remote, $content);
+	}
+}
+
+# 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 ($remotelist, $limit, $have_all, $head) = @_;
+	my %remotes = %$remotelist;
+	if ($have_all) {
+		while (my ($remote, $rdata) = each %remotes) {
+			git_remote_body($remote, $rdata, $limit, $head);
+		}
+	} else {
+		print "<table class=\"heads\">\n";
+		my $alternate = 1;
+		while (my ($remote, $rdata) = each (%$remotelist)) {
+			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>";
+	}
+}
+
 sub git_search_grep_body {
 	my ($commitlist, $from, $to, $extra) = @_;
 	$from = 0 unless defined $from;
@@ -5164,7 +5302,7 @@ sub git_summary {
 	# there are more ...
 	my @taglist  = git_get_tags_list(16);
 	my @headlist = git_get_heads_list(16, 'heads');
-	my @remotelist = $remote_heads ? git_get_heads_list(16, 'remotes') : ();
+	my @remotelist = $remote_heads ? git_get_remotes_data(16) : ();
 	my @forklist;
 	my $check_forks = gitweb_check_feature('forks');
 
@@ -5244,9 +5382,7 @@ sub git_summary {
 
 	if (@remotelist) {
 		git_print_header_div('remotes');
-		git_heads_body(\@remotelist, $head, 0, 15,
-		               $#remotelist <= 15 ? undef :
-		               $cgi->a({-href => href(action=>"remotes")}, "..."));
+		git_remotes_body(@remotelist, $head);
 	}
 
 	if (@forklist) {
@@ -5570,26 +5706,25 @@ sub git_remotes {
 	my $head = git_get_head_hash($project);
 	my $remote = $input_params{'hash'};
 
+	my @remotelist = git_get_remotes_data(undef, $remote);
+	die_error(500, "Unable to get remote information") unless @remotelist;
+
+	if (keys(%{$remotelist[0]}) == 0) {
+		die_error(404, defined $remote ?
+			"Remote $remote not found" :
+			"No remotes found");
+	}
+
 	git_header_html(undef, undef, 'header_extra' => $remote);
 	git_print_page_nav('', '',  $head, undef, $head,
 		format_ref_views($remote ? '' : 'remotes'));
-	git_print_header_div('summary', $project);
 
 	if (defined $remote) {
-		# only display the heads in a given remote
-		my @headslist = map {
-			my $ref = $_ ;
-			$ref->{'name'} =~ s!^$remote/!!;
-			$ref
-		} git_get_heads_list(undef, "remotes/$remote");
-		if (@headslist) {
-			git_heads_body(\@headslist, $head);
-		}
+		git_print_header_div('remotes', "$remote remote for $project");
+		git_remote_body($remote, $remotelist[0]->{$remote}, undef, $head, 1);
 	} else {
-		my @remotelist = git_get_heads_list(undef, 'remotes');
-		if (@remotelist) {
-			git_heads_body(\@remotelist, $head);
-		}
+		git_print_header_div('summary', "$project remotes");
+		git_remotes_body(@remotelist, $head);
 	}
 	git_footer_html();
 }
-- 
1.7.3.68.g6ec8

Re: [PATCHv5 01/12] gitweb: introduce remote_heads feature

[Jakub Narebski]

On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:

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

Nice and straighforward.  For what it is worth:

Acked-by: Jakub Narebski <jnareb@gmail.com>

> ---
>  gitweb/gitweb.perl |   18 ++++++++++++++++--
>  1 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index a85e2f6..f09fcee 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -486,6 +486,18 @@ our %feature = (
>  		'sub' => sub { feature_bool('highlight', @_) },
>  		'override' => 0,
>  		'default' => [0]},
> +
> +	# Make gitweb show remotes too in the heads list

Very minor nitpick: it should probably be (but I am not a native
English speaker, so I migh be mistaken)

  +	# Make gitweb show also remotes 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]},
[...]

-- 
Jakub Narebski
Poland

Re: [PATCHv5 02/12] gitweb: git_get_heads_list accepts an optional list of refs.

[Jakub Narebski]

On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:

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

Possible (very minor) nitpick:

s/on remote_heads option/on 'remote_heads' feature/

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

Very nice extension of an API.  Good work.  FWIW

Acked-by: Jakub Narebski <jnareb@gmail.com>

> ---
>  gitweb/gitweb.perl |   11 +++++++----
>  1 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index f09fcee..27c455e 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -3155,15 +3155,18 @@ sub parse_from_to_diffinfo {
>  ## parse to array of hashes functions
>  
>  sub git_get_heads_list {
> -	my $limit = shift;
> +	my ($limit, @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
> 
> 

-- 
Jakub Narebski
Poland

Re: [PATCHv5 03/12] gitweb: separate heads and remotes lists

[Jakub Narebski]

On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:

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

Nice (and still uncontroversial) solution.  FWIW

Acked-by: Jakub Narebski <jnareb@gmail.com>

> @@ -5118,7 +5120,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');
>  
> @@ -5196,6 +5199,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")}, "..."));
> +	}

Nice thing.  This meanst that "remotes" section is displayed *only* if
'remote_heads' feature is enabled and we actually have remote-tracking
branches.

> @@ -5503,13 +5513,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');

It's a pity that we can't simply write "git_get_heads_list('heads');",
but I think it would be serious overengineering for a tiny little gain.

>  	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();
> +}

Hmmmm... what we have there is a bit of code repetition with git_heads
and git_remotes, but I think is unevitable... besides you would be 
extending git_remotes subroutine in subsequent patches.  So it doesn't
matter, I think.

-- 
Jakub Narebski
Poland

Re: [PATCHv5 04/12] gitweb: nagivation menu for tags, heads and remotes

[Jakub Narebski]

On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:

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

Nice idea.  FWIW

Acked-by: Jakub Narebski <jnareb@gmail.com>

> ---
>  gitweb/gitweb.perl |   20 +++++++++++++++++---
>  1 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index fe9f73e..c3ce7a3 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -3721,6 +3721,20 @@ 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
> +

Minor nitpick: this empty line here is not necessary.  But I think
that Junio can remove it when applying.

> +sub format_ref_views {
> +	my ($current) = @_;
> +	my @ref_views = qw{tags heads};

Hmmm... should we pass it as argument, or use $action in place of
$current?  Each solution has its advantages and disadvantages.  Current
solution has the advantage of avoiding using global variables, solution
using $action has the (supposed) advantage of automatically detecting
current action.

I would probably write

  +	my $current) = shift;
  +	my @ref_views = qw(tags heads);

but it makes no difference, and this style is also good.

> +	push @ref_views, 'remotes' if gitweb_check_feature('remote_heads');
> +	return join " | ", map {
> +		$_ eq $current ? $_ :
> +		$cgi->a({-href => href(action=>$_, -replay=>1)}, $_)
> +	} @ref_views
> +}
[...]

> -	git_print_page_nav('','', $head,undef,$head);
> +	git_print_page_nav('','', $head,undef,$head,format_ref_views('tags'));

> -	git_print_page_nav('','', $head,undef,$head);
> +	git_print_page_nav('','', $head,undef,$head,format_ref_views('heads'));

> -	git_print_page_nav('','', $head,undef,$head);
> +	git_print_page_nav('','', $head,undef,$head,format_ref_views('remotes'));

Nice API.  I like it.

-- 
Jakub Narebski
Poland

Re: [PATCHv5 05/12] gitweb: use fullname as hash_base in heads link

[Jakub Narebski]

On Fri, 24 Sep 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>

Well, of course of 'hash' parameter uses 'fullname' then 'hash_base'
also should.  Good futureproofing, no argument here.  FWIW

Acked-by: Jakub Narebski <jnareb@gmail.com>

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


-- 
Jakub Narebski
Poland

Re: [PATCHv5 06/12] gitweb: allow extra text after action in page header

[Jakub Narebski]

On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:

> This extra text is intended to 'specify' the action. Therefore, if it's
> present, the action name in the header will be turned into a link to
> the action itself but without specifying any parameter.

What this feature is intended *for*?  I guess it is meant to be used
in the case where there is additional parameter that specifies action,
like e.g. a single remote view, where $action is 'remotes', but there
is extra parameter ('hash_base' is (ab)used for this) that specifies
remote.  Then we want to make 'remotes' in "breadcrumbs" navigation at
top of page to link to generic 'remotes' view.  Isn't it?

But the above is just my guess, covering only case where there is both
$action defined, and 'header_extra' option set.

You need to explain this in the commit message.

> 
> 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 e70897e..76cf806 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -3514,7 +3514,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{'header_extra'}) {

We spell optional named parameter with '-' as prefix, for example
$opts{'-remove_signoff'} in git_print_log(), to be able to use key
without quoting (bareword-like, autoquoting), like $opts{-nohtml}
or $opts{-pad_before}, or $opts{-size}, or $opts{-tag}... though
gitweb is not very consisnte here.

So it should probably be

  +			if (defined $opts{'-header_extra'}) {

or even

  +			if (defined $opts{-header_extra}) {


I also think that we can think of better name for this option than
'header_extra', although what this name could be eludes me.

> +				$action_print = $cgi->a({-href => href(action=>$action)},
> +					esc_html($action));

I don't think we need to run esc_html on action: it is checked that it
is one of specified set of possible values, and it can be ensured that
it neither contains anything than printable characters, not any HTML
special characters.

> +			}
> +			print " / $action_print";
> +		}
> +		if (defined $opts{'header_extra'}) {
> +			print " / $opts{'header_extra'}";

Hmmm...

>  		}
>  		print "\n";
>  	}
> -- 
> 1.7.3.68.g6ec8
> 
> 

-- 
Jakub Narebski
Poland

Re: [PATCHv5 00/12] gitweb: remote_heads feature

[Jakub Narebski]

On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:

> New version of the remote heads feature for gitweb (v5 because the
> previous rehash was actually v4, although I forgot to prefix the
> patchset accordingly).
> 
> I've included all the comments received from the previous review (unless
> I forgot about something), as well as the suggestions about how to
> select and present remotes in summary view.
> 
> 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 useful even without the rest of the series.

Those patches can go as-is.  For what it is worth they all have ACK
from me.
 
I am working on review of other patches in this series.

-- 
Jakub Narebski
Poland

Re: [PATCHv5 01/12] gitweb: introduce remote_heads feature

[Ævar Arnfjörð Bjarmason]

2010/9/26 Jakub Narebski <jnareb@gmail.com>:
> On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:
>> +     # Make gitweb show remotes too in the heads list
>
> Very minor nitpick: it should probably be (but I am not a native
> English speaker, so I migh be mistaken)
>
>  +     # Make gitweb show also remotes in the heads list

Maybe:

    # Configure the display of remotes in the heads list

or

    # Toggle the display of remotes in the heads list

?

Re: [PATCHv5 01/12] gitweb: introduce remote_heads feature

[David Ripton]

On 09/26/10 15:19, Ævar Arnfjörð Bjarmason wrote:
> 2010/9/26 Jakub Narebski<jnareb@gmail.com>:
>> On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:
>>> +     # Make gitweb show remotes too in the heads list
>>
>> Very minor nitpick: it should probably be (but I am not a native
>> English speaker, so I migh be mistaken)
>>
>>   +     # Make gitweb show also remotes in the heads list
>
> Maybe:
>
>      # Configure the display of remotes in the heads list
>
> or
>
>      # Toggle the display of remotes in the heads list

IMO either of those are fine.  Or you could just swap a couple of words 
in the original message to make it sound a bit more natural:

# Make gitweb also show remotes in the heads list

-- 
David Ripton    dripton@ripton.net

Re: [PATCHv5 07/12] gitweb: remotes view for a single remote

[Jakub Narebski]

On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:

> If the hash parameter is passed to gitweb, remotes will interpret it as
> the name of a remote and limit the view the the heads of that remote.

Errr... I think this commit message needs rewriting to be more clear.
Perhaps:

  When 'remotes' view is passed 'hash' parameter, it would interprete it
  as the name of a remote ...

> 
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
> ---
>  gitweb/gitweb.perl |   25 ++++++++++++++++++++-----
>  1 files changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 76cf806..7c62701 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -5547,13 +5547,28 @@ 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'));
> +	my $remote = $input_params{'hash'};

I am not sure about using 'hash' parameter for that.

On one hand it is a hack that allow us to not worry about adding extra
code to evaluate_path_info() subroutine, so that natural path_info URL
of http://git.example.com/repo.git/remotes/<remote> would use <remote>
as name of remote to limit to.

On the other hand it is abusing semantic of 'hash' parameter.  Remote
name is not revision name or object id.  

What makes this issue stronger is the fact that URL is part of API,
and if we make mistake here, we would have to maintain backward 
compatibility (at least if it appears in a released version).

> +
> +	git_header_html(undef, undef, 'header_extra' => $remote);

I don't quite like the name of this parameter, and I am not sure
if I like the API either.

> +	git_print_page_nav('', '',  $head, undef, $head,
> +		format_ref_views($remote ? '' : 'remotes'));

Why this change?

>  	git_print_header_div('summary', $project);
>  
> -	my @remotelist = git_get_heads_list(undef, 'remotes');
> -	if (@remotelist) {
> -		git_heads_body(\@remotelist, $head);
> +	if (defined $remote) {
> +		# only display the heads in a given remote
> +		my @headslist = map {
> +			my $ref = $_ ;
> +			$ref->{'name'} =~ s!^$remote/!!;
> +			$ref
> +		} git_get_heads_list(undef, "remotes/$remote");

Hmmm... do we need this temporary variable?  Does it make anything
more clear?

> +		if (@headslist) {
> +			git_heads_body(\@headslist, $head);
> +		}

This part is the same (modulo name of variable) in both branches of this
conditional.

> +	} else {
> +		my @remotelist = git_get_heads_list(undef, 'remotes');
> +		if (@remotelist) {
> +			git_heads_body(\@remotelist, $head);
> +		}
>  	}
>  	git_footer_html();
>  }
> -- 
> 1.7.3.68.g6ec8
> 
> 

-- 
Jakub Narebski
Poland

Re: [PATCHv5 08/12] gitweb: auxiliary function to group data

[Jakub Narebski]

On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:

> Subject: gitweb: auxiliary function to group data
>

Errr... what!?  git_group() is not "auxiliary function to group data",
but a template for output of group of data.

It would be probably good to describe how this output looks like
(using e.g. ASII-art mockup) in a commit message.

> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
> ---
>  gitweb/gitweb.perl |   16 ++++++++++++++++
>  1 files changed, 16 insertions(+), 0 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 7c62701..8f11fb5 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -3786,6 +3786,22 @@ sub git_print_header_div {
>  	      "\n</div>\n";
>  }
>  
> +sub git_group {

Name?

> +	my ($class, $id, @rest) = @_;
> +
> +	my $content_func = pop @rest;
> +
> +	$class = join(' ', 'group', $class);
> +
> +	print $cgi->start_div({
> +		-class => $class,
> +		-id => $id,
> +	});
> +	git_print_header_div(@rest);
> +	$content_func->() if defined $content_func;

More defensive programming would be to use

  +	$content_func->() if ref($content_func) eq 'CODE';

Or even:

  +	if (ref($content) eq 'CODE') {
  +		$content->();
  +	} elsif (ref($content) eq 'ARRAY') {
  +		print @$content;
  +	} elsif (!ref($content) && defined($content)) {
  +		print $content;
  +	}

Well, $content could be also open filehandle...

> +	print $cgi->end_div;
> +}

Nice usage of start_div and end_div.

> +
>  sub print_local_time {
>  	print format_local_time(@_);
>  }
> -- 
> 1.7.3.68.g6ec8
> 
> 

-- 
Jakub Narebski
Poland

Re: [PATCHv5 09/12] gitweb: group styling

[Jakub Narebski]

On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:

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

As I currently at this moment don't use web browser which supports 
'display: inline-block;', I can't say much about this patch.  But
it does degrade nicely.

-- 
Jakub Narebski
Poland

Re: [PATCHv5 10/12] gitweb: git_repo_url() routine

[Jakub Narebski]

On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:

> The routine creates a table row with a name and a repository address,
> like the one used at the top of summary view.
[...]

> +sub git_repo_url {
> +	my ($name, $url) = @_;
> +	return "<tr class=\"metadata_url\"><td>$name</td><td>$url</td></tr>\n";
> +}

It should be  *format_repo_url*, and not git_repo_url, isn't it?

By the way, doesn't gitweb include code dealing with repository URL;
why you don't _use_ this subroutine, then?

-- 
Jakub Narebski
Poland

Re: [PATCHv5 11/12] gitweb: use git_repo_url() in summary

[Jakub Narebski]

On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:

> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -5190,7 +5190,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 git_repo_url($url_tag, $git_url);
>  		$url_tag = "";
>  	}

I think this patch should be squashed together with previous one.
FWIW, ACK (after squashing of course).

-- 
Jakub Narebski
Poland

Re: [PATCHv5 01/12] gitweb: introduce remote_heads feature

[Giuseppe Bilotta]

On Sun, Sep 26, 2010 at 9:47 PM, David Ripton <dripton@ripton.net> wrote:
> On 09/26/10 15:19, Ævar Arnfjörð Bjarmason wrote:
>>
>> 2010/9/26 Jakub Narebski<jnareb@gmail.com>:
>>>
>>> On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:
>>>>
>>>> +     # Make gitweb show remotes too in the heads list
>>>
>>> Very minor nitpick: it should probably be (but I am not a native
>>> English speaker, so I migh be mistaken)
>>>
>>>  +     # Make gitweb show also remotes in the heads list
>>
>> Maybe:
>>
>>     # Configure the display of remotes in the heads list
>>
>> or
>>
>>     # Toggle the display of remotes in the heads list
>
> IMO either of those are fine.  Or you could just swap a couple of words in
> the original message to make it sound a bit more natural:
>
> # Make gitweb also show remotes in the heads list

I think I like the 'Configure the display' one better. I'll rephrase
the comment in the next rehash of the series.

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCHv5 04/12] gitweb: nagivation menu for tags, heads and remotes

[Giuseppe Bilotta]

2010/9/26 Jakub Narebski <jnareb@gmail.com>:
> On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:
>>
>> +# 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
>> +
>
> Minor nitpick: this empty line here is not necessary.  But I think
> that Junio can remove it when applying.

Since there have been a couple of stylistic suggestions for the
comments in the first two patches too, I can probably resend the whole
series including these changes, unless Junio wants to do the
hand-tuning.

>> +sub format_ref_views {
>> +     my ($current) = @_;
>> +     my @ref_views = qw{tags heads};
>
> Hmmm... should we pass it as argument, or use $action in place of
> $current?  Each solution has its advantages and disadvantages.  Current
> solution has the advantage of avoiding using global variables, solution
> using $action has the (supposed) advantage of automatically detecting
> current action.

Not using $action has the advantage of making it possible to enable
the $action command if it's wanted, which is something that I use in a
subsequent patch (when enabling single-remote view). But this is of
course debatable.

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCHv5 06/12] gitweb: allow extra text after action in page header

[Giuseppe Bilotta]

2010/9/26 Jakub Narebski <jnareb@gmail.com>:
> On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:
>
>> This extra text is intended to 'specify' the action. Therefore, if it's
>> present, the action name in the header will be turned into a link to
>> the action itself but without specifying any parameter.
>
> What this feature is intended *for*?  I guess it is meant to be used
> in the case where there is additional parameter that specifies action,
> like e.g. a single remote view, where $action is 'remotes', but there
> is extra parameter ('hash_base' is (ab)used for this) that specifies
> remote.  Then we want to make 'remotes' in "breadcrumbs" navigation at
> top of page to link to generic 'remotes' view.  Isn't it?

Yes, this is exactly the intended purpose of this feature. It is of
course possible to extend other views to have a similar behavior (I'm
thinking in particular about log views here).

> But the above is just my guess, covering only case where there is both
> $action defined, and 'header_extra' option set.
>
> You need to explain this in the commit message.

I will clarify this in the next rehash of the patch.

>> 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 e70897e..76cf806 100755
>> --- a/gitweb/gitweb.perl
>> +++ b/gitweb/gitweb.perl
>> @@ -3514,7 +3514,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{'header_extra'}) {
>
> We spell optional named parameter with '-' as prefix, for example
> $opts{'-remove_signoff'} in git_print_log(), to be able to use key
> without quoting (bareword-like, autoquoting), like $opts{-nohtml}
> or $opts{-pad_before}, or $opts{-size}, or $opts{-tag}... though
> gitweb is not very consisnte here.
>
> So it should probably be
>
>  +                     if (defined $opts{'-header_extra'}) {
>
> or even
>
>  +                     if (defined $opts{-header_extra}) {
>
>
> I also think that we can think of better name for this option than
> 'header_extra', although what this name could be eludes me.

I will add the dash to the option. Naming it header_extra keeps the
meaning of this extra text generic, but considering that the intended
use is mostly for the single-remote view (or similar, if/when they are
added) we could call it something related (I can only think of
'main_argument' right now but I think this would suck more than
header_extra).

>> +                             $action_print = $cgi->a({-href => href(action=>$action)},
>> +                                     esc_html($action));
>
> I don't think we need to run esc_html on action: it is checked that it
> is one of specified set of possible values, and it can be ensured that
> it neither contains anything than printable characters, not any HTML
> special characters.

Ok, I will remove it.

>> +                     }
>> +                     print " / $action_print";
>> +             }
>> +             if (defined $opts{'header_extra'}) {
>> +                     print " / $opts{'header_extra'}";
>
> Hmmm...

You don't sound very convinced. I had some doubts myself about whether
the slash should be inserted autmatically or whether it should be up
to the caller to include it in header_extra, but I'm not sure this is
what you are perplexed about.

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCHv5 07/12] gitweb: remotes view for a single remote

[Giuseppe Bilotta]

2010/9/26 Jakub Narebski <jnareb@gmail.com>:
> On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:
>
>> If the hash parameter is passed to gitweb, remotes will interpret it as
>> the name of a remote and limit the view the the heads of that remote.
>
> Errr... I think this commit message needs rewriting to be more clear.
> Perhaps:
>
>  When 'remotes' view is passed 'hash' parameter, it would interprete it
>  as the name of a remote ...

Can do.

>> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
>> ---
>>  gitweb/gitweb.perl |   25 ++++++++++++++++++++-----
>>  1 files changed, 20 insertions(+), 5 deletions(-)
>>
>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
>> index 76cf806..7c62701 100755
>> --- a/gitweb/gitweb.perl
>> +++ b/gitweb/gitweb.perl
>> @@ -5547,13 +5547,28 @@ 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'));
>> +     my $remote = $input_params{'hash'};
>
> I am not sure about using 'hash' parameter for that.
>
> On one hand it is a hack that allow us to not worry about adding extra
> code to evaluate_path_info() subroutine, so that natural path_info URL
> of http://git.example.com/repo.git/remotes/<remote> would use <remote>
> as name of remote to limit to.

It also spares us the introduction of a new parameter, since I'm not
aware of any of the current parameters that could take this role.

> On the other hand it is abusing semantic of 'hash' parameter.  Remote
> name is not revision name or object id.

True. But I cannot think of a use for the hash parameter in remotes
view, and since hash is also used for _named_ refs, it  "kind of"
makes sense to use for a name that is not actually a hash or ref. The
only other option (aside from the use of a new parameter) would be to
use 'hash_base', by reading it as 'base for the ref names' in the
sense that remote refs are 'refs/remote/<base>/name' where the base is
the remote name. But I doubt that's actually more sensible than using
'hash'.

> What makes this issue stronger is the fact that URL is part of API,
> and if we make mistake here, we would have to maintain backward
> compatibility (at least if it appears in a released version).

We _could_ go on the safe side and use a new parameter for this.

>> +
>> +     git_header_html(undef, undef, 'header_extra' => $remote);
>
> I don't quite like the name of this parameter, and I am not sure
> if I like the API either.
>
>> +     git_print_page_nav('', '',  $head, undef, $head,
>> +             format_ref_views($remote ? '' : 'remotes'));
>
> Why this change?

As I mentioned in my replies to the other respective patches, I think
it makes sense to make "all remotes" view easily accessible from the
"single remote" view, and there are two ways I can think of: one is
the "extra header text" way, by making the action name before it point
to "all remotes". The other is to enable 'remotes' in the page nav
submenu when we are in single remotes view (which is why I had the
$current in format_ref_views instead of $action, and which is what is
done by this change).  IMO it makes sense to have both ways available,
but I'm open to suggestions about different approaches.

>>       git_print_header_div('summary', $project);
>>
>> -     my @remotelist = git_get_heads_list(undef, 'remotes');
>> -     if (@remotelist) {
>> -             git_heads_body(\@remotelist, $head);
>> +     if (defined $remote) {
>> +             # only display the heads in a given remote
>> +             my @headslist = map {
>> +                     my $ref = $_ ;
>> +                     $ref->{'name'} =~ s!^$remote/!!;
>> +                     $ref
>> +             } git_get_heads_list(undef, "remotes/$remote");
>
> Hmmm... do we need this temporary variable?  Does it make anything
> more clear?
>
>> +             if (@headslist) {
>> +                     git_heads_body(\@headslist, $head);
>> +             }
>
> This part is the same (modulo name of variable) in both branches of this
> conditional.
>
>> +     } else {
>> +             my @remotelist = git_get_heads_list(undef, 'remotes');
>> +             if (@remotelist) {
>> +                     git_heads_body(\@remotelist, $head);
>> +             }
>>       }
>>       git_footer_html();

Yup, I can make the logic here cleaner and reuse variables (and code)
across the conditionals.

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCHv5 08/12] gitweb: auxiliary function to group data

[Giuseppe Bilotta]

2010/9/26 Jakub Narebski <jnareb@gmail.com>:
> On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:
>
>> Subject: gitweb: auxiliary function to group data
>>
>
> Errr... what!?  git_group() is not "auxiliary function to group data",
> but a template for output of group of data.

I will rephrase

> It would be probably good to describe how this output looks like
> (using e.g. ASII-art mockup) in a commit message.

Well, that would depend on the CSS that is used ... should I squash
the styling in this patch then?

>> +sub git_group {
>
> Name?

git_collection? git_collect_data? I'm a little short on ideas.
git_section? git_subsection? the function (with different styling) can
probably be used even for the main sections in each view (think
summary view in particular).

>> +     my ($class, $id, @rest) = @_;
>> +
>> +     my $content_func = pop @rest;
>> +
>> +     $class = join(' ', 'group', $class);
>> +
>> +     print $cgi->start_div({
>> +             -class => $class,
>> +             -id => $id,
>> +     });
>> +     git_print_header_div(@rest);
>> +     $content_func->() if defined $content_func;
>
> More defensive programming would be to use
>
>  +     $content_func->() if ref($content_func) eq 'CODE';
>
> Or even:
>
>  +     if (ref($content) eq 'CODE') {
>  +             $content->();
>  +     } elsif (ref($content) eq 'ARRAY') {
>  +             print @$content;
>  +     } elsif (!ref($content) && defined($content)) {
>  +             print $content;
>  +     }
>
> Well, $content could be also open filehandle...

Ah, very interesting and very flexible, I'll steal your idea.

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCHv5 09/12] gitweb: group styling

[Giuseppe Bilotta]

2010/9/27 Jakub Narebski <jnareb@gmail.com>:
> On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:
>
>> +div.group {
>> +     margin: .5em;
>> +     border: 1px solid #d9d8d1;
>> +     display: inline-block;
>> +}
>
> As I currently at this moment don't use web browser which supports
> 'display: inline-block;', I can't say much about this patch.  But
> it does degrade nicely.

Would you think it's sensible to squash this with the previous patch, btw?

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCHv5 10/12] gitweb: git_repo_url() routine

[Giuseppe Bilotta]

2010/9/27 Jakub Narebski <jnareb@gmail.com>:
> On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:
>
>> The routine creates a table row with a name and a repository address,
>> like the one used at the top of summary view.
> [...]
>
>> +sub git_repo_url {
>> +     my ($name, $url) = @_;
>> +     return "<tr class=\"metadata_url\"><td>$name</td><td>$url</td></tr>\n";
>> +}
>
> It should be  *format_repo_url*, and not git_repo_url, isn't it?

Right.

> By the way, doesn't gitweb include code dealing with repository URL;
> why you don't _use_ this subroutine, then?

The only code for the display of a repo URL is inlined into summary
view, which in the next patch uses this function to do the display. I
can probably squash them together.

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCHv5 06/12] gitweb: allow extra text after action in page header

[Jakub Narebski]

On Mon, 27 Sep 2010, Giuseppe Bilotta wrote:
> 2010/9/26 Jakub Narebski <jnareb@gmail.com>:
>> On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:

>>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
>>> index e70897e..76cf806 100755
>>> --- a/gitweb/gitweb.perl
>>> +++ b/gitweb/gitweb.perl
>>> @@ -3514,7 +3514,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{'header_extra'}) {
[...]

>> I also think that we can think of better name for this option than
>> 'header_extra', although what this name could be eludes me.
> 
> I will add the dash to the option. Naming it header_extra keeps the
> meaning of this extra text generic, but considering that the intended
> use is mostly for the single-remote view (or similar, if/when they are
> added) we could call it something related (I can only think of
> 'main_argument' right now but I think this would suck more than
> header_extra).

Perhaps name this argument '-subaction' or '-action_argument', or
something like that; the meaning of this argument (as shown by the fact
that action name is hyperlinked) is to specify subaction of given action,
i.e. (possibly) more detailed view of some part of generic (argument-less)
action output.
 
>>> +                     }
>>> +                     print " / $action_print";
>>> +             }
>>> +             if (defined $opts{'header_extra'}) {
>>> +                     print " / $opts{'header_extra'}";
>>
>> Hmmm...
> 
> You don't sound very convinced. I had some doubts myself about whether
> the slash should be inserted autmatically or whether it should be up
> to the caller to include it in header_extra, but I'm not sure this is
> what you are perplexed about.

Ah, I'm sorry, I have misunderstood the control flow here.  I see now
that it is about adding subaction specifier after action, so that
'remotes' view for single remote 'origin' (<URL>/remotes/origin path_info
URL, see comments for patch introducing single-remote view) has

  _projects_ / _repo.git_ / _remotes_ / origin

in the "breadcrumbs" navigation in page header.

This should be better described in the commit message.
-- 
Jakub Narebski
Poland

Re: [PATCHv5 07/12] gitweb: remotes view for a single remote

[Jakub Narebski]

On Mon, 27 Sep 2010, Giuseppe Bilotta wrote:
> 2010/9/26 Jakub Narebski <jnareb@gmail.com>:
>> On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:

>>> +
>>> +     git_header_html(undef, undef, 'header_extra' => $remote);
>>
>> I don't quite like the name of this parameter, and I am not sure
>> if I like the API either.

Explanation: what I don't like (a tiny bit) about API is the need for
those 'undef, undef'... but this might be unavoidable, at least without
heavy hackery for little gain, because of the way Perl passes arguments
to subroutines.  (And no, rewrite of gitweb in Perl 6 is not a viable
solution ;-))

> As I mentioned in my replies to the other respective patches, I think
> it makes sense to make "all remotes" view easily accessible from the
> "single remote" view, and there are two ways I can think of: one is
> the "extra header text" way, by making the action name before it point
> to "all remotes". The other is to enable 'remotes' in the page nav
> submenu when we are in single remotes view (which is why I had the
> $current in format_ref_views instead of $action, and which is what is
> done by this change).  IMO it makes sense to have both ways available,
> but I'm open to suggestions about different approaches.

Now I understand it, and I completly agree that it is a very good
solution.  But it needs better description in the commit message.

Sidenote: http://git.oblomov.eu/rbot/remotes/anonj2 looks like doesn't
have this patch.  In the navigation bar it has

  _projects_ / _rbot_ / remotes

and not

  _projects_ / _rbot_ / _remotes_ / anonj2

>>> -     git_print_page_nav('','', $head,undef,$head,format_ref_views('remotes'));
[...]
>>> +     git_print_page_nav('', '',  $head, undef, $head,

Why not

    +     git_print_page_nav('','', $head,undef,$head,

>>> +             format_ref_views($remote ? '' : 'remotes'));
>>
>> Why this change?

I might be not clear, but I meant here the change in formatting of call
to git_print_page_nav... and what I have not noticed is the fact that
format_ref_views is passed diferent argument.

And I see now here why passing action-like parameter ($current) to
format_ref_views allow for this.  I withdraw then proposal for using
$action global variable in place of $current argument; this API is
better (though perhaps this could be described in commit message)..
 
-- 
Jakub Narebski
Poland

Re: [PATCHv5 08/12] gitweb: auxiliary function to group data

[Jakub Narebski]

Dnia poniedziałek 27. września 2010 09:26, Giuseppe Bilotta napisał:
> 2010/9/26 Jakub Narebski <jnareb@gmail.com>:
>> On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:
>>
>>> Subject: gitweb: auxiliary function to group data
>>>
>>
>> Errr... what!?  git_group() is not "auxiliary function to group data",
>> but a template for output of group of data.
> 
> I will rephrase

Though I don't know how such rephrasing should look like.

>> It would be probably good to describe how this output looks like
>> (using e.g. ASCII-art mockup) in a commit message.
> 
> Well, that would depend on the CSS that is used ... should I squash
> the styling in this patch then?

No, I don't think it is needed, but it can be done.

>>> +sub git_group {
>>
>> Name?
> 
> git_collection? git_collect_data? I'm a little short on ideas.
> git_section? git_subsection? the function (with different styling) can
> probably be used even for the main sections in each view (think
> summary view in particular).

git_group_html / git_subgroup_html / git_subsection_html
print_group / print_subgroup / print_section / print_subsection

It needs either *_html suffix (like git_header_html), or print_* prefix
to denote that it prints HTML fragment, I think.

>>> +     $content_func->() if defined $content_func;
>>
>> More defensive programming would be to use
>>
>>  +     $content_func->() if ref($content_func) eq 'CODE';
>>
>> Or even:
>>
>>  +     if (ref($content) eq 'CODE') {
>>  +             $content->();
>>  +     } elsif (ref($content) eq 'ARRAY') {
>>  +             print @$content;

The 'ARRAY' part is probably unnecessary overengineering.

>>  +     } elsif (!ref($content) && defined($content)) {
>>  +             print $content;
>>  +     }

Or even (in the vein of further overengineering)

    +     } elsif (ref($content) eq 'SCALAR') {
    +             print esc_html($$content);
    +     } elsif (!ref($content) && defined($content)) {
    +             print $content;
    +     }

or vice versa ;-)

>>
>> Well, $content could be also open filehandle...

Though I don't know how to check that.  ref on filehandles return
'GLOB'... well, we can use 'openhandle' from Scalar::Util (core).
But that is probably unnecessary overengineering.
 
> Ah, very interesting and very flexible, I'll steal your idea.
> 
> -- 
> Giuseppe "Oblomov" Bilotta
> 

-- 
Jakub Narebski
Poland

Re: [PATCHv5 04/12] gitweb: nagivation menu for tags, heads and remotes

[Jakub Narebski]

On Mon, 27 Sep 2010, Giuseppe Bilotta wrote:
> 2010/9/26 Jakub Narebski <jnareb@gmail.com>:
>> On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:
>>>
>>> +# 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
>>> +
>>
>> Minor nitpick: this empty line here is not necessary.  But I think
>> that Junio can remove it when applying.
> 
> Since there have been a couple of stylistic suggestions for the
> comments in the first two patches too, I can probably resend the whole
> series including these changes, unless Junio wants to do the
> hand-tuning.

Well, the first half of this series (patches 1 to 5) are good enough,
or almost good enough, to be merged in as they are now.  They might
need at most some minor stylistic corrections.  It depends on Junio
whether he wants to have resend of early part of this series, or if
he prefers to hand-edit those minor corrections.

Patches 6-12 needs, I think, further discussion.
 
>>> +sub format_ref_views {
>>> +     my ($current) = @_;
>>> +     my @ref_views = qw{tags heads};
>>
>> Hmmm... should we pass it as argument, or use $action in place of
>> $current?  Each solution has its advantages and disadvantages.  Current
>> solution has the advantage of avoiding using global variables, solution
>> using $action has the (supposed) advantage of automatically detecting
>> current action.
> 
> Not using $action has the advantage of making it possible to enable
> the $action command if it's wanted, which is something that I use in a
> subsequent patch (when enabling single-remote view). But this is of
> course debatable.

I agree that use of format_ref_views further in this series shows that
your version (with $current passed as argument) is better.

-- 
Jakub Narebski
Poland

Re: [PATCHv5 12/12] gitweb: gather more remote data

[Jakub Narebski]

On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:

Isn't the most important and visible information the fact that both
standalone 'remotes' view and 'remotes' section in the 'summary' page
is now grouped by remotes?  This should be explicitely mentioned in the
commit message.  It is not very visible IMVHO in what is written below.

> Collect remote information by gathering the list of remotes, and then
> the URL(s) and heads in each remote. In summary view, limit the number
> of remotes for which we collect data, as well as the maximum number of
> heads per remote that we display.
> 
> If the number of remotes is higher than the prescribed limit, do not
> collect any heads information and just show the remotes names and the
> links to the corresponding fetch and push URLs. Otherwise, create a
> group for each remote and display all the information there.

You mean here that in 'summary' view, the 'remotes' section consist 
only of list of remote names (is it truncated to 15 remotes) with 
corresponding fetch and push URLs if number of remotes is higher than
the prescribed limit.  In other case 'remotes' section consist of
separate subsection for each remote (is number of remotes limited 
also in this case), and in each subsection there are displayed up to
prescribed limit remote-tracking branches associated with given remote.

Isn't it?

> 
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
> ---
>  gitweb/gitweb.perl |  171 ++++++++++++++++++++++++++++++++++++++++++++++------
>  1 files changed, 153 insertions(+), 18 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 93017a4..1e671ff 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -2758,6 +2758,57 @@ sub git_get_last_activity {
>  	return (undef, undef);
>  }
>  
> +# Return an array with: a hash of remote names mapping to the corresponding
> +# remote heads, the value of the $limit parameter, and a boolean indicating
> +# whether we managed to get all the remotes or not.

Perhaps it would be better to provide an example, rather than trying to
describe the output.

> +# If $limit is specified and the number of heads found is higher, the head
> +# list is empy. Additional filtering on the number of heads can be done when
> +# displaying the remotes.
> +sub git_get_remotes_data {
> +	my ($limit, $wanted) = @_;

I'm not sure if it wouldn't be better to not worry git_get_remotes_data
about $limit, but do limiting on output.  The amount of work gitweb
needs to do in both situation is, I guess, nearly the same.

> +	my %remotes;
> +	open my $fd, '-|' , git_cmd(), 'remote', '-v';
> +	return unless $fd;
> +	my $more = 1;
> +	while (my $remote = <$fd> and $more) {

Why not use 'last' instead of using $more variable (though I have not
checked if it would really make code simpler and more readable).

> +		chomp $remote;
> +		$remote =~ s!\t(.*?)\s+\((\w+)\)$!!;
> +		next if $wanted and not $remote eq $wanted;
> +		my $url = $1;
> +		my $key = $2;

Minor nitpick: why not

  +		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
> +		$more = exists $remotes{$remote};
> +		$more ||= defined $limit ? (keys(%remotes) < $limit) : 1;
> +		if ($more) {
> +			$remotes{$remote} ||= { 'heads' => () };
> +			$remotes{$remote}{$key} = $url;
> +		}
> +	}
> +	close $fd or return;
> +
> +	# if the while finished with $more being true, it means we ran
> +	# out of remotes before we hit $limit; paradoxically, it being true out
> +	# of the loop means there are 'no more' remotes.
> +	# Rather than waste time renaming the variable, we just read it to
> +	# answer the question: "did we get all remotes before we hit
> +	# the limit?"

Ah, I see that the fact that we exited early is important.

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

Hmmmm... as it can be seen making this function do more work results
in this ugly API.  If git_get_remotes_data didn't worry about limiting,
we could return simply %remotes hash (or \%remotes hash reference).

> +}
> +
>  sub git_get_references {
>  	my $type = shift || "";
>  	my %refs;
> @@ -5018,6 +5069,93 @@ sub git_heads_body {
>  	print "</table>\n";
>  }
>  
> +# Display a single remote block
> +sub git_remote_body {
> +	my ($remote, $rdata, $limit, $head, $single) = @_;
> +	my %rdata = %{$rdata};

Why do you need this, instead of simply using $rdata->{'heads'} etc.?

> +	my $heads = $rdata{'heads'};

Why not

  +	my @heads = @{$rdata{'heads'}};

Or

  +	my @heads = @{$rdata->{'heads'}};

without this strange '%rdata = %{$rdata};'

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

Perhaps reverse order of conditions would result in less nested 
conditional... but perhaps not.

> +
> +	$urls .= "</table>\n";
> +
> +	my ($maxheads, $dots);
> +	if (defined $limit) {
> +		$maxheads = $limit - 1;

If git_get_remotes_data didn't do limiting, you could use

  +		$maxheads = scalar @heads;

> +		if ($#{$heads} > $maxheads) {
> +			$dots = $cgi->a({-href => href(action=>"remotes", hash=>$remote)}, "...");
> +		}
> +	}

We can always check if we got more remotes than limit.

> +
> +	my $content = sub {
> +		print $urls;
> +		git_heads_body($heads, $head, 0, $maxheads, $dots);
> +	};
> +
> +	if (defined $single and $single) {

'defined $single and $single' is the same as '$single', because
undefined value is false-ish.

> +		$content->();
> +	} else {
> +		git_group("remotes", $remote, "remotes", $remote, $remote, $content);
> +	}

Hmmm... should git_remote_body be responsible for wrapping in subsection?
Wouldn't it be better to have caller use

  git_group("remotes", $remote, "remotes", $remote, $remote, 
            sub { git_remote_body($remote, $rdata, $limit, $head) });

Sidenote: strange (but perhaps unavoidable) repetition we have here...

> +}
> +
> +# 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

Wouldn't it be better to put displaying only remote names in a separate
subroutine, which would make git_remotes_body extremely simple?

> +sub git_remotes_body {
> +	my ($remotelist, $limit, $have_all, $head) = @_;
> +	my %remotes = %$remotelist;
> +	if ($have_all) {
> +		while (my ($remote, $rdata) = each %remotes) {
> +			git_remote_body($remote, $rdata, $limit, $head);
> +		}
> +	} else {
> +		print "<table class=\"heads\">\n";
> +		my $alternate = 1;
> +		while (my ($remote, $rdata) = each (%$remotelist)) {
> +			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>";

I see that you don't worry here if $fetch == $push, only if they
are defined.  But I think it might be all right... though the exact
output might need some discussion.

> +
> +			print "</tr>\n";
> +		}
> +		print "<tr>\n" .
> +		      "<td colspan=\"3\">" .
> +		      $cgi->a({-href => href(action=>"remotes")}, "...") .
> +		      "</td>\n" . "</tr>\n";
> +		print "</table>";
> +	}
> +}
> +
>  sub git_search_grep_body {
>  	my ($commitlist, $from, $to, $extra) = @_;
>  	$from = 0 unless defined $from;
> @@ -5164,7 +5302,7 @@ sub git_summary {
>  	# there are more ...
>  	my @taglist  = git_get_tags_list(16);
>  	my @headlist = git_get_heads_list(16, 'heads');
> -	my @remotelist = $remote_heads ? git_get_heads_list(16, 'remotes') : ();
> +	my @remotelist = $remote_heads ? git_get_remotes_data(16) : ();

That's not @remotelist any longer, isn't it?

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

Hmmm... (current API, with @remotelist (!) containing list of arguments
to a subroutine).

>  	}
>  
>  	if (@forklist) {
> @@ -5570,26 +5706,25 @@ sub git_remotes {
>  	my $head = git_get_head_hash($project);
>  	my $remote = $input_params{'hash'};
>  
> +	my @remotelist = git_get_remotes_data(undef, $remote);
> +	die_error(500, "Unable to get remote information") unless @remotelist;

@remotelist is not list of remotes.

What if there are no remotes, and no remote-tracking branches?

> +
> +	if (keys(%{$remotelist[0]}) == 0) {
> +		die_error(404, defined $remote ?
> +			"Remote $remote not found" :
> +			"No remotes found");
> +	}

Ah, I see that it is handled here.  Sidenote: with proposed changed
API we can distinguish error case from no-remotes case by returning
undefined value versus returning empty hash / empty hash reference.

> +
>  	git_header_html(undef, undef, 'header_extra' => $remote);
>  	git_print_page_nav('', '',  $head, undef, $head,
>  		format_ref_views($remote ? '' : 'remotes'));
> -	git_print_header_div('summary', $project);
>  
>  	if (defined $remote) {
> -		# only display the heads in a given remote
> -		my @headslist = map {
> -			my $ref = $_ ;
> -			$ref->{'name'} =~ s!^$remote/!!;
> -			$ref
> -		} git_get_heads_list(undef, "remotes/$remote");
> -		if (@headslist) {
> -			git_heads_body(\@headslist, $head);
> -		}
> +		git_print_header_div('remotes', "$remote remote for $project");
> +		git_remote_body($remote, $remotelist[0]->{$remote}, undef, $head, 1);
>  	} else {
> -		my @remotelist = git_get_heads_list(undef, 'remotes');
> -		if (@remotelist) {
> -			git_heads_body(\@remotelist, $head);
> -		}
> +		git_print_header_div('summary', "$project remotes");
> +		git_remotes_body(@remotelist, $head);
>  	}
>  	git_footer_html();
>  }
> -- 
> 1.7.3.68.g6ec8
> 
> 

-- 
Jakub Narebski
Poland

Re: [PATCHv5 08/12] gitweb: auxiliary function to group data

[Giuseppe Bilotta]

2010/9/27 Jakub Narebski <jnareb@gmail.com>:
>>>  +     if (ref($content) eq 'CODE') {
>>>  +             $content->();
>>>  +     } elsif (ref($content) eq 'ARRAY') {
>>>  +             print @$content;
>
> The 'ARRAY' part is probably unnecessary overengineering.
>
>>>  +     } elsif (!ref($content) && defined($content)) {
>>>  +             print $content;
>>>  +     }
>
> Or even (in the vein of further overengineering)
>
>    +     } elsif (ref($content) eq 'SCALAR') {
>    +             print esc_html($$content);
>    +     } elsif (!ref($content) && defined($content)) {
>    +             print $content;
>    +     }
>
> or vice versa ;-)
>
>>>
>>> Well, $content could be also open filehandle...
>
> Though I don't know how to check that.  ref on filehandles return
> 'GLOB'... well, we can use 'openhandle' from Scalar::Util (core).
> But that is probably unnecessary overengineering.

I have made cases for closures, scalar refs and scalar values. I've
also added a case for GLOB or IO::Handle to allow a file handle to be
passed as either *handle or *handle{IO}. I've also added a comment
block explaining the syntax better.

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCHv5 12/12] gitweb: gather more remote data

[Giuseppe Bilotta]

I'm awfully sorry for the delay in the reply to this email. I got
gobbled up by some real-world stuff just while finishing the review of
the last patch.

2010/9/27 Jakub Narebski <jnareb@gmail.com>:
> On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:
>
> Isn't the most important and visible information the fact that both
> standalone 'remotes' view and 'remotes' section in the 'summary' page
> is now grouped by remotes?  This should be explicitely mentioned in the
> commit message.  It is not very visible IMVHO in what is written below.

You're right. I've rewritten the message by putting the grouping
layout in the subject and rephrasing the body of the commit to better
describe what happens both in remotes view and in limited summary
view.

>>
>> +# Return an array with: a hash of remote names mapping to the corresponding
>> +# remote heads, the value of the $limit parameter, and a boolean indicating
>> +# whether we managed to get all the remotes or not.
>
> Perhaps it would be better to provide an example, rather than trying to
> describe the output.

I'm really not sure about how it would help, but I'll try.

>> +# If $limit is specified and the number of heads found is higher, the head
>> +# list is empy. Additional filtering on the number of heads can be done when
>> +# displaying the remotes.
>> +sub git_get_remotes_data {
>> +     my ($limit, $wanted) = @_;
>
> I'm not sure if it wouldn't be better to not worry git_get_remotes_data
> about $limit, but do limiting on output.  The amount of work gitweb
> needs to do in both situation is, I guess, nearly the same.

I think that the real issue is that situations in which gathering all
data and then discarding much of it would be very rare, being that it
would require a very large number of remotes (not remote heads, just
remotes). But considering that even I, a rather casual user, have at
least one project where the number of remotes is in the 20s, I
wouldn't be surprised if there were cases of humongous remote lists.

>> +     my %remotes;
>> +     open my $fd, '-|' , git_cmd(), 'remote', '-v';
>> +     return unless $fd;
>> +     my $more = 1;
>> +     while (my $remote = <$fd> and $more) {
>
> Why not use 'last' instead of using $more variable (though I have not
> checked if it would really make code simpler and more readable).

Because we need to know whether we bail out early or not from the
loop. It might be appropriate to document this in the code.

>> +             chomp $remote;
>> +             $remote =~ s!\t(.*?)\s+\((\w+)\)$!!;
>> +             next if $wanted and not $remote eq $wanted;
>> +             my $url = $1;
>> +             my $key = $2;
>
> Minor nitpick: why not
>
>  +             my ($url, $key) = ($1, $2);

No reason. Can do.

>> +     if ($more) {
>> +             my @heads = map { "remotes/$_" } keys %remotes;
>> +             my @remoteheads = git_get_heads_list(undef, @heads);
>> +             foreach (keys %remotes) {
>> +                     my $remote = $_;
>> +                     $remotes{$remote}{'heads'} = [ grep {
>> +                             $_->{'name'} =~ s!^$remote/!!
>> +                     } @remoteheads ];
>> +             }
>> +     }
>> +     return (\%remotes, $limit, $more);
>
> Hmmmm... as it can be seen making this function do more work results
> in this ugly API.  If git_get_remotes_data didn't worry about limiting,
> we could return simply %remotes hash (or \%remotes hash reference).

If I read your comments correctly, your idea would be to have two
separate functions, one that gets the list of remotes (with their
respective URLs), and a separate one (called when intended) to also
gather the list of heads. The only case in which the latter would not
be called would be when in summary view more than the given number of
maximum remotes were returned from the previous function. Correct?

>> +# Display a single remote block
>> +sub git_remote_body {
>> +     my ($remote, $rdata, $limit, $head, $single) = @_;
>> +     my %rdata = %{$rdata};
>
> Why do you need this, instead of simply using $rdata->{'heads'} etc.?

Because I keep getting messed up with the refs syntax in Perl. Thanks
for the hint.

>> +     my $heads = $rdata{'heads'};
>
> Why not
>
>  +     my @heads = @{$rdata{'heads'}};
>
> Or
>
>  +     my @heads = @{$rdata->{'heads'}};
>
> without this strange '%rdata = %{$rdata};'

Well, the main use of heads was to be passed to git_heads_body so I
thought I would just leave it as a ref.

>> +     if (defined $fetch) {
>> +             if ($fetch eq $push) {
>> +                     $urls .= git_repo_url("URL", $fetch);
>> +             } else {
>> +                     $urls .= git_repo_url("Fetch URL", $fetch);
>> +                     $urls .= git_repo_url("Push URL", $push) if defined $push;
>> +             }
>> +     } elsif (defined $push) {
>> +             $urls .= git_repo_url("Push URL", $push);
>> +     } else {
>> +             $urls .= git_repo_url("", "No remote URL");
>> +     }
>
> Perhaps reverse order of conditions would result in less nested
> conditional... but perhaps not.

I tried. The problem is that we just have that many cases (fetch and
no push, push and no fetch, equal fetch and push, different fetch and
push). I could not think of a way to handle them all in a more
streamlined way.

>> +
>> +     $urls .= "</table>\n";
>> +
>> +     my ($maxheads, $dots);
>> +     if (defined $limit) {
>> +             $maxheads = $limit - 1;
>
> If git_get_remotes_data didn't do limiting, you could use
>
>  +             $maxheads = scalar @heads;
>
>> +             if ($#{$heads} > $maxheads) {
>> +                     $dots = $cgi->a({-href => href(action=>"remotes", hash=>$remote)}, "...");
>> +             }
>> +     }
>
> We can always check if we got more remotes than limit.
>

Actually, this is in reverse. get_remotes_data doesn't do limiting on
the number of heads, which is why $maxheads is $limit - 1 and the
number of heads is compared against it. git_remotes_data does limiting
on the number of _remotes_, and if the limit is hit, git_remote_body
would just not be called.

>> +
>> +     my $content = sub {
>> +             print $urls;
>> +             git_heads_body($heads, $head, 0, $maxheads, $dots);
>> +     };
>> +
>> +     if (defined $single and $single) {
>
> 'defined $single and $single' is the same as '$single', because
> undefined value is false-ish.

I was over-protecting against warnings about the use of undefined
symbols. Cleaned up.

>> +             $content->();
>> +     } else {
>> +             git_group("remotes", $remote, "remotes", $remote, $remote, $content);
>> +     }
>
> Hmmm... should git_remote_body be responsible for wrapping in subsection?
> Wouldn't it be better to have caller use
>
>  git_group("remotes", $remote, "remotes", $remote, $remote,
>            sub { git_remote_body($remote, $rdata, $limit, $head) });

Right.

> Sidenote: strange (but perhaps unavoidable) repetition we have here...

Even with the new syntax discussed for the previous patches this is
unavoidable, it seems.

>> +}
>> +
>> +# 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
>
> Wouldn't it be better to put displaying only remote names in a separate
> subroutine, which would make git_remotes_body extremely simple?

Sure.

>> +                     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>";
>
> I see that you don't worry here if $fetch == $push, only if they
> are defined.  But I think it might be all right... though the exact
> output might need some discussion.

I'm open to suggestions.

>>  sub git_search_grep_body {
>>       my ($commitlist, $from, $to, $extra) = @_;
>>       $from = 0 unless defined $from;
>> @@ -5164,7 +5302,7 @@ sub git_summary {
>>       # there are more ...
>>       my @taglist  = git_get_tags_list(16);
>>       my @headlist = git_get_heads_list(16, 'heads');
>> -     my @remotelist = $remote_heads ? git_get_heads_list(16, 'remotes') : ();
>> +     my @remotelist = $remote_heads ? git_get_remotes_data(16) : ();
>
> That's not @remotelist any longer, isn't it?

Right. I renamed it to @remotedata across the whole file.

>>       my @forklist;
>>       my $check_forks = gitweb_check_feature('forks');
>>
>> @@ -5244,9 +5382,7 @@ sub git_summary {
>>
>>       if (@remotelist) {
>>               git_print_header_div('remotes');
>> -             git_heads_body(\@remotelist, $head, 0, 15,
>> -                            $#remotelist <= 15 ? undef :
>> -                            $cgi->a({-href => href(action=>"remotes")}, "..."));
>> +             git_remotes_body(@remotelist, $head);
>
> Hmmm... (current API, with @remotelist (!) containing list of arguments
> to a subroutine).

It does make the call much cleaner though (think remotedata: it has
the data, and the information on whether the data is complete or not.
doesn't it make sense?)

>>       }
>>
>>       if (@forklist) {
>> @@ -5570,26 +5706,25 @@ sub git_remotes {
>>       my $head = git_get_head_hash($project);
>>       my $remote = $input_params{'hash'};
>>
>> +     my @remotelist = git_get_remotes_data(undef, $remote);
>> +     die_error(500, "Unable to get remote information") unless @remotelist;
>
> What if there are no remotes, and no remote-tracking branches?

Presently, we get 404 - No remotes found if there are no remotes. If
there are remotes without heads, the section will only contain the
URL.

The funny thing is that if there are no remotes there will be an empty
remotes section in summary view, so we probably to check for that too.

>> +
>> +     if (keys(%{$remotelist[0]}) == 0) {
>> +             die_error(404, defined $remote ?
>> +                     "Remote $remote not found" :
>> +                     "No remotes found");
>> +     }
>
> Ah, I see that it is handled here.  Sidenote: with proposed changed
> API we can distinguish error case from no-remotes case by returning
> undefined value versus returning empty hash / empty hash reference.

The question is: do we really want the different API? It does make a
few things cleaner, but it makes other things messier (I'm thinking
here about the calls to git_*_body; compare what we have for remotes
with what we have for tags or heads list in summary view ...)

-- 
Giuseppe "Oblomov" Bilotta