Re: [PATCH 3/6] GITWEB - Add git:// link to summary pages
- From
Jakub Narebski <jnareb@gmail.com>
- Date
- Dec 11, 2009, 13:44 UTC
- Message-ID
- <m3tyvxd3pr.fsf@localhost.localdomain>
- In-Reply-To
- <1260488743-25855-4-git-send-email-warthog9@kernel.org>
"John 'Warthog9' Hawley" <warthog9@kernel.org> writes:
Show 6 quoted lines
> This adds a git:// link to the summary pages should a common > $gitlinkurl be defined (default is nothing defined, thus nothing > shown) > > This does make the assumption that the git trees share a common > path, and nothing to date is known to actually make use of the link
The problem I had and have with this patch is the duplication of data: $gitlinkurl contains subset of information in @git_base_url_list, which in turn is filled from GITWEB_BASE_URL build config variable.
I can understand that for performance reason you don't want to check $projectroot/$project/cloneurl nor gitweb.url config variable for each and every displayed project; if the link to repository (for git) cannot be derived from project path (repository path), then simply do not dosplay it.
Show 17 quoted lines
> > Signed-off-by: John 'Warthog9' Hawley <warthog9@eaglescrag.net> > --- > gitweb/gitweb.perl | 8 ++++++++ > 1 files changed, 8 insertions(+), 0 deletions(-) > > > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl > index d84f4c0..7ad096c 100755 > --- a/gitweb/gitweb.perl > +++ b/gitweb/gitweb.perl > @@ -224,6 +224,10 @@ our %avatar_size = ( > # This is here to allow for missmatch git & gitweb versions > our $missmatch_git = ''; > > +#This is here to deal with an extra link on the summary pages - if it's left blank > +# this link will not be shwon. If it's set, this will be prepended to the repo and used
s/shwon/shown/
I'd say that 'Full URL is "$gitlinkurl/$project"' instead of last sentence in above comment.
Please watch for excessive line lengths.
> +our $gitlinkurl = '';
Why not
our $gitlinkurl_base = "++GITWEB_BASE_URL++";
of course changing the name everywhere.
Show 12 quoted lines
> +
> # Used to set the maximum load that we will still respond to gitweb queries.
> # if we exceed this than we do the processing to figure out if there's a mirror
> # and redirect to it, or to just return 503 server busy
> @@ -4454,6 +4458,10 @@ sub git_project_list_body {
> $cgi->a({-href => href(project=>$pr->{'path'}, action=>"log")}, "log") . " | " .
> $cgi->a({-href => href(project=>$pr->{'path'}, action=>"tree")}, "tree") .
> ($pr->{'forks'} ? " | " . $cgi->a({-href => href(project=>$pr->{'path'}, action=>"forks")}, "forks") : '') .
> + if( $gitlinkurl ne '' ){
> + print " | ". $cgi->a({-href => "git://$gitlinkurl/".esc_html($pr->{'path'})}, "git");
> + }
> + print "".Does it even pass tests?
$cgi->a({-href => href(project=>$pr->{'path'}, action=>"log")}, "log") . " | " .
$cgi->a({-href => href(project=>$pr->{'path'}, action=>"tree")}, "tree") .
($pr->{'forks'} ? " | " . $cgi->a({-href => href(project=>$pr->{'path'}, action=>"forks")}, "forks") : '') .
+ ($gitlinkurl_base ?
+ " | " . $cgi->a({-href=>"$gitlinkurl_base/$pr->{'path'}", "git") : '') .
"</td>\n" .
"</tr>\n";
}Changes made: * Instead of using separate if conditional statement and print statement (note that you forgot to change '.' to ';' to end statement) use ternary conditional operator "?:" * Make $gitlinkurl_base include "git://" protocol specifier * Do not create "git" link if $gitlinkurl_base is false, which means undef, empty string '' and 0 (but 0 is not very likely to be base for "git" link). * Do not use esc_html on fragment of URL. The CGI.pm should escape attributes itself. If it was HTTP link, one should perhaps esc_url on whole link, but esc_html is for escaping HTML.
-- Jakub Narebski Poland ShadeHawk on #git