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

From: Jakub Narebski <jnareb@gmail.com>
Date: Nov 16, 2008, 01:13 UTC
Message-ID: <200811160213.43343.jnareb@gmail.com>
In-Reply-To: <cb7bb73a0811150204v15463275sf63098b819c6d259@mail.gmail.com>

On Sat, 15 Nov 2008, Giuseppe Bilotta wrote:

Show 32 quoted lines

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

I'm not sure if it would be that useful. How many people have _many_ stuff/morestuff branches for some values of stuff/? The convention of <initials>/<topic> of topic branches in git.git doesn't usually lead to many branches with the same <initials>/ prefix.

Show 6 quoted lines

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

Now I thought about it a bit, I think your solution has merit.

Splitting by remotes is hard and difficult to do right, especially if you consider than 'remote' prefix doesn't need to have anything in common with names (common prefix) of refs/remotes/* remote-tracking branches used. It is fairly easy to do it right in common case, but hard in uncommon one.

So perhaps the idea of using first dirname as a kind of category for remotes is a good idea. And usually it would be also remote name.

But it really needs explanation in commit message... and quite a bit of commit squashing.

Show 5 quoted lines

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

Hash of hashes (well, hash references), see perldsc(1)?

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

Code... well, perhaps... commit messages also matter.

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

Now I think that having [gsoc2008] subgroup here might be a good thing...

-- 
Jakub Narebski
Poland