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

[Krzesimir Nowak <krzesimir@endocode.com>]

On Tue, 2013-12-03 at 21:38 +0100, Jakub Narębski wrote:
> On Tue, Dec 3, 2013 at 9:15 PM, Junio C Hamano <gitster@pobox.com> wrote:
> > 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.

So what's the point of it? 'my @val = git_get_project_config ()' just
creates an array with one element.

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

Hm, actually not at all. Now, if I have a setup like Junio wrote the
git_get_project_config just returns an array ref. So modifying the
feature_extra_branch_refs to handle the returned value as either simple
scalar or array reference should be enough.

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

Oi, that was an omission from my side - at first I had that git config
setting with underscores. I removed them when I noticed that underscores
are not used there. Apparently I missed that one.

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

-- 
Krzesimir Nowak
Software Developer
Endocode AG

krzesimir@endocode.com

------
Endocode AG, Johannisstraße 20, 10117 Berlin
info@endocode.com | www.endocode.com

Vorstandsvorsitzender: Mirko Boehm
Vorstände: Dr. Karl Beecher, Chris Kühl, Sebastian Sucker
Aufsichtsratsvorsitzende: Jennifer Beecher

Registergericht: Amtsgericht Charlottenburg - HRB 150748 B