[Question] check_repository_format_gently() is not side-effect-free

[Tian Yuchen <cat@malon.dev>]

Hi Git community,

In setup.c:

> static const char *setup_explicit_git_dir(const char *gitdirenv,
> 					  struct strbuf *cwd,
> 					  struct repository_format *repo_fmt,
> 					  int *nongit_ok)
> {
> 	const char *work_tree_env = getenv(GIT_WORK_TREE_ENVIRONMENT);
> 	const char *worktree;
> 	char *gitfile;
> 	int offset;
> 
> 	if (PATH_MAX - 40 < strlen(gitdirenv))
> 		die(_("'$%s' too big"), GIT_DIR_ENVIRONMENT);
> 
> 	gitfile = (char*)read_gitfile(gitdirenv);
> 	if (gitfile) {
> 		gitfile = xstrdup(gitfile);
> 		gitdirenv = gitfile;
> 	}
> 
> 	if (!is_git_directory(gitdirenv)) {
> 		if (nongit_ok) {
> 			*nongit_ok = 1;
> 			free(gitfile);
> 			return NULL;
> 		}
> 		die(_("not a git repository: '%s'"), gitdirenv);
> 	}
> 

...

> 	if (check_repository_format_gently(gitdirenv, repo_fmt, nongit_ok)) {
> 		free(gitfile);
> 		return NULL;
> 	}

I've noticed that this function behaves rather strangely. If I am 
correct, The primary purpose of this function is to verify whether the 
Git binary understands the current repository format.

The implementation is as follows:

> static int check_repository_format_gently(const char *gitdir, struct repository_format *candidate, int *nongit_ok)
> {
> 	struct strbuf sb = STRBUF_INIT;
> 	struct strbuf err = STRBUF_INIT;
> 	int has_common;
> 
> 	has_common = get_common_dir(&sb, gitdir);
> 	strbuf_addstr(&sb, "/config");
> 	read_repository_format(candidate, sb.buf);
> 	strbuf_release(&sb);
> 
> 	/*
> 	 * For historical use of check_repository_format() in git-init,
> 	 * we treat a missing config as a silent "ok", even when nongit_ok
> 	 * is unset.
> 	 */
> 	if (candidate->version < 0)
> 		return 0;
> 
> 	if (verify_repository_format(candidate, &err) < 0) {
> 		if (nongit_ok) {
> 			warning("%s", err.buf);
> 			strbuf_release(&err);
> 			*nongit_ok = -1;
> 			return -1;
> 		}
> 		die("%s", err.buf);
> 	}
> 
> 	the_repository->repository_format_precious_objects = candidate->precious_objects;
> 
> 	string_list_clear(&candidate->unknown_extensions, 0);
> 	string_list_clear(&candidate->v1_only_extensions, 0);
> 
> 	if (candidate->worktree_config) {
> 		/*
> 		 * pick up core.bare and core.worktree from per-worktree
> 		 * config if present
> 		 */
> 		strbuf_addf(&sb, "%s/config.worktree", gitdir);
> 		git_config_from_file(read_worktree_config, sb.buf, candidate);
> 		strbuf_release(&sb);
> 		has_common = 0;
> 	}

...

> 	if (!has_common) {
> 		if (candidate->is_bare != -1) {
> 			is_bare_repository_cfg = candidate->is_bare;
> 			if (is_bare_repository_cfg == 1)
> 				inside_work_tree = -1;
> 		}
> 		if (candidate->work_tree) {
> 			free(git_work_tree_cfg);
> 			git_work_tree_cfg = xstrdup(candidate->work_tree);
> 			inside_work_tree = -1;
> 		}
> 	}
> 
> 	return 0;
> }

Obviously it applies the parsed results to the global environment. Since 
this function starts with 'check_' and ends with '_gently,' it's hard 
not to think of it as a side-effect-free diagnostic function.

git grep "^[a-z_]* check_[a-z_]*(" -- "*.c"

Using the command above, we can see that there are other functions in 
the Git source code that start with 'check_' but are not entirely 
side-effect-free. However, I believe that the two variables 
(is_bare_repository_cfg, git_work_tree_cfg) this function modifies are 
much more crucial: For example, if I want to handle both a bare 
repository and a normal repository within a single process, wouldn't 
problems arise soon?

I would love to hear if there are any historical reasons preventing us 
from stripping these global side-effects out of this function.

Regards,

Yuchen