2010/10/27 Jakub Narebski <jnareb@xxxxxxxxx>:
> On Sun, 24 Oct 2010, Giuseppe Bilotta wrote:
>
>> In remote and summary view, display a block for each remote, with the
>> fetch and push URL(s) as well as the list of the remote heads.
>>
>> In summary view, if the number of remotes is higher than a prescribed
>> limit, only display the first <limit> remotes and their fetch and push
>> urls, without any heads information and without grouping.
>
> I like this idea.
Thanks.
> I guess that we can always implement more fancy limiting in the future
> (e.g. taking into account total number of displayed remote heads).
The limiting seems to be the most debatable part of this patch 8-)
>> +# Returns a hash ref mapping remote names to their fetch and push URLs.
>> +# We return a hash ref rather than a hash so that a simple check with defined
>> +# can be used to tell apart the "no remotes" case from other kinds of
>> +# failures.
>
> Just an idea (it is not necessary to implement it; it has its own
> drawbacks): we could implement here
>
> return wantsarray ? %remotes : \%remotes;
>
> so the caller that wants hash, would get hash; and the one wanting
> reference (to distinguish error from no remotes), would get hash
> reference.
It does simplify the summary view case a little, good point.
>> +#
>> +# It is possible to limit the retrieved remotes either by number
>> +# (specifying a -limit parameter) or by name (-wanted parameter).
>
> I don't quite like limiting when generating, and would prefer do limiting
> on display, especially if not doing limiting would not affect performance
> much (git command invoked doesn't do limiting, like in case of
> git_get_heads_list / git_get_tags_list or *most important* parse_commits).
>
> Especially if it complicates code that much (see below).
>
> Not doing limiting here, in git_get_remotes_list (or just git_get_remotes)
> would also make API simpler; the single optional argument would be name of
> remote we want to retrieve.
Hm. By the same token, there would be no need to do the limiting even
when trying to get information about a single remote, meaning we could
make the sub totally parameter-less. OTOH, this would make the calling
routine quite more complex (since it would have to check if the key is
there, and then select that single key etc), much more so than
limiting the number of displayed heads. I'll take the numerical
limiting off the routine.
>> +#
>> +# When a single remote is wanted, we cannot use 'git remote show -n' because
>> +# that command always work (assuming it's a remote URL if it's not defined),
>> +# and we cannot use 'git remote show' because that would try to make a network
>> +# roundtrip. So the only way to find if that particular remote is defined is to
>> +# walk the list provided by 'git remote -v' and stop if and when we find what
>> +# we want.
>
> I would add 'Implemetation note: ' here, which means start with
> 'Implementation note: When ..." -- but it is not necessary.
Can do that.
>> +
>> +# Takes a hash of remotes as first parameter and fills it by adding the
>> +# available remote heads for each of the indicated remotes.
>> +# A maximum number of heads can also be specified.
>
> All git_get_* subroutines _return_ something; this looks more like fill_*
> function for me.
I'm not particularly enamored with _get_, fill will do
>
>> +sub git_get_remote_heads {
>> + my ($remotes, $limit) = @_;
>> + my @heads = map { "remotes/$_" } keys %$remotes;
>> + my @remoteheads = git_get_heads_list($limit, @heads);
>> + foreach (keys %$remotes) {
>> + my $remote = $_;
>> + $remotes->{$remote}{'heads'} = [ grep {
>> + $_->{'name'} =~ s!^$remote/!!
>> + } @remoteheads ];
>> + }
>> +}
>
> We could remove limiting number of heads shown per remote also from here,
> but this time 1.) the $limit parameter is passed down to git_get_heads_list
> which in turn uses $limit as parameter to git command 2.) it doesn't
> simplify code almost at all:
>
> +sub fill_remote_heads {
> + my $remotes = shift;
> +
> + my @heads = map { "remotes/$_" } keys %$remotes;
> + ## A QUESTION: perhaps it would be better to pass @remoteheads as parameter?
> + my @remoteheads = git_get_heads_list(undef, @heads);
> + foreach my $remote (keys %$remotes) {
> + $remotes->{$remote}{'heads'} =
> + [ grep { $_->{'name'} =~ s!^$remote/!! } @remoteheads ];
> + }
> +}
>
>
> A QUESTION: perhaps it would be better to pass @remoteheads as parameter?
> I wonder if won't end up with calling git_get_heads_list multiple times...
> well, the improvement can be left for later. Answering this question
> should not affect accepting this patch series.
I'm not sure I actually understand the question. Who should we pass
@remoteheads to?
>> +
>> sub git_get_references {
>> my $type = shift || "";
>> my %refs;
>> @@ -5054,6 +5114,100 @@ sub git_heads_body {
>> print "</table>\n";
>> }
>>
>> +# Display a single remote block
>
> Note that you would end up with two very similarly named subrotines:
> git_remote_body and git_remotes_body. Perhaps it would be better to
> call this one git_remote_block, or git_describe_remote?
Makes sense. I'll go with git_remote_block for similarity with git_remotes_list
>> +sub git_remote_body {
>> + my ($remote, $rdata, $limit, $head) = @_;
>> +
>> + my $heads = $rdata->{'heads'};
>> + my $fetch = $rdata->{'fetch'};
>> + my $push = $rdata->{'push'};
>
> We could have used the following Perl shortcut
>
> + my ($heads, $fetch, $push) = @{$rdata}{qw(heads fetch push)};
>
> but it isn't needed, and I guess it even could hurt readibility...
OTOH, if readibility was a priority, we wouldn't use Perl would we ;-)
Joking aside, I think I'll leave it as is, unless there's some
significant performance improvement in using the latter syntax.
>> +
>> + my $urls = "<table class=\"projects_list\">\n" ;
>> +
>> + if (defined $fetch) {
>> + if ($fetch eq $push) {
>> + $urls .= format_repo_url("URL", $fetch);
>> + } else {
>> + $urls .= format_repo_url("Fetch URL", $fetch);
>> + $urls .= format_repo_url("Push URL", $push) if defined $push;
>> + }
>> + } elsif (defined $push) {
>> + $urls .= format_repo_url("Push URL", $push);
>> + } else {
>> + $urls .= format_repo_url("", "No remote URL");
>> + }
>> +
>> + $urls .= "</table>\n";
>
> I'm not sure about naming this variable $urls...
I'm open to suggestions. $urls_table ?
>> +
>> + my ($maxheads, $dots);
>> + if (defined $limit) {
>> + $maxheads = $limit - 1;
>> + if ($#{$heads} > $maxheads) {
>> + $dots = $cgi->a({-href => href(action=>"remotes", hash=>$remote)}, "...");
>> + }
>> + }
>> +
>> + print $urls;
>> + git_heads_body($heads, $head, 0, $maxheads, $dots);
>
> Wouldn't this be simpler:
>
> + my $dots;
> + if (defined $limit && $limit < @$heads) {
> + $dots = $cgi->a({-href => href(action=>"remotes", hash=>$remote)}, "...");
> + }
> +
> + print $urls;
> + git_heads_body($heads, $head, 0, $limit, $dots);
>
> We would do similar trick as in other parts: request one item more than we
> display to check if there is more data.
Right.
>> +}
>> +
>> +# Display a list of remote names with the respective fetch and push URLs
>> +# This routine only gets called when there are more remotes than the given
>> +# limit, so we know that we have to append an ellipsis to the table and
>> +# that we have to pop one of the keys.
>
> I think it would be a better idea to make this subroutine generic: display
> only list of remotes, limited to $limit remotes maximum.
That's probably a good idea. I've taken some suggestions from your
proposed code, but kept the while loop.
>> + print "<td>" .
>> + $cgi->a({-href=> href(action=>'remotes', hash=>$remote),
>> + -class=> "list name"},esc_html($remote)) . "</td>";
>
> Very, very minor nitpick: if "<td>" is on separate line, why "</td>"
> isn't?
Good question.
>> + print "</table>";
>> +}
>> +
>> +# Display remote heads grouped by remote, unless there are too many
>> +# remotes ($have_all is false), in which case we only display the remote
>> +# names
>
> This of course also would have to be changed if limiting is done at
> display time, not at generation time (at least for limiting list of
> remotes, if not for generating list of refs).
Actually, it doesn't need to change much, since $limit was still taken
into consideration.
>> +sub git_remotes_body {
>> + my ($remotedata, $limit, $head) = @_;
>> + if (not defined $limit or scalar keys %$remotedata <= $limit) {
>> + git_get_remote_heads($remotedata, $limit);
>> + while (my ($remote, $rdata) = each %$remotedata) {
>> + git_print_section({-class=>"remote", -id=>$remote},
>> + ["remotes", $remote, $remote], sub {
>> + git_remote_body($remote, $rdata, $limit, $head);
>> + });
>> + }
>> + } else {
>> + git_remotes_list($remotedata, $limit);
>> + }
>
> Minor issue: wouldn't it make for easier reading to have less code in
> the 'if' part of if { ... } else { ... } clause, and more code in 'else'
> part.
I was just thinking about that. It also makes the conditional more obvious.
> Perhaps it would be worth adding comment about git_remotes:
>
> @@ -5599,6 +5740,7 @@ sub git_heads {
> git_footer_html();
> }
>
> +# dual lived: used both for single remote view, and for list of all remotes
Good idea.
> Though I am not sure if it is good public API. Perhaps it is...
The alternative would be to have git_remote to handle the single
remote case, and possibly even have the action name be 'remote' rather
than 'remotes' in that case.
>> - if (defined $remote) {
>> - # only display the heads in a given remote
>> - @remotelist = map {
>> - my $ref = $_ ;
>> - $ref->{'name'} =~ s!^$remote/!!;
>> - $ref
>> - } git_get_heads_list(undef, "remotes/$remote");
>> - } else {
>> - @remotelist = git_get_heads_list(undef, 'remotes');
>> + if (keys(%$remotedata) == 0) {
>
> You can write simply
>
> + unless (%$remotedata) {
>
> From perldata(1): "If you evaluate a hash in scalar context, it returns
> false if the hash is empty."
Right.
>> - if (@remotelist) {
>> - git_heads_body(\@remotelist, $head);
>> + git_remotes_body($remotedata, undef $head);
>> }
>
> You are missing comma after 'undef' here
Subtle bug. Thanks.
--
Giuseppe "Oblomov" Bilotta
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html