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

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

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

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

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