[PATCH] git init: --bare/--shared overrides system/global config

[PATCH] git init: --bare/--shared overrides system/global config

[Deskin Miller]

>From b6144562983703079a1eba8cdac3506c18d751a3 Mon Sep 17 00:00:00 2001
From: Deskin Miller <deskinm@umich.edu>
Date: Sat, 4 Oct 2008 20:07:44 -0400

If core.bare or core.sharedRepository are set in /etc/gitconfig or
~/.gitconfig, then 'git init' will read the values when constructing a
new config file; reading them, however, will override the values
specified on the command line.  In the case of --bare, this ends up
causing a segfault, without the repository being properly initialised;
in the case of --shared, the permissions are set according to the
existing config settings, not what was specified on the command line.

This fix saves any specified values for --bare and --shared prior to
reading existing config settings, and restores them after reading but
before writing the new config file.

Also includes a testcase which has a specified global config file
override, demonstrating the former failure scenario.

Signed-off-by: Deskin Miller <deskinm@umich.edu>
---
I'm not a great fan of the method I took to save and restore the values
specified to init, but it works.  Also, I think the testcase is nice (but I'm
biased, seeing how I wrote it): in general I'd argue for more testcases which
deal with issues caused by the user's complete installation.

I based this off of maint, because I think it should be applied there, but it
applies cleanly to master if you feel that's better.
 builtin-init-db.c |    8 ++++++++
 t/t0001-init.sh   |   17 +++++++++++++++++
 2 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/builtin-init-db.c b/builtin-init-db.c
index 8140c12..38e282c 100644
--- a/builtin-init-db.c
+++ b/builtin-init-db.c
@@ -17,6 +17,9 @@
 #define TEST_FILEMODE 1
 #endif
 
