On Tue, 2 Nov 2010, Giuseppe Bilotta wrote:
> 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 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
--
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