[PATCH 0/3] Show extra branch refs in gitweb

[PATCH 0/3] Show extra branch refs in gitweb

[Krzesimir Nowak]

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

[PATCH 1/3] 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>
Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
---
 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

[PATCH 2/3] 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>
Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
---
 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 <message>' 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

[PATCH 3/3] 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>
Reviewed-by: Junio C Hamano <gitster@pobox.com>
Reviewed-by: Jakub Narębski <jnareb@gmail.com>
Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
---
 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

Re: [PATCH 2/3] gitweb: Add a feature for adding more branch refs

[Junio C Hamano]

Krzesimir Nowak <krzesimir@endocode.com> 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 <krzesimir@endocode.com>
> Reviewed-by: Junio C Hamano <gitster@pobox.com>

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 <jnareb@gmail.com>
> Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>

Re: [PATCH 1/3] 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>
> Reviewed-by: Jakub Narębski <jnareb@gmail.com>
> Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
>  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 "<LF><INDENT>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.

>  }

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

[Jakub Narębski]

On Tue, Dec 3, 2013 at 8:02 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Krzesimir Nowak <krzesimir@endocode.com> 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 "<LF><INDENT>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

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

[Junio C Hamano]

Jakub Narębski <jnareb@gmail.com> 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.

Re: [PATCH 2/3] gitweb: Add a feature for adding more branch refs

[Junio C Hamano]

Krzesimir Nowak <krzesimir@endocode.com> 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;
> +}

Re: [PATCH 2/3] gitweb: Add a feature for adding more branch refs

[Jakub Narębski]

On Tue, Dec 3, 2013 at 9:15 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Krzesimir Nowak <krzesimir@endocode.com> 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

Re: [PATCH 2/3] gitweb: Add a feature for adding more branch refs

[Krzesimir Nowak]

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 <gitster@pobox.com> wrote:
> > Krzesimir Nowak <krzesimir@endocode.com> 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

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

[Krzesimir Nowak]

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 <gitster@pobox.com> wrote:
> > Krzesimir Nowak <krzesimir@endocode.com> 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 "<LF><INDENT>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

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

[Krzesimir Nowak]

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

Re: [PATCH 2/3] gitweb: Add a feature for adding more branch refs

[Junio C Hamano]

Krzesimir Nowak <krzesimir@endocode.com> 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 <gitster@pobox.com> wrote:
>> > Krzesimir Nowak <krzesimir@endocode.com> 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.