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

[Giuseppe Bilotta <giuseppe.bilotta@gmail.com>]

On Thu, Nov 4, 2010 at 11:41 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> On Wed, 3 Nov 2010, Giuseppe Bilotta wrote:
>>
>> 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.

That's a good point.

> 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)?

I will look into that.

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

Either solution is fine, but it would require grabbing all the remote
heads. The real issue here is, I think understanding what is the
purpose of limiting in gitweb. Is it to reduce runtime? is it to
reduce clutter on the screen? In the first case, the limiting should
be done as early as possible (i.e. during the git call that retrieves
the data); in the latter case, is it _really_ needed at all?


-- 
Giuseppe "Oblomov" Bilotta