# [PATCH 1/3] gitweb: Move check-ref-format code into separate function 14 messages from 2013-12-03 to 2013-12-04. Participants: Krzesimir Nowak, Junio C Hamano, Jakub Narębski. Thread: https://gitlist.dev/t/35464 ## Krzesimir Nowak, 2013-12-03 14:56 Subject: [PATCH 0/3] Show extra branch refs in gitweb Message-ID: <1386082603-8404-1-git-send-email-krzesimir@endocode.com> URL: https://gitlist.dev/e/1386082603-8404-1-git-send-email-krzesimir%40endocode.com ``` First patch just splits some code to a function, second patch adds the extra-branch-refs feature and third one adds some visual differentation of branches from non-standard ref directories. Krzesimir Nowak (3): gitweb: Move check-ref-format code into separate function gitweb: Add a feature for adding more branch refs gitweb: Denote non-heads, non-remotes branches. Documentation/gitweb.conf.txt | 27 ++++++++++ gitweb/gitweb.perl | 120 +++++++++++++++++++++++++++++++++++------- 2 files changed, 129 insertions(+), 18 deletions(-) -- 1.8.3.1 ``` ## Krzesimir Nowak, 2013-12-03 14:56 Subject: [PATCH 1/3] gitweb: Move check-ref-format code into separate function Message-ID: <1386082603-8404-2-git-send-email-krzesimir@endocode.com> URL: https://gitlist.dev/e/1386082603-8404-2-git-send-email-krzesimir%40endocode.com In-Reply-To: <1386082603-8404-1-git-send-email-krzesimir@endocode.com> ``` This check will be used in more than one place later. Signed-off-by: Krzesimir Nowak Reviewed-by: Junio C Hamano Reviewed-by: Jakub Narębski Reviewed-by: Eric Sunshine --- gitweb/gitweb.perl | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl index 68c77f6..f7730d7 100755 --- a/gitweb/gitweb.perl +++ b/gitweb/gitweb.perl @@ -1452,6 +1452,16 @@ sub validate_pathname { return $input; } +sub check_ref_format { + my $input = shift || return undef; + + # restrictions on ref name according to git-check-ref-format + if ($input =~ m!(/\.|\.\.|[\000-\040\177 ~^:?*\[]|/$)!) { + return undef; + } + return $input; +} + sub validate_refname { my $input = shift || return undef; @@ -1462,10 +1472,9 @@ sub validate_refname { # it must be correct pathname $input = validate_pathname($input) or return undef; - # restrictions on ref name according to git-check-ref-format - if ($input =~ m!(/\.|\.\.|[\000-\040\177 ~^:?*\[]|/$)!) { - return undef; - } + # check git-check-ref-format restrictions + $input = check_ref_format($input) + or return undef; return $input; } -- 1.8.3.1 ``` ## Krzesimir Nowak, 2013-12-03 14:56 Subject: [PATCH 2/3] gitweb: Add a feature for adding more branch refs Message-ID: <1386082603-8404-3-git-send-email-krzesimir@endocode.com> URL: https://gitlist.dev/e/1386082603-8404-3-git-send-email-krzesimir%40endocode.com In-Reply-To: <1386082603-8404-1-git-send-email-krzesimir@endocode.com> ``` Allow extra-branch-refs feature to tell gitweb to show refs from additional hierarchies in addition to branches in the list-of-branches view. Signed-off-by: Krzesimir Nowak Reviewed-by: Junio C Hamano Reviewed-by: Jakub Narębski Reviewed-by: Eric Sunshine --- Documentation/gitweb.conf.txt | 27 +++++++++++++++ gitweb/gitweb.perl | 76 ++++++++++++++++++++++++++++++++++++------- 2 files changed, 91 insertions(+), 12 deletions(-) diff --git a/Documentation/gitweb.conf.txt b/Documentation/gitweb.conf.txt index e2113d9..3de8e14 100644 --- a/Documentation/gitweb.conf.txt +++ b/Documentation/gitweb.conf.txt @@ -849,6 +849,33 @@ time zones in the form of "+/-HHMM", such as "+0200". + Project specific override is not supported. +extra-branch-refs:: + List of additional directories under "refs" which are going to + be used as branch refs. For example if you have a gerrit setup + where all branches under refs/heads/ are official, + push-after-review ones and branches under refs/sandbox/, + refs/wip and refs/other are user ones where permissions are + much wider, then you might want to set this variable as + follows: ++ +-------------------------------------------------------------------------------- +$feature{'extra-branch-refs'}{'default'} = + ['sandbox', 'wip', 'other']; +-------------------------------------------------------------------------------- ++ +If overriding was enabled then this feature can be configured on a +per-repository basis via repository's `gitweb.extrabranchrefs` +configuration variable, which contains a space separated list of +refs. An example: ++ +-------------------------------------------------------------------------------- +[gitweb] + extrabranchrefs = sandbox wip other +-------------------------------------------------------------------------------- ++ +It is an error to specify a ref that does not pass "git check-ref-format" +scrutiny. Duplicated values are filtered. + EXAMPLES -------- diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl index f7730d7..6326075 100755 --- a/gitweb/gitweb.perl +++ b/gitweb/gitweb.perl @@ -548,6 +548,20 @@ our %feature = ( 'sub' => sub { feature_bool('remote_heads', @_) }, 'override' => 0, 'default' => [0]}, + + # Enable showing branches under other refs in addition to heads + + # To set system wide extra branch refs have in $GITWEB_CONFIG + # $feature{'extra-branch-refs'}{'default'} = ['dirs', 'of', 'choice']; + # To have project specific config enable override in $GITWEB_CONFIG + # $feature{'extra-branch-refs'}{'override'} = 1; + # and in project config gitweb.extrabranchrefs = dirs of choice + # Every directory is separated with whitespace. + + 'extra-branch-refs' => { + 'sub' => \&feature_extra_branch_refs, + 'override' => 0, + 'default' => []}, ); sub gitweb_get_feature { @@ -626,6 +640,17 @@ sub feature_avatar { return @val ? @val : @_; } +sub feature_extra_branch_refs { + my (@branch_refs) = @_; + my $values = git_get_project_config('extra_branch_refs'); + + if ($values) { + @branch_refs = split /\s+/, $values; + } + + return @branch_refs; +} + # checking HEAD file with -e is fragile if the repository was # initialized long time ago (i.e. symlink HEAD) and was pack-ref'ed # and then pruned. @@ -656,6 +681,18 @@ sub filter_snapshot_fmts { !$known_snapshot_formats{$_}{'disabled'}} @fmts; } +sub filter_and_validate_refs { + my @refs = @_; + my %unique_refs = (); + + foreach my $ref (@refs) { + die_error(500, "Invalid ref '$ref' in 'extra-branch-refs' feature") unless (check_ref_format($ref)); + # 'heads' are added implicitly in get_branch_refs(). + $unique_refs{$ref} = 1 if ($ref ne 'heads'); + } + return sort keys %unique_refs; +} + # If it is set to code reference, it is code that it is to be run once per # request, allowing updating configurations that change with each request, # while running other code in config file only once. @@ -1113,7 +1150,7 @@ sub evaluate_git_dir { our $git_dir = "$projectroot/$project" if $project; } -our (@snapshot_fmts, $git_avatar); +our (@snapshot_fmts, $git_avatar, @extra_branch_refs); sub configure_gitweb_features { # list of supported snapshot formats our @snapshot_fmts = gitweb_get_feature('snapshot'); @@ -1131,6 +1168,13 @@ sub configure_gitweb_features { } else { $git_avatar = ''; } + + our @extra_branch_refs = gitweb_get_feature('extra-branch-refs'); + @extra_branch_refs = filter_and_validate_refs (@extra_branch_refs); +} + +sub get_branch_refs { + return ('heads', @extra_branch_refs); } # custom error handler: 'die ' is Internal Server Error @@ -2524,6 +2568,7 @@ sub format_snapshot_links { sub get_feed_info { my $format = shift || 'Atom'; my %res = (action => lc($format)); + my $matched_ref = 0; # feed links are possible only for project views return unless (defined $project); @@ -2531,12 +2576,17 @@ sub get_feed_info { # or don't have specific feed yet (so they should use generic) return if (!$action || $action =~ /^(?:tags|heads|forks|tag|search)$/x); - my $branch; - # branches refs uses 'refs/heads/' prefix (fullname) to differentiate - # from tag links; this also makes possible to detect branch links - if ((defined $hash_base && $hash_base =~ m!^refs/heads/(.*)$!) || - (defined $hash && $hash =~ m!^refs/heads/(.*)$!)) { - $branch = $1; + my $branch = undef; + # branches refs uses 'refs/' + $get_branch_refs()[x] + '/' prefix + # (fullname) to differentiate from tag links; this also makes + # possible to detect branch links + for my $ref (get_branch_refs()) { + if ((defined $hash_base && $hash_base =~ m!^refs/\Q$ref\E/(.*)$!) || + (defined $hash && $hash =~ m!^refs/\Q$ref\E/(.*)$!)) { + $branch = $1; + $matched_ref = $ref; + last; + } } # find log type for feed description (title) my $type = 'log'; @@ -2549,7 +2599,7 @@ sub get_feed_info { } $res{-title} = $type; - $res{'hash'} = (defined $branch ? "refs/heads/$branch" : undef); + $res{'hash'} = (defined $branch ? "refs/$matched_ref/$branch" : undef); $res{'file_name'} = $file_name; return %res; @@ -3202,7 +3252,7 @@ sub git_get_last_activity { '--format=%(committer)', '--sort=-committerdate', '--count=1', - 'refs/heads') or return; + map { "refs/$_" } get_branch_refs ()) or return; my $most_recent = <$fd>; close $fd or return; if (defined $most_recent && @@ -3653,7 +3703,7 @@ sub parse_from_to_diffinfo { sub git_get_heads_list { my ($limit, @classes) = @_; - @classes = ('heads') unless @classes; + @classes = get_branch_refs() unless @classes; my @patterns = map { "refs/$_" } @classes; my @headslist; @@ -3671,7 +3721,8 @@ sub git_get_heads_list { my ($committer, $epoch, $tz) = ($committerinfo =~ /^(.*) ([0-9]+) (.*)$/); $ref_item{'fullname'} = $name; - $name =~ s!^refs/(?:head|remote)s/!!; + my $strip_refs = join '|', map { quotemeta } get_branch_refs(); + $name =~ s!^refs/($strip_refs|remotes)/!!; $ref_item{'name'} = $name; $ref_item{'id'} = $hash; @@ -7188,7 +7239,8 @@ sub snapshot_name { $ver = $1; } else { # branches and other need shortened SHA-1 hash - if ($hash =~ m!^refs/(?:heads|remotes)/(.*)$!) { + my $strip_refs = join '|', map { quotemeta } get_branch_refs(); + if ($hash =~ m!^refs/($strip_refs|remotes)/(.*)$!) { $ver = $1; } $ver .= '-' . git_get_short_hash($project, $hash); -- 1.8.3.1 ``` ## Krzesimir Nowak, 2013-12-03 14:56 Subject: [PATCH 3/3] gitweb: Denote non-heads, non-remotes branches. Message-ID: <1386082603-8404-4-git-send-email-krzesimir@endocode.com> URL: https://gitlist.dev/e/1386082603-8404-4-git-send-email-krzesimir%40endocode.com In-Reply-To: <1386082603-8404-1-git-send-email-krzesimir@endocode.com> ``` Given two branches residing in refs/heads/master and refs/wip/feature the list-of-branches view will present them in following way: master feature (wip) When getting a snapshot of a 'feature' branch, the tarball is going to have name like 'project-wip-feature-.tgz'. Signed-off-by: Krzesimir Nowak Reviewed-by: Junio C Hamano Reviewed-by: Jakub Narębski Reviewed-by: Eric Sunshine --- gitweb/gitweb.perl | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl index 6326075..eb8d962 100755 --- a/gitweb/gitweb.perl +++ b/gitweb/gitweb.perl @@ -3723,8 +3723,14 @@ sub git_get_heads_list { $ref_item{'fullname'} = $name; my $strip_refs = join '|', map { quotemeta } get_branch_refs(); $name =~ s!^refs/($strip_refs|remotes)/!!; + $ref_item{'name'} = $name; + # for refs neither in 'heads' nor 'remotes' we want to + # show their different ref dir + my $ref_dir = (defined $1) ? $1 : ''; + if ($ref_dir ne '' and $ref_dir ne 'heads' and $ref_dir ne 'remotes') { + $ref_item{'name'} .= ' (' . $ref_dir . ')'; + } - $ref_item{'name'} = $name; $ref_item{'id'} = $hash; $ref_item{'title'} = $title || '(no commit message)'; $ref_item{'epoch'} = $epoch; @@ -7241,7 +7247,24 @@ sub snapshot_name { # branches and other need shortened SHA-1 hash my $strip_refs = join '|', map { quotemeta } get_branch_refs(); if ($hash =~ m!^refs/($strip_refs|remotes)/(.*)$!) { - $ver = $1; + my $ref_dir = $1; + $ver = $2; + + if (defined $ref_dir) { + # this is going to be a part of + # filename, so lets stick to + # alphanumerics, dashes and underlines + # only - some filesystems do not like + # some punctuation symbols for + # example. + $ref_dir =~ s/[^[:alnum:]_-]//g; + } + + # for refs not in heads nor remotes we want to + # add a ref dir to archive name + if ($ref_dir ne '' and $ref_dir ne 'heads' and $ref_dir ne 'remotes') { + $ver = $ref_dir . '-' . $ver; + } } $ver .= '-' . git_get_short_hash($project, $hash); } -- 1.8.3.1 ``` ## Junio C Hamano, 2013-12-03 18:51 Subject: Re: [PATCH 2/3] gitweb: Add a feature for adding more branch refs Message-ID: URL: https://gitlist.dev/e/xmqqli01ojiz.fsf%40gitster.dls.corp.google.com In-Reply-To: <1386082603-8404-3-git-send-email-krzesimir@endocode.com> ``` Krzesimir Nowak writes: > Allow extra-branch-refs feature to tell gitweb to show refs from > additional hierarchies in addition to branches in the list-of-branches > view. > > Signed-off-by: Krzesimir Nowak > Reviewed-by: Junio C Hamano Please do not add "Reviewed-by:" like this; I've never reviewed this version of the patch. These are to be added only when you re-send, for final application, the version as exactly reviewed, or adjusted a previous version you got reviewed in a way that match suggestions given by reviewers. > Reviewed-by: Jakub Narębski > Reviewed-by: Eric Sunshine ``` ## Junio C Hamano, 2013-12-03 19:02 Subject: Re: [PATCH 1/3] gitweb: Move check-ref-format code into separate function Message-ID: URL: https://gitlist.dev/e/xmqqd2ldoj0s.fsf%40gitster.dls.corp.google.com In-Reply-To: <1386082603-8404-2-git-send-email-krzesimir@endocode.com> ``` Krzesimir Nowak writes: > This check will be used in more than one place later. > > Signed-off-by: Krzesimir Nowak > Reviewed-by: Junio C Hamano > Reviewed-by: Jakub Narębski > Reviewed-by: Eric Sunshine > --- > gitweb/gitweb.perl | 17 +++++++++++++---- > 1 file changed, 13 insertions(+), 4 deletions(-) > > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl > index 68c77f6..f7730d7 100755 > --- a/gitweb/gitweb.perl > +++ b/gitweb/gitweb.perl > @@ -1452,6 +1452,16 @@ sub validate_pathname { > return $input; > } > > +sub check_ref_format { > + my $input = shift || return undef; > + > + # restrictions on ref name according to git-check-ref-format > + if ($input =~ m!(/\.|\.\.|[\000-\040\177 ~^:?*\[]|/$)!) { > + return undef; > + } > + return $input; > +} > + > sub validate_refname { > my $input = shift || return undef; > > @@ -1462,10 +1472,9 @@ sub validate_refname { > # it must be correct pathname > $input = validate_pathname($input) > or return undef; > - # restrictions on ref name according to git-check-ref-format > - if ($input =~ m!(/\.|\.\.|[\000-\040\177 ~^:?*\[]|/$)!) { > - return undef; > - } So far, so good. > + # check git-check-ref-format restrictions > + $input = check_ref_format($input) > + or return undef; > return $input; Hmmm. Why do you need "or return under" here? It would not hurt too much per-se (strictly speaking, if the $input were a string "0", this will return undef instead of "0", which should be an OK name as far as the regexp is concerned), but it seems to be making the logic unnecessarily complex for no real gain. > } ``` ## Jakub Narębski, 2013-12-03 19:38 Subject: Re: [PATCH 1/3] gitweb: Move check-ref-format code into separate function Message-ID: URL: https://gitlist.dev/e/CANQwDwd0_bgKfjsRFjohmzBAnN7vDm-pYYsfe0Q71Za2K7Hw4w%40mail.gmail.com In-Reply-To: ``` On Tue, Dec 3, 2013 at 8:02 PM, Junio C Hamano wrote: > Krzesimir Nowak writes: >> +sub check_ref_format { >> + my $input = shift || return undef; >> + >> + # restrictions on ref name according to git-check-ref-format >> + if ($input =~ m!(/\.|\.\.|[\000-\040\177 ~^:?*\[]|/$)!) { >> + return undef; >> + } >> + return $input; >> +} [...] >> @@ -1462,10 +1472,9 @@ sub validate_refname { >> # it must be correct pathname >> $input = validate_pathname($input) >> or return undef; >> - # restrictions on ref name according to git-check-ref-format >> - if ($input =~ m!(/\.|\.\.|[\000-\040\177 ~^:?*\[]|/$)!) { >> - return undef; >> - } > > So far, so good. > >> + # check git-check-ref-format restrictions >> + $input = check_ref_format($input) >> + or return undef; >> return $input; > > Hmmm. Why do you need "or return under" here? It would > not hurt too much per-se (strictly speaking, if the $input were a > string "0", this will return undef instead of "0", which should be > an OK name as far as the regexp is concerned), but it seems to be > making the logic unnecessarily complex for no real gain. I think this simply follows "$input = validate_sth($input) or return undef;" pattern used in this area of gitweb code (perhaps mis-used). Stricly speaking pure refactoring (no functional change, e.g. no assign to $input) would be "check_ref_format($input) or return undef;", or even "return check_ref_format($input);" if we keep check_ref_format() passthru on valid refname. -- Jakub Narebski ``` ## Junio C Hamano, 2013-12-03 19:48 Subject: Re: [PATCH 1/3] gitweb: Move check-ref-format code into separate function Message-ID: URL: https://gitlist.dev/e/xmqqzjohn2bq.fsf%40gitster.dls.corp.google.com In-Reply-To: ``` Jakub Narębski writes: > Stricly speaking pure refactoring (no functional change, e.g. no assign > to $input) would be "check_ref_format($input) or return undef;", or even > "return check_ref_format($input);" if we keep check_ref_format() passthru > on valid refname. Exactly. ``` ## Junio C Hamano, 2013-12-03 20:15 Subject: Re: [PATCH 2/3] gitweb: Add a feature for adding more branch refs Message-ID: URL: https://gitlist.dev/e/xmqqr49tn11c.fsf%40gitster.dls.corp.google.com In-Reply-To: <1386082603-8404-3-git-send-email-krzesimir@endocode.com> ``` Krzesimir Nowak writes: > @@ -626,6 +640,17 @@ sub feature_avatar { > return @val ? @val : @_; > } > > +sub feature_extra_branch_refs { > + my (@branch_refs) = @_; > + my $values = git_get_project_config('extra_branch_refs'); Hmph. Three points. * Almost all callers of this function use my ($val) = git_get_project_config(...); my @val = git_get_project_config(...); to expect that the function returns a list of things (and grab the first one among them, not the length of the list). Shoudln't this part do the same? * Wouldn't this be a good candidate for a multi-valued configuration variable, e.g. shouldn't this [gitweb] extraBranchRefs = wip extraBranchRefs = sandbox other be parsed as a three-item list, qw(wip sandbox other)? * I think the $key parameter to git_get_project_config() eventually is used to look up a key in the Git-style configuration file, and the 'words_with_underscore' goes against our convention (cf. see how 'show-sizes' feature is spelled as 'showsizes' there). > + if ($values) { > + @branch_refs = split /\s+/, $values; > + } > + > + return @branch_refs; > +} ``` ## Jakub Narębski, 2013-12-03 20:38 Subject: Re: [PATCH 2/3] gitweb: Add a feature for adding more branch refs Message-ID: URL: https://gitlist.dev/e/CANQwDwcpkbbfND9MmB9wGcL7mrhV_Mxb1amvGPgWeR344_fcfw%40mail.gmail.com In-Reply-To: ``` On Tue, Dec 3, 2013 at 9:15 PM, Junio C Hamano wrote: > Krzesimir Nowak writes: > >> @@ -626,6 +640,17 @@ sub feature_avatar { >> return @val ? @val : @_; >> } >> >> +sub feature_extra_branch_refs { >> + my (@branch_refs) = @_; >> + my $values = git_get_project_config('extra_branch_refs'); > > Hmph. Three points. > > * Almost all callers of this function use > > my ($val) = git_get_project_config(...); > my @val = git_get_project_config(...); > > to expect that the function returns a list of things (and grab the > first one among them, not the length of the list). Shouldn't this > part do the same? Right. feature_snapshot() has here my (@fmts) = @_; my ($val) = git_get_project_config('snapshot'); ...though git_get_project_config returns scalar. > * Wouldn't this be a good candidate for a multi-valued configuration > variable, e.g. shouldn't this > > [gitweb] > extraBranchRefs = wip > extraBranchRefs = sandbox other > > be parsed as a three-item list, qw(wip sandbox other)? This would require changes in git_get_project_config(), which would need to be able to deal with multi-valued result (it caches these results, so we pay only one cost of `git config` call). > * I think the $key parameter to git_get_project_config() eventually > is used to look up a key in the Git-style configuration file, and > the 'words_with_underscore' goes against our convention (cf. see > how 'show-sizes' feature is spelled as 'showsizes' there). Errr... actually git_get_project_config() strips '_' from $key, though not for some strange reason '-'. BTW. though it is 'showsizes' in code, it usually is 'showSizes' in config file (camelCase convention, lowercased by git-config). >> + if ($values) { >> + @branch_refs = split /\s+/, $values; >> + } >> + >> + return @branch_refs; >> +} -- Jakub Narebski ``` ## Krzesimir Nowak, 2013-12-04 12:49 Subject: Re: [PATCH 2/3] gitweb: Add a feature for adding more branch refs Message-ID: <1386161398.2173.9.camel@localhost.localdomain> URL: https://gitlist.dev/e/1386161398.2173.9.camel%40localhost.localdomain In-Reply-To: ``` On Tue, 2013-12-03 at 21:38 +0100, Jakub Narębski wrote: > On Tue, Dec 3, 2013 at 9:15 PM, Junio C Hamano wrote: > > Krzesimir Nowak writes: > > > >> @@ -626,6 +640,17 @@ sub feature_avatar { > >> return @val ? @val : @_; > >> } > >> > >> +sub feature_extra_branch_refs { > >> + my (@branch_refs) = @_; > >> + my $values = git_get_project_config('extra_branch_refs'); > > > > Hmph. Three points. > > > > * Almost all callers of this function use > > > > my ($val) = git_get_project_config(...); > > my @val = git_get_project_config(...); > > > > to expect that the function returns a list of things (and grab the > > first one among them, not the length of the list). Shouldn't this > > part do the same? > > Right. feature_snapshot() has here > > my (@fmts) = @_; > my ($val) = git_get_project_config('snapshot'); > > ...though git_get_project_config returns scalar. So what's the point of it? 'my @val = git_get_project_config ()' just creates an array with one element. > > > * Wouldn't this be a good candidate for a multi-valued configuration > > variable, e.g. shouldn't this > > > > [gitweb] > > extraBranchRefs = wip > > extraBranchRefs = sandbox other > > > > be parsed as a three-item list, qw(wip sandbox other)? > > This would require changes in git_get_project_config(), which would > need to be able to deal with multi-valued result (it caches these > results, so we pay only one cost of `git config` call). Hm, actually not at all. Now, if I have a setup like Junio wrote the git_get_project_config just returns an array ref. So modifying the feature_extra_branch_refs to handle the returned value as either simple scalar or array reference should be enough. > > > * I think the $key parameter to git_get_project_config() eventually > > is used to look up a key in the Git-style configuration file, and > > the 'words_with_underscore' goes against our convention (cf. see > > how 'show-sizes' feature is spelled as 'showsizes' there). > > Errr... actually git_get_project_config() strips '_' from $key, though > not for some strange reason '-'. > > BTW. though it is 'showsizes' in code, it usually is 'showSizes' in > config file (camelCase convention, lowercased by git-config). Oi, that was an omission from my side - at first I had that git config setting with underscores. I removed them when I noticed that underscores are not used there. Apparently I missed that one. > > >> + if ($values) { > >> + @branch_refs = split /\s+/, $values; > >> + } > >> + > >> + return @branch_refs; > >> +} > > > -- Krzesimir Nowak Software Developer Endocode AG krzesimir@endocode.com ------ Endocode AG, Johannisstraße 20, 10117 Berlin info@endocode.com | www.endocode.com Vorstandsvorsitzender: Mirko Boehm Vorstände: Dr. Karl Beecher, Chris Kühl, Sebastian Sucker Aufsichtsratsvorsitzende: Jennifer Beecher Registergericht: Amtsgericht Charlottenburg - HRB 150748 B ``` ## Krzesimir Nowak, 2013-12-04 13:06 Subject: Re: [PATCH 1/3] gitweb: Move check-ref-format code into separate function Message-ID: <1386162404.2173.15.camel@localhost.localdomain> URL: https://gitlist.dev/e/1386162404.2173.15.camel%40localhost.localdomain In-Reply-To: ``` On Tue, 2013-12-03 at 20:38 +0100, Jakub Narębski wrote: > On Tue, Dec 3, 2013 at 8:02 PM, Junio C Hamano wrote: > > Krzesimir Nowak writes: > > >> +sub check_ref_format { > >> + my $input = shift || return undef; > >> + > >> + # restrictions on ref name according to git-check-ref-format > >> + if ($input =~ m!(/\.|\.\.|[\000-\040\177 ~^:?*\[]|/$)!) { > >> + return undef; > >> + } > >> + return $input; > >> +} > [...] > >> @@ -1462,10 +1472,9 @@ sub validate_refname { > >> # it must be correct pathname > >> $input = validate_pathname($input) > >> or return undef; > >> - # restrictions on ref name according to git-check-ref-format > >> - if ($input =~ m!(/\.|\.\.|[\000-\040\177 ~^:?*\[]|/$)!) { > >> - return undef; > >> - } > > > > So far, so good. > > > >> + # check git-check-ref-format restrictions > >> + $input = check_ref_format($input) > >> + or return undef; > >> return $input; > > > > Hmmm. Why do you need "or return under" here? It would > > not hurt too much per-se (strictly speaking, if the $input were a > > string "0", this will return undef instead of "0", which should be > > an OK name as far as the regexp is concerned), but it seems to be > > making the logic unnecessarily complex for no real gain. > > I think this simply follows "$input = validate_sth($input) or return undef;" > pattern used in this area of gitweb code (perhaps mis-used). > Reading the validate_* subs and check_ref_format sub makes me think that they should just return either 0 when validation fails or 1 when it succeeds. If I add an extra branch ref named "0", then check_ref_format just returns "0" as it matches the git-check-ref-format style regex. But "0" is coerced to 0 and validate_and_filter_extra_branches will error out. I actually reproduced that. > Stricly speaking pure refactoring (no functional change, e.g. no assign > to $input) would be "check_ref_format($input) or return undef;", or even > "return check_ref_format($input);" if we keep check_ref_format() passthru > on valid refname. > I fixed that too. -- Krzesimir Nowak Software Developer Endocode AG krzesimir@endocode.com ------ Endocode AG, Johannisstraße 20, 10117 Berlin info@endocode.com | www.endocode.com Vorstandsvorsitzender: Mirko Boehm Vorstände: Dr. Karl Beecher, Chris Kühl, Sebastian Sucker Aufsichtsratsvorsitzende: Jennifer Beecher Registergericht: Amtsgericht Charlottenburg - HRB 150748 B ``` ## Krzesimir Nowak, 2013-12-04 13:44 Subject: Re: [PATCH 0/3] Show extra branch refs in gitweb Message-ID: <1386164697.2173.16.camel@localhost.localdomain> URL: https://gitlist.dev/e/1386164697.2173.16.camel%40localhost.localdomain In-Reply-To: <1386082603-8404-1-git-send-email-krzesimir@endocode.com> ``` On Tue, 2013-12-03 at 15:56 +0100, Krzesimir Nowak wrote: > First patch just splits some code to a function, second patch adds the > extra-branch-refs feature and third one adds some visual > differentation of branches from non-standard ref directories. > > Krzesimir Nowak (3): > gitweb: Move check-ref-format code into separate function > gitweb: Add a feature for adding more branch refs > gitweb: Denote non-heads, non-remotes branches. > > Documentation/gitweb.conf.txt | 27 ++++++++++ > gitweb/gitweb.perl | 120 +++++++++++++++++++++++++++++++++++------- > 2 files changed, 129 insertions(+), 18 deletions(-) > New version of patches are in "Show extra branch refs in gitweb v6" thread. Cheers, -- Krzesimir Nowak Software Developer Endocode AG krzesimir@endocode.com ------ Endocode AG, Johannisstraße 20, 10117 Berlin info@endocode.com | www.endocode.com Vorstandsvorsitzender: Mirko Boehm Vorstände: Dr. Karl Beecher, Chris Kühl, Sebastian Sucker Aufsichtsratsvorsitzende: Jennifer Beecher Registergericht: Amtsgericht Charlottenburg - HRB 150748 B ``` ## Junio C Hamano, 2013-12-04 17:57 Subject: Re: [PATCH 2/3] gitweb: Add a feature for adding more branch refs Message-ID: URL: https://gitlist.dev/e/xmqqli00lcs4.fsf%40gitster.dls.corp.google.com In-Reply-To: <1386161398.2173.9.camel@localhost.localdomain> ``` Krzesimir Nowak writes: > On Tue, 2013-12-03 at 21:38 +0100, Jakub Narębski wrote: >> On Tue, Dec 3, 2013 at 9:15 PM, Junio C Hamano wrote: >> > Krzesimir Nowak writes: >> > >> >> @@ -626,6 +640,17 @@ sub feature_avatar { >> >> return @val ? @val : @_; >> >> } >> >> >> >> +sub feature_extra_branch_refs { >> >> + my (@branch_refs) = @_; >> >> + my $values = git_get_project_config('extra_branch_refs'); >> > >> > Hmph. Three points. >> > >> > * Almost all callers of this function use >> > >> > my ($val) = git_get_project_config(...); >> > my @val = git_get_project_config(...); >> > >> > to expect that the function returns a list of things (and grab the >> > first one among them, not the length of the list). Shouldn't this >> > part do the same? >> >> Right. feature_snapshot() has here >> >> my (@fmts) = @_; >> my ($val) = git_get_project_config('snapshot'); >> >> ...though git_get_project_config returns scalar. > > So what's the point of it? 'my @val = git_get_project_config ()' just > creates an array with one element. The point is that "my ($val) = git_get_project_config('name')" calls the sub in the list context like everybody else, which would be more robust, if you want to be prepared for somebody else's change to the implementation in the future, I think. >> > * Wouldn't this be a good candidate for a multi-valued configuration >> > variable, e.g. shouldn't this >> > >> > [gitweb] >> > extraBranchRefs = wip >> > extraBranchRefs = sandbox other >> > >> > be parsed as a three-item list, qw(wip sandbox other)? >> >> This would require changes in git_get_project_config(), which would >> need to be able to deal with multi-valued result (it caches these >> results, so we pay only one cost of `git config` call). > > Hm, actually not at all. Now, if I have a setup like Junio wrote the > git_get_project_config just returns an array ref. So modifying the > feature_extra_branch_refs to handle the returned value as either simple > scalar or array reference should be enough. Yes, changing the calling site to use of config_to_multi() around (see the handling of 'ctag' for an example) and then concatenate the result of splitting each returned element would be one way to do this. Jakub may have had in mind to teach git_get_project_config() to return a list; because existing callers call the sub in the list context, they will not get surprising result---even though they may only use the first one and discard the rest. Which might not be a bad thing in the longer term, but I think it is outside the scope of this particular topic, but in order to prepare for that kind of internal API enhancement, it would still help to make sure that this new caller calls the sub in the list context like others. ```