Re: [PATCH v2 05/11] gitweb: git_split_heads_body function.

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

On Sat, Nov 15, 2008 at 12:59 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> On Thu, 13 Nov 2008, Giuseppe Bilotta wrote:
>
>> The purpose of this function is to split a headlist into groups
>> determined by the leading part of the refname, and call git_heads_body()
>> on each group.
>
> What is the reason of this patch? Is it to split remote-tracking
> branches ('remotes' references) into remotes, and group them by
> the remote repository name?
>
> If it is true, then first: you should have wrote the _reason_ behind
> this patch and not only what it does in this commit message. And use
> better summary (commit title / subject of this patch).
>
> Second, this patch wouldn't do what you want from it if there are
> remotes with '/' in name.  I for example use "gsoc2008/gitweb-caching"
> for Lea Wiemann repository with her GSoC 2008 work on adding caching
> to gitweb.  Because there are many ways to specify remotes due to
> backwards compatibility (and simplicity, as some for example prefer
> old 'branches/' way to specify remotes), namely config, files under
> '.git/remotes', and (from Cogito) files in '.git/branches', you would
> have to either reimplement/reuse parts of git-remote (there is old Perl
> implementation in contrib/examples), or use "git remote" or
> "git remote -v" command output[1].

The initially intended purpose for this patch was to group remote
heads by remotes, but an interesting side-effect of doing it this way
was that it allowed to group _local_ heads too, by using the
stuff/morestuff syntax. For example, I could group gitweb/pathinfo and
gitweb/allheads together (although I disabled this grouping for local
heads in the patchset).

However, as you remark, the current patch fails to achieve even its
intended purpose, so it looks like going the 'git remote' way would be
the right way to find at least the grouping keys: this has the benefit
of allowing us to retrieve the remote URL as well by using 'git remote
-v', although it has the underside of require one additional git call.

It would also probably be a good idea to separate the actual head
grouping from the display of the grouped head lists. I wonder if Perl
has a 'tree' data structure that could be used to store the grouped
head lists ...

Ah yes, the code in this patch I was never actually really satisfied
with, hopefully I can rewrite it more sensibly with the adittional
experience I've accumulated this year.

>> +
>> +     # Split @$headlist into a hash of lists
>> +     map {
>> +             my %ref = %$_;
>> +             $ref{'hname'} = $ref{'name'};
>> +             if ($ref{'name'} =~ /\//) {
>> +                     $ref{'name'} =~ s!^([^/]+)/!!;
>
> As I said, this would fail on for example "gsoc2008/gitweb-caching"
> remote...

Would you say that in this case we want 'gsoc2008/gitweb-caching' as
the group head, or would you rather have nested groups [gsoc2008
[gitweb-caching [branches in gsoc2008/gitweb-caching] [etc]] ? I must
say that I think the latter would be quite interesting, but I _am_ a
little afraid we could turn up with way too much nested groups ...


-- 
Giuseppe "Oblomov" Bilotta