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