{"thread":{"id":"35419","subject":"[PATCH v3] gitweb: Add an option for adding more branch refs","startedAt":"2013-11-28T11:44:52Z","lastAt":"2013-12-03T14:58:39Z","messageCount":12,"participants":["Krzesimir Nowak","Eric Sunshine","Jakub Narębski","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"231241","messageId":"1385639092-13362-1-git-send-email-krzesimir@endocode.com","threadId":"35419","inReplyTo":null,"subject":"[PATCH v3] gitweb: Add an option for adding more branch refs","fromName":"Krzesimir Nowak","fromEmail":"krzesimir@endocode.com","sentAt":"2013-11-28T11:44:52Z","receivedAt":"2013-11-28T11:44:52Z","isPatch":true,"sender":{"key":"krzesimir@endocode.com","avatar":"https://avatars.githubusercontent.com/u/96081?v=4"},"body":"Allow @additional_branch_refs configuration variable to tell gitweb to\nshow refs from additional hierarchies in addition to branches in the\nlist-of-branches view.\n\nSigned-off-by: Krzesimir Nowak <krzesimir@endocode.com>\n---\n Documentation/gitweb.conf.txt | 13 ++++++++\n gitweb/gitweb.perl            | 75 +++++++++++++++++++++++++++++++++----------\n 2 files changed, 71 insertions(+), 17 deletions(-)\n\ndiff --git a/Documentation/gitweb.conf.txt b/Documentation/gitweb.conf.txt\nindex e2113d9..cd1a945 100644\n--- a/Documentation/gitweb.conf.txt\n+++ b/Documentation/gitweb.conf.txt\n@@ -549,6 +549,19 @@ This variable matters only when using persistent web environments that\n serve multiple requests using single gitweb instance, like mod_perl,\n FastCGI or Plackup.\n \n+@additional_branch_refs::\n+\tList of additional directories under \"refs\" which are going to be used\n+\tas branch refs. You might want to set this variable if you have a gerrit\n+\tsetup where all branches under refs/heads/ are official,\n+\tpush-after-review ones and branches under refs/sandbox/, refs/wip and\n+\trefs/other are user ones where permissions are much wider, for example\n++\n+--------------------------------------------------------------------------------\n+our @additional_branch_refs = ('sandbox', 'wip', 'other');\n+--------------------------------------------------------------------------------\n++\n+It is an error to specify a ref that does not pass \"git check-ref-format\"\n+scrutiny.\n \n Other variables\n ~~~~~~~~~~~~~~~\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 68c77f6..25e1d37 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -122,6 +122,10 @@ our $logo_label = \"git homepage\";\n # source of projects list\n our $projects_list = \"++GITWEB_LIST++\";\n \n+# list of additional \"directories\" under \"refs/\" we want to display as\n+# branches\n+our @additional_branch_refs = ();\n+\n # the width (in characters) of the projects list \"Description\" column\n our $projects_list_description_width = 25;\n \n@@ -626,6 +630,10 @@ sub feature_avatar {\n \treturn @val ? @val : @_;\n }\n \n+sub get_branch_refs {\n+    return ('heads', @additional_branch_refs);\n+}\n+\n # checking HEAD file with -e is fragile if the repository was\n # initialized long time ago (i.e. symlink HEAD) and was pack-ref'ed\n # and then pruned.\n@@ -680,6 +688,19 @@ sub read_config_file {\n \treturn;\n }\n \n+# performs sanity checks on parts of configuration.\n+sub config_sanity_check {\n+\t# check additional refs validity\n+\tmy %unique_branch_refs = ();\n+\tfor my $ref (@additional_branch_refs) {\n+\t\tdie_error(500, \"Invalid ref '$ref' in \\@additional_branch_refs\") unless (validate_ref($ref));\n+\t\t# 'heads' are added implicitly in get_branch_refs().\n+\t\t$unique_branch_refs{$ref} = 1 if ($ref ne 'heads');\n+\t}\n+\t@additional_branch_refs = sort keys %unique_branch_refs;\n+\t%unique_branch_refs = undef;\n+}\n+\n our ($GITWEB_CONFIG, $GITWEB_CONFIG_SYSTEM, $GITWEB_CONFIG_COMMON);\n sub evaluate_gitweb_config {\n \tour $GITWEB_CONFIG = $ENV{'GITWEB_CONFIG'} || \"++GITWEB_CONFIG++\";\n@@ -698,8 +719,11 @@ sub evaluate_gitweb_config {\n \n \t# Use first config file that exists.  This means use the per-instance\n \t# GITWEB_CONFIG if exists, otherwise use GITWEB_SYSTEM_CONFIG.\n-\tread_config_file($GITWEB_CONFIG) and return;\n-\tread_config_file($GITWEB_CONFIG_SYSTEM);\n+\tif (!read_config_file($GITWEB_CONFIG)) {\n+\t\tread_config_file($GITWEB_CONFIG_SYSTEM);\n+\t}\n+\n+\tconfig_sanity_check();\n }\n \n # Get loadavg of system, to compare against $maxload.\n@@ -1452,6 +1476,16 @@ sub validate_pathname {\n \treturn $input;\n }\n \n+sub validate_ref {\n+\tmy $input = shift || return undef;\n+\n+\t# restrictions on ref name according to git-check-ref-format\n+\tif ($input =~ m!(/\\.|\\.\\.|[\\000-\\040\\177 ~^:?*\\[]|/$)!) {\n+\t\treturn undef;\n+\t}\n+\treturn $input;\n+}\n+\n sub validate_refname {\n \tmy $input = shift || return undef;\n \n@@ -1462,10 +1496,9 @@ sub validate_refname {\n \t# it must be correct pathname\n \t$input = validate_pathname($input)\n \t\tor return undef;\n-\t# restrictions on ref name according to git-check-ref-format\n-\tif ($input =~ m!(/\\.|\\.\\.|[\\000-\\040\\177 ~^:?*\\[]|/$)!) {\n-\t\treturn undef;\n-\t}\n+\t# check git-check-ref-format restrictions\n+\t$input = validate_ref($input)\n+\t\tor return undef;\n \treturn $input;\n }\n \n@@ -2515,6 +2548,7 @@ sub format_snapshot_links {\n sub get_feed_info {\n \tmy $format = shift || 'Atom';\n \tmy %res = (action => lc($format));\n+\tmy $matched_ref = 0;\n \n \t# feed links are possible only for project views\n \treturn unless (defined $project);\n@@ -2522,12 +2556,17 @@ sub get_feed_info {\n \t# or don't have specific feed yet (so they should use generic)\n \treturn if (!$action || $action =~ /^(?:tags|heads|forks|tag|search)$/x);\n \n-\tmy $branch;\n-\t# branches refs uses 'refs/heads/' prefix (fullname) to differentiate\n-\t# from tag links; this also makes possible to detect branch links\n-\tif ((defined $hash_base && $hash_base =~ m!^refs/heads/(.*)$!) ||\n-\t    (defined $hash      && $hash      =~ m!^refs/heads/(.*)$!)) {\n-\t\t$branch = $1;\n+\tmy $branch = undef;\n+\t# branches refs uses 'refs/' + $get_branch_refs()[x] + '/' prefix\n+\t# (fullname) to differentiate from tag links; this also makes\n+\t# possible to detect branch links\n+\tfor my $ref (get_branch_refs()) {\n+\t\tif ((defined $hash_base && $hash_base =~ m!^refs/\\Q$ref\\E/(.*)$!) ||\n+\t\t    (defined $hash      && $hash      =~ m!^refs/\\Q$ref\\E/(.*)$!)) {\n+\t\t\t$branch = $1;\n+\t\t\t$matched_ref = $ref;\n+\t\t\tlast;\n+\t\t}\n \t}\n \t# find log type for feed description (title)\n \tmy $type = 'log';\n@@ -2540,7 +2579,7 @@ sub get_feed_info {\n \t}\n \n \t$res{-title} = $type;\n-\t$res{'hash'} = (defined $branch ? \"refs/heads/$branch\" : undef);\n+\t$res{'hash'} = (defined $branch ? \"refs/$matched_ref/$branch\" : undef);\n \t$res{'file_name'} = $file_name;\n \n \treturn %res;\n@@ -3193,7 +3232,7 @@ sub git_get_last_activity {\n \t     '--format=%(committer)',\n \t     '--sort=-committerdate',\n \t     '--count=1',\n-\t     'refs/heads') or return;\n+\t     map { \"refs/$_\" } get_branch_refs ()) or return;\n \tmy $most_recent = <$fd>;\n \tclose $fd or return;\n \tif (defined $most_recent &&\n@@ -3644,7 +3683,7 @@ sub parse_from_to_diffinfo {\n \n sub git_get_heads_list {\n \tmy ($limit, @classes) = @_;\n-\t@classes = ('heads') unless @classes;\n+\t@classes = get_branch_refs() unless @classes;\n \tmy @patterns = map { \"refs/$_\" } @classes;\n \tmy @headslist;\n \n@@ -3662,7 +3701,8 @@ sub git_get_heads_list {\n \t\tmy ($committer, $epoch, $tz) =\n \t\t\t($committerinfo =~ /^(.*) ([0-9]+) (.*)$/);\n \t\t$ref_item{'fullname'}  = $name;\n-\t\t$name =~ s!^refs/(?:head|remote)s/!!;\n+\t\tmy $strip_refs = join '|', map { quotemeta } get_branch_refs();\n+\t\t$name =~ s!^refs/(?:$strip_refs|remotes)/!!;\n \n \t\t$ref_item{'name'}  = $name;\n \t\t$ref_item{'id'}    = $hash;\n@@ -7179,7 +7219,8 @@ sub snapshot_name {\n \t\t$ver = $1;\n \t} else {\n \t\t# branches and other need shortened SHA-1 hash\n-\t\tif ($hash =~ m!^refs/(?:heads|remotes)/(.*)$!) {\n+\t\tmy $strip_refs = join '|', map { quotemeta } get_branch_refs();\n+\t\tif ($hash =~ m!^refs/(?:$strip_refs|remotes)/(.*)$!) {\n \t\t\t$ver = $1;\n \t\t}\n \t\t$ver .= '-' . git_get_short_hash($project, $hash);\n-- \n1.8.3.1\n"},{"id":"231268","messageId":"CAPig+cSjzpSatp9EhpwA9b_XScAPUpkMxmPo70qJCxxakMwq3Q@mail.gmail.com","threadId":"35419","inReplyTo":"1385639092-13362-1-git-send-email-krzesimir@endocode.com","subject":"Re: [PATCH v3] gitweb: Add an option for adding more branch refs","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-11-29T01:13:42Z","receivedAt":"2013-11-29T01:13:42Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Nov 28, 2013 at 6:44 AM, Krzesimir Nowak <krzesimir@endocode.com> wrote:\n> Allow @additional_branch_refs configuration variable to tell gitweb to\n> show refs from additional hierarchies in addition to branches in the\n> list-of-branches view.\n>\n> Signed-off-by: Krzesimir Nowak <krzesimir@endocode.com>\n> ---\n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 68c77f6..25e1d37 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -680,6 +688,19 @@ sub read_config_file {\n>         return;\n>  }\n>\n> +# performs sanity checks on parts of configuration.\n> +sub config_sanity_check {\n> +       # check additional refs validity\n> +       my %unique_branch_refs = ();\n> +       for my $ref (@additional_branch_refs) {\n> +               die_error(500, \"Invalid ref '$ref' in \\@additional_branch_refs\") unless (validate_ref($ref));\n> +               # 'heads' are added implicitly in get_branch_refs().\n> +               $unique_branch_refs{$ref} = 1 if ($ref ne 'heads');\n> +       }\n> +       @additional_branch_refs = sort keys %unique_branch_refs;\n> +       %unique_branch_refs = undef;\n> +}\n\n%unique_branch_refs is going out of scope here, so clearing it seems\nunnecessary.\n\nMoreover, with warnings enabled, perl should be complaining about an\n\"Odd number of elements in hash assignment\". (Normally, you would\nclear a hash with '%foo=()' or 'undef %foo'.)\n\n> +\n>  our ($GITWEB_CONFIG, $GITWEB_CONFIG_SYSTEM, $GITWEB_CONFIG_COMMON);\n>  sub evaluate_gitweb_config {\n>         our $GITWEB_CONFIG = $ENV{'GITWEB_CONFIG'} || \"++GITWEB_CONFIG++\";\n"},{"id":"231346","messageId":"CANQwDwfbNfbFqX+hw09bPLVKAN3RZciJmwdixzHrj89KY8FsTQ@mail.gmail.com","threadId":"35419","inReplyTo":"1385639092-13362-1-git-send-email-krzesimir@endocode.com","subject":"Re: [PATCH v3] gitweb: Add an option for adding more branch refs","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2013-12-02T00:21:14Z","receivedAt":"2013-12-02T00:21:14Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, Nov 28, 2013 at 12:44 PM, Krzesimir Nowak\n<krzesimir@endocode.com> wrote:\n\n> Allow @additional_branch_refs configuration variable to tell gitweb to\n> show refs from additional hierarchies in addition to branches in the\n> list-of-branches view.\n>\n> Signed-off-by: Krzesimir Nowak <krzesimir@endocode.com>\n\nWhy not use %feature hash instead of adding new configuration variable?\nI think that this option is similar enough to 'remote_heads' feature\n(which BTW should be 'remote-heads'), and could conceveilably enabled\non a per-repository basis, i.e. with repository configuration override,\nisn't it?\n\nUsually %feature hash is preferred over adding new configuration variable\nbut this is not some hard rule. Note however that patches adding new config\nare met with more scrutiny, as it is harder to fix mistakes because of\nrequirement of backwards compatibility of configuration files.\n\nBTW. there really should be gitweb/CodingGuidelines...\n\n> ---\n>  Documentation/gitweb.conf.txt | 13 ++++++++\n>  gitweb/gitweb.perl            | 75 +++++++++++++++++++++++++++++++++----------\n>  2 files changed, 71 insertions(+), 17 deletions(-)\n>\n> diff --git a/Documentation/gitweb.conf.txt b/Documentation/gitweb.conf.txt\n> index e2113d9..cd1a945 100644\n> --- a/Documentation/gitweb.conf.txt\n> +++ b/Documentation/gitweb.conf.txt\n> @@ -549,6 +549,19 @@ This variable matters only when using persistent web environments that\n>  serve multiple requests using single gitweb instance, like mod_perl,\n>  FastCGI or Plackup.\n>\n> +@additional_branch_refs::\n> +       List of additional directories under \"refs\" which are going to be used\n> +       as branch refs. You might want to set this variable if you have a gerrit\n> +       setup where all branches under refs/heads/ are official,\n> +       push-after-review ones and branches under refs/sandbox/, refs/wip and\n> +       refs/other are user ones where permissions are much wider, for example\n> ++\n> +--------------------------------------------------------------------------------\n> +our @additional_branch_refs = ('sandbox', 'wip', 'other');\n> +--------------------------------------------------------------------------------\n\nI think the last (long) sentence would better read if it began with \"For example\nif you have... then you could set this variable to ...\", IMVHO.\n\nBTW. if we decide on using %feature hash instead, it would be in the\n\"CONFIGURING GITWEB FEATURES\" section.\n\n> ++\n> +It is an error to specify a ref that does not pass \"git check-ref-format\"\n> +scrutiny.\n\nHmmm... One one hand erroring out on invalid refs means that we can\nfind error in config earlier and easier, on the other hand ignoring invalid\nrefs would make it resilent to errors in gitweb config (and repository config,\nif we use %feature with per-repository override).\n\n\n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n[...]\n> @@ -626,6 +630,10 @@ sub feature_avatar {\n>         return @val ? @val : @_;\n>  }\n>\n> +sub get_branch_refs {\n> +    return ('heads', @additional_branch_refs);\n> +}\n\nNice way of ensuring that list of all branches starts with \"heads\".\n\n>  # checking HEAD file with -e is fragile if the repository was\n>  # initialized long time ago (i.e. symlink HEAD) and was pack-ref'ed\n>  # and then pruned.\n> @@ -680,6 +688,19 @@ sub read_config_file {\n>         return;\n>  }\n>\n> +# performs sanity checks on parts of configuration.\n> +sub config_sanity_check {\n> +       # check additional refs validity\n> +       my %unique_branch_refs = ();\n> +       for my $ref (@additional_branch_refs) {\n> +               die_error(500, \"Invalid ref '$ref' in \\@additional_branch_refs\") unless (validate_ref($ref));\n> +               # 'heads' are added implicitly in get_branch_refs().\n> +               $unique_branch_refs{$ref} = 1 if ($ref ne 'heads');\n> +       }\n> +       @additional_branch_refs = sort keys %unique_branch_refs;\n> +       %unique_branch_refs = undef;\n> +}\n\nThis subroutine is quite similar to filter_snapshot_fmts for 'snapshot'\nfeature, perhaps the name should be patterned after it, i.e.\nfilter_branch_refs() or something...\n\nIf there were generic config_sanity_check(), it would call filter_branch_refs().\n\n> @@ -698,8 +719,11 @@ sub evaluate_gitweb_config {\n>\n>         # Use first config file that exists.  This means use the per-instance\n>         # GITWEB_CONFIG if exists, otherwise use GITWEB_SYSTEM_CONFIG.\n> -       read_config_file($GITWEB_CONFIG) and return;\n> -       read_config_file($GITWEB_CONFIG_SYSTEM);\n> +       if (!read_config_file($GITWEB_CONFIG)) {\n> +               read_config_file($GITWEB_CONFIG_SYSTEM);\n> +       }\n> +\n> +       config_sanity_check();\n>  }\n\nI'm not sure if evaluate_gitweb_config is best place for sanity check\nof said gitweb config, and not e.g. in run_request()... though having\nit there has its own advantages.\n\nBTW. it can be written as:\n\n  -       read_config_file($GITWEB_CONFIG) and return;\n  -       read_config_file($GITWEB_CONFIG_SYSTEM);\n  +      read_config_file($GITWEB_CONFIG) or\n  +      read_config_file($GITWEB_CONFIG_SYSTEM);\n  +\n  +       config_sanity_check();\n\n\nAnyway if we were to use %feature hash, there is configure_gitweb_features()\nfor calling filter_branch_refs().\n\n>  # Get loadavg of system, to compare against $maxload.\n> @@ -1452,6 +1476,16 @@ sub validate_pathname {\n>         return $input;\n>  }\n>\n> +sub validate_ref {\n> +       my $input = shift || return undef;\n> +\n> +       # restrictions on ref name according to git-check-ref-format\n> +       if ($input =~ m!(/\\.|\\.\\.|[\\000-\\040\\177 ~^:?*\\[]|/$)!) {\n> +               return undef;\n> +       }\n> +       return $input;\n> +}\n> +\n>  sub validate_refname {\n>         my $input = shift || return undef;\n\nHmmm... validate_ref() is IMHO too similar to validate_refname(),\nand it isn't about *parameter* validation. Perhaps check_ref_format()?\n\n> @@ -1462,10 +1496,9 @@ sub validate_refname {\n>         # it must be correct pathname\n>         $input = validate_pathname($input)\n>                 or return undef;\n> -       # restrictions on ref name according to git-check-ref-format\n> -       if ($input =~ m!(/\\.|\\.\\.|[\\000-\\040\\177 ~^:?*\\[]|/$)!) {\n> -               return undef;\n> -       }\n> +       # check git-check-ref-format restrictions\n> +       $input = validate_ref($input)\n> +               or return undef;\n>         return $input;\n>  }\n\nNice refactoring (it *could*, but doesn't need to, be in separate patch).\n\n> @@ -2515,6 +2548,7 @@ sub format_snapshot_links {\n>  sub get_feed_info {\n>         my $format = shift || 'Atom';\n>         my %res = (action => lc($format));\n> +       my $matched_ref = 0;\n>\n>         # feed links are possible only for project views\n>         return unless (defined $project);\n> @@ -2522,12 +2556,17 @@ sub get_feed_info {\n>         # or don't have specific feed yet (so they should use generic)\n>         return if (!$action || $action =~ /^(?:tags|heads|forks|tag|search)$/x);\n>\n> -       my $branch;\n> -       # branches refs uses 'refs/heads/' prefix (fullname) to differentiate\n> -       # from tag links; this also makes possible to detect branch links\n> -       if ((defined $hash_base && $hash_base =~ m!^refs/heads/(.*)$!) ||\n> -           (defined $hash      && $hash      =~ m!^refs/heads/(.*)$!)) {\n> -               $branch = $1;\n> +       my $branch = undef;\n> +       # branches refs uses 'refs/' + $get_branch_refs()[x] + '/' prefix\n> +       # (fullname) to differentiate from tag links; this also makes\n> +       # possible to detect branch links\n> +       for my $ref (get_branch_refs()) {\n> +               if ((defined $hash_base && $hash_base =~ m!^refs/\\Q$ref\\E/(.*)$!) ||\n> +                   (defined $hash      && $hash      =~ m!^refs/\\Q$ref\\E/(.*)$!)) {\n> +                       $branch = $1;\n> +                       $matched_ref = $ref;\n> +                       last;\n> +               }\n>         }\n\nNice!\n\n> @@ -3662,7 +3701,8 @@ sub git_get_heads_list {\n>                 my ($committer, $epoch, $tz) =\n>                         ($committerinfo =~ /^(.*) ([0-9]+) (.*)$/);\n>                 $ref_item{'fullname'}  = $name;\n> -               $name =~ s!^refs/(?:head|remote)s/!!;\n> +               my $strip_refs = join '|', map { quotemeta } get_branch_refs();\n> +               $name =~ s!^refs/(?:$strip_refs|remotes)/!!;\n>\n>                 $ref_item{'name'}  = $name;\n>                 $ref_item{'id'}    = $hash;\n> @@ -7179,7 +7219,8 @@ sub snapshot_name {\n>                 $ver = $1;\n>         } else {\n>                 # branches and other need shortened SHA-1 hash\n> -               if ($hash =~ m!^refs/(?:heads|remotes)/(.*)$!) {\n> +               my $strip_refs = join '|', map { quotemeta } get_branch_refs();\n> +               if ($hash =~ m!^refs/(?:$strip_refs|remotes)/(.*)$!) {\n>                         $ver = $1;\n>                 }\n>                 $ver .= '-' . git_get_short_hash($project, $hash);\n\nOne one hand, it is about threating extra branch refs the same way as 'head'.\nOn the other hand we loose distinction between 'refs/heads/foo' and e.g.\n'refs/wip/foo'. But maybe that's all right...\n\n-- \nJakub Narebski\n"},{"id":"231360","messageId":"1385979199.2054.2.camel@localhost.localdomain","threadId":"35419","inReplyTo":"CAPig+cSjzpSatp9EhpwA9b_XScAPUpkMxmPo70qJCxxakMwq3Q@mail.gmail.com","subject":"Re: [PATCH v3] gitweb: Add an option for adding more branch refs","fromName":"Krzesimir Nowak","fromEmail":"krzesimir@endocode.com","sentAt":"2013-12-02T10:13:19Z","receivedAt":"2013-12-02T10:13:19Z","isPatch":true,"sender":{"key":"krzesimir@endocode.com","avatar":"https://avatars.githubusercontent.com/u/96081?v=4"},"body":"On Thu, 2013-11-28 at 20:13 -0500, Eric Sunshine wrote:\n> On Thu, Nov 28, 2013 at 6:44 AM, Krzesimir Nowak <krzesimir@endocode.com> wrote:\n> > Allow @additional_branch_refs configuration variable to tell gitweb to\n> > show refs from additional hierarchies in addition to branches in the\n> > list-of-branches view.\n> >\n> > Signed-off-by: Krzesimir Nowak <krzesimir@endocode.com>\n> > ---\n> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> > index 68c77f6..25e1d37 100755\n> > --- a/gitweb/gitweb.perl\n> > +++ b/gitweb/gitweb.perl\n> > @@ -680,6 +688,19 @@ sub read_config_file {\n> >         return;\n> >  }\n> >\n> > +# performs sanity checks on parts of configuration.\n> > +sub config_sanity_check {\n> > +       # check additional refs validity\n> > +       my %unique_branch_refs = ();\n> > +       for my $ref (@additional_branch_refs) {\n> > +               die_error(500, \"Invalid ref '$ref' in \\@additional_branch_refs\") unless (validate_ref($ref));\n> > +               # 'heads' are added implicitly in get_branch_refs().\n> > +               $unique_branch_refs{$ref} = 1 if ($ref ne 'heads');\n> > +       }\n> > +       @additional_branch_refs = sort keys %unique_branch_refs;\n> > +       %unique_branch_refs = undef;\n> > +}\n> \n> %unique_branch_refs is going out of scope here, so clearing it seems\n> unnecessary.\n\nI am cleaning it in case when more sanity checking code gets added. So\nthere is no need to keep the data further. \n\n> \n> Moreover, with warnings enabled, perl should be complaining about an\n> \"Odd number of elements in hash assignment\". (Normally, you would\n> clear a hash with '%foo=()' or 'undef %foo'.)\n> \n\nGah, ok. I'll fix it. Thanks.\n\n> > +\n> >  our ($GITWEB_CONFIG, $GITWEB_CONFIG_SYSTEM, $GITWEB_CONFIG_COMMON);\n> >  sub evaluate_gitweb_config {\n> >         our $GITWEB_CONFIG = $ENV{'GITWEB_CONFIG'} || \"++GITWEB_CONFIG++\";\n\n-- \nKrzesimir Nowak\nSoftware Developer\nEndocode AG\n\nkrzesimir@endocode.com\n\n------\nEndocode AG, Johannisstraße 20, 10117 Berlin\ninfo@endocode.com | www.endocode.com\n\nVorstandsvorsitzender: Mirko Boehm\nVorstände: Dr. Karl Beecher, Chris Kühl, Sebastian Sucker\nAufsichtsratsvorsitzende: Jennifer Beecher\n\nRegistergericht: Amtsgericht Charlottenburg - HRB 150748 B\n"},{"id":"231365","messageId":"1385985997.2054.27.camel@localhost.localdomain","threadId":"35419","inReplyTo":"CANQwDwfbNfbFqX+hw09bPLVKAN3RZciJmwdixzHrj89KY8FsTQ@mail.gmail.com","subject":"Re: [PATCH v3] gitweb: Add an option for adding more branch refs","fromName":"Krzesimir Nowak","fromEmail":"krzesimir@endocode.com","sentAt":"2013-12-02T12:06:37Z","receivedAt":"2013-12-02T12:06:37Z","isPatch":true,"sender":{"key":"krzesimir@endocode.com","avatar":"https://avatars.githubusercontent.com/u/96081?v=4"},"body":"On Mon, 2013-12-02 at 01:21 +0100, Jakub Narębski wrote:\n> On Thu, Nov 28, 2013 at 12:44 PM, Krzesimir Nowak\n> <krzesimir@endocode.com> wrote:\n> \n> > Allow @additional_branch_refs configuration variable to tell gitweb to\n> > show refs from additional hierarchies in addition to branches in the\n> > list-of-branches view.\n> >\n> > Signed-off-by: Krzesimir Nowak <krzesimir@endocode.com>\n> \n> Why not use %feature hash instead of adding new configuration variable?\n> I think that this option is similar enough to 'remote_heads' feature\n> (which BTW should be 'remote-heads'), and could conceveilably enabled\n> on a per-repository basis, i.e. with repository configuration override,\n> isn't it?\n\nI'd like to see some consensus on it before I start changing the patch\nagain.\n\n> \n> Usually %feature hash is preferred over adding new configuration variable\n> but this is not some hard rule. Note however that patches adding new config\n> are met with more scrutiny, as it is harder to fix mistakes because of\n> requirement of backwards compatibility of configuration files.\n> \n\nI don't know what kind of backwards compatibility you mention. Whether\nyou want gitweb to survive reading old config file or to honor\ndeprecated/old config variables. If the former than I have already read\nsomewhere that you always should use config vars like:\nour $config = 'value';\nNote the 'our' which avoids gitweb failures in case of config variable\nremoval.\n\n> BTW. there really should be gitweb/CodingGuidelines...\n> \n\nYes, would be useful. As in every other project. :)\n\n> > ---\n> >  Documentation/gitweb.conf.txt | 13 ++++++++\n> >  gitweb/gitweb.perl            | 75 +++++++++++++++++++++++++++++++++----------\n> >  2 files changed, 71 insertions(+), 17 deletions(-)\n> >\n> > diff --git a/Documentation/gitweb.conf.txt b/Documentation/gitweb.conf.txt\n> > index e2113d9..cd1a945 100644\n> > --- a/Documentation/gitweb.conf.txt\n> > +++ b/Documentation/gitweb.conf.txt\n> > @@ -549,6 +549,19 @@ This variable matters only when using persistent web environments that\n> >  serve multiple requests using single gitweb instance, like mod_perl,\n> >  FastCGI or Plackup.\n> >\n> > +@additional_branch_refs::\n> > +       List of additional directories under \"refs\" which are going to be used\n> > +       as branch refs. You might want to set this variable if you have a gerrit\n> > +       setup where all branches under refs/heads/ are official,\n> > +       push-after-review ones and branches under refs/sandbox/, refs/wip and\n> > +       refs/other are user ones where permissions are much wider, for example\n> > ++\n> > +--------------------------------------------------------------------------------\n> > +our @additional_branch_refs = ('sandbox', 'wip', 'other');\n> > +--------------------------------------------------------------------------------\n> \n> I think the last (long) sentence would better read if it began with \"For example\n> if you have... then you could set this variable to ...\", IMVHO.\n> \n\nRight, thanks. Will rephrase it.\n\n> BTW. if we decide on using %feature hash instead, it would be in the\n> \"CONFIGURING GITWEB FEATURES\" section.\n\nYes, but I'll wait for some consensus with it.\n\n> \n> > ++\n> > +It is an error to specify a ref that does not pass \"git check-ref-format\"\n> > +scrutiny.\n> \n> Hmmm... One one hand erroring out on invalid refs means that we can\n> find error in config earlier and easier, on the other hand ignoring invalid\n> refs would make it resilent to errors in gitweb config (and repository config,\n> if we use %feature with per-repository override).\n> \n\nWe could ignore bad values, but that would make it harder to find out\nwhat exactly is wrong when something we configured to be shown is not\nshown at all.\n\n> \n> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> [...]\n> > @@ -626,6 +630,10 @@ sub feature_avatar {\n> >         return @val ? @val : @_;\n> >  }\n> >\n> > +sub get_branch_refs {\n> > +    return ('heads', @additional_branch_refs);\n> > +}\n> \n> Nice way of ensuring that list of all branches starts with \"heads\".\n> \n> >  # checking HEAD file with -e is fragile if the repository was\n> >  # initialized long time ago (i.e. symlink HEAD) and was pack-ref'ed\n> >  # and then pruned.\n> > @@ -680,6 +688,19 @@ sub read_config_file {\n> >         return;\n> >  }\n> >\n> > +# performs sanity checks on parts of configuration.\n> > +sub config_sanity_check {\n> > +       # check additional refs validity\n> > +       my %unique_branch_refs = ();\n> > +       for my $ref (@additional_branch_refs) {\n> > +               die_error(500, \"Invalid ref '$ref' in \\@additional_branch_refs\") unless (validate_ref($ref));\n> > +               # 'heads' are added implicitly in get_branch_refs().\n> > +               $unique_branch_refs{$ref} = 1 if ($ref ne 'heads');\n> > +       }\n> > +       @additional_branch_refs = sort keys %unique_branch_refs;\n> > +       %unique_branch_refs = undef;\n> > +}\n> \n> This subroutine is quite similar to filter_snapshot_fmts for 'snapshot'\n> feature, perhaps the name should be patterned after it, i.e.\n> filter_branch_refs() or something...\n> \n> If there were generic config_sanity_check(), it would call filter_branch_refs().\n\nI had an additional_branches_refs_check() sub which was called by\nconfig_sanity_check(), but I scrapped it. I wanted config_sanity_check()\nto be general configuration checker, but for now it would only call a\nsingle function, so I inlined it. If later more configuration checking\nwill be added then the current body could be moved to separate sub.\n\nI can move it back to separate sub if you want.\n\n> \n> > @@ -698,8 +719,11 @@ sub evaluate_gitweb_config {\n> >\n> >         # Use first config file that exists.  This means use the per-instance\n> >         # GITWEB_CONFIG if exists, otherwise use GITWEB_SYSTEM_CONFIG.\n> > -       read_config_file($GITWEB_CONFIG) and return;\n> > -       read_config_file($GITWEB_CONFIG_SYSTEM);\n> > +       if (!read_config_file($GITWEB_CONFIG)) {\n> > +               read_config_file($GITWEB_CONFIG_SYSTEM);\n> > +       }\n> > +\n> > +       config_sanity_check();\n> >  }\n> \n> I'm not sure if evaluate_gitweb_config is best place for sanity check\n> of said gitweb config, and not e.g. in run_request()... though having\n> it there has its own advantages.\n> \n> BTW. it can be written as:\n> \n>   -       read_config_file($GITWEB_CONFIG) and return;\n>   -       read_config_file($GITWEB_CONFIG_SYSTEM);\n>   +      read_config_file($GITWEB_CONFIG) or\n>   +      read_config_file($GITWEB_CONFIG_SYSTEM);\n>   +\n>   +       config_sanity_check();\n> \n\nOk, will rewrite it.\n\n> \n> Anyway if we were to use %feature hash, there is configure_gitweb_features()\n> for calling filter_branch_refs().\n> \n> >  # Get loadavg of system, to compare against $maxload.\n> > @@ -1452,6 +1476,16 @@ sub validate_pathname {\n> >         return $input;\n> >  }\n> >\n> > +sub validate_ref {\n> > +       my $input = shift || return undef;\n> > +\n> > +       # restrictions on ref name according to git-check-ref-format\n> > +       if ($input =~ m!(/\\.|\\.\\.|[\\000-\\040\\177 ~^:?*\\[]|/$)!) {\n> > +               return undef;\n> > +       }\n> > +       return $input;\n> > +}\n> > +\n> >  sub validate_refname {\n> >         my $input = shift || return undef;\n> \n> Hmmm... validate_ref() is IMHO too similar to validate_refname(),\n> and it isn't about *parameter* validation. Perhaps check_ref_format()?\n\nOk.\n\n> \n> > @@ -1462,10 +1496,9 @@ sub validate_refname {\n> >         # it must be correct pathname\n> >         $input = validate_pathname($input)\n> >                 or return undef;\n> > -       # restrictions on ref name according to git-check-ref-format\n> > -       if ($input =~ m!(/\\.|\\.\\.|[\\000-\\040\\177 ~^:?*\\[]|/$)!) {\n> > -               return undef;\n> > -       }\n> > +       # check git-check-ref-format restrictions\n> > +       $input = validate_ref($input)\n> > +               or return undef;\n> >         return $input;\n> >  }\n> \n> Nice refactoring (it *could*, but doesn't need to, be in separate patch).\n> \n> > @@ -2515,6 +2548,7 @@ sub format_snapshot_links {\n> >  sub get_feed_info {\n> >         my $format = shift || 'Atom';\n> >         my %res = (action => lc($format));\n> > +       my $matched_ref = 0;\n> >\n> >         # feed links are possible only for project views\n> >         return unless (defined $project);\n> > @@ -2522,12 +2556,17 @@ sub get_feed_info {\n> >         # or don't have specific feed yet (so they should use generic)\n> >         return if (!$action || $action =~ /^(?:tags|heads|forks|tag|search)$/x);\n> >\n> > -       my $branch;\n> > -       # branches refs uses 'refs/heads/' prefix (fullname) to differentiate\n> > -       # from tag links; this also makes possible to detect branch links\n> > -       if ((defined $hash_base && $hash_base =~ m!^refs/heads/(.*)$!) ||\n> > -           (defined $hash      && $hash      =~ m!^refs/heads/(.*)$!)) {\n> > -               $branch = $1;\n> > +       my $branch = undef;\n> > +       # branches refs uses 'refs/' + $get_branch_refs()[x] + '/' prefix\n> > +       # (fullname) to differentiate from tag links; this also makes\n> > +       # possible to detect branch links\n> > +       for my $ref (get_branch_refs()) {\n> > +               if ((defined $hash_base && $hash_base =~ m!^refs/\\Q$ref\\E/(.*)$!) ||\n> > +                   (defined $hash      && $hash      =~ m!^refs/\\Q$ref\\E/(.*)$!)) {\n> > +                       $branch = $1;\n> > +                       $matched_ref = $ref;\n> > +                       last;\n> > +               }\n> >         }\n> \n> Nice!\n> \n> > @@ -3662,7 +3701,8 @@ sub git_get_heads_list {\n> >                 my ($committer, $epoch, $tz) =\n> >                         ($committerinfo =~ /^(.*) ([0-9]+) (.*)$/);\n> >                 $ref_item{'fullname'}  = $name;\n> > -               $name =~ s!^refs/(?:head|remote)s/!!;\n> > +               my $strip_refs = join '|', map { quotemeta } get_branch_refs();\n> > +               $name =~ s!^refs/(?:$strip_refs|remotes)/!!;\n> >\n> >                 $ref_item{'name'}  = $name;\n> >                 $ref_item{'id'}    = $hash;\n> > @@ -7179,7 +7219,8 @@ sub snapshot_name {\n> >                 $ver = $1;\n> >         } else {\n> >                 # branches and other need shortened SHA-1 hash\n> > -               if ($hash =~ m!^refs/(?:heads|remotes)/(.*)$!) {\n> > +               my $strip_refs = join '|', map { quotemeta } get_branch_refs();\n> > +               if ($hash =~ m!^refs/(?:$strip_refs|remotes)/(.*)$!) {\n> >                         $ver = $1;\n> >                 }\n> >                 $ver .= '-' . git_get_short_hash($project, $hash);\n> \n> One one hand, it is about threating extra branch refs the same way as 'head'.\n> On the other hand we loose distinction between 'refs/heads/foo' and e.g.\n> 'refs/wip/foo'. But maybe that's all right...\n> \n\nIn git_get_heads_list sub I could append a \" ($ref_dir)\" to refs which\nare in neither 'heads' nor 'remotes', so heads view would look like:\nmaster\nold-stable\nsome-work-in-progress (wip)\nsome-other-branch (other) \n\nwhere both master and old-stable are in refs/heads/,\nsome-work-in-progress in refs/wip/ and some-other-branch in refs/other/.\n\nIn case of branch snapshot names (snapshot_name sub) I could change it,\nso names for branches mentioned above would be\n\"Project-master-<short-hash>.tgz\",\n\"Project-old_stable-<short-hash>.tgz\",\n\"Project-wip-some-work-in-progress-<short-hash>.tgz\"\n\"Project-other-some-other-branch-<short-hash>.tgz\"\n\nWhat do you think?\n\nCheers,\n-- \nKrzesimir Nowak\nSoftware Developer\nEndocode AG\n\nkrzesimir@endocode.com\n\n------\nEndocode AG, Johannisstraße 20, 10117 Berlin\ninfo@endocode.com | www.endocode.com\n\nVorstandsvorsitzender: Mirko Boehm\nVorstände: Dr. Karl Beecher, Chris Kühl, Sebastian Sucker\nAufsichtsratsvorsitzende: Jennifer Beecher\n\nRegistergericht: Amtsgericht Charlottenburg - HRB 150748 B\n"},{"id":"231376","messageId":"529CC48C.5080902@gmail.com","threadId":"35419","inReplyTo":"1385985997.2054.27.camel@localhost.localdomain","subject":"Re: [PATCH v3] gitweb: Add an option for adding more branch refs","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2013-12-02T17:34:04Z","receivedAt":"2013-12-02T17:34:04Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"W dniu 2013-12-02 13:06, Krzesimir Nowak pisze:\n> On Mon, 2013-12-02 at 01:21 +0100, Jakub Narębski wrote:\n>> On Thu, Nov 28, 2013 at 12:44 PM, Krzesimir Nowak\n>> <krzesimir@endocode.com>  wrote:\n>>\n>>> Allow @additional_branch_refs configuration variable to tell gitweb to\n>>> show refs from additional hierarchies in addition to branches in the\n>>> list-of-branches view.\n>>>\n>>> Signed-off-by: Krzesimir Nowak<krzesimir@endocode.com>\n>>\n>> Why not use %feature hash instead of adding new configuration variable?\n>> I think that this option is similar enough to 'remote_heads' feature\n>> (which BTW should be 'remote-heads'), and could conceivably enabled\n>> on a per-repository basis, i.e. with repository configuration override,\n>> isn't it?\n>\n> I'd like to see some consensus on it before I start changing the patch\n> again.\n\n%feature hash is mainly (but not only) about options that can be\nconfigured on per-repository basis.  Configuration variables are\nabout options that are per-instance (per gitweb).\n\n>> Usually %feature hash is preferred over adding new configuration variable\n>> but this is not some hard rule. Note however that patches adding new config\n>> are met with more scrutiny, as it is harder to fix mistakes because of\n>> requirement of backwards compatibility of configuration files.\n>>\n>\n> I don't know what kind of backwards compatibility you mention. Whether\n> you want gitweb to survive reading old config file or to honor\n> deprecated/old config variables.\n\nI meant here honoring deprecated/old variables, i.e. honoring existing\nconfiguration files.  See for example backward compatibility for old\n$stylesheet variable vs new @stylesheets in print_header_links().\n\nThough in this case it shouldn't be much of a problem; it would be\neasy to honor @additional_branch_refs by setting 'default' for\n'extra-branch-refs' feature to it.\n\n>> BTW. there really should be gitweb/CodingGuidelines...\n>>\n>\n> Yes, would be useful. As in every other project. :)\n\nWell, Git itself *has* Documentation/CodingGuidelines, but perhaps\ngitweb subsystem should have it's own...\n\n[...]\n>>> @@ -3662,7 +3701,8 @@ sub git_get_heads_list {\n>>>                  my ($committer, $epoch, $tz) =\n>>>                          ($committerinfo =~ /^(.*) ([0-9]+) (.*)$/);\n>>>                  $ref_item{'fullname'}  = $name;\n>>> -               $name =~ s!^refs/(?:head|remote)s/!!;\n>>> +               my $strip_refs = join '|', map { quotemeta } get_branch_refs();\n>>> +               $name =~ s!^refs/(?:$strip_refs|remotes)/!!;\n>>>\n>>>                  $ref_item{'name'}  = $name;\n>>>                  $ref_item{'id'}    = $hash;\n>>> @@ -7179,7 +7219,8 @@ sub snapshot_name {\n>>>                  $ver = $1;\n>>>          } else {\n>>>                  # branches and other need shortened SHA-1 hash\n>>> -               if ($hash =~ m!^refs/(?:heads|remotes)/(.*)$!) {\n>>> +               my $strip_refs = join '|', map { quotemeta } get_branch_refs();\n>>> +               if ($hash =~ m!^refs/(?:$strip_refs|remotes)/(.*)$!) {\n>>>                          $ver = $1;\n>>>                  }\n>>>                  $ver .= '-' . git_get_short_hash($project, $hash);\n>>\n>> One one hand, it is about threating extra branch refs the same way as 'head'.\n>> On the other hand we loose distinction between 'refs/heads/foo' and e.g.\n>> 'refs/wip/foo'. But maybe that's all right...\n>>\n>\n> In git_get_heads_list sub I could append a \" ($ref_dir)\" to refs which\n> are in neither 'heads' nor 'remotes', so heads view would look like:\n> master\n> old-stable\n> some-work-in-progress (wip)\n> some-other-branch (other)\n>\n> where both master and old-stable are in refs/heads/,\n> some-work-in-progress in refs/wip/ and some-other-branch in refs/other/.\n>\n> In case of branch snapshot names (snapshot_name sub) I could change it,\n> so names for branches mentioned above would be\n> \"Project-master-<short-hash>.tgz\",\n> \"Project-old_stable-<short-hash>.tgz\",\n> \"Project-wip-some-work-in-progress-<short-hash>.tgz\"\n> \"Project-other-some-other-branch-<short-hash>.tgz\"\n>\n> What do you think?\n\nThat is, I think, a very good idea.  Though perhaps it would be more \nreadable to add this extra feature as a separate patch, on top of main one.\n\n-- \nJakub Narebski\n"},{"id":"231378","messageId":"xmqq61r7rua0.fsf@gitster.dls.corp.google.com","threadId":"35419","inReplyTo":"1385985997.2054.27.camel@localhost.localdomain","subject":"Re: [PATCH v3] gitweb: Add an option for adding more branch refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-02T18:18:31Z","receivedAt":"2013-12-02T18:18:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Krzesimir Nowak <krzesimir@endocode.com> writes:\n\n> On Mon, 2013-12-02 at 01:21 +0100, Jakub Narębski wrote:\n>> On Thu, Nov 28, 2013 at 12:44 PM, Krzesimir Nowak\n>> <krzesimir@endocode.com> wrote:\n>> \n>> > Allow @additional_branch_refs configuration variable to tell gitweb to\n>> > show refs from additional hierarchies in addition to branches in the\n>> > list-of-branches view.\n>> >\n>> > Signed-off-by: Krzesimir Nowak <krzesimir@endocode.com>\n>> \n>> Why not use %feature hash instead of adding new configuration variable?\n>> I think that this option is similar enough to 'remote_heads' feature\n>> (which BTW should be 'remote-heads'), and could conceveilably enabled\n>> on a per-repository basis, i.e. with repository configuration override,\n>> isn't it?\n>\n> I'd like to see some consensus on it before I start changing the patch\n> again.\n\nI missed the remote-heads which is an existing feature when I\ncommented; if this can be exposed to the users as an extension to\nit like Jakub suggests, it may be a better direction.\n\n>> Usually %feature hash is preferred over adding new configuration variable\n>> but this is not some hard rule. Note however that patches adding new config\n>> are met with more scrutiny, as it is harder to fix mistakes because of\n>> requirement of backwards compatibility of configuration files.\n>\n> I don't know what kind of backwards compatibility you mention.\n\nA patch adds new feature controlled by a configuration in one way\n(e.g. as a totally new ad-hoc switch that is seemingly orthogonal to\nall the existing features), people start using the feature using\nthat configuration method, and then later we find out that it is\nbetter controlled by a different way (e.g. as a natural extension to\nan existing feature, controlled by how the existing feature is\ncontrolled, perhaps after extending it) because it allows more\nflexibility in the future.\n\nAt that point, we can extend the way the existing feature is\ncontrolled to trigger the new feature in a more uniform way, but we\ncannot remove the new ad-hoc switch the patch originally added to\ncontrol this new feature because there already are people who are\nusing it, and we end up having to support the unified and extensible\nway to configure, which we prefer to keep in the longer term, and\nalso the ad-hoc switch the patch added, which we wish we never had\nin the first place.  The latter needs to be deprecated and removed\nover time.\n"},{"id":"231379","messageId":"CANQwDwe827gsq4pP_XN_ZUSxTJhZupijuVK0gwVJToNcEXDueg@mail.gmail.com","threadId":"35419","inReplyTo":"xmqq61r7rua0.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3] gitweb: Add an option for adding more branch refs","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2013-12-02T18:25:07Z","receivedAt":"2013-12-02T18:25:07Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Mon, Dec 2, 2013 at 7:18 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Krzesimir Nowak <krzesimir@endocode.com> writes:\n>> On Mon, 2013-12-02 at 01:21 +0100, Jakub Narębski wrote:\n>>> On Thu, Nov 28, 2013 at 12:44 PM, Krzesimir Nowak\n>>> <krzesimir@endocode.com> wrote:\n>>>\n>>> > Allow @additional_branch_refs configuration variable to tell gitweb to\n>>> > show refs from additional hierarchies in addition to branches in the\n>>> > list-of-branches view.\n>>> >\n>>> > Signed-off-by: Krzesimir Nowak <krzesimir@endocode.com>\n>>>\n>>> Why not use %feature hash instead of adding new configuration variable?\n>>> I think that this option is similar enough to 'remote_heads' feature\n>>> (which BTW should be 'remote-heads'), and could conceveilably enabled\n>>> on a per-repository basis, i.e. with repository configuration override,\n>>> isn't it?\n>>\n>> I'd like to see some consensus on it before I start changing the patch\n>> again.\n>\n> I missed the remote-heads which is an existing feature when I\n> commented; if this can be exposed to the users as an extension to\n> it like Jakub suggests, it may be a better direction.\n\nI think that additional-branch-refs (or just branch-refs) is different enough\nfrom remote_heads feature (which is about showing \"remotes\" section\nin \"summary\" view, and IIRC adding \"remotes\" view) that it should be\nkept separate.\n\nOn the other hand it is similar enough that I think style of implementation\nshould also be similar, i.e. use %feature hash.\n\nIMVHO\n\n-- \nJakub Narebski\n"},{"id":"231391","messageId":"xmqqbo0zq7tc.fsf@gitster.dls.corp.google.com","threadId":"35419","inReplyTo":"CANQwDwe827gsq4pP_XN_ZUSxTJhZupijuVK0gwVJToNcEXDueg@mail.gmail.com","subject":"Re: [PATCH v3] gitweb: Add an option for adding more branch refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-02T21:09:03Z","receivedAt":"2013-12-02T21:09:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narębski <jnareb@gmail.com> writes:\n\n> I think that additional-branch-refs (or just branch-refs) is different enough\n> from remote_heads feature (which is about showing \"remotes\" section\n> in \"summary\" view, and IIRC adding \"remotes\" view) that it should be\n> kept separate.\n>\n> On the other hand it is similar enough that I think style of implementation\n> should also be similar, i.e. use %feature hash.\n\nSounds sensible.  Thanks.\n"},{"id":"231423","messageId":"1386068026.2208.16.camel@localhost.localdomain","threadId":"35419","inReplyTo":"529CC48C.5080902@gmail.com","subject":"Re: [PATCH v3] gitweb: Add an option for adding more branch refs","fromName":"Krzesimir Nowak","fromEmail":"krzesimir@endocode.com","sentAt":"2013-12-03T10:53:46Z","receivedAt":"2013-12-03T10:53:46Z","isPatch":true,"sender":{"key":"krzesimir@endocode.com","avatar":"https://avatars.githubusercontent.com/u/96081?v=4"},"body":"On Mon, 2013-12-02 at 18:34 +0100, Jakub Narębski wrote:\n> W dniu 2013-12-02 13:06, Krzesimir Nowak pisze:\n> > On Mon, 2013-12-02 at 01:21 +0100, Jakub Narębski wrote:\n> >> On Thu, Nov 28, 2013 at 12:44 PM, Krzesimir Nowak\n> >> <krzesimir@endocode.com>  wrote:\n> >>\n> >>> Allow @additional_branch_refs configuration variable to tell gitweb to\n> >>> show refs from additional hierarchies in addition to branches in the\n> >>> list-of-branches view.\n> >>>\n> >>> Signed-off-by: Krzesimir Nowak<krzesimir@endocode.com>\n> >>\n> >> Why not use %feature hash instead of adding new configuration variable?\n> >> I think that this option is similar enough to 'remote_heads' feature\n> >> (which BTW should be 'remote-heads'), and could conceivably enabled\n> >> on a per-repository basis, i.e. with repository configuration override,\n> >> isn't it?\n> >\n> > I'd like to see some consensus on it before I start changing the patch\n> > again.\n> \n> %feature hash is mainly (but not only) about options that can be\n> configured on per-repository basis.  Configuration variables are\n> about options that are per-instance (per gitweb).\n\nWell, I am mostly interested in per-instance configuration in this case,\nbut if that is also possible with %feature hash, then ok, I'll try to\nmake it work.\n\nFrom what I've seen (correct me please if I got it wrong) feature\nsettings is taken from per-repository config file from [gitweb] section.\nIf there's nothing then some default value is taken. That default value\ncan be overriden with per-instance perl config file.\n\nSo it is easy to override it from per-instance perl config by typing:\n$feature{'additional-branch-refs'}{'default'} = ['wip', 'no|tf\"un,ny'];\n$feature{'additional-branch-refs'}{'override'} = 1;\n\n(Note the edge case of refs/no|tf\"un,ny, which passes the git\ncheck-ref-format scrutiny.)\n\nBut for now, most of features are quite simple - either booleans,\nintegers or list of simple strings (in snapshot feature). What I need\nhere is a list of strings, like CSV in following example:\n[gitweb]\n\tadditional_branch_refs = wip,\"no|tf\"\"un,ny\"\n\nIs dependency on external module like Text::CSV or Text::CSV_XS ok? If\nnot, I can hack some CSV reading code.\n\n> \n> >> Usually %feature hash is preferred over adding new configuration variable\n> >> but this is not some hard rule. Note however that patches adding new config\n> >> are met with more scrutiny, as it is harder to fix mistakes because of\n> >> requirement of backwards compatibility of configuration files.\n> >>\n> >\n> > I don't know what kind of backwards compatibility you mention. Whether\n> > you want gitweb to survive reading old config file or to honor\n> > deprecated/old config variables.\n> \n> I meant here honoring deprecated/old variables, i.e. honoring existing\n> configuration files.  See for example backward compatibility for old\n> $stylesheet variable vs new @stylesheets in print_header_links().\n> \n> Though in this case it shouldn't be much of a problem; it would be\n> easy to honor @additional_branch_refs by setting 'default' for\n> 'extra-branch-refs' feature to it.\n\nextra-branch-refs is nicer than additional-branch-refs, I'll use it.\n\n> \n> >> BTW. there really should be gitweb/CodingGuidelines...\n> >>\n> >\n> > Yes, would be useful. As in every other project. :)\n> \n> Well, Git itself *has* Documentation/CodingGuidelines, but perhaps\n> gitweb subsystem should have it's own...\n> \n> [...]\n> >>> @@ -3662,7 +3701,8 @@ sub git_get_heads_list {\n> >>>                  my ($committer, $epoch, $tz) =\n> >>>                          ($committerinfo =~ /^(.*) ([0-9]+) (.*)$/);\n> >>>                  $ref_item{'fullname'}  = $name;\n> >>> -               $name =~ s!^refs/(?:head|remote)s/!!;\n> >>> +               my $strip_refs = join '|', map { quotemeta } get_branch_refs();\n> >>> +               $name =~ s!^refs/(?:$strip_refs|remotes)/!!;\n> >>>\n> >>>                  $ref_item{'name'}  = $name;\n> >>>                  $ref_item{'id'}    = $hash;\n> >>> @@ -7179,7 +7219,8 @@ sub snapshot_name {\n> >>>                  $ver = $1;\n> >>>          } else {\n> >>>                  # branches and other need shortened SHA-1 hash\n> >>> -               if ($hash =~ m!^refs/(?:heads|remotes)/(.*)$!) {\n> >>> +               my $strip_refs = join '|', map { quotemeta } get_branch_refs();\n> >>> +               if ($hash =~ m!^refs/(?:$strip_refs|remotes)/(.*)$!) {\n> >>>                          $ver = $1;\n> >>>                  }\n> >>>                  $ver .= '-' . git_get_short_hash($project, $hash);\n> >>\n> >> One one hand, it is about threating extra branch refs the same way as 'head'.\n> >> On the other hand we loose distinction between 'refs/heads/foo' and e.g.\n> >> 'refs/wip/foo'. But maybe that's all right...\n> >>\n> >\n> > In git_get_heads_list sub I could append a \" ($ref_dir)\" to refs which\n> > are in neither 'heads' nor 'remotes', so heads view would look like:\n> > master\n> > old-stable\n> > some-work-in-progress (wip)\n> > some-other-branch (other)\n> >\n> > where both master and old-stable are in refs/heads/,\n> > some-work-in-progress in refs/wip/ and some-other-branch in refs/other/.\n> >\n> > In case of branch snapshot names (snapshot_name sub) I could change it,\n> > so names for branches mentioned above would be\n> > \"Project-master-<short-hash>.tgz\",\n> > \"Project-old_stable-<short-hash>.tgz\",\n> > \"Project-wip-some-work-in-progress-<short-hash>.tgz\"\n> > \"Project-other-some-other-branch-<short-hash>.tgz\"\n> >\n> > What do you think?\n> \n> That is, I think, a very good idea.  Though perhaps it would be more \n> readable to add this extra feature as a separate patch, on top of main one.\n> \n\nRight, I suppose this patch is going to end up being several patches.\n\n-- \nKrzesimir Nowak\nSoftware Developer\nEndocode AG\n\nkrzesimir@endocode.com\n\n------\nEndocode AG, Johannisstraße 20, 10117 Berlin\ninfo@endocode.com | www.endocode.com\n\nVorstandsvorsitzender: Mirko Boehm\nVorstände: Dr. Karl Beecher, Chris Kühl, Sebastian Sucker\nAufsichtsratsvorsitzende: Jennifer Beecher\n\nRegistergericht: Amtsgericht Charlottenburg - HRB 150748 B\n"},{"id":"231425","messageId":"CANQwDwcBx-rbiLwVY6zAmixr=93vyvpVWcCZMLuSEBgjTJ4-tg@mail.gmail.com","threadId":"35419","inReplyTo":"1386068026.2208.16.camel@localhost.localdomain","subject":"Re: [PATCH v3] gitweb: Add an option for adding more branch refs","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2013-12-03T13:02:26Z","receivedAt":"2013-12-03T13:02:26Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Tue, Dec 3, 2013 at 11:53 AM, Krzesimir Nowak <krzesimir@endocode.com> wrote:\n> On Mon, 2013-12-02 at 18:34 +0100, Jakub Narębski wrote:\n>> W dniu 2013-12-02 13:06, Krzesimir Nowak pisze:\n>>> On Mon, 2013-12-02 at 01:21 +0100, Jakub Narębski wrote:\n>>>> On Thu, Nov 28, 2013 at 12:44 PM, Krzesimir Nowak\n>>>> <krzesimir@endocode.com>  wrote:\n>>>>\n>>>>> Allow @additional_branch_refs configuration variable to tell gitweb to\n>>>>> show refs from additional hierarchies in addition to branches in the\n>>>>> list-of-branches view.\n>>>>>\n>>>>> Signed-off-by: Krzesimir Nowak<krzesimir@endocode.com>\n>>>>\n>>>> Why not use %feature hash instead of adding new configuration variable?\n>>>> I think that this option is similar enough to 'remote_heads' feature\n>>>> (which BTW should be 'remote-heads'), and could conceivably enabled\n>>>> on a per-repository basis, i.e. with repository configuration override,\n>>>> isn't it?\n>>>\n>>> I'd like to see some consensus on it before I start changing the patch\n>>> again.\n>>\n>> %feature hash is mainly (but not only) about options that can be\n>> configured on per-repository basis.  Configuration variables are\n>> about options that are per-instance (per gitweb).\n>\n> Well, I am mostly interested in per-instance configuration in this case,\n> but if that is also possible with %feature hash, then ok, I'll try to\n> make it work.\n\nYes, it is possible to have per-instance configuration (you can even\nforbid per-repository configuration).\n\n> From what I've seen (correct me please if I got it wrong) feature\n> settings is taken from per-repository config file from [gitweb] section.\n> If there's nothing then some default value is taken. That default value\n> can be overriden with per-instance perl config file.\n\n%feature settings are taken from gitweb configuration (the 'default'\nkey), and if given feature is overrideable and per-repository configuration\nin the form of appropriate key in [gitweb] section of repository config\nfile exists, it is used instead.\n\n> So it is easy to override it from per-instance perl config by typing:\n> $feature{'additional-branch-refs'}{'default'} = ['wip', 'no|tf\"un,ny'];\n> $feature{'additional-branch-refs'}{'override'} = 1;\n\nYes.  The 'override' is about checking (which imposes a bit of\nperformance penalty) and respecting per-repository configuration.\n\n> (Note the edge case of refs/no|tf\"un,ny, which passes the git\n> check-ref-format scrutiny.)\n>\n> But for now, most of features are quite simple - either booleans,\n> integers or list of simple strings (in snapshot feature). What I need\n> here is a list of strings, like CSV in following example:\n> [gitweb]\n>         additional_branch_refs = wip,\"no|tf\"\"un,ny\"\n>\n> Is dependency on external module like Text::CSV or Text::CSV_XS ok? If\n> not, I can hack some CSV reading code.\n\nWhy not use space, which is forbidden in refnames, to separate\nentries?  Similar to feature_snapshot(), which is about comma separated\nlist, without escaping.\n\n-- \nJakub Narebski\n"},{"id":"231433","messageId":"1386082719.2208.17.camel@localhost.localdomain","threadId":"35419","inReplyTo":"CANQwDwcBx-rbiLwVY6zAmixr=93vyvpVWcCZMLuSEBgjTJ4-tg@mail.gmail.com","subject":"Re: [PATCH v3] gitweb: Add an option for adding more branch refs","fromName":"Krzesimir Nowak","fromEmail":"krzesimir@endocode.com","sentAt":"2013-12-03T14:58:39Z","receivedAt":"2013-12-03T14:58:39Z","isPatch":true,"sender":{"key":"krzesimir@endocode.com","avatar":"https://avatars.githubusercontent.com/u/96081?v=4"},"body":"On Tue, 2013-12-03 at 14:02 +0100, Jakub Narębski wrote:\n> On Tue, Dec 3, 2013 at 11:53 AM, Krzesimir Nowak <krzesimir@endocode.com> wrote:\n> > On Mon, 2013-12-02 at 18:34 +0100, Jakub Narębski wrote:\n> >> W dniu 2013-12-02 13:06, Krzesimir Nowak pisze:\n> >>> On Mon, 2013-12-02 at 01:21 +0100, Jakub Narębski wrote:\n> >>>> On Thu, Nov 28, 2013 at 12:44 PM, Krzesimir Nowak\n> >>>> <krzesimir@endocode.com>  wrote:\n> >>>>\n> >>>>> Allow @additional_branch_refs configuration variable to tell gitweb to\n> >>>>> show refs from additional hierarchies in addition to branches in the\n> >>>>> list-of-branches view.\n> >>>>>\n> >>>>> Signed-off-by: Krzesimir Nowak<krzesimir@endocode.com>\n> >>>>\n> >>>> Why not use %feature hash instead of adding new configuration variable?\n> >>>> I think that this option is similar enough to 'remote_heads' feature\n> >>>> (which BTW should be 'remote-heads'), and could conceivably enabled\n> >>>> on a per-repository basis, i.e. with repository configuration override,\n> >>>> isn't it?\n> >>>\n> >>> I'd like to see some consensus on it before I start changing the patch\n> >>> again.\n> >>\n> >> %feature hash is mainly (but not only) about options that can be\n> >> configured on per-repository basis.  Configuration variables are\n> >> about options that are per-instance (per gitweb).\n> >\n> > Well, I am mostly interested in per-instance configuration in this case,\n> > but if that is also possible with %feature hash, then ok, I'll try to\n> > make it work.\n> \n> Yes, it is possible to have per-instance configuration (you can even\n> forbid per-repository configuration).\n> \n> > From what I've seen (correct me please if I got it wrong) feature\n> > settings is taken from per-repository config file from [gitweb] section.\n> > If there's nothing then some default value is taken. That default value\n> > can be overriden with per-instance perl config file.\n> \n> %feature settings are taken from gitweb configuration (the 'default'\n> key), and if given feature is overrideable and per-repository configuration\n> in the form of appropriate key in [gitweb] section of repository config\n> file exists, it is used instead.\n> \n> > So it is easy to override it from per-instance perl config by typing:\n> > $feature{'additional-branch-refs'}{'default'} = ['wip', 'no|tf\"un,ny'];\n> > $feature{'additional-branch-refs'}{'override'} = 1;\n> \n> Yes.  The 'override' is about checking (which imposes a bit of\n> performance penalty) and respecting per-repository configuration.\n> \n> > (Note the edge case of refs/no|tf\"un,ny, which passes the git\n> > check-ref-format scrutiny.)\n> >\n> > But for now, most of features are quite simple - either booleans,\n> > integers or list of simple strings (in snapshot feature). What I need\n> > here is a list of strings, like CSV in following example:\n> > [gitweb]\n> >         additional_branch_refs = wip,\"no|tf\"\"un,ny\"\n> >\n> > Is dependency on external module like Text::CSV or Text::CSV_XS ok? If\n> > not, I can hack some CSV reading code.\n> \n> Why not use space, which is forbidden in refnames, to separate\n> entries?  Similar to feature_snapshot(), which is about comma separated\n> list, without escaping.\n> \n\nThanks for explanations and a hint. Following patches are in \"Show extra\nbranch refs in gitweb\".\n\nCheers,\n-- \nKrzesimir Nowak\nSoftware Developer\nEndocode AG\n\nkrzesimir@endocode.com\n\n------\nEndocode AG, Johannisstraße 20, 10117 Berlin\ninfo@endocode.com | www.endocode.com\n\nVorstandsvorsitzender: Mirko Boehm\nVorstände: Dr. Karl Beecher, Chris Kühl, Sebastian Sucker\nAufsichtsratsvorsitzende: Jennifer Beecher\n\nRegistergericht: Amtsgericht Charlottenburg - HRB 150748 B\n"}]}