Re: gitosis-lite

From: Jakub Narebski <jnareb@gmail.com>
Date: Aug 25, 2009, 12:05 UTC
Message-ID: <200908251405.17644.jnareb@gmail.com>
In-Reply-To: <2e24e5b90908242253v411ad5f3t8a2802079914d0bf@mail.gmail.com>

Sitaram Chamarty wrote:

Show 10 quoted lines

> On Tue, Aug 25, 2009 at 12:14 AM, Jakub Narebski<jnareb@gmail.com> wrote:
> 
> > A few comments about the code, taking gl-auth-command as example.
> 
> > Wouldn't it be better to use "use warnings" instead of 'perl -w'?
> 
> I'm not sure what is the minimum perl required for git
> itself.  Has it needed perl > 5.6.0 for more than a year at
> least?  The only real difference between these two is scope,
> which is a non-issue here, so I played safe.

I think that git requires Perl at least version 5.6

Show 9 quoted lines

> > It would be, I think, better if you have used POD for such
> > documentation.  One would be able to generate manpage using pod2man,
> > and it is no less readable in source code.  See e.g. perl/Git.pm or
> > contrib/hooks/update-paranoid.
> 
> Hmm... I've been spoiled by Markdown's sane bullet list
> handling.  Visually, POD forces everything other than code
> to be flush left -- any sort of list is definitely less
> readable in source code as a result.  IMHO of course.

How it is relevant to the issue at hand? I was talking about replacing documentation comments in the header with POD markup.

Also you usually document top-level structures with POD.

Show 10 quoted lines

> > > # first, fix the biggest gripe I have with gitosis, a 1-line change
> > > my $user=$ENV{GL_USER}=shift;       # there; now that's available everywhere!
> >
> > Eh?  This is standalone script, isn't it?  Shouldn't it be
> >
> >   my $user = $ENV{GL_USER} = $ARGV[0];       # there; now that's available everywhere!
> 
> Hmm... I didn't know there was a difference, other than
> depleting @ARGV, if you're outside a subroutine.  I'll take
> a relook at it.

It is, I think, the matter of taste. IMHO using @ARGV to get _program_ parameters is better than use 'shift' which is used to get subroutine arguments.

BTW. have you tried using Perl::Critic or http://perlcritic.com on your code (but remember that those best practice recommendations do not need to be followed blindly)?

Show 16 quoted lines

> > > open(LOG, ">>", "$GL_ADMINDIR/log");
> > > print LOG "\n", scalar(localtime), " $ENV{SSH_ORIGINAL_COMMAND} $user\n";
> > > close(LOG);
> >
> > It is better practice to use lexical variables instead of barewords
> > for filehandles:
> 
> Good catch; thanks!  I guess I'm showing my age :)  Fixed
> all of them!
> 
> > Don't forget to check for error.
> 
> Hmm.. well I'm still debating if a log file write error
> should block git access / push, but there were two more
> important closes (again in gl-compile-conf) that were
> unguarded.  Fixed, thanks.

I was thinking about not writing to log file if you can't open it.

-- 
Jakub Narebski
Poland