[PATCH v3] gitweb: Add an option for adding more branch refs

[PATCH v3] gitweb: Add an option for adding more branch refs

[Krzesimir Nowak]

Allow @additional_branch_refs configuration variable 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>
---
 Documentation/gitweb.conf.txt | 13 ++++++++
 gitweb/gitweb.perl            | 75 +++++++++++++++++++++++++++++++++----------
 2 files changed, 71 insertions(+), 17 deletions(-)

diff --git a/Documentation/gitweb.conf.txt b/Documentation/gitweb.conf.txt
index e2113d9..cd1a945 100644
--- a/Documentation/gitweb.conf.txt
+++ b/Documentation/gitweb.conf.txt
@@ -549,6 +549,19 @@ This variable matters only when using persistent web environments that
 serve multiple requests using single gitweb instance, like mod_perl,
 FastCGI or Plackup.
 
+@additional_branch_refs::
+	List of additional directories under "refs" which are going to be used
+	as branch refs. You might want to set this variable 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, for example
++
+--------------------------------------------------------------------------------
+our @additional_branch_refs = ('sandbox', 'wip', 'other');
+--------------------------------------------------------------------------------
++
+It is an error to specify a ref that does not pass "git check-ref-format"
+scrutiny.
 
 Other variables
 ~~~~~~~~~~~~~~~
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 68c77f6..25e1d37 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -122,6 +122,10 @@ our $logo_label = "git homepage";
 # source of projects list
 our $projects_list = "++GITWEB_LIST++";
 
+# list of additional "directories" under "refs/" we want to display as
+# branches
+our @additional_branch_refs = ();
+
 # the width (in characters) of the projects list "Description" column
 our $projects_list_description_width = 25;
 
@@ -626,6 +630,10 @@ sub feature_avatar {
 	return @val ? @val : @_;
 }
 
+sub get_branch_refs {
+    return ('heads', @additional_branch_refs);
+}
+
 # checking HEAD file with -e is fragile if the repository was
 # initialized long time ago (i.e. symlink HEAD) and was pack-ref'ed
 # and then pruned.
@@ -680,6 +688,19 @@ sub read_config_file {
 	return;
 }
 
