Re: [PATCH 01/21] environment: make `get_git_dir()` accept a repository

From: Justin Tobler <jltobler@gmail.com>
Date: Aug 29, 2024, 20:15 UTC
Message-ID: <3fvisdomxvcepnfbx2aruzyhqj2ntfjaapk7escwhylxxuq7r4@inhrmm26blg2>
In-Reply-To: <0fe3e3e1cccf2eae7d81ccecf431da623b39fa79.1724923648.git.ps@pks.im>

On 24/08/29 11:38AM, Patrick Steinhardt wrote:

Show 5 quoted lines

> 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]

Show 8 quoted lines

> --- 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]

Show 11 quoted lines

> 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