git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jakub Narebski <jnareb@gmail.com>
To: git@vger.kernel.org
Subject: Re: [PATCH] gitweb: localhost placeholder parser for cloneurl file
Date: Mon, 30 Jul 2007 01:57:01 +0200	[thread overview]
Message-ID: <f8j9gb$voj$1@sea.gmane.org> (raw)
In-Reply-To: d68f80d90707282354l1593f2ctc0cf7a05eeb3b8e0@mail.gmail.com

[Cc: Leandro Dorileo <ldorileo@gmail.com>, git@vger.kernel.org]

Leandro Dorileo wrote:

> From 32da24e1e18a1c5f32bfa0afdbcb6d0f2b0432f3 Mon Sep 17 00:00:00 2001
> From: Leandro Dorileo <dorileo@ossystems.com.br>
> Date: Sat, 28 Jul 2007 15:34:20 -0400
> Subject: [PATCH] gitweb: localhost placeholder parser for cloneurl file
> 
> Implementation of a localhost placeholder parsing in git_get_project_url_list.
> It`s useful in cases of gitweb being hosted in a work-station (like a notebook)
> used in a local team development environment and, implementation of a git-url
> link in the git project list body like in git.kernel.org.

First, the commit message _has_ to be self explanatory, and be easily
understood _without_ email message which introduced it.

So you should clean-up the explanation about _why_ one would want to
replace #localhost#, or ++LOCALHOST++ in the cloneurls by the server
name (the same configuration, different machines; although I do not
understand why it is so hard to change configutation depending on
machine).

Second, please clean up the code. Comments below.

Third, sign-off your patches: see Documentation/SubmittingPatches

> ---
>  gitweb/gitweb.perl |   21 ++++++++++++++++++---
>  1 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index b381692..281d823 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -1471,7 +1471,16 @@ sub git_get_project_url_list {
>       my @git_project_url_list = map { chomp; $_ } <$fd>;
>       close $fd;
> 
> -     return wantarray ? @git_project_url_list : \@git_project_url_list;
> +        if(wantarray){

Style: "if (wantarray) {".

But I think you would want to do the substitution regardless of
whether git_get_project_url_list() subroutine is called in scalar
or list context. (By the way I think it might be a mistake to use
this trick: return array in the list context, return array reference
in the scalar context).

So you would put the code proposed below before

        return wantarray ? @git_project_url_list : \@git_project_url_list;

> +            for(my $i = 0; $i < @git_project_url_list; $i++){
> +                if(index(@git_project_url_list[$i], "#localhost#") != -1){
> +                    @git_project_url_list[$i] =~
> s/#localhost#/server_name()/eg;
> +                }
> +            }

Why not simply:

        @git_project_url_list = map { s/#localhost#/server_name()/eg; } 
                @git_project_url_list;

And didn't you meant $cgi->server_name() or just $ENV{'SERVER_NAME'}?

> +            return @git_project_url_list;
> +        }else{
> +            return \@git_project_url_list;
> +        }

Just end with what it was before:

        return wantarray ? @git_project_url_list : \@git_project_url_list;

>  }
 
>  sub git_get_projects_list {
> @@ -3384,8 +3393,14 @@ sub git_project_list_body {
>                     $cgi->a({-href => href(project=>$pr->{'path'},
> action=>"shortlog")}, "shortlog") . " | " .
>                     $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") : '') .
> -                   "</td>\n" .
> +                   ($pr->{'forks'} ? " | " . $cgi->a({-href =>
> href(project=>$pr->{'path'}, action=>"forks")}, "forks") : '');
> +
> +                      my @url_list = git_get_project_url_list($pr->{'path'});
> +                      if(@url_list != 0){
> +                        print " | " . $cgi->a({-href => @url_list[0]}, "git");
> +                      }

Style: "if (@url_list) {" is enough.
Style: use tabs for indent, now spaces.

But this is totally independent thing, and it should be put in
separate patch. And it doesn't make much sense as first URL in
the list might not be using scheme recognized by web browser:
 - it can be git:// URL.
 - it can be scp style ssh:// URL: <user>@<host>:<path>
 - it can be local path, without file:// prefix

> +
> +                   print "</td>\n" .
>                     "</tr>\n";
>       }
>       if (defined $extra) {


-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

      reply	other threads:[~2007-07-29 23:57 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-07-29  6:54 [PATCH] gitweb: localhost placeholder parser for cloneurl file Leandro Dorileo
2007-07-29 23:57 ` Jakub Narebski [this message]

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='f8j9gb$voj$1@sea.gmane.org' \
    --to=jnareb@gmail.com \
    --cc=git@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
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;
as well as URLs for NNTP newsgroup(s).