{"thread":{"id":"35464","subject":"[PATCH 1/3] gitweb: Move check-ref-format code into separate function","startedAt":"2013-12-03T14:56:40Z","lastAt":"2013-12-04T17:57:31Z","messageCount":14,"participants":["Krzesimir Nowak","Junio C Hamano","Jakub Narębski"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"231432","messageId":"1386082603-8404-1-git-send-email-krzesimir@endocode.com","threadId":"35464","inReplyTo":null,"subject":"[PATCH 0/3] Show extra branch refs in gitweb","fromName":"Krzesimir Nowak","fromEmail":"krzesimir@endocode.com","sentAt":"2013-12-03T14:56:40Z","receivedAt":"2013-12-03T14:56:40Z","isPatch":true,"sender":{"key":"krzesimir@endocode.com","avatar":"https://avatars.githubusercontent.com/u/96081?v=4"},"body":"First patch just splits some code to a function, second patch adds the\nextra-branch-refs feature and third one adds some visual\ndifferentation of branches from non-standard ref directories.\n\nKrzesimir Nowak (3):\n gitweb: Move check-ref-format code into separate function\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 | 120 +++++++++++++++++++++++++++++++++++-------\n 2 files changed, 129 insertions(+), 18 deletions(-)\n\n-- \n1.8.3.1\n"},{"id":"231429","messageId":"1386082603-8404-2-git-send-email-krzesimir@endocode.com","threadId":"35464","inReplyTo":"1386082603-8404-1-git-send-email-krzesimir@endocode.com","subject":"[PATCH 1/3] gitweb: Move check-ref-format code into separate function","fromName":"Krzesimir Nowak","fromEmail":"krzesimir@endocode.com","sentAt":"2013-12-03T14:56:41Z","receivedAt":"2013-12-03T14:56:41Z","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 \nReviewed-by: Eric Sunshine \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 68c77f6..f7730d7 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+\t$input = check_ref_format($input)\n+\t\tor return undef;\n \treturn $input;\n }\n \n-- \n1.8.3.1\n"},{"id":"231431","messageId":"1386082603-8404-3-git-send-email-krzesimir@endocode.com","threadId":"35464","inReplyTo":"1386082603-8404-1-git-send-email-krzesimir@endocode.com","subject":"[PATCH 2/3] gitweb: Add a feature for adding more branch refs","fromName":"Krzesimir Nowak","fromEmail":"krzesimir@endocode.com","sentAt":"2013-12-03T14:56:42Z","receivedAt":"2013-12-03T14:56:42Z","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 \nReviewed-by: Eric Sunshine \n---\n Documentation/gitweb.conf.txt | 27 +++++++++++++++\n gitweb/gitweb.perl | 76 ++++++++++++++++++++++++++++++++++++-------\n 2 files changed, 91 insertions(+), 12 deletions(-)\n\ndiff --git a/Documentation/gitweb.conf.txt b/Documentation/gitweb.conf.txt\nindex e2113d9..3de8e14 100644\n--- a/Documentation/gitweb.conf.txt\n+++ b/Documentation/gitweb.conf.txt\n@@ -849,6 +849,33 @@ 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+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 f7730d7..6326075 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,17 @@ 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('extra_branch_refs');\n+\n+\tif ($values) {\n+\t\t@branch_refs = split /\\s+/, $values;\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 +681,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 +1150,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 +1168,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@@ -2524,6 +2568,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@@ -2531,12 +2576,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@@ -2549,7 +2599,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@@ -3202,7 +3252,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@@ -3653,7 +3703,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@@ -3671,7 +3721,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@@ -7188,7 +7239,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":"231430","messageId":"1386082603-8404-4-git-send-email-krzesimir@endocode.com","threadId":"35464","inReplyTo":"1386082603-8404-1-git-send-email-krzesimir@endocode.com","subject":"[PATCH 3/3] gitweb: Denote non-heads, non-remotes branches.","fromName":"Krzesimir Nowak","fromEmail":"krzesimir@endocode.com","sentAt":"2013-12-03T14:56:43Z","receivedAt":"2013-12-03T14:56:43Z","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 \nReviewed-by: Junio C Hamano \nReviewed-by: Jakub Narębski \nReviewed-by: Eric Sunshine \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 6326075..eb8d962 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -3723,8 +3723,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@@ -7241,7 +7247,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":"231448","messageId":"xmqqli01ojiz.fsf@gitster.dls.corp.google.com","threadId":"35464","inReplyTo":"1386082603-8404-3-git-send-email-krzesimir@endocode.com","subject":"Re: [PATCH 2/3] gitweb: Add a feature for adding more branch refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-03T18:51:16Z","receivedAt":"2013-12-03T18:51:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Krzesimir Nowak writes:\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\nPlease do not add \"Reviewed-by:\" like this; I've never reviewed this\nversion of the patch.\n\nThese are to be added only when you re-send, for final application,\nthe version as exactly reviewed, or adjusted a previous version you\ngot reviewed in a way that match suggestions given by reviewers.\n\n> Reviewed-by: Jakub Narębski \n> Reviewed-by: Eric Sunshine \n"},{"id":"231451","messageId":"xmqqd2ldoj0s.fsf@gitster.dls.corp.google.com","threadId":"35464","inReplyTo":"1386082603-8404-2-git-send-email-krzesimir@endocode.com","subject":"Re: [PATCH 1/3] gitweb: Move check-ref-format code into separate function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-03T19:02:11Z","receivedAt":"2013-12-03T19:02:11Z","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> Reviewed-by: Jakub Narębski \n> Reviewed-by: Eric Sunshine \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 68c77f6..f7730d7 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\nSo far, so good.\n\n> +\t# check git-check-ref-format restrictions\n> +\t$input = check_ref_format($input)\n> +\t\tor return undef;\n> \treturn $input;\n\nHmmm. Why do you need \"or return under\" here? It would\nnot hurt too much per-se (strictly speaking, if the $input were a\nstring \"0\", this will return undef instead of \"0\", which should be\nan OK name as far as the regexp is concerned), but it seems to be\nmaking the logic unnecessarily complex for no real gain.\n\n> }\n"},{"id":"231459","messageId":"CANQwDwd0_bgKfjsRFjohmzBAnN7vDm-pYYsfe0Q71Za2K7Hw4w@mail.gmail.com","threadId":"35464","inReplyTo":"xmqqd2ldoj0s.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/3] gitweb: Move check-ref-format code into separate function","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2013-12-03T19:38:39Z","receivedAt":"2013-12-03T19:38:39Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Tue, Dec 3, 2013 at 8:02 PM, Junio C Hamano wrote:\n> Krzesimir Nowak writes:\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>> @@ -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>\n> So far, so good.\n>\n>> + # check git-check-ref-format restrictions\n>> + $input = check_ref_format($input)\n>> + or return undef;\n>> return $input;\n>\n> Hmmm. Why do you need \"or return under\" here? It would\n> not hurt too much per-se (strictly speaking, if the $input were a\n> string \"0\", this will return undef instead of \"0\", which should be\n> an OK name as far as the regexp is concerned), but it seems to be\n> making the logic unnecessarily complex for no real gain.\n\nI think this simply follows \"$input = validate_sth($input) or return undef;\"\npattern used in this area of gitweb code (perhaps mis-used).\n\nStricly speaking pure refactoring (no functional change, e.g. no assign\nto $input) would be \"check_ref_format($input) or return undef;\", or even\n\"return check_ref_format($input);\" if we keep check_ref_format() passthru\non valid refname.\n\n-- \nJakub Narebski\n"},{"id":"231460","messageId":"xmqqzjohn2bq.fsf@gitster.dls.corp.google.com","threadId":"35464","inReplyTo":"CANQwDwd0_bgKfjsRFjohmzBAnN7vDm-pYYsfe0Q71Za2K7Hw4w@mail.gmail.com","subject":"Re: [PATCH 1/3] gitweb: Move check-ref-format code into separate function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-03T19:48:09Z","receivedAt":"2013-12-03T19:48:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narębski writes:\n\n> Stricly speaking pure refactoring (no functional change, e.g. no assign\n> to $input) would be \"check_ref_format($input) or return undef;\", or even\n> \"return check_ref_format($input);\" if we keep check_ref_format() passthru\n> on valid refname.\n\nExactly.\n"},{"id":"231465","messageId":"xmqqr49tn11c.fsf@gitster.dls.corp.google.com","threadId":"35464","inReplyTo":"1386082603-8404-3-git-send-email-krzesimir@endocode.com","subject":"Re: [PATCH 2/3] gitweb: Add a feature for adding more branch refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-03T20:15:59Z","receivedAt":"2013-12-03T20:15:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Krzesimir Nowak writes:\n\n> @@ -626,6 +640,17 @@ 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('extra_branch_refs');\n\nHmph. Three points.\n\n* Almost all callers of this function use\n\n my ($val) = git_get_project_config(...);\n my @val = git_get_project_config(...);\n\n to expect that the function returns a list of things (and grab the\n first one among them, not the length of the list). Shoudln't this\n part do the same?\n\n\n* Wouldn't this be a good candidate for a multi-valued configuration\n variable, e.g. shouldn't this\n\n\t[gitweb]\n\t\textraBranchRefs = wip\n\t\textraBranchRefs = sandbox other\n\n be parsed as a three-item list, qw(wip sandbox other)?\n \n\n* I think the $key parameter to git_get_project_config() eventually\n is used to look up a key in the Git-style configuration file, and\n the 'words_with_underscore' goes against our convention (cf. see\n how 'show-sizes' feature is spelled as 'showsizes' there).\n\n> +\tif ($values) {\n> +\t\t@branch_refs = split /\\s+/, $values;\n> +\t}\n> +\n> +\treturn @branch_refs;\n> +}\n"},{"id":"231468","messageId":"CANQwDwcpkbbfND9MmB9wGcL7mrhV_Mxb1amvGPgWeR344_fcfw@mail.gmail.com","threadId":"35464","inReplyTo":"xmqqr49tn11c.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/3] gitweb: Add a feature for adding more branch refs","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2013-12-03T20:38:07Z","receivedAt":"2013-12-03T20:38:07Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Tue, Dec 3, 2013 at 9:15 PM, Junio C Hamano wrote:\n> Krzesimir Nowak writes:\n>\n>> @@ -626,6 +640,17 @@ sub feature_avatar {\n>> return @val ? @val : @_;\n>> }\n>>\n>> +sub feature_extra_branch_refs {\n>> + my (@branch_refs) = @_;\n>> + my $values = git_get_project_config('extra_branch_refs');\n>\n> Hmph. Three points.\n>\n> * Almost all callers of this function use\n>\n> my ($val) = git_get_project_config(...);\n> my @val = git_get_project_config(...);\n>\n> to expect that the function returns a list of things (and grab the\n> first one among them, not the length of the list). Shouldn't this\n> part do the same?\n\nRight. feature_snapshot() has here\n\n my (@fmts) = @_;\n my ($val) = git_get_project_config('snapshot');\n\n...though git_get_project_config returns scalar.\n\n> * Wouldn't this be a good candidate for a multi-valued configuration\n> variable, e.g. shouldn't this\n>\n> [gitweb]\n> extraBranchRefs = wip\n> extraBranchRefs = sandbox other\n>\n> be parsed as a three-item list, qw(wip sandbox other)?\n\nThis would require changes in git_get_project_config(), which would\nneed to be able to deal with multi-valued result (it caches these\nresults, so we pay only one cost of `git config` call).\n\n> * I think the $key parameter to git_get_project_config() eventually\n> is used to look up a key in the Git-style configuration file, and\n> the 'words_with_underscore' goes against our convention (cf. see\n> how 'show-sizes' feature is spelled as 'showsizes' there).\n\nErrr... actually git_get_project_config() strips '_' from $key, though\nnot for some strange reason '-'.\n\nBTW. though it is 'showsizes' in code, it usually is 'showSizes' in\nconfig file (camelCase convention, lowercased by git-config).\n\n>> + if ($values) {\n>> + @branch_refs = split /\\s+/, $values;\n>> + }\n>> +\n>> + return @branch_refs;\n>> +}\n\n\n\n-- \nJakub Narebski\n"},{"id":"231503","messageId":"1386161398.2173.9.camel@localhost.localdomain","threadId":"35464","inReplyTo":"CANQwDwcpkbbfND9MmB9wGcL7mrhV_Mxb1amvGPgWeR344_fcfw@mail.gmail.com","subject":"Re: [PATCH 2/3] gitweb: Add a feature for adding more branch refs","fromName":"Krzesimir Nowak","fromEmail":"krzesimir@endocode.com","sentAt":"2013-12-04T12:49:58Z","receivedAt":"2013-12-04T12:49:58Z","isPatch":true,"sender":{"key":"krzesimir@endocode.com","avatar":"https://avatars.githubusercontent.com/u/96081?v=4"},"body":"On Tue, 2013-12-03 at 21:38 +0100, Jakub Narębski wrote:\n> On Tue, Dec 3, 2013 at 9:15 PM, Junio C Hamano wrote:\n> > Krzesimir Nowak writes:\n> >\n> >> @@ -626,6 +640,17 @@ sub feature_avatar {\n> >> return @val ? @val : @_;\n> >> }\n> >>\n> >> +sub feature_extra_branch_refs {\n> >> + my (@branch_refs) = @_;\n> >> + my $values = git_get_project_config('extra_branch_refs');\n> >\n> > Hmph. Three points.\n> >\n> > * Almost all callers of this function use\n> >\n> > my ($val) = git_get_project_config(...);\n> > my @val = git_get_project_config(...);\n> >\n> > to expect that the function returns a list of things (and grab the\n> > first one among them, not the length of the list). Shouldn't this\n> > part do the same?\n> \n> Right. feature_snapshot() has here\n> \n> my (@fmts) = @_;\n> my ($val) = git_get_project_config('snapshot');\n> \n> ...though git_get_project_config returns scalar.\n\nSo what's the point of it? 'my @val = git_get_project_config ()' just\ncreates an array with one element.\n\n> \n> > * Wouldn't this be a good candidate for a multi-valued configuration\n> > variable, e.g. shouldn't this\n> >\n> > [gitweb]\n> > extraBranchRefs = wip\n> > extraBranchRefs = sandbox other\n> >\n> > be parsed as a three-item list, qw(wip sandbox other)?\n> \n> This would require changes in git_get_project_config(), which would\n> need to be able to deal with multi-valued result (it caches these\n> results, so we pay only one cost of `git config` call).\n\nHm, actually not at all. Now, if I have a setup like Junio wrote the\ngit_get_project_config just returns an array ref. So modifying the\nfeature_extra_branch_refs to handle the returned value as either simple\nscalar or array reference should be enough.\n\n> \n> > * I think the $key parameter to git_get_project_config() eventually\n> > is used to look up a key in the Git-style configuration file, and\n> > the 'words_with_underscore' goes against our convention (cf. see\n> > how 'show-sizes' feature is spelled as 'showsizes' there).\n> \n> Errr... actually git_get_project_config() strips '_' from $key, though\n> not for some strange reason '-'.\n> \n> BTW. though it is 'showsizes' in code, it usually is 'showSizes' in\n> config file (camelCase convention, lowercased by git-config).\n\nOi, that was an omission from my side - at first I had that git config\nsetting with underscores. I removed them when I noticed that underscores\nare not used there. Apparently I missed that one.\n\n> \n> >> + if ($values) {\n> >> + @branch_refs = split /\\s+/, $values;\n> >> + }\n> >> +\n> >> + return @branch_refs;\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":"231505","messageId":"1386162404.2173.15.camel@localhost.localdomain","threadId":"35464","inReplyTo":"CANQwDwd0_bgKfjsRFjohmzBAnN7vDm-pYYsfe0Q71Za2K7Hw4w@mail.gmail.com","subject":"Re: [PATCH 1/3] gitweb: Move check-ref-format code into separate function","fromName":"Krzesimir Nowak","fromEmail":"krzesimir@endocode.com","sentAt":"2013-12-04T13:06:44Z","receivedAt":"2013-12-04T13:06:44Z","isPatch":true,"sender":{"key":"krzesimir@endocode.com","avatar":"https://avatars.githubusercontent.com/u/96081?v=4"},"body":"On Tue, 2013-12-03 at 20:38 +0100, Jakub Narębski wrote:\n> On Tue, Dec 3, 2013 at 8:02 PM, Junio C Hamano wrote:\n> > Krzesimir Nowak writes:\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> >> @@ -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> >\n> > So far, so good.\n> >\n> >> + # check git-check-ref-format restrictions\n> >> + $input = check_ref_format($input)\n> >> + or return undef;\n> >> return $input;\n> >\n> > Hmmm. Why do you need \"or return under\" here? It would\n> > not hurt too much per-se (strictly speaking, if the $input were a\n> > string \"0\", this will return undef instead of \"0\", which should be\n> > an OK name as far as the regexp is concerned), but it seems to be\n> > making the logic unnecessarily complex for no real gain.\n> \n> I think this simply follows \"$input = validate_sth($input) or return undef;\"\n> pattern used in this area of gitweb code (perhaps mis-used).\n> \n\nReading the validate_* subs and check_ref_format sub makes me think that\nthey should just return either 0 when validation fails or 1 when it\nsucceeds.\n\nIf I add an extra branch ref named \"0\", then check_ref_format just\nreturns \"0\" as it matches the git-check-ref-format style regex. But \"0\"\nis coerced to 0 and validate_and_filter_extra_branches will error out. I\nactually reproduced that.\n\n> Stricly speaking pure refactoring (no functional change, e.g. no assign\n> to $input) would be \"check_ref_format($input) or return undef;\", or even\n> \"return check_ref_format($input);\" if we keep check_ref_format() passthru\n> on valid refname.\n> \n\nI fixed that too.\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":"231513","messageId":"1386164697.2173.16.camel@localhost.localdomain","threadId":"35464","inReplyTo":"1386082603-8404-1-git-send-email-krzesimir@endocode.com","subject":"Re: [PATCH 0/3] Show extra branch refs in gitweb","fromName":"Krzesimir Nowak","fromEmail":"krzesimir@endocode.com","sentAt":"2013-12-04T13:44:57Z","receivedAt":"2013-12-04T13:44:57Z","isPatch":true,"sender":{"key":"krzesimir@endocode.com","avatar":"https://avatars.githubusercontent.com/u/96081?v=4"},"body":"On Tue, 2013-12-03 at 15:56 +0100, Krzesimir Nowak wrote:\n> First patch just splits some code to a function, second patch adds the\n> extra-branch-refs feature and third one adds some visual\n> differentation of branches from non-standard ref directories.\n> \n> Krzesimir Nowak (3):\n> gitweb: Move check-ref-format code into separate function\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 | 120 +++++++++++++++++++++++++++++++++++-------\n> 2 files changed, 129 insertions(+), 18 deletions(-)\n> \n\nNew version of patches are in \"Show extra branch refs in gitweb v6\"\nthread.\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":"231526","messageId":"xmqqli00lcs4.fsf@gitster.dls.corp.google.com","threadId":"35464","inReplyTo":"1386161398.2173.9.camel@localhost.localdomain","subject":"Re: [PATCH 2/3] gitweb: Add a feature for adding more branch refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-04T17:57:31Z","receivedAt":"2013-12-04T17:57:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Krzesimir Nowak writes:\n\n> On Tue, 2013-12-03 at 21:38 +0100, Jakub Narębski wrote:\n>> On Tue, Dec 3, 2013 at 9:15 PM, Junio C Hamano wrote:\n>> > Krzesimir Nowak writes:\n>> >\n>> >> @@ -626,6 +640,17 @@ sub feature_avatar {\n>> >> return @val ? @val : @_;\n>> >> }\n>> >>\n>> >> +sub feature_extra_branch_refs {\n>> >> + my (@branch_refs) = @_;\n>> >> + my $values = git_get_project_config('extra_branch_refs');\n>> >\n>> > Hmph. Three points.\n>> >\n>> > * Almost all callers of this function use\n>> >\n>> > my ($val) = git_get_project_config(...);\n>> > my @val = git_get_project_config(...);\n>> >\n>> > to expect that the function returns a list of things (and grab the\n>> > first one among them, not the length of the list). Shouldn't this\n>> > part do the same?\n>> \n>> Right. feature_snapshot() has here\n>> \n>> my (@fmts) = @_;\n>> my ($val) = git_get_project_config('snapshot');\n>> \n>> ...though git_get_project_config returns scalar.\n>\n> So what's the point of it? 'my @val = git_get_project_config ()' just\n> creates an array with one element.\n\nThe point is that \"my ($val) = git_get_project_config('name')\" calls\nthe sub in the list context like everybody else, which would be more\nrobust, if you want to be prepared for somebody else's change to the\nimplementation in the future, I think.\n\n>> > * Wouldn't this be a good candidate for a multi-valued configuration\n>> > variable, e.g. shouldn't this\n>> >\n>> > [gitweb]\n>> > extraBranchRefs = wip\n>> > extraBranchRefs = sandbox other\n>> >\n>> > be parsed as a three-item list, qw(wip sandbox other)?\n>> \n>> This would require changes in git_get_project_config(), which would\n>> need to be able to deal with multi-valued result (it caches these\n>> results, so we pay only one cost of `git config` call).\n>\n> Hm, actually not at all. Now, if I have a setup like Junio wrote the\n> git_get_project_config just returns an array ref. So modifying the\n> feature_extra_branch_refs to handle the returned value as either simple\n> scalar or array reference should be enough.\n\nYes, changing the calling site to use of config_to_multi() around\n(see the handling of 'ctag' for an example) and then concatenate the\nresult of splitting each returned element would be one way to do\nthis.\n\nJakub may have had in mind to teach git_get_project_config() to\nreturn a list; because existing callers call the sub in the list\ncontext, they will not get surprising result---even though they may\nonly use the first one and discard the rest.\n\nWhich might not be a bad thing in the longer term, but I think it is\noutside the scope of this particular topic, but in order to prepare\nfor that kind of internal API enhancement, it would still help to\nmake sure that this new caller calls the sub in the list context\nlike others.\n"}]}