Re: [PATCH 7/7] gitweb: group remote heads
- From
Jakub Narebski <jnareb@gmail.com>
- Date
- Sep 17, 2010, 16:54 UTC
- Message-ID
- <201009171854.03476.jnareb@gmail.com>
- In-Reply-To
- <1284629465-14798-8-git-send-email-giuseppe.bilotta@gmail.com>
On Thu, 16 Sep 2010, Giuseppe Bilotta wrote:
Show 10 quoted lines
> 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.
Show 11 quoted lines
> +}
> +
> 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?
Show 11 quoted lines
> "</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.
Show 13 quoted lines
> + }
> +
> +}
> +
> 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.
Show 11 quoted lines
> 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...
Show 12 quoted lines
> }
>
> 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.
Show 7 quoted lines
> } > git_footer_html(); > } > -- > 1.7.3.rc1.230.g8b572 > >
-- Jakub Narebski Poland