[PATCH] gitweb: Add an option for adding more branch refs

[PATCH] gitweb: Add an option for adding more branch refs

[Krzesimir Nowak]

Overriding an @additional_branch_refs configuration variable with
value ('wip') will make gitweb to show branches that appear in
refs/heads and refs/wip (refs/heads is hardcoded). Might be useful for
gerrit setups where user branches are not stored under refs/heads/.

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 68c77f6..9bfd38b 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -17,6 +17,7 @@ use Encode;
 use Fcntl ':mode';
 use File::Find qw();
 use File::Basename qw(basename);
+use List::Util qw(min);
 use Time::HiRes qw(gettimeofday tv_interval);
 binmode STDOUT, ':utf8';
 
@@ -122,6 +123,10 @@ our $logo_label = "git homepage";
 # source of projects list
 our $projects_list = "++GITWEB_LIST++";
 
+# list of additional "directories" under "refs/" we want to display as
+# branches
+our @additional_branch_refs = ();
+
 # the width (in characters) of the projects list "Description" column
 our $projects_list_description_width = 25;
 
@@ -626,14 +631,29 @@ sub feature_avatar {
 	return @val ? @val : @_;
 }
 
+sub get_branch_refs {
+    return ('heads', @additional_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.
 sub check_head_link {
 	my ($dir) = @_;
 	my $headfile = "$dir/HEAD";
-	return ((-e $headfile) ||
-		(-l $headfile && readlink($headfile) =~ /^refs\/heads\//));
+
+	if (-e $headfile) {
+		return 1;
+	}
+	if (-l $headfile) {
+		my $rl = readlink($headfile);
+
+		for my $ref (get_branch_refs()) {
+			return 1 if $rl =~ /^refs\/$ref\//;
+		}
+	}
+
+	return 0;
 }
 
 sub check_export_ok {
@@ -2515,6 +2535,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);
@@ -2522,12 +2543,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/$ref/(.*)$!) ||
+		    (defined $hash      && $hash      =~ m!^refs/$ref/(.*)$!)) {
+			$branch = $1;
+			$matched_ref = $ref;
+			last;
+		}
 	}
 	# find log type for feed description (title)
 	my $type = 'log';
@@ -2540,7 +2566,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;
@@ -3184,24 +3210,43 @@ sub git_get_project_owner {
 	return $owner;
 }
 
-sub git_get_last_activity {
-	my ($path) = @_;
-	my $fd;
+sub git_get_last_activity_age {
+	my ($refs) = @_;
+	my $fd = -1;
 
-	$git_dir = "$projectroot/$path";
 	open($fd, "-|", git_cmd(), 'for-each-ref',
 	     '--format=%(committer)',
 	     '--sort=-committerdate',
 	     '--count=1',
-	     'refs/heads') or return;
+	     $refs) or return undef;
+
 	my $most_recent = <$fd>;
-	close $fd or return;
+	close $fd or return undef;
 	if (defined $most_recent &&
 	    $most_recent =~ / (\d+) [-+][01]\d\d\d$/) {
 		my $timestamp = $1;
-		my $age = time - $timestamp;
-		return ($age, age_string($age));
+		return time - $timestamp;
 	}
+
+	return undef;
+}
+
+sub git_get_last_activity {
+	my ($path) = @_;
+	my @ages = ();
+
+	$git_dir = "$projectroot/$path";
+	for my $ref (get_branch_refs()) {
+		my $age = git_get_last_activity_age('refs/' . $_);
+
+		push @ages, $age if defined $age;
+	}
+	if (@ages) {
+		my $min_age = min(@ages);
+
+		return ($min_age, age_string($min_age));
+	}
+
 	return (undef, undef);
 }
 
@@ -3223,7 +3268,7 @@ sub git_get_remotes_list {
 		next if $wanted and not $remote eq $wanted;
 		my ($url, $key) = ($1, $2);
 
-		$remotes{$remote} ||= { 'heads' => () };
+		$remotes{$remote} ||= { map { $_ => () } get_branch_refs() };
 		$remotes{$remote}{$key} = $url;
 	}
 	close $fd or return;
@@ -3237,9 +3282,11 @@ sub fill_remote_heads {
 	my @heads = map { "remotes/$_" } keys %$remotes;
 	my @remoteheads = git_get_heads_list(undef, @heads);
 	foreach my $remote (keys %$remotes) {
-		$remotes->{$remote}{'heads'} = [ grep {
-			$_->{'name'} =~ s!^$remote/!!
-			} @remoteheads ];
+		foreach my $ref (get_branch_refs()) {
+			$remotes->{$remote}{$ref} = [ grep {
+				$_->{'name'} =~ s!^$remote/!!
+				} @remoteheads ];
+		}
 	}
 }
 
@@ -3644,7 +3691,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;
 
@@ -3662,7 +3709,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 '|', get_branch_refs();
+		$name =~ s!^refs/(?:$strip_refs|remotes)/!!;
 
 		$ref_item{'name'}  = $name;
 		$ref_item{'id'}    = $hash;
@@ -4286,7 +4334,7 @@ sub git_print_page_nav {
 # available if the feature is enabled
 sub format_ref_views {
 	my ($current) = @_;
-	my @ref_views = qw{tags heads};
+	my @ref_views = ('tags', get_branch_refs());
 	push @ref_views, 'remotes' if gitweb_check_feature('remote_heads');
 	return join " | ", map {
 		$_ eq $current ? $_ :
@@ -7179,7 +7227,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 '|', get_branch_refs();
+		if ($hash =~ m!^refs/(?:$strip_refs|remotes)/(.*)$!) {
 			$ver = $1;
 		}
 		$ver .= '-' . git_get_short_hash($project, $hash);
-- 
1.8.3.1

Re: [PATCH] gitweb: Add an option for adding more branch refs

[Junio C Hamano]

Krzesimir Nowak <krzesimir@endocode.com> writes:

> Overriding an @additional_branch_refs configuration variable with
> value ('wip') will make gitweb to show branches that appear in
> refs/heads and refs/wip (refs/heads is hardcoded). Might be useful for
> gerrit setups where user branches are not stored under refs/heads/.
>
> Signed-off-by: Krzesimir Nowak <krzesimir@endocode.com>
> ---
>  gitweb/gitweb.perl | 99 ++++++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 74 insertions(+), 25 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 68c77f6..9bfd38b 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -17,6 +17,7 @@ use Encode;
>  use Fcntl ':mode';
>  use File::Find qw();
>  use File::Basename qw(basename);
> +use List::Util qw(min);
>  use Time::HiRes qw(gettimeofday tv_interval);
>  binmode STDOUT, ':utf8';
>  
> @@ -122,6 +123,10 @@ our $logo_label = "git homepage";
>  # source of projects list
>  our $projects_list = "++GITWEB_LIST++";
>  
> +# list of additional "directories" under "refs/" we want to display as
> +# branches
> +our @additional_branch_refs = ();
> +
>  # the width (in characters) of the projects list "Description" column
>  our $projects_list_description_width = 25;
>  
> @@ -626,14 +631,29 @@ sub feature_avatar {
>  	return @val ? @val : @_;
>  }
>  
> +sub get_branch_refs {
> +    return ('heads', @additional_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.
>  sub check_head_link {
>  	my ($dir) = @_;
>  	my $headfile = "$dir/HEAD";
> -	return ((-e $headfile) ||
> -		(-l $headfile && readlink($headfile) =~ /^refs\/heads\//));
> +
> +	if (-e $headfile) {
> +		return 1;
> +	}
> +	if (-l $headfile) {
> +		my $rl = readlink($headfile);
> +
> +		for my $ref (get_branch_refs()) {
> +			return 1 if $rl =~ /^refs\/$ref\//;
> +		}
> +	}
> +	return 0;

The change to this function is wrong. A non-detached HEAD that
points at anything other than refs/heads/ should be considered a
repository corruption and should not be encouraged.

> @@ -2515,6 +2535,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);
> @@ -2522,12 +2543,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/$ref/(.*)$!) ||
> +		    (defined $hash      && $hash      =~ m!^refs/$ref/(.*)$!)) {
> +			$branch = $1;
> +			$matched_ref = $ref;
> +			last;
> +		}
>  	}
>  	# find log type for feed description (title)
>  	my $type = 'log';
> @@ -2540,7 +2566,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;

OK.

> @@ -3184,24 +3210,43 @@ sub git_get_project_owner {
>  	return $owner;
>  }
>  
> -sub git_get_last_activity {
> -	my ($path) = @_;
> -	my $fd;
> +sub git_get_last_activity_age {
> +	my ($refs) = @_;
> +	my $fd = -1;
>  
> -	$git_dir = "$projectroot/$path";
>  	open($fd, "-|", git_cmd(), 'for-each-ref',
>  	     '--format=%(committer)',
>  	     '--sort=-committerdate',
>  	     '--count=1',
> -	     'refs/heads') or return;
> +	     $refs) or return undef;
> +
>  	my $most_recent = <$fd>;
> -	close $fd or return;
> +	close $fd or return undef;
>  	if (defined $most_recent &&
>  	    $most_recent =~ / (\d+) [-+][01]\d\d\d$/) {
>  		my $timestamp = $1;
> -		my $age = time - $timestamp;
> -		return ($age, age_string($age));
> +		return time - $timestamp;
>  	}
> +
> +	return undef;
> +}
> +
> +sub git_get_last_activity {
> +	my ($path) = @_;
> +	my @ages = ();
> +
> +	$git_dir = "$projectroot/$path";
> +	for my $ref (get_branch_refs()) {
> +		my $age = git_get_last_activity_age('refs/' . $_);
> +
> +		push @ages, $age if defined $age;
> +	}
> +	if (@ages) {
> +		my $min_age = min(@ages);
> +
> +		return ($min_age, age_string($min_age));
> +	}
> +
>  	return (undef, undef);
>  }

The original runs

	for-each-ref --count=1 refs/heads

and picks the latest one, so shouldn't it be the matter of running

	for-each-ref --count=1 refs/heads refs/extra

i.e. something like

@@ -3193,7 +3193,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 &&

to grab the latest among the refs you care about?

> @@ -3223,7 +3268,7 @@ sub git_get_remotes_list {
>  		next if $wanted and not $remote eq $wanted;
>  		my ($url, $key) = ($1, $2);
>  
> -		$remotes{$remote} ||= { 'heads' => () };
> +		$remotes{$remote} ||= { map { $_ => () } get_branch_refs() };
>  		$remotes{$remote}{$key} = $url;
>  	}
>  	close $fd or return;
> @@ -3237,9 +3282,11 @@ sub fill_remote_heads {
>  	my @heads = map { "remotes/$_" } keys %$remotes;
>  	my @remoteheads = git_get_heads_list(undef, @heads);
>  	foreach my $remote (keys %$remotes) {
> -		$remotes->{$remote}{'heads'} = [ grep {
> -			$_->{'name'} =~ s!^$remote/!!
> -			} @remoteheads ];
> +		foreach my $ref (get_branch_refs()) {
> +			$remotes->{$remote}{$ref} = [ grep {
> +				$_->{'name'} =~ s!^$remote/!!
> +				} @remoteheads ];
> +		}
>  	}
>  }

Hmmm, why?  Aren't these additional ones about the local
"branch-like" refs?  What makes us think that these names are also
shared with the names from remote hierarchies?

> @@ -3644,7 +3691,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;
>  
> @@ -3662,7 +3709,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 '|', get_branch_refs();
> +		$name =~ s!^refs/(?:$strip_refs|remotes)/!!;
>  
>  		$ref_item{'name'}  = $name;
>  		$ref_item{'id'}    = $hash;
> @@ -4286,7 +4334,7 @@ sub git_print_page_nav {
>  # available if the feature is enabled
>  sub format_ref_views {
>  	my ($current) = @_;
> -	my @ref_views = qw{tags heads};
> +	my @ref_views = ('tags', get_branch_refs());
>  	push @ref_views, 'remotes' if gitweb_check_feature('remote_heads');
>  	return join " | ", map {
>  		$_ eq $current ? $_ :
> @@ -7179,7 +7227,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 '|', get_branch_refs();
> +		if ($hash =~ m!^refs/(?:$strip_refs|remotes)/(.*)$!) {
>  			$ver = $1;
>  		}
>  		$ver .= '-' . git_get_short_hash($project, $hash);

As the elements of @additional_branch_refs are expected by code
added by this patch to never have any special metacharacters in
them, it probably is a good idea to sanity check it to catch
misconfigurations and typos before doing anything else.

Other than that, looks good to me.

Thanks.

Re: [PATCH] gitweb: Add an option for adding more branch refs

[Krzesimir Nowak]

On Tue, 2013-11-26 at 13:48 -0800, Junio C Hamano wrote:
> Krzesimir Nowak <krzesimir@endocode.com> writes:
> 
> > Overriding an @additional_branch_refs configuration variable with
> > value ('wip') will make gitweb to show branches that appear in
> > refs/heads and refs/wip (refs/heads is hardcoded). Might be useful for
> > gerrit setups where user branches are not stored under refs/heads/.
> >
> > Signed-off-by: Krzesimir Nowak <krzesimir@endocode.com>
> > ---
> >  gitweb/gitweb.perl | 99 ++++++++++++++++++++++++++++++++++++++++--------------
> >  1 file changed, 74 insertions(+), 25 deletions(-)
> >
> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> > index 68c77f6..9bfd38b 100755
> > --- a/gitweb/gitweb.perl
> > +++ b/gitweb/gitweb.perl
> > @@ -17,6 +17,7 @@ use Encode;
> >  use Fcntl ':mode';
> >  use File::Find qw();
> >  use File::Basename qw(basename);
> > +use List::Util qw(min);
> >  use Time::HiRes qw(gettimeofday tv_interval);
> >  binmode STDOUT, ':utf8';
> >  
> > @@ -122,6 +123,10 @@ our $logo_label = "git homepage";
> >  # source of projects list
> >  our $projects_list = "++GITWEB_LIST++";
> >  
> > +# list of additional "directories" under "refs/" we want to display as
> > +# branches
> > +our @additional_branch_refs = ();
> > +
> >  # the width (in characters) of the projects list "Description" column
> >  our $projects_list_description_width = 25;
> >  
> > @@ -626,14 +631,29 @@ sub feature_avatar {
> >  	return @val ? @val : @_;
> >  }
> >  
> > +sub get_branch_refs {
> > +    return ('heads', @additional_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.
> >  sub check_head_link {
> >  	my ($dir) = @_;
> >  	my $headfile = "$dir/HEAD";
> > -	return ((-e $headfile) ||
> > -		(-l $headfile && readlink($headfile) =~ /^refs\/heads\//));
> > +
> > +	if (-e $headfile) {
> > +		return 1;
> > +	}
> > +	if (-l $headfile) {
> > +		my $rl = readlink($headfile);
> > +
> > +		for my $ref (get_branch_refs()) {
> > +			return 1 if $rl =~ /^refs\/$ref\//;
> > +		}
> > +	}
> > +	return 0;
> 
> The change to this function is wrong. A non-detached HEAD that
> points at anything other than refs/heads/ should be considered a
> repository corruption and should not be encouraged.

Ok, I'll get rid of it.

> 
> > @@ -2515,6 +2535,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);
> > @@ -2522,12 +2543,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/$ref/(.*)$!) ||
> > +		    (defined $hash      && $hash      =~ m!^refs/$ref/(.*)$!)) {
> > +			$branch = $1;
> > +			$matched_ref = $ref;
> > +			last;
> > +		}
> >  	}
> >  	# find log type for feed description (title)
> >  	my $type = 'log';
> > @@ -2540,7 +2566,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;
> 
> OK.
> 
> > @@ -3184,24 +3210,43 @@ sub git_get_project_owner {
> >  	return $owner;
> >  }
> >  
> > -sub git_get_last_activity {
> > -	my ($path) = @_;
> > -	my $fd;
> > +sub git_get_last_activity_age {
> > +	my ($refs) = @_;
> > +	my $fd = -1;
> >  
> > -	$git_dir = "$projectroot/$path";
> >  	open($fd, "-|", git_cmd(), 'for-each-ref',
> >  	     '--format=%(committer)',
> >  	     '--sort=-committerdate',
> >  	     '--count=1',
> > -	     'refs/heads') or return;
> > +	     $refs) or return undef;
> > +
> >  	my $most_recent = <$fd>;
> > -	close $fd or return;
> > +	close $fd or return undef;
> >  	if (defined $most_recent &&
> >  	    $most_recent =~ / (\d+) [-+][01]\d\d\d$/) {
> >  		my $timestamp = $1;
> > -		my $age = time - $timestamp;
> > -		return ($age, age_string($age));
> > +		return time - $timestamp;
> >  	}
> > +
> > +	return undef;
> > +}
> > +
> > +sub git_get_last_activity {
> > +	my ($path) = @_;
> > +	my @ages = ();
> > +
> > +	$git_dir = "$projectroot/$path";
> > +	for my $ref (get_branch_refs()) {
> > +		my $age = git_get_last_activity_age('refs/' . $_);
> > +
> > +		push @ages, $age if defined $age;
> > +	}
> > +	if (@ages) {
> > +		my $min_age = min(@ages);
> > +
> > +		return ($min_age, age_string($min_age));
> > +	}
> > +
> >  	return (undef, undef);
> >  }
> 
> The original runs
> 
> 	for-each-ref --count=1 refs/heads
> 
> and picks the latest one, so shouldn't it be the matter of running
> 
> 	for-each-ref --count=1 refs/heads refs/extra
> 
> i.e. something like
> 
> @@ -3193,7 +3193,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 &&
> 
> to grab the latest among the refs you care about?

That's simpler, yes.

> 
> > @@ -3223,7 +3268,7 @@ sub git_get_remotes_list {
> >  		next if $wanted and not $remote eq $wanted;
> >  		my ($url, $key) = ($1, $2);
> >  
> > -		$remotes{$remote} ||= { 'heads' => () };
> > +		$remotes{$remote} ||= { map { $_ => () } get_branch_refs() };
> >  		$remotes{$remote}{$key} = $url;
> >  	}
> >  	close $fd or return;
> > @@ -3237,9 +3282,11 @@ sub fill_remote_heads {
> >  	my @heads = map { "remotes/$_" } keys %$remotes;
> >  	my @remoteheads = git_get_heads_list(undef, @heads);
> >  	foreach my $remote (keys %$remotes) {
> > -		$remotes->{$remote}{'heads'} = [ grep {
> > -			$_->{'name'} =~ s!^$remote/!!
> > -			} @remoteheads ];
> > +		foreach my $ref (get_branch_refs()) {
> > +			$remotes->{$remote}{$ref} = [ grep {
> > +				$_->{'name'} =~ s!^$remote/!!
> > +				} @remoteheads ];
> > +		}
> >  	}
> >  }
> 
> Hmmm, why?  Aren't these additional ones about the local
> "branch-like" refs?  What makes us think that these names are also
> shared with the names from remote hierarchies?

Might be another place where I had no clue if the change is right.
Sorry.

> 
> > @@ -3644,7 +3691,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;
> >  
> > @@ -3662,7 +3709,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 '|', get_branch_refs();
> > +		$name =~ s!^refs/(?:$strip_refs|remotes)/!!;
> >  
> >  		$ref_item{'name'}  = $name;
> >  		$ref_item{'id'}    = $hash;
> > @@ -4286,7 +4334,7 @@ sub git_print_page_nav {
> >  # available if the feature is enabled
> >  sub format_ref_views {
> >  	my ($current) = @_;
> > -	my @ref_views = qw{tags heads};
> > +	my @ref_views = ('tags', get_branch_refs());

I'll get rid of that change, too. I just verified that it is wrong too.

> >  	push @ref_views, 'remotes' if gitweb_check_feature('remote_heads');
> >  	return join " | ", map {
> >  		$_ eq $current ? $_ :
> > @@ -7179,7 +7227,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 '|', get_branch_refs();
> > +		if ($hash =~ m!^refs/(?:$strip_refs|remotes)/(.*)$!) {
> >  			$ver = $1;
> >  		}
> >  		$ver .= '-' . git_get_short_hash($project, $hash);
> 
> As the elements of @additional_branch_refs are expected by code
> added by this patch to never have any special metacharacters in
> them, it probably is a good idea to sanity check it to catch
> misconfigurations and typos before doing anything else.
> 

I assume that in case of misconfiguration we are returning 500.

I added a sanity checking for duplicate refs, for explicitly specified
'heads' and refs that fail to pass the validate_ref (which I splitted
from validate_refname).

As for metacharacters - valid ref can contain a '|' which is a
metacharacter in regex, so I wrapped some uses of
@additional_branch_refs members in regexes into \Q and \E or used a
quotemeta on them. That escapes metacharacters.

Also, I added some docs to gitweb.conf.txt.

Patch in "[PATCH v2] gitweb: Add an option for adding more branch refs"

> Other than that, looks good to me.
> 
> Thanks.

-- 
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] gitweb: Add an option for adding more branch refs

[Jakub Narębski]

Krzesimir Nowak wrote:

> Overriding an @additional_branch_refs configuration variable with
> value ('wip') will make gitweb to show branches that appear in
> refs/heads and refs/wip (refs/heads is hardcoded). Might be useful for
> gerrit setups where user branches are not stored under refs/heads/.
>

The description of this change starts with technical details,
instead of starting with intent of this change.

Perhaps (this is only a proposal)

   Introduce @additional_branch_refs configuration variable, holding
   names of references to be considered branches; by default empty.
   For example setting it to ('wip') will make gitweb ...


BTW. I have thought at first that is something similar to 'remote_heads'
feature, which among others adds 'remotes' section to 'summary' view
displaying refs/remotes/* refs... but no, gitweb still doesn't treat 
refs/remotes as branches, even with this feature set.

Nb. why new configuration variable, and not new %feature?

> Signed-off-by: Krzesimir Nowak <krzesimir@endocode.com>
> ---
>  gitweb/gitweb.perl | 99 ++++++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 74 insertions(+), 25 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 68c77f6..9bfd38b 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -17,6 +17,7 @@ use Encode;
>  use Fcntl ':mode';
>  use File::Find qw();
>  use File::Basename qw(basename);
> +use List::Util qw(min);
>  use Time::HiRes qw(gettimeofday tv_interval);
>  binmode STDOUT, ':utf8';
>
[...]
 > @@ -3184,24 +3210,43 @@ sub git_get_project_owner {
 >   	return $owner;
 >   }
 >
 > -sub git_get_last_activity {
 > -	my ($path) = @_;
 > -	my $fd;
 > +sub git_get_last_activity_age {
 > +	my ($refs) = @_;
 > +	my $fd = -1;
 >
 > -	$git_dir = "$projectroot/$path";
 >   	open($fd, "-|", git_cmd(), 'for-each-ref',
 >   	     '--format=%(committer)',
 >   	     '--sort=-committerdate',
 >   	     '--count=1',
 > -	     'refs/heads') or return;
 > +	     $refs) or return undef;

git-for-each-ref accepts more than one pattern. Why not simply

    	open($fd, "-|", git_cmd(), 'for-each-ref',
    	     '--format=%(committer)',
    	     '--sort=-committerdate',
    	     '--count=1',
   -	     'refs/heads') or return;
   +	     get_branch_refs()) or return;

Then we won't need List::Util::min.

[...]
 > +sub git_get_last_activity {
 > +	my ($path) = @_;
 > +	my @ages = ();
 > +
 > +	$git_dir = "$projectroot/$path";
 > +	for my $ref (get_branch_refs()) {
 > +		my $age = git_get_last_activity_age('refs/' . $_);
 > +
 > +		push @ages, $age if defined $age;
 > +	}
 > +	if (@ages) {
 > +		my $min_age = min(@ages);
 > +
 > +		return ($min_age, age_string($min_age));
 > +	}
 > +
 >   	return (undef, undef);
 >   }
 >
[...]
-- 
Jakub Narębski

Re: [PATCH] gitweb: Add an option for adding more branch refs

[Krzesimir Nowak]

On Wed, 2013-11-27 at 16:56 +0100, Jakub Narębski wrote:
> Krzesimir Nowak wrote:
> 
> > Overriding an @additional_branch_refs configuration variable with
> > value ('wip') will make gitweb to show branches that appear in
> > refs/heads and refs/wip (refs/heads is hardcoded). Might be useful for
> > gerrit setups where user branches are not stored under refs/heads/.
> >
> 
> The description of this change starts with technical details,
> instead of starting with intent of this change.
> 
> Perhaps (this is only a proposal)
> 
>    Introduce @additional_branch_refs configuration variable, holding
>    names of references to be considered branches; by default empty.
>    For example setting it to ('wip') will make gitweb ...
> 

I have already posted second version of the patch. But I didn't change
the commit message though. But thanks for proposal - it sounds better.
I'll try to make it better next time I post a patch.

> 
> BTW. I have thought at first that is something similar to 'remote_heads'
> feature, which among others adds 'remotes' section to 'summary' view
> displaying refs/remotes/* refs... but no, gitweb still doesn't treat 
> refs/remotes as branches, even with this feature set.
> 
> Nb. why new configuration variable, and not new %feature?

I dunno. Hard to tell where it fits. Junio told me about using "normal
gitweb configuration mechanism", so that's the first thing that got my
attention.

http://www.mail-archive.com/git@vger.kernel.org/msg39859.html

> 
> > Signed-off-by: Krzesimir Nowak <krzesimir@endocode.com>
> > ---
> >  gitweb/gitweb.perl | 99 ++++++++++++++++++++++++++++++++++++++++--------------
> >  1 file changed, 74 insertions(+), 25 deletions(-)
> >
> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> > index 68c77f6..9bfd38b 100755
> > --- a/gitweb/gitweb.perl
> > +++ b/gitweb/gitweb.perl
> > @@ -17,6 +17,7 @@ use Encode;
> >  use Fcntl ':mode';
> >  use File::Find qw();
> >  use File::Basename qw(basename);
> > +use List::Util qw(min);
> >  use Time::HiRes qw(gettimeofday tv_interval);
> >  binmode STDOUT, ':utf8';
> >
> [...]
>  > @@ -3184,24 +3210,43 @@ sub git_get_project_owner {
>  >   	return $owner;
>  >   }
>  >
>  > -sub git_get_last_activity {
>  > -	my ($path) = @_;
>  > -	my $fd;
>  > +sub git_get_last_activity_age {
>  > +	my ($refs) = @_;
>  > +	my $fd = -1;
>  >
>  > -	$git_dir = "$projectroot/$path";
>  >   	open($fd, "-|", git_cmd(), 'for-each-ref',
>  >   	     '--format=%(committer)',
>  >   	     '--sort=-committerdate',
>  >   	     '--count=1',
>  > -	     'refs/heads') or return;
>  > +	     $refs) or return undef;
> 
> git-for-each-ref accepts more than one pattern. Why not simply
> 
>     	open($fd, "-|", git_cmd(), 'for-each-ref',
>     	     '--format=%(committer)',
>     	     '--sort=-committerdate',
>     	     '--count=1',
>    -	     'refs/heads') or return;
>    +	     get_branch_refs()) or return;
> 
> Then we won't need List::Util::min.

Yes, Junio pointed that out to me - fixed in second version of the
patch.

> 
> [...]
>  > +sub git_get_last_activity {
>  > +	my ($path) = @_;
>  > +	my @ages = ();
>  > +
>  > +	$git_dir = "$projectroot/$path";
>  > +	for my $ref (get_branch_refs()) {
>  > +		my $age = git_get_last_activity_age('refs/' . $_);
>  > +
>  > +		push @ages, $age if defined $age;
>  > +	}
>  > +	if (@ages) {
>  > +		my $min_age = min(@ages);
>  > +
>  > +		return ($min_age, age_string($min_age));
>  > +	}
>  > +
>  >   	return (undef, undef);
>  >   }
>  >
> [...]

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