[PATCH 0/5] Show extra branch refs in gitweb v6

[PATCH 0/5] Show extra branch refs in gitweb v6

[Krzesimir Nowak]

First patch just adds a comment I though would be useful when trying
to understand how config parsing is done.

Second patch splits some code to a function.

Third patch fixes validation functions to return either 0 or 1,
instead of undef or passed $input.

Fourth patch adds the extra-branch-feature and some documentation.

Fifth patch adds some visual differentation of branches from
non-standard ref directories.

Krzesimir Nowak (5):
  gitweb: Add a comment explaining the meaning of $/
  gitweb: Move check-ref-format code into separate function
  gitweb: Return plain booleans in validation methods
  gitweb: Add a feature for adding more branch refs
  gitweb: Denote non-heads, non-remotes branches

 Documentation/gitweb.conf.txt |  27 +++++++
 gitweb/gitweb.perl            | 166 +++++++++++++++++++++++++++++++++---------
 2 files changed, 160 insertions(+), 33 deletions(-)

-- 
1.8.3.1

[PATCH 1/5] gitweb: Add a comment explaining the meaning of $/

[Krzesimir Nowak]

So future reader will know what does it mean without running "perldoc
perlvar".

Signed-off-by: Krzesimir Nowak <krzesimir@endocode.com>
---
 gitweb/gitweb.perl | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 68c77f6..ee61f9e 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2629,6 +2629,8 @@ sub git_parse_project_config {
 	my $section_regexp = shift;
 	my %config;
 
+	# input record separator, so getline does end on null, not
+	# newline
 	local $/ = "\0";
 
 	open my $fh, "-|", git_cmd(), "config", '-z', '-l',
-- 
1.8.3.1

[PATCH 2/5] gitweb: Move check-ref-format code into separate function

[Krzesimir Nowak]

This check will be used in more than one place later.

Signed-off-by: Krzesimir Nowak <krzesimir@endocode.com>
Reviewed-by: Junio C Hamano <gitster@pobox.com>
Reviewed-by: Jakub Narębski <jnareb@gmail.com>
---
 gitweb/gitweb.perl | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index ee61f9e..67415b9 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
+	check_ref_format($input)
+		or return undef;
 	return $input;
 }
 
-- 
1.8.3.1

[PATCH 3/5] gitweb: Return plain booleans in validation methods

[Krzesimir Nowak]

Users of validate_* passing "0" might get failures on correct name
because of coercion of "0" to false in code like:
die_error(500, "invalid ref") unless (check_ref_format ("0"));

