Re: [PATCHv5 12/12] gitweb: gather more remote data
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.
Show 9 quoted lines
> 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?
Show 17 quoted lines
>
> 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.
Show 5 quoted lines
> +# 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.
Show 5 quoted lines
> + 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).
Show 5 quoted lines
> + 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);
Show 20 quoted lines
> +
> + # 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.
Show 11 quoted lines
> + 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).
Show 13 quoted lines
> +}
> +
> 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};'Show 18 quoted lines
> +
> + 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.
Show 6 quoted lines
> +
> + $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.
Show 7 quoted lines
> +
> + 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...
Show 5 quoted lines
> +}
> +
> +# 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?
Show 27 quoted lines
> +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.
Show 20 quoted lines
> +
> + 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?
Show 11 quoted lines
> 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).
Show 9 quoted lines
> }
>
> 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?
Show 6 quoted lines
> +
> + 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.
Show 32 quoted lines
> +
> 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
Giuseppe Bilotta
