Re: [PATCHv6 10/10] gitweb: group remote heads by remote
- From
Jakub Narebski <jnareb@gmail.com>
- Date
- Nov 2, 2010, 23:58 UTC
- Message-ID
- <201011030058.53366.jnareb@gmail.com>
- In-Reply-To
- <AANLkTinyi22OczYuD4urJKkfh8AzyuZoTzvzAvWa1Bo4@mail.gmail.com>
On Tue, 2 Nov 2010, Giuseppe Bilotta wrote:
Show 9 quoted lines
> 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.
Show 21 quoted lines
>>> +# >>> +# 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?
Show 9 quoted lines
>>> + >>> +# 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.
Show 29 quoted lines
>>
>>> +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).
Show 7 quoted lines
>> 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.
Show 21 quoted lines
>>> +
>>> + 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.
Show 5 quoted lines
>> 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