Re: [PATCH 3/6] GITWEB - Add git:// link to summary pages

[Jakub Narebski <jnareb@gmail.com>]

"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