Re: [PATCHv3 1/4] gitweb: introduce remote_heads feature

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

On Sun, Nov 16, 2008 at 6:16 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
>
>> With this feature enabled, remotes are retrieved (and displayed)
>> when getting (and displaying) the heads list.
>
> Wouldn't it be easier to read if you just said: "Include 'remotes' in the
> heads_list", because:

[snip]

> I am also suggesting to drop "With this feature enabled"; I do not think
> of a case where somebody runs gitweb on a repository with refs/remotes and
> does not want to show them.

[snip]

> When proofreading what you've written, it is usually a good idea to read
> it without anything you wrote in parentheses once, and then re-read it
> with parentheses removed (but the stuff in your parentheses kept), and
> compare which one you like better.  More often than not, you'd find that
> either parenthesized parts are unnecessary, or they are important enough
> that you shouldn't put them in parentheses.

Good points, I'll rewrite the commit messagge following the suggestions.

>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
>> index b0d00ea..e1f81f6 100755
>> --- a/gitweb/gitweb.perl
>> +++ b/gitweb/gitweb.perl
>> @@ -329,6 +329,18 @@ our %feature = (
>> ...
>> @@ -410,6 +422,18 @@ sub feature_pickaxe {
>> ...
>> +sub feature_remote_heads {
>> ...
>> +}
>
> When would somebody want to disable this?  Please explain; I'd like to
> understand the motivation behind it.
>
> One argument for making this feature optional I can think of is to retain
> backward compatibility because we didn't show them before, but I would say
> that is a weak argument.  Before release 1.5.0 made the separate remotes
> layout the default, everything was in refs/heads/, so you could even argue
> that this "fixes" the gitweb bug introduced in that release that stopped
> showing the branches you copied from elsewhere.

For example, because the only remote for the tree is the author's own
private tree, or because the only remotes are the mirrors on gitorious
or github. In both cases, there would be no reaso to waste resources,
bandwidth and screen estate loading and displaying the remote
references.

OTOH, it might make sense to make remote_heads enabled by default (and
overridable).

>> @@ -2660,10 +2684,12 @@ sub git_get_heads_list {
>>       my $limit = shift;
>>       my @headslist;
>>
>> +     my $remote_heads = gitweb_check_feature('remote_heads');
>> +
>>       open my $fd, '-|', git_cmd(), 'for-each-ref',
>>               ($limit ? '--count='.($limit+1) : ()), '--sort=-committerdate',
>>               '--format=%(objectname) %(refname) %(subject)%00%(committer)',
>> -             'refs/heads'
>> +             'refs/heads', ( $remote_heads ? 'refs/remotes' : '')
>>               or return;
>>       while (my $line = <$fd>) {
>>               my %ref_item;
>
> Imagine a later version of git may introduce 'refs/frotz/nitfol' namespace
> hierarchy that is commonly known as the 'xyzzy class' and is also useful
> to show.  Wouldn't it be easier to update gitweb to match such a change if
> this part of the code were written like this?
>
>        my %head_class = ('refs/heads' => 'head');
>        $head_class{'refs/remotes'} = 'remote'
>                if ( this feature is used );
>        $head_class{'refs/frotz/nitfol'} = 'xyzzy'
>                if ( the xyzzy class is used);
>        open my $fd, ... (keys %head_class);
>
>> @@ -2674,8 +2700,9 @@ sub git_get_heads_list {
>>               my ($committer, $epoch, $tz) =
>>                       ($committerinfo =~ /^(.*) ([0-9]+) (.*)$/);
>>               $ref_item{'fullname'}  = $name;
>> -             $name =~ s!^refs/heads/!!;
>> +             $name =~ s!^refs/(head|remote)s/!!;
>>
>> +             $ref_item{'class'} = $1;
>
> And then outside the loop, you'd prepare:
>
>        my $headpat = join('|', map { quotemeta($_) } keys %head_class);
>
> and inside the loop you would do:
>
>        if ($name =~ s{^($headpat)/}{}) {
>                $ref_item{'class'} = $head_class{$1};
>                ...
>
> Only one place to configure the list of classes, and make everybody use
> that list instead of hardcoding the assumption that there are two and only
> two kinds of things "head" vs "remote".

Ah, good point. This change, and the one about extending
git_get_heads_list, could also be turned into prelliminary patches to
the actual remote_heads feature.

-- 
Giuseppe "Oblomov" Bilotta