git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: bug: 'core.logallrefupdates' is not set by default in non-bare repository

From
Jeff King <peff@peff.net>
Date
Sep 2, 2016, 08:04 UTC
Message-ID
<20160902080416.jmrctu3onfmylmeq@sigill.intra.peff.net>
In-Reply-To
<1472657553.4265.63.camel@kaarsemaker.net>
On Wed, Aug 31, 2016 at 05:32:33PM +0200, Dennis Kaarsemaker wrote:
Show 10 quoted lines
> > We may need to do something like turn off the
> > need_shared_repository_from_config in init-db, since I think it would
> > not want to ever read from the default config sources in most of its
> > code-paths (OTOH, it should in theory respect core.sharedRepository
> > in ~/.gitconfig, so maybe there is another more elegant way of
> > handling this).
> 
> I would go even further and say that git init should completely ignore
> the config of a repository you happen to be in when creating a new
> repository.

Hmm. I'd think we would already be avoiding that, because we shouldn't be calling setup_git_directory(). But some of the lazy-loaded setup is a bit overzealous, and we blindly look at ".git/config". If we try the same operation from a subdir of an existing repo, we _don't_ end up confused. Eek.

So I actually wonder if that is the root of the bug. In your patch, you disable config reading when we chdir to the new repo:

Show 14 quoted lines
> diff --git a/builtin/init-db.c b/builtin/init-db.c
> index 3a45f0b..d0fd3dc 100644
> --- a/builtin/init-db.c
> +++ b/builtin/init-db.c
> @@ -493,6 +493,12 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
>  		int mkdir_tried = 0;
>  	retry:
>  		if (chdir(argv[0]) < 0) {
> +			/*
> +			 * We're creating a new repository. If we're already in another
> +			 * repository, ignore its config
> +			 */
> +			ignore_repo_config = 1;
> +			git_config_clear();

But I think we should go further and avoid ever looking at the original repository in the first place. I.e., I would say that "git init" should never ever behave differently if run in an existing repo versus outside of one.

So really we ought to be setting ignore_repo_config from the very start of cmd_init(), and then re-enabling it once we are "inside" the new repo. The git_config_clear() should in theory come once we are "inside", as well; we may have cached system/global config, and need to flush so we read them anew along with the new local config.

OTOH, since there shouldn't be anything interesting in the new repo-level config, I'm not sure that's really necessary. The rest of "init" can probably proceed without caring.

I also wonder if there are other things besides config which might accidentally read from .git (because they call git_pathdup(), and it just blindly looks in ".git" if nobody called setup_git_directory()). So it would be nice to have some flag for "do not ever lazy-call setup_git_env; we do not care about any repository". But I think that's ahrd; functions like git_pathdup() are always expected to return _some_ value, so what would they say? The best we could do is return "/does-not-exist/" or something, but that is awfully hacky.

Show 8 quoted lines
> @@ -500,7 +506,6 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
>  				 * and we know shared_repository should always be 0;
>  				 * but just in case we play safe.
>  				 */
> -				saved = get_shared_repository();
>  				set_shared_repository(0);
>  				switch (safe_create_leading_directories_const(argv[0])) {
>  				case SCLD_OK:

I don't know if anybody cares about being able to set core.sharedRepository from ~/.gitconfig. It didn't work until v2.9.0 anyway (when I moved it out of the repository-format check), but it seems like you _should_ be able to set it and have it Just Work.

And in that case, this "we know shared_repository should always be 0" is not true, and we would want to keep doing the save/set-to-0/restore dance here.

Show 8 quoted lines
> @@ -524,6 +528,11 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
>  	} else if (0 < argc) {
>  		usage(init_db_usage[0]);
>  	}
> +
> +	need_shared_repository_from_config = 1;
> +	ignore_repo_config = 0;
> +	git_config_clear();

This is the part I think we could actually skip. The only thing we might not have loaded is the "config" we just wrote to the new repository. But I don't think we have to care about what is in it.

Show 11 quoted lines
> diff --git a/config.c b/config.c
> index 0dfed68..2df0189 100644
> --- a/config.c
> +++ b/config.c
> @@ -1304,7 +1304,7 @@ static int do_git_config_sequence(config_fn_t fn, void *data)
>  		ret += git_config_from_file(fn, user_config, data);
>  
>  	current_parsing_scope = CONFIG_SCOPE_REPO;
> -	if (repo_config && !access_or_die(repo_config, R_OK, 0))
> +	if (repo_config && !ignore_repo_config && !access_or_die(repo_config, R_OK, 0))
>  		ret += git_config_from_file(fn, repo_config, data);

We probably want to intercept the call to git_pathdup() earlier than this, if the point is not to touch any of the lazy-load setup_git_dir() stuff at all. The effect is the same for config, but I think it makes sense to have as little effect as possible.

So here's the minimal fix that seems to work for me:
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 3a45f0b..56e7b9a 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -484,6 +484,8 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
+	ignore_repo_config = 1;
+
 	argc = parse_options(argc, argv, prefix, init_db_options, init_db_usage, 0);
 
 	if (real_git_dir && !is_absolute_path(real_git_dir))
diff --git a/cache.h b/cache.h
index b780a91..13b78e4 100644
--- a/cache.h
+++ b/cache.h
@@ -1582,6 +1582,13 @@ enum config_origin_type {
 	CONFIG_ORIGIN_CMDLINE
 };
 
+/*
+ * If non-zero, git_config() will avoid any attempt to find the repo config;
+ * this is useful for programs like git-init that might look at config before
+ * actually setting up the new repository.
+ */
+extern int ignore_repo_config;
+
 typedef int (*config_fn_t)(const char *, const char *, void *);
 extern int git_default_config(const char *, const char *, void *);
 extern int git_config_from_file(config_fn_t fn, const char *, void *);
diff --git a/config.c b/config.c
index 0dfed68..c9fc62e 100644
--- a/config.c
+++ b/config.c
@@ -14,6 +14,8 @@
 #include "string-list.h"
 #include "utf8.h"
 
+int ignore_repo_config;
+
 struct config_source {
 	struct config_source *prev;
 	union {
@@ -1289,7 +1291,7 @@ static int do_git_config_sequence(config_fn_t fn, void *data)
 	int ret = 0;
 	char *xdg_config = xdg_config_home("config");
 	char *user_config = expand_user_path("~/.gitconfig");
-	char *repo_config = git_pathdup("config");
+	char *repo_config = ignore_repo_config ? NULL : git_pathdup("config");;
 
 	current_parsing_scope = CONFIG_SCOPE_SYSTEM;
 	if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0))
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index a6fdd5e..8efddaa 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -384,4 +384,13 @@ test_expect_success MINGW 'bare git dir not hidden' '
 	! is_hidden newdir
 '
 
+test_expect_success 'init from existing directory does not confuse config' '
+	rm -rf newdir &&
+	test_config core.logallrefupdates true &&
+	git init newdir &&
+	echo true >expect &&
+	git -C newdir config --bool core.logallrefupdates >actual &&
+	test_cmp expect actual
+'
+
 test_done
Previous: Dennis KaarsemakerNext: Jeff King
Message 5 of 8 in “bug: 'core.logallrefupdates' is not set by default in non-bare repository”
  1. doakAug 31, 2016
  2. Dennis KaarsemakerAug 31, 2016
  3. Jeff KingAug 31, 2016
  4. Dennis KaarsemakerAug 31, 2016
  5. Jeff KingSep 2, 2016
  6. Jeff KingSep 2, 2016
  7. Dennis KaarsemakerSep 2, 2016
  8. Jeff KingSep 2, 2016

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.