{"thread":{"id":"13061","subject":"[PATCH] Add two core.sharedRepository options: group-readable and world-readable","startedAt":"2008-04-11T14:09:16Z","lastAt":"2008-04-12T18:52:54Z","messageCount":9,"participants":["Heikki Orsila","Junio C Hamano","Björn Steinbrink"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"74122","messageId":"20080411140916.GA30667@zakalwe.fi","threadId":"13061","inReplyTo":null,"subject":"[PATCH] Add two core.sharedRepository options: group-readable and world-readable","fromName":"Heikki Orsila","fromEmail":"heikki.orsila@iki.fi","sentAt":"2008-04-11T14:09:16Z","receivedAt":"2008-04-11T14:09:16Z","isPatch":true,"sender":{"key":"heikki.orsila@iki.fi","avatar":null},"body":"* 'group-readable': Make the repository group-readable (and g+sx for\n  directories), even if the user's umask forbids it.\n\n* 'world-readable': Make the repository readable for anyone, including\n  the group (implies group-readable), even if the user's umask forbids\n  it.\n\n* Add a warning to cache.h that \"enum sharedrepo\" item order should\n  not be changed because it would break backwards compatibility.\n\nSigned-off-by: Heikki Orsila <heikki.orsila@iki.fi>\n---\n Documentation/config.txt   |   13 ++++++++---\n Documentation/git-init.txt |    8 ++++++-\n cache.h                    |    6 ++++-\n path.c                     |   48 +++++++++++++++++++++++++++++--------------\n setup.c                    |    4 +++\n 5 files changed, 57 insertions(+), 22 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex fe43b12..ee13b2b 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -258,10 +258,15 @@ core.repositoryFormatVersion::\n core.sharedRepository::\n \tWhen 'group' (or 'true'), the repository is made shareable between\n \tseveral users in a group (making sure all the files and objects are\n-\tgroup-writable). When 'all' (or 'world' or 'everybody'), the\n-\trepository will be readable by all users, additionally to being\n-\tgroup-shareable. When 'umask' (or 'false'), git will use permissions\n-\treported by umask(2). See linkgit:git-init[1]. False by default.\n+\tgroup-writable). When 'group-readable', the repository will be\n+\treadable, but not writable, for users in the same group, even if the\n+\tuser's umask forbids it. When 'all' (or 'world' or 'everybody'),\n+\tthe repository will be readable by all users, additionally to being\n+\tgroup-shareable. When 'world-readable', the repository will be\n+\treadable for anyone, even if the user's umask forbids it. This option\n+\timplies 'group-readable'. When 'umask' (or 'false'), git will use\n+\tpermissions reported by umask(2). See linkgit:git-init[1].\n+\tFalse by default.\n \n core.warnAmbiguousRefs::\n \tIf true, git will warn you if the ref name you passed it is ambiguous\ndiff --git a/Documentation/git-init.txt b/Documentation/git-init.txt\nindex 62914da..6bbc09c 100644\n--- a/Documentation/git-init.txt\n+++ b/Documentation/git-init.txt\n@@ -31,7 +31,7 @@ structure, some suggested \"exclude patterns\", and copies of non-executing\n \"hook\" files.  The suggested patterns and hook files are all modifiable and\n extensible.\n \n---shared[={false|true|umask|group|all|world|everybody}]::\n+--shared[={false|true|umask|group|group-readable|all|world|world-readable|everybody}]::\n \n Specify that the git repository is to be shared amongst several users.  This\n allows users belonging to the same group to push into that\n@@ -49,6 +49,12 @@ is given:\n  - 'group' (or 'true'): Make the repository group-writable, (and g+sx, since\n    the git group may be not the primary group of all users).\n \n+ - 'group-readable': Make the repository group-readable (and g+sx for\n+   directories), even if the user's umask forbids it.\n+\n+ - 'world-readable': Make the repository readable for anyone, including\n+   the group (implies group-readable), even if the user's umask forbids it.\n+\n  - 'all' (or 'world' or 'everybody'): Same as 'group', but make the repository\n    readable by all users.\n \ndiff --git a/cache.h b/cache.h\nindex 2a1e7ec..4af6d62 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -474,10 +474,14 @@ static inline void hashclr(unsigned char *hash)\n \n int git_mkstemp(char *path, size_t n, const char *template);\n \n+/* Warning: enum sharedrepo item order should not be changed since it will\n+ * break backwards compatibility. */\n enum sharedrepo {\n \tPERM_UMASK = 0,\n \tPERM_GROUP,\n-\tPERM_EVERYBODY\n+\tPERM_EVERYBODY,\n+\tPERM_GROUP_READABLE,\n+\tPERM_WORLD_READABLE,\n };\n int git_config_perm(const char *var, const char *value);\n int adjust_shared_perm(const char *path);\ndiff --git a/path.c b/path.c\nindex f4ed979..b900f62 100644\n--- a/path.c\n+++ b/path.c\n@@ -266,22 +266,38 @@ int adjust_shared_perm(const char *path)\n \tif (lstat(path, &st) < 0)\n \t\treturn -1;\n \tmode = st.st_mode;\n-\tif (mode & S_IRUSR)\n-\t\tmode |= (shared_repository == PERM_GROUP\n-\t\t\t ? S_IRGRP\n-\t\t\t : (shared_repository == PERM_EVERYBODY\n-\t\t\t    ? (S_IRGRP|S_IROTH)\n-\t\t\t    : 0));\n-\n-\tif (mode & S_IWUSR)\n-\t\tmode |= S_IWGRP;\n-\n-\tif (mode & S_IXUSR)\n-\t\tmode |= (shared_repository == PERM_GROUP\n-\t\t\t ? S_IXGRP\n-\t\t\t : (shared_repository == PERM_EVERYBODY\n-\t\t\t    ? (S_IXGRP|S_IXOTH)\n-\t\t\t    : 0));\n+\n+\t/* PERM_GROUP_READABLE:   g+r\n+\t * PERM_GROUP:            g+rw\n+\t * PERM_WORLD_READABLE:   g+r,  o+r\n+\t * PERM_EVERYBODY:        g+rw, o+r\n+\t */\n+\tif (mode & S_IRUSR) {\n+\t\tif (shared_repository == PERM_GROUP ||\n+\t\t    shared_repository == PERM_GROUP_READABLE) {\n+\t\t\tmode |= S_IRGRP;\n+\t\t} else if (shared_repository == PERM_EVERYBODY ||\n+\t\t\t   shared_repository == PERM_WORLD_READABLE) {\n+\t\t\tmode |= S_IRGRP | S_IROTH;\n+\t\t}\n+\t}\n+\n+\tif (mode & S_IWUSR) {\n+\t\tif (shared_repository != PERM_GROUP_READABLE &&\n+\t\t    shared_repository != PERM_WORLD_READABLE)\n+\t\t\tmode |= S_IWGRP;\n+\t}\n+\n+\tif (mode & S_IXUSR) {\n+\t\tif (shared_repository == PERM_GROUP ||\n+\t\t    shared_repository == PERM_GROUP_READABLE) {\n+\t\t\tmode |= S_IXGRP;\n+\t\t} else if (shared_repository == PERM_EVERYBODY ||\n+\t\t\t   shared_repository == PERM_WORLD_READABLE) {\n+\t\t\tmode |= S_IXGRP | S_IXOTH;\n+\t\t}\n+\t}\n+\n \tif (S_ISDIR(mode))\n \t\tmode |= FORCE_DIR_SET_GID;\n \tif ((mode & st.st_mode) != mode && chmod(path, mode) < 0)\ndiff --git a/setup.c b/setup.c\nindex 3d2d958..a33ae9d 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -434,10 +434,14 @@ int git_config_perm(const char *var, const char *value)\n \t\t\treturn PERM_UMASK;\n \t\tif (!strcmp(value, \"group\"))\n \t\t\treturn PERM_GROUP;\n+\t\tif (!strcmp(value, \"group-readable\"))\n+\t\t\treturn PERM_GROUP_READABLE;\n \t\tif (!strcmp(value, \"all\") ||\n \t\t    !strcmp(value, \"world\") ||\n \t\t    !strcmp(value, \"everybody\"))\n \t\t\treturn PERM_EVERYBODY;\n+\t\tif (!strcmp(value, \"world-readable\"))\n+\t\t\treturn PERM_WORLD_READABLE;\n \t\ti = atoi(value);\n \t\tif (i > 1)\n \t\t\treturn i;\n-- \n1.5.4.4\n"},{"id":"74168","messageId":"7vfxtrnban.fsf@gitster.siamese.dyndns.org","threadId":"13061","inReplyTo":"20080411140916.GA30667@zakalwe.fi","subject":"Re: [PATCH] Add two core.sharedRepository options: group-readable and world-readable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-04-12T00:53:36Z","receivedAt":"2008-04-12T00:53:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Heikki Orsila <heikki.orsila@iki.fi> writes:\n\n> diff --git a/cache.h b/cache.h\n> index 2a1e7ec..4af6d62 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -474,10 +474,14 @@ static inline void hashclr(unsigned char *hash)\n>  \n>  int git_mkstemp(char *path, size_t n, const char *template);\n>  \n> +/* Warning: enum sharedrepo item order should not be changed since it will\n> + * break backwards compatibility. */\n\nThat's not a \"Warning\" (which tends to mean \"you can violate this if you\nknow what you are doing\"), but should be stronger than that.  Something\nlike (also notice the multi-line comment style --- the first line ends\nwith \"/*\\n\"):\n\n/*\n * NOTE NOTE NOTE!!\n *\n * Do not reorder this list; numerically written core.sharedrepository\n * in config files have always been valid, and you would break existing\n * repositories if you move these around.\n */\n\n>  enum sharedrepo {\n>  \tPERM_UMASK = 0,\n>  \tPERM_GROUP,\n> -\tPERM_EVERYBODY\n> +\tPERM_EVERYBODY,\n> +\tPERM_GROUP_READABLE,\n> +\tPERM_WORLD_READABLE,\n>  };\n\nBut I have to wonder if this patch is necessary.\n\nNeither am I convinced if this set is sufficient.\n\n+\t/*\n\t * PERM_GROUP_READABLE:   g+r\n+\t * PERM_GROUP:            g+rw\n+\t * PERM_WORLD_READABLE:   g+r,  o+r\n+\t * PERM_EVERYBODY:        g+rw, o+r\n+\t */\n\nFor example, you may want to enforce \"ug+rw,o=\" in a repository.  How\nwould you do that?\n\nPerhaps if you really wanted to have such a fine grained control, you\nwould be better off defining core.sharedrepository as a set/unset pair?\n\n\tcore.sharedrepository = 0660,007 ;# ug+rw,o-rwx\n\nOr even stronger \"set to this bit pattern\"?\n\n\tcore.sharedrepository = 0660 ;# ug=rw,o=\n\n(I think you would need to flip executable bit for directories if you go\nthis route).\n"},{"id":"74171","messageId":"20080412030021.GB31039@zakalwe.fi","threadId":"13061","inReplyTo":"7vfxtrnban.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Add two core.sharedRepository options: group-readable and world-readable","fromName":"Heikki Orsila","fromEmail":"shdl@zakalwe.fi","sentAt":"2008-04-12T03:00:21Z","receivedAt":"2008-04-12T03:00:21Z","isPatch":true,"sender":{"key":"shdl@zakalwe.fi","avatar":null},"body":"On Fri, Apr 11, 2008 at 05:53:36PM -0700, Junio C Hamano wrote:\n> That's not a \"Warning\" (which tends to mean \"you can violate this if you\n> know what you are doing\"), but should be stronger than that.  Something\n> like (also notice the multi-line comment style --- the first line ends\n> with \"/*\\n\"):\n> \n> /*\n>  * NOTE NOTE NOTE!!\n>  *\n>  * Do not reorder this list; numerically written core.sharedrepository\n>  * in config files have always been valid, and you would break existing\n>  * repositories if you move these around.\n>  */\n\nOK\n\n> >  enum sharedrepo {\n> >  \tPERM_UMASK = 0,\n> >  \tPERM_GROUP,\n> > -\tPERM_EVERYBODY\n> > +\tPERM_EVERYBODY,\n> > +\tPERM_GROUP_READABLE,\n> > +\tPERM_WORLD_READABLE,\n> >  };\n> \n> But I have to wonder if this patch is necessary.\n\nIt is necessary for us, and I suspect it will be useful in some\norganizations. The situation is this:\n\n* multiple groups on the same machine\n* users may want to keep umask 0077 as the default\n\n> Neither am I convinced if this set is sufficient.\n\nIt will be easy to add new groups if the need arises, but I can't see \nmany other useful combinations..\n\n> For example, you may want to enforce \"ug+rw,o=\" in a repository.  How\n> would you do that?\n\nIsn't that PERM_GROUP? The user always keeps u+rw for oneself.\n\n> Perhaps if you really wanted to have such a fine grained control, you\n> would be better off defining core.sharedrepository as a set/unset pair?\n> \n> \tcore.sharedrepository = 0660,007 ;# ug+rw,o-rwx\n> \n> Or even stronger \"set to this bit pattern\"?\n> \n> \tcore.sharedrepository = 0660 ;# ug=rw,o=\n\nThe latter approach is simpler and probably more understandable.\n\n> (I think you would need to flip executable bit for directories if you go\n> this route).\n\nI can put x flag mode checking.\n\nDo you accept these proposals? I can submit another patch.\n\n-- \nHeikki Orsila\t\t\tBarbie's law:\nheikki.orsila@iki.fi\t\t\"Math is hard, let's go shopping!\"\nhttp://www.iki.fi/shd\n"},{"id":"74177","messageId":"7vzlrzlluj.fsf@gitster.siamese.dyndns.org","threadId":"13061","inReplyTo":"20080412030021.GB31039@zakalwe.fi","subject":"Re: [PATCH] Add two core.sharedRepository options: group-readable and world-readable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-04-12T04:48:36Z","receivedAt":"2008-04-12T04:48:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Heikki Orsila <shdl@zakalwe.fi> writes:\n\n> On Fri, Apr 11, 2008 at 05:53:36PM -0700, Junio C Hamano wrote:\n> ...\n>> For example, you may want to enforce \"ug+rw,o=\" in a repository.  How\n>> would you do that?\n>\n> Isn't that PERM_GROUP? The user always keeps u+rw for oneself.\n\nMy question was about the \"o=\" part.  I did not see you dropping bits for\nothers in your patch.\n\nAnd if your answer is \"the user should have xx7 umask\", that defeats the\nwhole point of your patch, as you are trying to dissociate the umask used\nby the user for his usual task and enforce particular permission policy\nfor the repository.\n"},{"id":"74187","messageId":"20080412091723.GB20443@atjola.homenet","threadId":"13061","inReplyTo":"7vzlrzlluj.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Add two core.sharedRepository options: group-readable and world-readable","fromName":"Björn Steinbrink","fromEmail":"b.steinbrink@gmx.de","sentAt":"2008-04-12T09:17:23Z","receivedAt":"2008-04-12T09:17:23Z","isPatch":true,"sender":{"key":"b.steinbrink@gmx.de","avatar":"https://avatars.githubusercontent.com/u/230962?v=4"},"body":"On 2008.04.11 21:48:36 -0700, Junio C Hamano wrote:\n> Heikki Orsila <shdl@zakalwe.fi> writes:\n> \n> > On Fri, Apr 11, 2008 at 05:53:36PM -0700, Junio C Hamano wrote:\n> > ...\n> >> For example, you may want to enforce \"ug+rw,o=\" in a repository.  How\n> >> would you do that?\n> >\n> > Isn't that PERM_GROUP? The user always keeps u+rw for oneself.\n> \n> My question was about the \"o=\" part.  I did not see you dropping bits for\n> others in your patch.\n> \n> And if your answer is \"the user should have xx7 umask\", that defeats the\n> whole point of your patch, as you are trying to dissociate the umask used\n> by the user for his usual task and enforce particular permission policy\n> for the repository.\n\nI don't think it defeats the purpose of the patch. Currently, I guess\nthat for most users (umask like 0022 or 0077), both shared and all mean\nthat the umask is overriden in a way that grants more permissions on the\nrepository. As it is, we only support that in a way that grants write\npermissions to the group, while others may get read-only access (via\n\"all\"). From that point of view, I think that the patch is a natural\nenhancement, allowing to override the umask in a way that only grants\nadditional read permissions for either the group, or the group and\nothers. And that's exactly what Heikki was after.\n\nOf course, having a \"ignore the umask, use those permissions\" setting\nmight be nice, but short of that, the patch makes sense to me.\n\nBjörn\n"},{"id":"74190","messageId":"7vprsvie4x.fsf@gitster.siamese.dyndns.org","threadId":"13061","inReplyTo":"20080412091723.GB20443@atjola.homenet","subject":"Re: [PATCH] Add two core.sharedRepository options: group-readable and world-readable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-04-12T10:03:26Z","receivedAt":"2008-04-12T10:03:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Björn Steinbrink <B.Steinbrink@gmx.de> writes:\n\n> On 2008.04.11 21:48:36 -0700, Junio C Hamano wrote:\n>> ...\n>> My question was about the \"o=\" part.  I did not see you dropping bits for\n>> others in your patch.\n>> \n>> And if your answer is \"the user should have xx7 umask\", that defeats the\n>> whole point of your patch, as you are trying to dissociate the umask used\n>> by the user for his usual task and enforce particular permission policy\n>> for the repository.\n>\n> I don't think it defeats the purpose of the patch...\n> ... From that point of view, I think that the patch is a natural\n> enhancement, allowing to override the umask in a way that only grants\n> additional read permissions for either the group, or the group and\n> others. And that's exactly what Heikki was after.\n\nThat is exactly my point.\n\nIf a patch (cl)aims to improve things by overriding user's umask, why\nshould it limit itself only in the loosening direction?\n\nThe patch adds a handful new keywords allowed in the config area; if we\nwant to replace or enhance what Heikki's patch does later with a more\ngeneral mechanism that allows both loosening and tightening user's umask,\nthese keywords that represent \"a set of umask modifications that were\ndesigned with only loosening in mind\" can become maintenance burden, as we\nmust keep them for backward compatibility.\n\nThey may look small, but these things add up.  Even though incremental\nimprovement is possible and it generally is a much better way to enhance\nthe system, compared to an overengineered complexity that ends up being\nnot used by anybody, it is my job to bring it up as a potential issue when\nthere is a future enhancement possibilty (which may or may not be useful\nafter all) that may become more cumbersome to implement if we do this\nhalf-step now.\n\nFor this reason, I am not impressed by a \"this is a half-step and is\nbetter than the status quo\" as a supporting argument.  A supporting\nargument that says \"Theoretically tightening could also be useful, but it\nis much less so, compared to loosening which Heikki implemented.  The\nreason why tightening is not so useful is because of such-and-such.\nHence, it is reasonable to implement only loosening.\"  is much much more\nconvincing.\n\nPlease fill in the \"such-and-such\" above, convince me and others, so that\nI can apply Heikki's patch.\n\nI use quite a liberal umask myself for normal uses and a natural\nexpectation for a patch to improve in this area from me is to allow\napplying a tighter-than-usual mask for a particular set of repositories\nthat house my personal stuff.  One practical workaround I can think of is\nto just manually limit the access by running chmod at the toplevel of the\nrepository, so that _could_ be your \"such-and-such\".  Although it feels\nsomewhat hacky, I can live with it.\n"},{"id":"74196","messageId":"20080412120159.GC31039@zakalwe.fi","threadId":"13061","inReplyTo":"7vzlrzlluj.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Add two core.sharedRepository options: group-readable and world-readable","fromName":"Heikki Orsila","fromEmail":"shdl@zakalwe.fi","sentAt":"2008-04-12T12:01:59Z","receivedAt":"2008-04-12T12:01:59Z","isPatch":true,"sender":{"key":"shdl@zakalwe.fi","avatar":null},"body":"On Fri, Apr 11, 2008 at 09:48:36PM -0700, Junio C Hamano wrote:\n> Heikki Orsila <shdl@zakalwe.fi> writes:\n> \n> > On Fri, Apr 11, 2008 at 05:53:36PM -0700, Junio C Hamano wrote:\n> > ...\n> >> For example, you may want to enforce \"ug+rw,o=\" in a repository.  How\n> >> would you do that?\n> >\n> > Isn't that PERM_GROUP? The user always keeps u+rw for oneself.\n> \n> My question was about the \"o=\" part.  I did not see you dropping bits for\n> others in your patch.\n> \n> And if your answer is \"the user should have xx7 umask\", that defeats the\n> whole point of your patch, as you are trying to dissociate the umask used\n> by the user for his usual task and enforce particular permission policy\n> for the repository.\n\nYou're correct. There are two options, do you have a preference on this \nmatter?\n\n1. current method + mask off others when needed\n\n2. core.sharedRepository=0xxx\n\nI think I like option 2 better (preserving backwards compatibility of \ncourse).\n\n-- \nHeikki Orsila\t\t\tBarbie's law:\nheikki.orsila@iki.fi\t\t\"Math is hard, let's go shopping!\"\nhttp://www.iki.fi/shd\n"},{"id":"74197","messageId":"20080412120512.GD31039@zakalwe.fi","threadId":"13061","inReplyTo":"20080412120159.GC31039@zakalwe.fi","subject":"Re: [PATCH] Add two core.sharedRepository options: group-readable and world-readable","fromName":"Heikki Orsila","fromEmail":"shdl@zakalwe.fi","sentAt":"2008-04-12T12:05:12Z","receivedAt":"2008-04-12T12:05:12Z","isPatch":true,"sender":{"key":"shdl@zakalwe.fi","avatar":null},"body":"On Sat, Apr 12, 2008 at 03:01:59PM +0300, Heikki Orsila wrote:\n> You're correct. There are two options, do you have a preference on this \n> matter?\n> \n> 1. current method + mask off others when needed\n> \n> 2. core.sharedRepository=0xxx\n> \n> I think I like option 2 better (preserving backwards compatibility of \n> course).\n\nOkay, based on your later email. I think I will implement option 2.\n\n-- \nHeikki Orsila\t\t\tBarbie's law:\nheikki.orsila@iki.fi\t\t\"Math is hard, let's go shopping!\"\nhttp://www.iki.fi/shd\n"},{"id":"74222","messageId":"20080412185254.GE31039@zakalwe.fi","threadId":"13061","inReplyTo":"7vprsvie4x.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Add two core.sharedRepository options: group-readable and world-readable","fromName":"Heikki Orsila","fromEmail":"shdl@zakalwe.fi","sentAt":"2008-04-12T18:52:54Z","receivedAt":"2008-04-12T18:52:54Z","isPatch":true,"sender":{"key":"shdl@zakalwe.fi","avatar":null},"body":"On Sat, Apr 12, 2008 at 03:03:26AM -0700, Junio C Hamano wrote:\n> \"a set of umask modifications that were\n> designed with only loosening in mind\" can become maintenance burden, as we\n> must keep them for backward compatibility.\n\nThe latest patch, just posted a few minutes ago, does not add any new \nkeywords, but a generic 0xxx filemode value.\n\n-- \nHeikki Orsila\t\t\tBarbie's law:\nheikki.orsila@iki.fi\t\t\"Math is hard, let's go shopping!\"\nhttp://www.iki.fi/shd\n"}]}