Re: [PATCH 01/16] path: refactor `repo_common_path()` family of functions

[shejialuo <shejialuo@gmail.com>]

On Thu, Feb 06, 2025 at 08:57:57AM +0100, Patrick Steinhardt wrote:
> The functions provided by the "path" subsystem to derive repository
> paths for the commondir, gitdir, worktrees and submodules are quite
> inconsistent. Some functions have a `strbuf_` prefix, others have
> different return values, some don't provide a variant working on top of
> `strbuf`s.
> 
> We're thus about to refactor all of these family of functions so that
> they follow a common pattern:
> 
>   - `repo_*_path()` returns an allocated string.
> 
>   - `repo_*_path_append()` appends the path to the caller-provided
>     buffer while returning a constant pointer to the buffer. This
>     clarifies whether the buffer is being appended to or rewritten,
>     which otherwise wasn't immediately obvious.
> 
>   - `repo_*_path_replace()` replaces contents of the buffer with the
>     computed path, again returning a pointer to the buffer contents.
> 

I want to ask a design question about this. Why do we need to return the
raw pointer to the `struct strbuf` for the last two cases? I somehow
understand why you want to do this. You want to follow a common pattern
for those three functions. But I wonder should we let the caller to
decide whether they want to use the raw pointer?

And in this patch, the return value of the last two cases has never been
used. Until I read the next patch, I have seen the usage of the return
value thus I could understand your motivation.

> diff --git a/path.h b/path.h
> index 5f6c85e5f8..3c75495e1a 100644
> --- a/path.h
> +++ b/path.h
> @@ -25,22 +25,20 @@ char *mkpathdup(const char *fmt, ...)
>  	__attribute__((format (printf, 1, 2)));
>  
>  /*
> - * The `strbuf_git_common_path` family of functions will construct a path into a
> + * The `repo_common_path` family of functions will construct a path into a
>   * repository's common git directory, which is shared by all worktrees.
>   */
> -
> -/*
> - * Constructs a path into the common git directory of repository `repo` and
> - * append it in the provided buffer `sb`.
> - */
> -void strbuf_git_common_path(struct strbuf *sb,
> -			    const struct repository *repo,
> -			    const char *fmt, ...)
> +char *repo_common_path(const struct repository *repo,
> +		       const char *fmt, ...)
> +	__attribute__((format (printf, 2, 3)));
> +const char *repo_common_path_append(const struct repository *repo,
> +				    struct strbuf *sb,
> +				    const char *fmt, ...)
> +	__attribute__((format (printf, 3, 4)));
> +const char *repo_common_path_replace(const struct repository *repo,
> +				     struct strbuf *sb,
> +				     const char *fmt, ...)
>  	__attribute__((format (printf, 3, 4)));
> -void repo_common_pathv(const struct repository *repo,
> -		       struct strbuf *buf,
> -		       const char *fmt,
> -		       va_list args);
>  
>  /*
>   * The `repo_git_path` family of functions will construct a path into a repository's
> @@ -243,6 +241,12 @@ struct strbuf *get_pathname(void);
>  #  include "strbuf.h"
>  #  include "repository.h"
>  
> +/* Internal implementation detail that should not be used. */
> +void repo_common_pathv(const struct repository *repo,
> +		       struct strbuf *buf,
> +		       const char *fmt,
> +		       va_list args);
> +

If we decide to make this as internal implementation, why we don't just
delete this declaration in the header file? Do I miss out something
here?

Thanks,
Jialuo