+static int init_is_bare_repository = 0;
+static int init_shared_repository = PERM_UMASK;
+
 static void safe_create_dir(const char *dir, int share)
 {
 	if (mkdir(dir, 0777) < 0) {
@@ -191,6 +194,8 @@ static int create_default_files(const char *template_path)
 	copy_templates(template_path);
 
 	git_config(git_default_config, NULL);
+	is_bare_repository_cfg = init_is_bare_repository;
+	shared_repository = init_shared_repository;
 
 	/*
 	 * We would have created the above under user's umask -- under
@@ -277,6 +282,9 @@ int init_db(const char *template_dir, unsigned int flags)
 
 	safe_create_dir(get_git_dir(), 0);
 
+	init_is_bare_repository = is_bare_repository();
+	init_shared_repository = shared_repository;
+
 	/* Check to see if the repository version is right.
 	 * Note that a newly created repository does not have
 	 * config file, so this will not fail.  What we are catching
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index 620da5b..6a6bca0 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -167,4 +167,21 @@ test_expect_success 'init with --template (blank)' '
 	! test -f template-blank/.git/info/exclude
 '
 
+test_expect_success 'init --bare/--shared overrides system/global config' '
+	(
+		HOME="`pwd`" &&
+		export HOME &&
+		test_config="$HOME"/.gitconfig &&
+		unset GIT_CONFIG_NOGLOBAL &&
+		git config -f "$test_config" core.bare false &&
+		git config -f "$test_config" core.sharedRepository 0640 &&
+		mkdir init-bare-shared-override &&
+		cd init-bare-shared-override &&
+		git init --bare --shared=0666
+	) &&
+	check_config init-bare-shared-override true unset &&
+	test 0666 = \
+	`git config -f init-bare-shared-override/config core.sharedRepository`
+'
+
 test_done
-- 
1.6.0.2.307.gc427

Re: [PATCH] git init: --bare/--shared overrides system/global config

[Shawn O. Pearce]

Deskin Miller <deskinm@umich.edu> wrote:
> From b6144562983703079a1eba8cdac3506c18d751a3 Mon Sep 17 00:00:00 2001
> From: Deskin Miller <deskinm@umich.edu>
> Date: Sat, 4 Oct 2008 20:07:44 -0400

FWIW please don't include these lines in the commit message part
of the patch email.  The only reason to include a "From: blah" line
(the 2nd one above) is when the name and/or email address that you
want to attribute the change to differs from the name/email that
the message is sent from.  (E.g. sent from gmail.com but you want
to attribute to umich.edu.)
 
> If core.bare or core.sharedRepository are set in /etc/gitconfig or
> ~/.gitconfig, then 'git init' will read the values when constructing a
> new config file; [...]
> ---

Yikes.

> diff --git a/builtin-init-db.c b/builtin-init-db.c
> index 8140c12..38e282c 100644
> --- a/builtin-init-db.c
> +++ b/builtin-init-db.c
> @@ -191,6 +194,8 @@ static int create_default_files(const char *template_path)
>  	copy_templates(template_path);
>  
>  	git_config(git_default_config, NULL);
> +	is_bare_repository_cfg = init_is_bare_repository;
> +	shared_repository = init_shared_repository;

Is this really the right thing to do?  It seems like it would prevent
a user from setting core.sharedRepository = group in their template
and thus always have a shared repository on their system.

I think we should only be using the command line shared option if
it was supplied, but if it was not we should be honoring what we
recieved from git_config().

However I agree that is_bare shouldn't be inherited from the config.
Its a per-repository attribute and no matter what the user asked
for in their /etc/gitconfig or ~/.gitconfig we should correctly
set it for this current repository.
  
> diff --git a/t/t0001-init.sh b/t/t0001-init.sh
> index 620da5b..6a6bca0 100755
> --- a/t/t0001-init.sh
> +++ b/t/t0001-init.sh
> @@ -167,4 +167,21 @@ test_expect_success 'init with --template (blank)' '
>  	! test -f template-blank/.git/info/exclude
>  '
>  
> +test_expect_success 'init --bare/--shared overrides system/global config' '
> +	(
> +		HOME="`pwd`" &&
> +		export HOME &&
> +		test_config="$HOME"/.gitconfig &&
> +		unset GIT_CONFIG_NOGLOBAL &&
> +		git config -f "$test_config" core.bare false &&
> +		git config -f "$test_config" core.sharedRepository 0640 &&
> +		mkdir init-bare-shared-override &&
> +		cd init-bare-shared-override &&
> +		git init --bare --shared=0666
> +	) &&
> +	check_config init-bare-shared-override true unset &&
> +	test 0666 = \
> +	`git config -f init-bare-shared-override/config core.sharedRepository`
> +'

A second related test would be a ~/.gitconfig which sets
core.sharedRepository = 0666 and then does "git init".  I think
the right outcome is a repository which has that set.

-- 
Shawn.

Re: [PATCH v2] git init: --bare/--shared overrides system/global config

[Deskin Miller]

If core.bare or core.sharedRepository are set in /etc/gitconfig or
~/.gitconfig, then 'git init' will read the values when constructing a
new config file; reading them, however, will override the values
specified on the command line.  In the case of --bare, this ends up
causing a segfault, without the repository being properly initialised;
in the case of --shared, the permissions are set according to the
existing config settings, not what was specified on the command line.

This fix saves any specified values for --bare and --shared prior to
reading existing config settings, and restores them after reading but
before writing the new config file.  core.bare is ignored in all
situations, while core.sharedRepository will only be used if --shared
is not specified to git init.

Also includes testcases which use a specified global config file
override, demonstrating the former failure scenario.

Signed-off-by: Deskin Miller <deskinm@umich.edu>
---
On Mon, Oct 06, 2008 at 07:14:52AM -0700, Shawn O. Pearce wrote:
> Deskin Miller <deskinm@umich.edu> wrote:
> > From b6144562983703079a1eba8cdac3506c18d751a3 Mon Sep 17 00:00:00 2001
> > From: Deskin Miller <deskinm@umich.edu>
> > Date: Sat, 4 Oct 2008 20:07:44 -0400
> 
> FWIW please don't include these lines in the commit message part
> of the patch email. [...]

Alright, sorry for the messiness; I guess I thought preserving the original
commit date was important?  Won't happen again.

> > diff --git a/builtin-init-db.c b/builtin-init-db.c
> > index 8140c12..38e282c 100644
> > --- a/builtin-init-db.c
> > +++ b/builtin-init-db.c
> > @@ -191,6 +194,8 @@ static int create_default_files(const char *template_path)
> >  	copy_templates(template_path);
> >  
> >  	git_config(git_default_config, NULL);
> > +	is_bare_repository_cfg = init_is_bare_repository;
> > +	shared_repository = init_shared_repository;
> 
> Is this really the right thing to do?  It seems like it would prevent
> a user from setting core.sharedRepository = group in their template
> and thus always have a shared repository on their system.
 
You're right.  Fixed in this version: core.bare ignores any config, while
--shared will override config settings, but init will use the config settings
if --shared is not specified.
 
> A second related test would be a ~/.gitconfig which sets
> core.sharedRepository = 0666 and then does "git init".  I think
> the right outcome is a repository which has that set.

Good suggestion, I added such a case in this version.  My first version fails
this new testcase, while maint fails the original testcase I wrote.

 builtin-init-db.c |   12 ++++++++++--
 t/t0001-init.sh   |   32 ++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/builtin-init-db.c b/builtin-init-db.c
index 8140c12..d30c3fe 100644
--- a/builtin-init-db.c
+++ b/builtin-init-db.c
@@ -17,6 +17,9 @@
 #define TEST_FILEMODE 1
 #endif
 
+static int init_is_bare_repository = 0;
+static int init_shared_repository = -1;
+
 static void safe_create_dir(const char *dir, int share)
 {
 	if (mkdir(dir, 0777) < 0) {
@@ -191,6 +194,9 @@ static int create_default_files(const char *template_path)
 	copy_templates(template_path);
 
 	git_config(git_default_config, NULL);
+	is_bare_repository_cfg = init_is_bare_repository;
+	if (init_shared_repository != -1)
+		shared_repository = init_shared_repository;
 
 	/*
 	 * We would have created the above under user's umask -- under
@@ -277,6 +283,8 @@ int init_db(const char *template_dir, unsigned int flags)
 
 	safe_create_dir(get_git_dir(), 0);
 
+	init_is_bare_repository = is_bare_repository();
+
 	/* Check to see if the repository version is right.
 	 * Note that a newly created repository does not have
 	 * config file, so this will not fail.  What we are catching
@@ -381,9 +389,9 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
 			setenv(GIT_DIR_ENVIRONMENT, getcwd(git_dir,
 						sizeof(git_dir)), 0);
 		} else if (!strcmp(arg, "--shared"))
-			shared_repository = PERM_GROUP;
+			init_shared_repository = PERM_GROUP;
 		else if (!prefixcmp(arg, "--shared="))
-			shared_repository = git_config_perm("arg", arg+9);
+			init_shared_repository = git_config_perm("arg", arg+9);
 		else if (!strcmp(arg, "-q") || !strcmp(arg, "--quiet"))
 			flags |= INIT_DB_QUIET;
 		else
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index 620da5b..5ac0a27 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -167,4 +167,36 @@ test_expect_success 'init with --template (blank)' '
 	! test -f template-blank/.git/info/exclude
 '
 
+test_expect_success 'init --bare/--shared overrides system/global config' '
+	(
+		HOME="`pwd`" &&
+		export HOME &&
+		test_config="$HOME"/.gitconfig &&
+		unset GIT_CONFIG_NOGLOBAL &&
+		git config -f "$test_config" core.bare false &&
+		git config -f "$test_config" core.sharedRepository 0640 &&
+		mkdir init-bare-shared-override &&
+		cd init-bare-shared-override &&
+		git init --bare --shared=0666
+	) &&
+	check_config init-bare-shared-override true unset &&
+	test x0666 = \
+	x`git config -f init-bare-shared-override/config core.sharedRepository`
+'
+
+test_expect_success 'init honors global core.sharedRepository' '
+	(
+		HOME="`pwd`" &&
+		export HOME &&
+		test_config="$HOME"/.gitconfig &&
+		unset GIT_CONFIG_NOGLOBAL &&
+		git config -f "$test_config" core.sharedRepository 0666 &&
+		mkdir shared-honor-global &&
+		cd shared-honor-global &&
+		git init
+	) &&
+	test x0666 = \
+	x`git config -f shared-honor-global/.git/config core.sharedRepository`
+'
+
 test_done
-- 
1.6.0.2.307.gc427