Re: [PATCH 05/16] path: refactor `repo_submodule_path()` family of functions

[shejialuo <shejialuo@gmail.com>]

On Thu, Feb 06, 2025 at 08:58:01AM +0100, Patrick Steinhardt wrote:
> As explained in an earlier commit, we're refactoring path-related
> functions to provide a consistent interface for computing paths into the
> commondir, gitdir and worktree. Refactor the "submodule" family of
> functions accordingly.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/submodule--helper.c |  2 +-
>  path.c                      | 37 +++++++++++++++++++++++++++++--------
>  path.h                      | 30 ++++++++++++++++++------------
>  t/helper/test-ref-store.c   |  7 +++----
>  worktree.c                  |  3 ++-
>  5 files changed, 53 insertions(+), 26 deletions(-)
> 
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 3a64f7e605..c1a8029714 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -1826,7 +1826,7 @@ static int clone_submodule(const struct module_clone_data *clone_data,
>  
>  	connect_work_tree_and_git_dir(clone_data_path, sm_gitdir, 0);
>  
> -	p = git_pathdup_submodule(clone_data_path, "config");
> +	p = repo_submodule_path(the_repository, clone_data_path, "config");
>  	if (!p)
>  		die(_("could not get submodule directory for '%s'"), clone_data_path);
>  
> diff --git a/path.c b/path.c
> index d918d0409e..d721507be8 100644
> --- a/path.c
> +++ b/path.c
> @@ -560,14 +560,15 @@ const char *repo_worktree_path_replace(const struct repository *repo,
>  }
>  
>  /* Returns 0 on success, negative on failure. */
> -static int do_submodule_path(struct strbuf *buf, const char *path,
> +static int do_submodule_path(struct repository *repo,
> +			     struct strbuf *buf, const char *path,
>  			     const char *fmt, va_list args)
>  {
>  	struct strbuf git_submodule_common_dir = STRBUF_INIT;
>  	struct strbuf git_submodule_dir = STRBUF_INIT;
>  	int ret;
>  
> -	ret = submodule_to_gitdir(the_repository, &git_submodule_dir, path);
> +	ret = submodule_to_gitdir(repo, &git_submodule_dir, path);
>  	if (ret)
>  		goto cleanup;
>  
> @@ -586,13 +587,14 @@ static int do_submodule_path(struct strbuf *buf, const char *path,
>  	return ret;
>  }
>  
> -char *git_pathdup_submodule(const char *path, const char *fmt, ...)
> +char *repo_submodule_path(struct repository *repo,
> +			  const char *path, const char *fmt, ...)
>  {
>  	int err;
>  	va_list args;
>  	struct strbuf buf = STRBUF_INIT;
>  	va_start(args, fmt);
> -	err = do_submodule_path(&buf, path, fmt, args);
> +	err = do_submodule_path(repo, &buf, path, fmt, args);
>  	va_end(args);
>  	if (err) {
>  		strbuf_release(&buf);
> @@ -601,16 +603,35 @@ char *git_pathdup_submodule(const char *path, const char *fmt, ...)
>  	return strbuf_detach(&buf, NULL);
>  }
>  
> -int strbuf_git_path_submodule(struct strbuf *buf, const char *path,
> -			      const char *fmt, ...)
> +const char *repo_submodule_path_append(struct repository *repo,
> +				       struct strbuf *buf,
> +				       const char *path,
> +				       const char *fmt, ...)
>  {
>  	int err;
>  	va_list args;
>  	va_start(args, fmt);
> -	err = do_submodule_path(buf, path, fmt, args);
> +	err = do_submodule_path(repo, buf, path, fmt, args);
>  	va_end(args);
> +	if (err)
> +		return NULL;
> +	return buf->buf;
> +}
>  
> -	return err;
> +const char *repo_submodule_path_replace(struct repository *repo,
> +					struct strbuf *buf,
> +					const char *path,
> +					const char *fmt, ...)
> +{
> +	int err;
> +	va_list args;
> +	strbuf_reset(buf);
> +	va_start(args, fmt);
> +	err = do_submodule_path(repo, buf, path, fmt, args);
> +	va_end(args);
> +	if (err)
> +		return NULL;
> +	return buf->buf;
>  }

By reading through the patches from 1 to this. I gradually understand
your design now. For every refactor, we will provide three kinds of
functions. All of these functions will return `const char *` and we
could elegantly use `NULL` to indicate the error.

Thanks,
Jialuo