Re: [PATCH 10/16] path: drop `git_common_path()` in favor of `repo_common_path()`

[Patrick Steinhardt <ps@pks.im>]

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