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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 4, 2013, 17:57 UTC
Message-ID: <xmqqli00lcs4.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1386161398.2173.9.camel@localhost.localdomain>

Krzesimir Nowak <krzesimir@endocode.com> writes:

Show 32 quoted lines

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

Show 17 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).
>
> 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.