On Mon, 2013-12-02 at 01:21 +0100, Jakub Narębski wrote:
> On Thu, Nov 28, 2013 at 12:44 PM, Krzesimir Nowak
> <krzesimir@xxxxxxxxxxxx> 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 <krzesimir@xxxxxxxxxxxx>
>
> 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 filter_branch_refs().
I had an additional_branches_refs_check() sub which was called by
config_sanity_check(), but I scrapped it. I wanted config_sanity_check()
to be general configuration checker, but for now it would only call a
single function, so I inlined it. If later more configuration checking
will be added then the current body could be moved to separate sub.
I can move it back to separate sub if you want.
>
> > @@ -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);
> +
> + config_sanity_check();
>
Ok, will rewrite it.
>
> Anyway if we were to use %feature hash, there is configure_gitweb_features()
> for calling filter_branch_refs().
>
> > # 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;
>
> Hmmm... validate_ref() is IMHO too similar to validate_refname(),
> and it isn't about *parameter* validation. Perhaps check_ref_format()?
Ok.
>
> > @@ -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;
> > }
>
> Nice refactoring (it *could*, but doesn't need to, be in separate patch).
>
> > @@ -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;
> > + }
> > }
>
> Nice!
>
> > @@ -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?
Cheers,
--
Krzesimir Nowak
Software Developer
Endocode AG
krzesimir@xxxxxxxxxxxx
------
Endocode AG, Johannisstraße 20, 10117 Berlin
info@xxxxxxxxxxxx | 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html