[PATCH] Implement selectable group ownership in git-init

[PATCH] Implement selectable group ownership in git-init

[Francesco Pretto]

Rationale: continuing the *nix tradition, git is very tied to fs permissions. Groups ownership of git repositories can in fact perfectly resemble projects work groups.
The problem came when sysadmins or git admins create shared repositories: it does not have sense to create repositories with :root group ownership, if you have to put users in the root group to let them use it (!). But the same stands for the administrative git:git user (if you have one): having all the commit users in the git group means that it's impossible to selectively give (or prevent) access to users to different projects. For this reason, git-init should give the possibility to create shared repositories of a selectable group. Moreover, it should warn the user if no specific group is provided, just to warn the user in the case of wrong usage pattern (like :root or :git repositories). The following patch implements this possibility (and the warning), please review it.

---
 builtin-init-db.c |   26 ++++++++++++++++++++++++--
 cache.h           |    2 ++
 environment.c     |    2 ++
 path.c            |    3 +++
 4 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/builtin-init-db.c b/builtin-init-db.c
index 763fa55..c8bed1e 100644
--- a/builtin-init-db.c
+++ b/builtin-init-db.c
@@ -321,7 +321,7 @@ static void guess_repository_type(const char *git_dir)
 }
 
 static const char init_db_usage[] =
-"git-init [-q | --quiet] [--template=<template-directory>] [--shared]";
+"git-init [-q | --quiet] [--template=<template-directory>] [--shared] [--group=<project-group>]";
 
 /*
  * If you want to, you can share the DB area with any number of branches.
@@ -346,6 +346,10 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
 			shared_repository = PERM_GROUP;
 		else if (!prefixcmp(arg, "--shared="))
 			shared_repository = git_config_perm("arg", arg+9);
+		else if (!prefixcmp(arg, "--group=")) {
+			owner_group = arg+8;
+			grouped_repository = 1;
+		}
 		else if (!strcmp(arg, "-q") || !strcmp(arg, "--quiet"))
 		        quiet = 1;
 		else
@@ -376,6 +380,20 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
 	}
 
 	/*
+	 * Complain if the repository is shared and no owner group have
+	 * been selected. 
+	 */
+	if (shared_repository && !grouped_repository)
+		printf("WARNING: You haven't selected any owner group!\n");
+	
+	/*
+	 * Catch the error early if the group provided doesn't exist
+	 */
+	if (getgrnam(owner_group) == NULL)
+		die("The group '%s' doesn't esist",
+		    owner_group);
+
+	/*
 	 * Set up the default .git directory contents
 	 */
 	git_dir = getenv(GIT_DIR_ENVIRONMENT);
@@ -417,11 +435,15 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
 		git_config_set("receive.denyNonFastforwards", "true");
 	}
 
-	if (!quiet)
+	if (!quiet) {
 		printf("%s%s Git repository in %s/\n",
 		       reinit ? "Reinitialized existing" : "Initialized empty",
 		       shared_repository ? " shared" : "",
 		       git_dir);
+		if (shared_repository)
+			printf("Put the commit users in the '%s' group\n",
+		       	       grouped_repository ? owner_group : getgrgid(getgid())->gr_name);
+	}
 
 	return 0;
 }
diff --git a/cache.h b/cache.h
index bfffa05..282b0f6 100644
--- a/cache.h
+++ b/cache.h
@@ -306,6 +306,8 @@ extern int prefer_symlink_refs;
 extern int log_all_ref_updates;
 extern int warn_ambiguous_refs;
 extern int shared_repository;
+extern int grouped_repository;
+extern const char *owner_group;
 extern const char *apply_default_whitespace;
 extern int zlib_compression_level;
 extern int core_compression_level;
diff --git a/environment.c b/environment.c
index b5a6c69..a518619 100644
--- a/environment.c
+++ b/environment.c
@@ -23,6 +23,8 @@ int repository_format_version;
 const char *git_commit_encoding;
 const char *git_log_output_encoding;
 int shared_repository = PERM_UMASK;
+int grouped_repository = 0;
+const char *owner_group = NULL;
 const char *apply_default_whitespace;
 int zlib_compression_level = Z_BEST_SPEED;
 int core_compression_level;
diff --git a/path.c b/path.c
index 4260952..1ec1379 100644
--- a/path.c
+++ b/path.c
@@ -286,6 +286,9 @@ int adjust_shared_perm(const char *path)
 		mode |= S_ISGID;
 	if ((mode & st.st_mode) != mode && chmod(path, mode) < 0)
 		return -2;
+	if (grouped_repository)
+		if (chown(path, getuid(), getgrnam(owner_group)->gr_gid) < 0 )
+			return -3;
 	return 0;
 }
 

Re: [PATCH] Implement selectable group ownership in git-init

[Francesco Pretto]

