{"thread":{"id":"10625","subject":"[PATCH] Implement selectable group ownership in git-init","startedAt":"2007-11-03T19:05:26Z","lastAt":"2007-11-06T10:31:21Z","messageCount":9,"participants":["Francesco Pretto","Junio C Hamano","Wincent Colaiuta"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"58165","messageId":"472CC676.3000603@gmail.com","threadId":"10625","inReplyTo":null,"subject":"[PATCH] Implement selectable group ownership in git-init","fromName":"Francesco Pretto","fromEmail":"ceztkoml@gmail.com","sentAt":"2007-11-03T19:05:26Z","receivedAt":"2007-11-03T19:05:26Z","isPatch":true,"sender":{"key":"ceztkoml@gmail.com","avatar":null},"body":"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.\nThe 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.\n\n---\n builtin-init-db.c |   26 ++++++++++++++++++++++++--\n cache.h           |    2 ++\n environment.c     |    2 ++\n path.c            |    3 +++\n 4 files changed, 31 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin-init-db.c b/builtin-init-db.c\nindex 763fa55..c8bed1e 100644\n--- a/builtin-init-db.c\n+++ b/builtin-init-db.c\n@@ -321,7 +321,7 @@ static void guess_repository_type(const char *git_dir)\n }\n \n static const char init_db_usage[] =\n-\"git-init [-q | --quiet] [--template=<template-directory>] [--shared]\";\n+\"git-init [-q | --quiet] [--template=<template-directory>] [--shared] [--group=<project-group>]\";\n \n /*\n  * If you want to, you can share the DB area with any number of branches.\n@@ -346,6 +346,10 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)\n \t\t\tshared_repository = PERM_GROUP;\n \t\telse if (!prefixcmp(arg, \"--shared=\"))\n \t\t\tshared_repository = git_config_perm(\"arg\", arg+9);\n+\t\telse if (!prefixcmp(arg, \"--group=\")) {\n+\t\t\towner_group = arg+8;\n+\t\t\tgrouped_repository = 1;\n+\t\t}\n \t\telse if (!strcmp(arg, \"-q\") || !strcmp(arg, \"--quiet\"))\n \t\t        quiet = 1;\n \t\telse\n@@ -376,6 +380,20 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)\n \t}\n \n \t/*\n+\t * Complain if the repository is shared and no owner group have\n+\t * been selected. \n+\t */\n+\tif (shared_repository && !grouped_repository)\n+\t\tprintf(\"WARNING: You haven't selected any owner group!\\n\");\n+\t\n+\t/*\n+\t * Catch the error early if the group provided doesn't exist\n+\t */\n+\tif (getgrnam(owner_group) == NULL)\n+\t\tdie(\"The group '%s' doesn't esist\",\n+\t\t    owner_group);\n+\n+\t/*\n \t * Set up the default .git directory contents\n \t */\n \tgit_dir = getenv(GIT_DIR_ENVIRONMENT);\n@@ -417,11 +435,15 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)\n \t\tgit_config_set(\"receive.denyNonFastforwards\", \"true\");\n \t}\n \n-\tif (!quiet)\n+\tif (!quiet) {\n \t\tprintf(\"%s%s Git repository in %s/\\n\",\n \t\t       reinit ? \"Reinitialized existing\" : \"Initialized empty\",\n \t\t       shared_repository ? \" shared\" : \"\",\n \t\t       git_dir);\n+\t\tif (shared_repository)\n+\t\t\tprintf(\"Put the commit users in the '%s' group\\n\",\n+\t\t       \t       grouped_repository ? owner_group : getgrgid(getgid())->gr_name);\n+\t}\n \n \treturn 0;\n }\ndiff --git a/cache.h b/cache.h\nindex bfffa05..282b0f6 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -306,6 +306,8 @@ extern int prefer_symlink_refs;\n extern int log_all_ref_updates;\n extern int warn_ambiguous_refs;\n extern int shared_repository;\n+extern int grouped_repository;\n+extern const char *owner_group;\n extern const char *apply_default_whitespace;\n extern int zlib_compression_level;\n extern int core_compression_level;\ndiff --git a/environment.c b/environment.c\nindex b5a6c69..a518619 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -23,6 +23,8 @@ int repository_format_version;\n const char *git_commit_encoding;\n const char *git_log_output_encoding;\n int shared_repository = PERM_UMASK;\n+int grouped_repository = 0;\n+const char *owner_group = NULL;\n const char *apply_default_whitespace;\n int zlib_compression_level = Z_BEST_SPEED;\n int core_compression_level;\ndiff --git a/path.c b/path.c\nindex 4260952..1ec1379 100644\n--- a/path.c\n+++ b/path.c\n@@ -286,6 +286,9 @@ int adjust_shared_perm(const char *path)\n \t\tmode |= S_ISGID;\n \tif ((mode & st.st_mode) != mode && chmod(path, mode) < 0)\n \t\treturn -2;\n+\tif (grouped_repository)\n+\t\tif (chown(path, getuid(), getgrnam(owner_group)->gr_gid) < 0 )\n+\t\t\treturn -3;\n \treturn 0;\n }\n \n"},{"id":"58167","messageId":"b13782500711031236k6b4faef9oe5780cb39afe25db@mail.gmail.com","threadId":"10625","inReplyTo":"472CC676.3000603@gmail.com","subject":"Re: [PATCH] Implement selectable group ownership in git-init","fromName":"Francesco Pretto","fromEmail":"ceztkoml@gmail.com","sentAt":"2007-11-03T19:36:07Z","receivedAt":"2007-11-03T19:36:07Z","isPatch":true,"sender":{"key":"ceztkoml@gmail.com","avatar":null},"body":"2007/11/3, Francesco Pretto <ceztkoml@gmail.com>:\n> Rationale ...\n\nJust to point, I wrote this patch for usability purpose and because i\nthink git still need a strong imprinting about its correct usage\npattern (i've read around that people using it with shared\nrepositories are already having problems with file permissions,\nwrongly thinking it's inflexible for their needs). If you stand with\nmy reasoning, please help me to integrate this patch :-), I have other\nideas following this. NB: i'm not a C developer (that was my first\npatch in pure C), have mercy...\n\nThe function adjust_shared_perm is used in other places: is there\nother git commands that maybe needs to create files/dirs of a specific\ngroup (not being in a g+sx dir)? Hopefully not, but i don't know git\nsource much. These are the occurrences (with the exclusion of\nbuiltin-init-db.c):\n\nrefs.c: adjust_shared_perm(log_file);\nrefs.c: if (adjust_shared_perm(git_HEAD)) {\n..\nbuiltin-pack-objects.c: return adjust_shared_perm(path);\n...\nbuiltin-rerere.c:           (mkdir(rr_cache, 0777) ||\nadjust_shared_perm(rr_cache)))\n...\nlockfile.c:             if (adjust_shared_perm(lk->filename))\n...\nsha1_file.c:            else if (adjust_shared_perm(path)) {\n"},{"id":"58183","messageId":"7vabpvx8uu.fsf@gitster.siamese.dyndns.org","threadId":"10625","inReplyTo":"472CC676.3000603@gmail.com","subject":"Re: [PATCH] Implement selectable group ownership in git-init","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-11-03T22:27:53Z","receivedAt":"2007-11-03T22:27:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Francesco Pretto <ceztkoml@gmail.com> writes:\n\n> Rationale: continuing the *nix tradition, git is very tied to fs ...\n> ... please review it.\n\nIt is impossible to read, let alone comment on, your proposed\ncommit log message with such a looooooong line.  Please split\nthe lines to reasonable length, as all other people do.\n\nI wonder if \"--group\" is given, wouldn't it make sense to\ndefault to \"shared_repository = group\" even without --shared\n(alternatively, if only --group is given without --shared, you\ncould error out).\n\n> diff --git a/builtin-init-db.c b/builtin-init-db.c\n> index 763fa55..c8bed1e 100644\n> --- a/builtin-init-db.c\n> +++ b/builtin-init-db.c\n> @@ -376,6 +380,20 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)\n\n>  \t/*\n> +\t * Complain if the repository is shared and no owner group have\n> +\t * been selected. \n> +\t */\n> +\tif (shared_repository && !grouped_repository)\n> +\t\tprintf(\"WARNING: You haven't selected any owner group!\\n\");\n> +\t\n\nI think it is wrong to give this warning when --group is not\ngiven, and it is doubly wrong to give the warning to stdout.\n\n> +\t/*\n> +\t * Catch the error early if the group provided doesn't exist\n> +\t */\n\nNo need to make this into three lines.\n\n> +\tif (getgrnam(owner_group) == NULL)\n> +\t\tdie(\"The group '%s' doesn't esist\",\n> +\t\t    owner_group);\n\nNo need to split this into two lines.\n\n> @@ -417,11 +435,15 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)\n>  \t\tgit_config_set(\"receive.denyNonFastforwards\", \"true\");\n>  \t}\n>  \n> -\tif (!quiet)\n> +\tif (!quiet) {\n>  \t\tprintf(\"%s%s Git repository in %s/\\n\",\n>  \t\t       reinit ? \"Reinitialized existing\" : \"Initialized empty\",\n>  \t\t       shared_repository ? \" shared\" : \"\",\n>  \t\t       git_dir);\n> +\t\tif (shared_repository)\n> +\t\t\tprintf(\"Put the commit users in the '%s' group\\n\",\n> +\t\t       \t       grouped_repository ? owner_group : getgrgid(getgid())->gr_name);\n> +\t}\n\nOnly useful for the first time users; iow, too verbose for\ncommon usage.\n\n> diff --git a/environment.c b/environment.c\n> index b5a6c69..a518619 100644\n> --- a/environment.c\n> +++ b/environment.c\n> @@ -23,6 +23,8 @@ int repository_format_version;\n>  const char *git_commit_encoding;\n>  const char *git_log_output_encoding;\n>  int shared_repository = PERM_UMASK;\n> +int grouped_repository = 0;\n> +const char *owner_group = NULL;\n\nInitialization to 0 and NULL should be left out, both for\nreadability and to keep these variables in BSS.\n\n>  const char *apply_default_whitespace;\n>  int zlib_compression_level = Z_BEST_SPEED;\n>  int core_compression_level;\n> diff --git a/path.c b/path.c\n> index 4260952..1ec1379 100644\n> --- a/path.c\n> +++ b/path.c\n> @@ -286,6 +286,9 @@ int adjust_shared_perm(const char *path)\n>  \t\tmode |= S_ISGID;\n>  \tif ((mode & st.st_mode) != mode && chmod(path, mode) < 0)\n>  \t\treturn -2;\n> +\tif (grouped_repository)\n> +\t\tif (chown(path, getuid(), getgrnam(owner_group)->gr_gid) < 0 )\n> +\t\t\treturn -3;\n>  \treturn 0;\n>  }\n\nI suspect this is good only for init-db.  When normal codepaths\ncreate a new file under .git/ and call adjust_shared_perm(), who\ninitializes owner_group and to what value with your patch?\n\nThe way the world works is that adjust_shared_perm() relies on a\nnew directory and/or file in .git/ being created in the same\ngroup as its parent directory .git/ ways the case).  So it is\njust the matter of:\n\n\t$ mkdir myproject.git\n        $ chgrp projectgroup myproject.git\n        $ GIT_DIR=myproject.git git init --shared\n\nI think what the patch attempts to achieve may be good, but only\nto reduce a few keystrokes of doing the \"chgrp\".  Is it really\nworth it, I have to wonder...\n"},{"id":"58184","messageId":"7v640jx8o8.fsf@gitster.siamese.dyndns.org","threadId":"10625","inReplyTo":"7vabpvx8uu.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Implement selectable group ownership in git-init","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-11-03T22:31:51Z","receivedAt":"2007-11-03T22:31:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> I suspect this is good only for init-db.  When normal codepaths\n> create a new file under .git/ and call adjust_shared_perm(), who\n> initializes owner_group and to what value with your patch?\n>\n> The way the world works is that adjust_shared_perm() relies on a\n> new directory and/or file in .git/ being created in the same\n> group as its parent directory .git/ ways the case).\n\nsorry, did not finish editing before sending.\n\n\ts| \\.git.*.$|.|;\n"},{"id":"58382","messageId":"8EF5148D-C1F0-4329-A221-82D0B7E9932C@wincent.com","threadId":"10625","inReplyTo":"7vabpvx8uu.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Implement selectable group ownership in git-init","fromName":"Wincent Colaiuta","fromEmail":"win@wincent.com","sentAt":"2007-11-05T13:49:17Z","receivedAt":"2007-11-05T13:49:17Z","isPatch":true,"sender":{"key":"greg@hurrell.net","avatar":"https://avatars.githubusercontent.com/u/7074?v=4"},"body":"El 3/11/2007, a las 23:27, Junio C Hamano escribió:\n\n> I think what the patch attempts to achieve may be good, but only\n> to reduce a few keystrokes of doing the \"chgrp\".  Is it really\n> worth it, I have to wonder...\n\nI think this proposal adds unnecessary clutter to the codebase for  \nsomething that can easily be achieved (and *should*) using chown,  \nchgrp, or \"sudo -u\" etc.\n\nThe permissions of your Git installation and your repositories are  \ncompletely outside of the domain of Git itself, and should be  \ncontrolled using the administration tools designed for managing  \naccess, just like every other SCM (and every server, and every piece  \nof software which can be accessed by many on a multi-user system).\n\nCheers,\nWincent\n"},{"id":"58385","messageId":"472F3212.4090800@gmail.com","threadId":"10625","inReplyTo":"8EF5148D-C1F0-4329-A221-82D0B7E9932C@wincent.com","subject":"Re: [PATCH] Implement selectable group ownership in git-init","fromName":"Francesco Pretto","fromEmail":"ceztkoml@gmail.com","sentAt":"2007-11-05T15:09:06Z","receivedAt":"2007-11-05T15:09:06Z","isPatch":true,"sender":{"key":"ceztkoml@gmail.com","avatar":null},"body":"Wincent Colaiuta ha scritto:\n> \n> using the administration tools designed for managing access, just like\n> every other SCM (and every server, and every piece of software which can\n> be accessed by many on a multi-user system).\n> \n\nI don't agree in general: in SCMs and other multi-user softwares, the access\ncontrol configuration can be safely postponed just because it's in their\nstandard usage pattern that the access should be conditioned by a daemon\nto be configured later. It's not the case of git, just because git is very\ntied to *nix permissions.\nBut as it is now, it could seems that it's good to put committers in the (for\nexample) git group, just because you have a git administrative account\ngit:git . This is caused, imo, by the fact that the flow of creating a shared\nrepository for a specific work/project group with git-init run by an\nadministrative user (as it should be) is something like this:\n\n\t- Do it wrong;\n\t- Fix it immediately.\n\nI don't like the \"Do it wrong\" part. I'm trying to produce a sane and\ntransparent patch to implement the selectable group just in case of repository\nfirst initialization. Why do I care so much of first time users? Dunno, but\nI think it's important.\n"},{"id":"58386","messageId":"5C7AE6D3-5846-4E04-85CA-75B6C2108504@wincent.com","threadId":"10625","inReplyTo":"472F3212.4090800@gmail.com","subject":"Re: [PATCH] Implement selectable group ownership in git-init","fromName":"Wincent Colaiuta","fromEmail":"win@wincent.com","sentAt":"2007-11-05T15:26:36Z","receivedAt":"2007-11-05T15:26:36Z","isPatch":true,"sender":{"key":"greg@hurrell.net","avatar":"https://avatars.githubusercontent.com/u/7074?v=4"},"body":"El 5/11/2007, a las 16:09, Francesco Pretto escribió:\n\n> Wincent Colaiuta ha scritto:\n>>\n>> using the administration tools designed for managing access, just  \n>> like\n>> every other SCM (and every server, and every piece of software  \n>> which can\n>> be accessed by many on a multi-user system).\n>>\n>\n> I don't agree in general: in SCMs and other multi-user softwares,  \n> the access\n> control configuration can be safely postponed just because it's in  \n> their\n> standard usage pattern that the access should be conditioned by a  \n> daemon\n> to be configured later. It's not the case of git, just because git  \n> is very\n> tied to *nix permissions.\n> But as it is now, it could seems that it's good to put committers in  \n> the (for\n> example) git group, just because you have a git administrative account\n> git:git . This is caused, imo, by the fact that the flow of creating  \n> a shared\n> repository for a specific work/project group with git-init run by an\n> administrative user (as it should be) is something like this:\n>\n> \t- Do it wrong;\n> \t- Fix it immediately.\n>\n> I don't like the \"Do it wrong\" part. I'm trying to produce a sane and\n> transparent patch to implement the selectable group just in case of  \n> repository\n> first initialization. Why do I care so much of first time users?  \n> Dunno, but\n> I think it's important.\n\nWhat's stopping you from using \"sudo -u\"?\n\nWincent\n"},{"id":"58437","messageId":"472F98DE.2010406@gmail.com","threadId":"10625","inReplyTo":"8EF5148D-C1F0-4329-A221-82D0B7E9932C@wincent.com","subject":"Re: [PATCH] Implement selectable group ownership in git-init","fromName":"Francesco Pretto","fromEmail":"ceztkoml@gmail.com","sentAt":"2007-11-05T22:27:42Z","receivedAt":"2007-11-05T22:27:42Z","isPatch":true,"sender":{"key":"ceztkoml@gmail.com","avatar":null},"body":"Wincent Colaiuta ha scritto:\n> I think this proposal adds unnecessary clutter to the codebase for\n> something that can easily be achieved (and *should*) using chown, chgrp,\n> or \"sudo -u\" etc.\n> \n\nWhile still not convinced about that \"unnecessary\", i had to admit the risk\nof breaking something with my addition was too high. What about a compromise?\nI have improved the documentation about shared repositories. A patch coming\nshortly.\n\nFrancesco\n"},{"id":"58519","messageId":"143A8D66-66A3-4E71-A8FD-90254CD99C1E@wincent.com","threadId":"10625","inReplyTo":"472F98DE.2010406@gmail.com","subject":"Re: [PATCH] Implement selectable group ownership in git-init","fromName":"Wincent Colaiuta","fromEmail":"win@wincent.com","sentAt":"2007-11-06T10:31:21Z","receivedAt":"2007-11-06T10:31:21Z","isPatch":true,"sender":{"key":"greg@hurrell.net","avatar":"https://avatars.githubusercontent.com/u/7074?v=4"},"body":"El 5/11/2007, a las 23:27, Francesco Pretto escribió:\n\n> Wincent Colaiuta ha scritto:\n>> I think this proposal adds unnecessary clutter to the codebase for\n>> something that can easily be achieved (and *should*) using chown,  \n>> chgrp,\n>> or \"sudo -u\" etc.\n>\n> While still not convinced about that \"unnecessary\", i had to admit  \n> the risk\n> of breaking something with my addition was too high. What about a  \n> compromise?\n> I have improved the documentation about shared repositories. A patch  \n> coming\n> shortly.\n\nSounds like an excellent idea.\n\nCheers,\nWincent\n"}]}