On Tue, Dec 3, 2013 at 9:15 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Krzesimir Nowak <krzesimir@xxxxxxxxxxxx> 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.
> * 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).
>> + if ($values) {
>> + @branch_refs = split /\s+/, $values;
>> + }
>> +
>> + return @branch_refs;
>> +}
--
Jakub Narebski
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html