threads / patch / 35464

patch, 3 partsgitweb: Move check-ref-format code into separate function

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

## tl;dr

14 messages between Dec 3, 2013 and Dec 4, 2013. Diffs are folded; open one to read it.

replies: 13people: 3as markdown or json

Krzesimir Nowak· Dec 3, 2013, 14:56 UTC · lore

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

First patch just splits some code to a function, second patch adds the extra-branch-refs feature and third one adds some visual differentation of branches from non-standard ref directories.

Krzesimir Nowak (3):
  gitweb: Move check-ref-format code into separate function
  gitweb: Add a feature for adding more branch refs
  gitweb: Denote non-heads, non-remotes branches.
 Documentation/gitweb.conf.txt |  27 ++++++++++
 gitweb/gitweb.perl            | 120 +++++++++++++++++++++++++++++++++++-------
 2 files changed, 129 insertions(+), 18 deletions(-)
-- 
1.8.3.1
Krzesimir Nowak· Dec 3, 2013, 14:56 UTC · re: Krzesimir Nowak · lore
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(-)
Show changes to gitweb/gitweb.perl +13 −4
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
Junio C Hamano· Dec 3, 2013, 19:02 UTC · re: Krzesimir Nowak · lore

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

Krzesimir Nowak <krzesimir@endocode.com> writes:
Show 39 quoted lines
> 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.

>  }
Jakub Narębski· Dec 3, 2013, 19:38 UTC · re: Junio C Hamano · lore

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

On Tue, Dec 3, 2013 at 8:02 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Krzesimir Nowak <krzesimir@endocode.com> writes:
Show 9 quoted lines
>> +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;
>> +}
[...]
Show 21 quoted lines
>> @@ -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
Junio C Hamano· Dec 3, 2013, 19:48 UTC · re: Jakub Narębski · lore

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

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.
Krzesimir Nowak· Dec 4, 2013, 13:06 UTC · re: Jakub Narębski · lore

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

On Tue, 2013-12-03 at 20:38 +0100, Jakub Narębski wrote:
Show 38 quoted lines
> 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.

Show 5 quoted lines
> Stricly speaking pure refactoring (no functional change, e.g. no assign
> to $input) would be  "check_ref_format($input) or return undef;", or even
> "return check_ref_format($input);" if we keep check_ref_format() passthru
> on valid refname.
> 
I fixed that too.
-- 
Krzesimir Nowak
Software Developer
Endocode AG

krzesimir@endocode.com

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

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

Registergericht: Amtsgericht Charlottenburg - HRB 150748 B
Krzesimir Nowak· Dec 3, 2013, 14:56 UTC · re: Krzesimir Nowak · lore

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

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(-)
Show changes to 2 files +91 −12

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

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
Junio C Hamano· Dec 3, 2013, 18:51 UTC · re: Krzesimir Nowak · lore

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

Krzesimir Nowak <krzesimir@endocode.com> writes:
Show 6 quoted lines
> 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>
Junio C Hamano· Dec 3, 2013, 20:15 UTC · re: Krzesimir Nowak · lore

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

Krzesimir Nowak <krzesimir@endocode.com> writes:
Show 7 quoted lines
> @@ -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).
Show 6 quoted lines
> +	if ($values) {
> +		@branch_refs = split /\s+/, $values;
> +	}
> +
> +	return @branch_refs;
> +}
Jakub Narębski· Dec 3, 2013, 20:38 UTC · re: Junio C Hamano · lore

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

On Tue, Dec 3, 2013 at 9:15 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 20 quoted lines
> 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.
Show 8 quoted lines
> * 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).

Show 6 quoted lines
>> +     if ($values) {
>> +             @branch_refs = split /\s+/, $values;
>> +     }
>> +
>> +     return @branch_refs;
>> +}
-- 
Jakub Narebski
Krzesimir Nowak· Dec 4, 2013, 12:49 UTC · re: Jakub Narębski · lore

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

On Tue, 2013-12-03 at 21:38 +0100, Jakub Narębski wrote:
Show 28 quoted lines
> 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.

Show 13 quoted lines
> 
> > * 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.

Show 11 quoted lines
> 
> > * 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.

Show 10 quoted lines
> 
> >> +     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
Junio C Hamano· Dec 4, 2013, 17:57 UTC · re: Krzesimir Nowak · lore

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

Krzesimir Nowak <krzesimir@endocode.com> writes:
Show 32 quoted lines
> 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.

Show 17 quoted lines
>> > * 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.

Krzesimir Nowak· Dec 3, 2013, 14:56 UTC · re: Krzesimir Nowak · lore

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

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(-)
Show changes to gitweb/gitweb.perl +25 −2
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
Krzesimir Nowak· Dec 4, 2013, 13:44 UTC · re: Krzesimir Nowak · lore

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

On Tue, 2013-12-03 at 15:56 +0100, Krzesimir Nowak wrote:
Show 13 quoted lines
> 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

← back to recent threads