From: Jakub Narebski <jnareb@gmail•com>
To: "John 'Warthog9' Hawley" <warthog9@kernel•org>
Cc: git@vger•kernel.org, "John 'Warthog9' Hawley" <warthog9@eaglescrag•net>
Subject: Re: [PATCH 3/6] GITWEB - Add git:// link to summary pages
Date: Fri, 11 Dec 2009 05:44:52 -0800 (PST) [thread overview]
Message-ID: <m3tyvxd3pr.fsf@localhost.localdomain> (raw)
In-Reply-To: <1260488743-25855-4-git-send-email-warthog9@kernel.org>
"John 'Warthog9' Hawley" <warthog9@kernel•org> writes:
> 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.
>
> 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.
> +
> # 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
next prev parent reply other threads:[~2009-12-11 13:45 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2009-12-10 23:45 [PATCH 0/6] Gitweb caching changes v2 John 'Warthog9' Hawley
2009-12-10 23:45 ` [PATCH 1/6] GITWEB - Load Checking John 'Warthog9' Hawley
2009-12-10 23:45 ` [PATCH 2/6] GITWEB - Missmatching git w/ gitweb John 'Warthog9' Hawley
2009-12-10 23:45 ` [PATCH 3/6] GITWEB - Add git:// link to summary pages John 'Warthog9' Hawley
2009-12-10 23:45 ` [PATCH 4/6] GITWEB - Makefile changes John 'Warthog9' Hawley
[not found] ` <1260488743-25855-6-git-send-email-warthog9@kernel.org>
2009-12-10 23:45 ` [PATCH 6/6] GITWEB - Separate defaults from main file John 'Warthog9' Hawley
2009-12-11 15:46 ` Jakub Narebski
2009-12-11 15:58 ` J.H.
2009-12-11 22:53 ` Jakub Narebski
2009-12-16 1:22 ` Junio C Hamano
2009-12-16 2:00 ` J.H.
2009-12-16 19:52 ` Jakub Narebski
2009-12-16 20:04 ` J.H.
2009-12-16 2:22 ` Jakub Narebski
2009-12-11 14:28 ` [PATCH 4/6] GITWEB - Makefile changes Jakub Narebski
2009-12-11 16:22 ` J.H.
2009-12-11 16:41 ` Jakub Narebski
2009-12-19 13:32 ` [PATCH/RFCv2 4/6] gitweb: Makefile improvements Jakub Narebski
2009-12-11 12:52 ` [PATCH 3/6] GITWEB - Add git:// link to summary pages Johannes Schindelin
2009-12-11 13:44 ` Jakub Narebski [this message]
2009-12-18 21:02 ` [PATCHv2 3/6] gitweb: Optionally add "git" links in project list page Jakub Narebski
2009-12-11 10:52 ` [PATCH 2/6] GITWEB - Missmatching git w/ gitweb Jakub Narebski
2009-12-18 19:18 ` [RFC/PATCHv2 2/6] gitweb: Add option to force version match Jakub Narebski
2009-12-11 12:49 ` [PATCH 2/6] GITWEB - Missmatching git w/ gitweb Johannes Schindelin
2009-12-10 23:54 ` [PATCH 1/6] GITWEB - Load Checking Sverre Rabbelier
2009-12-11 0:52 ` Jakub Narebski
2009-12-11 1:10 ` Junio C Hamano
2009-12-11 2:19 ` J.H.
2009-12-11 2:50 ` Junio C Hamano
2009-12-11 2:58 ` J.H.
2009-12-11 3:07 ` J.H.
2009-12-11 3:09 ` Junio C Hamano
2009-12-11 10:09 ` Jakub Narebski
2009-12-18 16:36 ` [PATCHv2 1/6] gitweb: Load checking Jakub Narebski
2009-12-11 13:53 ` [PATCH 1/6] GITWEB - Load Checking Mihamina Rakotomandimby
2009-12-10 23:53 ` [PATCH 0/6] Gitweb caching changes v2 Sverre Rabbelier
2009-12-11 15:51 ` Jakub Narebski
[not found] ` <4B226D56.7000004@kernel.org>
2009-12-11 18:01 ` Jakub Narebski
2009-12-11 18:26 ` J.H.
2009-12-12 1:37 ` Jakub Narebski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=m3tyvxd3pr.fsf@localhost.localdomain \
--to=jnareb@gmail$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=warthog9@eaglescrag$(echo .)net \
--cc=warthog9@kernel$(echo .)org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox