Re: gitosis-lite

Sitaram Chamarty wrote:

On Tue, Aug 25, 2009 at 12:14 AM, Jakub Narebski[off-list ref] wrote:

quoted

A few comments about the code, taking gl-auth-command as example.

quoted

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
 

quoted

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.

quoted

quoted

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

quoted

quoted

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!

quoted

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