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

[Junio C Hamano <gitster@pobox.com>]

Krzesimir Nowak <krzesimir@endocode.com> writes:

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

The point is that "my ($val) = git_get_project_config('name')" calls
the sub in the list context like everybody else, which would be more
robust, if you want to be prepared for somebody else's change to the
implementation in the future, I think.

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

Yes, changing the calling site to use of config_to_multi() around
(see the handling of 'ctag' for an example) and then concatenate the
result of splitting each returned element would be one way to do
this.

Jakub may have had in mind to teach git_get_project_config() to
return a list; because existing callers call the sub in the list
context, they will not get surprising result---even though they may
only use the first one and discard the rest.

Which might not be a bad thing in the longer term, but I think it is
outside the scope of this particular topic, but in order to prepare
for that kind of internal API enhancement, it would still help to
make sure that this new caller calls the sub in the list context
like others.