[PATCH] Add two core.sharedRepository options: group-readable and world-readable

[PATCH] Add two core.sharedRepository options: group-readable and world-readable

[Heikki Orsila]

* 'group-readable': Make the repository group-readable (and g+sx for
  directories), even if the user's umask forbids it.

* 'world-readable': Make the repository readable for anyone, including
  the group (implies group-readable), even if the user's umask forbids
  it.

* Add a warning to cache.h that "enum sharedrepo" item order should
  not be changed because it would break backwards compatibility.

Signed-off-by: Heikki Orsila <heikki.orsila@iki.fi>
---
 Documentation/config.txt   |   13 ++++++++---
 Documentation/git-init.txt |    8 ++++++-
 cache.h                    |    6 ++++-
 path.c                     |   48 +++++++++++++++++++++++++++++--------------
 setup.c                    |    4 +++
 5 files changed, 57 insertions(+), 22 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index fe43b12..ee13b2b 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -258,10 +258,15 @@ core.repositoryFormatVersion::
 core.sharedRepository::
 	When 'group' (or 'true'), the repository is made shareable between
 	several users in a group (making sure all the files and objects are
-	group-writable). When 'all' (or 'world' or 'everybody'), the
-	repository will be readable by all users, additionally to being
-	group-shareable. When 'umask' (or 'false'), git will use permissions
-	reported by umask(2). See linkgit:git-init[1]. False by default.
+	group-writable). When 'group-readable', the repository will be
+	readable, but not writable, for users in the same group, even if the
+	user's umask forbids it. When 'all' (or 'world' or 'everybody'),
+	the repository will be readable by all users, additionally to being
+	group-shareable. When 'world-readable', the repository will be
+	readable for anyone, even if the user's umask forbids it. This option
+	implies 'group-readable'. When 'umask' (or 'false'), git will use
+	permissions reported by umask(2). See linkgit:git-init[1].
+	False by default.
 
 core.warnAmbiguousRefs::
 	If true, git will warn you if the ref name you passed it is ambiguous
diff --git a/Documentation/git-init.txt b/Documentation/git-init.txt
index 62914da..6bbc09c 100644
--- a/Documentation/git-init.txt
+++ b/Documentation/git-init.txt
@@ -31,7 +31,7 @@ structure, some suggested "exclude patterns", and copies of non-executing
 "hook" files.  The suggested patterns and hook files are all modifiable and
 extensible.
 
---shared[={false|true|umask|group|all|world|everybody}]::
+--shared[={false|true|umask|group|group-readable|all|world|world-readable|everybody}]::
 
 Specify that the git repository is to be shared amongst several users.  This
 allows users belonging to the same group to push into that
@@ -49,6 +49,12 @@ is given:
  - 'group' (or 'true'): Make the repository group-writable, (and g+sx, since
    the git group may be not the primary group of all users).
 
+ - 'group-readable': Make the repository group-readable (and g+sx for
+   directories), even if the user's umask forbids it.
+
+ - 'world-readable': Make the repository readable for anyone, including
+   the group (implies group-readable), even if the user's umask forbids it.
+
  - 'all' (or 'world' or 'everybody'): Same as 'group', but make the repository
    readable by all users.
 
diff --git a/cache.h b/cache.h
index 2a1e7ec..4af6d62 100644
--- a/cache.h
+++ b/cache.h
@@ -474,10 +474,14 @@ static inline void hashclr(unsigned char *hash)
 
 int git_mkstemp(char *path, size_t n, const char *template);
 
+/* Warning: enum sharedrepo item order should not be changed since it will
+ * break backwards compatibility. */
 enum sharedrepo {
 	PERM_UMASK = 0,
 	PERM_GROUP,
-	PERM_EVERYBODY
+	PERM_EVERYBODY,
+	PERM_GROUP_READABLE,
+	PERM_WORLD_READABLE,
 };
 int git_config_perm(const char *var, const char *value);
 int adjust_shared_perm(const char *path);
diff --git a/path.c b/path.c
index f4ed979..b900f62 100644
--- a/path.c
+++ b/path.c
@@ -266,22 +266,38 @@ int adjust_shared_perm(const char *path)
 	if (lstat(path, &st) < 0)
 		return -1;
 	mode = st.st_mode;
