RFC patch, 3 partsParallel submodule status

> Migrate read_gitfile_gently and resolve_gitfile_gently to take strbuf
> out params to allocate their return values into, rather than returning
> a view into a shared buffer.
>
> Leave the shared buffer in case a caller passes null for this param (for
> cases we haven't cleaned up yet).
>
> Migrate callers of resolve_gitfile to resolve_gitfile_gently.
>
> Signed-off-by: Atneya Nair <atneya@google.com>
> ---
>
> Notes:
>     Open questions:
>      - Is checking the return value of read_gitfile necessary if on error,
>        we are supposed to die, or set the error field to a non-zero value?

>  builtin/init-db.c   |  7 ++++---
>  builtin/rev-parse.c |  4 +++-
>  repository.c        |  9 +++++----
>  setup.c             | 36 +++++++++++++++++++++++++-----------
>  setup.h             |  7 +++----
>  submodule.c         | 32 +++++++++++++++++++++++---------
>  worktree.c          | 27 +++++++++++++--------------
>  7 files changed, 76 insertions(+), 46 deletions(-)
>
> diff --git a/builtin/init-db.c b/builtin/init-db.c
> index 0170469b84..9135d07a0d 100644
> --- a/builtin/init-db.c
> +++ b/builtin/init-db.c
> @@ -198,11 +198,11 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
>  	 */
>  	if (real_git_dir) {
>  		int err;
> -		const char *p;
>  		struct strbuf sb = STRBUF_INIT;
> +		struct strbuf gitfile = STRBUF_INIT;
>  
> -		p = read_gitfile_gently(git_dir, &err);
> -		if (p && get_common_dir(&sb, p)) {
> +		read_gitfile_gently(git_dir, &err, &gitfile);
> +		if (!err && get_common_dir(&sb, gitfile.buf)) {
>  			struct strbuf mainwt = STRBUF_INIT;
>  
>  			strbuf_addbuf(&mainwt, &sb);
> @@ -213,6 +213,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
>  			git_dir = strbuf_detach(&sb, NULL);
>  		}
>  		strbuf_release(&sb);
> +		strbuf_release(&gitfile);
>  	}
>  
>  	if (is_bare_repository_cfg < 0)
> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> index d08987646a..e1db6b3231 100644
> --- a/builtin/rev-parse.c
> +++ b/builtin/rev-parse.c
> @@ -728,12 +728,14 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
>  			}
>  			if (!strcmp(arg, "--resolve-git-dir")) {
>  				const char *gitdir = argv[++i];
> +				struct strbuf resolved_gitdir = STRBUF_INIT;
>  				if (!gitdir)
>  					die(_("--resolve-git-dir requires an argument"));
> -				gitdir = resolve_gitdir(gitdir);
> +				gitdir = resolve_gitdir_gently(gitdir, NULL, &resolved_gitdir);
>  				if (!gitdir)
>  					die(_("not a gitdir '%s'"), argv[i]);
>  				puts(gitdir);
> +				strbuf_release(&resolved_gitdir);
>  				continue;
>  			}
>  		}
> diff --git a/repository.c b/repository.c
> index 7aacb51b65..3ca6dbcf16 100644
> --- a/repository.c
> +++ b/repository.c
> @@ -118,7 +118,7 @@ static int repo_init_gitdir(struct repository *repo, const char *gitdir)
>  	int ret = 0;
>  	int error = 0;
>  	char *abspath = NULL;
> -	const char *resolved_gitdir;
> +	struct strbuf resolved_gitdir = STRBUF_INIT;
>  	struct set_gitdir_args args = { NULL };
>  
>  	abspath = real_pathdup(gitdir, 0);
> @@ -128,15 +128,16 @@ static int repo_init_gitdir(struct repository *repo, const char *gitdir)
>  	}
>  
>  	/* 'gitdir' must reference the gitdir directly */
> -	resolved_gitdir = resolve_gitdir_gently(abspath, &error);
> -	if (!resolved_gitdir) {
> +	resolve_gitdir_gently(abspath, &error, &resolved_gitdir);
> +	if (error) {
>  		ret = -1;
>  		goto out;
>  	}
>  
> -	repo_set_gitdir(repo, resolved_gitdir, &args);
> +	repo_set_gitdir(repo, resolved_gitdir.buf, &args);
>  
>  out:
> +	strbuf_release(&resolved_gitdir);
>  	free(abspath);
>  	return ret;
>  }
> diff --git a/setup.c b/setup.c
> index b69b1cbc2a..2e118cf216 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -397,14 +397,17 @@ int is_nonbare_repository_dir(struct strbuf *path)
>  	int ret = 0;
>  	int gitfile_error;
>  	size_t orig_path_len = path->len;
> +	struct strbuf gitfile = STRBUF_INIT;
> +
>  	assert(orig_path_len != 0);
>  	strbuf_complete(path, '/');
>  	strbuf_addstr(path, ".git");
> -	if (read_gitfile_gently(path->buf, &gitfile_error) || is_git_directory(path->buf))
> +	if (read_gitfile_gently(path->buf, &gitfile_error, &gitfile) || is_git_directory(path->buf))
>  		ret = 1;
>  	if (gitfile_error == READ_GITFILE_ERR_OPEN_FAILED ||
>  	    gitfile_error == READ_GITFILE_ERR_READ_FAILED)
>  		ret = 1;
> +	strbuf_release(&gitfile);
>  	strbuf_setlen(path, orig_path_len);
>  	return ret;
>  }
> @@ -830,7 +833,8 @@ void read_gitfile_error_die(int error_code, const char *path, const char *dir)
>  
>  /*
>   * Try to read the location of the git directory from the .git file,
> - * return path to git directory if found. The return value comes from
> + * return path to git directory if found. If passed a valid strbuf, the return
> + * value is is a ptr to within the buffer. If strbuf is null, the return value comes from
>   * a shared buffer.
>   *
>   * On failure, if return_error_code is not NULL, return_error_code
> @@ -838,7 +842,7 @@ void read_gitfile_error_die(int error_code, const char *path, const char *dir)
>   * return_error_code is NULL the function will die instead (for most
>   * cases).
>   */
> -const char *read_gitfile_gently(const char *path, int *return_error_code)
> +const char *read_gitfile_gently(const char *path, int *return_error_code, struct strbuf* result_buf)
>  {
>  	const int max_file_size = 1 << 20;  /* 1MB */
>  	int error_code = 0;
> @@ -848,7 +852,10 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)
>  	struct stat st;
>  	int fd;
>  	ssize_t len;
> -	static struct strbuf realpath = STRBUF_INIT;
> +	static struct strbuf shared = STRBUF_INIT;
> +	if (!result_buf) {
> +		result_buf = &shared;
> +	}
>  
>  	if (stat(path, &st)) {
>  		/* NEEDSWORK: discern between ENOENT vs other errors */
> @@ -900,8 +907,10 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)
>  		goto cleanup_return;
>  	}
>  
> -	strbuf_realpath(&realpath, dir, 1);
> -	path = realpath.buf;
> +	strbuf_realpath(result_buf, dir, 1);
> +	path = result_buf->buf;
> +	// TODO is this valid?
> +	if (!path) die(_("Unexpected null from realpath '%s'"), dir);
>  
>  cleanup_return:
>  	if (return_error_code)
> @@ -1316,12 +1325,13 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
>  		int offset = dir->len, error_code = 0;
>  		char *gitdir_path = NULL;
>  		char *gitfile = NULL;
> +		struct strbuf gitdirenvbuf = STRBUF_INIT;
>  
>  		if (offset > min_offset)
>  			strbuf_addch(dir, '/');
>  		strbuf_addstr(dir, DEFAULT_GIT_DIR_ENVIRONMENT);
>  		gitdirenv = read_gitfile_gently(dir->buf, die_on_error ?
> -						NULL : &error_code);
> +						NULL : &error_code, &gitdirenvbuf);
>  		if (!gitdirenv) {
>  			if (die_on_error ||
>  			    error_code == READ_GITFILE_ERR_NOT_A_FILE) {
> @@ -1330,8 +1340,10 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
>  					gitdirenv = DEFAULT_GIT_DIR_ENVIRONMENT;
>  					gitdir_path = xstrdup(dir->buf);
>  				}
> -			} else if (error_code != READ_GITFILE_ERR_STAT_FAILED)
> +			} else if (error_code != READ_GITFILE_ERR_STAT_FAILED) {
> +				strbuf_release(&gitdirenvbuf);
>  				return GIT_DIR_INVALID_GITFILE;
> +			}
>  		} else
>  			gitfile = xstrdup(dir->buf);
>  		/*
> @@ -1365,9 +1377,10 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
>  			 */
>  			free(gitdir_path);
>  			free(gitfile);
> -
> +			strbuf_release(&gitdirenvbuf);
>  			return ret;
>  		}
> +		strbuf_release(&gitdirenvbuf);
>  
>  		if (is_git_directory(dir->buf)) {
>  			trace2_data_string("setup", NULL, "implicit-bare-repository", dir->buf);
> @@ -1692,11 +1705,12 @@ const char *setup_git_directory(void)
>  	return setup_git_directory_gently(NULL);
>  }
>  
> -const char *resolve_gitdir_gently(const char *suspect, int *return_error_code)
> +const char *resolve_gitdir_gently(const char *suspect, int *return_error_code,
> +		struct strbuf* return_buf)
>  {
>  	if (is_git_directory(suspect))
>  		return suspect;
> -	return read_gitfile_gently(suspect, return_error_code);
> +	return read_gitfile_gently(suspect, return_error_code, return_buf);
>  }
>  
>  /* if any standard file descriptor is missing open it to /dev/null */
> diff --git a/setup.h b/setup.h
> index 3599aec93c..cf5a63762b 100644
> --- a/setup.h
> +++ b/setup.h
> @@ -36,10 +36,9 @@ int is_nonbare_repository_dir(struct strbuf *path);
>  #define READ_GITFILE_ERR_NOT_A_REPO 7
>  #define READ_GITFILE_ERR_TOO_LARGE 8
>  void read_gitfile_error_die(int error_code, const char *path, const char *dir);
> -const char *read_gitfile_gently(const char *path, int *return_error_code);
> -#define read_gitfile(path) read_gitfile_gently((path), NULL)
> -const char *resolve_gitdir_gently(const char *suspect, int *return_error_code);
> -#define resolve_gitdir(path) resolve_gitdir_gently((path), NULL)
> +const char *read_gitfile_gently(const char *path, int *return_error_code, struct strbuf *buf);
> +#define read_gitfile(path) read_gitfile_gently((path), NULL, NULL)
> +const char *resolve_gitdir_gently(const char *suspect, int *return_error_code, struct strbuf *buf);
>  
>  void setup_work_tree(void);
>  
> diff --git a/submodule.c b/submodule.c
> index 213da79f66..455444321b 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -316,9 +316,10 @@ int is_submodule_populated_gently(const char *path, int *return_error_code)
>  	int ret = 0;
>  	char *gitdir = xstrfmt("%s/.git", path);
>  
> -	if (resolve_gitdir_gently(gitdir, return_error_code))
> +	struct strbuf resolved_gitdir_buf = STRBUF_INIT;
> +	if (resolve_gitdir_gently(gitdir, return_error_code, &resolved_gitdir_buf))
>  		ret = 1;
> -
> +	strbuf_release(&resolved_gitdir_buf);
>  	free(gitdir);
>  	return ret;
>  }
> @@ -1879,22 +1880,25 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
>  {
>  	struct child_process cp = CHILD_PROCESS_INIT;
>  	struct strbuf buf = STRBUF_INIT;
> +	struct strbuf gitdirbuf = STRBUF_INIT;
>  	FILE *fp;
>  	unsigned dirty_submodule = 0;
>  	const char *git_dir;
>  	int ignore_cp_exit_code = 0;
>  
>  	strbuf_addf(&buf, "%s/.git", path);
> -	git_dir = read_gitfile(buf.buf);
> +	git_dir = read_gitfile_gently(buf.buf, NULL, &gitdirbuf);
>  	if (!git_dir)
>  		git_dir = buf.buf;
>  	if (!is_git_directory(git_dir)) {
>  		if (is_directory(git_dir))
>  			die(_("'%s' not recognized as a git repository"), git_dir);
>  		strbuf_release(&buf);
> +		strbuf_release(&gitdirbuf);
>  		/* The submodule is not checked out, so it is not modified */
>  		return 0;
>  	}
> +		strbuf_release(&gitdirbuf);
>  	strbuf_reset(&buf);
>  
>  	strvec_pushl(&cp.args, "status", "--porcelain=2", NULL);
> @@ -1958,15 +1962,16 @@ int submodule_uses_gitfile(const char *path)
>  {
>  	struct child_process cp = CHILD_PROCESS_INIT;
>  	struct strbuf buf = STRBUF_INIT;
> -	const char *git_dir;
> +	struct strbuf gitfilebuf = STRBUF_INIT;
>  
>  	strbuf_addf(&buf, "%s/.git", path);
> -	git_dir = read_gitfile(buf.buf);
> -	if (!git_dir) {
> +	read_gitfile_gently(buf.buf, NULL, &gitfilebuf);
> +	if (!gitfilebuf.buf) {
>  		strbuf_release(&buf);
>  		return 0;
>  	}
>  	strbuf_release(&buf);
> +	strbuf_release(&gitfilebuf);
>  
>  	/* Now test that all nested submodules use a gitfile too */
>  	strvec_pushl(&cp.args,
> @@ -2276,6 +2281,7 @@ static void relocate_single_git_dir_into_superproject(const char *path,
>  {
>  	char *old_git_dir = NULL, *real_old_git_dir = NULL, *real_new_git_dir = NULL;
>  	struct strbuf new_gitdir = STRBUF_INIT;
> +	struct strbuf gitfilebuf = STRBUF_INIT;
>  	const struct submodule *sub;
>  
>  	if (submodule_uses_worktrees(path))
> @@ -2283,9 +2289,12 @@ static void relocate_single_git_dir_into_superproject(const char *path,
>  		      "more than one worktree not supported"), path);
>  
>  	old_git_dir = xstrfmt("%s/.git", path);
> -	if (read_gitfile(old_git_dir))
> +	if (read_gitfile_gently(old_git_dir, NULL, &gitfilebuf)) {
>  		/* If it is an actual gitfile, it doesn't need migration. */
> +		strbuf_release(&gitfilebuf);
>  		return;
> +	}
> +	strbuf_release(&gitfilebuf);
>  
>  	real_old_git_dir = real_pathdup(old_git_dir, 1);
>  
> @@ -2343,8 +2352,9 @@ void absorb_git_dir_into_superproject(const char *path,
>  	int err_code;
>  	const char *sub_git_dir;
>  	struct strbuf gitdir = STRBUF_INIT;
> +	struct strbuf resolved_gitdir_buf = STRBUF_INIT;
>  	strbuf_addf(&gitdir, "%s/.git", path);
> -	sub_git_dir = resolve_gitdir_gently(gitdir.buf, &err_code);
> +	sub_git_dir = resolve_gitdir_gently(gitdir.buf, &err_code, &resolved_gitdir_buf);
>  
>  	/* Not populated? */
>  	if (!sub_git_dir) {
> @@ -2385,6 +2395,8 @@ void absorb_git_dir_into_superproject(const char *path,
>  		free(real_sub_git_dir);
>  		free(real_common_git_dir);
>  	}
> +
> +	strbuf_release(&resolved_gitdir_buf);
>  	strbuf_release(&gitdir);
>  
>  	absorb_git_dir_into_superproject_recurse(path, super_prefix);
> @@ -2484,17 +2496,19 @@ int submodule_to_gitdir(struct strbuf *buf, const char *submodule)
>  	const struct submodule *sub;
>  	const char *git_dir;
>  	int ret = 0;
> +	struct strbuf gitfilebuf = STRBUF_INIT;
>  
>  	strbuf_reset(buf);
>  	strbuf_addstr(buf, submodule);
>  	strbuf_complete(buf, '/');
>  	strbuf_addstr(buf, ".git");
>  
> -	git_dir = read_gitfile(buf->buf);
> +	git_dir = read_gitfile_gently(buf->buf, NULL, &gitfilebuf);
>  	if (git_dir) {
>  		strbuf_reset(buf);
>  		strbuf_addstr(buf, git_dir);
>  	}
> +	strbuf_release(&gitfilebuf);
>  	if (!is_git_directory(buf->buf)) {
>  		sub = submodule_from_path(the_repository, null_oid(),
>  					  submodule);
> diff --git a/worktree.c b/worktree.c
> index b02a05a74a..a6f125c8da 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -309,7 +309,7 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg,
>  {
>  	struct strbuf wt_path = STRBUF_INIT;
>  	struct strbuf realpath = STRBUF_INIT;
> -	char *path = NULL;
> +	struct strbuf gitfile = STRBUF_INIT;
>  	int err, ret = -1;
>  
>  	strbuf_addf(&wt_path, "%s/.git", wt->path);
> @@ -353,21 +353,20 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg,
>  		goto done;
>  	}
>  
> -	path = xstrdup_or_null(read_gitfile_gently(wt_path.buf, &err));
> -	if (!path) {
> +	if (!read_gitfile_gently(wt_path.buf, &err, &gitfile)) {
>  		strbuf_addf_gently(errmsg, _("'%s' is not a .git file, error code %d"),
>  				   wt_path.buf, err);
>  		goto done;
>  	}
>  
>  	strbuf_realpath(&realpath, git_common_path("worktrees/%s", wt->id), 1);
> -	ret = fspathcmp(path, realpath.buf);
> +	ret = fspathcmp(gitfile.buf, realpath.buf);
>  
>  	if (ret)
>  		strbuf_addf_gently(errmsg, _("'%s' does not point back to '%s'"),
>  				   wt->path, git_common_path("worktrees/%s", wt->id));
>  done:
> -	free(path);
> +	strbuf_release(&gitfile);
>  	strbuf_release(&wt_path);
>  	strbuf_release(&realpath);
>  	return ret;
> @@ -567,7 +566,7 @@ static void repair_gitfile(struct worktree *wt,
>  {
>  	struct strbuf dotgit = STRBUF_INIT;
>  	struct strbuf repo = STRBUF_INIT;
> -	char *backlink;
> +	struct strbuf backlink = STRBUF_INIT;
>  	const char *repair = NULL;
>  	int err;
>  
> @@ -582,13 +581,13 @@ static void repair_gitfile(struct worktree *wt,
>  
>  	strbuf_realpath(&repo, git_common_path("worktrees/%s", wt->id), 1);
>  	strbuf_addf(&dotgit, "%s/.git", wt->path);
> -	backlink = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err));
> +	read_gitfile_gently(dotgit.buf, &err, &backlink);
>  
>  	if (err == READ_GITFILE_ERR_NOT_A_FILE)
>  		fn(1, wt->path, _(".git is not a file"), cb_data);
>  	else if (err)
>  		repair = _(".git file broken");
> -	else if (fspathcmp(backlink, repo.buf))
> +	else if (fspathcmp(backlink.buf, repo.buf))
>  		repair = _(".git file incorrect");
>  
>  	if (repair) {
> @@ -596,7 +595,7 @@ static void repair_gitfile(struct worktree *wt,
>  		write_file(dotgit.buf, "gitdir: %s", repo.buf);
>  	}
>  
> -	free(backlink);
> +	strbuf_release(&backlink);
>  	strbuf_release(&repo);
>  	strbuf_release(&dotgit);
>  }
> @@ -685,7 +684,7 @@ void repair_worktree_at_path(const char *path,
>  	struct strbuf realdotgit = STRBUF_INIT;
>  	struct strbuf gitdir = STRBUF_INIT;
>  	struct strbuf olddotgit = STRBUF_INIT;
> -	char *backlink = NULL;
> +	struct strbuf backlink = STRBUF_INIT;
>  	const char *repair = NULL;
>  	int err;
>  
> @@ -701,12 +700,12 @@ void repair_worktree_at_path(const char *path,
>  		goto done;
>  	}
>  
> -	backlink = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err));
> +	read_gitfile_gently(realdotgit.buf, &err, &backlink);
>  	if (err == READ_GITFILE_ERR_NOT_A_FILE) {
>  		fn(1, realdotgit.buf, _("unable to locate repository; .git is not a file"), cb_data);
>  		goto done;
>  	} else if (err == READ_GITFILE_ERR_NOT_A_REPO) {
> -		if (!(backlink = infer_backlink(realdotgit.buf))) {
> +		if (!(backlink.buf = infer_backlink(realdotgit.buf))) {
>  			fn(1, realdotgit.buf, _("unable to locate repository; .git file does not reference a repository"), cb_data);
>  			goto done;
>  		}
> @@ -715,7 +714,7 @@ void repair_worktree_at_path(const char *path,
>  		goto done;
>  	}
>  
> -	strbuf_addf(&gitdir, "%s/gitdir", backlink);
> +	strbuf_addf(&gitdir, "%s/gitdir", backlink.buf);
>  	if (strbuf_read_file(&olddotgit, gitdir.buf, 0) < 0)
>  		repair = _("gitdir unreadable");
>  	else {
> @@ -729,7 +728,7 @@ void repair_worktree_at_path(const char *path,
>  		write_file(gitdir.buf, "%s", realdotgit.buf);
>  	}
>  done:
> -	free(backlink);
> +	strbuf_release(&backlink);
>  	strbuf_release(&olddotgit);
>  	strbuf_release(&gitdir);
>  	strbuf_release(&realdotgit);

> Continue the work in commit: 3d7747e318532a36a263c61cdf92f2decb6424ff
> to remove unsafe shared buffer usage in read_gitfile_gently.
>
> Migrate read_gitfile_gently and resolve_gitfile_gently to take strbuf
> out params to allocate their return values into, rather than returning
> a view into a shared buffer.
>
> Leave the shared buffer in case a caller passes null for this param (for
> cases we haven't cleaned up yet).
>
> Migrate callers of resolve_gitfile to resolve_gitfile_gently.
>
> Signed-off-by: Atneya Nair <atneya@google.com>
> ---
> diff --git a/builtin/init-db.c b/builtin/init-db.c
> @@ -198,11 +198,11 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
> -               const char *p;
> +               struct strbuf gitfile = STRBUF_INIT;
>
> -               p = read_gitfile_gently(git_dir, &err);
> -               if (p && get_common_dir(&sb, p)) {
> +               read_gitfile_gently(git_dir, &err, &gitfile);
> +               if (!err && get_common_dir(&sb, gitfile.buf)) {
>                         struct strbuf mainwt = STRBUF_INIT;

> diff --git a/setup.c b/setup.c
> @@ -830,7 +833,8 @@ void read_gitfile_error_die(int error_code, const char *path, const char *dir)
>  /*
>   * Try to read the location of the git directory from the .git file,
> - * return path to git directory if found. The return value comes from
> + * return path to git directory if found. If passed a valid strbuf, the return
> + * value is is a ptr to within the buffer. If strbuf is null, the return value comes from
>   * a shared buffer.

> @@ -848,7 +852,10 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)
> -       static struct strbuf realpath = STRBUF_INIT;
> +       static struct strbuf shared = STRBUF_INIT;
> +       if (!result_buf) {
> +               result_buf = &shared;
> +       }

> @@ -900,8 +907,10 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)
> -       strbuf_realpath(&realpath, dir, 1);
> -       path = realpath.buf;
> +       strbuf_realpath(result_buf, dir, 1);
> +       path = result_buf->buf;

> @@ -1316,12 +1325,13 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
> +               struct strbuf gitdirenvbuf = STRBUF_INIT;
>                 gitdirenv = read_gitfile_gently(dir->buf, die_on_error ?
> -                                               NULL : &error_code);
> +                                               NULL : &error_code, &gitdirenvbuf);
>                 if (!gitdirenv) {
> @@ -1330,8 +1340,10 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
> -                       } else if (error_code != READ_GITFILE_ERR_STAT_FAILED)
> +                       } else if (error_code != READ_GITFILE_ERR_STAT_FAILED) {
> +                               strbuf_release(&gitdirenvbuf);
>                                 return GIT_DIR_INVALID_GITFILE;
> +                       }

> @@ -1365,9 +1377,10 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
>                         free(gitdir_path);
>                         free(gitfile);
> +                       strbuf_release(&gitdirenvbuf);
>                         return ret;

>                 }
> +               strbuf_release(&gitdirenvbuf);
>
>                 if (is_git_directory(dir->buf)) {
>                         trace2_data_string("setup", NULL, "implicit-bare-repository", dir->buf);

> diff --git a/submodule.c b/submodule.c
> @@ -316,9 +316,10 @@ int is_submodule_populated_gently(const char *path, int *return_error_code)
>         int ret = 0;
>         char *gitdir = xstrfmt("%s/.git", path);
>
> -       if (resolve_gitdir_gently(gitdir, return_error_code))
> +       struct strbuf resolved_gitdir_buf = STRBUF_INIT;
> +       if (resolve_gitdir_gently(gitdir, return_error_code, &resolved_gitdir_buf))
>                 ret = 1;

> @@ -1879,22 +1880,25 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
> +       struct strbuf gitdirbuf = STRBUF_INIT;
> +               strbuf_release(&gitdirbuf);
>                 /* The submodule is not checked out, so it is not modified */
>                 return 0;
>         }
> +               strbuf_release(&gitdirbuf);
>         strbuf_reset(&buf);

> @@ -1958,15 +1962,16 @@ int submodule_uses_gitfile(const char *path)
> -       const char *git_dir;
> +       struct strbuf gitfilebuf = STRBUF_INIT;
>
>         strbuf_addf(&buf, "%s/.git", path);
> -       git_dir = read_gitfile(buf.buf);
> -       if (!git_dir) {
> +       read_gitfile_gently(buf.buf, NULL, &gitfilebuf);
> +       if (!gitfilebuf.buf) {
>                 strbuf_release(&buf);
>                 return 0;
>         }

> diff --git a/worktree.c b/worktree.c
> @@ -685,7 +684,7 @@ void repair_worktree_at_path(const char *path,
> -       char *backlink = NULL;
> +       struct strbuf backlink = STRBUF_INIT;
> @@ -701,12 +700,12 @@ void repair_worktree_at_path(const char *path,
> -       backlink = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err));
> +       read_gitfile_gently(realdotgit.buf, &err, &backlink);
>         if (err == READ_GITFILE_ERR_NOT_A_FILE) {
>                 fn(1, realdotgit.buf, _("unable to locate repository; .git is not a file"), cb_data);
>                 goto done;
>         } else if (err == READ_GITFILE_ERR_NOT_A_REPO) {
> -               if (!(backlink = infer_backlink(realdotgit.buf))) {
> +               if (!(backlink.buf = infer_backlink(realdotgit.buf))) {

>
> >         } else if (err == READ_GITFILE_ERR_NOT_A_REPO) {
> > -               if (!(backlink = infer_backlink(realdotgit.buf))) {
> > +               if (!(backlink.buf = infer_backlink(realdotgit.buf))) {
>
> Don't do this. Never modify the internal state of strbuf directly;
> consider the state read-only. Modifications should only be made via
> the API. You'll need to rewrite this code a bit to make it work
> correctly with the changes proposed by this patch.

> @@ -830,7 +833,8 @@ void read_gitfile_error_die(int error_code, const char *path, const char *dir)
>  
>  /*
>   * Try to read the location of the git directory from the .git file,
> - * return path to git directory if found. The return value comes from
> + * return path to git directory if found. If passed a valid strbuf, the return
> + * value is is a ptr to within the buffer. If strbuf is null, the return value comes from
>   * a shared buffer.
>   *
>   * On failure, if return_error_code is not NULL, return_error_code
> @@ -838,7 +842,7 @@ void read_gitfile_error_die(int error_code, const char *path, const char *dir)
>   * return_error_code is NULL the function will die instead (for most
>   * cases).
>   */
> -const char *read_gitfile_gently(const char *path, int *return_error_code)
> +const char *read_gitfile_gently(const char *path, int *return_error_code, struct strbuf* result_buf)
>  {
>  	const int max_file_size = 1 << 20;  /* 1MB */
>  	int error_code = 0;
> @@ -848,7 +852,10 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)
>  	struct stat st;
>  	int fd;
>  	ssize_t len;
> -	static struct strbuf realpath = STRBUF_INIT;
> +	static struct strbuf shared = STRBUF_INIT;
> +	if (!result_buf) {
> +		result_buf = &shared;
> +	}
>  

>> -const char *read_gitfile_gently(const char *path, int *return_error_code)
>> +const char *read_gitfile_gently(const char *path, int *return_error_code, struct strbuf* result_buf)
>>  {
>>  	const int max_file_size = 1 << 20;  /* 1MB */
>>  	int error_code = 0;
>> @@ -848,7 +852,10 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)
>>  	struct stat st;
>>  	int fd;
>>  	ssize_t len;
>> -	static struct strbuf realpath = STRBUF_INIT;
>> +	static struct strbuf shared = STRBUF_INIT;
>> +	if (!result_buf) {
>> +		result_buf = &shared;
>> +	}
>>  
>
> Question about general style: is it accepted practice to override a
> parameter of a function?

>> -	strbuf_realpath(&realpath, dir, 1);
>> -	path = realpath.buf;
>> +	strbuf_realpath(result_buf, dir, 1);
>> +	path = result_buf->buf;
>> +	// TODO is this valid?
>> +	if (!path) die(_("Unexpected null from realpath '%s'"), dir);
>
> In fact, this is not a null path, but an empty path (null is not part of
> the string).
> By the way, shouldn't this be an internal bug instead of a message to
> the user?

> >> +    if (!path) die(_("Unexpected null from realpath '%s'"), dir);
> >
> > In fact, this is not a null path, but an empty path (null is not part of
> > the string).
> > By the way, shouldn't this be an internal bug instead of a message to
> > the user?
>
> Unless the strbuf instance the result_buf pointer points at is
> corrupt, its .buf member should *NEVER* be NULL.  Testing for NULL
> is meaningless, unless you are manually futzing with the members of
> strbuf (you shouldn't).
>
> Thanks for carefully reading.
>

> To enable parallel update of the read cache for submodules,
> ce_compare_gitlink must be thread safe (for different objects).
>
> Remove string interning in do_config_from (called from
> repo_submodule_init) and add locking around accessing the ref_store_map.

> Signed-off-by: Atneya Nair <atneya@google.com>
> ---
>
> Notes:
>     Chasing down thread unsafe code was done using tsan.

>  config.c | 3 ++-
>  config.h | 2 +-
>  refs.c   | 9 +++++++++
>  3 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/config.c b/config.c
> index 3cfeb3d8bd..d7f73d8745 100644
> --- a/config.c
> +++ b/config.c
> @@ -1017,7 +1017,7 @@ static void kvi_from_source(struct config_source *cs,
>  			    enum config_scope scope,
>  			    struct key_value_info *out)
>  {
> -	out->filename = strintern(cs->name);
> +	out->filename = strdup(cs->name);
>  	out->origin_type = cs->origin_type;
>  	out->linenr = cs->linenr;
>  	out->scope = scope;
> @@ -1857,6 +1857,7 @@ static int do_config_from(struct config_source *top, config_fn_t fn,
>  
>  	strbuf_release(&top->value);
>  	strbuf_release(&top->var);
> +	free(kvi.filename);
>  
>  	return ret;
>  }
> diff --git a/config.h b/config.h
> index 5dba984f77..b78f1b6667 100644
> --- a/config.h
> +++ b/config.h
> @@ -118,7 +118,7 @@ struct config_options {
>  
>  /* Config source metadata for a given config key-value pair */
>  struct key_value_info {
> -	const char *filename;
> +	char *filename;
>  	int linenr;
>  	enum config_origin_type origin_type;
>  	enum config_scope scope;
> diff --git a/refs.c b/refs.c
> index c633abf284..cce8a31b22 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2126,6 +2126,9 @@ struct ref_store *get_submodule_ref_store(const char *submodule)
>  	size_t len;
>  	struct repository *subrepo;
>  
> +	// TODO is this locking tolerable, and/or can we get any finer
> +	static pthread_mutex_t lock = PTHREAD_MUTEX_INITIALIZER;
> +
>  	if (!submodule)
>  		return NULL;
>  
> @@ -2139,7 +2142,9 @@ struct ref_store *get_submodule_ref_store(const char *submodule)
>  		/* We need to strip off one or more trailing slashes */
>  		submodule = to_free = xmemdupz(submodule, len);
>  
> +	pthread_mutex_lock(&lock);
>  	refs = lookup_ref_store_map(&submodule_ref_stores, submodule);
> +	pthread_mutex_unlock(&lock);
>  	if (refs)
>  		goto done;
>  
> @@ -2162,10 +2167,14 @@ struct ref_store *get_submodule_ref_store(const char *submodule)
>  		free(subrepo);
>  		goto done;
>  	}
> +
> +	pthread_mutex_lock(&lock);
> +	// TODO maybe lock this separately
>  	refs = ref_store_init(subrepo, submodule_sb.buf,
>  			      REF_STORE_READ | REF_STORE_ODB);
>  	register_ref_store_map(&submodule_ref_stores, "submodule",
>  			       refs, submodule);
> +	pthread_mutex_unlock(&lock);
>  
>  done:
>  	strbuf_release(&submodule_sb);

> The use of strintern() comes originally from 3df8fd62 ...
> ..., so they may
> know how safe the change on the config side would be (I still do
> not understand why you'd want to do this in the first place, though,
> especially if you are protecting the callsites with mutex).

>> diff --git a/config.c b/config.c
>> index 3cfeb3d8bd..d7f73d8745 100644
>> --- a/config.c
>> +++ b/config.c
>> @@ -1017,7 +1017,7 @@ static void kvi_from_source(struct config_source *cs,
>>  			    enum config_scope scope,
>>  			    struct key_value_info *out)
>>  {
>> -	out->filename = strintern(cs->name);
>> +	out->filename = strdup(cs->name);
>>  	out->origin_type = cs->origin_type;
>>  	out->linenr = cs->linenr;
>>  	out->scope = scope;
>> @@ -1857,6 +1857,7 @@ static int do_config_from(struct config_source *top, config_fn_t fn,
>>  
>>  	strbuf_release(&top->value);
>>  	strbuf_release(&top->var);
>> +	free(kvi.filename);
>>  
>>  	return ret;
>>  }

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > The use of strintern() comes originally from 3df8fd62 ...
> > ..., so they may
> > know how safe the change on the config side would be (I still do
> > not understand why you'd want to do this in the first place, though,
> > especially if you are protecting the callsites with mutex).
> 
> The risks of turning code that uses strintern() to use strdup() are
> 
>  * you will leak the allocated string unless you explicitly free the
>    string you now own.
> 
>  * you may consume too much memory if you are creating too many
>    copies of the same string (e.g. if you need filename for each
>    line in a file in an application, the memory consumption can
>    become 1000-fold).
> 
>  * the code may be taking advantage of the fact that two such
>    strings can be compared for (in)equality simply by comparing
>    their addresses, which you would need to adjust to use !strcmp()
>    and the like.

> A few other things to note, looking at this code:
>
>   - isn't kvi->path in the same boat? We do not duplicate it at all, so
>     it seems like the shallow copy made in the configset could cause a
>     user-after-free.
>
>   - the "fix" I showed above hits your point 2: now we are making a lot
>     more copies of that string. I will note that we're already making a
>     lot of copies of the kvi struct in the first place, so unless you
>     have really long pathnames, it probably isn't a big difference.
>
>     But it possibly could make sense to have the configset own a single
>     duplicate string, and then let the kvi structs it holds point to
>     that string. But IMHO all of this should be details of the configset
>     code, and the main config-iteration code should not have to worry
>     about this at all. I.e., I think kvi_from_source() should not be
>     duplicating anything in the first place.

> And I think that may be the case here. The "kvi" struct itself is local
> to do_config_from(). But when we load the caching configset, we do so in
> configset_add_value() which makes a shallow copy of the kvi struct. And
> then that shallow copy may live on and be accessed with further calls to
> git_config().

>
> though probably an actual kvi_copy() function would be less horrible.
>
> A few other things to note, looking at this code:
>
>   - isn't kvi->path in the same boat? We do not duplicate it at all, so
>     it seems like the shallow copy made in the configset could cause a
>     user-after-free.

>     But it possibly could make sense to have the configset own a single
>     duplicate string, and then let the kvi structs it holds point to
>     that string. But IMHO all of this should be details of the configset
>     code, and the main config-iteration code should not have to worry
>     about this at all. I.e., I think kvi_from_source() should not be
>     duplicating anything in the first place.
>
> -Peff

>
> Atneya Nair <atneya@google.com> writes:
>
> > To enable parallel update of the read cache for submodules,
> > ce_compare_gitlink must be thread safe (for different objects).
> >
> > Remove string interning in do_config_from (called from
> > repo_submodule_init) and add locking around accessing the ref_store_map.
>
> This step does two independent things, even though they may have
> dependencies, i.e., for one to be a solution for the problem it is
> tackling, the other may have to be there already.  E.g., even after
> calls to ce_compare_gitlink() get serialized via a mutex, it may for
> some reason not work without giving each kvi.filename its own copy
> [*], and if that is the case, you may need to have the "stop
> interning" step in a single patch with its own justification, and
> then "have mutex around ref_store calls" patch has to come after it.
>