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

2013-12-04 Thread Junio C Hamano 
Krzesimir Nowak krzesi...@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 gits...@pobox.com wrote:
  Krzesimir Nowak krzesi...@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.

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-12-03 Thread 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 krzesi...@endocode.com
Reviewed-by: Junio C Hamano gits...@pobox.com
Reviewed-by: Jakub Narębski jna...@gmail.com
Reviewed-by: Eric Sunshine sunsh...@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

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

2013-12-03 Thread Junio C Hamano 
Krzesimir Nowak krzesi...@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 krzesi...@endocode.com
 Reviewed-by: Junio C Hamano gits...@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 jna...@gmail.com
 Reviewed-by: Eric Sunshine sunsh...@sunshineco.com
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-12-03 Thread Junio C Hamano 
Krzesimir Nowak krzesi...@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;
 +}
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-12-03 Thread Jakub Narębski 
On Tue, Dec 3, 2013 at 9:15 PM, Junio C Hamano gits...@pobox.com wrote:
 Krzesimir Nowak krzesi...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html