| Message ID | 20250206-b4-pks-path-drop-the-repository-v1-10-4e77f0313206@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | path: remove dependency on `the_repository` | expand |
On Thu, Feb 06, 2025 at 08:58:06AM +0100, Patrick Steinhardt wrote: > diff --git a/builtin/worktree.c b/builtin/worktree.c > index 2cea9441a6..761e302a36 100644 > --- a/builtin/worktree.c > +++ b/builtin/worktree.c > @@ -151,7 +151,7 @@ static int delete_git_dir(const char *id) > struct strbuf sb = STRBUF_INIT; > int ret; > > - strbuf_addstr(&sb, git_common_path("worktrees/%s", id)); > + repo_common_path_append(the_repository, &sb, "worktrees/%s", id); > ret = remove_dir_recursively(&sb, 0); > if (ret < 0 && errno == ENOTDIR) > ret = unlink(sb.buf); > @@ -1102,6 +1102,7 @@ static int lock_worktree(int ac, const char **av, const char *prefix, > OPT_END() > }; > struct worktree **worktrees, *wt; > + char *path; > > ac = parse_options(ac, av, prefix, options, git_worktree_lock_usage, 0); > if (ac != 1) > @@ -1122,9 +1123,11 @@ static int lock_worktree(int ac, const char **av, const char *prefix, > die(_("'%s' is already locked"), av[0]); > } > > - write_file(git_common_path("worktrees/%s/locked", wt->id), > - "%s", reason); > + path = repo_common_path(the_repository, "worktrees/%s/locked", wt->id); From my perspective, we may use `repo_common_path_replace` here to avoid using the raw string pointer? This is because we return a changeable pointer "char *". But we pass this pointer to a "const char *". This is not critical, but we may make the semantics clearer. > + write_file(path, "%s", reason); > + > free_worktrees(worktrees); > + free(path); > return 0; > } > > @@ -1135,6 +1138,7 @@ static int unlock_worktree(int ac, const char **av, const char *prefix, > OPT_END() > }; > struct worktree **worktrees, *wt; > + char *path; > int ret; > > ac = parse_options(ac, av, prefix, options, git_worktree_unlock_usage, 0); > @@ -1149,8 +1153,12 @@ static int unlock_worktree(int ac, const char **av, const char *prefix, > die(_("The main working tree cannot be locked or unlocked")); > if (!worktree_lock_reason(wt)) > die(_("'%s' is not locked"), av[0]); > - ret = unlink_or_warn(git_common_path("worktrees/%s/locked", wt->id)); > + > + path = repo_common_path(the_repository, "worktrees/%s/locked", wt->id); This one as above. > + ret = unlink_or_warn(path); > + > free_worktrees(worktrees); > + free(path); > return ret; > } > > diff --git a/path.c b/path.c > index d721507be8..f6b795d75f 100644 > --- a/path.c > +++ b/path.c > @@ -634,10 +634,10 @@ const char *repo_submodule_path_replace(struct repository *repo, > return buf->buf; > } > > -void repo_common_pathv(const struct repository *repo, > - struct strbuf *sb, > - const char *fmt, > - va_list args) > +static void repo_common_pathv(const struct repository *repo, > + struct strbuf *sb, > + const char *fmt, > + va_list args) > { > strbuf_addstr(sb, repo->commondir); > if (sb->len && !is_dir_sep(sb->buf[sb->len - 1])) > diff --git a/path.h b/path.h > index 904eeac068..496f27fdfd 100644 > --- a/path.h > +++ b/path.h > @@ -233,29 +233,10 @@ struct strbuf *get_pathname(void); > # include "repository.h" > > /* Internal implementation details that should not be used. */ > -void repo_common_pathv(const struct repository *repo, > - struct strbuf *buf, > - const char *fmt, > - va_list args); So, we finally mark this function "static" and delete the declaration in this patch. We cannot do this in the earlier patch because "git_common_path" is defined in the header file and it needs to use this function. Make sense. However, I somehow feel a little strange especially in [PATCH 01/16] that you have added a comment: /* Internal implementation detail that should not be used. * When I see this comment, my first intuitive thinking is that if we should not use this function, why do we need to expose this in the first place? This really introduces confusion. > @@ -343,7 +344,7 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg, > if (!is_absolute_path(wt->path)) { > strbuf_addf_gently(errmsg, > _("'%s' file does not contain absolute path to the working tree location"), > - git_common_path("worktrees/%s/gitdir", wt->id)); > + repo_common_path_replace(the_repository, &buf, "worktrees/%s/gitdir", wt->id)); > goto done; > } > > @@ -365,14 +366,16 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg, > goto done; > } > > - strbuf_realpath(&realpath, git_common_path("worktrees/%s", wt->id), 1); > + strbuf_realpath(&realpath, repo_common_path_replace(the_repository, &buf, "worktrees/%s", wt->id), 1); We rely on the return value of `repo_common_path_replace` to elegantly do this. Make sense. > ret = fspathcmp(path, realpath.buf); > > if (ret) > strbuf_addf_gently(errmsg, _("'%s' does not point back to '%s'"), > - wt->path, git_common_path("worktrees/%s", wt->id)); > + wt->path, repo_common_path_replace(the_repository, &buf, > + "worktrees/%s", wt->id)); > done: > free(path); > + strbuf_release(&buf); > strbuf_release(&wt_path); > strbuf_release(&realpath); > return ret; > @@ -384,11 +387,13 @@ void update_worktree_location(struct worktree *wt, const char *path_, > struct strbuf path = STRBUF_INIT; > struct strbuf dotgit = STRBUF_INIT; > struct strbuf gitdir = STRBUF_INIT; > + char *wt_gitdir; > > if (is_main_worktree(wt)) > BUG("can't relocate main worktree"); > > - strbuf_realpath(&gitdir, git_common_path("worktrees/%s/gitdir", wt->id), 1); > + wt_gitdir = repo_common_path(the_repository, "worktrees/%s/gitdir", wt->id); > + strbuf_realpath(&gitdir, wt_gitdir, 1); Why we don't use above pattern which means the following: strbuf_realpath(&gitdir, git_common_path_replace(...), ...); I think we should be consistent. And we should not use "char *" type to pass to a "const char *" type here although this won't be harmful to the program. However, git_common_path_replace will return a "const char *" to make sure the caller cannot change this pointer. > strbuf_realpath(&path, path_, 1); > strbuf_addf(&dotgit, "%s/.git", path.buf); > if (fspathcmp(wt->path, path.buf)) { > @@ -400,6 +405,7 @@ void update_worktree_location(struct worktree *wt, const char *path_, > strbuf_release(&path); > strbuf_release(&dotgit); > strbuf_release(&gitdir); > + free(wt_gitdir); > } > > int is_worktree_being_rebased(const struct worktree *wt, > @@ -585,6 +591,7 @@ static void repair_gitfile(struct worktree *wt, > struct strbuf backlink = STRBUF_INIT; > char *dotgit_contents = NULL; > const char *repair = NULL; > + char *path = NULL; > int err; > > /* missing worktree can't be repaired */ > @@ -596,7 +603,8 @@ static void repair_gitfile(struct worktree *wt, > goto done; > } > > - strbuf_realpath(&repo, git_common_path("worktrees/%s", wt->id), 1); > + path = repo_common_path(the_repository, "worktrees/%s", wt->id); > + strbuf_realpath(&repo, path, 1); This one as above. > strbuf_addf(&dotgit, "%s/.git", wt->path); > strbuf_addf(&gitdir, "%s/gitdir", repo.buf); > dotgit_contents = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err)); > @@ -626,6 +634,7 @@ static void repair_gitfile(struct worktree *wt, > > done: > free(dotgit_contents); > + free(path); > strbuf_release(&repo); > strbuf_release(&dotgit); > strbuf_release(&gitdir); > @@ -657,11 +666,13 @@ void repair_worktree_after_gitdir_move(struct worktree *wt, const char *old_path > struct strbuf gitdir = STRBUF_INIT; > struct strbuf dotgit = STRBUF_INIT; > int is_relative_path; > + char *path = NULL; > > if (is_main_worktree(wt)) > goto done; > > - strbuf_realpath(&gitdir, git_common_path("worktrees/%s/gitdir", wt->id), 1); > + path = repo_common_path(the_repository, "worktrees/%s/gitdir", wt->id); > + strbuf_realpath(&gitdir, path, 1); > Also this one. > if (strbuf_read_file(&dotgit, gitdir.buf, 0) < 0) > goto done; > @@ -680,6 +691,7 @@ void repair_worktree_after_gitdir_move(struct worktree *wt, const char *old_path > done: > strbuf_release(&gitdir); > strbuf_release(&dotgit); > + free(path); > } > > void repair_worktrees_after_gitdir_move(const char *old_path) > @@ -871,7 +883,11 @@ int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath, > ssize_t read_result; > > *wtpath = NULL; > - strbuf_realpath(&repo, git_common_path("worktrees/%s", id), 1); > + > + path = repo_common_path(the_repository, "worktrees/%s", id); > + strbuf_realpath(&repo, path, 1); > + FREE_AND_NULL(path); > + I somehow agree that we could use `repo_common_path` in this way where we want to reuse "path" variable. Thanks, Jialuo
On Thu, Feb 06, 2025 at 11:54:24PM +0800, shejialuo wrote: > On Thu, Feb 06, 2025 at 08:58:06AM +0100, Patrick Steinhardt wrote: > > > > diff --git a/builtin/worktree.c b/builtin/worktree.c > > index 2cea9441a6..761e302a36 100644 > > --- a/builtin/worktree.c > > +++ b/builtin/worktree.c > > @@ -151,7 +151,7 @@ static int delete_git_dir(const char *id) > > struct strbuf sb = STRBUF_INIT; > > int ret; > > > > - strbuf_addstr(&sb, git_common_path("worktrees/%s", id)); > > + repo_common_path_append(the_repository, &sb, "worktrees/%s", id); > > ret = remove_dir_recursively(&sb, 0); > > if (ret < 0 && errno == ENOTDIR) > > ret = unlink(sb.buf); > > @@ -1102,6 +1102,7 @@ static int lock_worktree(int ac, const char **av, const char *prefix, > > OPT_END() > > }; > > struct worktree **worktrees, *wt; > > + char *path; > > > > ac = parse_options(ac, av, prefix, options, git_worktree_lock_usage, 0); > > if (ac != 1) > > @@ -1122,9 +1123,11 @@ static int lock_worktree(int ac, const char **av, const char *prefix, > > die(_("'%s' is already locked"), av[0]); > > } > > > > - write_file(git_common_path("worktrees/%s/locked", wt->id), > > - "%s", reason); > > + path = repo_common_path(the_repository, "worktrees/%s/locked", wt->id); > > From my perspective, we may use `repo_common_path_replace` here to avoid > using the raw string pointer? This is because we return a changeable > pointer "char *". But we pass this pointer to a "const char *". This is > not critical, but we may make the semantics clearer. I don't think there's much of a point doing so though. We only use the string a single time, so using a `strbuf` doesn't buy us anything. This was different if we already had a buffer available that we could reuse, but we don't. > > diff --git a/path.c b/path.c > > index d721507be8..f6b795d75f 100644 > > --- a/path.c > > +++ b/path.c > > @@ -634,10 +634,10 @@ const char *repo_submodule_path_replace(struct repository *repo, > > return buf->buf; > > } > > > > -void repo_common_pathv(const struct repository *repo, > > - struct strbuf *sb, > > - const char *fmt, > > - va_list args) > > +static void repo_common_pathv(const struct repository *repo, > > + struct strbuf *sb, > > + const char *fmt, > > + va_list args) > > { > > strbuf_addstr(sb, repo->commondir); > > if (sb->len && !is_dir_sep(sb->buf[sb->len - 1])) > > diff --git a/path.h b/path.h > > index 904eeac068..496f27fdfd 100644 > > --- a/path.h > > +++ b/path.h > > @@ -233,29 +233,10 @@ struct strbuf *get_pathname(void); > > # include "repository.h" > > > > /* Internal implementation details that should not be used. */ > > -void repo_common_pathv(const struct repository *repo, > > - struct strbuf *buf, > > - const char *fmt, > > - va_list args); > > So, we finally mark this function "static" and delete the declaration in > this patch. We cannot do this in the earlier patch because > "git_common_path" is defined in the header file and it needs to use this > function. Make sense. > > However, I somehow feel a little strange especially in [PATCH 01/16] > that you have added a comment: > > /* Internal implementation detail that should not be used. * > > When I see this comment, my first intuitive thinking is that if we > should not use this function, why do we need to expose this in the first > place? > > This really introduces confusion. I've touched up the first commit message to explain this a bit better. > > @@ -384,11 +387,13 @@ void update_worktree_location(struct worktree *wt, const char *path_, > > struct strbuf path = STRBUF_INIT; > > struct strbuf dotgit = STRBUF_INIT; > > struct strbuf gitdir = STRBUF_INIT; > > + char *wt_gitdir; > > > > if (is_main_worktree(wt)) > > BUG("can't relocate main worktree"); > > > > - strbuf_realpath(&gitdir, git_common_path("worktrees/%s/gitdir", wt->id), 1); > > + wt_gitdir = repo_common_path(the_repository, "worktrees/%s/gitdir", wt->id); > > + strbuf_realpath(&gitdir, wt_gitdir, 1); > > Why we don't use above pattern which means the following: > > strbuf_realpath(&gitdir, git_common_path_replace(...), ...); > > I think we should be consistent. We can't because `strbuf_realpath()` is not prepared to use the buffer as in-out parameter. > And we should not use "char *" type to pass to a "const char *" type > here although this won't be harmful to the program. However, > git_common_path_replace will return a "const char *" to make sure the > caller cannot change this pointer. Passing a `char *` to a `const char *` parameter is fine in general and expected in places where the string is allocated. Using a `strbuf` everywhere wouldn't buy us much in cases where we cannot reuse it. Patrick
diff --git a/builtin/worktree.c b/builtin/worktree.c index 2cea9441a6..761e302a36 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -151,7 +151,7 @@ static int delete_git_dir(const char *id) struct strbuf sb = STRBUF_INIT; int ret; - strbuf_addstr(&sb, git_common_path("worktrees/%s", id)); + repo_common_path_append(the_repository, &sb, "worktrees/%s", id); ret = remove_dir_recursively(&sb, 0); if (ret < 0 && errno == ENOTDIR) ret = unlink(sb.buf); @@ -1102,6 +1102,7 @@ static int lock_worktree(int ac, const char **av, const char *prefix, OPT_END() }; struct worktree **worktrees, *wt; + char *path; ac = parse_options(ac, av, prefix, options, git_worktree_lock_usage, 0); if (ac != 1) @@ -1122,9 +1123,11 @@ static int lock_worktree(int ac, const char **av, const char *prefix, die(_("'%s' is already locked"), av[0]); } - write_file(git_common_path("worktrees/%s/locked", wt->id), - "%s", reason); + path = repo_common_path(the_repository, "worktrees/%s/locked", wt->id); + write_file(path, "%s", reason); + free_worktrees(worktrees); + free(path); return 0; } @@ -1135,6 +1138,7 @@ static int unlock_worktree(int ac, const char **av, const char *prefix, OPT_END() }; struct worktree **worktrees, *wt; + char *path; int ret; ac = parse_options(ac, av, prefix, options, git_worktree_unlock_usage, 0); @@ -1149,8 +1153,12 @@ static int unlock_worktree(int ac, const char **av, const char *prefix, die(_("The main working tree cannot be locked or unlocked")); if (!worktree_lock_reason(wt)) die(_("'%s' is not locked"), av[0]); - ret = unlink_or_warn(git_common_path("worktrees/%s/locked", wt->id)); + + path = repo_common_path(the_repository, "worktrees/%s/locked", wt->id); + ret = unlink_or_warn(path); + free_worktrees(worktrees); + free(path); return ret; } diff --git a/path.c b/path.c index d721507be8..f6b795d75f 100644 --- a/path.c +++ b/path.c @@ -634,10 +634,10 @@ const char *repo_submodule_path_replace(struct repository *repo, return buf->buf; } -void repo_common_pathv(const struct repository *repo, - struct strbuf *sb, - const char *fmt, - va_list args) +static void repo_common_pathv(const struct repository *repo, + struct strbuf *sb, + const char *fmt, + va_list args) { strbuf_addstr(sb, repo->commondir); if (sb->len && !is_dir_sep(sb->buf[sb->len - 1])) diff --git a/path.h b/path.h index 904eeac068..496f27fdfd 100644 --- a/path.h +++ b/path.h @@ -233,29 +233,10 @@ struct strbuf *get_pathname(void); # include "repository.h" /* Internal implementation details that should not be used. */ -void repo_common_pathv(const struct repository *repo, - struct strbuf *buf, - const char *fmt, - va_list args); void repo_git_pathv(struct repository *repo, const struct worktree *wt, struct strbuf *buf, const char *fmt, va_list args); -/* - * Return a statically allocated path into the main repository's - * (the_repository) common git directory. - */ -__attribute__((format (printf, 1, 2))) -static inline const char *git_common_path(const char *fmt, ...) -{ - struct strbuf *pathname = get_pathname(); - va_list args; - va_start(args, fmt); - repo_common_pathv(the_repository, pathname, fmt, args); - va_end(args); - return pathname->buf; -} - /* * Return a statically allocated path into the main repository's * (the_repository) git directory. diff --git a/worktree.c b/worktree.c index 3b94535963..d5d07d7a84 100644 --- a/worktree.c +++ b/worktree.c @@ -183,7 +183,7 @@ char *get_worktree_git_dir(const struct worktree *wt) else if (!wt->id) return xstrdup(repo_get_common_dir(the_repository)); else - return xstrdup(git_common_path("worktrees/%s", wt->id)); + return repo_common_path(the_repository, "worktrees/%s", wt->id); } static struct worktree *find_worktree_by_suffix(struct worktree **list, @@ -314,6 +314,7 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg, { struct strbuf wt_path = STRBUF_INIT; struct strbuf realpath = STRBUF_INIT; + struct strbuf buf = STRBUF_INIT; char *path = NULL; int err, ret = -1; @@ -343,7 +344,7 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg, if (!is_absolute_path(wt->path)) { strbuf_addf_gently(errmsg, _("'%s' file does not contain absolute path to the working tree location"), - git_common_path("worktrees/%s/gitdir", wt->id)); + repo_common_path_replace(the_repository, &buf, "worktrees/%s/gitdir", wt->id)); goto done; } @@ -365,14 +366,16 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg, goto done; } - strbuf_realpath(&realpath, git_common_path("worktrees/%s", wt->id), 1); + strbuf_realpath(&realpath, repo_common_path_replace(the_repository, &buf, "worktrees/%s", wt->id), 1); ret = fspathcmp(path, realpath.buf); if (ret) strbuf_addf_gently(errmsg, _("'%s' does not point back to '%s'"), - wt->path, git_common_path("worktrees/%s", wt->id)); + wt->path, repo_common_path_replace(the_repository, &buf, + "worktrees/%s", wt->id)); done: free(path); + strbuf_release(&buf); strbuf_release(&wt_path); strbuf_release(&realpath); return ret; @@ -384,11 +387,13 @@ void update_worktree_location(struct worktree *wt, const char *path_, struct strbuf path = STRBUF_INIT; struct strbuf dotgit = STRBUF_INIT; struct strbuf gitdir = STRBUF_INIT; + char *wt_gitdir; if (is_main_worktree(wt)) BUG("can't relocate main worktree"); - strbuf_realpath(&gitdir, git_common_path("worktrees/%s/gitdir", wt->id), 1); + wt_gitdir = repo_common_path(the_repository, "worktrees/%s/gitdir", wt->id); + strbuf_realpath(&gitdir, wt_gitdir, 1); strbuf_realpath(&path, path_, 1); strbuf_addf(&dotgit, "%s/.git", path.buf); if (fspathcmp(wt->path, path.buf)) { @@ -400,6 +405,7 @@ void update_worktree_location(struct worktree *wt, const char *path_, strbuf_release(&path); strbuf_release(&dotgit); strbuf_release(&gitdir); + free(wt_gitdir); } int is_worktree_being_rebased(const struct worktree *wt, @@ -585,6 +591,7 @@ static void repair_gitfile(struct worktree *wt, struct strbuf backlink = STRBUF_INIT; char *dotgit_contents = NULL; const char *repair = NULL; + char *path = NULL; int err; /* missing worktree can't be repaired */ @@ -596,7 +603,8 @@ static void repair_gitfile(struct worktree *wt, goto done; } - strbuf_realpath(&repo, git_common_path("worktrees/%s", wt->id), 1); + path = repo_common_path(the_repository, "worktrees/%s", wt->id); + strbuf_realpath(&repo, path, 1); strbuf_addf(&dotgit, "%s/.git", wt->path); strbuf_addf(&gitdir, "%s/gitdir", repo.buf); dotgit_contents = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err)); @@ -626,6 +634,7 @@ static void repair_gitfile(struct worktree *wt, done: free(dotgit_contents); + free(path); strbuf_release(&repo); strbuf_release(&dotgit); strbuf_release(&gitdir); @@ -657,11 +666,13 @@ void repair_worktree_after_gitdir_move(struct worktree *wt, const char *old_path struct strbuf gitdir = STRBUF_INIT; struct strbuf dotgit = STRBUF_INIT; int is_relative_path; + char *path = NULL; if (is_main_worktree(wt)) goto done; - strbuf_realpath(&gitdir, git_common_path("worktrees/%s/gitdir", wt->id), 1); + path = repo_common_path(the_repository, "worktrees/%s/gitdir", wt->id); + strbuf_realpath(&gitdir, path, 1); if (strbuf_read_file(&dotgit, gitdir.buf, 0) < 0) goto done; @@ -680,6 +691,7 @@ void repair_worktree_after_gitdir_move(struct worktree *wt, const char *old_path done: strbuf_release(&gitdir); strbuf_release(&dotgit); + free(path); } void repair_worktrees_after_gitdir_move(const char *old_path) @@ -871,7 +883,11 @@ int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath, ssize_t read_result; *wtpath = NULL; - strbuf_realpath(&repo, git_common_path("worktrees/%s", id), 1); + + path = repo_common_path(the_repository, "worktrees/%s", id); + strbuf_realpath(&repo, path, 1); + FREE_AND_NULL(path); + strbuf_addf(&gitdir, "%s/gitdir", repo.buf); if (!is_directory(repo.buf)) { strbuf_addstr(reason, _("not a valid directory"));
Remove `git_common_path()` in favor of the `repo_common_path()` family of functions, which makes the implicit dependency on `the_repository` go away. Note that `git_common_path()` used to return a string allocated via `get_pathname()`, which uses a rotating set of statically allocated buffers. Consequently, callers didn't have to free the returned string. The same isn't true for `repo_common_path()`, so we also have to add logic to free the returned strings. This refactoring also allows us to remove `repo_common_pathv()` from the public interface. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- builtin/worktree.c | 16 ++++++++++++---- path.c | 8 ++++---- path.h | 19 ------------------- worktree.c | 32 ++++++++++++++++++++++++-------- 4 files changed, 40 insertions(+), 35 deletions(-)