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

[Jakub Narebski <jnareb@gmail.com>]

On Thu, 16 Sep 2010, Giuseppe Bilotta wrote:

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

Probably more Perl-ish way would be to use

  +	my $limit = shift;

but this version is also all right.

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

Gitweb usualy uses

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

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

About Ævar comment: while

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

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

> +	close $fd or return ();

  +	close $fd or return;

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

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

  +	return @remotes;

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

That would require more changes to the code.

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

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

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

Why not

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

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

> +	foreach my $remote (@remotenames) {

It would be then

  +	foreach my $remote (@$remotenames) {

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

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

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

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

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

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

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

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

Calling convention would change with my proposed change.

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

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

Same here.

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

-- 
Jakub Narebski
Poland