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

[Jakub Narebski <jnareb@gmail.com>]

On Wed, 3 Nov 2010, Giuseppe Bilotta wrote:
> 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)?

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