2007/11/3, Francesco Pretto <ceztkoml@gmail.com>:
> Rationale ...

Just to point, I wrote this patch for usability purpose and because i
think git still need a strong imprinting about its correct usage
pattern (i've read around that people using it with shared
repositories are already having problems with file permissions,
wrongly thinking it's inflexible for their needs). If you stand with
my reasoning, please help me to integrate this patch :-), I have other
ideas following this. NB: i'm not a C developer (that was my first
patch in pure C), have mercy...

The function adjust_shared_perm is used in other places: is there
other git commands that maybe needs to create files/dirs of a specific
group (not being in a g+sx dir)? Hopefully not, but i don't know git
source much. These are the occurrences (with the exclusion of
builtin-init-db.c):

refs.c: adjust_shared_perm(log_file);
refs.c: if (adjust_shared_perm(git_HEAD)) {
..
builtin-pack-objects.c: return adjust_shared_perm(path);
...
builtin-rerere.c:           (mkdir(rr_cache, 0777) ||
adjust_shared_perm(rr_cache)))
...
lockfile.c:             if (adjust_shared_perm(lk->filename))
...
sha1_file.c:            else if (adjust_shared_perm(path)) {

Re: [PATCH] Implement selectable group ownership in git-init

[Junio C Hamano]

Francesco Pretto <ceztkoml@gmail.com> writes:

> Rationale: continuing the *nix tradition, git is very tied to fs ...
> ... please review it.

It is impossible to read, let alone comment on, your proposed
commit log message with such a looooooong line.  Please split
the lines to reasonable length, as all other people do.

I wonder if "--group" is given, wouldn't it make sense to
default to "shared_repository = group" even without --shared
(alternatively, if only --group is given without --shared, you
could error out).

> diff --git a/builtin-init-db.c b/builtin-init-db.c
> index 763fa55..c8bed1e 100644
> --- a/builtin-init-db.c
> +++ b/builtin-init-db.c
> @@ -376,6 +380,20 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)

>  	/*
> +	 * Complain if the repository is shared and no owner group have
> +	 * been selected. 
> +	 */
> +	if (shared_repository && !grouped_repository)
> +		printf("WARNING: You haven't selected any owner group!\n");
> +	

I think it is wrong to give this warning when --group is not
given, and it is doubly wrong to give the warning to stdout.

> +	/*
> +	 * Catch the error early if the group provided doesn't exist
> +	 */

No need to make this into three lines.

> +	if (getgrnam(owner_group) == NULL)
> +		die("The group '%s' doesn't esist",
> +		    owner_group);

No need to split this into two lines.

> @@ -417,11 +435,15 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
>  		git_config_set("receive.denyNonFastforwards", "true");
>  	}
>  
> -	if (!quiet)
> +	if (!quiet) {
>  		printf("%s%s Git repository in %s/\n",
>  		       reinit ? "Reinitialized existing" : "Initialized empty",
>  		       shared_repository ? " shared" : "",
>  		       git_dir);
> +		if (shared_repository)
> +			printf("Put the commit users in the '%s' group\n",
> +		       	       grouped_repository ? owner_group : getgrgid(getgid())->gr_name);
> +	}

Only useful for the first time users; iow, too verbose for
common usage.

> diff --git a/environment.c b/environment.c
> index b5a6c69..a518619 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -23,6 +23,8 @@ int repository_format_version;
>  const char *git_commit_encoding;
>  const char *git_log_output_encoding;
>  int shared_repository = PERM_UMASK;
> +int grouped_repository = 0;
> +const char *owner_group = NULL;

Initialization to 0 and NULL should be left out, both for
readability and to keep these variables in BSS.

>  const char *apply_default_whitespace;
>  int zlib_compression_level = Z_BEST_SPEED;
>  int core_compression_level;
> diff --git a/path.c b/path.c
> index 4260952..1ec1379 100644
> --- a/path.c
> +++ b/path.c
> @@ -286,6 +286,9 @@ int adjust_shared_perm(const char *path)
>  		mode |= S_ISGID;
>  	if ((mode & st.st_mode) != mode && chmod(path, mode) < 0)
>  		return -2;
> +	if (grouped_repository)
> +		if (chown(path, getuid(), getgrnam(owner_group)->gr_gid) < 0 )
> +			return -3;
>  	return 0;
>  }

I suspect this is good only for init-db.  When normal codepaths
create a new file under .git/ and call adjust_shared_perm(), who
initializes owner_group and to what value with your patch?

The way the world works is that adjust_shared_perm() relies on a
new directory and/or file in .git/ being created in the same
group as its parent directory .git/ ways the case).  So it is
just the matter of:

	$ mkdir myproject.git
        $ chgrp projectgroup myproject.git
        $ GIT_DIR=myproject.git git init --shared

