From: Justin Tobler <jltobler@gmail.com>
Date: Thu, 29 Aug 2024 20:15:36 GMT
Subject: Re: [PATCH 01/21] environment: make `get_git_dir()` accept a repository
Message-ID: <3fvisdomxvcepnfbx2aruzyhqj2ntfjaapk7escwhylxxuq7r4@inhrmm26blg2>
In-Reply-To: <0fe3e3e1cccf2eae7d81ccecf431da623b39fa79.1724923648.git.ps@pks.im>

On 24/08/29 11:38AM, Patrick Steinhardt wrote:
> The `get_git_dir()` function retrieves the path to the Git directory for
> `the_repository`. Make it accept a `struct repository` such that it can
> work on arbitrary repositories and make it part of the repository
> subsystem. This reduces our reliance on `the_repository` and clarifies
> scope.

Seems sensible to me. We are simply making it so `the_repository` is no
longer implicit to the function.

> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
[snip]
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -407,7 +407,7 @@ static const char *prepare_index(const char **argv, const char *prefix,
>  
>  		discard_index(the_repository->index);
>  		read_index_from(the_repository->index, get_lock_file_path(&index_lock),
> -				get_git_dir());
> +				repo_get_git_dir(the_repository));

Now that the `repo_get_git_dir()` function has been moved to
"repository.h" should we also directly include this header in the c
file? Or is the implicit reference sufficent? From our coding guidelines
is seems we preferred to be direct.

It looks like there are several other hunks missing direct references to
"repository.h" too.

[snip]
> diff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c
> index 1593713f4cb..c54e736716a 100644
> --- a/builtin/fsmonitor--daemon.c
> +++ b/builtin/fsmonitor--daemon.c
> @@ -1311,7 +1311,8 @@ static int fsmonitor_run_daemon(void)
>  	strbuf_addstr(&state.path_gitdir_watch, "/.git");
>  	if (!is_directory(state.path_gitdir_watch.buf)) {
>  		strbuf_reset(&state.path_gitdir_watch);
> -		strbuf_addstr(&state.path_gitdir_watch, absolute_path(get_git_dir()));
> +		strbuf_addstr(&state.path_gitdir_watch,
> +			      absolute_path(repo_get_git_dir(the_repository)));

In this c file, now that `repo_get_git_dir()` lives in "repository.h",
is the "environment.h" header file needed anymore?

-Justin