Re: [PATCHv6 10/10] gitweb: group remote heads by remote
- From
Jakub Narebski <jnareb@gmail.com>
- Date
- Nov 4, 2010, 10:41 UTC
- Message-ID
- <201011041141.58334.jnareb@gmail.com>
- In-Reply-To
- <AANLkTi=8Qz3bFCc1qocpOqsCdSWtwUHQDiwkS7H2ypad@mail.gmail.com>
On Wed, 3 Nov 2010, Giuseppe Bilotta wrote:
Show 10 quoted lines
> On Wed, Nov 3, 2010 at 12:58 AM, Jakub Narebski <jnareb@gmail.com> wrote: >> On Tue, 2 Nov 2010, Giuseppe Bilotta wrote: >>> 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. > > I think that in most cases there won't be any need for limiting. > Public cases of lots of remotes with lots of branches are, I suspect, > rare.
It's not about _public_ cases; I think that it is in very rare cases that public repository would want to display remotes and remote-tracking branches.
I think remote_heads feature is more important for _local_ use, for example browsing one own repository using git-instaweb. In such cases number of remotes and of remote-tracking branches might be large (I have 11 remotes, not all active, and 58 remote-tracking branches).
BTW. would next version of this series include patch to git-instaweb enabling 'remote_heads' feature for it (gitweb_conf function)?
Show 11 quoted lines
>>>> +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 ];
>>>> + }
>>>> +}[...]
Show 14 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. > > I'm still not sure I follow. git_get_heads_list only gets called once,
Ah, that's all right then. I was worring that it is called more than once in one request by gitweb.
Show 8 quoted lines
> with all the remotes/<remotename> pathspecs as a single array > argument. This _does_ mean that when the total number of remote heads > is greater than the limit some remotes will not display complete > information in summary view. The real issue here is, I think, that > there is no trivial way to tell which remotes have incomplete > information and which don't, meaning that in the subsequent > git_remote_block calls we'll have no way to provide visual feedback > (the ellipsis) when some heads are missing.
Errr... shouldn't we leave limiting number of heads to fill_remote_heads, which can do limiting per remote (with each remote having up to $limit remote-tracking branches / remote heads), instead of having git_get_heads_list do it?
Something like this:
+sub fill_remote_heads {
+ my ($remotes, $limit) = @_;
+
+ my @heads = map { "remotes/$_" } keys %$remotes;
+ my @remoteheads = git_get_heads_list(undef, @heads);
+ foreach my $remote (keys %$remotes) {
+ $remotes->{$remote}{'heads'} =
+ [ grep { $_->{'name'} =~ s!^$remote/!! } @remoteheads ];
+ $remotes->{$remote}{'heads'} =
+ [ @{$remotes->{$remote}{'heads'}}[0..$limit-1] ]
+ if (@{$remotes->{$remote}{'heads'}} > $limit);
+ }
+}Though perhaps it will be more clear with if as statement, not as modifier:
+sub fill_remote_heads {
+ my ($remotes, $limit) = @_;
+
+ my @heads = map { "remotes/$_" } keys %$remotes;
+ my @remoteheads = git_get_heads_list(undef, @heads);
+ foreach my $remote (keys %$remotes) {
+ $remotes->{$remote}{'heads'} =
+ [ grep { $_->{'name'} =~ s!^$remote/!! } @remoteheads ];
+ if (@{$remotes->{$remote}{'heads'}} > $limit) {
+ $remotes->{$remote}{'heads'} =
+ [ @{$remotes->{$remote}{'heads'}}[0..$limit-1] ]
+ }
+ }
+}Show 12 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. > > I hope you don't mind if I opt to leave these refinements for later ;-)
Not a problem. For backward compatibility we would have to accept 'remotes/<remote>' in addition to more correct 'remote/<remote>', but this is not a problem.
-- Jakub Narebski Poland