| Message ID | 20240305012112.1598053-3-atneya@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Parallel submodule status | expand |
Atneya Nair <atneya@google.com> writes: > Continue the work in commit: 3d7747e318532a36a263c61cdf92f2decb6424ff > to remove unsafe shared buffer usage in read_gitfile_gently. The usual way to compose a log message of this project is to - Give an observation on how the current system work in the present tense (so no need to say "Currently X is Y", just "X is Y"), and discuss what you perceive as a problem in it. - Propose a solution (optional---often, problem description trivially leads to an obvious solution in reader's minds). - Give commands to the codebase to "become like so". in this order. The introductory part may become like so. Notice the format in which we usually refer to an existing commit: 3d7747e3 (real_path: remove unsafe API, 2020-03-10) started the process of making more API functions thread-safe. Many callers of read_gitfile() still depend on the shared buffer used to hold the return value, and convenience of not having to allocate/free their own storage, but this hinders multi-threading. And the remainder of the proposed log message should propose a solution and tell the codebase to become like so. See Documentation/SubmittingPatches for other general guidelines. Documentation/CodingGuidelines would also be helpful. > 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? The conversion done by this step seems to be a more or less faithful rewrite. If the original checked for an error from the original read_gitfile_gently() by seeing if it returned a NULL, the updated code should instead check for what is returned in &err. And that seems to be what you did, which is good. > - Should we clean up the other call-sites of read_gitfile? It is OK to do the ones you care about (read: needed to be converted before you can do the multi-threading) first, and then update the rest. The conversion itself in the patch looked obvious and trivially correct from a cursory read, but I have to admit that my concentration level was not sufficiently high while I reviewed the patch, as my reading was constantly hiccupping whenever I saw a coding style glitches. The authors can help by sticking to what the Documentation/CodingGuidelines document says, paying special attention to the comment styles, decl-after-statement, rules regarding braces around a single-statement block. Thanks. > 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);
On Mon, Mar 4, 2024 at 8:22 PM Atneya Nair <atneya@google.com> 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@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; 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.
Le 05/03/2024 à 02:21, Atneya Nair a écrit : <snip> > @@ -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; > + } > Question about general style: is it accepted practice to override a parameter of a function? I would have written: @@ -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* input_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; + struct strbuf* result_buf; + if (!input_buf) { + result_buf = &shared; + } else { + result_buf = input_buf; + } > 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); 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? Thanks
Jean-Noël Avila <avila.jn@gmail.com> writes: >> -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? We do not forbid it. We have a rule against enclosing a single statement block inside {braces}, though ;-). > I would have written: If it matters to know what the caller-supplied value of the parameter was, then we would probably write that way. If it does not, then the above is perfectly fine. Even with the above, if a later code really wanted to, it can compare the pointers to find out if the caller was uninterested in the result (i.e., passed NULL), but at that point, we may be better off to (re)write it your way. >> - 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? 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.
On Mon, Mar 4, 2024 at 8:29 PM Eric Sunshine <sunshine@sunshineco.com> wrote: > Style: Declare `resolved_gitdir_buf` along with `ret` and `gitdir`, > then have a blank line before the actual code. Thanks for the detailed style feedback. > > > } 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. Good catch, I should've paid more attention in the refactoring. Fixed all of the discussed notes in v2.
On Wed, Mar 6, 2024 at 8:57 AM Junio C Hamano <gitster@pobox.com> wrote: > >> + 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. > Thanks for pointing this out, I fixed this issue in v2.
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> --- 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? - Should we clean up the other call-sites of read_gitfile? 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(-)