Re: [PATCHv6 10/10] gitweb: group remote heads by remote

[Jakub Narebski <jnareb@gmail.com>]

On Tue, 2 Nov 2010, Giuseppe Bilotta wrote:
> 2010/10/27 Jakub Narebski <jnareb@gmail.com>:
>> 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 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-)

I think that untill this feature gets into gitweb we can only guess
as to what kind of limiting would look and behave best on RL cases.
 
>>> +#
>>> +# 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. 

True, we have to balance complexity in generating function vs. complexity
in display code.

> I'll take the numerical limiting off the routine.

You meant here limiting number of remotes returned (except of case of
limiting to single remote), isn't it?

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

We have fill_from_file_info, fill_project_list_info; I think 
fill_remote_heads would be a good name.
 
>>
>>> +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 ];
>> +       }
>> +}

I now think that passing limit to fill_remote_heads (like you have) might
be a good solution (contrary to what I wrote earlier).

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

I meant here passing @remoteheads or \@remoteheads, i.e. result of call
to git_get_heads_list, to fill_remote_heads (as one of parameters).  
I was worrying, perhaps unnecessarily, about calling git_get_heads_list
more than once... but I guess that we did it anyway.

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

It is better.
 
>> 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.

We might want to have 'remote' as alias to 'remotes' anyway.

Though what you propose, having separately 'remotes' for list of all
remotes, and 'remote' + name of remote, might be a good idea.
 

Thank you on diligently working on this feature.

-- 
Jakub Narebski
Poland