threads / patch / 35411

v2gitweb: Add an option for adding more branch refs

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

## tl;dr

5 messages between Nov 27, 2013 and Nov 28, 2013. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

Krzesimir Nowak· Nov 27, 2013, 15:30 UTC · lore

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>
---
 Documentation/gitweb.conf.txt | 14 ++++++++
 gitweb/gitweb.perl            | 75 +++++++++++++++++++++++++++++++++----------
 2 files changed, 72 insertions(+), 17 deletions(-)
Show changes to 2 files +72 −17

Documentation/gitweb.conf.txt, gitweb/gitweb.perl

diff --git a/Documentation/gitweb.conf.txt b/Documentation/gitweb.conf.txt
index e2113d9..74de87a 100644
--- a/Documentation/gitweb.conf.txt
+++ b/Documentation/gitweb.conf.txt
@@ -549,6 +549,20 @@ This variable matters only when using persistent web environments that
 serve multiple requests using single gitweb instance, like mod_perl,
 FastCGI or Plackup.
 
+@additional_branch_refs::
+	List of additional directories under "refs" which are going to be used
+	as branch refs. You might want to set this variable 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, for example
++
+--------------------------------------------------------------------------------
+our @additional_branch_refs = ('sandbox', 'wip', 'other');
+--------------------------------------------------------------------------------
++
+It is an error to specify 'heads' in the list - it is implicitly added. It is an
+error to specify a ref that does not pass "git check-ref-format" scrutiny. It is
+an error to specify a ref twice in the list.
 
 Other variables
 ~~~~~~~~~~~~~~~
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 68c77f6..499281b 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -122,6 +122,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,6 +630,10 @@ 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.
@@ -680,6 +688,19 @@ sub read_config_file {
 	return;
 }
 
