Re: [PATCH v3] gitweb: Add an option for adding more branch refs
On Mon, 2013-12-02 at 18:34 +0100, Jakub Narębski wrote:
W dniu 2013-12-02 13:06, Krzesimir Nowak pisze:
On Mon, 2013-12-02 at 01:21 +0100, Jakub Narębski wrote:
On Thu, Nov 28, 2013 at 12:44 PM, Krzesimir Nowak
krzesi...@endocode.com wrote:
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.
Signed-off-by: Krzesimir Nowakkrzesi...@endocode.com
Why not use %feature hash instead of adding new configuration variable?
I think that this option is similar enough to 'remote_heads' feature
(which BTW should be 'remote-heads'), and could conceivably enabled
on a per-repository basis, i.e. with repository configuration override,
isn't it?
I'd like to see some consensus on it before I start changing the patch
again.
%feature hash is mainly (but not only) about options that can be
configured on per-repository basis. Configuration variables are
about options that are per-instance (per gitweb).
Well, I am mostly interested in per-instance configuration in this case,
but if that is also possible with %feature hash, then ok, I'll try to
make it work.
From what I've seen (correct me please if I got it wrong) feature
settings is taken from per-repository config file from [gitweb] section.
If there's nothing then some default value is taken. That default value
can be overriden with per-instance perl config file.
So it is easy to override it from per-instance perl config by typing:
$feature{'additional-branch-refs'}{'default'} = ['wip', 'no|tfun,ny'];
$feature{'additional-branch-refs'}{'override'} = 1;
(Note the edge case of refs/no|tfun,ny, which passes the git
check-ref-format scrutiny.)
But for now, most of features are quite simple - either booleans,
integers or list of simple strings (in snapshot feature). What I need
here is a list of strings, like CSV in following example:
[gitweb]
additional_branch_refs = wip,no|tfun,ny
Is dependency on external module like Text::CSV or Text::CSV_XS ok? If
not, I can hack some CSV reading code.
Usually %feature hash is preferred over adding new configuration variable
but this is not some hard rule. Note however that patches adding new config
are met with more scrutiny, as it is harder to fix mistakes because of
requirement of backwards compatibility of configuration files.
I don't know what kind of backwards compatibility you mention. Whether
you want gitweb to survive reading old config file or to honor
deprecated/old config variables.
I meant here honoring deprecated/old variables, i.e. honoring existing
configuration files. See for example backward compatibility for old
$stylesheet variable vs new @stylesheets in print_header_links().
Though in this case it shouldn't be much of a problem; it would be
easy to honor @additional_branch_refs by setting 'default' for
'extra-branch-refs' feature to it.
extra-branch-refs is nicer than additional-branch-refs, I'll use it.
BTW. there really should be gitweb/CodingGuidelines...
Yes, would be useful. As in every other project. :)
Well, Git itself *has* Documentation/CodingGuidelines, but perhaps
gitweb subsystem should have it's own...
[...]
@@ -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);
One one hand, it is about threating extra branch refs the same way as
'head'.
On the other hand we loose distinction between 'refs/heads/foo' and e.g.
'refs/wip/foo'. But maybe that's all right...
In git_get_heads_list sub I could append a ($ref_dir) to refs which
are in neither 'heads' nor 'remotes', so heads view would look like:
master
old-stable
some-work-in-progress (wip)
some-other-branch (other)
where both master and old-stable are in refs/heads/,
some-work-in-progress in refs/wip/ and some-other-branch in refs/other/.
In case of branch snapshot names (snapshot_name sub) I could change it,
so names for branches mentioned above
Re: [PATCH v3] gitweb: Add an option for adding more branch refs
On Tue, Dec 3, 2013 at 11:53 AM, Krzesimir Nowak krzesi...@endocode.com wrote:
On Mon, 2013-12-02 at 18:34 +0100, Jakub Narębski wrote:
W dniu 2013-12-02 13:06, Krzesimir Nowak pisze:
On Mon, 2013-12-02 at 01:21 +0100, Jakub Narębski wrote:
On Thu, Nov 28, 2013 at 12:44 PM, Krzesimir Nowak
krzesi...@endocode.com wrote:
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.
Signed-off-by: Krzesimir Nowakkrzesi...@endocode.com
Why not use %feature hash instead of adding new configuration variable?
I think that this option is similar enough to 'remote_heads' feature
(which BTW should be 'remote-heads'), and could conceivably enabled
on a per-repository basis, i.e. with repository configuration override,
isn't it?
I'd like to see some consensus on it before I start changing the patch
again.
%feature hash is mainly (but not only) about options that can be
configured on per-repository basis. Configuration variables are
about options that are per-instance (per gitweb).
Well, I am mostly interested in per-instance configuration in this case,
but if that is also possible with %feature hash, then ok, I'll try to
make it work.
Yes, it is possible to have per-instance configuration (you can even
forbid per-repository configuration).
From what I've seen (correct me please if I got it wrong) feature
settings is taken from per-repository config file from [gitweb] section.
If there's nothing then some default value is taken. That default value
can be overriden with per-instance perl config file.
%feature settings are taken from gitweb configuration (the 'default'
key), and if given feature is overrideable and per-repository configuration
in the form of appropriate key in [gitweb] section of repository config
file exists, it is used instead.
So it is easy to override it from per-instance perl config by typing:
$feature{'additional-branch-refs'}{'default'} = ['wip', 'no|tfun,ny'];
$feature{'additional-branch-refs'}{'override'} = 1;
Yes. The 'override' is about checking (which imposes a bit of
performance penalty) and respecting per-repository configuration.
(Note the edge case of refs/no|tfun,ny, which passes the git
check-ref-format scrutiny.)
But for now, most of features are quite simple - either booleans,
integers or list of simple strings (in snapshot feature). What I need
here is a list of strings, like CSV in following example:
[gitweb]
additional_branch_refs = wip,no|tfun,ny
Is dependency on external module like Text::CSV or Text::CSV_XS ok? If
not, I can hack some CSV reading code.
Why not use space, which is forbidden in refnames, to separate
entries? Similar to feature_snapshot(), which is about comma separated
list, without escaping.
--
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
Re: [PATCH v3] gitweb: Add an option for adding more branch refs
On Tue, 2013-12-03 at 14:02 +0100, Jakub Narębski wrote:
On Tue, Dec 3, 2013 at 11:53 AM, Krzesimir Nowak krzesi...@endocode.com
wrote:
On Mon, 2013-12-02 at 18:34 +0100, Jakub Narębski wrote:
W dniu 2013-12-02 13:06, Krzesimir Nowak pisze:
On Mon, 2013-12-02 at 01:21 +0100, Jakub Narębski wrote:
On Thu, Nov 28, 2013 at 12:44 PM, Krzesimir Nowak
krzesi...@endocode.com wrote:
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.
Signed-off-by: Krzesimir Nowakkrzesi...@endocode.com
Why not use %feature hash instead of adding new configuration variable?
I think that this option is similar enough to 'remote_heads' feature
(which BTW should be 'remote-heads'), and could conceivably enabled
on a per-repository basis, i.e. with repository configuration override,
isn't it?
I'd like to see some consensus on it before I start changing the patch
again.
%feature hash is mainly (but not only) about options that can be
configured on per-repository basis. Configuration variables are
about options that are per-instance (per gitweb).
Well, I am mostly interested in per-instance configuration in this case,
but if that is also possible with %feature hash, then ok, I'll try to
make it work.
Yes, it is possible to have per-instance configuration (you can even
forbid per-repository configuration).
From what I've seen (correct me please if I got it wrong) feature
settings is taken from per-repository config file from [gitweb] section.
If there's nothing then some default value is taken. That default value
can be overriden with per-instance perl config file.
%feature settings are taken from gitweb configuration (the 'default'
key), and if given feature is overrideable and per-repository configuration
in the form of appropriate key in [gitweb] section of repository config
file exists, it is used instead.
So it is easy to override it from per-instance perl config by typing:
$feature{'additional-branch-refs'}{'default'} = ['wip', 'no|tfun,ny'];
$feature{'additional-branch-refs'}{'override'} = 1;
Yes. The 'override' is about checking (which imposes a bit of
performance penalty) and respecting per-repository configuration.
(Note the edge case of refs/no|tfun,ny, which passes the git
check-ref-format scrutiny.)
But for now, most of features are quite simple - either booleans,
integers or list of simple strings (in snapshot feature). What I need
here is a list of strings, like CSV in following example:
[gitweb]
additional_branch_refs = wip,no|tfun,ny
Is dependency on external module like Text::CSV or Text::CSV_XS ok? If
not, I can hack some CSV reading code.
Why not use space, which is forbidden in refnames, to separate
entries? Similar to feature_snapshot(), which is about comma separated
list, without escaping.
Thanks for explanations and a hint. Following patches are in Show extra
branch refs in gitweb.
Cheers,
--
Krzesimir Nowak
Software Developer
Endocode AG
krzesi...@endocode.com
--
Endocode AG, Johannisstraße 20, 10117 Berlin
i...@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
--
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 v3] gitweb: Add an option for adding more branch refs
On Thu, 2013-11-28 at 20:13 -0500, Eric Sunshine wrote:
On Thu, Nov 28, 2013 at 6:44 AM, Krzesimir Nowak krzesi...@endocode.com
wrote:
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.
Signed-off-by: Krzesimir Nowak krzesi...@endocode.com
---
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 68c77f6..25e1d37 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 '$ref' in
\@additional_branch_refs) unless (validate_ref($ref));
+ # 'heads' are added implicitly in get_branch_refs().
+ $unique_branch_refs{$ref} = 1 if ($ref ne 'heads');
+ }
+ @additional_branch_refs = sort keys %unique_branch_refs;
+ %unique_branch_refs = undef;
+}
%unique_branch_refs is going out of scope here, so clearing it seems
unnecessary.
I am cleaning it in case when more sanity checking code gets added. So
there is no need to keep the data further.
Moreover, with warnings enabled, perl should be complaining about an
Odd number of elements in hash assignment. (Normally, you would
clear a hash with '%foo=()' or 'undef %foo'.)
Gah, ok. I'll fix it. Thanks.
+
our ($GITWEB_CONFIG, $GITWEB_CONFIG_SYSTEM, $GITWEB_CONFIG_COMMON);
sub evaluate_gitweb_config {
our $GITWEB_CONFIG = $ENV{'GITWEB_CONFIG'} || ++GITWEB_CONFIG++;
--
Krzesimir Nowak
Software Developer
Endocode AG
krzesi...@endocode.com
--
Endocode AG, Johannisstraße 20, 10117 Berlin
i...@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
--
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 v3] gitweb: Add an option for adding more branch refs
On Mon, 2013-12-02 at 01:21 +0100, Jakub Narębski wrote:
On Thu, Nov 28, 2013 at 12:44 PM, Krzesimir Nowak
krzesi...@endocode.com wrote:
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.
Signed-off-by: Krzesimir Nowak krzesi...@endocode.com
Why not use %feature hash instead of adding new configuration variable?
I think that this option is similar enough to 'remote_heads' feature
(which BTW should be 'remote-heads'), and could conceveilably enabled
on a per-repository basis, i.e. with repository configuration override,
isn't it?
I'd like to see some consensus on it before I start changing the patch
again.
Usually %feature hash is preferred over adding new configuration variable
but this is not some hard rule. Note however that patches adding new config
are met with more scrutiny, as it is harder to fix mistakes because of
requirement of backwards compatibility of configuration files.
I don't know what kind of backwards compatibility you mention. Whether
you want gitweb to survive reading old config file or to honor
deprecated/old config variables. If the former than I have already read
somewhere that you always should use config vars like:
our $config = 'value';
Note the 'our' which avoids gitweb failures in case of config variable
removal.
BTW. there really should be gitweb/CodingGuidelines...
Yes, would be useful. As in every other project. :)
---
Documentation/gitweb.conf.txt | 13
gitweb/gitweb.perl| 75
+--
2 files changed, 71 insertions(+), 17 deletions(-)
diff --git a/Documentation/gitweb.conf.txt b/Documentation/gitweb.conf.txt
index e2113d9..cd1a945 100644
--- a/Documentation/gitweb.conf.txt
+++ b/Documentation/gitweb.conf.txt
@@ -549,6 +549,19 @@ 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');
+
I think the last (long) sentence would better read if it began with For
example
if you have... then you could set this variable to ..., IMVHO.
Right, thanks. Will rephrase it.
BTW. if we decide on using %feature hash instead, it would be in the
CONFIGURING GITWEB FEATURES section.
Yes, but I'll wait for some consensus with it.
++
+It is an error to specify a ref that does not pass git check-ref-format
+scrutiny.
Hmmm... One one hand erroring out on invalid refs means that we can
find error in config earlier and easier, on the other hand ignoring invalid
refs would make it resilent to errors in gitweb config (and repository config,
if we use %feature with per-repository override).
We could ignore bad values, but that would make it harder to find out
what exactly is wrong when something we configured to be shown is not
shown at all.
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
[...]
@@ -626,6 +630,10 @@ sub feature_avatar {
return @val ? @val : @_;
}
+sub get_branch_refs {
+return ('heads', @additional_branch_refs);
+}
Nice way of ensuring that list of all branches starts with heads.
# 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 '$ref' in
\@additional_branch_refs) unless (validate_ref($ref));
+ # 'heads' are added implicitly in get_branch_refs().
+ $unique_branch_refs{$ref} = 1 if ($ref ne 'heads');
+ }
+ @additional_branch_refs = sort keys %unique_branch_refs;
+ %unique_branch_refs = undef;
+}
This subroutine is quite similar to filter_snapshot_fmts for 'snapshot'
feature, perhaps the name should be patterned after it, i.e.
filter_branch_refs() or something...
If there were generic config_sanity_check(), it would call
Re: [PATCH v3] gitweb: Add an option for adding more branch refs
W dniu 2013-12-02 13:06, Krzesimir Nowak pisze:
On Mon, 2013-12-02 at 01:21 +0100, Jakub Narębski wrote:
On Thu, Nov 28, 2013 at 12:44 PM, Krzesimir Nowak
krzesi...@endocode.com wrote:
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.
Signed-off-by: Krzesimir Nowakkrzesi...@endocode.com
Why not use %feature hash instead of adding new configuration variable?
I think that this option is similar enough to 'remote_heads' feature
(which BTW should be 'remote-heads'), and could conceivably enabled
on a per-repository basis, i.e. with repository configuration override,
isn't it?
I'd like to see some consensus on it before I start changing the patch
again.
%feature hash is mainly (but not only) about options that can be
configured on per-repository basis. Configuration variables are
about options that are per-instance (per gitweb).
Usually %feature hash is preferred over adding new configuration variable
but this is not some hard rule. Note however that patches adding new config
are met with more scrutiny, as it is harder to fix mistakes because of
requirement of backwards compatibility of configuration files.
I don't know what kind of backwards compatibility you mention. Whether
you want gitweb to survive reading old config file or to honor
deprecated/old config variables.
I meant here honoring deprecated/old variables, i.e. honoring existing
configuration files. See for example backward compatibility for old
$stylesheet variable vs new @stylesheets in print_header_links().
Though in this case it shouldn't be much of a problem; it would be
easy to honor @additional_branch_refs by setting 'default' for
'extra-branch-refs' feature to it.
BTW. there really should be gitweb/CodingGuidelines...
Yes, would be useful. As in every other project. :)
Well, Git itself *has* Documentation/CodingGuidelines, but perhaps
gitweb subsystem should have it's own...
[...]
@@ -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);
One one hand, it is about threating extra branch refs the same way as 'head'.
On the other hand we loose distinction between 'refs/heads/foo' and e.g.
'refs/wip/foo'. But maybe that's all right...
In git_get_heads_list sub I could append a ($ref_dir) to refs which
are in neither 'heads' nor 'remotes', so heads view would look like:
master
old-stable
some-work-in-progress (wip)
some-other-branch (other)
where both master and old-stable are in refs/heads/,
some-work-in-progress in refs/wip/ and some-other-branch in refs/other/.
In case of branch snapshot names (snapshot_name sub) I could change it,
so names for branches mentioned above would be
Project-master-short-hash.tgz,
Project-old_stable-short-hash.tgz,
Project-wip-some-work-in-progress-short-hash.tgz
Project-other-some-other-branch-short-hash.tgz
What do you think?
That is, I think, a very good idea. Though perhaps it would be more
readable to add this extra feature as a separate patch, on top of main one.
--
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
Re: [PATCH v3] gitweb: Add an option for adding more branch refs
Krzesimir Nowak krzesi...@endocode.com writes: On Mon, 2013-12-02 at 01:21 +0100, Jakub Narębski wrote: On Thu, Nov 28, 2013 at 12:44 PM, Krzesimir Nowak krzesi...@endocode.com wrote: 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. Signed-off-by: Krzesimir Nowak krzesi...@endocode.com Why not use %feature hash instead of adding new configuration variable? I think that this option is similar enough to 'remote_heads' feature (which BTW should be 'remote-heads'), and could conceveilably enabled on a per-repository basis, i.e. with repository configuration override, isn't it? I'd like to see some consensus on it before I start changing the patch again. I missed the remote-heads which is an existing feature when I commented; if this can be exposed to the users as an extension to it like Jakub suggests, it may be a better direction. Usually %feature hash is preferred over adding new configuration variable but this is not some hard rule. Note however that patches adding new config are met with more scrutiny, as it is harder to fix mistakes because of requirement of backwards compatibility of configuration files. I don't know what kind of backwards compatibility you mention. A patch adds new feature controlled by a configuration in one way (e.g. as a totally new ad-hoc switch that is seemingly orthogonal to all the existing features), people start using the feature using that configuration method, and then later we find out that it is better controlled by a different way (e.g. as a natural extension to an existing feature, controlled by how the existing feature is controlled, perhaps after extending it) because it allows more flexibility in the future. At that point, we can extend the way the existing feature is controlled to trigger the new feature in a more uniform way, but we cannot remove the new ad-hoc switch the patch originally added to control this new feature because there already are people who are using it, and we end up having to support the unified and extensible way to configure, which we prefer to keep in the longer term, and also the ad-hoc switch the patch added, which we wish we never had in the first place. The latter needs to be deprecated and removed over time. -- 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 v3] gitweb: Add an option for adding more branch refs
On Mon, Dec 2, 2013 at 7:18 PM, Junio C Hamano gits...@pobox.com wrote: Krzesimir Nowak krzesi...@endocode.com writes: On Mon, 2013-12-02 at 01:21 +0100, Jakub Narębski wrote: On Thu, Nov 28, 2013 at 12:44 PM, Krzesimir Nowak krzesi...@endocode.com wrote: 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. Signed-off-by: Krzesimir Nowak krzesi...@endocode.com Why not use %feature hash instead of adding new configuration variable? I think that this option is similar enough to 'remote_heads' feature (which BTW should be 'remote-heads'), and could conceveilably enabled on a per-repository basis, i.e. with repository configuration override, isn't it? I'd like to see some consensus on it before I start changing the patch again. I missed the remote-heads which is an existing feature when I commented; if this can be exposed to the users as an extension to it like Jakub suggests, it may be a better direction. I think that additional-branch-refs (or just branch-refs) is different enough from remote_heads feature (which is about showing remotes section in summary view, and IIRC adding remotes view) that it should be kept separate. On the other hand it is similar enough that I think style of implementation should also be similar, i.e. use %feature hash. IMVHO -- 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
Re: [PATCH v3] gitweb: Add an option for adding more branch refs
Jakub Narębski jna...@gmail.com writes: I think that additional-branch-refs (or just branch-refs) is different enough from remote_heads feature (which is about showing remotes section in summary view, and IIRC adding remotes view) that it should be kept separate. On the other hand it is similar enough that I think style of implementation should also be similar, i.e. use %feature hash. Sounds sensible. Thanks. -- 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 v3] gitweb: Add an option for adding more branch refs
On Thu, Nov 28, 2013 at 12:44 PM, Krzesimir Nowak
krzesi...@endocode.com wrote:
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.
Signed-off-by: Krzesimir Nowak krzesi...@endocode.com
Why not use %feature hash instead of adding new configuration variable?
I think that this option is similar enough to 'remote_heads' feature
(which BTW should be 'remote-heads'), and could conceveilably enabled
on a per-repository basis, i.e. with repository configuration override,
isn't it?
Usually %feature hash is preferred over adding new configuration variable
but this is not some hard rule. Note however that patches adding new config
are met with more scrutiny, as it is harder to fix mistakes because of
requirement of backwards compatibility of configuration files.
BTW. there really should be gitweb/CodingGuidelines...
---
Documentation/gitweb.conf.txt | 13
gitweb/gitweb.perl| 75
+--
2 files changed, 71 insertions(+), 17 deletions(-)
diff --git a/Documentation/gitweb.conf.txt b/Documentation/gitweb.conf.txt
index e2113d9..cd1a945 100644
--- a/Documentation/gitweb.conf.txt
+++ b/Documentation/gitweb.conf.txt
@@ -549,6 +549,19 @@ 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');
+
I think the last (long) sentence would better read if it began with For example
if you have... then you could set this variable to ..., IMVHO.
BTW. if we decide on using %feature hash instead, it would be in the
CONFIGURING GITWEB FEATURES section.
++
+It is an error to specify a ref that does not pass git check-ref-format
+scrutiny.
Hmmm... One one hand erroring out on invalid refs means that we can
find error in config earlier and easier, on the other hand ignoring invalid
refs would make it resilent to errors in gitweb config (and repository config,
if we use %feature with per-repository override).
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
[...]
@@ -626,6 +630,10 @@ sub feature_avatar {
return @val ? @val : @_;
}
+sub get_branch_refs {
+return ('heads', @additional_branch_refs);
+}
Nice way of ensuring that list of all branches starts with heads.
# 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 '$ref' in
\@additional_branch_refs) unless (validate_ref($ref));
+ # 'heads' are added implicitly in get_branch_refs().
+ $unique_branch_refs{$ref} = 1 if ($ref ne 'heads');
+ }
+ @additional_branch_refs = sort keys %unique_branch_refs;
+ %unique_branch_refs = undef;
+}
This subroutine is quite similar to filter_snapshot_fmts for 'snapshot'
feature, perhaps the name should be patterned after it, i.e.
filter_branch_refs() or something...
If there were generic config_sanity_check(), it would call filter_branch_refs().
@@ -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();
}
I'm not sure if evaluate_gitweb_config is best place for sanity check
of said gitweb config, and not e.g. in run_request()... though having
it there has its own advantages.
BTW. it can be written as:
- read_config_file($GITWEB_CONFIG) and return;
- read_config_file($GITWEB_CONFIG_SYSTEM);
+ read_config_file($GITWEB_CONFIG) or
+ read_config_file($GITWEB_CONFIG_SYSTEM);
+
+
Re: [PATCH v3] gitweb: Add an option for adding more branch refs
On Thu, Nov 28, 2013 at 6:44 AM, Krzesimir Nowak krzesi...@endocode.com wrote:
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.
Signed-off-by: Krzesimir Nowak krzesi...@endocode.com
---
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 68c77f6..25e1d37 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 '$ref' in
\@additional_branch_refs) unless (validate_ref($ref));
+ # 'heads' are added implicitly in get_branch_refs().
+ $unique_branch_refs{$ref} = 1 if ($ref ne 'heads');
+ }
+ @additional_branch_refs = sort keys %unique_branch_refs;
+ %unique_branch_refs = undef;
+}
%unique_branch_refs is going out of scope here, so clearing it seems
unnecessary.
Moreover, with warnings enabled, perl should be complaining about an
Odd number of elements in hash assignment. (Normally, you would
clear a hash with '%foo=()' or 'undef %foo'.)
+
our ($GITWEB_CONFIG, $GITWEB_CONFIG_SYSTEM, $GITWEB_CONFIG_COMMON);
sub evaluate_gitweb_config {
our $GITWEB_CONFIG = $ENV{'GITWEB_CONFIG'} || ++GITWEB_CONFIG++;
--
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
