Re: [PATCH v2] git.c: remove the_repository dependence in run_builtin()

[Junio C Hamano <gitster@pobox.com>]

Lidong Yan <yldhome2d2@gmail.com> writes:

> The comment preceding trace_repo_setup() was originally introduced
> in commit a9ca8a85. Since get_git_dir() modifies global variables
> such as git_dir and git_objects_dir which only valid when inside a git
> repository. The intention of the comment was to emphasize that
> get_git_dir() should not be called before confirming that the current
> directory is indeed part of a git repository. However, get_git_dir()
> has since been renamed to repo_get_git_dir(), and repo_get_git_dir()
> no longer modifies the global the_repository state. As a result,
> the original comment is no longer relevant and can be safely removed.

Sorry, but the above makes it sound as if 246deeac (environment:
make `get_git_dir()` accept a repository, 2024-09-12) that retired
get_git_dir() and introduced repo_get_git_dir() was the culprit that
made their semantics change, but is that really true?  It appears
that in the version immediately before that commit, get_git_dir()
was also a reference to a variable, without any lazy initialization
the above message says that the code tries to avoid, so I am even
more confused after reading the above.

Perhaps you have 73f192c9 (setup: don't perform lazy initialization
of repository state, 2017-06-20) in mind?  That one did stop calling
setup_git_env() and instead force a hard BUG("") when git_dir is not
set up yet.  And that BUG("") still survives in repo_get_git_dir()
we have today.

So the call to repo_get_git_dir() may still not be made from this
code path.  It may not attempt to set up, but instead it would die
if we haven't successfully set up the repository before.  The
relevance of the comment was not changed by 246deeac that moved this
code from get_git_dir() to repo_get_git_dir(), and more importantly,
it was not changed by this patch we are reviewing here.

But stepping back a bit, is it what a9ca8a85 originally wanted to
achieve with this comment to "avoid calling get_git_dir()" in the
first place?  Once the guarding condition is satisfied, it calls
trace_repo_setup(), which in turn calls get_git_dir() anyway.
Perhaps it wanted to explain why startup_info->have_repository is
checked here?

> Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>
> ---
>  git.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/git.c b/git.c
> index 77c4359522..429ad1c2fb 100644
> --- a/git.c
> +++ b/git.c
> @@ -462,12 +462,11 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct
>  	precompose_argv_prefix(argc, argv, NULL);
>  	if (use_pager == -1 && run_setup &&
>  		!(p->option & DELAY_PAGER_CONFIG))
> -		use_pager = check_pager_config(the_repository, p->cmd);
> +		use_pager = check_pager_config(repo, p->cmd);
>  	if (use_pager == -1 && p->option & USE_PAGER)
>  		use_pager = 1;
>  	if (run_setup && startup_info->have_repository)
> -		/* get_git_dir() may set up repo, avoid that */
> -		trace_repo_setup(the_repository);
> +		trace_repo_setup(repo);
>  	commit_pager_choice();
>  
>  	if (!help && p->option & NEED_WORK_TREE)