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

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

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.

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

Yes.

> We have fill_from_file_info, fill_project_list_info; I think
> fill_remote_heads would be a good name.

That's what will be in the next rehash of the patchset

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

I hadn't taken it out anyway, so that's good ;-)

>>> 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,
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.

>>> I'm not sure about naming this variable $urls...
>>
>> I'm open to suggestions. $urls_table ?
>
> It is better.

Can do.

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

> Thank you on diligently working on this feature.

Thank you for your review.

-- 
Giuseppe "Oblomov" Bilotta