On Sat, Apr 12, 2008 at 09:15:03PM +0200, Samuel Tardieu wrote: > 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. > 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@xxxxxx http://www.iki.fi/shd -- 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
