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

From: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
Date: Nov 15, 2008, 10:04 UTC
Message-ID: <cb7bb73a0811150204v15463275sf63098b819c6d259@mail.gmail.com>
In-Reply-To: <200811150059.14515.jnareb@gmail.com>

On Sat, Nov 15, 2008 at 12:59 AM, Jakub Narebski <jnareb@gmail.com> wrote:

Show 24 quoted lines

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

Show 10 quoted lines

>> +
>> +     # 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