-	if (mode & S_IRUSR)
-		mode |= (shared_repository == PERM_GROUP
-			 ? S_IRGRP
-			 : (shared_repository == PERM_EVERYBODY
-			    ? (S_IRGRP|S_IROTH)
-			    : 0));
-
-	if (mode & S_IWUSR)
-		mode |= S_IWGRP;
-
-	if (mode & S_IXUSR)
-		mode |= (shared_repository == PERM_GROUP
-			 ? S_IXGRP
-			 : (shared_repository == PERM_EVERYBODY
-			    ? (S_IXGRP|S_IXOTH)
-			    : 0));
+
+	/* PERM_GROUP_READABLE:   g+r
+	 * PERM_GROUP:            g+rw
+	 * PERM_WORLD_READABLE:   g+r,  o+r
+	 * PERM_EVERYBODY:        g+rw, o+r
+	 */
+	if (mode & S_IRUSR) {
+		if (shared_repository == PERM_GROUP ||
+		    shared_repository == PERM_GROUP_READABLE) {
+			mode |= S_IRGRP;
+		} else if (shared_repository == PERM_EVERYBODY ||
+			   shared_repository == PERM_WORLD_READABLE) {
+			mode |= S_IRGRP | S_IROTH;
+		}
+	}
+
+	if (mode & S_IWUSR) {
+		if (shared_repository != PERM_GROUP_READABLE &&
+		    shared_repository != PERM_WORLD_READABLE)
+			mode |= S_IWGRP;
+	}
+
+	if (mode & S_IXUSR) {
+		if (shared_repository == PERM_GROUP ||
+		    shared_repository == PERM_GROUP_READABLE) {
+			mode |= S_IXGRP;
+		} else if (shared_repository == PERM_EVERYBODY ||
+			   shared_repository == PERM_WORLD_READABLE) {
+			mode |= S_IXGRP | S_IXOTH;
+		}
+	}
+
 	if (S_ISDIR(mode))
 		mode |= FORCE_DIR_SET_GID;
 	if ((mode & st.st_mode) != mode && chmod(path, mode) < 0)
diff --git a/setup.c b/setup.c
index 3d2d958..a33ae9d 100644
--- a/setup.c
+++ b/setup.c
@@ -434,10 +434,14 @@ int git_config_perm(const char *var, const char *value)
 			return PERM_UMASK;
 		if (!strcmp(value, "group"))
 			return PERM_GROUP;
+		if (!strcmp(value, "group-readable"))
+			return PERM_GROUP_READABLE;
 		if (!strcmp(value, "all") ||
 		    !strcmp(value, "world") ||
 		    !strcmp(value, "everybody"))
 			return PERM_EVERYBODY;
+		if (!strcmp(value, "world-readable"))
+			return PERM_WORLD_READABLE;
 		i = atoi(value);
 		if (i > 1)
 			return i;
-- 
1.5.4.4

Re: [PATCH] Add two core.sharedRepository options: group-readable and world-readable

[Heikki Orsila]

Sorry, this is not a proper reply to my previous mail. I just subscribed 
to the list.

I want to add this patch was a replacement for an earlier one. See

	[PATCH] Add two core.sharedRepository options: group-readable
	and world-readable
	http://marc.info/?l=git&m=120792433430899&w=2
and
	[PATCH] Add core.sharedRepository == read-only-group
	http://marc.info/?l=git&m=120777054707525&w=2

'read-only-group' was renamed to 'group-readable', and 'world-readable' 
option was added, as per comments of Bjorn Steinbrick.

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

Re: [PATCH] Add two core.sharedRepository options: group-readable and world-readable

[Junio C Hamano]

Heikki Orsila <heikki.orsila@iki.fi> writes:

> diff --git a/cache.h b/cache.h
> index 2a1e7ec..4af6d62 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -474,10 +474,14 @@ static inline void hashclr(unsigned char *hash)
>  
>  int git_mkstemp(char *path, size_t n, const char *template);
>  
> +/* Warning: enum sharedrepo item order should not be changed since it will
> + * break backwards compatibility. */

That's not a "Warning" (which tends to mean "you can violate this if you
know what you are doing"), but should be stronger than that.  Something
like (also notice the multi-line comment style --- the first line ends
with "/*\n"):

/*
 * NOTE NOTE NOTE!!
 *
 * Do not reorder this list; numerically written core.sharedrepository
 * in config files have always been valid, and you would break existing
 * repositories if you move these around.
 */

>  enum sharedrepo {
>  	PERM_UMASK = 0,
>  	PERM_GROUP,
> -	PERM_EVERYBODY
> +	PERM_EVERYBODY,
> +	PERM_GROUP_READABLE,
> +	PERM_WORLD_READABLE,
>  };

But I have to wonder if this patch is necessary.

Neither am I convinced if this set is sufficient.

+	/*
	 * PERM_GROUP_READABLE:   g+r
+	 * PERM_GROUP:            g+rw
+	 * PERM_WORLD_READABLE:   g+r,  o+r
+	 * PERM_EVERYBODY:        g+rw, o+r
+	 */

For example, you may want to enforce "ug+rw,o=" in a repository.  How
would you do that?