+# performs sanity checks on parts of configuration.
+sub config_sanity_check {
+	# check additional refs validity
+	my %unique_branch_refs = ();
+	for my $ref (@additional_branch_refs) {
+		die_error(500, 'Invalid ref in @additional_branch_refs') unless (validate_ref($ref));
+		die_error(500, '"heads" specified in @additional_branch_refs') if ($ref eq 'heads');
+		die_error(500, "Ref '$ref' repeated in \@additional_branch_refs") if (exists $unique_branch_refs{$ref});
+		$unique_branch_refs{$ref} = 1;
+	}
+	%unique_branch_refs = undef;
+}
+
 our ($GITWEB_CONFIG, $GITWEB_CONFIG_SYSTEM, $GITWEB_CONFIG_COMMON);
 sub evaluate_gitweb_config {
 	our $GITWEB_CONFIG = $ENV{'GITWEB_CONFIG'} || "++GITWEB_CONFIG++";
@@ -698,8 +719,11 @@ sub evaluate_gitweb_config {
 
 	# Use first config file that exists.  This means use the per-instance
 	# GITWEB_CONFIG if exists, otherwise use GITWEB_SYSTEM_CONFIG.
-	read_config_file($GITWEB_CONFIG) and return;
-	read_config_file($GITWEB_CONFIG_SYSTEM);
+	if (!read_config_file($GITWEB_CONFIG)) {
+		read_config_file($GITWEB_CONFIG_SYSTEM);
+	}
+
+	config_sanity_check();
 }
 
 # Get loadavg of system, to compare against $maxload.
@@ -1452,6 +1476,16 @@ sub validate_pathname {
 	return $input;
 }
 
+sub validate_ref {
+	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 +1496,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 = validate_ref($input)
+		or return undef;
 	return $input;
 }
 
@@ -2515,6 +2548,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 +2556,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';
@@ -2540,7 +2579,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;
@@ -3193,7 +3232,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 &&
@@ -3644,7 +3683,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 +3701,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;
@@ -7179,7 +7219,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
Eric Sunshine· Nov 27, 2013, 20:34 UTC · re: Krzesimir Nowak · lore

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

On Wed, Nov 27, 2013 at 10:30 AM, Krzesimir Nowak <krzesimir@endocode.com> wrote:

Show 21 quoted lines
> 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>
> ---
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 68c77f6..499281b 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -680,6 +688,19 @@ sub read_config_file {
>         return;
>  }
>
> +# performs sanity checks on parts of configuration.
> +sub config_sanity_check {
> +       # check additional refs validity
> +       my %unique_branch_refs = ();
> +       for my $ref (@additional_branch_refs) {
> +               die_error(500, 'Invalid ref in @additional_branch_refs') unless (validate_ref($ref));

Mentioning $ref in the error message would help the user resolve the problem more quickly.

> +               die_error(500, '"heads" specified in @additional_branch_refs') if ($ref eq 'heads');
Rephrasing this as
    "heads" disallowed in @additional_branch_refs

would better explain the problem to a user who has only made a cursory read of the documentation.

Show 9 quoted lines
> +               die_error(500, "Ref '$ref' repeated in \@additional_branch_refs") if (exists $unique_branch_refs{$ref});
> +               $unique_branch_refs{$ref} = 1;
> +       }
> +       %unique_branch_refs = undef;
> +}
> +
>  our ($GITWEB_CONFIG, $GITWEB_CONFIG_SYSTEM, $GITWEB_CONFIG_COMMON);
>  sub evaluate_gitweb_config {
>         our $GITWEB_CONFIG = $ENV{'GITWEB_CONFIG'} || "++GITWEB_CONFIG++";
Eric Sunshine· Nov 27, 2013, 20:38 UTC · re: Eric Sunshine · lore

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

On Wed, Nov 27, 2013 at 3:34 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 35 quoted lines
> On Wed, Nov 27, 2013 at 10:30 AM, Krzesimir Nowak
> <krzesimir@endocode.com> 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/.
>>
>> Signed-off-by: Krzesimir Nowak <krzesimir@endocode.com>
>> ---
>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
>> index 68c77f6..499281b 100755
>> --- a/gitweb/gitweb.perl
>> +++ b/gitweb/gitweb.perl
>> @@ -680,6 +688,19 @@ sub read_config_file {
>>         return;
>>  }
>>
>> +# performs sanity checks on parts of configuration.
>> +sub config_sanity_check {
>> +       # check additional refs validity
>> +       my %unique_branch_refs = ();
>> +       for my $ref (@additional_branch_refs) {
>> +               die_error(500, 'Invalid ref in @additional_branch_refs') unless (validate_ref($ref));
>
> Mentioning $ref in the error message would help the user resolve the
> problem more quickly.
>
>> +               die_error(500, '"heads" specified in @additional_branch_refs') if ($ref eq 'heads');
>
> Rephrasing this as
>
>     "heads" disallowed in @additional_branch_refs
>
> would better explain the problem to a user who has only made a cursory
> read of the documentation.

The program could easily filter out the redundant 'heads', so does this really deserve a diagnostic?

Junio C Hamano· Nov 27, 2013, 20:55 UTC · re: Eric Sunshine · lore

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

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 6 quoted lines
> On Wed, Nov 27, 2013 at 3:34 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Wed, Nov 27, 2013 at 10:30 AM, Krzesimir Nowak
>> <krzesimir@endocode.com> 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).
"branches" are by definition what are in refs/heads/ hierarchy, so 
  Allow @additional_branch_refs configuration variable to tell
  gitweb to show refs from additional hierarchies in addition to
  branches in the list-of-branches view.
would be more appropriate and sufficient.
Show 14 quoted lines
>> Mentioning $ref in the error message would help the user resolve the
>> problem more quickly.
>>
>>> +               die_error(500, '"heads" specified in @additional_branch_refs') if ($ref eq 'heads');
>>
>> Rephrasing this as
>>
>>     "heads" disallowed in @additional_branch_refs
>>
>> would better explain the problem to a user who has only made a cursory
>> read of the documentation.
>
> The program could easily filter out the redundant 'heads', so does
> this really deserve a diagnostic?
True.

I was primarily worried about metacharacters in the specified strings getting in the way of regexp matches the new code allows on them, but that has been resolved with the use of \Q..\E; if that automatic deduping is done, I do not immediately see any remaining issues in the latest round of the patch.

Thanks.
Krzesimir Nowak· Nov 28, 2013, 11:49 UTC · re: Junio C Hamano · lore

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

On Wed, 2013-11-27 at 12:55 -0800, Junio C Hamano wrote:
Show 16 quoted lines
> Eric Sunshine <sunshine@sunshineco.com> writes:
> 
> > On Wed, Nov 27, 2013 at 3:34 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> >> On Wed, Nov 27, 2013 at 10:30 AM, Krzesimir Nowak
> >> <krzesimir@endocode.com> 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).
> 
> "branches" are by definition what are in refs/heads/ hierarchy, so 
> 
>   Allow @additional_branch_refs configuration variable to tell
>   gitweb to show refs from additional hierarchies in addition to
>   branches in the list-of-branches view.
> 
> would be more appropriate and sufficient.
Thanks.
Show 17 quoted lines
> 
> >> Mentioning $ref in the error message would help the user resolve the
> >> problem more quickly.
> >>
> >>> +               die_error(500, '"heads" specified in @additional_branch_refs') if ($ref eq 'heads');
> >>
> >> Rephrasing this as
> >>
> >>     "heads" disallowed in @additional_branch_refs
> >>
> >> would better explain the problem to a user who has only made a cursory
> >> read of the documentation.
> >
> > The program could easily filter out the redundant 'heads', so does
> > this really deserve a diagnostic?
> 
> True.

Ok, I'm deduping both heads and other refs as well. Now we send 500 only if the ref is simply invalid.

Show 7 quoted lines
> 
> I was primarily worried about metacharacters in the specified
> strings getting in the way of regexp matches the new code allows on
> them, but that has been resolved with the use of \Q..\E; if that
> automatic deduping is done, I do not immediately see any remaining
> issues in the latest round of the patch.
> 

New patch in "[PATCH v3] gitweb: Add an option for adding more branch refs". Thanks for reviews!

> 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

← back to recent threads