Sitaram Chamarty wrote:
> On Tue, Aug 25, 2009 at 12:14 AM, Jakub Narebski<jnareb@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html