Re: [PATCH 4/5] gitweb: Add a feature for adding more branch refs
On Wed, 2013-12-04 at 19:06 +0100, Jakub Narębski wrote:
Show 48 quoted lines
> 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.
Show 40 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.
>
> > ++
> > +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.
Show 11 quoted lines
>
> > ++
> > +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
Krzesimir Nowak
