patchgit.c: remove the_repository dependence in run_builtin()

run_builtin() takes a repo parameter, so the use of the_repository is no longer necessary. Removed the usage of the_repository.

The comment before trace_repo_setup() advises not to use get_git_dir(), but this note is unrelated to trace_repo_setup() itself. Additionally, get_git_dir() has now been renamed to repo_get_git_dir(). Remove this comment line.

Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>
---
 git.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Lidong Yan <yldhome2d2@gmail.com> writes:

Good. The caller always calls this function with the_repository, so this patch does not change anything in the bigger picture.

Isn't it still relevant to explain the reason why this codepath avoids calling the repo_get_git_dir() function?

e5b17bda (git: ensure correct git directory setup with -h, 2021-12-06) tells us that the comment is about use of startup_info->have_repository, which was added by a9ca8a85 (builtins: print setup info if repo is found, 2010-11-26).

Junio C Hamano <gitster@pobox.com> 写道：

In commit a9ca8a85, the intention was to avoid calling get_git_dir() before confirming that we are indeed inside a Git repository and determining the prefix between the current working directory and the repository root.

However, I believe this concern is no longer relevant: repo_get_git_dir() no longer sets up the Git repository environment as the original comment implied. Instead, all the necessary setup is now handled by setup_git_env(), which is invoked by setup_git_directory_gently() after the prefix has been determined. As a result, I believe it is no longer necessary to retain this comment message.

lidongyan <502024330056@smail.nju.edu.cn> writes:

If so, please update the explanation. It reads as if the only reason for removing the comment is because of the rename of the function. If the reason is because the behaviour has changed and it is not relevant anymore, the readers should be told about it.

Thanks.

run_builtin() takes a repo parameter, so the use of the_repository is no longer necessary. Removed the usage of the_repository.

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.

Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>
---
 git.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Lidong Yan <yldhome2d2@gmail.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.

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?

Junio C Hamano <gitster@pobox.com> writes：

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.

Exactly

Yes, I think so. Maybe updating the comment to say “call repo_get_git_dir() after setting up the_repository” would be more appropriate.

Thank you for your review, Lidong

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

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.

Junio C Hamano <gitster@pobox.com> writes：

I’ve changed my mind. Layout like

	if (run_setup &&
		startup_info->have_repository) /* get_git_dir() may set up repo, avoid that */
		trace_repo_setup(repo);

Looks unbalanced, and git-clang-format tries to format this code snippet into

	if (run_setup && startup_info->have_repository) /* get_git_dir() may set
							   up repo, avoid that
							 */

Which looks even worse, I will leave this comment intact.

Thanks, Lidong

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

That is perfectly fine. After all, this is not about comment style. It is about the_repository. Let's stay focused on that.

I've been reading through the git code from the beginning. This patch series fixes some NEEDSWORKs and cleans up some unnecessary uses of the_repository that I came across.

The first commit replace the use of the_repository to run_builtin()'s argument repo. Since each caller pass the_repository to run_builtin(), this replacement is safe. This commit also rewrite a comment before trace_repo_setup(), I am not sure my version of rewrite match the code semantics. So I am reaching out for help and hoping to get some feedback and discussion.

The second commit takes care of a NEEDSWORK in setup_git_directory_gently() we now properly error out if we hit a .git that is not a file or directory when looking for the .git.

Lidong Yan (2):
  git.c: remove the_repository dependence in run_builtin()
  setup: fix NEEDSWORK in setup_git_directory_gently()

 git.c      |  6 +++---
 setup.c    | 11 +++++++----
 setup.h    | 18 +++++++++++-------
 worktree.c |  4 ++--
 4 files changed, 23 insertions(+), 16 deletions(-)

run_builtin() takes a repo parameter, so the use of the_repository is no longer necessary. Removed the usage of the_repository.

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 been renamed to repo_get_git_dir() in commit 246deeac. And later in commit 73f192c9, repo_get_git_dir() stoped calling setup_get_env() anymore. Rewrite origin comment message.

Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>
---
 git.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

In setup.c:setup_git_directory_gently(), the case that fail when dir->buf is neither a file nor a directory is currently marked as NEEDSWORK. Add two new READ_GITFILE_ERR types to handle this case more explicitly:

  * READ_GITFILE_ERR_NOT_A_FILE_OR_DIR:
    path exist, but path is neither a file nor directory
  * READ_GITFILE_ERR_IS_DIR:
    path exist, and it is a directory.

If dir->buf is neither a file nor a directory, then depending on die_or_error, either read_gitfile_gently() will die, or this function will return GIT_DIR_INVALID_GIT_FILE.

To make old use of READ_GITFILE_ERR_NOT_A_FILE still works, Add READ_GITFILE_ERR_NOT_A_FILE(err) macro, which has the same effect as `err == READ_GITFILE_ERR_NOT_A_FILE` in the origin code.

Also add die message for READ_GITFILE_ERR_NOT_A_FILE_OR_DIR in read_gitfile_error_die().

Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>
---
 setup.c    | 11 +++++++----
 setup.h    | 18 +++++++++++-------
 worktree.c |  4 ++--
 3 files changed, 20 insertions(+), 13 deletions(-)

Lidong Yan <yldhome2d2@gmail.com> writes:

FYI, when we have "NEEDSWORK: do X", the intention is often "we haven't spent enough brain cycles when we wrote this comment, so the first step is to evaluate if doing X is a sensible thing in the first place, and only if that is the case, do X".

That change is safe. I'd rather see the comment left intact or reverted to the original shape to clarify what code the comment applies to (see the other message).

We used to just ignore and keep going to check the parent directory, right? Now we would error out when .git is a FIFO or device or any other random things. Is a bit of behaviour change, but I am not sure if it is worth doing. As finding these weird non-file things in your working tree and naming them ".git" is extremely rare and useless (from Git's point of view), I suspect that the user is deliberately doing so for whatever reason they have, so it smells like this change has very little chance to detect a real problem with a larger chance to break a set-up that was deliberately done by the end-user. I dunno.

Thanks.

Junio C Hamano <gitster@pobox.com> writes：

Understood. I initially thought that NEEDSWORK indicated work that was guaranteed to be implemented later.

I’d like to revert the comment to original shape and split it into separate commit. The reason I changed this comment was simply because I found it a bit confusing when reading through the code.

I would just discard the commit about NEEDSWORK.

Thanks for your review, Lidong

run_builtin() takes a repo parameter, so the use of the_repository is no longer necessary. Removed the usage of the_repository.

Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>
---
 git.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)