Re: gitosis-lite

From: Jakub Narebski <jnareb@gmail.com>
Date: Aug 24, 2009, 18:44 UTC
Message-ID: <m363cdm4pm.fsf@localhost.localdomain>
In-Reply-To: <2e24e5b90908240528v56fdf30cq4c046fca321a3b17@mail.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,... ;-)

Show 5 quoted lines

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

Show 15 quoted lines

> # === 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.

Show 6 quoted lines

> 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