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

[Krzesimir Nowak <krzesimir@endocode.com>]

On Wed, 2013-12-04 at 19:06 +0100, Jakub Narębski wrote:
> On Wed, Dec 4, 2013 at 2:43 PM, Krzesimir Nowak <krzesimir@endocode.com> wrote:
> >
> > 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.
> 
> > ---
> >  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` ...

I see that you also insist on using camelCase for config variables. I
will rephrase it.

> 
> > +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.
> 
> > ++
> > +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').

That point in this example is a bit moot IMO - if you don't want sandbox
ref to appear in list of branches view then just delete the offending
line.

I also made a small experiment. In per-instance config I have
$feature{'extra-branch-refs'}{'default'} = ['wip'];
$feature{'extra-branch-refs'}{'override'} = 1;

In per-repository config I have:
[gitweb]
	extrabranchrefs = sandbox
	extrabranchrefs = other

List of branches view shows only branches from sandbox and other. So
clearly per-repository config overrides per-instance config.

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

So, should I just ignore the invalid refs or treat them as errors?

> [cut]
> 
> Nice!
> 

-- 
Krzesimir Nowak
Software Developer
Endocode AG

krzesimir@endocode.com

------
Endocode AG, Johannisstraße 20, 10117 Berlin
info@endocode.com | www.endocode.com

Vorstandsvorsitzender: Mirko Boehm
Vorstände: Dr. Karl Beecher, Chris Kühl, Sebastian Sucker
Aufsichtsratsvorsitzende: Jennifer Beecher

Registergericht: Amtsgericht Charlottenburg - HRB 150748 B