I think what the patch attempts to achieve may be good, but only
to reduce a few keystrokes of doing the "chgrp".  Is it really
worth it, I have to wonder...

Re: [PATCH] Implement selectable group ownership in git-init

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I suspect this is good only for init-db.  When normal codepaths
> create a new file under .git/ and call adjust_shared_perm(), who
> initializes owner_group and to what value with your patch?
>
> The way the world works is that adjust_shared_perm() relies on a
> new directory and/or file in .git/ being created in the same
> group as its parent directory .git/ ways the case).

sorry, did not finish editing before sending.

	s| \.git.*.$|.|;

Re: [PATCH] Implement selectable group ownership in git-init

[Wincent Colaiuta]

El 3/11/2007, a las 23:27, Junio C Hamano escribió:

> I think what the patch attempts to achieve may be good, but only
> to reduce a few keystrokes of doing the "chgrp".  Is it really
> worth it, I have to wonder...

I think this proposal adds unnecessary clutter to the codebase for  
something that can easily be achieved (and *should*) using chown,  
chgrp, or "sudo -u" etc.

The permissions of your Git installation and your repositories are  
completely outside of the domain of Git itself, and should be  
controlled using the administration tools designed for managing  
access, just like every other SCM (and every server, and every piece  
of software which can be accessed by many on a multi-user system).

Cheers,
Wincent

Re: [PATCH] Implement selectable group ownership in git-init

[Francesco Pretto]

Wincent Colaiuta ha scritto:
> 
> using the administration tools designed for managing access, just like
> every other SCM (and every server, and every piece of software which can
> be accessed by many on a multi-user system).
> 

I don't agree in general: in SCMs and other multi-user softwares, the access
control configuration can be safely postponed just because it's in their
standard usage pattern that the access should be conditioned by a daemon
to be configured later. It's not the case of git, just because git is very
tied to *nix permissions.
But as it is now, it could seems that it's good to put committers in the (for
example) git group, just because you have a git administrative account
git:git . This is caused, imo, by the fact that the flow of creating a shared
repository for a specific work/project group with git-init run by an
administrative user (as it should be) is something like this:

	- Do it wrong;
	- Fix it immediately.

I don't like the "Do it wrong" part. I'm trying to produce a sane and
transparent patch to implement the selectable group just in case of repository
first initialization. Why do I care so much of first time users? Dunno, but
I think it's important.

Re: [PATCH] Implement selectable group ownership in git-init

[Wincent Colaiuta]

El 5/11/2007, a las 16:09, Francesco Pretto escribió:

> Wincent Colaiuta ha scritto:
>>
>> using the administration tools designed for managing access, just  
>> like
>> every other SCM (and every server, and every piece of software  
>> which can
>> be accessed by many on a multi-user system).
>>
>
> I don't agree in general: in SCMs and other multi-user softwares,  
> the access
> control configuration can be safely postponed just because it's in  
> their
> standard usage pattern that the access should be conditioned by a  
> daemon
> to be configured later. It's not the case of git, just because git  
> is very
> tied to *nix permissions.
> But as it is now, it could seems that it's good to put committers in  
> the (for
> example) git group, just because you have a git administrative account
> git:git . This is caused, imo, by the fact that the flow of creating  
> a shared
> repository for a specific work/project group with git-init run by an
> administrative user (as it should be) is something like this:
>
> 	- Do it wrong;
> 	- Fix it immediately.
>
> I don't like the "Do it wrong" part. I'm trying to produce a sane and
> transparent patch to implement the selectable group just in case of  
> repository
> first initialization. Why do I care so much of first time users?  
> Dunno, but
> I think it's important.

What's stopping you from using "sudo -u"?

Wincent

Re: [PATCH] Implement selectable group ownership in git-init

[Francesco Pretto]

Wincent Colaiuta ha scritto:
> I think this proposal adds unnecessary clutter to the codebase for
> something that can easily be achieved (and *should*) using chown, chgrp,
> or "sudo -u" etc.
> 

While still not convinced about that "unnecessary", i had to admit the risk
of breaking something with my addition was too high. What about a compromise?
I have improved the documentation about shared repositories. A patch coming
shortly.

Francesco

Re: [PATCH] Implement selectable group ownership in git-init

[Wincent Colaiuta]

El 5/11/2007, a las 23:27, Francesco Pretto escribió:

> Wincent Colaiuta ha scritto:
>> I think this proposal adds unnecessary clutter to the codebase for
>> something that can easily be achieved (and *should*) using chown,  
>> chgrp,
>> or "sudo -u" etc.
>
> While still not convinced about that "unnecessary", i had to admit  
> the risk
> of breaking something with my addition was too high. What about a  
> compromise?
> I have improved the documentation about shared repositories. A patch  
> coming
> shortly.

Sounds like an excellent idea.

Cheers,
Wincent