Francesco Pretto <ceztkoml@xxxxxxxxx> 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...
-
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
