{"thread":{"id":"15793","subject":"[PATCH] git init: --bare/--shared overrides system/global config","startedAt":"2008-10-05T19:44:12Z","lastAt":"2008-10-07T05:37:48Z","messageCount":3,"participants":["Deskin Miller","Shawn O. Pearce"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"92354","messageId":"20081005194412.GE3052@riemann.deskinm.fdns.net","threadId":"15793","inReplyTo":null,"subject":"[PATCH] git init: --bare/--shared overrides system/global config","fromName":"Deskin Miller","fromEmail":"deskinm@umich.edu","sentAt":"2008-10-05T19:44:12Z","receivedAt":"2008-10-05T19:44:12Z","isPatch":true,"sender":{"key":"deskinm@umich.edu","avatar":"https://gravatar.com/avatar/d340a0e612cdf0a79535c71863c0b4c535e9aba63b42032226ae903e638b64f9?d=mp&s=160"},"body":">From b6144562983703079a1eba8cdac3506c18d751a3 Mon Sep 17 00:00:00 2001\nFrom: Deskin Miller <deskinm@umich.edu>\nDate: Sat, 4 Oct 2008 20:07:44 -0400\n\nIf core.bare or core.sharedRepository are set in /etc/gitconfig or\n~/.gitconfig, then 'git init' will read the values when constructing a\nnew config file; reading them, however, will override the values\nspecified on the command line.  In the case of --bare, this ends up\ncausing a segfault, without the repository being properly initialised;\nin the case of --shared, the permissions are set according to the\nexisting config settings, not what was specified on the command line.\n\nThis fix saves any specified values for --bare and --shared prior to\nreading existing config settings, and restores them after reading but\nbefore writing the new config file.\n\nAlso includes a testcase which has a specified global config file\noverride, demonstrating the former failure scenario.\n\nSigned-off-by: Deskin Miller <deskinm@umich.edu>\n---\nI'm not a great fan of the method I took to save and restore the values\nspecified to init, but it works.  Also, I think the testcase is nice (but I'm\nbiased, seeing how I wrote it): in general I'd argue for more testcases which\ndeal with issues caused by the user's complete installation.\n\nI based this off of maint, because I think it should be applied there, but it\napplies cleanly to master if you feel that's better.\n builtin-init-db.c |    8 ++++++++\n t/t0001-init.sh   |   17 +++++++++++++++++\n 2 files changed, 25 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin-init-db.c b/builtin-init-db.c\nindex 8140c12..38e282c 100644\n--- a/builtin-init-db.c\n+++ b/builtin-init-db.c\n@@ -17,6 +17,9 @@\n #define TEST_FILEMODE 1\n #endif\n \n+static int init_is_bare_repository = 0;\n+static int init_shared_repository = PERM_UMASK;\n+\n static void safe_create_dir(const char *dir, int share)\n {\n \tif (mkdir(dir, 0777) < 0) {\n@@ -191,6 +194,8 @@ static int create_default_files(const char *template_path)\n \tcopy_templates(template_path);\n \n \tgit_config(git_default_config, NULL);\n+\tis_bare_repository_cfg = init_is_bare_repository;\n+\tshared_repository = init_shared_repository;\n \n \t/*\n \t * We would have created the above under user's umask -- under\n@@ -277,6 +282,9 @@ int init_db(const char *template_dir, unsigned int flags)\n \n \tsafe_create_dir(get_git_dir(), 0);\n \n+\tinit_is_bare_repository = is_bare_repository();\n+\tinit_shared_repository = shared_repository;\n+\n \t/* Check to see if the repository version is right.\n \t * Note that a newly created repository does not have\n \t * config file, so this will not fail.  What we are catching\ndiff --git a/t/t0001-init.sh b/t/t0001-init.sh\nindex 620da5b..6a6bca0 100755\n--- a/t/t0001-init.sh\n+++ b/t/t0001-init.sh\n@@ -167,4 +167,21 @@ test_expect_success 'init with --template (blank)' '\n \t! test -f template-blank/.git/info/exclude\n '\n \n+test_expect_success 'init --bare/--shared overrides system/global config' '\n+\t(\n+\t\tHOME=\"`pwd`\" &&\n+\t\texport HOME &&\n+\t\ttest_config=\"$HOME\"/.gitconfig &&\n+\t\tunset GIT_CONFIG_NOGLOBAL &&\n+\t\tgit config -f \"$test_config\" core.bare false &&\n+\t\tgit config -f \"$test_config\" core.sharedRepository 0640 &&\n+\t\tmkdir init-bare-shared-override &&\n+\t\tcd init-bare-shared-override &&\n+\t\tgit init --bare --shared=0666\n+\t) &&\n+\tcheck_config init-bare-shared-override true unset &&\n+\ttest 0666 = \\\n+\t`git config -f init-bare-shared-override/config core.sharedRepository`\n+'\n+\n test_done\n-- \n1.6.0.2.307.gc427\n"},{"id":"92407","messageId":"20081006141452.GA7684@spearce.org","threadId":"15793","inReplyTo":"20081005194412.GE3052@riemann.deskinm.fdns.net","subject":"Re: [PATCH] git init: --bare/--shared overrides system/global config","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-10-06T14:14:52Z","receivedAt":"2008-10-06T14:14:52Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Deskin Miller <deskinm@umich.edu> wrote:\n> From b6144562983703079a1eba8cdac3506c18d751a3 Mon Sep 17 00:00:00 2001\n> From: Deskin Miller <deskinm@umich.edu>\n> Date: Sat, 4 Oct 2008 20:07:44 -0400\n\nFWIW please don't include these lines in the commit message part\nof the patch email.  The only reason to include a \"From: blah\" line\n(the 2nd one above) is when the name and/or email address that you\nwant to attribute the change to differs from the name/email that\nthe message is sent from.  (E.g. sent from gmail.com but you want\nto attribute to umich.edu.)\n \n> If core.bare or core.sharedRepository are set in /etc/gitconfig or\n> ~/.gitconfig, then 'git init' will read the values when constructing a\n> new config file; [...]\n> ---\n\nYikes.\n\n> diff --git a/builtin-init-db.c b/builtin-init-db.c\n> index 8140c12..38e282c 100644\n> --- a/builtin-init-db.c\n> +++ b/builtin-init-db.c\n> @@ -191,6 +194,8 @@ static int create_default_files(const char *template_path)\n>  \tcopy_templates(template_path);\n>  \n>  \tgit_config(git_default_config, NULL);\n> +\tis_bare_repository_cfg = init_is_bare_repository;\n> +\tshared_repository = init_shared_repository;\n\nIs this really the right thing to do?  It seems like it would prevent\na user from setting core.sharedRepository = group in their template\nand thus always have a shared repository on their system.\n\nI think we should only be using the command line shared option if\nit was supplied, but if it was not we should be honoring what we\nrecieved from git_config().\n\nHowever I agree that is_bare shouldn't be inherited from the config.\nIts a per-repository attribute and no matter what the user asked\nfor in their /etc/gitconfig or ~/.gitconfig we should correctly\nset it for this current repository.\n  \n> diff --git a/t/t0001-init.sh b/t/t0001-init.sh\n> index 620da5b..6a6bca0 100755\n> --- a/t/t0001-init.sh\n> +++ b/t/t0001-init.sh\n> @@ -167,4 +167,21 @@ test_expect_success 'init with --template (blank)' '\n>  \t! test -f template-blank/.git/info/exclude\n>  '\n>  \n> +test_expect_success 'init --bare/--shared overrides system/global config' '\n> +\t(\n> +\t\tHOME=\"`pwd`\" &&\n> +\t\texport HOME &&\n> +\t\ttest_config=\"$HOME\"/.gitconfig &&\n> +\t\tunset GIT_CONFIG_NOGLOBAL &&\n> +\t\tgit config -f \"$test_config\" core.bare false &&\n> +\t\tgit config -f \"$test_config\" core.sharedRepository 0640 &&\n> +\t\tmkdir init-bare-shared-override &&\n> +\t\tcd init-bare-shared-override &&\n> +\t\tgit init --bare --shared=0666\n> +\t) &&\n> +\tcheck_config init-bare-shared-override true unset &&\n> +\ttest 0666 = \\\n> +\t`git config -f init-bare-shared-override/config core.sharedRepository`\n> +'\n\nA second related test would be a ~/.gitconfig which sets\ncore.sharedRepository = 0666 and then does \"git init\".  I think\nthe right outcome is a repository which has that set.\n\n-- \nShawn.\n"},{"id":"92486","messageId":"20081007053748.GF3052@riemann.deskinm.fdns.net","threadId":"15793","inReplyTo":"20081006141452.GA7684@spearce.org","subject":"Re: [PATCH v2] git init: --bare/--shared overrides system/global config","fromName":"Deskin Miller","fromEmail":"deskinm@umich.edu","sentAt":"2008-10-07T05:37:48Z","receivedAt":"2008-10-07T05:37:48Z","isPatch":true,"sender":{"key":"deskinm@umich.edu","avatar":"https://gravatar.com/avatar/d340a0e612cdf0a79535c71863c0b4c535e9aba63b42032226ae903e638b64f9?d=mp&s=160"},"body":"If core.bare or core.sharedRepository are set in /etc/gitconfig or\n~/.gitconfig, then 'git init' will read the values when constructing a\nnew config file; reading them, however, will override the values\nspecified on the command line.  In the case of --bare, this ends up\ncausing a segfault, without the repository being properly initialised;\nin the case of --shared, the permissions are set according to the\nexisting config settings, not what was specified on the command line.\n\nThis fix saves any specified values for --bare and --shared prior to\nreading existing config settings, and restores them after reading but\nbefore writing the new config file.  core.bare is ignored in all\nsituations, while core.sharedRepository will only be used if --shared\nis not specified to git init.\n\nAlso includes testcases which use a specified global config file\noverride, demonstrating the former failure scenario.\n\nSigned-off-by: Deskin Miller <deskinm@umich.edu>\n---\nOn Mon, Oct 06, 2008 at 07:14:52AM -0700, Shawn O. Pearce wrote:\n> Deskin Miller <deskinm@umich.edu> wrote:\n> > From b6144562983703079a1eba8cdac3506c18d751a3 Mon Sep 17 00:00:00 2001\n> > From: Deskin Miller <deskinm@umich.edu>\n> > Date: Sat, 4 Oct 2008 20:07:44 -0400\n> \n> FWIW please don't include these lines in the commit message part\n> of the patch email. [...]\n\nAlright, sorry for the messiness; I guess I thought preserving the original\ncommit date was important?  Won't happen again.\n\n> > diff --git a/builtin-init-db.c b/builtin-init-db.c\n> > index 8140c12..38e282c 100644\n> > --- a/builtin-init-db.c\n> > +++ b/builtin-init-db.c\n> > @@ -191,6 +194,8 @@ static int create_default_files(const char *template_path)\n> >  \tcopy_templates(template_path);\n> >  \n> >  \tgit_config(git_default_config, NULL);\n> > +\tis_bare_repository_cfg = init_is_bare_repository;\n> > +\tshared_repository = init_shared_repository;\n> \n> Is this really the right thing to do?  It seems like it would prevent\n> a user from setting core.sharedRepository = group in their template\n> and thus always have a shared repository on their system.\n \nYou're right.  Fixed in this version: core.bare ignores any config, while\n--shared will override config settings, but init will use the config settings\nif --shared is not specified.\n \n> A second related test would be a ~/.gitconfig which sets\n> core.sharedRepository = 0666 and then does \"git init\".  I think\n> the right outcome is a repository which has that set.\n\nGood suggestion, I added such a case in this version.  My first version fails\nthis new testcase, while maint fails the original testcase I wrote.\n\n builtin-init-db.c |   12 ++++++++++--\n t/t0001-init.sh   |   32 ++++++++++++++++++++++++++++++++\n 2 files changed, 42 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin-init-db.c b/builtin-init-db.c\nindex 8140c12..d30c3fe 100644\n--- a/builtin-init-db.c\n+++ b/builtin-init-db.c\n@@ -17,6 +17,9 @@\n #define TEST_FILEMODE 1\n #endif\n \n+static int init_is_bare_repository = 0;\n+static int init_shared_repository = -1;\n+\n static void safe_create_dir(const char *dir, int share)\n {\n \tif (mkdir(dir, 0777) < 0) {\n@@ -191,6 +194,9 @@ static int create_default_files(const char *template_path)\n \tcopy_templates(template_path);\n \n \tgit_config(git_default_config, NULL);\n+\tis_bare_repository_cfg = init_is_bare_repository;\n+\tif (init_shared_repository != -1)\n+\t\tshared_repository = init_shared_repository;\n \n \t/*\n \t * We would have created the above under user's umask -- under\n@@ -277,6 +283,8 @@ int init_db(const char *template_dir, unsigned int flags)\n \n \tsafe_create_dir(get_git_dir(), 0);\n \n+\tinit_is_bare_repository = is_bare_repository();\n+\n \t/* Check to see if the repository version is right.\n \t * Note that a newly created repository does not have\n \t * config file, so this will not fail.  What we are catching\n@@ -381,9 +389,9 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)\n \t\t\tsetenv(GIT_DIR_ENVIRONMENT, getcwd(git_dir,\n \t\t\t\t\t\tsizeof(git_dir)), 0);\n \t\t} else if (!strcmp(arg, \"--shared\"))\n-\t\t\tshared_repository = PERM_GROUP;\n+\t\t\tinit_shared_repository = PERM_GROUP;\n \t\telse if (!prefixcmp(arg, \"--shared=\"))\n-\t\t\tshared_repository = git_config_perm(\"arg\", arg+9);\n+\t\t\tinit_shared_repository = git_config_perm(\"arg\", arg+9);\n \t\telse if (!strcmp(arg, \"-q\") || !strcmp(arg, \"--quiet\"))\n \t\t\tflags |= INIT_DB_QUIET;\n \t\telse\ndiff --git a/t/t0001-init.sh b/t/t0001-init.sh\nindex 620da5b..5ac0a27 100755\n--- a/t/t0001-init.sh\n+++ b/t/t0001-init.sh\n@@ -167,4 +167,36 @@ test_expect_success 'init with --template (blank)' '\n \t! test -f template-blank/.git/info/exclude\n '\n \n+test_expect_success 'init --bare/--shared overrides system/global config' '\n+\t(\n+\t\tHOME=\"`pwd`\" &&\n+\t\texport HOME &&\n+\t\ttest_config=\"$HOME\"/.gitconfig &&\n+\t\tunset GIT_CONFIG_NOGLOBAL &&\n+\t\tgit config -f \"$test_config\" core.bare false &&\n+\t\tgit config -f \"$test_config\" core.sharedRepository 0640 &&\n+\t\tmkdir init-bare-shared-override &&\n+\t\tcd init-bare-shared-override &&\n+\t\tgit init --bare --shared=0666\n+\t) &&\n+\tcheck_config init-bare-shared-override true unset &&\n+\ttest x0666 = \\\n+\tx`git config -f init-bare-shared-override/config core.sharedRepository`\n+'\n+\n+test_expect_success 'init honors global core.sharedRepository' '\n+\t(\n+\t\tHOME=\"`pwd`\" &&\n+\t\texport HOME &&\n+\t\ttest_config=\"$HOME\"/.gitconfig &&\n+\t\tunset GIT_CONFIG_NOGLOBAL &&\n+\t\tgit config -f \"$test_config\" core.sharedRepository 0666 &&\n+\t\tmkdir shared-honor-global &&\n+\t\tcd shared-honor-global &&\n+\t\tgit init\n+\t) &&\n+\ttest x0666 = \\\n+\tx`git config -f shared-honor-global/.git/config core.sharedRepository`\n+'\n+\n test_done\n-- \n1.6.0.2.307.gc427\n"}]}