From: Junio C Hamano <gitster@pobox.com>
Date: Tue, 03 Dec 2013 20:15:59 GMT
Subject: Re: [PATCH 2/3] gitweb: Add a feature for adding more branch refs
Message-ID: <xmqqr49tn11c.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1386082603-8404-3-git-send-email-krzesimir@endocode.com>

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).  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;
> +}