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

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

Lidong Yan <502024330056@smail.nju.edu.cn> writes:

> Junio C Hamano <gitster@pobox.com> writes：
>> 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.
>
> I was reading the code in master and noticed that repo_get_git_dir()
> no longer sets up the environment. I’ve learned that I should use git blame
> to identify which commit changed the code, so I can make my message clearer.
>
>> 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.
>
> Exactly
>
>> 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?
>
> Yes, I think so. Maybe updating the comment to say 
> “call repo_get_git_dir() after setting up the_repository”
> would be more appropriate.

I do not think so.

e5b17bda (git: ensure correct git directory setup with -h,
2021-12-06) unfortunately moved lines around and made it look like
the comment is about what happens when the if() condition holds, but
if we look at the way how a9ca8a85 (builtins: print setup info if
repo is found, 2010-11-26) initially placed this comment, we can see
that this comment was to only explain the reason why we look at
startup_info->have_repository there.  "Only if we know we have
repository, do the trace_repo_setup() thing because that one calls
get_git_dir() that would die otherwise" is what the comment wants to
say, and if we revert the moving-line-around done by e5b17bda to
recover the original layout in a9ca8a85, I think it is clear enough.

But the theme of this patch is not about improving the comment
anyway, so it should probably be done as a separate patch (if we
really cared, that is).

Thanks.