{"thread":{"id":"43965","subject":"bug: 'core.logallrefupdates' is not set by default in non-bare repository","startedAt":"2016-08-31T08:06:34Z","lastAt":"2016-09-02T09:11:37Z","messageCount":8,"participants":["doak","Dennis Kaarsemaker","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"300631","messageId":"c46d36ef-3c2e-374f-0f2e-ffe31104e023@gmx.de","threadId":"43965","inReplyTo":null,"subject":"bug: 'core.logallrefupdates' is not set by default in non-bare repository","fromName":"doak","fromEmail":"doak@gmx.de","sentAt":"2016-08-31T08:09:44Z","receivedAt":"2016-08-31T08:06:34Z","isPatch":false,"sender":{"key":"doak@gmx.de","avatar":null},"body":"Hi there,\n\nIf 'git init /path/to/repo' is called inside another repository with 'core.logallrefupdates' set to true, it will be not set by default in the created repository.\nThis seems to be a bug.\nI am using Git v2.9.3 (Arch).\n\nSteps to reproduce:\n    ---------------------\n    git init t1\n    cd t1\n    # 'core.logallrefupdates' will not be set for 't2'.\n    git init ../t2\n    ---------------------\n\nStated from 'git-config(1)':\n    ---------------------\n    core.logAllRefUpdates\n        [...]\n        This value is true by default in a repository that has a working directory associated with it, and false by default in a bare repository.\n    ---------------------\n\n\nWith best regards,\ndoak\n\n\n"},{"id":"300670","messageId":"1472634746.4265.47.camel@kaarsemaker.net","threadId":"43965","inReplyTo":"c46d36ef-3c2e-374f-0f2e-ffe31104e023@gmx.de","subject":"Re: bug: 'core.logallrefupdates' is not set by default in non-bare repository","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2016-08-31T09:12:26Z","receivedAt":"2016-08-31T09:12:36Z","isPatch":false,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"That is indeed a bug. git reads the config of t1 and then thinks a\ntemplate config has set that value, so it won't override it.\n\nThis is caused by git init reading the config via\nget_shared_repository. The comment above it indicates that this may not\nbe needed, and indeed not doing it makes this bug go away.\n\ndiff --git a/builtin/init-db.c b/builtin/init-db.c\nindex 3a45f0b..b2883a6 100644\n--- a/builtin/init-db.c\n+++ b/builtin/init-db.c\n@@ -494,14 +494,6 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)\n        retry:\n                if (chdir(argv[0]) < 0) {\n                        if (!mkdir_tried) {\n-                               int saved;\n-                               /*\n-                                * At this point we haven't read any configuration,\n-                                * and we know shared_repository should always be 0;\n-                                * but just in case we play safe.\n-                                */\n-                               saved = get_shared_repository();\n-                               set_shared_repository(0);\n                                switch (safe_create_leading_directories_const(argv[0])) {\n                                case SCLD_OK:\n                                case SCLD_PERMS:\n@@ -513,7 +505,6 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)\n                                        die_errno(_(\"cannot mkdir %s\"), argv[0]);\n                                        break;\n                                }\n-                               set_shared_repository(saved);\n                                if (mkdir(argv[0], 0777) < 0)\n                                        die_errno(_(\"cannot mkdir %s\"), argv[0]);\n                                mkdir_tried = 1; \n\nOn wo, 2016-08-31 at 10:09 +0200, doak wrote:\n> Hi there,\n> \n> If 'git init /path/to/repo' is called inside another repository with\n> 'core.logallrefupdates' set to true, it will be not set by default in\n> the created repository.\n> This seems to be a bug.\n> I am using Git v2.9.3 (Arch).\n> \n> Steps to reproduce:\n>     ---------------------\n>     git init t1\n>     cd t1\n>     # 'core.logallrefupdates' will not be set for 't2'.\n>     git init ../t2\n>     ---------------------\n> \n> Stated from 'git-config(1)':\n>     ---------------------\n>     core.logAllRefUpdates\n>         [...]\n>         This value is true by default in a repository that has a\n> working directory associated with it, and false by default in a bare\n> repository.\n>     ---------------------\n> \n> \n> With best regards,\n> doak\n> \n> \n"},{"id":"300673","messageId":"20160831104825.quyqb54bo5k7fdxs@sigill.intra.peff.net","threadId":"43965","inReplyTo":"1472634746.4265.47.camel@kaarsemaker.net","subject":"Re: bug: 'core.logallrefupdates' is not set by default in non-bare repository","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-31T10:48:26Z","receivedAt":"2016-08-31T10:48:37Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 31, 2016 at 11:12:26AM +0200, Dennis Kaarsemaker wrote:\n\n> That is indeed a bug. git reads the config of t1 and then thinks a\n> template config has set that value, so it won't override it.\n\nThis is a regression in v2.9.0 due to my ae5f677 (lazily load\ncore.sharedrepository, 2016-03-11).\n\n> This is caused by git init reading the config via\n> get_shared_repository. The comment above it indicates that this may not\n> be needed, and indeed not doing it makes this bug go away.\n\nHrm. I'm not sure if that will work, though, because we may call\nget_shared_repository() from other code-paths (e.g., anything that calls\nadjust_shared_perm(), like safe_create_leading_directories).\n\nWe may need to do something like turn off the\nneed_shared_repository_from_config in init-db, since I think it would\nnot want to ever read from the default config sources in most of its\ncode-paths (OTOH, it should in theory respect core.sharedRepository in\n~/.gitconfig, so maybe there is another more elegant way of handling\nthis).\n\nI'm out of time for the day, so it will be a while before I can dig\nfurther. Please feel free to figure it out while I am sleeping. :)\n\n-Peff\n"},{"id":"300694","messageId":"1472657553.4265.63.camel@kaarsemaker.net","threadId":"43965","inReplyTo":"20160831104825.quyqb54bo5k7fdxs@sigill.intra.peff.net","subject":"Re: bug: 'core.logallrefupdates' is not set by default in non-bare repository","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2016-08-31T15:32:33Z","receivedAt":"2016-08-31T15:32:42Z","isPatch":false,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"On wo, 2016-08-31 at 06:48 -0400, Jeff King wrote:\n> On Wed, Aug 31, 2016 at 11:12:26AM +0200, Dennis Kaarsemaker wrote:\n> \n> > \n> > That is indeed a bug. git reads the config of t1 and then thinks a\n> > template config has set that value, so it won't override it.\n> This is a regression in v2.9.0 due to my ae5f677 (lazily load\n> core.sharedrepository, 2016-03-11).\n> \n> > \n> > This is caused by git init reading the config via\n> > get_shared_repository. The comment above it indicates that this may\n> > not\n> > be needed, and indeed not doing it makes this bug go away.\n> Hrm. I'm not sure if that will work, though, because we may call\n> get_shared_repository() from other code-paths (e.g., anything that\n> calls\n> adjust_shared_perm(), like safe_create_leading_directories).\n\nAgreed, the diff was more to point out what triggered this (so I could\nsend that and run away to spend the day with jr.) than an attempt at a\npatch.\n\n> We may need to do something like turn off the\n> need_shared_repository_from_config in init-db, since I think it would\n> not want to ever read from the default config sources in most of its\n> code-paths (OTOH, it should in theory respect core.sharedRepository\n> in ~/.gitconfig, so maybe there is another more elegant way of\n> handling this).\n\nI would go even further and say that git init should completely ignore\nthe config of a repository you happen to be in when creating a new\nrepository.\n\n> I'm out of time for the day, so it will be a while before I can dig\n> further. Please feel free to figure it out while I am sleeping. :)\n> \n> -Peff\n\nI hope you slept well :)\n\nThis is what I came up with to implement my suggestion above, comments\nwelcome.\n\ndiff --git a/builtin/init-db.c b/builtin/init-db.c\nindex 3a45f0b..d0fd3dc 100644\n--- a/builtin/init-db.c\n+++ b/builtin/init-db.c\n@@ -493,6 +493,12 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)\n \t\tint mkdir_tried = 0;\n \tretry:\n \t\tif (chdir(argv[0]) < 0) {\n+\t\t\t/*\n+\t\t\t * We're creating a new repository. If we're already in another\n+\t\t\t * repository, ignore its config\n+\t\t\t */\n+\t\t\tignore_repo_config = 1;\n+\t\t\tgit_config_clear();\n \t\t\tif (!mkdir_tried) {\n \t\t\t\tint saved;\n \t\t\t\t/*\n@@ -500,7 +506,6 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)\n \t\t\t\t * and we know shared_repository should always be 0;\n \t\t\t\t * but just in case we play safe.\n \t\t\t\t */\n-\t\t\t\tsaved = get_shared_repository();\n \t\t\t\tset_shared_repository(0);\n \t\t\t\tswitch (safe_create_leading_directories_const(argv[0])) {\n \t\t\t\tcase SCLD_OK:\n@@ -513,7 +518,6 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)\n \t\t\t\t\tdie_errno(_(\"cannot mkdir %s\"), argv[0]);\n \t\t\t\t\tbreak;\n \t\t\t\t}\n-\t\t\t\tset_shared_repository(saved);\n \t\t\t\tif (mkdir(argv[0], 0777) < 0)\n \t\t\t\t\tdie_errno(_(\"cannot mkdir %s\"), argv[0]);\n \t\t\t\tmkdir_tried = 1;\n@@ -524,6 +528,11 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)\n \t} else if (0 < argc) {\n \t\tusage(init_db_usage[0]);\n \t}\n+\n+\tneed_shared_repository_from_config = 1;\n+\tignore_repo_config = 0;\n+\tgit_config_clear();\n+\n \tif (is_bare_repository_cfg == 1) {\n \t\tchar *cwd = xgetcwd();\n \t\tsetenv(GIT_DIR_ENVIRONMENT, cwd, argc > 0);\ndiff --git a/cache.h b/cache.h\nindex f30a441..1be16fc 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -640,6 +640,8 @@ extern int hold_locked_index(struct lock_file *, int);\n extern void set_alternate_index_output(const char *);\n \n /* Environment bits from configuration mechanism */\n+extern int ignore_repo_config;\n+extern int need_shared_repository_from_config;\n extern int trust_executable_bit;\n extern int trust_ctime;\n extern int check_stat;\ndiff --git a/config.c b/config.c\nindex 0dfed68..2df0189 100644\n--- a/config.c\n+++ b/config.c\n@@ -1304,7 +1304,7 @@ static int do_git_config_sequence(config_fn_t fn, void *data)\n \t\tret += git_config_from_file(fn, user_config, data);\n \n \tcurrent_parsing_scope = CONFIG_SCOPE_REPO;\n-\tif (repo_config && !access_or_die(repo_config, R_OK, 0))\n+\tif (repo_config && !ignore_repo_config && !access_or_die(repo_config, R_OK, 0))\n \t\tret += git_config_from_file(fn, repo_config, data);\n \n \tcurrent_parsing_scope = CONFIG_SCOPE_CMDLINE;\ndiff --git a/environment.c b/environment.c\nindex ca72464..f6dbba8 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -12,6 +12,8 @@\n #include \"fmt-merge-msg.h\"\n #include \"commit.h\"\n \n+int ignore_repo_config = 0;\n+int need_shared_repository_from_config = 1;\n int trust_executable_bit = 1;\n int trust_ctime = 1;\n int check_stat = 1;\n@@ -326,7 +328,6 @@ const char *get_commit_output_encoding(void)\n }\n \n static int the_shared_repository = PERM_UMASK;\n-static int need_shared_repository_from_config = 1;\n \n void set_shared_repository(int value)\n {\n"},{"id":"300869","messageId":"20160902080416.jmrctu3onfmylmeq@sigill.intra.peff.net","threadId":"43965","inReplyTo":"1472657553.4265.63.camel@kaarsemaker.net","subject":"Re: bug: 'core.logallrefupdates' is not set by default in non-bare repository","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-02T08:04:17Z","receivedAt":"2016-09-02T08:04:26Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 31, 2016 at 05:32:33PM +0200, Dennis Kaarsemaker wrote:\n\n> > We may need to do something like turn off the\n> > need_shared_repository_from_config in init-db, since I think it would\n> > not want to ever read from the default config sources in most of its\n> > code-paths (OTOH, it should in theory respect core.sharedRepository\n> > in ~/.gitconfig, so maybe there is another more elegant way of\n> > handling this).\n> \n> I would go even further and say that git init should completely ignore\n> the config of a repository you happen to be in when creating a new\n> repository.\n\nHmm. I'd think we would already be avoiding that, because we shouldn't\nbe calling setup_git_directory(). But some of the lazy-loaded setup is a\nbit overzealous, and we blindly look at \".git/config\". If we try the\nsame operation from a subdir of an existing repo, we _don't_ end up\nconfused. Eek.\n\nSo I actually wonder if that is the root of the bug. In your patch, you\ndisable config reading when we chdir to the new repo:\n\n> diff --git a/builtin/init-db.c b/builtin/init-db.c\n> index 3a45f0b..d0fd3dc 100644\n> --- a/builtin/init-db.c\n> +++ b/builtin/init-db.c\n> @@ -493,6 +493,12 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)\n>  \t\tint mkdir_tried = 0;\n>  \tretry:\n>  \t\tif (chdir(argv[0]) < 0) {\n> +\t\t\t/*\n> +\t\t\t * We're creating a new repository. If we're already in another\n> +\t\t\t * repository, ignore its config\n> +\t\t\t */\n> +\t\t\tignore_repo_config = 1;\n> +\t\t\tgit_config_clear();\n\nBut I think we should go further and avoid ever looking at the original\nrepository in the first place. I.e., I would say that \"git init\" should\nnever ever behave differently if run in an existing repo versus outside\nof one.\n\nSo really we ought to be setting ignore_repo_config from the very start\nof cmd_init(), and then re-enabling it once we are \"inside\" the new\nrepo.  The git_config_clear() should in theory come once we are\n\"inside\", as well; we may have cached system/global config, and\nneed to flush so we read them anew along with the new local config.\n\nOTOH, since there shouldn't be anything interesting in the new\nrepo-level config, I'm not sure that's really necessary. The rest of\n\"init\" can probably proceed without caring.\n\nI also wonder if there are other things besides config which might\naccidentally read from .git (because they call git_pathdup(), and it\njust blindly looks in \".git\" if nobody called setup_git_directory()). So\nit would be nice to have some flag for \"do not ever lazy-call\nsetup_git_env; we do not care about any repository\".  But I think that's\nahrd; functions like git_pathdup() are always expected to return _some_\nvalue, so what would they say? The best we could do is return\n\"/does-not-exist/\" or something, but that is awfully hacky.\n\n> @@ -500,7 +506,6 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)\n>  \t\t\t\t * and we know shared_repository should always be 0;\n>  \t\t\t\t * but just in case we play safe.\n>  \t\t\t\t */\n> -\t\t\t\tsaved = get_shared_repository();\n>  \t\t\t\tset_shared_repository(0);\n>  \t\t\t\tswitch (safe_create_leading_directories_const(argv[0])) {\n>  \t\t\t\tcase SCLD_OK:\n\nI don't know if anybody cares about being able to set core.sharedRepository\nfrom ~/.gitconfig. It didn't work until v2.9.0 anyway (when I moved it\nout of the repository-format check), but it seems like you _should_ be\nable to set it and have it Just Work.\n\nAnd in that case, this \"we know shared_repository should always be 0\" is\nnot true, and we would want to keep doing the save/set-to-0/restore\ndance here.\n\n> @@ -524,6 +528,11 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)\n>  \t} else if (0 < argc) {\n>  \t\tusage(init_db_usage[0]);\n>  \t}\n> +\n> +\tneed_shared_repository_from_config = 1;\n> +\tignore_repo_config = 0;\n> +\tgit_config_clear();\n\nThis is the part I think we could actually skip. The only thing we might\nnot have loaded is the \"config\" we just wrote to the new repository. But\nI don't think we have to care about what is in it.\n\n> diff --git a/config.c b/config.c\n> index 0dfed68..2df0189 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -1304,7 +1304,7 @@ static int do_git_config_sequence(config_fn_t fn, void *data)\n>  \t\tret += git_config_from_file(fn, user_config, data);\n>  \n>  \tcurrent_parsing_scope = CONFIG_SCOPE_REPO;\n> -\tif (repo_config && !access_or_die(repo_config, R_OK, 0))\n> +\tif (repo_config && !ignore_repo_config && !access_or_die(repo_config, R_OK, 0))\n>  \t\tret += git_config_from_file(fn, repo_config, data);\n\nWe probably want to intercept the call to git_pathdup() earlier than\nthis, if the point is not to touch any of the lazy-load setup_git_dir()\nstuff at all. The effect is the same for config, but I think it makes\nsense to have as little effect as possible.\n\nSo here's the minimal fix that seems to work for me:\n\ndiff --git a/builtin/init-db.c b/builtin/init-db.c\nindex 3a45f0b..56e7b9a 100644\n--- a/builtin/init-db.c\n+++ b/builtin/init-db.c\n@@ -484,6 +484,8 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)\n \t\tOPT_END()\n \t};\n \n+\tignore_repo_config = 1;\n+\n \targc = parse_options(argc, argv, prefix, init_db_options, init_db_usage, 0);\n \n \tif (real_git_dir && !is_absolute_path(real_git_dir))\ndiff --git a/cache.h b/cache.h\nindex b780a91..13b78e4 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1582,6 +1582,13 @@ enum config_origin_type {\n \tCONFIG_ORIGIN_CMDLINE\n };\n \n+/*\n+ * If non-zero, git_config() will avoid any attempt to find the repo config;\n+ * this is useful for programs like git-init that might look at config before\n+ * actually setting up the new repository.\n+ */\n+extern int ignore_repo_config;\n+\n typedef int (*config_fn_t)(const char *, const char *, void *);\n extern int git_default_config(const char *, const char *, void *);\n extern int git_config_from_file(config_fn_t fn, const char *, void *);\ndiff --git a/config.c b/config.c\nindex 0dfed68..c9fc62e 100644\n--- a/config.c\n+++ b/config.c\n@@ -14,6 +14,8 @@\n #include \"string-list.h\"\n #include \"utf8.h\"\n \n+int ignore_repo_config;\n+\n struct config_source {\n \tstruct config_source *prev;\n \tunion {\n@@ -1289,7 +1291,7 @@ static int do_git_config_sequence(config_fn_t fn, void *data)\n \tint ret = 0;\n \tchar *xdg_config = xdg_config_home(\"config\");\n \tchar *user_config = expand_user_path(\"~/.gitconfig\");\n-\tchar *repo_config = git_pathdup(\"config\");\n+\tchar *repo_config = ignore_repo_config ? NULL : git_pathdup(\"config\");;\n \n \tcurrent_parsing_scope = CONFIG_SCOPE_SYSTEM;\n \tif (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0))\ndiff --git a/t/t0001-init.sh b/t/t0001-init.sh\nindex a6fdd5e..8efddaa 100755\n--- a/t/t0001-init.sh\n+++ b/t/t0001-init.sh\n@@ -384,4 +384,13 @@ test_expect_success MINGW 'bare git dir not hidden' '\n \t! is_hidden newdir\n '\n \n+test_expect_success 'init from existing directory does not confuse config' '\n+\trm -rf newdir &&\n+\ttest_config core.logallrefupdates true &&\n+\tgit init newdir &&\n+\techo true >expect &&\n+\tgit -C newdir config --bool core.logallrefupdates >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n\n\n"},{"id":"300873","messageId":"20160902084732.6yfbzipkqwwgk4yt@sigill.intra.peff.net","threadId":"43965","inReplyTo":"20160902080416.jmrctu3onfmylmeq@sigill.intra.peff.net","subject":"Re: bug: 'core.logallrefupdates' is not set by default in non-bare repository","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-02T08:47:32Z","receivedAt":"2016-09-02T08:47:39Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 02, 2016 at 04:04:16AM -0400, Jeff King wrote:\n\n> So here's the minimal fix that seems to work for me:\n> \n> diff --git a/builtin/init-db.c b/builtin/init-db.c\n> index 3a45f0b..56e7b9a 100644\n> --- a/builtin/init-db.c\n> +++ b/builtin/init-db.c\n\nI also wonder if \"clone\" should be doing something similar. Or, for that\nmatter, things like git-daemon that operate outside of a repo. They work\nnow because they do not happen to trigger any library calls which look\nat config under the hood.\n\nTraditionally these were supposed to just use git_config_early(), but\nthat's really not possible when the config calls are happening behind\nthe scenes (e.g., when lazy-loading the config cache). And so we\neventually got rid of git_config_early() entirely.\n\nBut I wonder if we could enforce that concept automatically for config.\n\nThe simple patch below does fix this case:\n\ndiff --git a/config.c b/config.c\nindex 0dfed68..b62bb40 100644\n--- a/config.c\n+++ b/config.c\n@@ -1289,7 +1289,7 @@ static int do_git_config_sequence(config_fn_t fn, void *data)\n \tint ret = 0;\n \tchar *xdg_config = xdg_config_home(\"config\");\n \tchar *user_config = expand_user_path(\"~/.gitconfig\");\n-\tchar *repo_config = git_pathdup(\"config\");\n+\tchar *repo_config = startup_info->have_repository ? git_pathdup(\"config\") : NULL;\n \n \tcurrent_parsing_scope = CONFIG_SCOPE_SYSTEM;\n \tif (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0))\n\nbut it causes a few test failures. Some of those are arguably\nreasonable, though. E.g., several of the diff tests use \"git diff\n--no-index\" and expect to read local config. But \"--no-index\" explicitly\n_avoids_ setting up the git repository, so the current code just falls\nback to reading \".git/config\". Which means it works when you are at the\ntop-level of a repository, but not in a subdir!\n\nSo I think this patch is an improvement; if we have not set up the\nrepository, then we should not be reading its config! (It's another\nquestion of whether --no-index should try setup_git_directory_gently(),\nbut then this patch would just do the right thing).\n\nI think \"hash-object\" without \"-w\" is in the same boat. It does not even\nbother looking for a git dir, but we assume that it can see config like\ncore.autocrlf. It works in the top-level, but not elsewhere:\n\n  $ git init\n  $ git config core.autocrlf true\n  $ printf 'foo\\r\\n' >file\n  $ git hash-object file\n  257cc5642cb1a054f08cc83f2d943e56fd3ebe99\n  $ mkdir subdir\n  $ cd subdir\n  $ git hash-object ../file\n  e48b03ece74f47d1ae20075200c64aeaa01a9cdb\n\nSo there is definitely some cleanup work, but it seems like it would be\nfixing a bunch of bugs.\n\nSome of the other failures are not so obvious. In particular, t7006\ntests the core.pager settings that are looked up before we set up the\ngit directory, and those are now broken. OTOH, I suspect that doing it\n_correctly_ would fix all of the known breakages like:\n\n  not ok 46 - git -p true - core.pager overrides PAGER from subdirectory\n\nThey are hitting that same subdirectory problem mentioned above.\n\n-Peff\n"},{"id":"300874","messageId":"1472806914.4680.50.camel@kaarsemaker.net","threadId":"43965","inReplyTo":"20160902080416.jmrctu3onfmylmeq@sigill.intra.peff.net","subject":"Re: bug: 'core.logallrefupdates' is not set by default in non-bare repository","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2016-09-02T09:01:54Z","receivedAt":"2016-09-02T09:02:04Z","isPatch":false,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"On vr, 2016-09-02 at 04:04 -0400, Jeff King wrote:\n> On Wed, Aug 31, 2016 at 05:32:33PM +0200, Dennis Kaarsemaker wrote:\n> \n> > \n> > > \n> > > We may need to do something like turn off the\n> > > need_shared_repository_from_config in init-db, since I think it would\n> > > not want to ever read from the default config sources in most of its\n> > > code-paths (OTOH, it should in theory respect core.sharedRepository\n> > > in ~/.gitconfig, so maybe there is another more elegant way of\n> > > handling this).\n> > I would go even further and say that git init should completely ignore\n> > the config of a repository you happen to be in when creating a new\n> > repository.\n> Hmm. I'd think we would already be avoiding that, because we shouldn't\n> be calling setup_git_directory(). But some of the lazy-loaded setup is a\n> bit overzealous, and we blindly look at \".git/config\". If we try the\n> same operation from a subdir of an existing repo, we _don't_ end up\n> confused. Eek.\n\nYikes. Didnt' dig that deep, but that sounds wrong :)\n\n> So I actually wonder if that is the root of the bug. In your patch, you\n> disable config reading when we chdir to the new repo:\n> \n> > \n> > diff --git a/builtin/init-db.c b/builtin/init-db.c\n> > index 3a45f0b..d0fd3dc 100644\n> > --- a/builtin/init-db.c\n> > +++ b/builtin/init-db.c\n> > @@ -493,6 +493,12 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)\n> >  \t\tint mkdir_tried = 0;\n> >  \tretry:\n> >  \t\tif (chdir(argv[0]) < 0) {\n> > +\t\t\t/*\n> > +\t\t\t * We're creating a new repository. If we're already in another\n> > +\t\t\t * repository, ignore its config\n> > +\t\t\t */\n> > +\t\t\tignore_repo_config = 1;\n> > +\t\t\tgit_config_clear();\n> But I think we should go further and avoid ever looking at the original\n> repository in the first place. I.e., I would say that \"git init\" should\n> never ever behave differently if run in an existing repo versus outside\n> of one.\n\nWell, 'git init' is a valid operation to run inside an existing repo to\nreinitialize some bits, so we definitely need to not ignore the config\nonce we're sure we're not creating a new repo.\n\n> So really we ought to be setting ignore_repo_config from the very start\n> of cmd_init(), and then re-enabling it once we are \"inside\" the new\n> repo.  The git_config_clear() should in theory come once we are\n> \"inside\", as well; we may have cached system/global config, and\n> need to flush so we read them anew along with the new local config.\n\nThat's why I git_config_clear() twice.\n\n> OTOH, since there shouldn't be anything interesting in the new\n> repo-level config, I'm not sure that's really necessary. The rest of\n> \"init\" can probably proceed without caring.\n\nExcept when running 'git init' to re-init existing repo.\n\n> I also wonder if there are other things besides config which might\n> accidentally read from .git (because they call git_pathdup(), and it\n> just blindly looks in \".git\" if nobody called setup_git_directory()). So\n> it would be nice to have some flag for \"do not ever lazy-call\n> setup_git_env; we do not care about any repository\".  But I think that's\n> ahrd; functions like git_pathdup() are always expected to return _some_\n> value, so what would they say? The best we could do is return\n> \"/does-not-exist/\" or something, but that is awfully hacky.\n> \n> > \n> > @@ -500,7 +506,6 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)\n> >  \t\t\t\t * and we know shared_repository should always be 0;\n> >  \t\t\t\t * but just in case we play safe.\n> >  \t\t\t\t */\n> > -\t\t\t\tsaved = get_shared_repository();\n> >  \t\t\t\tset_shared_repository(0);\n> >  \t\t\t\tswitch (safe_create_leading_directories_const(argv[0])) {\n> >  \t\t\t\tcase SCLD_OK:\n> I don't know if anybody cares about being able to set core.sharedRepository\n> from ~/.gitconfig. It didn't work until v2.9.0 anyway (when I moved it\n> out of the repository-format check), but it seems like you _should_ be\n> able to set it and have it Just Work.\n> \n> And in that case, this \"we know shared_repository should always be 0\" is\n> not true, and we would want to keep doing the save/set-to-0/restore\n> dance here.\n\nWe don't need to save if we throw away the cache below.\n\n> > @@ -524,6 +528,11 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)\n> >  \t} else if (0 < argc) {\n> >  \t\tusage(init_db_usage[0]);\n> >  \t}\n> > +\n> > +\tneed_shared_repository_from_config = 1;\n> > +\tignore_repo_config = 0;\n> > +\tgit_config_clear();\n> This is the part I think we could actually skip. The only thing we might\n> not have loaded is the \"config\" we just wrote to the new repository. But\n> I don't think we have to care about what is in it.\n\nWe do, because this is also called for existing repos.\n\n> > diff --git a/config.c b/config.c\n> > index 0dfed68..2df0189 100644\n> > --- a/config.c\n> > +++ b/config.c\n> > @@ -1304,7 +1304,7 @@ static int do_git_config_sequence(config_fn_t fn, void *data)\n> >  \t\tret += git_config_from_file(fn, user_config, data);\n> >  \n> >  \tcurrent_parsing_scope = CONFIG_SCOPE_REPO;\n> > -\tif (repo_config && !access_or_die(repo_config, R_OK, 0))\n> > +\tif (repo_config && !ignore_repo_config && !access_or_die(repo_config, R_OK, 0))\n> >  \t\tret += git_config_from_file(fn, repo_config, data);\n> We probably want to intercept the call to git_pathdup() earlier than\n> this, if the point is not to touch any of the lazy-load setup_git_dir()\n> stuff at all. The effect is the same for config, but I think it makes\n> sense to have as little effect as possible.\n\nThought about doing that, but didn't know what side-effects that would\nhave.\n\n> So here's the minimal fix that seems to work for me:\n> \n> diff --git a/builtin/init-db.c b/builtin/init-db.c\n> index 3a45f0b..56e7b9a 100644\n> --- a/builtin/init-db.c\n> +++ b/builtin/init-db.c\n> @@ -484,6 +484,8 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)\n>  \t\tOPT_END()\n>  \t};\n>  \n> +\tignore_repo_config = 1;\n> +\n>  \targc = parse_options(argc, argv, prefix, init_db_options, init_db_usage, 0);\n>  \n>  \tif (real_git_dir && !is_absolute_path(real_git_dir))\n> diff --git a/cache.h b/cache.h\n> index b780a91..13b78e4 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -1582,6 +1582,13 @@ enum config_origin_type {\n>  \tCONFIG_ORIGIN_CMDLINE\n>  };\n>  \n> +/*\n> + * If non-zero, git_config() will avoid any attempt to find the repo config;\n> + * this is useful for programs like git-init that might look at config before\n> + * actually setting up the new repository.\n> + */\n> +extern int ignore_repo_config;\n> +\n>  typedef int (*config_fn_t)(const char *, const char *, void *);\n>  extern int git_default_config(const char *, const char *, void *);\n>  extern int git_config_from_file(config_fn_t fn, const char *, void *);\n> diff --git a/config.c b/config.c\n> index 0dfed68..c9fc62e 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -14,6 +14,8 @@\n>  #include \"string-list.h\"\n>  #include \"utf8.h\"\n>  \n> +int ignore_repo_config;\n> +\n>  struct config_source {\n>  \tstruct config_source *prev;\n>  \tunion {\n> @@ -1289,7 +1291,7 @@ static int do_git_config_sequence(config_fn_t fn, void *data)\n>  \tint ret = 0;\n>  \tchar *xdg_config = xdg_config_home(\"config\");\n>  \tchar *user_config = expand_user_path(\"~/.gitconfig\");\n> -\tchar *repo_config = git_pathdup(\"config\");\n> +\tchar *repo_config = ignore_repo_config ? NULL : git_pathdup(\"config\");;\n>  \n>  \tcurrent_parsing_scope = CONFIG_SCOPE_SYSTEM;\n>  \tif (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0))\n> diff --git a/t/t0001-init.sh b/t/t0001-init.sh\n> index a6fdd5e..8efddaa 100755\n> --- a/t/t0001-init.sh\n> +++ b/t/t0001-init.sh\n> @@ -384,4 +384,13 @@ test_expect_success MINGW 'bare git dir not hidden' '\n>  \t! is_hidden newdir\n>  '\n>  \n> +test_expect_success 'init from existing directory does not confuse config' '\n> +\trm -rf newdir &&\n> +\ttest_config core.logallrefupdates true &&\n> +\tgit init newdir &&\n> +\techo true >expect &&\n> +\tgit -C newdir config --bool core.logallrefupdates >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n>  test_done\n> \n> \n"},{"id":"300876","messageId":"20160902091130.jxckdeomw35eewky@sigill.intra.peff.net","threadId":"43965","inReplyTo":"1472806914.4680.50.camel@kaarsemaker.net","subject":"Re: bug: 'core.logallrefupdates' is not set by default in non-bare repository","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-02T09:11:31Z","receivedAt":"2016-09-02T09:11:37Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 02, 2016 at 11:01:54AM +0200, Dennis Kaarsemaker wrote:\n\n> Well, 'git init' is a valid operation to run inside an existing repo to\n> reinitialize some bits, so we definitely need to not ignore the config\n> once we're sure we're not creating a new repo.\n\nGood point, I hadn't considered re-initializing.\n\nFor the follow-up patch I sent, where we check\nstartup_info->have_repository, I think the right thing would probably be\nto call setup_git_directory() after seeing we are in a re-init case.\nProbably even the \"gently\" form, as I think you can \"re-init\" a\npartially corrupted repository.\n\n> > > @@ -500,7 +506,6 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)\n> > >  \t\t\t\t * and we know shared_repository should always be 0;\n> > >  \t\t\t\t * but just in case we play safe.\n> > >  \t\t\t\t */\n> > > -\t\t\t\tsaved = get_shared_repository();\n> > >  \t\t\t\tset_shared_repository(0);\n> > >  \t\t\t\tswitch (safe_create_leading_directories_const(argv[0])) {\n> > >  \t\t\t\tcase SCLD_OK:\n> > I don't know if anybody cares about being able to set core.sharedRepository\n> > from ~/.gitconfig. It didn't work until v2.9.0 anyway (when I moved it\n> > out of the repository-format check), but it seems like you _should_ be\n> > able to set it and have it Just Work.\n> > \n> > And in that case, this \"we know shared_repository should always be 0\" is\n> > not true, and we would want to keep doing the save/set-to-0/restore\n> > dance here.\n> \n> We don't need to save if we throw away the cache below.\n\nYeah, you're right. Though I somehow lost my train of thought between\nthe two paragraphs. I was thinking that we would want to actually\nrespect the ~/.gitconfig setting for sharedrepository. Which would\nactually mean _dropping_ the save/zero/restore entirely, and just using\nthe value we get from the config. But I guess the point here is to avoid\ns_c_l_d creating \"shared\" leading directories that are outside any\nrepository. I could see an argument either way on whether that is the\nright thing to do when core.sharedRepository is set in ~/.gitconfig.\n\n-Peff\n"}]}