Re: gitosis-lite

[Jakub Narebski <jnareb@gmail.com>]

Sitaram Chamarty wrote:
> 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
 
> > 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.


> > > # 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)?

> > > 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