On Mon, Mar 4, 2024 at 8:22 PM Atneya Nair <atneya@xxxxxxxxxx> wrote:
> 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@xxxxxxxxxx>
> ---
> 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;
If you're going to adopt this idiom of checking `err` rather than the
return code of read_gitfile_gently(), then you should document that
`err` will be set to zero in the success case. Presently, the
documentation for read_gitfile_gently() only talks about the failure
case and doesn't mention that zero indicates success.
> 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.
What is "a valid strbuf"? Perhaps say instead "if `result_buf` is not
NULL, ...". The "is not NULL" wording is consistent with the existing
wording used below...
Also...
s/is is/is/
s/ptr/pointer/
> * On failure, if return_error_code is not NULL, return_error_code
... "is not NULL" wording is already used here.
> @@ -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;
> + }
Junio mentioned style violations in his response. Omit braces around
one line `if` bodies.
> @@ -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;
It's a minor thing, but if you name the function argument `realpath`,
then the diff becomes less noisy since changes such as these do not
need to be made. On the other hand, if `realpath` isn't a good output
variable name, then by all means choose a better name.
> @@ -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;
> + }
Releasing the strbuf before `return`. Good.
> @@ -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;
Likewise. Good.
> }
> + strbuf_release(&gitdirenvbuf);
>
> if (is_git_directory(dir->buf)) {
> trace2_data_string("setup", NULL, "implicit-bare-repository", dir->buf);
There are additional `return` statements (not shown in the context)
following this code, but you make this final strbuf_release() call
before any of those other `return` statements can be taken. Good.
> 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;
Style: Declare `resolved_gitdir_buf` along with `ret` and `gitdir`,
then have a blank line before the actual code.
> @@ -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);
Style: Strange indentation?
> @@ -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;
> }
Not sure what you're trying to do here. strbuf guarantees that its
`buf` member will never be NULL, so the new `if (!gitfilebuf.buf)`
conditional seems to be dead code. If you really want to check whether
an error occurred, pass non-NULL for the second argument and check the
return value of read_gitfile_gently() or check the error code.
> 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))) {
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.