Perhaps if you really wanted to have such a fine grained control, you
would be better off defining core.sharedrepository as a set/unset pair?

	core.sharedrepository = 0660,007 ;# ug+rw,o-rwx

Or even stronger "set to this bit pattern"?

	core.sharedrepository = 0660 ;# ug=rw,o=

(I think you would need to flip executable bit for directories if you go
this route).

Re: [PATCH] Add two core.sharedRepository options: group-readable and world-readable

[Heikki Orsila]

On Fri, Apr 11, 2008 at 05:53:36PM -0700, Junio C Hamano wrote:
> That's not a "Warning" (which tends to mean "you can violate this if you
> know what you are doing"), but should be stronger than that.  Something
> like (also notice the multi-line comment style --- the first line ends
> with "/*\n"):
> 
> /*
>  * NOTE NOTE NOTE!!
>  *
>  * Do not reorder this list; numerically written core.sharedrepository
>  * in config files have always been valid, and you would break existing
>  * repositories if you move these around.
>  */

OK

> >  enum sharedrepo {
> >  	PERM_UMASK = 0,
> >  	PERM_GROUP,
> > -	PERM_EVERYBODY
> > +	PERM_EVERYBODY,
> > +	PERM_GROUP_READABLE,
> > +	PERM_WORLD_READABLE,
> >  };
> 
> But I have to wonder if this patch is necessary.

It is necessary for us, and I suspect it will be useful in some
organizations. The situation is this:

* multiple groups on the same machine
* users may want to keep umask 0077 as the default

> Neither am I convinced if this set is sufficient.

It will be easy to add new groups if the need arises, but I can't see 
many other useful combinations..

> For example, you may want to enforce "ug+rw,o=" in a repository.  How
> would you do that?

Isn't that PERM_GROUP? The user always keeps u+rw for oneself.

> Perhaps if you really wanted to have such a fine grained control, you
> would be better off defining core.sharedrepository as a set/unset pair?
> 
> 	core.sharedrepository = 0660,007 ;# ug+rw,o-rwx
> 
> Or even stronger "set to this bit pattern"?
> 
> 	core.sharedrepository = 0660 ;# ug=rw,o=

The latter approach is simpler and probably more understandable.

> (I think you would need to flip executable bit for directories if you go
> this route).

I can put x flag mode checking.

Do you accept these proposals? I can submit another patch.

-- 
Heikki Orsila			Barbie's law:
heikki.orsila@iki.fi		"Math is hard, let's go shopping!"
http://www.iki.fi/shd

Re: [PATCH] Add two core.sharedRepository options: group-readable and world-readable

[Junio C Hamano]

Heikki Orsila <shdl@zakalwe.fi> writes:

> On Fri, Apr 11, 2008 at 05:53:36PM -0700, Junio C Hamano wrote:
> ...
>> For example, you may want to enforce "ug+rw,o=" in a repository.  How
>> would you do that?
>
> Isn't that PERM_GROUP? The user always keeps u+rw for oneself.

My question was about the "o=" part.  I did not see you dropping bits for
others in your patch.

And if your answer is "the user should have xx7 umask", that defeats the
whole point of your patch, as you are trying to dissociate the umask used
by the user for his usual task and enforce particular permission policy
for the repository.

Re: [PATCH] Add two core.sharedRepository options: group-readable and world-readable

[Björn Steinbrink]

On 2008.04.11 21:48:36 -0700, Junio C Hamano wrote:
> Heikki Orsila <shdl@zakalwe.fi> writes:
> 
> > On Fri, Apr 11, 2008 at 05:53:36PM -0700, Junio C Hamano wrote:
> > ...
> >> For example, you may want to enforce "ug+rw,o=" in a repository.  How
> >> would you do that?
> >
> > Isn't that PERM_GROUP? The user always keeps u+rw for oneself.
> 
> My question was about the "o=" part.  I did not see you dropping bits for
> others in your patch.
> 
> And if your answer is "the user should have xx7 umask", that defeats the
> whole point of your patch, as you are trying to dissociate the umask used
> by the user for his usual task and enforce particular permission policy
> for the repository.

I don't think it defeats the purpose of the patch. Currently, I guess
that for most users (umask like 0022 or 0077), both shared and all mean
that the umask is overriden in a way that grants more permissions on the
repository. As it is, we only support that in a way that grants write
permissions to the group, while others may get read-only access (via
"all"). From that point of view, I think that the patch is a natural
enhancement, allowing to override the umask in a way that only grants
additional read permissions for either the group, or the group and
others. And that's exactly what Heikki was after.

Of course, having a "ignore the umask, use those permissions" setting
might be nice, but short of that, the patch makes sense to me.

Björn

Re: [PATCH] Add two core.sharedRepository options: group-readable and world-readable

[Junio C Hamano]

Björn Steinbrink <B.Steinbrink@gmx.de> writes:

