Re: [PATCH 06/20] path: stop relying on `the_repository` in `worktree_git_path()`

From: Justin Tobler <jltobler@gmail.com>
Date: Aug 9, 2024, 19:02 UTC
Message-ID: <cqc4xlbrusu5edfsh2snvekkd2qfwjxu3x33pzizp22rsuidbw@pz4e3fsqfhgv>
In-Reply-To: <67405dcd0a121aee971f854dc35ba89bd4f808c4.1723013714.git.ps@pks.im>

On 24/08/07 08:57AM, Patrick Steinhardt wrote:

> When not provided a worktree, then `worktree_git_path()` will fall back
> to returning a path relative to the main repository. In this case, we
> implicitly rely on `the_repository` to derive the path. Remove this
> dependency by passing a `struct repository` as parameter.

Are there many situations where `worktree_git_path()` would expect to not be provided a worktree? I wonder whether this implicit behavior is really necessary to begin with.

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---

[snip]

Show 9 quoted lines

> -const char *worktree_git_path(const struct worktree *wt, const char *fmt, ...)
> +const char *worktree_git_path(struct repository *r,
> +			      const struct worktree *wt, const char *fmt, ...)
>  {
>  	struct strbuf *pathname = get_pathname();
>  	va_list args;
> +
> +	if (wt && wt->repo != r)
> +		BUG("worktree not connected to expected repository");

So if we receive a worktree, we expect that it should always match the main repostiory. Makes sense.

Show 15 quoted lines

> +
>  	va_start(args, fmt);
> -	repo_git_pathv(the_repository, wt, pathname, fmt, args);
> +	repo_git_pathv(r, wt, pathname, fmt, args);
>  	va_end(args);
>  	return pathname->buf;
>  }
> diff --git a/path.h b/path.h
> index 3d21b9cd16..6228ca03d7 100644
> --- a/path.h
> +++ b/path.h
> @@ -97,9 +97,10 @@ const char *git_path(const char *fmt, ...)
>   * Similar to git_path() but can produce paths for a specified
>   * worktree instead of current one
>   */

Now that the previously implicit behavior is more explicit, it might be update the comment to explain that the provided repository is used as a fallback.

Show 6 quoted lines

> -const char *worktree_git_path(const struct worktree *wt,
> +const char *worktree_git_path(struct repository *r,
> +			      const struct worktree *wt,
>  			      const char *fmt, ...)
> -	__attribute__((format (printf, 2, 3)));
> +	__attribute__((format (printf, 3, 4)));

Overall, I quite like how this series is bubbling up `the_repository` usages from the bottom up. :)