On Wed, 3 Nov 2010, Giuseppe Bilotta wrote:
> On Wed, Nov 3, 2010 at 12:58 AM, Jakub Narebski <jnareb@xxxxxxxxx> 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)?
>>>> +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?
>>
>> 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.
> 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] ]
+ }
+ }
+}
>>>> 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
--
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