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

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

[doak]

Hi there,

If '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.
This seems to be a bug.
I am using Git v2.9.3 (Arch).

Steps to reproduce:
    ---------------------
    git init t1
    cd t1
    # 'core.logallrefupdates' will not be set for 't2'.
    git init ../t2
    ---------------------

Stated from 'git-config(1)':
    ---------------------
    core.logAllRefUpdates
        [...]
        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.
    ---------------------


With best regards,
doak

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

[Dennis Kaarsemaker]

That is indeed a bug. git reads the config of t1 and then thinks a
template config has set that value, so it won't override it.

This is caused by git init reading the config via
get_shared_repository. The comment above it indicates that this may not
be needed, and indeed not doing it makes this bug go away.

diff --git a/builtin/init-db.c b/builtin/init-db.c
index 3a45f0b..b2883a6 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -494,14 +494,6 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
        retry:
                if (chdir(argv[0]) < 0) {
                        if (!mkdir_tried) {
-                               int saved;
-                               /*
-                                * At this point we haven't read any configuration,
-                                * 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:
                                case SCLD_PERMS:
@@ -513,7 +505,6 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
                                        die_errno(_("cannot mkdir %s"), argv[0]);
                                        break;
                                }
-                               set_shared_repository(saved);
                                if (mkdir(argv[0], 0777) < 0)
                                        die_errno(_("cannot mkdir %s"), argv[0]);
                                mkdir_tried = 1; 

On wo, 2016-08-31 at 10:09 +0200, doak wrote:
> Hi there,
> 
> If '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.
> This seems to be a bug.
> I am using Git v2.9.3 (Arch).
> 
> Steps to reproduce:
>     ---------------------
>     git init t1
>     cd t1
>     # 'core.logallrefupdates' will not be set for 't2'.
>     git init ../t2
>     ---------------------
> 
> Stated from 'git-config(1)':
>     ---------------------
>     core.logAllRefUpdates
>         [...]
>         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.
>     ---------------------
> 
> 
> With best regards,
> doak
> 
> 

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

[Jeff King]

On Wed, Aug 31, 2016 at 11:12:26AM +0200, Dennis Kaarsemaker wrote:

> That is indeed a bug. git reads the config of t1 and then thinks a
> template config has set that value, so it won't override it.

This is a regression in v2.9.0 due to my ae5f677 (lazily load
core.sharedrepository, 2016-03-11).

> This is caused by git init reading the config via
> get_shared_repository. The comment above it indicates that this may not
> be needed, and indeed not doing it makes this bug go away.

Hrm. I'm not sure if that will work, though, because we may call
get_shared_repository() from other code-paths (e.g., anything that calls
adjust_shared_perm(), like safe_create_leading_directories).

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'm out of time for the day, so it will be a while before I can dig
further. Please feel free to figure it out while I am sleeping. :)

-Peff

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

[Dennis Kaarsemaker]

On wo, 2016-08-31 at 06:48 -0400, Jeff King wrote:
> On Wed, Aug 31, 2016 at 11:12:26AM +0200, Dennis Kaarsemaker wrote:
> 
> > 
> > That is indeed a bug. git reads the config of t1 and then thinks a
> > template config has set that value, so it won't override it.
> This is a regression in v2.9.0 due to my ae5f677 (lazily load
> core.sharedrepository, 2016-03-11).
> 
> > 
> > This is caused by git init reading the config via
> > get_shared_repository. The comment above it indicates that this may
> > not
> > be needed, and indeed not doing it makes this bug go away.
> Hrm. I'm not sure if that will work, though, because we may call
> get_shared_repository() from other code-paths (e.g., anything that
> calls
> adjust_shared_perm(), like safe_create_leading_directories).

Agreed, the diff was more to point out what triggered this (so I could
send that and run away to spend the day with jr.) than an attempt at a
patch.

> 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.

> I'm out of time for the day, so it will be a while before I can dig
> further. Please feel free to figure it out while I am sleeping. :)
> 
> -Peff

I hope you slept well :)

This is what I came up with to implement my suggestion above, comments
welcome.

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();
 			if (!mkdir_tried) {
 				int saved;
 				/*
@@ -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:
@@ -513,7 +518,6 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
 					die_errno(_("cannot mkdir %s"), argv[0]);
 					break;
 				}
-				set_shared_repository(saved);
 				if (mkdir(argv[0], 0777) < 0)
 					die_errno(_("cannot mkdir %s"), argv[0]);
 				mkdir_tried = 1;
@@ -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();
+
 	if (is_bare_repository_cfg == 1) {
 		char *cwd = xgetcwd();
 		setenv(GIT_DIR_ENVIRONMENT, cwd, argc > 0);
diff --git a/cache.h b/cache.h
index f30a441..1be16fc 100644
--- a/cache.h
+++ b/cache.h
@@ -640,6 +640,8 @@ extern int hold_locked_index(struct lock_file *, int);
 extern void set_alternate_index_output(const char *);
 
 /* Environment bits from configuration mechanism */
+extern int ignore_repo_config;
+extern int need_shared_repository_from_config;
 extern int trust_executable_bit;
 extern int trust_ctime;
 extern int check_stat;
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);
 
 	current_parsing_scope = CONFIG_SCOPE_CMDLINE;
diff --git a/environment.c b/environment.c
index ca72464..f6dbba8 100644
--- a/environment.c
+++ b/environment.c
@@ -12,6 +12,8 @@
 #include "fmt-merge-msg.h"
 #include "commit.h"
 
+int ignore_repo_config = 0;
+int need_shared_repository_from_config = 1;
 int trust_executable_bit = 1;
 int trust_ctime = 1;
 int check_stat = 1;
@@ -326,7 +328,6 @@ const char *get_commit_output_encoding(void)
 }
 
 static int the_shared_repository = PERM_UMASK;
