Re: [PATCH] Make core.sharedRepository more generic

From: Heikki Orsila <shdl@zakalwe.fi>
Date: Apr 12, 2008, 19:46 UTC
Message-ID: <20080412194634.GG31039@zakalwe.fi>
In-Reply-To: <2008-04-12-21-15-04+trackit+sam@rfc1149.net>

On Sat, Apr 12, 2008 at 09:15:03PM +0200, Samuel Tardieu wrote:

Show 19 quoted lines

> The use of named constants vs. literals seem inconsistent in your
> patch, compare
> 
> | +               mode = (mode & ~0777) | shared_repository;
> 
> to
> 
> | +               mode |= (shared_repository & 0600) ? S_IXUSR : 0;
> | +               mode |= (shared_repository & 0060) ? S_IXGRP : 0;
> | +               mode |= (shared_repository & 0006) ? S_IXOTH : 0;
> 
> I first thought that you were using literals with "shared_repository"
> and named constants with mode but the first line I quoted shows that
> this is not the case.
> 
> Btw, aren't those last three lines better replaced by
> 
>   /* Copy read bits to execute bits */
>   mode |= (shared_repository & 0444) >> 2;

Agreed on both previous points. Will submit a new patch.

> I don't see where you deal with executable files.

Show 8 quoted lines

> Also, wouldn't it be more consistent to use a negative value to
> --shared, that is a umask-compatible one, rather than a positive value
> which needs to be tweaked for directories and executable files? You
> would only have to "&" 0666 or 0777 with "~perms" to get the right
> permissions.
> 
> --shared=0007 would be equivalent to PERM_GROUP, --shared=0027 to
> group-readable-but-not-writable, and --shared=0002 to PERM_EVERYBODY.

I see the point, but I like to think it as a chmod value rather than a umask value.

-- 
Heikki Orsila
heikki.orsila@iki.fi
http://www.iki.fi/shd