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

[Dennis Kaarsemaker <dennis@kaarsemaker.net>]

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