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
