On Tue, 2013-11-26 at 13:48 -0800, Junio C Hamano wrote:
> Krzesimir Nowak <krzesimir@xxxxxxxxxxxx> writes:
>
> > 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@xxxxxxxxxxxx>
> > ---
> > gitweb/gitweb.perl | 99 ++++++++++++++++++++++++++++++++++++++++--------------
> > 1 file changed, 74 insertions(+), 25 deletions(-)
> >
> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> > index 68c77f6..9bfd38b 100755
> > --- a/gitweb/gitweb.perl
> > +++ b/gitweb/gitweb.perl
> > @@ -17,6 +17,7 @@ use Encode;
> > use Fcntl ':mode';
> > use File::Find qw();
> > use File::Basename qw(basename);
> > +use List::Util qw(min);
> > use Time::HiRes qw(gettimeofday tv_interval);
> > binmode STDOUT, ':utf8';
> >
> > @@ -122,6 +123,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,14 +631,29 @@ 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.
> > sub check_head_link {
> > my ($dir) = @_;
> > my $headfile = "$dir/HEAD";
> > - return ((-e $headfile) ||
> > - (-l $headfile && readlink($headfile) =~ /^refs\/heads\//));
> > +
> > + if (-e $headfile) {
> > + return 1;
> > + }
> > + if (-l $headfile) {
> > + my $rl = readlink($headfile);
> > +
> > + for my $ref (get_branch_refs()) {
> > + return 1 if $rl =~ /^refs\/$ref\//;
> > + }
> > + }
> > + return 0;
>
> The change to this function is wrong. A non-detached HEAD that
> points at anything other than refs/heads/ should be considered a
> repository corruption and should not be encouraged.
Ok, I'll get rid of it.
>
> > @@ -2515,6 +2535,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 +2543,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/$ref/(.*)$!) ||
> > + (defined $hash && $hash =~ m!^refs/$ref/(.*)$!)) {
> > + $branch = $1;
> > + $matched_ref = $ref;
> > + last;
> > + }
> > }
> > # find log type for feed description (title)
> > my $type = 'log';
> > @@ -2540,7 +2566,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;
>
> OK.
>
> > @@ -3184,24 +3210,43 @@ sub git_get_project_owner {
> > return $owner;
> > }
> >
> > -sub git_get_last_activity {
> > - my ($path) = @_;
> > - my $fd;
> > +sub git_get_last_activity_age {
> > + my ($refs) = @_;
> > + my $fd = -1;
> >
> > - $git_dir = "$projectroot/$path";
> > open($fd, "-|", git_cmd(), 'for-each-ref',
> > '--format=%(committer)',
> > '--sort=-committerdate',
> > '--count=1',
> > - 'refs/heads') or return;
> > + $refs) or return undef;
> > +
> > my $most_recent = <$fd>;
> > - close $fd or return;
> > + close $fd or return undef;
> > if (defined $most_recent &&
> > $most_recent =~ / (\d+) [-+][01]\d\d\d$/) {
> > my $timestamp = $1;
> > - my $age = time - $timestamp;
> > - return ($age, age_string($age));
> > + return time - $timestamp;
> > }
> > +
> > + return undef;
> > +}
> > +
> > +sub git_get_last_activity {
> > + my ($path) = @_;
> > + my @ages = ();
> > +
> > + $git_dir = "$projectroot/$path";
> > + for my $ref (get_branch_refs()) {
> > + my $age = git_get_last_activity_age('refs/' . $_);
> > +
> > + push @ages, $age if defined $age;
> > + }
> > + if (@ages) {
> > + my $min_age = min(@ages);
> > +
> > + return ($min_age, age_string($min_age));
> > + }
> > +
> > return (undef, undef);
> > }
>
> The original runs
>
> for-each-ref --count=1 refs/heads
>
> and picks the latest one, so shouldn't it be the matter of running
>
> for-each-ref --count=1 refs/heads refs/extra
>
> i.e. something like
>
> @@ -3193,7 +3193,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 &&
>
> to grab the latest among the refs you care about?
That's simpler, yes.
>
> > @@ -3223,7 +3268,7 @@ sub git_get_remotes_list {
> > next if $wanted and not $remote eq $wanted;
> > my ($url, $key) = ($1, $2);
> >
> > - $remotes{$remote} ||= { 'heads' => () };
> > + $remotes{$remote} ||= { map { $_ => () } get_branch_refs() };
> > $remotes{$remote}{$key} = $url;
> > }
> > close $fd or return;
> > @@ -3237,9 +3282,11 @@ sub fill_remote_heads {
> > my @heads = map { "remotes/$_" } keys %$remotes;
> > my @remoteheads = git_get_heads_list(undef, @heads);
> > foreach my $remote (keys %$remotes) {
> > - $remotes->{$remote}{'heads'} = [ grep {
> > - $_->{'name'} =~ s!^$remote/!!
> > - } @remoteheads ];
> > + foreach my $ref (get_branch_refs()) {
> > + $remotes->{$remote}{$ref} = [ grep {
> > + $_->{'name'} =~ s!^$remote/!!
> > + } @remoteheads ];
> > + }
> > }
> > }
>
> Hmmm, why? Aren't these additional ones about the local
> "branch-like" refs? What makes us think that these names are also
> shared with the names from remote hierarchies?
Might be another place where I had no clue if the change is right.
Sorry.
>
> > @@ -3644,7 +3691,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 +3709,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 '|', get_branch_refs();
> > + $name =~ s!^refs/(?:$strip_refs|remotes)/!!;
> >
> > $ref_item{'name'} = $name;
> > $ref_item{'id'} = $hash;
> > @@ -4286,7 +4334,7 @@ sub git_print_page_nav {
> > # available if the feature is enabled
> > sub format_ref_views {
> > my ($current) = @_;
> > - my @ref_views = qw{tags heads};
> > + my @ref_views = ('tags', get_branch_refs());
I'll get rid of that change, too. I just verified that it is wrong too.
> > push @ref_views, 'remotes' if gitweb_check_feature('remote_heads');
> > return join " | ", map {
> > $_ eq $current ? $_ :
> > @@ -7179,7 +7227,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 '|', get_branch_refs();
> > + if ($hash =~ m!^refs/(?:$strip_refs|remotes)/(.*)$!) {
> > $ver = $1;
> > }
> > $ver .= '-' . git_get_short_hash($project, $hash);
>
> As the elements of @additional_branch_refs are expected by code
> added by this patch to never have any special metacharacters in
> them, it probably is a good idea to sanity check it to catch
> misconfigurations and typos before doing anything else.
>
I assume that in case of misconfiguration we are returning 500.
I added a sanity checking for duplicate refs, for explicitly specified
'heads' and refs that fail to pass the validate_ref (which I splitted
from validate_refname).
As for metacharacters - valid ref can contain a '|' which is a
metacharacter in regex, so I wrapped some uses of
@additional_branch_refs members in regexes into \Q and \E or used a
quotemeta on them. That escapes metacharacters.
Also, I added some docs to gitweb.conf.txt.
Patch in "[PATCH v2] gitweb: Add an option for adding more branch refs"
> Other than that, looks good to me.
>
> Thanks.
--
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