Re: [PATCH 2/3] gitweb: Add a feature for adding more branch refs

From: Jakub Narębski <jnareb@gmail.com>
Date: Dec 3, 2013, 20:38 UTC
Message-ID: <CANQwDwcpkbbfND9MmB9wGcL7mrhV_Mxb1amvGPgWeR344_fcfw@mail.gmail.com>
In-Reply-To: <xmqqr49tn11c.fsf@gitster.dls.corp.google.com>

On Tue, Dec 3, 2013 at 9:15 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 20 quoted lines

> Krzesimir Nowak <krzesimir@endocode.com> writes:
>
>> @@ -626,6 +640,17 @@ sub feature_avatar {
>>       return @val ? @val : @_;
>>  }
>>
>> +sub feature_extra_branch_refs {
>> +     my (@branch_refs) = @_;
>> +     my $values = git_get_project_config('extra_branch_refs');
>
> Hmph.  Three points.
>
> * Almost all callers of this function use
>
>     my ($val) = git_get_project_config(...);
>     my @val = git_get_project_config(...);
>
>   to expect that the function returns a list of things (and grab the
>   first one among them, not the length of the list).  Shouldn't this
>   part do the same?

Right. feature_snapshot() has here

    my (@fmts) = @_;
    my ($val) = git_get_project_config('snapshot');

...though git_get_project_config returns scalar.

Show 8 quoted lines

> * Wouldn't this be a good candidate for a multi-valued configuration
>   variable, e.g. shouldn't this
>
>         [gitweb]
>                 extraBranchRefs = wip
>                 extraBranchRefs = sandbox other
>
>   be parsed as a three-item list, qw(wip sandbox other)?

This would require changes in git_get_project_config(), which would need to be able to deal with multi-valued result (it caches these results, so we pay only one cost of `git config` call).

> * I think the $key parameter to git_get_project_config() eventually
>   is used to look up a key in the Git-style configuration file, and
>   the 'words_with_underscore' goes against our convention (cf. see
>   how 'show-sizes' feature is spelled as 'showsizes' there).

Errr... actually git_get_project_config() strips '_' from $key, though not for some strange reason '-'.

BTW. though it is 'showsizes' in code, it usually is 'showSizes' in config file (camelCase convention, lowercased by git-config).

Show 6 quoted lines

>> +     if ($values) {
>> +             @branch_refs = split /\s+/, $values;
>> +     }
>> +
>> +     return @branch_refs;
>> +}

-- 
Jakub Narebski