Re: [PATCH 4/5] gitweb: Add a feature for adding more branch refs

From: Jakub Narębski <jnareb@gmail.com>
Date: Dec 4, 2013, 18:06 UTC
Message-ID: <CANQwDwe+a2P0Jxqw0k7sHWv3exdb4k+NU3jL3ogR-rcetd82TQ@mail.gmail.com>
In-Reply-To: <1386164583-14109-5-git-send-email-krzesimir@endocode.com>

On Wed, Dec 4, 2013 at 2:43 PM, Krzesimir Nowak <krzesimir@endocode.com> wrote:

Show 8 quoted lines

>
> Allow extra-branch-refs feature to tell gitweb to show refs from
> additional hierarchies in addition to branches in the list-of-branches
> view.
>
> Signed-off-by: Krzesimir Nowak <krzesimir@endocode.com>
> Reviewed-by: Junio C Hamano <gitster@pobox.com>
> Reviewed-by: Jakub Narębski <jnareb@gmail.com>

This version is Helped-by (maybe), but not (yet!) Reviewed-by.

Show 28 quoted lines

> ---
>  Documentation/gitweb.conf.txt | 37 +++++++++++++++++++
>  gitweb/gitweb.perl            | 85 +++++++++++++++++++++++++++++++++++++------
>  2 files changed, 110 insertions(+), 12 deletions(-)
>
> diff --git a/Documentation/gitweb.conf.txt b/Documentation/gitweb.conf.txt
> index e2113d9..5a77452 100644
> --- a/Documentation/gitweb.conf.txt
> +++ b/Documentation/gitweb.conf.txt
> @@ -849,6 +849,43 @@ time zones in the form of "+/-HHMM", such as "+0200".
>  +
>  Project specific override is not supported.
>
> +extra-branch-refs::
> +       List of additional directories under "refs" which are going to
> +       be used as branch refs. For example if you have a gerrit setup
> +       where all branches under refs/heads/ are official,
> +       push-after-review ones and branches under refs/sandbox/,
> +       refs/wip and refs/other are user ones where permissions are
> +       much wider, then you might want to set this variable as
> +       follows:
> ++
> +--------------------------------------------------------------------------------
> +$feature{'extra-branch-refs'}{'default'} =
> +       ['sandbox', 'wip', 'other'];
> +--------------------------------------------------------------------------------
> ++
> +If overriding was enabled then this feature can be configured on a

s/was/is/;

Perhaps it would better read as

    This feature can be configured on per-repository basis after setting
    $feature{'extra-branch-refs'}{'override'} to true, via repository's
    `gitweb.extraBranchRefs` ...

Show 8 quoted lines

> +per-repository basis via repository's `gitweb.extrabranchrefs`
> +configuration variable, which contains a space separated list of
> +refs. An example:
> ++
> +--------------------------------------------------------------------------------
> +[gitweb]
> +       extrabranchrefs = sandbox wip other
> +--------------------------------------------------------------------------------

O.K.

Show 10 quoted lines

> ++
> +The gitweb.extrabranchrefs is actually a multi-valued configuration
> +variable, so following example is also correct and the result is the
> +same as of the snippet above:
> ++
> +--------------------------------------------------------------------------------
> +[gitweb]
> +       extrabranchrefs = sandbox
> +       extrabranchrefs = wip other
> +--------------------------------------------------------------------------------

I think this part should be better left for a separate patch. There is important difference between single-valued and multi-valued configuration variable: with single-valued later occurrences override earlier ones, which includes settings in more specific config file (e.g. per-repository) overriding setting in more general one (e.g. per-user or system-wide).

With multi-valued we won't be able to override earlier / more generic settings... well, unless we add support for no-value, or empty-value as clearer, i.e.

  [gitweb]
           extrabranchrefs = sandbox
           extrabranchrefs
 # or    extrabranchrefs =
           extrabranchrefs = wip other

resulting in ('wip', 'other').

> ++
> +It is an error to specify a ref that does not pass "git check-ref-format"
> +scrutiny. Duplicated values are filtered.
> +

Hmmm... 'snapshot' feature ignores invalid values, but in this case formerly valid compression schemes might get invalid via tightening %known_snapshot_formats, and we don't want existing config getting suddenly invalid.

[cut]

Nice!

-- 
Jakub Narebski