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

Krzesimir Nowak [off-list ref] writes:

On Tue, 2013-12-03 at 21:38 +0100, Jakub Narębski wrote:

quoted

On Tue, Dec 3, 2013 at 9:15 PM, Junio C Hamano [off-list ref] wrote:

quoted

Krzesimir Nowak [off-list ref] writes:

quoted

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

quoted

quoted

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