{"thread":{"id":"35475","subject":"[PATCH 0/5] Show extra branch refs in gitweb v6","startedAt":"2013-12-04T13:42:58Z","lastAt":"2013-12-10T19:44:07Z","messageCount":32,"participants":["Krzesimir Nowak","Jakub Narębski","Martin Langhoff","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"231507","messageId":"1386164583-14109-1-git-send-email-krzesimir@endocode.com","threadId":"35475","inReplyTo":null,"subject":"[PATCH 0/5] Show extra branch refs in gitweb v6","fromName":"Krzesimir Nowak","fromEmail":"krzesimir@endocode.com","sentAt":"2013-12-04T13:42:58Z","receivedAt":"2013-12-04T13:42:58Z","isPatch":true,"sender":{"key":"krzesimir@endocode.com","avatar":"https://avatars.githubusercontent.com/u/96081?v=4"},"body":"First patch just adds a comment I though would be useful when trying\nto understand how config parsing is done.\n\nSecond patch splits some code to a function.\n\nThird patch fixes validation functions to return either 0 or 1,\ninstead of undef or passed $input.\n\nFourth patch adds the extra-branch-feature and some documentation.\n\nFifth patch adds some visual differentation of branches from\nnon-standard ref directories.\n\nKrzesimir Nowak (5):\n gitweb: Add a comment explaining the meaning of $/\n gitweb: Move check-ref-format code into separate function\n gitweb: Return plain booleans in validation methods\n gitweb: Add a feature for adding more branch refs\n gitweb: Denote non-heads, non-remotes branches\n\n Documentation/gitweb.conf.txt | 27 +++++++\n gitweb/gitweb.perl | 166 +++++++++++++++++++++++++++++++++---------\n 2 files changed, 160 insertions(+), 33 deletions(-)\n\n-- \n1.8.3.1\n"},{"id":"231508","messageId":"1386164583-14109-2-git-send-email-krzesimir@endocode.com","threadId":"35475","inReplyTo":"1386164583-14109-1-git-send-email-krzesimir@endocode.com","subject":"[PATCH 1/5] gitweb: Add a comment explaining the meaning of $/","fromName":"Krzesimir Nowak","fromEmail":"krzesimir@endocode.com","sentAt":"2013-12-04T13:42:59Z","receivedAt":"2013-12-04T13:42:59Z","isPatch":true,"sender":{"key":"krzesimir@endocode.com","avatar":"https://avatars.githubusercontent.com/u/96081?v=4"},"body":"So future reader will know what does it mean without running \"perldoc\nperlvar\".\n\nSigned-off-by: Krzesimir Nowak \n---\n gitweb/gitweb.perl | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 68c77f6..ee61f9e 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -2629,6 +2629,8 @@ sub git_parse_project_config {\n \tmy $section_regexp = shift;\n \tmy %config;\n \n+\t# input record separator, so getline does end on null, not\n+\t# newline\n \tlocal $/ = \"\\0\";\n \n \topen my $fh, \"-|\", git_cmd(), \"config\", '-z', '-l',\n-- \n1.8.3.1\n"},{"id":"231509","messageId":"1386164583-14109-3-git-send-email-krzesimir@endocode.com","threadId":"35475","inReplyTo":"1386164583-14109-1-git-send-email-krzesimir@endocode.com","subject":"[PATCH 2/5] gitweb: Move check-ref-format code into separate function","fromName":"Krzesimir Nowak","fromEmail":"krzesimir@endocode.com","sentAt":"2013-12-04T13:43:00Z","receivedAt":"2013-12-04T13:43:00Z","isPatch":true,"sender":{"key":"krzesimir@endocode.com","avatar":"https://avatars.githubusercontent.com/u/96081?v=4"},"body":"This check will be used in more than one place later.\n\nSigned-off-by: Krzesimir Nowak \nReviewed-by: Junio C Hamano \nReviewed-by: Jakub Narębski \n---\n gitweb/gitweb.perl | 17 +++++++++++++----\n 1 file changed, 13 insertions(+), 4 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex ee61f9e..67415b9 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -1452,6 +1452,16 @@ sub validate_pathname {\n \treturn $input;\n }\n \n+sub check_ref_format {\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 +1472,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+\tcheck_ref_format($input)\n+\t\tor return undef;\n \treturn $input;\n }\n \n-- \n1.8.3.1\n"},{"id":"231510","messageId":"1386164583-14109-4-git-send-email-krzesimir@endocode.com","threadId":"35475","inReplyTo":"1386164583-14109-1-git-send-email-krzesimir@endocode.com","subject":"[PATCH 3/5] gitweb: Return plain booleans in validation methods","fromName":"Krzesimir Nowak","fromEmail":"krzesimir@endocode.com","sentAt":"2013-12-04T13:43:01Z","receivedAt":"2013-12-04T13:43:01Z","isPatch":true,"sender":{"key":"krzesimir@endocode.com","avatar":"https://avatars.githubusercontent.com/u/96081?v=4"},"body":"Users of validate_* passing \"0\" might get failures on correct name\nbecause of coercion of \"0\" to false in code like:\ndie_error(500, \"invalid ref\") unless (check_ref_format (\"0\"));\n\nSigned-off-by: Krzesimir Nowak \n---\n gitweb/gitweb.perl | 45 +++++++++++++++++++++++++--------------------\n 1 file changed, 25 insertions(+), 20 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 67415b9..3434602 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -1419,63 +1419,68 @@ sub href {\n ## validation, quoting/unquoting and escaping\n \n sub validate_action {\n-\tmy $input = shift || return undef;\n-\treturn undef unless exists $actions{$input};\n-\treturn $input;\n+\tmy $input = shift;\n+\n+\treturn 0 unless defined $input;\n+\treturn 0 unless exists $actions{$input};\n+\treturn 1;\n }\n \n sub validate_project {\n-\tmy $input = shift || return undef;\n+\tmy $input = shift;\n+\n+\treturn 0 unless defined $input;\n \tif (!validate_pathname($input) ||\n \t\t!(-d \"$projectroot/$input\") ||\n \t\t!check_export_ok(\"$projectroot/$input\") ||\n \t\t($strict_export && !project_in_list($input))) {\n-\t\treturn undef;\n+\t\treturn 0;\n \t} else {\n-\t\treturn $input;\n+\t\treturn 1;\n \t}\n }\n \n sub validate_pathname {\n-\tmy $input = shift || return undef;\n+\tmy $input = shift;\n \n+\treturn 0 unless defined $input;\n \t# no '.' or '..' as elements of path, i.e. no '.' nor '..'\n \t# at the beginning, at the end, and between slashes.\n \t# also this catches doubled slashes\n \tif ($input =~ m!(^|/)(|\\.|\\.\\.)(/|$)!) {\n-\t\treturn undef;\n+\t\treturn 0;\n \t}\n \t# no null characters\n \tif ($input =~ m!\\0!) {\n-\t\treturn undef;\n+\t\treturn 0;\n \t}\n-\treturn $input;\n+\treturn 1;\n }\n \n sub check_ref_format {\n-\tmy $input = shift || return undef;\n+\tmy $input = shift;\n \n+\treturn 0 unless defined $input;\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\treturn 0;\n \t}\n-\treturn $input;\n+\treturn 1;\n }\n \n sub validate_refname {\n-\tmy $input = shift || return undef;\n+\tmy $input = shift;\n \n+\treturn undef unless defined $input;\n \t# textual hashes are O.K.\n \tif ($input =~ m/^[0-9a-fA-F]{40}$/) {\n-\t\treturn $input;\n+\t\treturn 1;\n \t}\n \t# it must be correct pathname\n-\t$input = validate_pathname($input)\n-\t\tor return undef;\n+\tvalidate_pathname($input) or return 0;\n \t# check git-check-ref-format restrictions\n-\tcheck_ref_format($input)\n-\t\tor return undef;\n-\treturn $input;\n+\tcheck_ref_format($input) or return 0;\n+\treturn 1;\n }\n \n # decode sequences of octets in utf8 into Perl's internal form,\n-- \n1.8.3.1\n"},{"id":"231511","messageId":"1386164583-14109-5-git-send-email-krzesimir@endocode.com","threadId":"35475","inReplyTo":"1386164583-14109-1-git-send-email-krzesimir@endocode.com","subject":"[PATCH 4/5] gitweb: Add a feature for adding more branch refs","fromName":"Krzesimir Nowak","fromEmail":"krzesimir@endocode.com","sentAt":"2013-12-04T13:43:02Z","receivedAt":"2013-12-04T13:43:02Z","isPatch":true,"sender":{"key":"krzesimir@endocode.com","avatar":"https://avatars.githubusercontent.com/u/96081?v=4"},"body":"Allow extra-branch-refs feature to tell gitweb to show refs from\nadditional hierarchies in addition to branches in the list-of-branches\nview.\n\nSigned-off-by: Krzesimir Nowak \nReviewed-by: Junio C Hamano \nReviewed-by: Jakub Narębski \n---\n Documentation/gitweb.conf.txt | 37 +++++++++++++++++++\n gitweb/gitweb.perl | 85 +++++++++++++++++++++++++++++++++++++------\n 2 files changed, 110 insertions(+), 12 deletions(-)\n\ndiff --git a/Documentation/gitweb.conf.txt b/Documentation/gitweb.conf.txt\nindex e2113d9..5a77452 100644\n--- a/Documentation/gitweb.conf.txt\n+++ b/Documentation/gitweb.conf.txt\n@@ -849,6 +849,43 @@ time zones in the form of \"+/-HHMM\", such as \"+0200\".\n +\n Project specific override is not supported.\n \n+extra-branch-refs::\n+\tList of additional directories under \"refs\" which are going to\n+\tbe used as branch refs. For example if you have a gerrit setup\n+\twhere all branches under refs/heads/ are official,\n+\tpush-after-review ones and branches under refs/sandbox/,\n+\trefs/wip and refs/other are user ones where permissions are\n+\tmuch wider, then you might want to set this variable as\n+\tfollows:\n++\n+--------------------------------------------------------------------------------\n+$feature{'extra-branch-refs'}{'default'} =\n+\t['sandbox', 'wip', 'other'];\n+--------------------------------------------------------------------------------\n++\n+If overriding was enabled then this feature can be configured on a\n+per-repository basis via repository's `gitweb.extrabranchrefs`\n+configuration variable, which contains a space separated list of\n+refs. An example:\n++\n+--------------------------------------------------------------------------------\n+[gitweb]\n+\textrabranchrefs = sandbox wip other\n+--------------------------------------------------------------------------------\n++\n+The gitweb.extrabranchrefs is actually a multi-valued configuration\n+variable, so following example is also correct and the result is the\n+same as of the snippet above:\n++\n+--------------------------------------------------------------------------------\n+[gitweb]\n+\textrabranchrefs = sandbox\n+\textrabranchrefs = wip other\n+--------------------------------------------------------------------------------\n++\n+It is an error to specify a ref that does not pass \"git check-ref-format\"\n+scrutiny. Duplicated values are filtered.\n+\n \n EXAMPLES\n --------\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 3434602..6d3d52d 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -548,6 +548,20 @@ our %feature = (\n \t\t'sub' => sub { feature_bool('remote_heads', @_) },\n \t\t'override' => 0,\n \t\t'default' => [0]},\n+\n+\t# Enable showing branches under other refs in addition to heads\n+\n+\t# To set system wide extra branch refs have in $GITWEB_CONFIG\n+\t# $feature{'extra-branch-refs'}{'default'} = ['dirs', 'of', 'choice'];\n+\t# To have project specific config enable override in $GITWEB_CONFIG\n+\t# $feature{'extra-branch-refs'}{'override'} = 1;\n+\t# and in project config gitweb.extrabranchrefs = dirs of choice\n+\t# Every directory is separated with whitespace.\n+\n+\t'extra-branch-refs' => {\n+\t\t'sub' => \\&feature_extra_branch_refs,\n+\t\t'override' => 0,\n+\t\t'default' => []},\n );\n \n sub gitweb_get_feature {\n@@ -626,6 +640,26 @@ sub feature_avatar {\n \treturn @val ? @val : @_;\n }\n \n+sub feature_extra_branch_refs {\n+\tmy (@branch_refs) = @_;\n+\tmy $values = git_get_project_config('extrabranchrefs');\n+\n+\tif ($values) {\n+\t\tunless (ref $values) {\n+\t\t\t$values = [$values];\n+\t\t}\n+\t\tunless (ref $values eq 'ARRAY') {\n+\t\t\treturn @branch_refs;\n+\t\t}\n+\t\t@branch_refs = ();\n+\t\tforeach my $value (@{$values}) {\n+\t\t\tpush @branch_refs, split /\\s+/, $value;\n+\t\t}\n+\t}\n+\n+\treturn @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@@ -656,6 +690,18 @@ sub filter_snapshot_fmts {\n \t\t!$known_snapshot_formats{$_}{'disabled'}} @fmts;\n }\n \n+sub filter_and_validate_refs {\n+\tmy @refs = @_;\n+\tmy %unique_refs = ();\n+\n+\tforeach my $ref (@refs) {\n+\t\tdie_error(500, \"Invalid ref '$ref' in 'extra-branch-refs' feature\") unless (check_ref_format($ref));\n+\t\t# 'heads' are added implicitly in get_branch_refs().\n+\t\t$unique_refs{$ref} = 1 if ($ref ne 'heads');\n+\t}\n+\treturn sort keys %unique_refs;\n+}\n+\n # If it is set to code reference, it is code that it is to be run once per\n # request, allowing updating configurations that change with each request,\n # while running other code in config file only once.\n@@ -1113,7 +1159,7 @@ sub evaluate_git_dir {\n \tour $git_dir = \"$projectroot/$project\" if $project;\n }\n \n-our (@snapshot_fmts, $git_avatar);\n+our (@snapshot_fmts, $git_avatar, @extra_branch_refs);\n sub configure_gitweb_features {\n \t# list of supported snapshot formats\n \tour @snapshot_fmts = gitweb_get_feature('snapshot');\n@@ -1131,6 +1177,13 @@ sub configure_gitweb_features {\n \t} else {\n \t\t$git_avatar = '';\n \t}\n+\n+\tour @extra_branch_refs = gitweb_get_feature('extra-branch-refs');\n+\t@extra_branch_refs = filter_and_validate_refs (@extra_branch_refs);\n+}\n+\n+sub get_branch_refs {\n+\treturn ('heads', @extra_branch_refs);\n }\n \n # custom error handler: 'die ' is Internal Server Error\n@@ -2529,6 +2582,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@@ -2536,12 +2590,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@@ -2554,7 +2613,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@@ -3209,7 +3268,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@@ -3660,7 +3719,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@@ -3678,7 +3737,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@@ -7195,7 +7255,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":"231512","messageId":"1386164583-14109-6-git-send-email-krzesimir@endocode.com","threadId":"35475","inReplyTo":"1386164583-14109-1-git-send-email-krzesimir@endocode.com","subject":"[PATCH 5/5] gitweb: Denote non-heads, non-remotes branches","fromName":"Krzesimir Nowak","fromEmail":"krzesimir@endocode.com","sentAt":"2013-12-04T13:43:03Z","receivedAt":"2013-12-04T13:43:03Z","isPatch":true,"sender":{"key":"krzesimir@endocode.com","avatar":"https://avatars.githubusercontent.com/u/96081?v=4"},"body":"Given two branches residing in refs/heads/master and refs/wip/feature\nthe list-of-branches view will present them in following way:\nmaster\nfeature (wip)\n\nWhen getting a snapshot of a 'feature' branch, the tarball is going to\nhave name like 'project-wip-feature-.tgz'.\n\nSigned-off-by: Krzesimir Nowak \n---\n gitweb/gitweb.perl | 27 +++++++++++++++++++++++++--\n 1 file changed, 25 insertions(+), 2 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 6d3d52d..9a63ea9 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -3739,8 +3739,14 @@ sub git_get_heads_list {\n \t\t$ref_item{'fullname'} = $name;\n \t\tmy $strip_refs = join '|', map { quotemeta } get_branch_refs();\n \t\t$name =~ s!^refs/($strip_refs|remotes)/!!;\n+\t\t$ref_item{'name'} = $name;\n+\t\t# for refs neither in 'heads' nor 'remotes' we want to\n+\t\t# show their different ref dir\n+\t\tmy $ref_dir = (defined $1) ? $1 : '';\n+\t\tif ($ref_dir ne '' and $ref_dir ne 'heads' and $ref_dir ne 'remotes') {\n+\t\t $ref_item{'name'} .= ' (' . $ref_dir . ')';\n+\t\t}\n \n-\t\t$ref_item{'name'} = $name;\n \t\t$ref_item{'id'} = $hash;\n \t\t$ref_item{'title'} = $title || '(no commit message)';\n \t\t$ref_item{'epoch'} = $epoch;\n@@ -7257,7 +7263,24 @@ sub snapshot_name {\n \t\t# branches and other need shortened SHA-1 hash\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\tmy $ref_dir = $1;\n+\t\t\t$ver = $2;\n+\n+\t\t\tif (defined $ref_dir) {\n+\t\t\t\t# this is going to be a part of\n+\t\t\t\t# filename, so lets stick to\n+\t\t\t\t# alphanumerics, dashes and underlines\n+\t\t\t\t# only - some filesystems do not like\n+\t\t\t\t# some punctuation symbols for\n+\t\t\t\t# example.\n+\t\t\t\t$ref_dir =~ s/[^[:alnum:]_-]//g;\n+\t\t\t}\n+\n+\t\t\t# for refs not in heads nor remotes we want to\n+\t\t\t# add a ref dir to archive name\n+\t\t\tif ($ref_dir ne '' and $ref_dir ne 'heads' and $ref_dir ne 'remotes') {\n+\t\t\t\t$ver = $ref_dir . '-' . $ver;\n+\t\t\t}\n \t\t}\n \t\t$ver .= '-' . git_get_short_hash($project, $hash);\n \t}\n-- \n1.8.3.1\n"},{"id":"231514","messageId":"CANQwDwdiNHFCChVdkKxkEa+HOKzzhgfzy4d6iZB6ejSfva3Z3A@mail.gmail.com","threadId":"35475","inReplyTo":"1386164583-14109-2-git-send-email-krzesimir@endocode.com","subject":"Re: [PATCH 1/5] gitweb: Add a comment explaining the meaning of $/","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2013-12-04T15:11:42Z","receivedAt":"2013-12-04T15:11:42Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Wed, Dec 4, 2013 at 2:42 PM, Krzesimir Nowak wrote:\n\n> So future reader will know what does it mean without running \"perldoc\n> perlvar\".\n\nHmmm... shouldn't future reader know it anyway? It is not that cryptic.\nI'd say it is idiomatic Perl.\n\nBesides, it is not the only place where we set input record separator to NUL,\nto match corresponding option to git command to separate records with NUL\n(the '-z' option in this case). Others are git_get_path_by_hash(),\nparse_commit(),\nand parse_commits(), git_tree(), not including places where we set $/ to undef\nto slurp whole file: git_get_link_target(), git_blobdiff() for $format\n== 'plain', etc.\n\n> Signed-off-by: Krzesimir Nowak \n> ---\n> gitweb/gitweb.perl | 2 ++\n> 1 file changed, 2 insertions(+)\n>\n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 68c77f6..ee61f9e 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -2629,6 +2629,8 @@ sub git_parse_project_config {\n> my $section_regexp = shift;\n> my %config;\n>\n> + # input record separator, so getline does end on null, not\n> + # newline\n> local $/ = \"\\0\";\n\nIt is here because of '-z' option below (to account for values with\nembedded newlines).\n\n>\n> open my $fh, \"-|\", git_cmd(), \"config\", '-z', '-l',\n> --\n> 1.8.3.1\n>\n\n\n\n-- \nJakub Narebski\n"},{"id":"231515","messageId":"1386171977.2173.23.camel@localhost.localdomain","threadId":"35475","inReplyTo":"CANQwDwdiNHFCChVdkKxkEa+HOKzzhgfzy4d6iZB6ejSfva3Z3A@mail.gmail.com","subject":"Re: [PATCH 1/5] gitweb: Add a comment explaining the meaning of $/","fromName":"Krzesimir Nowak","fromEmail":"krzesimir@endocode.com","sentAt":"2013-12-04T15:46:17Z","receivedAt":"2013-12-04T15:46:17Z","isPatch":true,"sender":{"key":"krzesimir@endocode.com","avatar":"https://avatars.githubusercontent.com/u/96081?v=4"},"body":"On Wed, 2013-12-04 at 16:11 +0100, Jakub Narębski wrote:\n> On Wed, Dec 4, 2013 at 2:42 PM, Krzesimir Nowak wrote:\n> \n> > So future reader will know what does it mean without running \"perldoc\n> > perlvar\".\n> \n> Hmmm... shouldn't future reader know it anyway? It is not that cryptic.\n> I'd say it is idiomatic Perl.\n> \n\nIt's plainly obscure. And I think it is not that often used - I keep\nforgetting what that pair of punctuation is actually meaning. In this\ncase I guess it would be more readable to use the following code\ninstead:\n$fh->input_record_separator (\"\\0\");\n\n> Besides, it is not the only place where we set input record separator to NUL,\n> to match corresponding option to git command to separate records with NUL\n> (the '-z' option in this case). Others are git_get_path_by_hash(),\n> parse_commit(),\n> and parse_commits(), git_tree(), not including places where we set $/ to undef\n> to slurp whole file: git_get_link_target(), git_blobdiff() for $format\n> == 'plain', etc.\n\nYes, but I stumbled upon that one when trying to understand how config\nparsing works. So I added a comment.\n\n> \n> > Signed-off-by: Krzesimir Nowak \n> > ---\n> > gitweb/gitweb.perl | 2 ++\n> > 1 file changed, 2 insertions(+)\n> >\n> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> > index 68c77f6..ee61f9e 100755\n> > --- a/gitweb/gitweb.perl\n> > +++ b/gitweb/gitweb.perl\n> > @@ -2629,6 +2629,8 @@ sub git_parse_project_config {\n> > my $section_regexp = shift;\n> > my %config;\n> >\n> > + # input record separator, so getline does end on null, not\n> > + # newline\n> > local $/ = \"\\0\";\n> \n> It is here because of '-z' option below (to account for values with\n> embedded newlines).\n> \n> >\n> > open my $fh, \"-|\", git_cmd(), \"config\", '-z', '-l',\n> > --\n> > 1.8.3.1\n> >\n> \n> \n> \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":"231516","messageId":"CANQwDwfWPuH_OOU46yOrRN23qbppHBQbXEUnDcR37dF=93A-iA@mail.gmail.com","threadId":"35475","inReplyTo":"1386164583-14109-3-git-send-email-krzesimir@endocode.com","subject":"Re: [PATCH 2/5] gitweb: Move check-ref-format code into separate function","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2013-12-04T15:56:22Z","receivedAt":"2013-12-04T15:56:22Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Wed, Dec 4, 2013 at 2:43 PM, Krzesimir Nowak wrote:\n\n> This check will be used in more than one place later.\n>\n> Signed-off-by: Krzesimir Nowak \n> Reviewed-by: Junio C Hamano \n> Reviewed-by: Jakub Narębski \n\nAll right, that is nice refactoring.\n\n> ---\n> gitweb/gitweb.perl | 17 +++++++++++++----\n> 1 file changed, 13 insertions(+), 4 deletions(-)\n>\n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index ee61f9e..67415b9 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -1452,6 +1452,16 @@ sub validate_pathname {\n> return $input;\n> }\n>\n> +sub check_ref_format {\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> @@ -1462,10 +1472,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> + check_ref_format($input)\n> + or return undef;\n> return $input;\n> }\n\nRight, check_ref_format() has name after git-check-ref-format...\nthough... check_ref_format() or die doesn't read completely\nnaturally...\n\n-- \nJakub Narebski\n"},{"id":"231517","messageId":"CANQwDweGgQpnqDqaekBWt-rczjHkJEmFyXW9qDh2En2r=ZXbMw@mail.gmail.com","threadId":"35475","inReplyTo":"1386164583-14109-4-git-send-email-krzesimir@endocode.com","subject":"Re: [PATCH 3/5] gitweb: Return plain booleans in validation methods","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2013-12-04T16:07:08Z","receivedAt":"2013-12-04T16:07:08Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Wed, Dec 4, 2013 at 2:43 PM, Krzesimir Nowak wrote:\n\n> Users of validate_* passing \"0\" might get failures on correct name\n> because of coercion of \"0\" to false in code like:\n> die_error(500, \"invalid ref\") unless (check_ref_format (\"0\"));\n\nI would say that the problem was that validate_sth() subroutines returned\nvalue of parameter if it was valid, which could be a problem if said value is\nfalse-ish (e.g. validate_refname(\"0\"), or validate_pathname(\"0\")).\n\nReturning undef on invalid data newer was a problem, using 'return $input;'\non valid input was, especially that validate_sth() functions were ever used\nin a conditional:\n\n if (!validate_sth($param)) {\n die_error(...)\n }\n\nWhile at it validate_sth() is not a best name for boolean predicate:\nis_valid_sth() would be better, I think.\n\n> Signed-off-by: Krzesimir Nowak \n> ---\n> gitweb/gitweb.perl | 45 +++++++++++++++++++++++++--------------------\n> 1 file changed, 25 insertions(+), 20 deletions(-)\n>\n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 67415b9..3434602 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -1419,63 +1419,68 @@ sub href {\n> ## validation, quoting/unquoting and escaping\n>\n> sub validate_action {\n> - my $input = shift || return undef;\n> - return undef unless exists $actions{$input};\n> - return $input;\n> + my $input = shift;\n> +\n> + return 0 unless defined $input;\n> + return 0 unless exists $actions{$input};\n> + return 1;\n> }\n\nThe only change that needs to be doe is replacing\n\n return $input;\n\nwith\n\n return 1;\n\n-- \nJakub Narebski\n"},{"id":"231519","messageId":"CACPiFCJON33dz64qSZDnrCo8hbVpbxk7k65xmW1oBiZ4Muzcbg@mail.gmail.com","threadId":"35475","inReplyTo":"1386171977.2173.23.camel@localhost.localdomain","subject":"Re: [PATCH 1/5] gitweb: Add a comment explaining the meaning of $/","fromName":"Martin Langhoff","fromEmail":"martin.langhoff@gmail.com","sentAt":"2013-12-04T16:19:02Z","receivedAt":"2013-12-04T16:19:02Z","isPatch":true,"sender":{"key":"martin.langhoff@gmail.com","avatar":"https://gravatar.com/avatar/1e3f311b6c4c15836501901ca58f8c0b0667246488084ba524d8bc9867e22fd9?d=mp&s=160"},"body":"On Wed, Dec 4, 2013 at 10:46 AM, Krzesimir Nowak wrote:\n> On Wed, 2013-12-04 at 16:11 +0100, Jakub Narębski wrote:\n>> On Wed, Dec 4, 2013 at 2:42 PM, Krzesimir Nowak wrote:\n>>\n>> > So future reader will know what does it mean without running \"perldoc\n>> > perlvar\".\n>>\n>> Hmmm... shouldn't future reader know it anyway? It is not that cryptic.\n>> I'd say it is idiomatic Perl.\n>\n> It's plainly obscure. And I think it is not that often used -\n\nIt's classic Perl.\n\nPerhaps you'd want to \"use English;\" and call it\n$INPUT_RECORD_SEPARATOR in a patch titled \"Make things readable to\nnon-Perl natives\".\n\ncheers,\n\n\n\nm\n-- \n martin.langhoff@gmail.com\n - ask interesting questions\n - don't get distracted with shiny stuff - working code first\n ~ http://docs.moodle.org/en/User:Martin_Langhoff\n"},{"id":"231521","messageId":"CANQwDwfiYAz7dro9mppR62xEFMPTSNjEKYM=7Dhq7auBvyXjSg@mail.gmail.com","threadId":"35475","inReplyTo":"1386171977.2173.23.camel@localhost.localdomain","subject":"Re: [PATCH 1/5] gitweb: Add a comment explaining the meaning of $/","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2013-12-04T17:34:30Z","receivedAt":"2013-12-04T17:34:30Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Wed, Dec 4, 2013 at 4:46 PM, Krzesimir Nowak wrote:\n> On Wed, 2013-12-04 at 16:11 +0100, Jakub Narębski wrote:\n>> On Wed, Dec 4, 2013 at 2:42 PM, Krzesimir Nowak wrote:\n>>\n>> > So future reader will know what does it mean without running \"perldoc\n>> > perlvar\".\n>>\n>> Hmmm... shouldn't future reader know it anyway? It is not that cryptic.\n>> I'd say it is idiomatic Perl.\n>\n> It's plainly obscure. And I think it is not that often used - I keep\n> forgetting what that pair of punctuation is actually meaning.\n\nI think it depends on what kind of Perl code one is used to. It is not\nas obscure as $; and similar to $|, I think.\n\n> In this case I guess it would be more readable to use the following code\n> instead:\n>\n> $fh->input_record_separator (\"\\0\");\n\nThat would be a good change to replace\n\n local $/ = \"\\0\";\n\n open my $fh, \"-|\", git_cmd(), ..., '-z', ...\n\nwith\n\n open my $fh, \"-|\", git_cmd(), ..., '-z', ...\n $fh->input_record_separator (\"\\0\");\n\n(not forgetting about \"use IO::Handle\", which module is core Perl module);\n\n\nAnyway, this change (or proposed change) does not, IMHO, belong\nin this series.\n\n-- \nJakub Narebski\n"},{"id":"231523","messageId":"CANQwDwdUY7FunkZEsAn5PuXH+e1yBL7mfA2vTdQwfOTTmAh+ig@mail.gmail.com","threadId":"35475","inReplyTo":"CANQwDwfiYAz7dro9mppR62xEFMPTSNjEKYM=7Dhq7auBvyXjSg@mail.gmail.com","subject":"Re: [PATCH 1/5] gitweb: Add a comment explaining the meaning of $/","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2013-12-04T17:37:59Z","receivedAt":"2013-12-04T17:37:59Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Wed, Dec 4, 2013 at 6:34 PM, Jakub Narębski wrote:\n> On Wed, Dec 4, 2013 at 4:46 PM, Krzesimir Nowak wrote:\n>> On Wed, 2013-12-04 at 16:11 +0100, Jakub Narębski wrote:\n>>> On Wed, Dec 4, 2013 at 2:42 PM, Krzesimir Nowak wrote:\n>>>\n>>> > So future reader will know what does it mean without running \"perldoc\n>>> > perlvar\".\n>>>\n>>> Hmmm... shouldn't future reader know it anyway? It is not that cryptic.\n>>> I'd say it is idiomatic Perl.\n>>\n>> It's plainly obscure. And I think it is not that often used - I keep\n>> forgetting what that pair of punctuation is actually meaning.\n>\n> I think it depends on what kind of Perl code one is used to. It is not\n> as obscure as $; and similar to $|, I think.\n>\n>> In this case I guess it would be more readable to use the following code\n>> instead:\n>>\n>> $fh->input_record_separator (\"\\0\");\n>\n> That would be a good change to replace\n>\n> local $/ = \"\\0\";\n>\n> open my $fh, \"-|\", git_cmd(), ..., '-z', ...\n>\n> with\n>\n> open my $fh, \"-|\", git_cmd(), ..., '-z', ...\n> $fh->input_record_separator (\"\\0\");\n>\n> (not forgetting about \"use IO::Handle\", which module is core Perl module);\n\nActually it is replacing\n\n local $/ = \"\\0\";\n\nwith\n\n IO::Handle->input_record_separator(\"\\0\");\n\nsee http://p3rl.org/IO::Handle\n"},{"id":"231527","messageId":"CANQwDwe+a2P0Jxqw0k7sHWv3exdb4k+NU3jL3ogR-rcetd82TQ@mail.gmail.com","threadId":"35475","inReplyTo":"1386164583-14109-5-git-send-email-krzesimir@endocode.com","subject":"Re: [PATCH 4/5] gitweb: Add a feature for adding more branch refs","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2013-12-04T18:06:28Z","receivedAt":"2013-12-04T18:06:28Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Wed, Dec 4, 2013 at 2:43 PM, Krzesimir Nowak wrote:\n>\n> Allow extra-branch-refs feature to tell gitweb to show refs from\n> additional hierarchies in addition to branches in the list-of-branches\n> view.\n>\n> Signed-off-by: Krzesimir Nowak \n> Reviewed-by: Junio C Hamano \n> Reviewed-by: Jakub Narębski \n\nThis version is Helped-by (maybe), but not (yet!) Reviewed-by.\n\n> ---\n> Documentation/gitweb.conf.txt | 37 +++++++++++++++++++\n> gitweb/gitweb.perl | 85 +++++++++++++++++++++++++++++++++++++------\n> 2 files changed, 110 insertions(+), 12 deletions(-)\n>\n> diff --git a/Documentation/gitweb.conf.txt b/Documentation/gitweb.conf.txt\n> index e2113d9..5a77452 100644\n> --- a/Documentation/gitweb.conf.txt\n> +++ b/Documentation/gitweb.conf.txt\n> @@ -849,6 +849,43 @@ time zones in the form of \"+/-HHMM\", such as \"+0200\".\n> +\n> Project specific override is not supported.\n>\n> +extra-branch-refs::\n> + List of additional directories under \"refs\" which are going to\n> + be used as branch refs. For example if you have a gerrit setup\n> + where all branches under refs/heads/ are official,\n> + push-after-review ones and branches under refs/sandbox/,\n> + refs/wip and refs/other are user ones where permissions are\n> + much wider, then you might want to set this variable as\n> + follows:\n> ++\n> +--------------------------------------------------------------------------------\n> +$feature{'extra-branch-refs'}{'default'} =\n> + ['sandbox', 'wip', 'other'];\n> +--------------------------------------------------------------------------------\n> ++\n> +If overriding was enabled then this feature can be configured on a\n\ns/was/is/;\n\nPerhaps it would better read as\n\n This feature can be configured on per-repository basis after setting\n $feature{'extra-branch-refs'}{'override'} to true, via repository's\n `gitweb.extraBranchRefs` ...\n\n> +per-repository basis via repository's `gitweb.extrabranchrefs`\n> +configuration variable, which contains a space separated list of\n> +refs. An example:\n> ++\n> +--------------------------------------------------------------------------------\n> +[gitweb]\n> + extrabranchrefs = sandbox wip other\n> +--------------------------------------------------------------------------------\n\nO.K.\n\n> ++\n> +The gitweb.extrabranchrefs is actually a multi-valued configuration\n> +variable, so following example is also correct and the result is the\n> +same as of the snippet above:\n> ++\n> +--------------------------------------------------------------------------------\n> +[gitweb]\n> + extrabranchrefs = sandbox\n> + extrabranchrefs = wip other\n> +--------------------------------------------------------------------------------\n\nI think this part should be better left for a separate patch. There is\nimportant difference between single-valued and multi-valued configuration\nvariable: with single-valued later occurrences override earlier ones,\nwhich includes settings in more specific config file (e.g. per-repository)\noverriding setting in more general one (e.g. per-user or system-wide).\n\nWith multi-valued we won't be able to override earlier / more generic\nsettings... well, unless we add support for no-value, or empty-value\nas clearer, i.e.\n\n [gitweb]\n extrabranchrefs = sandbox\n extrabranchrefs\n # or extrabranchrefs =\n extrabranchrefs = wip other\n\nresulting in ('wip', 'other').\n\n> ++\n> +It is an error to specify a ref that does not pass \"git check-ref-format\"\n> +scrutiny. Duplicated values are filtered.\n> +\n\nHmmm... 'snapshot' feature ignores invalid values, but in this case\nformerly valid compression schemes might get invalid via tightening\n%known_snapshot_formats, and we don't want existing config getting\nsuddenly invalid.\n\n[cut]\n\nNice!\n\n-- \nJakub Narebski\n"},{"id":"231528","messageId":"xmqqeh5slc53.fsf@gitster.dls.corp.google.com","threadId":"35475","inReplyTo":"CANQwDweGgQpnqDqaekBWt-rczjHkJEmFyXW9qDh2En2r=ZXbMw@mail.gmail.com","subject":"Re: [PATCH 3/5] gitweb: Return plain booleans in validation methods","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-04T18:11:20Z","receivedAt":"2013-12-04T18:11:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narębski writes:\n\n> On Wed, Dec 4, 2013 at 2:43 PM, Krzesimir Nowak wrote:\n>\n>> Users of validate_* passing \"0\" might get failures on correct name\n>> because of coercion of \"0\" to false in code like:\n>> die_error(500, \"invalid ref\") unless (check_ref_format (\"0\"));\n>\n> I would say that the problem was that validate_sth() subroutines returned\n> value of parameter if it was valid, which could be a problem if said value is\n> false-ish (e.g. validate_refname(\"0\"), or validate_pathname(\"0\")).\n>\n> Returning undef on invalid data newer was a problem, using 'return $input;'\n> on valid input was, especially that validate_sth() functions were ever used\n> in a conditional:\n>\n> if (!validate_sth($param)) {\n> die_error(...)\n> }\n\nCorrect, and I think that is exactly what the above three lines in\nthe proposed log message says.\n\n> While at it validate_sth() is not a best name for boolean predicate:\n> is_valid_sth() would be better, I think.\n\nYeah, I tend to agree. As we are doing a whole-sale API clean-up in\nthis patch, we might as well do that at the same time.\n\n>> Signed-off-by: Krzesimir Nowak \n>> ---\n>> gitweb/gitweb.perl | 45 +++++++++++++++++++++++++--------------------\n>> 1 file changed, 25 insertions(+), 20 deletions(-)\n>>\n>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n>> index 67415b9..3434602 100755\n>> --- a/gitweb/gitweb.perl\n>> +++ b/gitweb/gitweb.perl\n>> @@ -1419,63 +1419,68 @@ sub href {\n>> ## validation, quoting/unquoting and escaping\n>>\n>> sub validate_action {\n>> - my $input = shift || return undef;\n>> - return undef unless exists $actions{$input};\n>> - return $input;\n>> + my $input = shift;\n>> +\n>> + return 0 unless defined $input;\n>> + return 0 unless exists $actions{$input};\n>> + return 1;\n>> }\n>\n> The only change that needs to be doe is replacing\n>\n> return $input;\n>\n> with\n>\n> return 1;\n"},{"id":"231531","messageId":"CANQwDwfBWbgUfi8w+p3K1OySiMhp_OaxL2TGRyX6Qaeape=DCA@mail.gmail.com","threadId":"35475","inReplyTo":"1386164583-14109-6-git-send-email-krzesimir@endocode.com","subject":"Re: [PATCH 5/5] gitweb: Denote non-heads, non-remotes branches","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2013-12-04T18:54:59Z","receivedAt":"2013-12-04T18:54:59Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Wed, Dec 4, 2013 at 2:43 PM, Krzesimir Nowak wrote:\n\n> Given two branches residing in refs/heads/master and refs/wip/feature\n> the list-of-branches view will present them in following way:\n>\n> master\n> feature (wip)\n>\n> When getting a snapshot of a 'feature' branch, the tarball is going to\n> have name like 'project-wip-feature-.tgz'.\n>\n> Signed-off-by: Krzesimir Nowak \n\nVery nice feature, which allows to distinguish between refs/heads/feature\nand refs/wip/feature.\n\n> ---\n> gitweb/gitweb.perl | 27 +++++++++++++++++++++++++--\n> 1 file changed, 25 insertions(+), 2 deletions(-)\n>\n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 6d3d52d..9a63ea9 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -3739,8 +3739,14 @@ sub git_get_heads_list {\n> $ref_item{'fullname'} = $name;\n> my $strip_refs = join '|', map { quotemeta } get_branch_refs();\n> $name =~ s!^refs/($strip_refs|remotes)/!!;\n> + $ref_item{'name'} = $name;\n> + # for refs neither in 'heads' nor 'remotes' we want to\n> + # show their different ref dir\n\nPerhaps simply:\n\n + # for refs neither in 'heads' nor 'remotes' we want\nto show their ref dir\n\n> + my $ref_dir = (defined $1) ? $1 : '';\n> + if ($ref_dir ne '' and $ref_dir ne 'heads' and $ref_dir ne 'remotes') {\n> + $ref_item{'name'} .= ' (' . $ref_dir . ')';\n> + }\n>\n> - $ref_item{'name'} = $name;\n> $ref_item{'id'} = $hash;\n> $ref_item{'title'} = $title || '(no commit message)';\n> $ref_item{'epoch'} = $epoch;\n> @@ -7257,7 +7263,24 @@ sub snapshot_name {\n> # branches and other need shortened SHA-1 hash\n> my $strip_refs = join '|', map { quotemeta } get_branch_refs();\n> if ($hash =~ m!^refs/($strip_refs|remotes)/(.*)$!) {\n> - $ver = $1;\n> + my $ref_dir = $1;\n> + $ver = $2;\n> +\n> + if (defined $ref_dir) {\n> + # this is going to be a part of\n> + # filename, so lets stick to\n> + # alphanumerics, dashes and underlines\n> + # only - some filesystems do not like\n> + # some punctuation symbols for\n> + # example.\n> + $ref_dir =~ s/[^[:alnum:]_-]//g;\n> + }\n\nI think this safety replacement should apply also to other parts of filename,\nif it is to be used.\n\n$ref_dir should be compatibile with path-part - in loose form it is stored\non filesystem... though different filesystems might have different restrictions.\n\nOne thing we should worry about is '/' in hierarchical refdir names, e.g.\n'wip/jk', which needs to be replaced / sanitized somehow, though probably\nas 'wip-jk' or 'wip_jk' rather than 'wipjk', isn't it?\n\n> +\n> + # for refs not in heads nor remotes we want to\n> + # add a ref dir to archive name\n> + if ($ref_dir ne '' and $ref_dir ne 'heads' and $ref_dir ne 'remotes') {\n> + $ver = $ref_dir . '-' . $ver;\n> + }\n> }\n> $ver .= '-' . git_get_short_hash($project, $hash);\n> }\n> --\n> 1.8.3.1\n>\n\n\n\n-- \nJakub Narebski\n"},{"id":"231539","messageId":"xmqqwqjkjr7e.fsf@gitster.dls.corp.google.com","threadId":"35475","inReplyTo":"CACPiFCJON33dz64qSZDnrCo8hbVpbxk7k65xmW1oBiZ4Muzcbg@mail.gmail.com","subject":"Re: [PATCH 1/5] gitweb: Add a comment explaining the meaning of $/","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-04T20:28:53Z","receivedAt":"2013-12-04T20:28:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Langhoff writes:\n\n> On Wed, Dec 4, 2013 at 10:46 AM, Krzesimir Nowak wrote:\n>> On Wed, 2013-12-04 at 16:11 +0100, Jakub Narębski wrote:\n>>> On Wed, Dec 4, 2013 at 2:42 PM, Krzesimir Nowak wrote:\n>>>\n>>> > So future reader will know what does it mean without running \"perldoc\n>>> > perlvar\".\n>>>\n>>> Hmmm... shouldn't future reader know it anyway? It is not that cryptic.\n>>> I'd say it is idiomatic Perl.\n>>\n>> It's plainly obscure. And I think it is not that often used -\n>\n> It's classic Perl.\n>\n> Perhaps you'd want to \"use English;\" and call it\n> $INPUT_RECORD_SEPARATOR in a patch titled \"Make things readable to\n> non-Perl natives\".\n\nHmm, but do we want to see \"use English\" there in the first place?\n"},{"id":"231540","messageId":"xmqqsiu8jr2i.fsf@gitster.dls.corp.google.com","threadId":"35475","inReplyTo":"1386164583-14109-3-git-send-email-krzesimir@endocode.com","subject":"Re: [PATCH 2/5] gitweb: Move check-ref-format code into separate function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-04T20:31:49Z","receivedAt":"2013-12-04T20:31:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Krzesimir Nowak writes:\n\n> This check will be used in more than one place later.\n>\n> Signed-off-by: Krzesimir Nowak \n> Reviewed-by: Junio C Hamano \n\nAgain, I do not think I reviewed this exact version. Nor did I say\nthat use of the \"... or return undef\" is a good idea.\n\n> Reviewed-by: Jakub Narębski \n> ---\n> gitweb/gitweb.perl | 17 +++++++++++++----\n> 1 file changed, 13 insertions(+), 4 deletions(-)\n>\n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index ee61f9e..67415b9 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -1452,6 +1452,16 @@ sub validate_pathname {\n> \treturn $input;\n> }\n> \n> +sub check_ref_format {\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 +1472,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> +\tcheck_ref_format($input)\n> +\t\tor return undef;\n> \treturn $input;\n> }\n"},{"id":"231541","messageId":"xmqqob4wjqt3.fsf@gitster.dls.corp.google.com","threadId":"35475","inReplyTo":"1386164583-14109-1-git-send-email-krzesimir@endocode.com","subject":"Re: [PATCH 0/5] Show extra branch refs in gitweb v6","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-04T20:37:28Z","receivedAt":"2013-12-04T20:37:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Krzesimir Nowak writes:\n\n> First patch just adds a comment I though would be useful when trying\n> to understand how config parsing is done.\n>\n> Second patch splits some code to a function.\n>\n> Third patch fixes validation functions to return either 0 or 1,\n> instead of undef or passed $input.\n>\n> Fourth patch adds the extra-branch-feature and some documentation.\n>\n> Fifth patch adds some visual differentation of branches from\n> non-standard ref directories.\n>\n> Krzesimir Nowak (5):\n> gitweb: Add a comment explaining the meaning of $/\n> gitweb: Move check-ref-format code into separate function\n> gitweb: Return plain booleans in validation methods\n> gitweb: Add a feature for adding more branch refs\n> gitweb: Denote non-heads, non-remotes branches\n>\n> Documentation/gitweb.conf.txt | 27 +++++++\n> gitweb/gitweb.perl | 166 +++++++++++++++++++++++++++++++++---------\n> 2 files changed, 160 insertions(+), 33 deletions(-)\n\nThanks; I'll tentatively queue 2 thru 5 on 'pu', but people are\nfeeding good suggestions on it, so I'd expect further rerolls.\n\nA tip: \"git format-patch -v7 --cover-letter\" may be handy.\n"},{"id":"231577","messageId":"1386234974.2186.1.camel@localhost.localdomain","threadId":"35475","inReplyTo":"xmqqwqjkjr7e.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/5] gitweb: Add a comment explaining the meaning of $/","fromName":"Krzesimir Nowak","fromEmail":"krzesimir@endocode.com","sentAt":"2013-12-05T09:16:14Z","receivedAt":"2013-12-05T09:16:14Z","isPatch":true,"sender":{"key":"krzesimir@endocode.com","avatar":"https://avatars.githubusercontent.com/u/96081?v=4"},"body":"On Wed, 2013-12-04 at 12:28 -0800, Junio C Hamano wrote:\n> Martin Langhoff writes:\n> \n> > On Wed, Dec 4, 2013 at 10:46 AM, Krzesimir Nowak wrote:\n> >> On Wed, 2013-12-04 at 16:11 +0100, Jakub Narębski wrote:\n> >>> On Wed, Dec 4, 2013 at 2:42 PM, Krzesimir Nowak wrote:\n> >>>\n> >>> > So future reader will know what does it mean without running \"perldoc\n> >>> > perlvar\".\n> >>>\n> >>> Hmmm... shouldn't future reader know it anyway? It is not that cryptic.\n> >>> I'd say it is idiomatic Perl.\n> >>\n> >> It's plainly obscure. And I think it is not that often used -\n> >\n> > It's classic Perl.\n> >\n> > Perhaps you'd want to \"use English;\" and call it\n> > $INPUT_RECORD_SEPARATOR in a patch titled \"Make things readable to\n> > non-Perl natives\".\n> \n> Hmm, but do we want to see \"use English\" there in the first place?\n\nNevermind, I'm going to drop that patch.\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":"231578","messageId":"1386235120.2186.2.camel@localhost.localdomain","threadId":"35475","inReplyTo":"xmqqsiu8jr2i.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/5] gitweb: Move check-ref-format code into separate function","fromName":"Krzesimir Nowak","fromEmail":"krzesimir@endocode.com","sentAt":"2013-12-05T09:18:40Z","receivedAt":"2013-12-05T09:18:40Z","isPatch":true,"sender":{"key":"krzesimir@endocode.com","avatar":"https://avatars.githubusercontent.com/u/96081?v=4"},"body":"On Wed, 2013-12-04 at 12:31 -0800, Junio C Hamano wrote:\n> Krzesimir Nowak writes:\n> \n> > This check will be used in more than one place later.\n> >\n> > Signed-off-by: Krzesimir Nowak \n> > Reviewed-by: Junio C Hamano \n> \n> Again, I do not think I reviewed this exact version. Nor did I say\n> that use of the \"... or return undef\" is a good idea.\n\nOk, I'll drop them. Too much fuss over those lines.\n\n> \n> > Reviewed-by: Jakub Narębski \n> > ---\n> > gitweb/gitweb.perl | 17 +++++++++++++----\n> > 1 file changed, 13 insertions(+), 4 deletions(-)\n> >\n> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> > index ee61f9e..67415b9 100755\n> > --- a/gitweb/gitweb.perl\n> > +++ b/gitweb/gitweb.perl\n> > @@ -1452,6 +1452,16 @@ sub validate_pathname {\n> > \treturn $input;\n> > }\n> > \n> > +sub check_ref_format {\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 +1472,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> > +\tcheck_ref_format($input)\n> > +\t\tor return undef;\n> > \treturn $input;\n> > }\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":"231579","messageId":"1386235193.2186.3.camel@localhost.localdomain","threadId":"35475","inReplyTo":"CANQwDwfWPuH_OOU46yOrRN23qbppHBQbXEUnDcR37dF=93A-iA@mail.gmail.com","subject":"Re: [PATCH 2/5] gitweb: Move check-ref-format code into separate function","fromName":"Krzesimir Nowak","fromEmail":"krzesimir@endocode.com","sentAt":"2013-12-05T09:19:53Z","receivedAt":"2013-12-05T09:19:53Z","isPatch":true,"sender":{"key":"krzesimir@endocode.com","avatar":"https://avatars.githubusercontent.com/u/96081?v=4"},"body":"On Wed, 2013-12-04 at 16:56 +0100, Jakub Narębski wrote:\n> On Wed, Dec 4, 2013 at 2:43 PM, Krzesimir Nowak wrote:\n> \n> > This check will be used in more than one place later.\n> >\n> > Signed-off-by: Krzesimir Nowak \n> > Reviewed-by: Junio C Hamano \n> > Reviewed-by: Jakub Narębski \n> \n> All right, that is nice refactoring.\n> \n> > ---\n> > gitweb/gitweb.perl | 17 +++++++++++++----\n> > 1 file changed, 13 insertions(+), 4 deletions(-)\n> >\n> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> > index ee61f9e..67415b9 100755\n> > --- a/gitweb/gitweb.perl\n> > +++ b/gitweb/gitweb.perl\n> > @@ -1452,6 +1452,16 @@ sub validate_pathname {\n> > return $input;\n> > }\n> >\n> > +sub check_ref_format {\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> > @@ -1462,10 +1472,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> > + check_ref_format($input)\n> > + or return undef;\n> > return $input;\n> > }\n> \n> Right, check_ref_format() has name after git-check-ref-format...\n> though... check_ref_format() or die doesn't read completely\n> naturally...\n> \n\nOk, I'll rename it to is_valid_ref_format.\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":"231580","messageId":"1386235422.2186.7.camel@localhost.localdomain","threadId":"35475","inReplyTo":"CANQwDweGgQpnqDqaekBWt-rczjHkJEmFyXW9qDh2En2r=ZXbMw@mail.gmail.com","subject":"Re: [PATCH 3/5] gitweb: Return plain booleans in validation methods","fromName":"Krzesimir Nowak","fromEmail":"krzesimir@endocode.com","sentAt":"2013-12-05T09:23:42Z","receivedAt":"2013-12-05T09:23:42Z","isPatch":true,"sender":{"key":"krzesimir@endocode.com","avatar":"https://avatars.githubusercontent.com/u/96081?v=4"},"body":"On Wed, 2013-12-04 at 17:07 +0100, Jakub Narębski wrote:\n> On Wed, Dec 4, 2013 at 2:43 PM, Krzesimir Nowak wrote:\n> \n> > Users of validate_* passing \"0\" might get failures on correct name\n> > because of coercion of \"0\" to false in code like:\n> > die_error(500, \"invalid ref\") unless (check_ref_format (\"0\"));\n> \n> I would say that the problem was that validate_sth() subroutines returned\n> value of parameter if it was valid, which could be a problem if said value is\n> false-ish (e.g. validate_refname(\"0\"), or validate_pathname(\"0\")).\n\nThat's what I meant.\n\n> \n> Returning undef on invalid data newer was a problem, using 'return $input;'\n> on valid input was, especially that validate_sth() functions were ever used\n> in a conditional:\n> \n> if (!validate_sth($param)) {\n> die_error(...)\n> }\n> \n> While at it validate_sth() is not a best name for boolean predicate:\n> is_valid_sth() would be better, I think.\n\nOk, I'll rename those.\n\n> \n> > Signed-off-by: Krzesimir Nowak \n> > ---\n> > gitweb/gitweb.perl | 45 +++++++++++++++++++++++++--------------------\n> > 1 file changed, 25 insertions(+), 20 deletions(-)\n> >\n> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> > index 67415b9..3434602 100755\n> > --- a/gitweb/gitweb.perl\n> > +++ b/gitweb/gitweb.perl\n> > @@ -1419,63 +1419,68 @@ sub href {\n> > ## validation, quoting/unquoting and escaping\n> >\n> > sub validate_action {\n> > - my $input = shift || return undef;\n> > - return undef unless exists $actions{$input};\n> > - return $input;\n> > + my $input = shift;\n> > +\n> > + return 0 unless defined $input;\n> > + return 0 unless exists $actions{$input};\n> > + return 1;\n> > }\n> \n> The only change that needs to be doe is replacing\n> \n> return $input;\n> \n> with\n> \n> return 1;\n> \n\nI prefer to use zeros instead of undefs - one might wonder if that undef\nis somewhat special that we can't use 0.\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":"231581","messageId":"1386237635.2186.22.camel@localhost.localdomain","threadId":"35475","inReplyTo":"CANQwDwe+a2P0Jxqw0k7sHWv3exdb4k+NU3jL3ogR-rcetd82TQ@mail.gmail.com","subject":"Re: [PATCH 4/5] gitweb: Add a feature for adding more branch refs","fromName":"Krzesimir Nowak","fromEmail":"krzesimir@endocode.com","sentAt":"2013-12-05T10:00:35Z","receivedAt":"2013-12-05T10:00:35Z","isPatch":true,"sender":{"key":"krzesimir@endocode.com","avatar":"https://avatars.githubusercontent.com/u/96081?v=4"},"body":"On Wed, 2013-12-04 at 19:06 +0100, Jakub Narębski wrote:\n> On Wed, Dec 4, 2013 at 2:43 PM, Krzesimir Nowak wrote:\n> >\n> > Allow extra-branch-refs feature to tell gitweb to show refs from\n> > additional hierarchies in addition to branches in the list-of-branches\n> > view.\n> >\n> > Signed-off-by: Krzesimir Nowak \n> > Reviewed-by: Junio C Hamano \n> > Reviewed-by: Jakub Narębski \n> \n> This version is Helped-by (maybe), but not (yet!) Reviewed-by.\n> \n> > ---\n> > Documentation/gitweb.conf.txt | 37 +++++++++++++++++++\n> > gitweb/gitweb.perl | 85 +++++++++++++++++++++++++++++++++++++------\n> > 2 files changed, 110 insertions(+), 12 deletions(-)\n> >\n> > diff --git a/Documentation/gitweb.conf.txt b/Documentation/gitweb.conf.txt\n> > index e2113d9..5a77452 100644\n> > --- a/Documentation/gitweb.conf.txt\n> > +++ b/Documentation/gitweb.conf.txt\n> > @@ -849,6 +849,43 @@ time zones in the form of \"+/-HHMM\", such as \"+0200\".\n> > +\n> > Project specific override is not supported.\n> >\n> > +extra-branch-refs::\n> > + List of additional directories under \"refs\" which are going to\n> > + be used as branch refs. For example if you have a gerrit setup\n> > + where all branches under refs/heads/ are official,\n> > + push-after-review ones and branches under refs/sandbox/,\n> > + refs/wip and refs/other are user ones where permissions are\n> > + much wider, then you might want to set this variable as\n> > + follows:\n> > ++\n> > +--------------------------------------------------------------------------------\n> > +$feature{'extra-branch-refs'}{'default'} =\n> > + ['sandbox', 'wip', 'other'];\n> > +--------------------------------------------------------------------------------\n> > ++\n> > +If overriding was enabled then this feature can be configured on a\n> \n> s/was/is/;\n> \n> Perhaps it would better read as\n> \n> This feature can be configured on per-repository basis after setting\n> $feature{'extra-branch-refs'}{'override'} to true, via repository's\n> `gitweb.extraBranchRefs` ...\n\nI see that you also insist on using camelCase for config variables. I\nwill rephrase it.\n\n> \n> > +per-repository basis via repository's `gitweb.extrabranchrefs`\n> > +configuration variable, which contains a space separated list of\n> > +refs. An example:\n> > ++\n> > +--------------------------------------------------------------------------------\n> > +[gitweb]\n> > + extrabranchrefs = sandbox wip other\n> > +--------------------------------------------------------------------------------\n> \n> O.K.\n> \n> > ++\n> > +The gitweb.extrabranchrefs is actually a multi-valued configuration\n> > +variable, so following example is also correct and the result is the\n> > +same as of the snippet above:\n> > ++\n> > +--------------------------------------------------------------------------------\n> > +[gitweb]\n> > + extrabranchrefs = sandbox\n> > + extrabranchrefs = wip other\n> > +--------------------------------------------------------------------------------\n> \n> I think this part should be better left for a separate patch. There is\n> important difference between single-valued and multi-valued configuration\n> variable: with single-valued later occurrences override earlier ones,\n> which includes settings in more specific config file (e.g. per-repository)\n> overriding setting in more general one (e.g. per-user or system-wide).\n> \n> With multi-valued we won't be able to override earlier / more generic\n> settings... well, unless we add support for no-value, or empty-value\n> as clearer, i.e.\n> \n> [gitweb]\n> extrabranchrefs = sandbox\n> extrabranchrefs\n> # or extrabranchrefs =\n> extrabranchrefs = wip other\n> \n> resulting in ('wip', 'other').\n\nThat point in this example is a bit moot IMO - if you don't want sandbox\nref to appear in list of branches view then just delete the offending\nline.\n\nI also made a small experiment. In per-instance config I have\n$feature{'extra-branch-refs'}{'default'} = ['wip'];\n$feature{'extra-branch-refs'}{'override'} = 1;\n\nIn per-repository config I have:\n[gitweb]\n\textrabranchrefs = sandbox\n\textrabranchrefs = other\n\nList of branches view shows only branches from sandbox and other. So\nclearly per-repository config overrides per-instance config.\n\n> \n> > ++\n> > +It is an error to specify a ref that does not pass \"git check-ref-format\"\n> > +scrutiny. Duplicated values are filtered.\n> > +\n> \n> Hmmm... 'snapshot' feature ignores invalid values, but in this case\n> formerly valid compression schemes might get invalid via tightening\n> %known_snapshot_formats, and we don't want existing config getting\n> suddenly invalid.\n> \n\nSo, should I just ignore the invalid refs or treat them as errors?\n\n> [cut]\n> \n> Nice!\n> \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":"231583","messageId":"CANQwDwdfbAyJyCWwEXBDp5BGz1JvVj_eUn5NOCWEthUtKfF5Lg@mail.gmail.com","threadId":"35475","inReplyTo":"1386237635.2186.22.camel@localhost.localdomain","subject":"Re: [PATCH 4/5] gitweb: Add a feature for adding more branch refs","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2013-12-05T11:40:42Z","receivedAt":"2013-12-05T11:40:42Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, Dec 5, 2013 at 11:00 AM, Krzesimir Nowak wrote:\n> On Wed, 2013-12-04 at 19:06 +0100, Jakub Narębski wrote:\n>> On Wed, Dec 4, 2013 at 2:43 PM, Krzesimir Nowak wrote:\n\n>>> +The gitweb.extrabranchrefs is actually a multi-valued configuration\n>>> +variable, so following example is also correct and the result is the\n>>> +same as of the snippet above:\n>>> ++\n>>> +--------------------------------------------------------------------------------\n>>> +[gitweb]\n>>> + extrabranchrefs = sandbox\n>>> + extrabranchrefs = wip other\n>>> +--------------------------------------------------------------------------------\n>>\n>> I think this part should be better left for a separate patch. There is\n>> important difference between single-valued and multi-valued configuration\n>> variable: with single-valued later occurrences override earlier ones,\n>> which includes settings in more specific config file (e.g. per-repository)\n>> overriding setting in more general one (e.g. per-user or system-wide).\n>>\n>> With multi-valued we won't be able to override earlier / more generic\n>> settings... well, unless we add support for no-value, or empty-value\n>> as clearer, i.e.\n>>\n>> [gitweb]\n>> extrabranchrefs = sandbox\n>> extrabranchrefs\n>> # or extrabranchrefs =\n>> extrabranchrefs = wip other\n>>\n>> resulting in ('wip', 'other').\n\nWhat I didn't notice is that gitweb already supports and *uses* multi-value\nconfiguration variables: gitweb.url and gitweb.ctag (note: the feature is\ncalled 'ctags' and is not overridable), though not for %feature-s.\n\nSo the point is moot, sorry for distraction.\n\nBTW. there is config_to_multi() subroutine that you can use.\n\n> That point in this example is a bit moot IMO - if you don't want sandbox\n> ref to appear in list of branches view then just delete the offending\n> line.\n\nI was thinking about more complicated situation: if you have gitweb\nconfigured in \"scan for repositories\" mode (like mod_homedir), then\nrepository owner may change gitweb configuration for own repository\nvia repo config file. But gitweb reads also web server user config file\nand system wide git config file (/etc/gitconfig). If gitweb.extraBranchRefs\nis set in one of those files, repo owner cannot override, but only add\nto it.\n\nBut I think this is worrying about nothing. I'm sorry for distraction.\n\n> I also made a small experiment. In per-instance config I have\n> $feature{'extra-branch-refs'}{'default'} = ['wip'];\n> $feature{'extra-branch-refs'}{'override'} = 1;\n>\n> In per-repository config I have:\n> [gitweb]\n> extrabranchrefs = sandbox\n> extrabranchrefs = other\n>\n> List of branches view shows only branches from sandbox and other. So\n> clearly per-repository config overrides per-instance config.\n\nThat's why is called 'override'... but it is not what I was worrying about,\nsee above.\n\n-- \nJakub Narebski\n"},{"id":"231619","messageId":"xmqq61r3i2ol.fsf@gitster.dls.corp.google.com","threadId":"35475","inReplyTo":"1386235422.2186.7.camel@localhost.localdomain","subject":"Re: [PATCH 3/5] gitweb: Return plain booleans in validation methods","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-05T18:16:10Z","receivedAt":"2013-12-05T18:16:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Krzesimir Nowak writes:\n\n> On Wed, 2013-12-04 at 17:07 +0100, Jakub Narębski wrote:\n>> The only change that needs to be doe is replacing\n>> \n>> return $input;\n>> \n>> with\n>> \n>> return 1;\n>> \n>\n> I prefer to use zeros instead of undefs - one might wonder if that undef\n> is somewhat special that we can't use 0.\n\nFor Perl speakers, I suspect the code gives a totally opposite\nimpression. Normal \"false\" return from a sub, when there is no\nspecial need, is to return an undef from it, and a \"return 0\" would\nmake the readers wonder if there is something special about the way\nthe returned value has to be numeric zero, no?\n"},{"id":"231623","messageId":"CANQwDwdw6MQW2wWvL=GmwYeDKF-gikNx55QdYH2oD50ZZUcPRw@mail.gmail.com","threadId":"35475","inReplyTo":"xmqq61r3i2ol.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 3/5] gitweb: Return plain booleans in validation methods","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2013-12-05T19:11:23Z","receivedAt":"2013-12-05T19:11:23Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, Dec 5, 2013 at 7:16 PM, Junio C Hamano wrote:\n> Krzesimir Nowak writes:\n>> On Wed, 2013-12-04 at 17:07 +0100, Jakub Narębski wrote:\n\n>>> The only change that needs to be done is replacing\n>>>\n>>> return $input;\n>>>\n>>> with\n>>>\n>>> return 1;\n>>>\n>>\n>> I prefer to use zeros instead of undefs - one might wonder if that undef\n>> is somewhat special that we can't use 0.\n>\n> For Perl speakers, I suspect the code gives a totally opposite\n> impression. Normal \"false\" return from a sub, when there is no\n> special need, is to return an undef from it, and a \"return 0\" would\n> make the readers wonder if there is something special about the way\n> the returned value has to be numeric zero, no?\n\nOr even plain \"return;\" (see explanation for policy in [1])...\nthough for functions returning scalar it is recommended\nto use explicit \"return 0;\" (or \"return undef;\").\n\nAnyway, it is easier to see the change and intention of the change\nif all that is changed id \"return $input;\" to \"return 1;\"\n\nBut I am not against \"return 0;\" on validation error (would putting\nit in separate patch make review easier, or just pointlessly proliferate\npatches?).\n\n[1]: http://search.cpan.org/~thaljef/Perl-Critic-1.121/lib/Perl/Critic/Policy/Subroutines/ProhibitExplicitReturnUndef.pm\n-- \nJakub Narebski\n"},{"id":"231631","messageId":"xmqqob4v3w3w.fsf@gitster.dls.corp.google.com","threadId":"35475","inReplyTo":"CANQwDwdw6MQW2wWvL=GmwYeDKF-gikNx55QdYH2oD50ZZUcPRw@mail.gmail.com","subject":"Re: [PATCH 3/5] gitweb: Return plain booleans in validation methods","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-05T20:01:55Z","receivedAt":"2013-12-05T20:01:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narębski writes:\n\n> But I am not against \"return 0;\" on validation error (would putting\n> it in separate patch make review easier, or just pointlessly proliferate\n> patches?).\n\nOK.\n"},{"id":"231857","messageId":"1386691473.2143.1.camel@localhost.localdomain","threadId":"35475","inReplyTo":"1386237635.2186.22.camel@localhost.localdomain","subject":"Re: [PATCH 4/5] gitweb: Add a feature for adding more branch refs","fromName":"Krzesimir Nowak","fromEmail":"krzesimir@endocode.com","sentAt":"2013-12-10T16:04:33Z","receivedAt":"2013-12-10T16:04:33Z","isPatch":true,"sender":{"key":"krzesimir@endocode.com","avatar":"https://avatars.githubusercontent.com/u/96081?v=4"},"body":"On Thu, 2013-12-05 at 11:00 +0100, Krzesimir Nowak wrote:\n> On Wed, 2013-12-04 at 19:06 +0100, Jakub Narębski wrote:\n> > On Wed, Dec 4, 2013 at 2:43 PM, Krzesimir Nowak wrote:\n> > > ++\n> > > +It is an error to specify a ref that does not pass \"git check-ref-format\"\n> > > +scrutiny. Duplicated values are filtered.\n> > > +\n> > \n> > Hmmm... 'snapshot' feature ignores invalid values, but in this case\n> > formerly valid compression schemes might get invalid via tightening\n> > %known_snapshot_formats, and we don't want existing config getting\n> > suddenly invalid.\n> > \n> \n> So, should I just ignore the invalid refs or treat them as errors?\n\nPing, I'd like to know the answer for this question before I roll out\nanother set of patches - there seem to be no agreement over it. Thanks!\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":"231864","messageId":"xmqq4n6gv8n0.fsf@gitster.dls.corp.google.com","threadId":"35475","inReplyTo":"CANQwDwe+a2P0Jxqw0k7sHWv3exdb4k+NU3jL3ogR-rcetd82TQ@mail.gmail.com","subject":"Re: [PATCH 4/5] gitweb: Add a feature for adding more branch refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-10T18:54:59Z","receivedAt":"2013-12-10T18:54:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narębski writes:\n\n> With multi-valued we won't be able to override earlier / more generic\n> settings... well, unless we add support for no-value, or empty-value\n> as clearer, i.e.\n>\n> [gitweb]\n> extrabranchrefs = sandbox\n> extrabranchrefs\n> # or extrabranchrefs =\n> extrabranchrefs = wip other\n>\n> resulting in ('wip', 'other').\n\nPlease don't, unless you are going to change the entire config\nmachinery (including config.c bits that are used by the rest of the\nsystem) to follow such a convention to \"clear settings so far read\".\n\n>> ++\n>> +It is an error to specify a ref that does not pass \"git check-ref-format\"\n>> +scrutiny. Duplicated values are filtered.\n>> +\n>\n> Hmmm... 'snapshot' feature ignores invalid values, but in this case\n> formerly valid compression schemes might get invalid via tightening\n> %known_snapshot_formats, and we don't want existing config getting\n> suddenly invalid.\n\nSorry, but what does check-ref-format have to do with \"formerly\nvalid compression scheme that suddenly gets invalidated\"???\n"},{"id":"231868","messageId":"CANQwDwdoba5SbiiF1ZA2LHCwM-5KnEkVek5ekvrrQ-minLYNew@mail.gmail.com","threadId":"35475","inReplyTo":"xmqq4n6gv8n0.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 4/5] gitweb: Add a feature for adding more branch refs","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2013-12-10T19:06:25Z","receivedAt":"2013-12-10T19:06:25Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Tue, Dec 10, 2013 at 7:54 PM, Junio C Hamano wrote:\n> Jakub Narębski writes:\n>\n>> With multi-valued we won't be able to override earlier / more generic\n>> settings... well, unless we add support for no-value, or empty-value\n>> as clearer, i.e.\n>>\n>> [gitweb]\n>> extrabranchrefs = sandbox\n>> extrabranchrefs\n>> # or extrabranchrefs =\n>> extrabranchrefs = wip other\n>>\n>> resulting in ('wip', 'other').\n>\n> Please don't, unless you are going to change the entire config\n> machinery (including config.c bits that are used by the rest of the\n> system) to follow such a convention to \"clear settings so far read\".\n\nRight. Also I haven't noticed that gitweb already uses some\nmulti-valued config variables, though not for %features.\n\nPlease disregard this.\n\n>>> ++\n>>> +It is an error to specify a ref that does not pass \"git check-ref-format\"\n>>> +scrutiny. Duplicated values are filtered.\n>>> +\n>>\n>> Hmmm... 'snapshot' feature ignores invalid values, but in this case\n>> formerly valid compression schemes might get invalid via tightening\n>> %known_snapshot_formats, and we don't want existing config getting\n>> suddenly invalid.\n>\n> Sorry, but what does check-ref-format have to do with \"formerly\n> valid compression scheme that suddenly gets invalidated\"???\n\nWhat I started to write was that 'snapshot' feature ignores invalid\nvalues, but halfway through writing it out I have noticed that the\nsituation is really different.\n\nIn 'snapshot' case one can have formerly valid per-repository config,\nmade invalid by gitweb admin tightening %known_snapshot_formats\n(for example removing some compression format due to bug). We\ndo not want for repository to stop being displayed because of something\nthat is outside control of repository owner.\n\nIn 'check-ref-format' feature we have ref either valid or not. This does\nnot depend on external factors. Something that is valid ref would\nremain valid ref.\n\nSo 'snaphot' being forgiving doesn't mean that 'check-ref-format'\nshould be forgiving.\n\n-- \nJakub Narebski\n"},{"id":"231871","messageId":"xmqqr49ktrso.fsf@gitster.dls.corp.google.com","threadId":"35475","inReplyTo":"CANQwDwdoba5SbiiF1ZA2LHCwM-5KnEkVek5ekvrrQ-minLYNew@mail.gmail.com","subject":"Re: [PATCH 4/5] gitweb: Add a feature for adding more branch refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-10T19:44:07Z","receivedAt":"2013-12-10T19:44:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narębski writes:\n\n> So 'snaphot' being forgiving doesn't mean that 'check-ref-format'\n> should be forgiving.\n\nOK, thanks for clarifying.\n"}]}