+# performs sanity checks on parts of configuration.
+sub config_sanity_check {
+	# check additional refs validity
+	my %unique_branch_refs = ();
+	for my $ref (@additional_branch_refs) {
+		die_error(500, "Invalid ref '$ref' in \@additional_branch_refs") unless (validate_ref($ref));
+		# 'heads' are added implicitly in get_branch_refs().
+		$unique_branch_refs{$ref} = 1 if ($ref ne 'heads');
+	}
+	@additional_branch_refs = sort keys %unique_branch_refs;
+	%unique_branch_refs = undef;
+}
+
 our ($GITWEB_CONFIG, $GITWEB_CONFIG_SYSTEM, $GITWEB_CONFIG_COMMON);
 sub evaluate_gitweb_config {
 	our $GITWEB_CONFIG = $ENV{'GITWEB_CONFIG'} || "++GITWEB_CONFIG++";
@@ -698,8 +719,11 @@ sub evaluate_gitweb_config {
 
 	# Use first config file that exists.  This means use the per-instance
 	# GITWEB_CONFIG if exists, otherwise use GITWEB_SYSTEM_CONFIG.
-	read_config_file($GITWEB_CONFIG) and return;
-	read_config_file($GITWEB_CONFIG_SYSTEM);
+	if (!read_config_file($GITWEB_CONFIG)) {
+		read_config_file($GITWEB_CONFIG_SYSTEM);
+	}
+
+	config_sanity_check();
 }
 
 # Get loadavg of system, to compare against $maxload.
@@ -1452,6 +1476,16 @@ sub validate_pathname {
 	return $input;
 }
 
+sub validate_ref {
+	my $input = shift || return undef;
+
+	# restrictions on ref name according to git-check-ref-format
+	if ($input =~ m!(/\.|\.\.|[\000-\040\177 ~^:?*\[]|/$)!) {
+		return undef;
+	}
+	return $input;
+}
+
 sub validate_refname {
 	my $input = shift || return undef;
 
@@ -1462,10 +1496,9 @@ sub validate_refname {
 	# it must be correct pathname
 	$input = validate_pathname($input)
 		or return undef;
-	# restrictions on ref name according to git-check-ref-format
-	if ($input =~ m!(/\.|\.\.|[\000-\040\177 ~^:?*\[]|/$)!) {
-		return undef;
-	}
+	# check git-check-ref-format restrictions
+	$input = validate_ref($input)
+		or return undef;
 	return $input;
 }
 
@@ -2515,6 +2548,7 @@ sub format_snapshot_links {
 sub get_feed_info {
 	my $format = shift || 'Atom';
 	my %res = (action => lc($format));
+	my $matched_ref = 0;
 
 	# feed links are possible only for project views
 	return unless (defined $project);
@@ -2522,12 +2556,17 @@ sub get_feed_info {
 	# or don't have specific feed yet (so they should use generic)
 	return if (!$action || $action =~ /^(?:tags|heads|forks|tag|search)$/x);
 
-	my $branch;
-	# branches refs uses 'refs/heads/' prefix (fullname) to differentiate
-	# from tag links; this also makes possible to detect branch links
-	if ((defined $hash_base && $hash_base =~ m!^refs/heads/(.*)$!) ||
-	    (defined $hash      && $hash      =~ m!^refs/heads/(.*)$!)) {
-		$branch = $1;
+	my $branch = undef;
+	# branches refs uses 'refs/' + $get_branch_refs()[x] + '/' prefix
+	# (fullname) to differentiate from tag links; this also makes
+	# possible to detect branch links
+	for my $ref (get_branch_refs()) {
+		if ((defined $hash_base && $hash_base =~ m!^refs/\Q$ref\E/(.*)$!) ||
+		    (defined $hash      && $hash      =~ m!^refs/\Q$ref\E/(.*)$!)) {
+			$branch = $1;
+			$matched_ref = $ref;
+			last;
+		}
 	}
 	# find log type for feed description (title)
 	my $type = 'log';
@@ -2540,7 +2579,7 @@ sub get_feed_info {
 	}
 
 	$res{-title} = $type;
-	$res{'hash'} = (defined $branch ? "refs/heads/$branch" : undef);
+	$res{'hash'} = (defined $branch ? "refs/$matched_ref/$branch" : undef);
 	$res{'file_name'} = $file_name;
 
 	return %res;
@@ -3193,7 +3232,7 @@ sub git_get_last_activity {
 	     '--format=%(committer)',
 	     '--sort=-committerdate',
 	     '--count=1',
-	     'refs/heads') or return;
+	     map { "refs/$_" } get_branch_refs ()) or return;
 	my $most_recent = <$fd>;
 	close $fd or return;
 	if (defined $most_recent &&
@@ -3644,7 +3683,7 @@ sub parse_from_to_diffinfo {
 
 sub git_get_heads_list {
 	my ($limit, @classes) = @_;
-	@classes = ('heads') unless @classes;
+	@classes = get_branch_refs() unless @classes;
 	my @patterns = map { "refs/$_" } @classes;
 	my @headslist;
 
@@ -3662,7 +3701,8 @@ sub git_get_heads_list {
 		my ($committer, $epoch, $tz) =
 			($committerinfo =~ /^(.*) ([0-9]+) (.*)$/);
 		$ref_item{'fullname'}  = $name;
-		$name =~ s!^refs/(?:head|remote)s/!!;
+		my $strip_refs = join '|', map { quotemeta } get_branch_refs();
+		$name =~ s!^refs/(?:$strip_refs|remotes)/!!;
 
 		$ref_item{'name'}  = $name;
 		$ref_item{'id'}    = $hash;
@@ -7179,7 +7219,8 @@ sub snapshot_name {
 		$ver = $1;
 	} else {
 		# branches and other need shortened SHA-1 hash
-		if ($hash =~ m!^refs/(?:heads|remotes)/(.*)$!) {
+		my $strip_refs = join '|', map { quotemeta } get_branch_refs();
+		if ($hash =~ m!^refs/(?:$strip_refs|remotes)/(.*)$!) {
 			$ver = $1;
 		}
 		$ver .= '-' . git_get_short_hash($project, $hash);
-- 
1.8.3.1

Re: [PATCH v3] gitweb: Add an option for adding more branch refs

[Eric Sunshine]

On Thu, Nov 28, 2013 at 6:44 AM, Krzesimir Nowak <krzesimir@endocode.com> wrote:
> Allow @additional_branch_refs configuration variable 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>
> ---
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 68c77f6..25e1d37 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -680,6 +688,19 @@ sub read_config_file {
>         return;
>  }
>
> +# performs sanity checks on parts of configuration.
> +sub config_sanity_check {
> +       # check additional refs validity
> +       my %unique_branch_refs = ();
> +       for my $ref (@additional_branch_refs) {
> +               die_error(500, "Invalid ref '$ref' in \@additional_branch_refs") unless (validate_ref($ref));
> +               # 'heads' are added implicitly in get_branch_refs().
> +               $unique_branch_refs{$ref} = 1 if ($ref ne 'heads');
> +       }
> +       @additional_branch_refs = sort keys %unique_branch_refs;
> +       %unique_branch_refs = undef;
> +}

%unique_branch_refs is going out of scope here, so clearing it seems
unnecessary.

Moreover, with warnings enabled, perl should be complaining about an
"Odd number of elements in hash assignment". (Normally, you would
clear a hash with '%foo=()' or 'undef %foo'.)

> +
>  our ($GITWEB_CONFIG, $GITWEB_CONFIG_SYSTEM, $GITWEB_CONFIG_COMMON);
>  sub evaluate_gitweb_config {
>         our $GITWEB_CONFIG = $ENV{'GITWEB_CONFIG'} || "++GITWEB_CONFIG++";

Re: [PATCH v3] gitweb: Add an option for adding more branch refs

[Krzesimir Nowak]

On Thu, 2013-11-28 at 20:13 -0500, Eric Sunshine wrote:
> On Thu, Nov 28, 2013 at 6:44 AM, Krzesimir Nowak <krzesimir@endocode.com> wrote:
> > Allow @additional_branch_refs configuration variable 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>
> > ---
> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> > index 68c77f6..25e1d37 100755
> > --- a/gitweb/gitweb.perl
> > +++ b/gitweb/gitweb.perl
> > @@ -680,6 +688,19 @@ sub read_config_file {
> >         return;
> >  }
> >
> > +# performs sanity checks on parts of configuration.
> > +sub config_sanity_check {
> > +       # check additional refs validity
> > +       my %unique_branch_refs = ();
> > +       for my $ref (@additional_branch_refs) {
> > +               die_error(500, "Invalid ref '$ref' in \@additional_branch_refs") unless (validate_ref($ref));
> > +               # 'heads' are added implicitly in get_branch_refs().
> > +               $unique_branch_refs{$ref} = 1 if ($ref ne 'heads');
> > +       }
> > +       @additional_branch_refs = sort keys %unique_branch_refs;
> > +       %unique_branch_refs = undef;
> > +}
> 
> %unique_branch_refs is going out of scope here, so clearing it seems
> unnecessary.

I am cleaning it in case when more sanity checking code gets added. So
there is no need to keep the data further. 

> 
> Moreover, with warnings enabled, perl should be complaining about an
> "Odd number of elements in hash assignment". (Normally, you would
> clear a hash with '%foo=()' or 'undef %foo'.)
> 

Gah, ok. I'll fix it. Thanks.

> > +
> >  our ($GITWEB_CONFIG, $GITWEB_CONFIG_SYSTEM, $GITWEB_CONFIG_COMMON);
> >  sub evaluate_gitweb_config {
> >         our $GITWEB_CONFIG = $ENV{'GITWEB_CONFIG'} || "++GITWEB_CONFIG++";

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

Re: [PATCH v3] gitweb: Add an option for adding more branch refs

[Jakub Narębski]

On Thu, Nov 28, 2013 at 12:44 PM, Krzesimir Nowak
<krzesimir@endocode.com> wrote:

> Allow @additional_branch_refs configuration variable 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>

Why not use %feature hash instead of adding new configuration variable?
I think that this option is similar enough to 'remote_heads' feature
(which BTW should be 'remote-heads'), and could conceveilably enabled
on a per-repository basis, i.e. with repository configuration override,
isn't it?

Usually %feature hash is preferred over adding new configuration variable
but this is not some hard rule. Note however that patches adding new config
are met with more scrutiny, as it is harder to fix mistakes because of
requirement of backwards compatibility of configuration files.

BTW. there really should be gitweb/CodingGuidelines...

> ---
>  Documentation/gitweb.conf.txt | 13 ++++++++
>  gitweb/gitweb.perl            | 75 +++++++++++++++++++++++++++++++++----------
>  2 files changed, 71 insertions(+), 17 deletions(-)
>
> diff --git a/Documentation/gitweb.conf.txt b/Documentation/gitweb.conf.txt
> index e2113d9..cd1a945 100644
> --- a/Documentation/gitweb.conf.txt
> +++ b/Documentation/gitweb.conf.txt
> @@ -549,6 +549,19 @@ This variable matters only when using persistent web environments that
>  serve multiple requests using single gitweb instance, like mod_perl,
>  FastCGI or Plackup.
>
> +@additional_branch_refs::
> +       List of additional directories under "refs" which are going to be used
> +       as branch refs. You might want to set this variable 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, for example
> ++
> +--------------------------------------------------------------------------------
> +our @additional_branch_refs = ('sandbox', 'wip', 'other');
> +--------------------------------------------------------------------------------

I think the last (long) sentence would better read if it began with "For example
if you have... then you could set this variable to ...", IMVHO.

BTW. if we decide on using %feature hash instead, it would be in the
"CONFIGURING GITWEB FEATURES" section.

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

Hmmm... One one hand erroring out on invalid refs means that we can
find error in config earlier and easier, on the other hand ignoring invalid
refs would make it resilent to errors in gitweb config (and repository config,
if we use %feature with per-repository override).


> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
[...]
> @@ -626,6 +630,10 @@ sub feature_avatar {
>         return @val ? @val : @_;
>  }
>
> +sub get_branch_refs {
> +    return ('heads', @additional_branch_refs);
> +}

Nice way of ensuring that list of all branches starts with "heads".

>  # checking HEAD file with -e is fragile if the repository was
>  # initialized long time ago (i.e. symlink HEAD) and was pack-ref'ed
>  # and then pruned.
> @@ -680,6 +688,19 @@ sub read_config_file {
>         return;
>  }
>
> +# performs sanity checks on parts of configuration.
> +sub config_sanity_check {
> +       # check additional refs validity
> +       my %unique_branch_refs = ();
> +       for my $ref (@additional_branch_refs) {
> +               die_error(500, "Invalid ref '$ref' in \@additional_branch_refs") unless (validate_ref($ref));
> +               # 'heads' are added implicitly in get_branch_refs().
> +               $unique_branch_refs{$ref} = 1 if ($ref ne 'heads');
> +       }
> +       @additional_branch_refs = sort keys %unique_branch_refs;
> +       %unique_branch_refs = undef;
> +}

This subroutine is quite similar to filter_snapshot_fmts for 'snapshot'
feature, perhaps the name should be patterned after it, i.e.
filter_branch_refs() or something...

If there were generic config_sanity_check(), it would call filter_branch_refs().

> @@ -698,8 +719,11 @@ sub evaluate_gitweb_config {
>
>         # Use first config file that exists.  This means use the per-instance
>         # GITWEB_CONFIG if exists, otherwise use GITWEB_SYSTEM_CONFIG.
> -       read_config_file($GITWEB_CONFIG) and return;
> -       read_config_file($GITWEB_CONFIG_SYSTEM);
> +       if (!read_config_file($GITWEB_CONFIG)) {
> +               read_config_file($GITWEB_CONFIG_SYSTEM);
> +       }
> +
> +       config_sanity_check();
>  }

I'm not sure if evaluate_gitweb_config is best place for sanity check
of said gitweb config, and not e.g. in run_request()... though having
it there has its own advantages.

BTW. it can be written as:

  -       read_config_file($GITWEB_CONFIG) and return;
  -       read_config_file($GITWEB_CONFIG_SYSTEM);
  +      read_config_file($GITWEB_CONFIG) or
  +      read_config_file($GITWEB_CONFIG_SYSTEM);
  +
  +       config_sanity_check();


Anyway if we were to use %feature hash, there is configure_gitweb_features()
for calling filter_branch_refs().

>  # Get loadavg of system, to compare against $maxload.
> @@ -1452,6 +1476,16 @@ sub validate_pathname {
>         return $input;
>  }
>
> +sub validate_ref {
> +       my $input = shift || return undef;
> +
> +       # restrictions on ref name according to git-check-ref-format
> +       if ($input =~ m!(/\.|\.\.|[\000-\040\177 ~^:?*\[]|/$)!) {
> +               return undef;
> +       }
> +       return $input;
> +}
> +
>  sub validate_refname {
>         my $input = shift || return undef;

Hmmm... validate_ref() is IMHO too similar to validate_refname(),
and it isn't about *parameter* validation. Perhaps check_ref_format()?

> @@ -1462,10 +1496,9 @@ sub validate_refname {
>         # it must be correct pathname
>         $input = validate_pathname($input)
>                 or return undef;
> -       # restrictions on ref name according to git-check-ref-format
> -       if ($input =~ m!(/\.|\.\.|[\000-\040\177 ~^:?*\[]|/$)!) {
> -               return undef;
> -       }
> +       # check git-check-ref-format restrictions
> +       $input = validate_ref($input)
> +               or return undef;
>         return $input;
>  }

Nice refactoring (it *could*, but doesn't need to, be in separate patch).

> @@ -2515,6 +2548,7 @@ sub format_snapshot_links {
>  sub get_feed_info {
>         my $format = shift || 'Atom';
>         my %res = (action => lc($format));
> +       my $matched_ref = 0;
>
>         # feed links are possible only for project views
>         return unless (defined $project);
> @@ -2522,12 +2556,17 @@ sub get_feed_info {
>         # or don't have specific feed yet (so they should use generic)
>         return if (!$action || $action =~ /^(?:tags|heads|forks|tag|search)$/x);
>
> -       my $branch;
> -       # branches refs uses 'refs/heads/' prefix (fullname) to differentiate
> -       # from tag links; this also makes possible to detect branch links
> -       if ((defined $hash_base && $hash_base =~ m!^refs/heads/(.*)$!) ||
> -           (defined $hash      && $hash      =~ m!^refs/heads/(.*)$!)) {
> -               $branch = $1;
> +       my $branch = undef;
> +       # branches refs uses 'refs/' + $get_branch_refs()[x] + '/' prefix
> +       # (fullname) to differentiate from tag links; this also makes
> +       # possible to detect branch links
> +       for my $ref (get_branch_refs()) {
> +               if ((defined $hash_base && $hash_base =~ m!^refs/\Q$ref\E/(.*)$!) ||
> +                   (defined $hash      && $hash      =~ m!^refs/\Q$ref\E/(.*)$!)) {
> +                       $branch = $1;
> +                       $matched_ref = $ref;
> +                       last;
> +               }
>         }

Nice!

> @@ -3662,7 +3701,8 @@ sub git_get_heads_list {
>                 my ($committer, $epoch, $tz) =
>                         ($committerinfo =~ /^(.*) ([0-9]+) (.*)$/);
>                 $ref_item{'fullname'}  = $name;
> -               $name =~ s!^refs/(?:head|remote)s/!!;
> +               my $strip_refs = join '|', map { quotemeta } get_branch_refs();
> +               $name =~ s!^refs/(?:$strip_refs|remotes)/!!;
>
>                 $ref_item{'name'}  = $name;
>                 $ref_item{'id'}    = $hash;
> @@ -7179,7 +7219,8 @@ sub snapshot_name {
>                 $ver = $1;
>         } else {
>                 # branches and other need shortened SHA-1 hash
> -               if ($hash =~ m!^refs/(?:heads|remotes)/(.*)$!) {
> +               my $strip_refs = join '|', map { quotemeta } get_branch_refs();
> +               if ($hash =~ m!^refs/(?:$strip_refs|remotes)/(.*)$!) {
>                         $ver = $1;
>                 }
>                 $ver .= '-' . git_get_short_hash($project, $hash);

One one hand, it is about threating extra branch refs the same way as 'head'.
On the other hand we loose distinction between 'refs/heads/foo' and e.g.
'refs/wip/foo'. But maybe that's all right...

-- 
Jakub Narebski

Re: [PATCH v3] gitweb: Add an option for adding more branch refs

[Krzesimir Nowak]

On Mon, 2013-12-02 at 01:21 +0100, Jakub Narębski wrote:
> On Thu, Nov 28, 2013 at 12:44 PM, Krzesimir Nowak
> <krzesimir@endocode.com> wrote:
> 
> > Allow @additional_branch_refs configuration variable 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>
> 
> Why not use %feature hash instead of adding new configuration variable?
> I think that this option is similar enough to 'remote_heads' feature
> (which BTW should be 'remote-heads'), and could conceveilably enabled
> on a per-repository basis, i.e. with repository configuration override,
> isn't it?

I'd like to see some consensus on it before I start changing the patch
again.

> 
> Usually %feature hash is preferred over adding new configuration variable
> but this is not some hard rule. Note however that patches adding new config
> are met with more scrutiny, as it is harder to fix mistakes because of
> requirement of backwards compatibility of configuration files.
> 

I don't know what kind of backwards compatibility you mention. Whether
you want gitweb to survive reading old config file or to honor
deprecated/old config variables. If the former than I have already read
somewhere that you always should use config vars like:
our $config = 'value';
Note the 'our' which avoids gitweb failures in case of config variable
removal.

> BTW. there really should be gitweb/CodingGuidelines...
> 

Yes, would be useful. As in every other project. :)

> > ---
> >  Documentation/gitweb.conf.txt | 13 ++++++++
> >  gitweb/gitweb.perl            | 75 +++++++++++++++++++++++++++++++++----------
> >  2 files changed, 71 insertions(+), 17 deletions(-)
> >
> > diff --git a/Documentation/gitweb.conf.txt b/Documentation/gitweb.conf.txt
> > index e2113d9..cd1a945 100644
> > --- a/Documentation/gitweb.conf.txt
> > +++ b/Documentation/gitweb.conf.txt
> > @@ -549,6 +549,19 @@ This variable matters only when using persistent web environments that
> >  serve multiple requests using single gitweb instance, like mod_perl,
> >  FastCGI or Plackup.
> >
> > +@additional_branch_refs::
> > +       List of additional directories under "refs" which are going to be used
> > +       as branch refs. You might want to set this variable 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, for example
> > ++
> > +--------------------------------------------------------------------------------
> > +our @additional_branch_refs = ('sandbox', 'wip', 'other');
> > +--------------------------------------------------------------------------------
> 
> I think the last (long) sentence would better read if it began with "For example
> if you have... then you could set this variable to ...", IMVHO.
> 

Right, thanks. Will rephrase it.

> BTW. if we decide on using %feature hash instead, it would be in the
> "CONFIGURING GITWEB FEATURES" section.

Yes, but I'll wait for some consensus with it.

> 
> > ++
> > +It is an error to specify a ref that does not pass "git check-ref-format"
> > +scrutiny.
> 
> Hmmm... One one hand erroring out on invalid refs means that we can
> find error in config earlier and easier, on the other hand ignoring invalid
> refs would make it resilent to errors in gitweb config (and repository config,
> if we use %feature with per-repository override).
> 

We could ignore bad values, but that would make it harder to find out
what exactly is wrong when something we configured to be shown is not
shown at all.

> 
> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> [...]
> > @@ -626,6 +630,10 @@ sub feature_avatar {
> >         return @val ? @val : @_;
> >  }
> >
> > +sub get_branch_refs {
> > +    return ('heads', @additional_branch_refs);
> > +}
> 
> Nice way of ensuring that list of all branches starts with "heads".
> 
> >  # checking HEAD file with -e is fragile if the repository was
> >  # initialized long time ago (i.e. symlink HEAD) and was pack-ref'ed
> >  # and then pruned.
> > @@ -680,6 +688,19 @@ sub read_config_file {
> >         return;
> >  }
> >
> > +# performs sanity checks on parts of configuration.
> > +sub config_sanity_check {
> > +       # check additional refs validity
> > +       my %unique_branch_refs = ();
> > +       for my $ref (@additional_branch_refs) {
> > +               die_error(500, "Invalid ref '$ref' in \@additional_branch_refs") unless (validate_ref($ref));
> > +               # 'heads' are added implicitly in get_branch_refs().
> > +               $unique_branch_refs{$ref} = 1 if ($ref ne 'heads');
> > +       }
> > +       @additional_branch_refs = sort keys %unique_branch_refs;
> > +       %unique_branch_refs = undef;
> > +}
> 
> This subroutine is quite similar to filter_snapshot_fmts for 'snapshot'
> feature, perhaps the name should be patterned after it, i.e.
> filter_branch_refs() or something...
> 
> If there were generic config_sanity_check(), it would call filter_branch_refs().

I had an additional_branches_refs_check() sub which was called by
config_sanity_check(), but I scrapped it. I wanted config_sanity_check()
to be general configuration checker, but for now it would only call a
single function, so I inlined it. If later more configuration checking
will be added then the current body could be moved to separate sub.

I can move it back to separate sub if you want.

> 
> > @@ -698,8 +719,11 @@ sub evaluate_gitweb_config {
> >
> >         # Use first config file that exists.  This means use the per-instance
> >         # GITWEB_CONFIG if exists, otherwise use GITWEB_SYSTEM_CONFIG.
> > -       read_config_file($GITWEB_CONFIG) and return;
> > -       read_config_file($GITWEB_CONFIG_SYSTEM);
> > +       if (!read_config_file($GITWEB_CONFIG)) {
> > +               read_config_file($GITWEB_CONFIG_SYSTEM);
> > +       }
> > +
> > +       config_sanity_check();
> >  }
> 
> I'm not sure if evaluate_gitweb_config is best place for sanity check
> of said gitweb config, and not e.g. in run_request()... though having
> it there has its own advantages.
> 
> BTW. it can be written as:
> 
>   -       read_config_file($GITWEB_CONFIG) and return;
>   -       read_config_file($GITWEB_CONFIG_SYSTEM);
>   +      read_config_file($GITWEB_CONFIG) or
>   +      read_config_file($GITWEB_CONFIG_SYSTEM);
>   +
>   +       config_sanity_check();
> 

Ok, will rewrite it.

> 
> Anyway if we were to use %feature hash, there is configure_gitweb_features()
> for calling filter_branch_refs().
> 
> >  # Get loadavg of system, to compare against $maxload.
> > @@ -1452,6 +1476,16 @@ sub validate_pathname {
> >         return $input;
> >  }
> >
> > +sub validate_ref {
> > +       my $input = shift || return undef;
> > +
> > +       # restrictions on ref name according to git-check-ref-format
> > +       if ($input =~ m!(/\.|\.\.|[\000-\040\177 ~^:?*\[]|/$)!) {
> > +               return undef;
> > +       }
> > +       return $input;
> > +}
> > +
> >  sub validate_refname {
> >         my $input = shift || return undef;
> 
> Hmmm... validate_ref() is IMHO too similar to validate_refname(),
> and it isn't about *parameter* validation. Perhaps check_ref_format()?

Ok.

> 
> > @@ -1462,10 +1496,9 @@ sub validate_refname {
> >         # it must be correct pathname
> >         $input = validate_pathname($input)
> >                 or return undef;
> > -       # restrictions on ref name according to git-check-ref-format
> > -       if ($input =~ m!(/\.|\.\.|[\000-\040\177 ~^:?*\[]|/$)!) {
> > -               return undef;
> > -       }
> > +       # check git-check-ref-format restrictions
> > +       $input = validate_ref($input)
> > +               or return undef;
> >         return $input;
> >  }
> 
> Nice refactoring (it *could*, but doesn't need to, be in separate patch).
> 
> > @@ -2515,6 +2548,7 @@ sub format_snapshot_links {
> >  sub get_feed_info {
> >         my $format = shift || 'Atom';
> >         my %res = (action => lc($format));
> > +       my $matched_ref = 0;
> >
> >         # feed links are possible only for project views
> >         return unless (defined $project);
> > @@ -2522,12 +2556,17 @@ sub get_feed_info {
> >         # or don't have specific feed yet (so they should use generic)
> >         return if (!$action || $action =~ /^(?:tags|heads|forks|tag|search)$/x);
> >
> > -       my $branch;
> > -       # branches refs uses 'refs/heads/' prefix (fullname) to differentiate
> > -       # from tag links; this also makes possible to detect branch links
> > -       if ((defined $hash_base && $hash_base =~ m!^refs/heads/(.*)$!) ||
> > -           (defined $hash      && $hash      =~ m!^refs/heads/(.*)$!)) {
> > -               $branch = $1;
> > +       my $branch = undef;
> > +       # branches refs uses 'refs/' + $get_branch_refs()[x] + '/' prefix
> > +       # (fullname) to differentiate from tag links; this also makes
> > +       # possible to detect branch links
> > +       for my $ref (get_branch_refs()) {
> > +               if ((defined $hash_base && $hash_base =~ m!^refs/\Q$ref\E/(.*)$!) ||
> > +                   (defined $hash      && $hash      =~ m!^refs/\Q$ref\E/(.*)$!)) {
> > +                       $branch = $1;
> > +                       $matched_ref = $ref;
> > +                       last;
> > +               }
> >         }
> 
> Nice!
> 
> > @@ -3662,7 +3701,8 @@ sub git_get_heads_list {
> >                 my ($committer, $epoch, $tz) =
> >                         ($committerinfo =~ /^(.*) ([0-9]+) (.*)$/);
> >                 $ref_item{'fullname'}  = $name;
> > -               $name =~ s!^refs/(?:head|remote)s/!!;
> > +               my $strip_refs = join '|', map { quotemeta } get_branch_refs();
> > +               $name =~ s!^refs/(?:$strip_refs|remotes)/!!;
> >
> >                 $ref_item{'name'}  = $name;
> >                 $ref_item{'id'}    = $hash;
> > @@ -7179,7 +7219,8 @@ sub snapshot_name {
> >                 $ver = $1;
> >         } else {
> >                 # branches and other need shortened SHA-1 hash
> > -               if ($hash =~ m!^refs/(?:heads|remotes)/(.*)$!) {
> > +               my $strip_refs = join '|', map { quotemeta } get_branch_refs();
> > +               if ($hash =~ m!^refs/(?:$strip_refs|remotes)/(.*)$!) {
> >                         $ver = $1;
> >                 }
> >                 $ver .= '-' . git_get_short_hash($project, $hash);
> 
> One one hand, it is about threating extra branch refs the same way as 'head'.
> On the other hand we loose distinction between 'refs/heads/foo' and e.g.
> 'refs/wip/foo'. But maybe that's all right...
> 

In git_get_heads_list sub I could append a " ($ref_dir)" to refs which
are in neither 'heads' nor 'remotes', so heads view would look like:
master
old-stable
some-work-in-progress (wip)
some-other-branch (other) 

where both master and old-stable are in refs/heads/,
some-work-in-progress in refs/wip/ and some-other-branch in refs/other/.

In case of branch snapshot names (snapshot_name sub) I could change it,
so names for branches mentioned above would be
"Project-master-<short-hash>.tgz",
"Project-old_stable-<short-hash>.tgz",
"Project-wip-some-work-in-progress-<short-hash>.tgz"
"Project-other-some-other-branch-<short-hash>.tgz"

What do you think?

Cheers,
-- 
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

Re: [PATCH v3] gitweb: Add an option for adding more branch refs

[Jakub Narębski]

W dniu 2013-12-02 13:06, Krzesimir Nowak pisze:
> On Mon, 2013-12-02 at 01:21 +0100, Jakub Narębski wrote:
>> On Thu, Nov 28, 2013 at 12:44 PM, Krzesimir Nowak
>> <krzesimir@endocode.com>  wrote:
>>
>>> Allow @additional_branch_refs configuration variable 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>
>>
>> Why not use %feature hash instead of adding new configuration variable?
>> I think that this option is similar enough to 'remote_heads' feature
>> (which BTW should be 'remote-heads'), and could conceivably enabled
>> on a per-repository basis, i.e. with repository configuration override,
>> isn't it?
>
> I'd like to see some consensus on it before I start changing the patch
> again.

%feature hash is mainly (but not only) about options that can be
configured on per-repository basis.  Configuration variables are
about options that are per-instance (per gitweb).

>> Usually %feature hash is preferred over adding new configuration variable
>> but this is not some hard rule. Note however that patches adding new config
>> are met with more scrutiny, as it is harder to fix mistakes because of
>> requirement of backwards compatibility of configuration files.
>>
>
> I don't know what kind of backwards compatibility you mention. Whether
> you want gitweb to survive reading old config file or to honor
> deprecated/old config variables.

I meant here honoring deprecated/old variables, i.e. honoring existing
configuration files.  See for example backward compatibility for old
$stylesheet variable vs new @stylesheets in print_header_links().

Though in this case it shouldn't be much of a problem; it would be
easy to honor @additional_branch_refs by setting 'default' for
'extra-branch-refs' feature to it.

>> BTW. there really should be gitweb/CodingGuidelines...
>>
>
> Yes, would be useful. As in every other project. :)

Well, Git itself *has* Documentation/CodingGuidelines, but perhaps
gitweb subsystem should have it's own...

[...]
>>> @@ -3662,7 +3701,8 @@ sub git_get_heads_list {
>>>                  my ($committer, $epoch, $tz) =
>>>                          ($committerinfo =~ /^(.*) ([0-9]+) (.*)$/);
>>>                  $ref_item{'fullname'}  = $name;
>>> -               $name =~ s!^refs/(?:head|remote)s/!!;
>>> +               my $strip_refs = join '|', map { quotemeta } get_branch_refs();
>>> +               $name =~ s!^refs/(?:$strip_refs|remotes)/!!;
>>>
>>>                  $ref_item{'name'}  = $name;
>>>                  $ref_item{'id'}    = $hash;
>>> @@ -7179,7 +7219,8 @@ sub snapshot_name {
>>>                  $ver = $1;
>>>          } else {
>>>                  # branches and other need shortened SHA-1 hash
>>> -               if ($hash =~ m!^refs/(?:heads|remotes)/(.*)$!) {
>>> +               my $strip_refs = join '|', map { quotemeta } get_branch_refs();
>>> +               if ($hash =~ m!^refs/(?:$strip_refs|remotes)/(.*)$!) {
>>>                          $ver = $1;
>>>                  }
>>>                  $ver .= '-' . git_get_short_hash($project, $hash);
>>
>> One one hand, it is about threating extra branch refs the same way as 'head'.
>> On the other hand we loose distinction between 'refs/heads/foo' and e.g.
>> 'refs/wip/foo'. But maybe that's all right...
>>
>
> In git_get_heads_list sub I could append a " ($ref_dir)" to refs which
> are in neither 'heads' nor 'remotes', so heads view would look like:
> master
> old-stable
> some-work-in-progress (wip)
> some-other-branch (other)
>
> where both master and old-stable are in refs/heads/,
> some-work-in-progress in refs/wip/ and some-other-branch in refs/other/.
>
> In case of branch snapshot names (snapshot_name sub) I could change it,
> so names for branches mentioned above would be
> "Project-master-<short-hash>.tgz",
> "Project-old_stable-<short-hash>.tgz",
> "Project-wip-some-work-in-progress-<short-hash>.tgz"
> "Project-other-some-other-branch-<short-hash>.tgz"
>
> What do you think?

That is, I think, a very good idea.  Though perhaps it would be more 
readable to add this extra feature as a separate patch, on top of main one.

-- 
Jakub Narebski

Re: [PATCH v3] gitweb: Add an option for adding more branch refs

[Krzesimir Nowak]

On Mon, 2013-12-02 at 18:34 +0100, Jakub Narębski wrote:
> W dniu 2013-12-02 13:06, Krzesimir Nowak pisze:
> > On Mon, 2013-12-02 at 01:21 +0100, Jakub Narębski wrote:
> >> On Thu, Nov 28, 2013 at 12:44 PM, Krzesimir Nowak
> >> <krzesimir@endocode.com>  wrote:
> >>
> >>> Allow @additional_branch_refs configuration variable 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>
> >>
> >> Why not use %feature hash instead of adding new configuration variable?
> >> I think that this option is similar enough to 'remote_heads' feature
> >> (which BTW should be 'remote-heads'), and could conceivably enabled
> >> on a per-repository basis, i.e. with repository configuration override,
> >> isn't it?
> >
> > I'd like to see some consensus on it before I start changing the patch
> > again.
> 
> %feature hash is mainly (but not only) about options that can be
> configured on per-repository basis.  Configuration variables are
> about options that are per-instance (per gitweb).

Well, I am mostly interested in per-instance configuration in this case,
but if that is also possible with %feature hash, then ok, I'll try to
make it work.

From what I've seen (correct me please if I got it wrong) feature
settings is taken from per-repository config file from [gitweb] section.
If there's nothing then some default value is taken. That default value
can be overriden with per-instance perl config file.

So it is easy to override it from per-instance perl config by typing:
$feature{'additional-branch-refs'}{'default'} = ['wip', 'no|tf"un,ny'];
$feature{'additional-branch-refs'}{'override'} = 1;

(Note the edge case of refs/no|tf"un,ny, which passes the git
check-ref-format scrutiny.)

But for now, most of features are quite simple - either booleans,
integers or list of simple strings (in snapshot feature). What I need
here is a list of strings, like CSV in following example:
[gitweb]
	additional_branch_refs = wip,"no|tf""un,ny"

Is dependency on external module like Text::CSV or Text::CSV_XS ok? If
not, I can hack some CSV reading code.

> 
> >> Usually %feature hash is preferred over adding new configuration variable
> >> but this is not some hard rule. Note however that patches adding new config
> >> are met with more scrutiny, as it is harder to fix mistakes because of
> >> requirement of backwards compatibility of configuration files.
> >>
> >
> > I don't know what kind of backwards compatibility you mention. Whether
> > you want gitweb to survive reading old config file or to honor
> > deprecated/old config variables.
> 
> I meant here honoring deprecated/old variables, i.e. honoring existing
> configuration files.  See for example backward compatibility for old
> $stylesheet variable vs new @stylesheets in print_header_links().
> 
> Though in this case it shouldn't be much of a problem; it would be
> easy to honor @additional_branch_refs by setting 'default' for
> 'extra-branch-refs' feature to it.

extra-branch-refs is nicer than additional-branch-refs, I'll use it.

> 
> >> BTW. there really should be gitweb/CodingGuidelines...
> >>
> >
> > Yes, would be useful. As in every other project. :)
> 
> Well, Git itself *has* Documentation/CodingGuidelines, but perhaps
> gitweb subsystem should have it's own...
> 
> [...]
> >>> @@ -3662,7 +3701,8 @@ sub git_get_heads_list {
> >>>                  my ($committer, $epoch, $tz) =
> >>>                          ($committerinfo =~ /^(.*) ([0-9]+) (.*)$/);
> >>>                  $ref_item{'fullname'}  = $name;
> >>> -               $name =~ s!^refs/(?:head|remote)s/!!;
> >>> +               my $strip_refs = join '|', map { quotemeta } get_branch_refs();
> >>> +               $name =~ s!^refs/(?:$strip_refs|remotes)/!!;
> >>>
> >>>                  $ref_item{'name'}  = $name;
> >>>                  $ref_item{'id'}    = $hash;
> >>> @@ -7179,7 +7219,8 @@ sub snapshot_name {
> >>>                  $ver = $1;
> >>>          } else {
> >>>                  # branches and other need shortened SHA-1 hash
> >>> -               if ($hash =~ m!^refs/(?:heads|remotes)/(.*)$!) {
> >>> +               my $strip_refs = join '|', map { quotemeta } get_branch_refs();
> >>> +               if ($hash =~ m!^refs/(?:$strip_refs|remotes)/(.*)$!) {
> >>>                          $ver = $1;
> >>>                  }
> >>>                  $ver .= '-' . git_get_short_hash($project, $hash);
> >>
> >> One one hand, it is about threating extra branch refs the same way as 'head'.
> >> On the other hand we loose distinction between 'refs/heads/foo' and e.g.
> >> 'refs/wip/foo'. But maybe that's all right...
> >>
> >
> > In git_get_heads_list sub I could append a " ($ref_dir)" to refs which
> > are in neither 'heads' nor 'remotes', so heads view would look like:
> > master
> > old-stable
> > some-work-in-progress (wip)
> > some-other-branch (other)
> >
> > where both master and old-stable are in refs/heads/,
> > some-work-in-progress in refs/wip/ and some-other-branch in refs/other/.
> >
> > In case of branch snapshot names (snapshot_name sub) I could change it,
> > so names for branches mentioned above would be
> > "Project-master-<short-hash>.tgz",
> > "Project-old_stable-<short-hash>.tgz",
> > "Project-wip-some-work-in-progress-<short-hash>.tgz"
> > "Project-other-some-other-branch-<short-hash>.tgz"
> >
> > What do you think?
> 
> That is, I think, a very good idea.  Though perhaps it would be more 
> readable to add this extra feature as a separate patch, on top of main one.
> 

Right, I suppose this patch is going to end up being several patches.

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

Re: [PATCH v3] gitweb: Add an option for adding more branch refs

[Jakub Narębski]

On Tue, Dec 3, 2013 at 11:53 AM, Krzesimir Nowak <krzesimir@endocode.com> wrote:
> On Mon, 2013-12-02 at 18:34 +0100, Jakub Narębski wrote:
>> W dniu 2013-12-02 13:06, Krzesimir Nowak pisze:
>>> On Mon, 2013-12-02 at 01:21 +0100, Jakub Narębski wrote:
>>>> On Thu, Nov 28, 2013 at 12:44 PM, Krzesimir Nowak
>>>> <krzesimir@endocode.com>  wrote:
>>>>
>>>>> Allow @additional_branch_refs configuration variable 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>
>>>>
>>>> Why not use %feature hash instead of adding new configuration variable?
>>>> I think that this option is similar enough to 'remote_heads' feature
>>>> (which BTW should be 'remote-heads'), and could conceivably enabled
>>>> on a per-repository basis, i.e. with repository configuration override,
>>>> isn't it?
>>>
>>> I'd like to see some consensus on it before I start changing the patch
>>> again.
>>
>> %feature hash is mainly (but not only) about options that can be
>> configured on per-repository basis.  Configuration variables are
>> about options that are per-instance (per gitweb).
>
> Well, I am mostly interested in per-instance configuration in this case,
> but if that is also possible with %feature hash, then ok, I'll try to
> make it work.

Yes, it is possible to have per-instance configuration (you can even
forbid per-repository configuration).

> From what I've seen (correct me please if I got it wrong) feature
> settings is taken from per-repository config file from [gitweb] section.
> If there's nothing then some default value is taken. That default value
> can be overriden with per-instance perl config file.

%feature settings are taken from gitweb configuration (the 'default'
key), and if given feature is overrideable and per-repository configuration
in the form of appropriate key in [gitweb] section of repository config
file exists, it is used instead.

> So it is easy to override it from per-instance perl config by typing:
> $feature{'additional-branch-refs'}{'default'} = ['wip', 'no|tf"un,ny'];
> $feature{'additional-branch-refs'}{'override'} = 1;

Yes.  The 'override' is about checking (which imposes a bit of
performance penalty) and respecting per-repository configuration.

> (Note the edge case of refs/no|tf"un,ny, which passes the git
> check-ref-format scrutiny.)
>
> But for now, most of features are quite simple - either booleans,
> integers or list of simple strings (in snapshot feature). What I need
> here is a list of strings, like CSV in following example:
> [gitweb]
>         additional_branch_refs = wip,"no|tf""un,ny"
>
> Is dependency on external module like Text::CSV or Text::CSV_XS ok? If
> not, I can hack some CSV reading code.

Why not use space, which is forbidden in refnames, to separate
entries?  Similar to feature_snapshot(), which is about comma separated
list, without escaping.

-- 
Jakub Narebski

Re: [PATCH v3] gitweb: Add an option for adding more branch refs

[Krzesimir Nowak]

On Tue, 2013-12-03 at 14:02 +0100, Jakub Narębski wrote:
> On Tue, Dec 3, 2013 at 11:53 AM, Krzesimir Nowak <krzesimir@endocode.com> wrote:
> > On Mon, 2013-12-02 at 18:34 +0100, Jakub Narębski wrote:
> >> W dniu 2013-12-02 13:06, Krzesimir Nowak pisze:
> >>> On Mon, 2013-12-02 at 01:21 +0100, Jakub Narębski wrote:
> >>>> On Thu, Nov 28, 2013 at 12:44 PM, Krzesimir Nowak
> >>>> <krzesimir@endocode.com>  wrote:
> >>>>
> >>>>> Allow @additional_branch_refs configuration variable 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>
> >>>>
> >>>> Why not use %feature hash instead of adding new configuration variable?
> >>>> I think that this option is similar enough to 'remote_heads' feature
> >>>> (which BTW should be 'remote-heads'), and could conceivably enabled
> >>>> on a per-repository basis, i.e. with repository configuration override,
> >>>> isn't it?
> >>>
> >>> I'd like to see some consensus on it before I start changing the patch
> >>> again.
> >>
> >> %feature hash is mainly (but not only) about options that can be
> >> configured on per-repository basis.  Configuration variables are
> >> about options that are per-instance (per gitweb).
> >
> > Well, I am mostly interested in per-instance configuration in this case,
> > but if that is also possible with %feature hash, then ok, I'll try to
> > make it work.
> 
> Yes, it is possible to have per-instance configuration (you can even
> forbid per-repository configuration).
> 
> > From what I've seen (correct me please if I got it wrong) feature
> > settings is taken from per-repository config file from [gitweb] section.
> > If there's nothing then some default value is taken. That default value
> > can be overriden with per-instance perl config file.
> 
> %feature settings are taken from gitweb configuration (the 'default'
> key), and if given feature is overrideable and per-repository configuration
> in the form of appropriate key in [gitweb] section of repository config
> file exists, it is used instead.
> 
> > So it is easy to override it from per-instance perl config by typing:
> > $feature{'additional-branch-refs'}{'default'} = ['wip', 'no|tf"un,ny'];
> > $feature{'additional-branch-refs'}{'override'} = 1;
> 
> Yes.  The 'override' is about checking (which imposes a bit of
> performance penalty) and respecting per-repository configuration.
> 
> > (Note the edge case of refs/no|tf"un,ny, which passes the git
> > check-ref-format scrutiny.)
> >
> > But for now, most of features are quite simple - either booleans,
> > integers or list of simple strings (in snapshot feature). What I need
> > here is a list of strings, like CSV in following example:
> > [gitweb]
> >         additional_branch_refs = wip,"no|tf""un,ny"
> >
> > Is dependency on external module like Text::CSV or Text::CSV_XS ok? If
> > not, I can hack some CSV reading code.
> 
> Why not use space, which is forbidden in refnames, to separate
> entries?  Similar to feature_snapshot(), which is about comma separated
> list, without escaping.
> 

Thanks for explanations and a hint. Following patches are in "Show extra
branch refs in gitweb".

Cheers,
-- 
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

Re: [PATCH v3] gitweb: Add an option for adding more branch refs

[Junio C Hamano]

Krzesimir Nowak <krzesimir@endocode.com> writes:

> On Mon, 2013-12-02 at 01:21 +0100, Jakub Narębski wrote:
>> On Thu, Nov 28, 2013 at 12:44 PM, Krzesimir Nowak
>> <krzesimir@endocode.com> wrote:
>> 
>> > Allow @additional_branch_refs configuration variable 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>
>> 
>> Why not use %feature hash instead of adding new configuration variable?
>> I think that this option is similar enough to 'remote_heads' feature
>> (which BTW should be 'remote-heads'), and could conceveilably enabled
>> on a per-repository basis, i.e. with repository configuration override,
>> isn't it?
>
> I'd like to see some consensus on it before I start changing the patch
> again.

I missed the remote-heads which is an existing feature when I
commented; if this can be exposed to the users as an extension to
it like Jakub suggests, it may be a better direction.

>> Usually %feature hash is preferred over adding new configuration variable
>> but this is not some hard rule. Note however that patches adding new config
>> are met with more scrutiny, as it is harder to fix mistakes because of
>> requirement of backwards compatibility of configuration files.
>
> I don't know what kind of backwards compatibility you mention.

A patch adds new feature controlled by a configuration in one way
(e.g. as a totally new ad-hoc switch that is seemingly orthogonal to
all the existing features), people start using the feature using
that configuration method, and then later we find out that it is
better controlled by a different way (e.g. as a natural extension to
an existing feature, controlled by how the existing feature is
controlled, perhaps after extending it) because it allows more
flexibility in the future.

At that point, we can extend the way the existing feature is
controlled to trigger the new feature in a more uniform way, but we
cannot remove the new ad-hoc switch the patch originally added to
control this new feature because there already are people who are
using it, and we end up having to support the unified and extensible
way to configure, which we prefer to keep in the longer term, and
also the ad-hoc switch the patch added, which we wish we never had
in the first place.  The latter needs to be deprecated and removed
over time.

Re: [PATCH v3] gitweb: Add an option for adding more branch refs

[Jakub Narębski]

On Mon, Dec 2, 2013 at 7:18 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Krzesimir Nowak <krzesimir@endocode.com> writes:
>> On Mon, 2013-12-02 at 01:21 +0100, Jakub Narębski wrote:
>>> On Thu, Nov 28, 2013 at 12:44 PM, Krzesimir Nowak
>>> <krzesimir@endocode.com> wrote:
>>>
>>> > Allow @additional_branch_refs configuration variable 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>
>>>
>>> Why not use %feature hash instead of adding new configuration variable?
>>> I think that this option is similar enough to 'remote_heads' feature
>>> (which BTW should be 'remote-heads'), and could conceveilably enabled
>>> on a per-repository basis, i.e. with repository configuration override,
>>> isn't it?
>>
>> I'd like to see some consensus on it before I start changing the patch
>> again.
>
> I missed the remote-heads which is an existing feature when I
> commented; if this can be exposed to the users as an extension to
> it like Jakub suggests, it may be a better direction.

I think that additional-branch-refs (or just branch-refs) is different enough
from remote_heads feature (which is about showing "remotes" section
in "summary" view, and IIRC adding "remotes" view) that it should be
kept separate.

On the other hand it is similar enough that I think style of implementation
should also be similar, i.e. use %feature hash.

IMVHO

-- 
Jakub Narebski

Re: [PATCH v3] gitweb: Add an option for adding more branch refs

[Junio C Hamano]

Jakub Narębski <jnareb@gmail.com> writes:

> I think that additional-branch-refs (or just branch-refs) is different enough
> from remote_heads feature (which is about showing "remotes" section
> in "summary" view, and IIRC adding "remotes" view) that it should be
> kept separate.
>
> On the other hand it is similar enough that I think style of implementation
> should also be similar, i.e. use %feature hash.

Sounds sensible.  Thanks.