From mboxrd@z Thu Jan  1 00:00:00 1970
From: Jason at zx2c4.com (Jason A. Donenfeld)
Date: Tue, 16 Oct 2012 13:21:53 +0200
Subject: [PATCH 2/3] Add ability to authorize viewing a repository
In-Reply-To: <1350378927-10834-3-git-send-email-valentin.haenel@gmx.de>
References: <1350378927-10834-1-git-send-email-valentin.haenel@gmx.de>
 <1350378927-10834-3-git-send-email-valentin.haenel@gmx.de>
Message-ID: <CAHmME9rejoYeRMY0vs0Ab+1bVOxmVRSDL5fNnmt7v0FHU1x4AQ@mail.gmail.com>

On Tue, Oct 16, 2012 at 11:15 AM, Valentin Haenel
<valentin.haenel at gmx.de> wrote:
>         ctx->cfg.ssdiff = 0;
> +       ctx->cfg.authorization = "/bin/true";

This should be set to null by default.

Also, choose a more specific variable name than "authorization".


>
> +       /* Here we do the authorization check.
> +        *
> +        * TODO
> +        * ----
> +        *  * figure out if this is the correct place to do the check
> +        *  * check that the authorization script exists and is executable
> +        *
> +        */

It's best to submit code that's been completed, not with glaring
TODOs, whose fate of completion is unknown.


> +       if (ctx->repo && system(fmt("%s %s %s",
> +                                       ctx->cfg.authorization,
> +                                       ctx->repo->name,
> +                                       ctx->env.remote_user)) != 0) {


Considering the null change above, this should also be changed to  "if
(ctx..authorization && !system(...)) return;


> +authorization::
> +       Specifies a path to authorization executable. The executable must take two
> +       arguments: "<repository> <user> [<permissions>]". The permission is the
> +       type of permission we wish to check for, e.g. "R" or "RW" or "RW+". A
> +       common use case is to call gitolite through this machinery.
> +

Remove reference to gitolite. Add default value.



In general, the idea of having this is nice, but the implementation
needs to be polished.