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

From: Junio C Hamano <[email-masked]>
Date: Tue, 03 Dec 2013 12:15:59 -0800

Krzesimir Nowak <[email-masked]> 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).  Shoudln't this
  part do the same?

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

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

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