Signed-off-by: Krzesimir Nowak <krzesimir@endocode.com>
---
 gitweb/gitweb.perl | 45 +++++++++++++++++++++++++--------------------
 1 file changed, 25 insertions(+), 20 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 67415b9..3434602 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1419,63 +1419,68 @@ sub href {
 ## validation, quoting/unquoting and escaping
 
 sub validate_action {
-	my $input = shift || return undef;
-	return undef unless exists $actions{$input};
-	return $input;
+	my $input = shift;
+
+	return 0 unless defined $input;
+	return 0 unless exists $actions{$input};
+	return 1;
 }
 
 sub validate_project {
-	my $input = shift || return undef;
+	my $input = shift;
+
+	return 0 unless defined $input;
 	if (!validate_pathname($input) ||
 		!(-d "$projectroot/$input") ||
 		!check_export_ok("$projectroot/$input") ||
 		($strict_export && !project_in_list($input))) {
-		return undef;
+		return 0;
 	} else {
-		return $input;
+		return 1;
 	}
 }
 
 sub validate_pathname {
-	my $input = shift || return undef;
+	my $input = shift;
 
+	return 0 unless defined $input;
 	# no '.' or '..' as elements of path, i.e. no '.' nor '..'
 	# at the beginning, at the end, and between slashes.
 	# also this catches doubled slashes
 	if ($input =~ m!(^|/)(|\.|\.\.)(/|$)!) {
-		return undef;
+		return 0;
 	}
 	# no null characters
 	if ($input =~ m!\0!) {
-		return undef;
+		return 0;
 	}
-	return $input;
+	return 1;
 }
 
 sub check_ref_format {
-	my $input = shift || return undef;
+	my $input = shift;
 
+	return 0 unless defined $input;
 	# restrictions on ref name according to git-check-ref-format
 	if ($input =~ m!(/\.|\.\.|[\000-\040\177 ~^:?*\[]|/$)!) {
-		return undef;
+		return 0;
 	}
-	return $input;
+	return 1;
 }
 
 sub validate_refname {
-	my $input = shift || return undef;
+	my $input = shift;
 
+	return undef unless defined $input;
 	# textual hashes are O.K.
 	if ($input =~ m/^[0-9a-fA-F]{40}$/) {
-		return $input;
+		return 1;
 	}
 	# it must be correct pathname
-	$input = validate_pathname($input)
-		or return undef;
+	validate_pathname($input) or return 0;
 	# check git-check-ref-format restrictions
-	check_ref_format($input)
-		or return undef;
-	return $input;
+	check_ref_format($input) or return 0;
+	return 1;
 }
 
 # decode sequences of octets in utf8 into Perl's internal form,
-- 
1.8.3.1

[PATCH 4/5] gitweb: Add a feature for adding more branch refs

[Krzesimir Nowak]

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 <krzesimir@endocode.com>
Reviewed-by: Junio C Hamano <gitster@pobox.com>
Reviewed-by: Jakub Narębski <jnareb@gmail.com>
---
 Documentation/gitweb.conf.txt | 37 +++++++++++++++++++
 gitweb/gitweb.perl            | 85 +++++++++++++++++++++++++++++++++++++------
 2 files changed, 110 insertions(+), 12 deletions(-)

diff --git a/Documentation/gitweb.conf.txt b/Documentation/gitweb.conf.txt
index e2113d9..5a77452 100644
--- a/Documentation/gitweb.conf.txt
+++ b/Documentation/gitweb.conf.txt
@@ -849,6 +849,43 @@ 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
+--------------------------------------------------------------------------------
++
+The gitweb.extrabranchrefs is actually a multi-valued configuration
+variable, so following example is also correct and the result is the
+same as of the snippet above:
++
+--------------------------------------------------------------------------------
+[gitweb]
+	extrabranchrefs = sandbox
+	extrabranchrefs = 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 3434602..6d3d52d 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,26 @@ sub feature_avatar {
 	return @val ? @val : @_;
 }
 
+sub feature_extra_branch_refs {
+	my (@branch_refs) = @_;
+	my $values = git_get_project_config('extrabranchrefs');
+
+	if ($values) {
+		unless (ref $values) {
+			$values = [$values];
+		}
+		unless (ref $values eq 'ARRAY') {
+			return @branch_refs;
+		}
+		@branch_refs = ();
+		foreach my $value (@{$values}) {
+			push @branch_refs, split /\s+/, $value;
+		}
+	}
+
+	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 +690,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 +1159,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 +1177,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 <message>' is Internal Server Error
@@ -2529,6 +2582,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);
@@ -2536,12 +2590,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';
@@ -2554,7 +2613,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;
@@ -3209,7 +3268,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 &&
@@ -3660,7 +3719,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;
 
@@ -3678,7 +3737,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;
@@ -7195,7 +7255,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

[PATCH 5/5] gitweb: Denote non-heads, non-remotes branches

[Krzesimir Nowak]

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-<short hash>.tgz'.

Signed-off-by: Krzesimir Nowak <krzesimir@endocode.com>
---
 gitweb/gitweb.perl | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 6d3d52d..9a63ea9 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3739,8 +3739,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;
@@ -7257,7 +7263,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

Re: [PATCH 1/5] gitweb: Add a comment explaining the meaning of $/

[Jakub Narębski]

On Wed, Dec 4, 2013 at 2:42 PM, Krzesimir Nowak <krzesimir@endocode.com> wrote:

> So future reader will know what does it mean without running "perldoc
> perlvar".

Hmmm... shouldn't future reader know it anyway?  It is not that cryptic.
I'd say it is idiomatic Perl.

Besides, it is not the only place where we set input record separator to NUL,
to match corresponding option to git command to separate records with NUL
(the '-z' option in this case).  Others are git_get_path_by_hash(),
parse_commit(),
and parse_commits(), git_tree(), not including places where we set $/ to undef
to slurp whole file: git_get_link_target(), git_blobdiff() for $format
== 'plain', etc.

> Signed-off-by: Krzesimir Nowak <krzesimir@endocode.com>
> ---
>  gitweb/gitweb.perl | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 68c77f6..ee61f9e 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -2629,6 +2629,8 @@ sub git_parse_project_config {
>         my $section_regexp = shift;
>         my %config;
>
> +       # input record separator, so getline does end on null, not
> +       # newline
>         local $/ = "\0";

It is here because of '-z' option below (to account for values with
embedded newlines).

>
>         open my $fh, "-|", git_cmd(), "config", '-z', '-l',
> --
> 1.8.3.1
>



-- 
Jakub Narebski

Re: [PATCH 1/5] gitweb: Add a comment explaining the meaning of $/

[Krzesimir Nowak]

On Wed, 2013-12-04 at 16:11 +0100, Jakub Narębski wrote:
> On Wed, Dec 4, 2013 at 2:42 PM, Krzesimir Nowak <krzesimir@endocode.com> wrote:
> 
> > So future reader will know what does it mean without running "perldoc
> > perlvar".
> 
> Hmmm... shouldn't future reader know it anyway?  It is not that cryptic.
> I'd say it is idiomatic Perl.
> 

It's plainly obscure. And I think it is not that often used - I keep
forgetting what that pair of punctuation is actually meaning. In this
case I guess it would be more readable to use the following code
instead:
$fh->input_record_separator ("\0");

> Besides, it is not the only place where we set input record separator to NUL,
> to match corresponding option to git command to separate records with NUL
> (the '-z' option in this case).  Others are git_get_path_by_hash(),
> parse_commit(),
> and parse_commits(), git_tree(), not including places where we set $/ to undef
> to slurp whole file: git_get_link_target(), git_blobdiff() for $format
> == 'plain', etc.

Yes, but I stumbled upon that one when trying to understand how config
parsing works. So I added a comment.

> 
> > Signed-off-by: Krzesimir Nowak <krzesimir@endocode.com>
> > ---
> >  gitweb/gitweb.perl | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> > index 68c77f6..ee61f9e 100755
> > --- a/gitweb/gitweb.perl
> > +++ b/gitweb/gitweb.perl
> > @@ -2629,6 +2629,8 @@ sub git_parse_project_config {
> >         my $section_regexp = shift;
> >         my %config;
> >
> > +       # input record separator, so getline does end on null, not
> > +       # newline
> >         local $/ = "\0";
> 
> It is here because of '-z' option below (to account for values with
> embedded newlines).
> 
> >
> >         open my $fh, "-|", git_cmd(), "config", '-z', '-l',
> > --
> > 1.8.3.1
> >
> 
> 
> 

-- 
Krzesimir Nowak
Software Developer
Endocode AG

krzesimir@endocode.com

------
Endocode AG, Johannisstraße 20, 10117 Berlin
info@endocode.com | www.endocode.com

Vorstandsvorsitzender: Mirko Boehm
Vorstände: Dr. Karl Beecher, Chris Kühl, Sebastian Sucker
Aufsichtsratsvorsitzende: Jennifer Beecher

Registergericht: Amtsgericht Charlottenburg - HRB 150748 B

Re: [PATCH 2/5] gitweb: Move check-ref-format code into separate function

[Jakub Narębski]

On Wed, Dec 4, 2013 at 2:43 PM, Krzesimir Nowak <krzesimir@endocode.com> wrote:

> This check will be used in more than one place later.
>
> Signed-off-by: Krzesimir Nowak <krzesimir@endocode.com>
> Reviewed-by: Junio C Hamano <gitster@pobox.com>
> Reviewed-by: Jakub Narębski <jnareb@gmail.com>

All right, that is nice refactoring.

> ---
>  gitweb/gitweb.perl | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index ee61f9e..67415b9 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
> +       check_ref_format($input)
> +               or return undef;
>         return $input;
>  }

Right, check_ref_format() has name after git-check-ref-format...
though... check_ref_format() or die  doesn't read completely
naturally...

-- 
Jakub Narebski

Re: [PATCH 3/5] gitweb: Return plain booleans in validation methods

[Jakub Narębski]

On Wed, Dec 4, 2013 at 2:43 PM, Krzesimir Nowak <krzesimir@endocode.com> wrote:

> Users of validate_* passing "0" might get failures on correct name
> because of coercion of "0" to false in code like:
> die_error(500, "invalid ref") unless (check_ref_format ("0"));

I would say that the problem was that validate_sth() subroutines returned
value of parameter if it was valid, which could be a problem if said value is
false-ish (e.g. validate_refname("0"), or validate_pathname("0")).

Returning undef on invalid data newer was a problem, using 'return $input;'
on valid input was, especially that validate_sth() functions were ever used
in a conditional:

  if (!validate_sth($param)) {
      die_error(...)
  }

While at it validate_sth() is not a best name for boolean predicate:
is_valid_sth() would be better, I think.

> Signed-off-by: Krzesimir Nowak <krzesimir@endocode.com>
> ---
>  gitweb/gitweb.perl | 45 +++++++++++++++++++++++++--------------------
>  1 file changed, 25 insertions(+), 20 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 67415b9..3434602 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -1419,63 +1419,68 @@ sub href {
>  ## validation, quoting/unquoting and escaping
>
>  sub validate_action {
> -       my $input = shift || return undef;
> -       return undef unless exists $actions{$input};
> -       return $input;
> +       my $input = shift;
> +
> +       return 0 unless defined $input;
> +       return 0 unless exists $actions{$input};
> +       return 1;
>  }

The only change that needs to be doe is replacing

           return $input;

with

           return 1;

-- 
Jakub Narebski

Re: [PATCH 1/5] gitweb: Add a comment explaining the meaning of $/

[Martin Langhoff]

On Wed, Dec 4, 2013 at 10:46 AM, Krzesimir Nowak <krzesimir@endocode.com> wrote:
> On Wed, 2013-12-04 at 16:11 +0100, Jakub Narębski wrote:
>> On Wed, Dec 4, 2013 at 2:42 PM, Krzesimir Nowak <krzesimir@endocode.com> wrote:
>>
>> > So future reader will know what does it mean without running "perldoc
>> > perlvar".
>>
>> Hmmm... shouldn't future reader know it anyway?  It is not that cryptic.
>> I'd say it is idiomatic Perl.
>
> It's plainly obscure. And I think it is not that often used -

It's classic Perl.

Perhaps you'd want to "use English;" and call it
$INPUT_RECORD_SEPARATOR in a patch titled "Make things readable to
non-Perl natives".

cheers,



m
-- 
 martin.langhoff@gmail.com
 -  ask interesting questions
 - don't get distracted with shiny stuff  - working code first
 ~ http://docs.moodle.org/en/User:Martin_Langhoff

Re: [PATCH 1/5] gitweb: Add a comment explaining the meaning of $/

[Jakub Narębski]

On Wed, Dec 4, 2013 at 4:46 PM, Krzesimir Nowak <krzesimir@endocode.com> wrote:
> On Wed, 2013-12-04 at 16:11 +0100, Jakub Narębski wrote:
>> On Wed, Dec 4, 2013 at 2:42 PM, Krzesimir Nowak <krzesimir@endocode.com> wrote:
>>
>> > So future reader will know what does it mean without running "perldoc
>> > perlvar".
>>
>> Hmmm... shouldn't future reader know it anyway?  It is not that cryptic.
>> I'd say it is idiomatic Perl.
>
> It's plainly obscure. And I think it is not that often used - I keep
> forgetting what that pair of punctuation is actually meaning.

I think it depends on what kind of Perl code one is used to. It is not
as obscure as $; and similar to $|, I think.

> In this case I guess it would be more readable to use the following code
> instead:
>
>    $fh->input_record_separator ("\0");

That would be a good change to replace

        local $/ = "\0";

        open my $fh, "-|", git_cmd(), ..., '-z', ...

with

        open my $fh, "-|", git_cmd(), ..., '-z', ...
        $fh->input_record_separator ("\0");

(not forgetting about "use IO::Handle", which module is core Perl module);


Anyway, this change (or proposed change) does not, IMHO, belong
in this series.

-- 
Jakub Narebski

Re: [PATCH 1/5] gitweb: Add a comment explaining the meaning of $/

[Jakub Narębski]

On Wed, Dec 4, 2013 at 6:34 PM, Jakub Narębski <jnareb@gmail.com> wrote:
> On Wed, Dec 4, 2013 at 4:46 PM, Krzesimir Nowak <krzesimir@endocode.com> wrote:
>> On Wed, 2013-12-04 at 16:11 +0100, Jakub Narębski wrote:
>>> On Wed, Dec 4, 2013 at 2:42 PM, Krzesimir Nowak <krzesimir@endocode.com> wrote:
>>>
>>> > So future reader will know what does it mean without running "perldoc
>>> > perlvar".
>>>
>>> Hmmm... shouldn't future reader know it anyway?  It is not that cryptic.
>>> I'd say it is idiomatic Perl.
>>
>> It's plainly obscure. And I think it is not that often used - I keep
>> forgetting what that pair of punctuation is actually meaning.
>
> I think it depends on what kind of Perl code one is used to. It is not
> as obscure as $; and similar to $|, I think.
>
>> In this case I guess it would be more readable to use the following code
>> instead:
>>
>>    $fh->input_record_separator ("\0");
>
> That would be a good change to replace
>
>         local $/ = "\0";
>
>         open my $fh, "-|", git_cmd(), ..., '-z', ...
>
> with
>
>         open my $fh, "-|", git_cmd(), ..., '-z', ...
>         $fh->input_record_separator ("\0");
>
> (not forgetting about "use IO::Handle", which module is core Perl module);

Actually it is replacing

         local $/ = "\0";

with

        IO::Handle->input_record_separator("\0");

see http://p3rl.org/IO::Handle

Re: [PATCH 4/5] gitweb: Add a feature for adding more branch refs

[Jakub Narębski]

On Wed, Dec 4, 2013 at 2:43 PM, Krzesimir Nowak <krzesimir@endocode.com> wrote:
>
> 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 <krzesimir@endocode.com>
> Reviewed-by: Junio C Hamano <gitster@pobox.com>
> Reviewed-by: Jakub Narębski <jnareb@gmail.com>

This version is Helped-by (maybe), but not (yet!) Reviewed-by.

> ---
>  Documentation/gitweb.conf.txt | 37 +++++++++++++++++++
>  gitweb/gitweb.perl            | 85 +++++++++++++++++++++++++++++++++++++------
>  2 files changed, 110 insertions(+), 12 deletions(-)
>
> diff --git a/Documentation/gitweb.conf.txt b/Documentation/gitweb.conf.txt
> index e2113d9..5a77452 100644
> --- a/Documentation/gitweb.conf.txt
> +++ b/Documentation/gitweb.conf.txt
> @@ -849,6 +849,43 @@ 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

s/was/is/;

Perhaps it would better read as

    This feature can be configured on per-repository basis after setting
    $feature{'extra-branch-refs'}{'override'} to true, via repository's
    `gitweb.extraBranchRefs` ...

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

O.K.

> ++
> +The gitweb.extrabranchrefs is actually a multi-valued configuration
> +variable, so following example is also correct and the result is the
> +same as of the snippet above:
> ++
> +--------------------------------------------------------------------------------
> +[gitweb]
> +       extrabranchrefs = sandbox
> +       extrabranchrefs = wip other
> +--------------------------------------------------------------------------------

I think this part should be better left for a separate patch. There is
important difference between single-valued and multi-valued configuration
variable: with single-valued later occurrences override earlier ones,
which includes settings in more specific config file (e.g. per-repository)
overriding setting in more general one (e.g. per-user or system-wide).

With multi-valued we won't be able to override earlier / more generic
settings... well, unless we add support for no-value, or empty-value
as clearer, i.e.

  [gitweb]
           extrabranchrefs = sandbox
           extrabranchrefs
 # or    extrabranchrefs =
           extrabranchrefs = wip other

resulting in ('wip', 'other').

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

Hmmm... 'snapshot' feature ignores invalid values, but in this case
formerly valid compression schemes might get invalid via tightening
%known_snapshot_formats, and we don't want existing config getting
suddenly invalid.

[cut]

Nice!

-- 
Jakub Narebski

Re: [PATCH 3/5] gitweb: Return plain booleans in validation methods

[Junio C Hamano]

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

> On Wed, Dec 4, 2013 at 2:43 PM, Krzesimir Nowak <krzesimir@endocode.com> wrote:
>
>> Users of validate_* passing "0" might get failures on correct name
>> because of coercion of "0" to false in code like:
>> die_error(500, "invalid ref") unless (check_ref_format ("0"));
>
> I would say that the problem was that validate_sth() subroutines returned
> value of parameter if it was valid, which could be a problem if said value is
> false-ish (e.g. validate_refname("0"), or validate_pathname("0")).
>
> Returning undef on invalid data newer was a problem, using 'return $input;'
> on valid input was, especially that validate_sth() functions were ever used
> in a conditional:
>
>   if (!validate_sth($param)) {
>       die_error(...)
>   }

Correct, and I think that is exactly what the above three lines in
the proposed log message says.

> While at it validate_sth() is not a best name for boolean predicate:
> is_valid_sth() would be better, I think.

Yeah, I tend to agree.  As we are doing a whole-sale API clean-up in
this patch, we might as well do that at the same time.

>> Signed-off-by: Krzesimir Nowak <krzesimir@endocode.com>
>> ---
>>  gitweb/gitweb.perl | 45 +++++++++++++++++++++++++--------------------
>>  1 file changed, 25 insertions(+), 20 deletions(-)
>>
>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
>> index 67415b9..3434602 100755
>> --- a/gitweb/gitweb.perl
>> +++ b/gitweb/gitweb.perl
>> @@ -1419,63 +1419,68 @@ sub href {
>>  ## validation, quoting/unquoting and escaping
>>
>>  sub validate_action {
>> -       my $input = shift || return undef;
>> -       return undef unless exists $actions{$input};
>> -       return $input;
>> +       my $input = shift;
>> +
>> +       return 0 unless defined $input;
>> +       return 0 unless exists $actions{$input};
>> +       return 1;
>>  }
>
> The only change that needs to be doe is replacing
>
>            return $input;
>
> with
>
>            return 1;

Re: [PATCH 5/5] gitweb: Denote non-heads, non-remotes branches

[Jakub Narębski]

On Wed, Dec 4, 2013 at 2:43 PM, Krzesimir Nowak <krzesimir@endocode.com> wrote:

> 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-<short hash>.tgz'.
>
> Signed-off-by: Krzesimir Nowak <krzesimir@endocode.com>

Very nice feature, which allows to distinguish between refs/heads/feature
and refs/wip/feature.

> ---
>  gitweb/gitweb.perl | 27 +++++++++++++++++++++++++--
>  1 file changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 6d3d52d..9a63ea9 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -3739,8 +3739,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

Perhaps simply:

  +               # for refs neither in 'heads' nor 'remotes' we want
to show their 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;
> @@ -7257,7 +7263,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;
> +                       }

I think this safety replacement should apply also to other parts of filename,
if it is to be used.

$ref_dir should be compatibile with path-part - in loose form it is stored
on filesystem... though different filesystems might have different restrictions.

One thing we should worry about is '/' in hierarchical refdir names, e.g.
'wip/jk', which needs to be replaced / sanitized somehow, though probably
as 'wip-jk' or 'wip_jk' rather than 'wipjk', isn't it?

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



-- 
Jakub Narebski

Re: [PATCH 1/5] gitweb: Add a comment explaining the meaning of $/

[Junio C Hamano]

Martin Langhoff <martin.langhoff@gmail.com> writes:

> On Wed, Dec 4, 2013 at 10:46 AM, Krzesimir Nowak <krzesimir@endocode.com> wrote:
>> On Wed, 2013-12-04 at 16:11 +0100, Jakub Narębski wrote:
>>> On Wed, Dec 4, 2013 at 2:42 PM, Krzesimir Nowak <krzesimir@endocode.com> wrote:
>>>
>>> > So future reader will know what does it mean without running "perldoc
>>> > perlvar".
>>>
>>> Hmmm... shouldn't future reader know it anyway?  It is not that cryptic.
>>> I'd say it is idiomatic Perl.
>>
>> It's plainly obscure. And I think it is not that often used -
>
> It's classic Perl.
>
> Perhaps you'd want to "use English;" and call it
> $INPUT_RECORD_SEPARATOR in a patch titled "Make things readable to
> non-Perl natives".

Hmm, but do we want to see "use English" there in the first place?

Re: [PATCH 2/5] gitweb: Move check-ref-format code into separate function

[Junio C Hamano]

Krzesimir Nowak <krzesimir@endocode.com> writes:

> This check will be used in more than one place later.
>
> Signed-off-by: Krzesimir Nowak <krzesimir@endocode.com>
> Reviewed-by: Junio C Hamano <gitster@pobox.com>

Again, I do not think I reviewed this exact version. Nor did I say
that use of the "... or return undef" is a good idea.

> Reviewed-by: Jakub Narębski <jnareb@gmail.com>
> ---
>  gitweb/gitweb.perl | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index ee61f9e..67415b9 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
> +	check_ref_format($input)
> +		or return undef;
>  	return $input;
>  }

Re: [PATCH 0/5] Show extra branch refs in gitweb v6

[Junio C Hamano]

Krzesimir Nowak <krzesimir@endocode.com> writes:

> First patch just adds a comment I though would be useful when trying
> to understand how config parsing is done.
>
> Second patch splits some code to a function.
>
> Third patch fixes validation functions to return either 0 or 1,
> instead of undef or passed $input.
>
> Fourth patch adds the extra-branch-feature and some documentation.
>
> Fifth patch adds some visual differentation of branches from
> non-standard ref directories.
>
> Krzesimir Nowak (5):
>   gitweb: Add a comment explaining the meaning of $/
>   gitweb: Move check-ref-format code into separate function
>   gitweb: Return plain booleans in validation methods
>   gitweb: Add a feature for adding more branch refs
>   gitweb: Denote non-heads, non-remotes branches
>
>  Documentation/gitweb.conf.txt |  27 +++++++
>  gitweb/gitweb.perl            | 166 +++++++++++++++++++++++++++++++++---------
>  2 files changed, 160 insertions(+), 33 deletions(-)

Thanks; I'll tentatively queue 2 thru 5 on 'pu', but people are
feeding good suggestions on it, so I'd expect further rerolls.

A tip: "git format-patch -v7 --cover-letter" may be handy.

Re: [PATCH 1/5] gitweb: Add a comment explaining the meaning of $/

[Krzesimir Nowak]

On Wed, 2013-12-04 at 12:28 -0800, Junio C Hamano wrote:
> Martin Langhoff <martin.langhoff@gmail.com> writes:
> 
> > On Wed, Dec 4, 2013 at 10:46 AM, Krzesimir Nowak <krzesimir@endocode.com> wrote:
> >> On Wed, 2013-12-04 at 16:11 +0100, Jakub Narębski wrote:
> >>> On Wed, Dec 4, 2013 at 2:42 PM, Krzesimir Nowak <krzesimir@endocode.com> wrote:
> >>>
> >>> > So future reader will know what does it mean without running "perldoc
> >>> > perlvar".
> >>>
> >>> Hmmm... shouldn't future reader know it anyway?  It is not that cryptic.
> >>> I'd say it is idiomatic Perl.
> >>
> >> It's plainly obscure. And I think it is not that often used -
> >
> > It's classic Perl.
> >
> > Perhaps you'd want to "use English;" and call it
> > $INPUT_RECORD_SEPARATOR in a patch titled "Make things readable to
> > non-Perl natives".
> 
> Hmm, but do we want to see "use English" there in the first place?

Nevermind, I'm going to drop that patch.

-- 
Krzesimir Nowak
Software Developer
Endocode AG

krzesimir@endocode.com

------
Endocode AG, Johannisstraße 20, 10117 Berlin
info@endocode.com | www.endocode.com

Vorstandsvorsitzender: Mirko Boehm
Vorstände: Dr. Karl Beecher, Chris Kühl, Sebastian Sucker
Aufsichtsratsvorsitzende: Jennifer Beecher

Registergericht: Amtsgericht Charlottenburg - HRB 150748 B

Re: [PATCH 2/5] gitweb: Move check-ref-format code into separate function

[Krzesimir Nowak]

On Wed, 2013-12-04 at 12:31 -0800, Junio C Hamano wrote:
> Krzesimir Nowak <krzesimir@endocode.com> writes:
> 
> > This check will be used in more than one place later.
> >
> > Signed-off-by: Krzesimir Nowak <krzesimir@endocode.com>
> > Reviewed-by: Junio C Hamano <gitster@pobox.com>
> 
> Again, I do not think I reviewed this exact version. Nor did I say
> that use of the "... or return undef" is a good idea.

Ok, I'll drop them. Too much fuss over those lines.

> 
> > Reviewed-by: Jakub Narębski <jnareb@gmail.com>
> > ---
> >  gitweb/gitweb.perl | 17 +++++++++++++----
> >  1 file changed, 13 insertions(+), 4 deletions(-)
> >
> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> > index ee61f9e..67415b9 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
> > +	check_ref_format($input)
> > +		or return undef;
> >  	return $input;
> >  }

-- 
Krzesimir Nowak
Software Developer
Endocode AG

krzesimir@endocode.com

------
Endocode AG, Johannisstraße 20, 10117 Berlin
info@endocode.com | www.endocode.com

Vorstandsvorsitzender: Mirko Boehm
Vorstände: Dr. Karl Beecher, Chris Kühl, Sebastian Sucker
Aufsichtsratsvorsitzende: Jennifer Beecher

Registergericht: Amtsgericht Charlottenburg - HRB 150748 B

Re: [PATCH 2/5] gitweb: Move check-ref-format code into separate function

[Krzesimir Nowak]

On Wed, 2013-12-04 at 16:56 +0100, Jakub Narębski wrote:
> On Wed, Dec 4, 2013 at 2:43 PM, Krzesimir Nowak <krzesimir@endocode.com> wrote:
> 
> > This check will be used in more than one place later.
> >
> > Signed-off-by: Krzesimir Nowak <krzesimir@endocode.com>
> > Reviewed-by: Junio C Hamano <gitster@pobox.com>
> > Reviewed-by: Jakub Narębski <jnareb@gmail.com>
> 
> All right, that is nice refactoring.
> 
> > ---
> >  gitweb/gitweb.perl | 17 +++++++++++++----
> >  1 file changed, 13 insertions(+), 4 deletions(-)
> >
> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> > index ee61f9e..67415b9 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
> > +       check_ref_format($input)
> > +               or return undef;
> >         return $input;
> >  }
> 
> Right, check_ref_format() has name after git-check-ref-format...
> though... check_ref_format() or die  doesn't read completely
> naturally...
> 

Ok, I'll rename it to is_valid_ref_format.

-- 
Krzesimir Nowak
Software Developer
Endocode AG

krzesimir@endocode.com

------
Endocode AG, Johannisstraße 20, 10117 Berlin
info@endocode.com | www.endocode.com

Vorstandsvorsitzender: Mirko Boehm
Vorstände: Dr. Karl Beecher, Chris Kühl, Sebastian Sucker
Aufsichtsratsvorsitzende: Jennifer Beecher

Registergericht: Amtsgericht Charlottenburg - HRB 150748 B

Re: [PATCH 3/5] gitweb: Return plain booleans in validation methods

[Krzesimir Nowak]

On Wed, 2013-12-04 at 17:07 +0100, Jakub Narębski wrote:
> On Wed, Dec 4, 2013 at 2:43 PM, Krzesimir Nowak <krzesimir@endocode.com> wrote:
> 
> > Users of validate_* passing "0" might get failures on correct name
> > because of coercion of "0" to false in code like:
> > die_error(500, "invalid ref") unless (check_ref_format ("0"));
> 
> I would say that the problem was that validate_sth() subroutines returned
> value of parameter if it was valid, which could be a problem if said value is
> false-ish (e.g. validate_refname("0"), or validate_pathname("0")).

That's what I meant.

> 
> Returning undef on invalid data newer was a problem, using 'return $input;'
> on valid input was, especially that validate_sth() functions were ever used
> in a conditional:
> 
>   if (!validate_sth($param)) {
>       die_error(...)
>   }
> 
> While at it validate_sth() is not a best name for boolean predicate:
> is_valid_sth() would be better, I think.

Ok, I'll rename those.

> 
> > Signed-off-by: Krzesimir Nowak <krzesimir@endocode.com>
> > ---
> >  gitweb/gitweb.perl | 45 +++++++++++++++++++++++++--------------------
> >  1 file changed, 25 insertions(+), 20 deletions(-)
> >
> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> > index 67415b9..3434602 100755
> > --- a/gitweb/gitweb.perl
> > +++ b/gitweb/gitweb.perl
> > @@ -1419,63 +1419,68 @@ sub href {
> >  ## validation, quoting/unquoting and escaping
> >
> >  sub validate_action {
> > -       my $input = shift || return undef;
> > -       return undef unless exists $actions{$input};
> > -       return $input;
> > +       my $input = shift;
> > +
> > +       return 0 unless defined $input;
> > +       return 0 unless exists $actions{$input};
> > +       return 1;
> >  }
> 
> The only change that needs to be doe is replacing
> 
>            return $input;
> 
> with
> 
>            return 1;
> 

I prefer to use zeros instead of undefs - one might wonder if that undef
is somewhat special that we can't use 0.

-- 
Krzesimir Nowak
Software Developer
Endocode AG

krzesimir@endocode.com

------
Endocode AG, Johannisstraße 20, 10117 Berlin
info@endocode.com | www.endocode.com

Vorstandsvorsitzender: Mirko Boehm
Vorstände: Dr. Karl Beecher, Chris Kühl, Sebastian Sucker
Aufsichtsratsvorsitzende: Jennifer Beecher

Registergericht: Amtsgericht Charlottenburg - HRB 150748 B

Re: [PATCH 4/5] gitweb: Add a feature for adding more branch refs

[Krzesimir Nowak]

On Wed, 2013-12-04 at 19:06 +0100, Jakub Narębski wrote:
> On Wed, Dec 4, 2013 at 2:43 PM, Krzesimir Nowak <krzesimir@endocode.com> wrote:
> >
> > 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 <krzesimir@endocode.com>
> > Reviewed-by: Junio C Hamano <gitster@pobox.com>
> > Reviewed-by: Jakub Narębski <jnareb@gmail.com>
> 
> This version is Helped-by (maybe), but not (yet!) Reviewed-by.
> 
> > ---
> >  Documentation/gitweb.conf.txt | 37 +++++++++++++++++++
> >  gitweb/gitweb.perl            | 85 +++++++++++++++++++++++++++++++++++++------
> >  2 files changed, 110 insertions(+), 12 deletions(-)
> >
> > diff --git a/Documentation/gitweb.conf.txt b/Documentation/gitweb.conf.txt
> > index e2113d9..5a77452 100644
> > --- a/Documentation/gitweb.conf.txt
> > +++ b/Documentation/gitweb.conf.txt
> > @@ -849,6 +849,43 @@ 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
> 
> s/was/is/;
> 
> Perhaps it would better read as
> 
>     This feature can be configured on per-repository basis after setting
>     $feature{'extra-branch-refs'}{'override'} to true, via repository's
>     `gitweb.extraBranchRefs` ...

I see that you also insist on using camelCase for config variables. I
will rephrase it.

> 
> > +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
> > +--------------------------------------------------------------------------------
> 
> O.K.
> 
> > ++
> > +The gitweb.extrabranchrefs is actually a multi-valued configuration
> > +variable, so following example is also correct and the result is the
> > +same as of the snippet above:
> > ++
> > +--------------------------------------------------------------------------------
> > +[gitweb]
> > +       extrabranchrefs = sandbox
> > +       extrabranchrefs = wip other
> > +--------------------------------------------------------------------------------
> 
> I think this part should be better left for a separate patch. There is
> important difference between single-valued and multi-valued configuration
> variable: with single-valued later occurrences override earlier ones,
> which includes settings in more specific config file (e.g. per-repository)
> overriding setting in more general one (e.g. per-user or system-wide).
> 
> With multi-valued we won't be able to override earlier / more generic
> settings... well, unless we add support for no-value, or empty-value
> as clearer, i.e.
> 
>   [gitweb]
>            extrabranchrefs = sandbox
>            extrabranchrefs
>  # or    extrabranchrefs =
>            extrabranchrefs = wip other
> 
> resulting in ('wip', 'other').

That point in this example is a bit moot IMO - if you don't want sandbox
ref to appear in list of branches view then just delete the offending
line.

I also made a small experiment. In per-instance config I have
$feature{'extra-branch-refs'}{'default'} = ['wip'];
$feature{'extra-branch-refs'}{'override'} = 1;

In per-repository config I have:
[gitweb]
	extrabranchrefs = sandbox
	extrabranchrefs = other

List of branches view shows only branches from sandbox and other. So
clearly per-repository config overrides per-instance config.

> 
> > ++
> > +It is an error to specify a ref that does not pass "git check-ref-format"
> > +scrutiny. Duplicated values are filtered.
> > +
> 
> Hmmm... 'snapshot' feature ignores invalid values, but in this case
> formerly valid compression schemes might get invalid via tightening
> %known_snapshot_formats, and we don't want existing config getting
> suddenly invalid.
> 

So, should I just ignore the invalid refs or treat them as errors?

> [cut]
> 
> Nice!
> 

-- 
Krzesimir Nowak
Software Developer
Endocode AG

krzesimir@endocode.com

------
Endocode AG, Johannisstraße 20, 10117 Berlin
info@endocode.com | www.endocode.com

Vorstandsvorsitzender: Mirko Boehm
Vorstände: Dr. Karl Beecher, Chris Kühl, Sebastian Sucker
Aufsichtsratsvorsitzende: Jennifer Beecher

Registergericht: Amtsgericht Charlottenburg - HRB 150748 B

Re: [PATCH 4/5] gitweb: Add a feature for adding more branch refs

[Jakub Narębski]

On Thu, Dec 5, 2013 at 11:00 AM, Krzesimir Nowak <krzesimir@endocode.com> wrote:
> On Wed, 2013-12-04 at 19:06 +0100, Jakub Narębski wrote:
>> On Wed, Dec 4, 2013 at 2:43 PM, Krzesimir Nowak <krzesimir@endocode.com> wrote:

>>> +The gitweb.extrabranchrefs is actually a multi-valued configuration
>>> +variable, so following example is also correct and the result is the
>>> +same as of the snippet above:
>>> ++
>>> +--------------------------------------------------------------------------------
>>> +[gitweb]
>>> +       extrabranchrefs = sandbox
>>> +       extrabranchrefs = wip other
>>> +--------------------------------------------------------------------------------
>>
>> I think this part should be better left for a separate patch. There is
>> important difference between single-valued and multi-valued configuration
>> variable: with single-valued later occurrences override earlier ones,
>> which includes settings in more specific config file (e.g. per-repository)
>> overriding setting in more general one (e.g. per-user or system-wide).
>>
>> With multi-valued we won't be able to override earlier / more generic
>> settings... well, unless we add support for no-value, or empty-value
>> as clearer, i.e.
>>
>>   [gitweb]
>>            extrabranchrefs = sandbox
>>            extrabranchrefs
>>  # or    extrabranchrefs =
>>            extrabranchrefs = wip other
>>
>> resulting in ('wip', 'other').

What I didn't notice is that gitweb already supports and *uses* multi-value
configuration variables: gitweb.url and gitweb.ctag (note: the feature is
called 'ctags' and is not overridable), though not for %feature-s.

So the point is moot, sorry for distraction.

BTW. there is config_to_multi() subroutine that you can use.

> That point in this example is a bit moot IMO - if you don't want sandbox
> ref to appear in list of branches view then just delete the offending
> line.

I was thinking about more complicated situation: if you have gitweb
configured in "scan for repositories" mode (like mod_homedir), then
repository owner may change gitweb configuration for own repository
via repo config file.  But gitweb reads also web server user config file
and system wide git config file (/etc/gitconfig).  If gitweb.extraBranchRefs
is set in one of those files, repo owner cannot override, but only add
to it.

But I think this is worrying about nothing. I'm sorry for distraction.

> I also made a small experiment. In per-instance config I have
> $feature{'extra-branch-refs'}{'default'} = ['wip'];
> $feature{'extra-branch-refs'}{'override'} = 1;
>
> In per-repository config I have:
> [gitweb]
>         extrabranchrefs = sandbox
>         extrabranchrefs = other
>
> List of branches view shows only branches from sandbox and other. So
> clearly per-repository config overrides per-instance config.

That's why is called 'override'... but it is not what I was worrying about,
see above.

-- 
Jakub Narebski

Re: [PATCH 3/5] gitweb: Return plain booleans in validation methods

[Junio C Hamano]

Krzesimir Nowak <krzesimir@endocode.com> writes:

> On Wed, 2013-12-04 at 17:07 +0100, Jakub Narębski wrote:
>> The only change that needs to be doe is replacing
>> 
>>            return $input;
>> 
>> with
>> 
>>            return 1;
>> 
>
> I prefer to use zeros instead of undefs - one might wonder if that undef
> is somewhat special that we can't use 0.

For Perl speakers, I suspect the code gives a totally opposite
impression.  Normal "false" return from a sub, when there is no
special need, is to return an undef from it, and a "return 0" would
make the readers wonder if there is something special about the way
the returned value has to be numeric zero, no?

Re: [PATCH 3/5] gitweb: Return plain booleans in validation methods

[Jakub Narębski]

On Thu, Dec 5, 2013 at 7:16 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Krzesimir Nowak <krzesimir@endocode.com> writes:
>> On Wed, 2013-12-04 at 17:07 +0100, Jakub Narębski wrote:

>>> The only change that needs to be done is replacing
>>>
>>>            return $input;
>>>
>>> with
>>>
>>>            return 1;
>>>
>>
>> I prefer to use zeros instead of undefs - one might wonder if that undef
>> is somewhat special that we can't use 0.
>
> For Perl speakers, I suspect the code gives a totally opposite
> impression.  Normal "false" return from a sub, when there is no
> special need, is to return an undef from it, and a "return 0" would
> make the readers wonder if there is something special about the way
> the returned value has to be numeric zero, no?

Or even plain "return;" (see explanation for policy in [1])...
though for functions returning scalar it is recommended
to use explicit "return 0;" (or "return undef;").

Anyway, it is easier to see the change and intention of the change
if all that is changed id "return $input;" to "return 1;"

But I am not against "return 0;" on validation error (would putting
it in separate patch make review easier, or just pointlessly proliferate
patches?).

[1]: http://search.cpan.org/~thaljef/Perl-Critic-1.121/lib/Perl/Critic/Policy/Subroutines/ProhibitExplicitReturnUndef.pm
-- 
Jakub Narebski

Re: [PATCH 3/5] gitweb: Return plain booleans in validation methods

[Junio C Hamano]

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

> But I am not against "return 0;" on validation error (would putting
> it in separate patch make review easier, or just pointlessly proliferate
> patches?).

OK.

Re: [PATCH 4/5] gitweb: Add a feature for adding more branch refs

[Krzesimir Nowak]

On Thu, 2013-12-05 at 11:00 +0100, Krzesimir Nowak wrote:
> On Wed, 2013-12-04 at 19:06 +0100, Jakub Narębski wrote:
> > On Wed, Dec 4, 2013 at 2:43 PM, Krzesimir Nowak <krzesimir@endocode.com> wrote:
> > > ++
> > > +It is an error to specify a ref that does not pass "git check-ref-format"
> > > +scrutiny. Duplicated values are filtered.
> > > +
> > 
> > Hmmm... 'snapshot' feature ignores invalid values, but in this case
> > formerly valid compression schemes might get invalid via tightening
> > %known_snapshot_formats, and we don't want existing config getting
> > suddenly invalid.
> > 
> 
> So, should I just ignore the invalid refs or treat them as errors?

Ping, I'd like to know the answer for this question before I roll out
another set of patches - there seem to be no agreement over it. Thanks!

Cheers,
-- 
Krzesimir Nowak
Software Developer
Endocode AG

krzesimir@endocode.com

------
Endocode AG, Johannisstraße 20, 10117 Berlin
info@endocode.com | www.endocode.com

Vorstandsvorsitzender: Mirko Boehm
Vorstände: Dr. Karl Beecher, Chris Kühl, Sebastian Sucker
Aufsichtsratsvorsitzende: Jennifer Beecher

Registergericht: Amtsgericht Charlottenburg - HRB 150748 B

Re: [PATCH 4/5] gitweb: Add a feature for adding more branch refs

[Junio C Hamano]

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

> With multi-valued we won't be able to override earlier / more generic
> settings... well, unless we add support for no-value, or empty-value
> as clearer, i.e.
>
>   [gitweb]
>            extrabranchrefs = sandbox
>            extrabranchrefs
>  # or    extrabranchrefs =
>            extrabranchrefs = wip other
>
> resulting in ('wip', 'other').

Please don't, unless you are going to change the entire config
machinery (including config.c bits that are used by the rest of the
system) to follow such a convention to "clear settings so far read".

>> ++
>> +It is an error to specify a ref that does not pass "git check-ref-format"
>> +scrutiny. Duplicated values are filtered.
>> +
>
> Hmmm... 'snapshot' feature ignores invalid values, but in this case
> formerly valid compression schemes might get invalid via tightening
> %known_snapshot_formats, and we don't want existing config getting
> suddenly invalid.

Sorry, but what does check-ref-format have to do with "formerly
valid compression scheme that suddenly gets invalidated"???

Re: [PATCH 4/5] gitweb: Add a feature for adding more branch refs

[Jakub Narębski]

On Tue, Dec 10, 2013 at 7:54 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jakub Narębski <jnareb@gmail.com> writes:
>
>> With multi-valued we won't be able to override earlier / more generic
>> settings... well, unless we add support for no-value, or empty-value
>> as clearer, i.e.
>>
>>   [gitweb]
>>            extrabranchrefs = sandbox
>>            extrabranchrefs
>>  # or    extrabranchrefs =
>>            extrabranchrefs = wip other
>>
>> resulting in ('wip', 'other').
>
> Please don't, unless you are going to change the entire config
> machinery (including config.c bits that are used by the rest of the
> system) to follow such a convention to "clear settings so far read".

Right. Also I haven't noticed that gitweb already uses some
multi-valued config variables, though not for %features.

Please disregard this.

>>> ++
>>> +It is an error to specify a ref that does not pass "git check-ref-format"
>>> +scrutiny. Duplicated values are filtered.
>>> +
>>
>> Hmmm... 'snapshot' feature ignores invalid values, but in this case
>> formerly valid compression schemes might get invalid via tightening
>> %known_snapshot_formats, and we don't want existing config getting
>> suddenly invalid.
>
> Sorry, but what does check-ref-format have to do with "formerly
> valid compression scheme that suddenly gets invalidated"???

What I started to write was that 'snapshot' feature ignores invalid
values, but halfway through writing it out I have noticed that the
situation is really different.

In 'snapshot' case one can have formerly valid per-repository config,
made invalid by gitweb admin tightening %known_snapshot_formats
(for example removing some compression format due to bug). We
do not want for repository to stop being displayed because of something
that is outside control of repository owner.

In 'check-ref-format' feature we have ref either valid or not. This does
not depend on external factors. Something that is valid ref would
remain valid ref.

So 'snaphot' being forgiving doesn't mean that 'check-ref-format'
should be forgiving.

-- 
Jakub Narebski

Re: [PATCH 4/5] gitweb: Add a feature for adding more branch refs

[Junio C Hamano]

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

> So 'snaphot' being forgiving doesn't mean that 'check-ref-format'
> should be forgiving.

OK, thanks for clarifying.