Re: [PATCHv6 10/10] gitweb: group remote heads by remote
- From
Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
- Date
- Nov 3, 2010, 07:49 UTC
- Message-ID
- <AANLkTi=8Qz3bFCc1qocpOqsCdSWtwUHQDiwkS7H2ypad@mail.gmail.com>
- In-Reply-To
- <201011030058.53366.jnareb@gmail.com>
On Wed, Nov 3, 2010 at 12:58 AM, Jakub Narebski <jnareb@gmail.com> wrote:
Show 5 quoted lines
> 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
Show 14 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 ];
>>> + }
>>> +}
>
> 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 ;-)
Show 12 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, 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.
Show 5 quoted lines
>>> I'm not sure about naming this variable $urls... >> >> I'm open to suggestions. $urls_table ? > > It is better.
Can do.
Show 10 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 ;-)
> Thank you on diligently working on this feature.
Thank you for your review.
-- Giuseppe "Oblomov" Bilotta