> On 2008.04.11 21:48:36 -0700, Junio C Hamano wrote:
>> ...
>> My question was about the "o=" part.  I did not see you dropping bits for
>> others in your patch.
>> 
>> And if your answer is "the user should have xx7 umask", that defeats the
>> whole point of your patch, as you are trying to dissociate the umask used
>> by the user for his usual task and enforce particular permission policy
>> for the repository.
>
> I don't think it defeats the purpose of the patch...
> ... From that point of view, I think that the patch is a natural
> enhancement, allowing to override the umask in a way that only grants
> additional read permissions for either the group, or the group and
> others. And that's exactly what Heikki was after.

That is exactly my point.

If a patch (cl)aims to improve things by overriding user's umask, why
should it limit itself only in the loosening direction?

The patch adds a handful new keywords allowed in the config area; if we
want to replace or enhance what Heikki's patch does later with a more
general mechanism that allows both loosening and tightening user's umask,
these keywords that represent "a set of umask modifications that were
designed with only loosening in mind" can become maintenance burden, as we
must keep them for backward compatibility.

They may look small, but these things add up.  Even though incremental
improvement is possible and it generally is a much better way to enhance
the system, compared to an overengineered complexity that ends up being
not used by anybody, it is my job to bring it up as a potential issue when
there is a future enhancement possibilty (which may or may not be useful
after all) that may become more cumbersome to implement if we do this
half-step now.

For this reason, I am not impressed by a "this is a half-step and is
better than the status quo" as a supporting argument.  A supporting
argument that says "Theoretically tightening could also be useful, but it
is much less so, compared to loosening which Heikki implemented.  The
reason why tightening is not so useful is because of such-and-such.
Hence, it is reasonable to implement only loosening."  is much much more
convincing.

Please fill in the "such-and-such" above, convince me and others, so that
I can apply Heikki's patch.

I use quite a liberal umask myself for normal uses and a natural
expectation for a patch to improve in this area from me is to allow
applying a tighter-than-usual mask for a particular set of repositories
that house my personal stuff.  One practical workaround I can think of is
to just manually limit the access by running chmod at the toplevel of the
repository, so that _could_ be your "such-and-such".  Although it feels
somewhat hacky, I can live with it.

Re: [PATCH] Add two core.sharedRepository options: group-readable and world-readable

[Heikki Orsila]

On Fri, Apr 11, 2008 at 09:48:36PM -0700, Junio C Hamano wrote:
> Heikki Orsila <shdl@zakalwe.fi> writes:
> 
> > On Fri, Apr 11, 2008 at 05:53:36PM -0700, Junio C Hamano wrote:
> > ...
> >> For example, you may want to enforce "ug+rw,o=" in a repository.  How
> >> would you do that?
> >
> > Isn't that PERM_GROUP? The user always keeps u+rw for oneself.
> 
> My question was about the "o=" part.  I did not see you dropping bits for
> others in your patch.
> 
> And if your answer is "the user should have xx7 umask", that defeats the
> whole point of your patch, as you are trying to dissociate the umask used
> by the user for his usual task and enforce particular permission policy
> for the repository.

You're correct. There are two options, do you have a preference on this 
matter?

1. current method + mask off others when needed

2. core.sharedRepository=0xxx

I think I like option 2 better (preserving backwards compatibility of 
course).

-- 
Heikki Orsila			Barbie's law:
heikki.orsila@iki.fi		"Math is hard, let's go shopping!"
http://www.iki.fi/shd

Re: [PATCH] Add two core.sharedRepository options: group-readable and world-readable

[Heikki Orsila]

On Sat, Apr 12, 2008 at 03:01:59PM +0300, Heikki Orsila wrote:
> You're correct. There are two options, do you have a preference on this 
> matter?
> 
> 1. current method + mask off others when needed
> 
> 2. core.sharedRepository=0xxx
> 
> I think I like option 2 better (preserving backwards compatibility of 
> course).

Okay, based on your later email. I think I will implement option 2.

-- 
Heikki Orsila			Barbie's law:
heikki.orsila@iki.fi		"Math is hard, let's go shopping!"
http://www.iki.fi/shd

Re: [PATCH] Add two core.sharedRepository options: group-readable and world-readable

[Heikki Orsila]

On Sat, Apr 12, 2008 at 03:03:26AM -0700, Junio C Hamano wrote:
> "a set of umask modifications that were
> designed with only loosening in mind" can become maintenance burden, as we
> must keep them for backward compatibility.

The latest patch, just posted a few minutes ago, does not add any new 
keywords, but a generic 0xxx filemode value.

-- 
Heikki Orsila			Barbie's law:
heikki.orsila@iki.fi		"Math is hard, let's go shopping!"
http://www.iki.fi/shd