Re: gitosis-lite

[Jakub Narebski <jnareb@gmail.com>]

Sitaram Chamarty <sitaramc@gmail.com> writes:

> I created a new project called gitosis-lite, which combines
> the essential pieces of gitosis with the per-branch
> permissions stuff in the example in the howto directory of
> git.git.

As for the name: gitness, gitamine, gitrify,... ;-)
 
> The config file is different, (there's an annotated example
> you can look at).
> 
> The "why" and the "what" are all at
> http://github.com/sitaramc/gitosis-lite

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

> #!/usr/bin/perl -w
>
> use strict;

Wouldn't it be better to use "use warnings" instead of 'perl -w'?

> # === auth-command ===
> # the command that GL users actually run
> 
> # part of the gitosis-lite (GL) suite
> 
> # how run:      via sshd, being listed in "command=" in ssh authkeys
> # when:         every login by a GL user
> # input:        $1 is GL username, plus $SSH_ORIGINAL_COMMAND
> # output:
> # security:
> #     - currently, we just make some basic checks, copied from gitosis
>
> # robustness:
>
> # other notes:

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.

> our $GL_ADMINDIR;
> our $GL_CONF;
> our $GL_KEYDIR;
> our $GL_CONF_COMPILED;
> our $REPO_BASE;
> our %repos;

Why is the reason behind using 'our' instead of 'my' here?

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

> my $perm = 'W'; $perm = 'R' if $verb =~ $R_COMMANDS;

Either split it into two lines, or use ?: confitional operator:

  my $perm = ($verb =~ $R_COMMANDS ? 'R' : 'W');

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

  if (open my $logfh, ">>", "$GL_ADMINDIR/log") {
  	print $logfh "\n", scalar(localtime), " $ENV{SSH_ORIGINAL_COMMAND} $user\n";
  	close $logfh;
  }

Don't forget to check for error.

> $repo = "'$REPO_BASE/$repo.git'";
> exec("git", "shell", "-c", "$verb $repo");

That's not enough.  You have to shell-quote $repo, like in gitweb or
using String::ShellQuote module, or somehow use list form to pass
arguments to git-shell.  You protect here againts spaces in filename,
but not againts "'" (single quote) and for show shells "!"
(exclamation mark).

-- 
Jakub Narebski
Poland
ShadeHawk on #git