On Sun, 19 Sep 2010, Giuseppe Bilotta wrote:
> On Fri, Sep 17, 2010 at 6:54 PM, Jakub Narebski <jnareb@xxxxxxxxx> wrote:
>> On Thu, 16 Sep 2010, Giuseppe Bilotta wrote:
>>
>>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
>>> index 92551e4..66b5400 100755
>>> --- a/gitweb/gitweb.perl
>>> +++ b/gitweb/gitweb.perl
>>> @@ -2758,6 +2758,16 @@ sub git_get_last_activity {
>>> return (undef, undef);
>>> }
>>>
>>> +sub git_get_remotes {
>
> [snip]
>
>>> + my @remoteheads = git_get_heads_list($limit, 'remotes');
>>> + return (\@remotes, \@remoteheads);
>>
>> Why do you want for git_get_remotes() to also return remote-tracking
>> branches (refs/remotes/)? Those are separate issues, and I think it
>> would be better API for git_get_remotes() to provide only list of
>> remotes, i.e.
>>
>> + return @remotes;
>>
>> Especially that we might want in the summary view to only list remotes,
>> without listing remote-tracking branches.
>>
>> That would require more changes to the code.
>
> This is kind of the main issue with this patch. What do we want to do
> with the remotes list in summary view and the remotes view? We
> basically have three possibilities:
>
> (1) we can make the remotes list in summary view be a 'reduced
> remotes' view: this would make it behave the most similarly to the
> other components of summary view
> (2) we can make the remotes list be much more stripped down, by only
> listing the remotes and possibly some summarizing property such as the
> number of branches in it or when it was last updated
> (3) we can make the remotes list be just a copy of the full remotes view.
>
> The third option is surely the easiest to implement. The second option
> with _only_ a list of remotes (no extra info) is also very easy to
> implement _and_ fast to render. The second option with extra info, or
> the first option, on the other hand, require the retrieval of some
> additional data which, maybe due to my limited knowledge of git,
> essentially means retrieving _all_ the remote heads and then doing the
> filtering in gitweb. But once we're getting all the information, why
> not display it all? isn't it faster to just display all of it, in
> which case we go back to option 3?
When thinking about how display information about remotes and
remote-tracking branches in 'summary' view, we have to consider that
this view is meant to be what it says: a *summary*. That is why both
the 'heads' and the 'tags' section displays only 15 items.
Without grouping remote heads the natural solution was to simply repeat
what we used for 'heads' subsection, namely limit displaying to 15
remote-tracking branches in 'remotes' subsection of the 'summary' view.
With grouping it is more complicated. We can limit number of remote
head *per remote*, we can limit number of remotes... but what makes
IMVHO least sense is limit number of remote heads *then* do grouping.
The solution (1) i.e. limiting number of remote heads per remote, with
or without limiting number of remotes behaves, as you wrote, most
similarly to other components of 'summary' view. On the other hand
with large number of remotes, and large number of remote heads in those
remotes it might be too large for a *summary* view.
The solution (3) i.e. displaying only list of remotes (perhaps limited
to 15 remotes) is simple and fast to render. On the other hand it offers
least information and might be too little in the case of single remote.
>
> If we go with option 3, it does make sense to get all remote names and
> all remote branches at once, and thus to make the git_get_remotes()
> call do all of the work.
Not if subroutine is called git_get_remotes(), IMVHO. Perhaps
git_group_remotes()?
>>> @@ -4991,6 +5001,19 @@ sub git_heads_body {
>>> print "</table>\n";
>>> }
>>>
>>> +sub git_remotes_body {
>>> + my ($remotedata, $head) = @_;
>>> + my @remotenames = @{$remotedata->[0]};
>>> + my @allheads = @{$remotedata->[1]};
>>
>> Why not
>>
>> + my ($remotenames, $allheads, $head) = @_;
>>
>> Beside, isn't it $remote_heads and not $allheads?
>
> I think it's a leftover name choice from the first version of the
> patch. Can change.
O.K.
>>> + foreach my $remote (@remotenames) {
>>
>> It would be then
>>
>> + foreach my $remote (@$remotenames) {
>>
>>> + my @remoteheads = grep { $_->{'name'} =~ s!^\Q$remote\E/!! } @allheads;
>>
>> Should we display remote even if it doesn't have any remote heads
>> associated with it?
>>
>> By the way, it would be place where we could limit number of
>> remote-tracking branches displayed in each remote block.
>
> But does it make sense to reduce the number of displayed branches
> after we got the information about all of them? I think it depends on
> what summary view is intended to do exactly.
Right. But I think limit then group is not a very good choice from
all other available.
>>> + git_begin_group("remotes", $remote, "remotes/$remote",$remote);
>>> + git_heads_body(\@remoteheads, $head);
>>> + git_end_group();
>>
>> This would have to be modified with change to git_begin_group() /
>> / git_end_group().
>
> Of course.
>
>> BTW isn't it premature generalization? It is only place AFAIKS that
>> uses git_*_group() subroutines.
>
> It's the only current use but I believe that, since it's factored out
> now already and since it may be used in other views too (think:
> grouping heads or tags by prefix) it might make sense to keep it this
> way.
If it could be used for other blocks (like 'readme', or 'heads',
or 'tags') itself it would be even better.
>>> + }
>>> +
>>> +}
>>> +
>>> sub git_search_grep_body {
>>> my ($commitlist, $from, $to, $extra) = @_;
>>> $from = 0 unless defined $from;
>>> @@ -5137,7 +5160,7 @@ sub git_summary {
>>> # there are more ...
>>> my @taglist = git_get_tags_list(16);
>>> my @headlist = git_get_heads_list(16, 'heads');
>>> - my @remotelist = $remote_heads ? git_get_heads_list(16, 'remotes') : ();
>>> + my @remotelist = $remote_heads ? git_get_remotes(16) : ();
>>
>> No change of git_get_remotes() does only one thing: returning list
>> of remotes.
>
> See above about what should git_get_remotes() do. Even better, I was
> thinking about git_get_remotes() returning a hash (mapping remote
> names to the heads from that remote)
Wouldn't it be then named git_group_remotes() or something like that?
If remote doesn't have remote-tracking branches associated with it
(e.g. push-only remote, or remote predating separate remotes layout)
the value would be undef for given remote as key in hash.
> So the big question (which essentially determines the functionality
> provided by this last patch in the set) is: what do we want to do in
> summary view?
--
Jakub Narebski
Poland
--
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