-static int need_shared_repository_from_config = 1;
 
 void set_shared_repository(int value)
 {

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

[Jeff King]

On Wed, Aug 31, 2016 at 05:32:33PM +0200, Dennis Kaarsemaker wrote:

> > 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:

> 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.

> @@ -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.

> @@ -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.

> 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

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

[Jeff King]

On Fri, Sep 02, 2016 at 04:04:16AM -0400, Jeff King wrote:

> 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

I also wonder if "clone" should be doing something similar. Or, for that
matter, things like git-daemon that operate outside of a repo. They work
now because they do not happen to trigger any library calls which look
at config under the hood.

Traditionally these were supposed to just use git_config_early(), but
that's really not possible when the config calls are happening behind
the scenes (e.g., when lazy-loading the config cache). And so we
eventually got rid of git_config_early() entirely.

But I wonder if we could enforce that concept automatically for config.

The simple patch below does fix this case:

diff --git a/config.c b/config.c
index 0dfed68..b62bb40 100644
--- a/config.c
+++ b/config.c
@@ -1289,7 +1289,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 = startup_info->have_repository ? git_pathdup("config") : NULL;
 
 	current_parsing_scope = CONFIG_SCOPE_SYSTEM;
 	if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0))

but it causes a few test failures. Some of those are arguably
reasonable, though. E.g., several of the diff tests use "git diff
--no-index" and expect to read local config. But "--no-index" explicitly
_avoids_ setting up the git repository, so the current code just falls
back to reading ".git/config". Which means it works when you are at the
top-level of a repository, but not in a subdir!

So I think this patch is an improvement; if we have not set up the
repository, then we should not be reading its config! (It's another
question of whether --no-index should try setup_git_directory_gently(),
but then this patch would just do the right thing).

I think "hash-object" without "-w" is in the same boat. It does not even
bother looking for a git dir, but we assume that it can see config like
core.autocrlf. It works in the top-level, but not elsewhere:

  $ git init
  $ git config core.autocrlf true
  $ printf 'foo\r\n' >file
  $ git hash-object file
  257cc5642cb1a054f08cc83f2d943e56fd3ebe99
  $ mkdir subdir
  $ cd subdir
  $ git hash-object ../file
  e48b03ece74f47d1ae20075200c64aeaa01a9cdb

So there is definitely some cleanup work, but it seems like it would be
fixing a bunch of bugs.

Some of the other failures are not so obvious. In particular, t7006
tests the core.pager settings that are looked up before we set up the
git directory, and those are now broken. OTOH, I suspect that doing it
_correctly_ would fix all of the known breakages like:

  not ok 46 - git -p true - core.pager overrides PAGER from subdirectory

They are hitting that same subdirectory problem mentioned above.

-Peff

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

[Dennis Kaarsemaker]

On vr, 2016-09-02 at 04:04 -0400, Jeff King wrote:
> On Wed, Aug 31, 2016 at 05:32:33PM +0200, Dennis Kaarsemaker wrote:
> 
> > 
> > > 
> > > 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.

Yikes. Didnt' dig that deep, but that sounds wrong :)

> 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:
> 
> > 
> > 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.

Well, 'git init' is a valid operation to run inside an existing repo to
reinitialize some bits, so we definitely need to not ignore the config
once we're sure we're not creating a new repo.

> 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.

That's why I git_config_clear() twice.

> 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.

Except when running 'git init' to re-init existing repo.

> 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.
> 
> > 
> > @@ -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.

We don't need to save if we throw away the cache below.

> > @@ -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.

We do, because this is also called for existing repos.

> > 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.

Thought about doing that, but didn't know what side-effects that would
have.

> 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
> 
> 

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

[Jeff King]

On Fri, Sep 02, 2016 at 11:01:54AM +0200, Dennis Kaarsemaker wrote:

> Well, 'git init' is a valid operation to run inside an existing repo to
> reinitialize some bits, so we definitely need to not ignore the config
> once we're sure we're not creating a new repo.

Good point, I hadn't considered re-initializing.

For the follow-up patch I sent, where we check
startup_info->have_repository, I think the right thing would probably be
to call setup_git_directory() after seeing we are in a re-init case.
Probably even the "gently" form, as I think you can "re-init" a
partially corrupted repository.

> > > @@ -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.
> 
> We don't need to save if we throw away the cache below.

Yeah, you're right. Though I somehow lost my train of thought between
the two paragraphs. I was thinking that we would want to actually
respect the ~/.gitconfig setting for sharedrepository. Which would
actually mean _dropping_ the save/zero/restore entirely, and just using
the value we get from the config. But I guess the point here is to avoid
s_c_l_d creating "shared" leading directories that are outside any
repository. I could see an argument either way on whether that is the
right thing to do when core.sharedRepository is set in ~/.gitconfig.

-Peff