{"thread":{"id":"63632","subject":"[PATCH] git.c: remove the_repository dependence in run_builtin()","startedAt":"2025-06-12T04:59:25Z","lastAt":"2025-06-16T06:22:42Z","messageCount":16,"participants":["Lidong Yan","Junio C Hamano","lidongyan"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"520163","messageId":"20250612045905.3023227-1-502024330056@smail.nju.edu.cn","threadId":"63632","inReplyTo":null,"subject":"[PATCH] git.c: remove the_repository dependence in run_builtin()","fromName":"Lidong Yan","fromEmail":"yldhome2d2@gmail.com","sentAt":"2025-06-12T04:59:05Z","receivedAt":"2025-06-12T04:59:25Z","isPatch":true,"sender":{"key":"yldhome2d2@gmail.com","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"run_builtin() takes a repo parameter, so the use of the_repository\nis no longer necessary. Removed the usage of the_repository.\n\nThe comment before trace_repo_setup() advises not to use get_git_dir(),\nbut this note is unrelated to trace_repo_setup() itself. Additionally,\nget_git_dir() has now been renamed to repo_get_git_dir(). Remove this\ncomment line.\n\nSigned-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n---\n git.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex 77c4359522..429ad1c2fb 100644\n--- a/git.c\n+++ b/git.c\n@@ -462,12 +462,11 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct\n \tprecompose_argv_prefix(argc, argv, NULL);\n \tif (use_pager == -1 && run_setup &&\n \t\t!(p->option & DELAY_PAGER_CONFIG))\n-\t\tuse_pager = check_pager_config(the_repository, p->cmd);\n+\t\tuse_pager = check_pager_config(repo, p->cmd);\n \tif (use_pager == -1 && p->option & USE_PAGER)\n \t\tuse_pager = 1;\n \tif (run_setup && startup_info->have_repository)\n-\t\t/* get_git_dir() may set up repo, avoid that */\n-\t\ttrace_repo_setup(the_repository);\n+\t\ttrace_repo_setup(repo);\n \tcommit_pager_choice();\n \n \tif (!help && p->option & NEED_WORK_TREE)\n-- \n2.43.0\n\n"},{"id":"520180","messageId":"xmqqecvoev8g.fsf@gitster.g","threadId":"63632","inReplyTo":"20250612045905.3023227-1-502024330056@smail.nju.edu.cn","subject":"Re: [PATCH] git.c: remove the_repository dependence in run_builtin()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-12T17:16:15Z","receivedAt":"2025-06-12T17:16:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lidong Yan <yldhome2d2@gmail.com> writes:\n\n> run_builtin() takes a repo parameter, so the use of the_repository\n> is no longer necessary. Removed the usage of the_repository.\n\nGood.  The caller always calls this function with the_repository, so\nthis patch does not change anything in the bigger picture.\n\n> The comment before trace_repo_setup() advises not to use get_git_dir(),\n> but this note is unrelated to trace_repo_setup() itself. Additionally,\n> get_git_dir() has now been renamed to repo_get_git_dir(). Remove this\n> comment line.\n\nIsn't it still relevant to explain the reason why this codepath\navoids calling the repo_get_git_dir() function?\n\ne5b17bda (git: ensure correct git directory setup with -h,\n2021-12-06) tells us that the comment is about use of\nstartup_info->have_repository, which was added by a9ca8a85\n(builtins: print setup info if repo is found, 2010-11-26).\n\n> Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n> ---\n>  git.c | 5 ++---\n>  1 file changed, 2 insertions(+), 3 deletions(-)\n>\n> diff --git a/git.c b/git.c\n> index 77c4359522..429ad1c2fb 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -462,12 +462,11 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct\n>  \tprecompose_argv_prefix(argc, argv, NULL);\n>  \tif (use_pager == -1 && run_setup &&\n>  \t\t!(p->option & DELAY_PAGER_CONFIG))\n> -\t\tuse_pager = check_pager_config(the_repository, p->cmd);\n> +\t\tuse_pager = check_pager_config(repo, p->cmd);\n>  \tif (use_pager == -1 && p->option & USE_PAGER)\n>  \t\tuse_pager = 1;\n>  \tif (run_setup && startup_info->have_repository)\n> -\t\t/* get_git_dir() may set up repo, avoid that */\n> -\t\ttrace_repo_setup(the_repository);\n> +\t\ttrace_repo_setup(repo);\n>  \tcommit_pager_choice();\n>  \n>  \tif (!help && p->option & NEED_WORK_TREE)\n"},{"id":"520205","messageId":"11AE19A1-7B19-45CE-AFCE-98D89A4570F7@smail.nju.edu.cn","threadId":"63632","inReplyTo":"xmqqecvoev8g.fsf@gitster.g","subject":"Re: [PATCH] git.c: remove the_repository dependence in run_builtin()","fromName":"lidongyan","fromEmail":"502024330056@smail.nju.edu.cn","sentAt":"2025-06-13T01:53:40Z","receivedAt":"2025-06-13T01:55:31Z","isPatch":true,"sender":{"key":"502024330056@smail.nju.edu.cn","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"Junio C Hamano <gitster@pobox.com> 写道：\n> \n> Lidong Yan <yldhome2d2@gmail.com> writes:\n> \n>> run_builtin() takes a repo parameter, so the use of the_repository\n>> is no longer necessary. Removed the usage of the_repository.\n> \n> Good.  The caller always calls this function with the_repository, so\n> this patch does not change anything in the bigger picture.\n> \n>> The comment before trace_repo_setup() advises not to use get_git_dir(),\n>> but this note is unrelated to trace_repo_setup() itself. Additionally,\n>> get_git_dir() has now been renamed to repo_get_git_dir(). Remove this\n>> comment line.\n> \n> Isn't it still relevant to explain the reason why this codepath\n> avoids calling the repo_get_git_dir() function?\n> \n> e5b17bda (git: ensure correct git directory setup with -h,\n> 2021-12-06) tells us that the comment is about use of\n> startup_info->have_repository, which was added by a9ca8a85\n> (builtins: print setup info if repo is found, 2010-11-26).\n\nIn commit a9ca8a85, the intention was to avoid calling get_git_dir() before\nconfirming that we are indeed inside a Git repository and determining the prefix\nbetween the current working directory and the repository root.\n\nHowever, I believe this concern is no longer relevant:\nrepo_get_git_dir() no longer sets up the Git repository environment as the\noriginal comment implied. Instead, all the necessary setup is now handled\nby setup_git_env(), which is invoked by setup_git_directory_gently() after\nthe prefix has been determined. As a result, I believe it is no longer necessary\nto retain this comment message."},{"id":"520206","messageId":"xmqq5xh0b60c.fsf@gitster.g","threadId":"63632","inReplyTo":"11AE19A1-7B19-45CE-AFCE-98D89A4570F7@smail.nju.edu.cn","subject":"Re: [PATCH] git.c: remove the_repository dependence in run_builtin()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-13T04:49:23Z","receivedAt":"2025-06-13T04:49:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"lidongyan <502024330056@smail.nju.edu.cn> writes:\n\n>>> The comment before trace_repo_setup() advises not to use get_git_dir(),\n>>> but this note is unrelated to trace_repo_setup() itself. Additionally,\n>>> get_git_dir() has now been renamed to repo_get_git_dir(). Remove this\n>>> comment line.\n>> ... \n> However, I believe this concern is no longer relevant:\n> repo_get_git_dir() no longer sets up the Git repository environment as the\n> original comment implied. Instead, all the necessary setup is now handled\n> by setup_git_env(), which is invoked by setup_git_directory_gently() after\n> the prefix has been determined. As a result, I believe it is no longer necessary\n> to retain this comment message.\n\nIf so, please update the explanation.  It reads as if the only\nreason for removing the comment is because of the rename of the\nfunction.  If the reason is because the behaviour has changed and it\nis not relevant anymore, the readers should be told about it.\n\nThanks.\n"},{"id":"520243","messageId":"20250614050331.304405-1-502024330056@smail.nju.edu.cn","threadId":"63632","inReplyTo":"20250612045905.3023227-1-502024330056@smail.nju.edu.cn","subject":"[PATCH v2] git.c: remove the_repository dependence in run_builtin()","fromName":"Lidong Yan","fromEmail":"yldhome2d2@gmail.com","sentAt":"2025-06-14T05:03:31Z","receivedAt":"2025-06-14T05:03:57Z","isPatch":true,"sender":{"key":"yldhome2d2@gmail.com","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"run_builtin() takes a repo parameter, so the use of the_repository\nis no longer necessary. Removed the usage of the_repository.\n\nThe comment preceding trace_repo_setup() was originally introduced\nin commit a9ca8a85. Since get_git_dir() modifies global variables\nsuch as git_dir and git_objects_dir which only valid when inside a git\nrepository. The intention of the comment was to emphasize that\nget_git_dir() should not be called before confirming that the current\ndirectory is indeed part of a git repository. However, get_git_dir()\nhas since been renamed to repo_get_git_dir(), and repo_get_git_dir()\nno longer modifies the global the_repository state. As a result,\nthe original comment is no longer relevant and can be safely removed.\n\nSigned-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n---\n git.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex 77c4359522..429ad1c2fb 100644\n--- a/git.c\n+++ b/git.c\n@@ -462,12 +462,11 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct\n \tprecompose_argv_prefix(argc, argv, NULL);\n \tif (use_pager == -1 && run_setup &&\n \t\t!(p->option & DELAY_PAGER_CONFIG))\n-\t\tuse_pager = check_pager_config(the_repository, p->cmd);\n+\t\tuse_pager = check_pager_config(repo, p->cmd);\n \tif (use_pager == -1 && p->option & USE_PAGER)\n \t\tuse_pager = 1;\n \tif (run_setup && startup_info->have_repository)\n-\t\t/* get_git_dir() may set up repo, avoid that */\n-\t\ttrace_repo_setup(the_repository);\n+\t\ttrace_repo_setup(repo);\n \tcommit_pager_choice();\n \n \tif (!help && p->option & NEED_WORK_TREE)\n-- \n2.50.0-rc2\n\n"},{"id":"520254","messageId":"xmqqwm9d6gn0.fsf@gitster.g","threadId":"63632","inReplyTo":"20250614050331.304405-1-502024330056@smail.nju.edu.cn","subject":"Re: [PATCH v2] git.c: remove the_repository dependence in run_builtin()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-14T23:35:31Z","receivedAt":"2025-06-14T23:35:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lidong Yan <yldhome2d2@gmail.com> writes:\n\n> The comment preceding trace_repo_setup() was originally introduced\n> in commit a9ca8a85. Since get_git_dir() modifies global variables\n> such as git_dir and git_objects_dir which only valid when inside a git\n> repository. The intention of the comment was to emphasize that\n> get_git_dir() should not be called before confirming that the current\n> directory is indeed part of a git repository. However, get_git_dir()\n> has since been renamed to repo_get_git_dir(), and repo_get_git_dir()\n> no longer modifies the global the_repository state. As a result,\n> the original comment is no longer relevant and can be safely removed.\n\nSorry, but the above makes it sound as if 246deeac (environment:\nmake `get_git_dir()` accept a repository, 2024-09-12) that retired\nget_git_dir() and introduced repo_get_git_dir() was the culprit that\nmade their semantics change, but is that really true?  It appears\nthat in the version immediately before that commit, get_git_dir()\nwas also a reference to a variable, without any lazy initialization\nthe above message says that the code tries to avoid, so I am even\nmore confused after reading the above.\n\nPerhaps you have 73f192c9 (setup: don't perform lazy initialization\nof repository state, 2017-06-20) in mind?  That one did stop calling\nsetup_git_env() and instead force a hard BUG(\"\") when git_dir is not\nset up yet.  And that BUG(\"\") still survives in repo_get_git_dir()\nwe have today.\n\nSo the call to repo_get_git_dir() may still not be made from this\ncode path.  It may not attempt to set up, but instead it would die\nif we haven't successfully set up the repository before.  The\nrelevance of the comment was not changed by 246deeac that moved this\ncode from get_git_dir() to repo_get_git_dir(), and more importantly,\nit was not changed by this patch we are reviewing here.\n\nBut stepping back a bit, is it what a9ca8a85 originally wanted to\nachieve with this comment to \"avoid calling get_git_dir()\" in the\nfirst place?  Once the guarding condition is satisfied, it calls\ntrace_repo_setup(), which in turn calls get_git_dir() anyway.\nPerhaps it wanted to explain why startup_info->have_repository is\nchecked here?\n\n> Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n> ---\n>  git.c | 5 ++---\n>  1 file changed, 2 insertions(+), 3 deletions(-)\n>\n> diff --git a/git.c b/git.c\n> index 77c4359522..429ad1c2fb 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -462,12 +462,11 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct\n>  \tprecompose_argv_prefix(argc, argv, NULL);\n>  \tif (use_pager == -1 && run_setup &&\n>  \t\t!(p->option & DELAY_PAGER_CONFIG))\n> -\t\tuse_pager = check_pager_config(the_repository, p->cmd);\n> +\t\tuse_pager = check_pager_config(repo, p->cmd);\n>  \tif (use_pager == -1 && p->option & USE_PAGER)\n>  \t\tuse_pager = 1;\n>  \tif (run_setup && startup_info->have_repository)\n> -\t\t/* get_git_dir() may set up repo, avoid that */\n> -\t\ttrace_repo_setup(the_repository);\n> +\t\ttrace_repo_setup(repo);\n>  \tcommit_pager_choice();\n>  \n>  \tif (!help && p->option & NEED_WORK_TREE)\n"},{"id":"520262","messageId":"BE43915C-E780-4166-9C23-81F9A8CBDEDC@smail.nju.edu.cn","threadId":"63632","inReplyTo":"xmqqwm9d6gn0.fsf@gitster.g","subject":"Re: [PATCH v2] git.c: remove the_repository dependence in run_builtin()","fromName":"Lidong Yan","fromEmail":"502024330056@smail.nju.edu.cn","sentAt":"2025-06-15T01:49:55Z","receivedAt":"2025-06-15T01:51:08Z","isPatch":true,"sender":{"key":"502024330056@smail.nju.edu.cn","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes：\n> Sorry, but the above makes it sound as if 246deeac (environment:\n> make `get_git_dir()` accept a repository, 2024-09-12) that retired\n> get_git_dir() and introduced repo_get_git_dir() was the culprit that\n> made their semantics change, but is that really true?  It appears\n> that in the version immediately before that commit, get_git_dir()\n> was also a reference to a variable, without any lazy initialization\n> the above message says that the code tries to avoid, so I am even\n> more confused after reading the above.\n\nI was reading the code in master and noticed that repo_get_git_dir()\nno longer sets up the environment. I’ve learned that I should use git blame\nto identify which commit changed the code, so I can make my message clearer.\n\n> Perhaps you have 73f192c9 (setup: don't perform lazy initialization\n> of repository state, 2017-06-20) in mind?  That one did stop calling\n> setup_git_env() and instead force a hard BUG(\"\") when git_dir is not\n> set up yet.  And that BUG(\"\") still survives in repo_get_git_dir()\n> we have today.\n\nExactly\n\n> So the call to repo_get_git_dir() may still not be made from this\n> code path.  It may not attempt to set up, but instead it would die\n> if we haven't successfully set up the repository before.  The\n> relevance of the comment was not changed by 246deeac that moved this\n> code from get_git_dir() to repo_get_git_dir(), and more importantly,\n> it was not changed by this patch we are reviewing here.\n> \n> But stepping back a bit, is it what a9ca8a85 originally wanted to\n> achieve with this comment to \"avoid calling get_git_dir()\" in the\n> first place?  Once the guarding condition is satisfied, it calls\n> trace_repo_setup(), which in turn calls get_git_dir() anyway.\n> Perhaps it wanted to explain why startup_info->have_repository is\n> checked here?\n\nYes, I think so. Maybe updating the comment to say \n“call repo_get_git_dir() after setting up the_repository”\nwould be more appropriate.\n\nThank you for your review,\nLidong\n\n"},{"id":"520266","messageId":"20250615144604.1447302-1-502024330056@smail.nju.edu.cn","threadId":"63632","inReplyTo":"20250614050331.304405-1-502024330056@smail.nju.edu.cn","subject":"[RFC PATCH v3 0/2] small fixes for git.c and setup.c","fromName":"Lidong Yan","fromEmail":"yldhome2d2@gmail.com","sentAt":"2025-06-15T14:46:02Z","receivedAt":"2025-06-15T14:46:44Z","isPatch":true,"sender":{"key":"yldhome2d2@gmail.com","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"I've been reading through the git code from the beginning. This\npatch series fixes some NEEDSWORKs and cleans up some unnecessary\nuses of the_repository that I came across.\n\nThe first commit replace the use of the_repository to run_builtin()'s\nargument repo. Since each caller pass the_repository to run_builtin(),\nthis replacement is safe. This commit also rewrite a comment before\ntrace_repo_setup(), I am not sure my version of rewrite match the code\nsemantics. So I am reaching out for help and hoping to get some feedback\nand discussion.\n\nThe second commit takes care of a NEEDSWORK in setup_git_directory_gently()\nwe now properly error out if we hit a .git that is not a file or directory\nwhen looking for the .git.\n\nLidong Yan (2):\n  git.c: remove the_repository dependence in run_builtin()\n  setup: fix NEEDSWORK in setup_git_directory_gently()\n\n git.c      |  6 +++---\n setup.c    | 11 +++++++----\n setup.h    | 18 +++++++++++-------\n worktree.c |  4 ++--\n 4 files changed, 23 insertions(+), 16 deletions(-)\n\n-- \n2.50.0-rc1\n\n"},{"id":"520267","messageId":"20250615144604.1447302-2-502024330056@smail.nju.edu.cn","threadId":"63632","inReplyTo":"20250615144604.1447302-1-502024330056@smail.nju.edu.cn","subject":"[PATCH v3 1/2] git.c: remove the_repository dependence in run_builtin()","fromName":"Lidong Yan","fromEmail":"yldhome2d2@gmail.com","sentAt":"2025-06-15T14:46:03Z","receivedAt":"2025-06-15T14:46:54Z","isPatch":true,"sender":{"key":"yldhome2d2@gmail.com","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"run_builtin() takes a repo parameter, so the use of the_repository\nis no longer necessary. Removed the usage of the_repository.\n\nThe comment preceding trace_repo_setup() was originally introduced\nin commit a9ca8a85. Since get_git_dir() modifies global variables\nsuch as git_dir and git_objects_dir which only valid when inside a git\nrepository. The intention of the comment was to emphasize that\nget_git_dir() should not be called before confirming that the current\ndirectory is indeed part of a git repository. However, get_git_dir()\nhas been renamed to repo_get_git_dir() in commit 246deeac. And later\nin commit 73f192c9, repo_get_git_dir() stoped calling setup_get_env()\nanymore. Rewrite origin comment message.\n\nSigned-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n---\n git.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex 77c4359522..7fa81dde18 100644\n--- a/git.c\n+++ b/git.c\n@@ -462,12 +462,12 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct\n \tprecompose_argv_prefix(argc, argv, NULL);\n \tif (use_pager == -1 && run_setup &&\n \t\t!(p->option & DELAY_PAGER_CONFIG))\n-\t\tuse_pager = check_pager_config(the_repository, p->cmd);\n+\t\tuse_pager = check_pager_config(repo, p->cmd);\n \tif (use_pager == -1 && p->option & USE_PAGER)\n \t\tuse_pager = 1;\n \tif (run_setup && startup_info->have_repository)\n-\t\t/* get_git_dir() may set up repo, avoid that */\n-\t\ttrace_repo_setup(the_repository);\n+\t\t/* avoid repo_get_git_dir(), repo must be set up */\n+\t\ttrace_repo_setup(repo);\n \tcommit_pager_choice();\n \n \tif (!help && p->option & NEED_WORK_TREE)\n-- \n2.50.0-rc1\n\n"},{"id":"520268","messageId":"20250615144604.1447302-3-502024330056@smail.nju.edu.cn","threadId":"63632","inReplyTo":"20250615144604.1447302-1-502024330056@smail.nju.edu.cn","subject":"[PATCH v3 2/2] setup: fix NEEDSWORK in setup_git_directory_gently()","fromName":"Lidong Yan","fromEmail":"yldhome2d2@gmail.com","sentAt":"2025-06-15T14:46:04Z","receivedAt":"2025-06-15T14:46:56Z","isPatch":true,"sender":{"key":"yldhome2d2@gmail.com","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"In setup.c:setup_git_directory_gently(), the case that fail when dir->buf\nis neither a file nor a directory is currently marked as NEEDSWORK. Add\ntwo new READ_GITFILE_ERR types to handle this case more explicitly:\n\n  * READ_GITFILE_ERR_NOT_A_FILE_OR_DIR:\n    path exist, but path is neither a file nor directory\n  * READ_GITFILE_ERR_IS_DIR:\n    path exist, and it is a directory.\n\nIf dir->buf is neither a file nor a directory, then depending on\ndie_or_error, either read_gitfile_gently() will die, or this\nfunction will return GIT_DIR_INVALID_GIT_FILE.\n\nTo make old use of READ_GITFILE_ERR_NOT_A_FILE still works,\nAdd READ_GITFILE_ERR_NOT_A_FILE(err) macro, which has the same\neffect as `err == READ_GITFILE_ERR_NOT_A_FILE` in the origin code.\n\nAlso add die message for READ_GITFILE_ERR_NOT_A_FILE_OR_DIR in\nread_gitfile_error_die().\n\nSigned-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n---\n setup.c    | 11 +++++++----\n setup.h    | 18 +++++++++++-------\n worktree.c |  4 ++--\n 3 files changed, 20 insertions(+), 13 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex f93bd6a24a..b18c43d93b 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -893,9 +893,11 @@ void read_gitfile_error_die(int error_code, const char *path, const char *dir)\n {\n \tswitch (error_code) {\n \tcase READ_GITFILE_ERR_STAT_FAILED:\n-\tcase READ_GITFILE_ERR_NOT_A_FILE:\n+\tcase READ_GITFILE_ERR_IS_DIR:\n \t\t/* non-fatal; follow return path */\n \t\tbreak;\n+\tcase READ_GITFILE_ERR_NOT_A_FILE_OR_DIR:\n+\t\tdie(_(\"'%s' is not a file or directory\"), path);\n \tcase READ_GITFILE_ERR_OPEN_FAILED:\n \t\tdie_errno(_(\"error opening '%s'\"), path);\n \tcase READ_GITFILE_ERR_TOO_LARGE:\n@@ -941,7 +943,9 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)\n \t\tgoto cleanup_return;\n \t}\n \tif (!S_ISREG(st.st_mode)) {\n-\t\terror_code = READ_GITFILE_ERR_NOT_A_FILE;\n+\t\terror_code = S_ISDIR(st.st_mode) ?\n+\t\t\t\t     READ_GITFILE_ERR_IS_DIR :\n+\t\t\t\t     READ_GITFILE_ERR_NOT_A_FILE_OR_DIR;\n \t\tgoto cleanup_return;\n \t}\n \tif (st.st_size > max_file_size) {\n@@ -1499,8 +1503,7 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n \t\t\t\t\t\tNULL : &error_code);\n \t\tif (!gitdirenv) {\n \t\t\tif (die_on_error ||\n-\t\t\t    error_code == READ_GITFILE_ERR_NOT_A_FILE) {\n-\t\t\t\t/* NEEDSWORK: fail if .git is not file nor dir */\n+\t\t\t    error_code == READ_GITFILE_ERR_IS_DIR) {\n \t\t\t\tif (is_git_directory(dir->buf)) {\n \t\t\t\t\tgitdirenv = DEFAULT_GIT_DIR_ENVIRONMENT;\n \t\t\t\t\tgitdir_path = xstrdup(dir->buf);\ndiff --git a/setup.h b/setup.h\nindex 18dc3b7368..7f246ad248 100644\n--- a/setup.h\n+++ b/setup.h\n@@ -29,13 +29,17 @@ int is_git_directory(const char *path);\n int is_nonbare_repository_dir(struct strbuf *path);\n \n #define READ_GITFILE_ERR_STAT_FAILED 1\n-#define READ_GITFILE_ERR_NOT_A_FILE 2\n-#define READ_GITFILE_ERR_OPEN_FAILED 3\n-#define READ_GITFILE_ERR_READ_FAILED 4\n-#define READ_GITFILE_ERR_INVALID_FORMAT 5\n-#define READ_GITFILE_ERR_NO_PATH 6\n-#define READ_GITFILE_ERR_NOT_A_REPO 7\n-#define READ_GITFILE_ERR_TOO_LARGE 8\n+#define READ_GITFILE_ERR_IS_DIR\t\t   2\n+#define READ_GITFILE_ERR_NOT_A_FILE_OR_DIR 3\n+#define READ_GITFILE_ERR_OPEN_FAILED\t   4\n+#define READ_GITFILE_ERR_READ_FAILED\t   5\n+#define READ_GITFILE_ERR_INVALID_FORMAT\t   6\n+#define READ_GITFILE_ERR_NO_PATH\t   7\n+#define READ_GITFILE_ERR_NOT_A_REPO\t   8\n+#define READ_GITFILE_ERR_TOO_LARGE\t   9\n+#define READ_GITFILE_ERR_NOT_A_FILE(x)                \\\n+\t((x) == READ_GITFILE_ERR_NOT_A_FILE_OR_DIR || \\\n+\t (x) == READ_GITFILE_ERR_IS_DIR)\n void read_gitfile_error_die(int error_code, const char *path, const char *dir);\n const char *read_gitfile_gently(const char *path, int *return_error_code);\n #define read_gitfile(path) read_gitfile_gently((path), NULL)\ndiff --git a/worktree.c b/worktree.c\nindex c34b9eb74e..3f0e9748ec 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -646,7 +646,7 @@ static void repair_gitfile(struct worktree *wt,\n \t\t}\n \t}\n \n-\tif (err == READ_GITFILE_ERR_NOT_A_FILE)\n+\tif (READ_GITFILE_ERR_NOT_A_FILE(err))\n \t\tfn(1, wt->path, _(\".git is not a file\"), cb_data);\n \telse if (err)\n \t\trepair = _(\".git file broken\");\n@@ -826,7 +826,7 @@ void repair_worktree_at_path(const char *path,\n \t\t\tstrbuf_addstr(&backlink, dotgit_contents);\n \t\t\tstrbuf_realpath_forgiving(&backlink, backlink.buf, 0);\n \t\t}\n-\t} else if (err == READ_GITFILE_ERR_NOT_A_FILE) {\n+\t} else if (READ_GITFILE_ERR_NOT_A_FILE(err)) {\n \t\tfn(1, dotgit.buf, _(\"unable to locate repository; .git is not a file\"), cb_data);\n \t\tgoto done;\n \t} else if (err == READ_GITFILE_ERR_NOT_A_REPO) {\n-- \n2.50.0-rc1\n\n"},{"id":"520273","messageId":"xmqqsek04id9.fsf@gitster.g","threadId":"63632","inReplyTo":"BE43915C-E780-4166-9C23-81F9A8CBDEDC@smail.nju.edu.cn","subject":"Re: [PATCH v2] git.c: remove the_repository dependence in run_builtin()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-16T00:53:22Z","receivedAt":"2025-06-16T00:53:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lidong Yan <502024330056@smail.nju.edu.cn> writes:\n\n> Junio C Hamano <gitster@pobox.com> writes：\n>> Sorry, but the above makes it sound as if 246deeac (environment:\n>> make `get_git_dir()` accept a repository, 2024-09-12) that retired\n>> get_git_dir() and introduced repo_get_git_dir() was the culprit that\n>> made their semantics change, but is that really true?  It appears\n>> that in the version immediately before that commit, get_git_dir()\n>> was also a reference to a variable, without any lazy initialization\n>> the above message says that the code tries to avoid, so I am even\n>> more confused after reading the above.\n>\n> I was reading the code in master and noticed that repo_get_git_dir()\n> no longer sets up the environment. I’ve learned that I should use git blame\n> to identify which commit changed the code, so I can make my message clearer.\n>\n>> Perhaps you have 73f192c9 (setup: don't perform lazy initialization\n>> of repository state, 2017-06-20) in mind?  That one did stop calling\n>> setup_git_env() and instead force a hard BUG(\"\") when git_dir is not\n>> set up yet.  And that BUG(\"\") still survives in repo_get_git_dir()\n>> we have today.\n>\n> Exactly\n>\n>> So the call to repo_get_git_dir() may still not be made from this\n>> code path.  It may not attempt to set up, but instead it would die\n>> if we haven't successfully set up the repository before.  The\n>> relevance of the comment was not changed by 246deeac that moved this\n>> code from get_git_dir() to repo_get_git_dir(), and more importantly,\n>> it was not changed by this patch we are reviewing here.\n>> \n>> But stepping back a bit, is it what a9ca8a85 originally wanted to\n>> achieve with this comment to \"avoid calling get_git_dir()\" in the\n>> first place?  Once the guarding condition is satisfied, it calls\n>> trace_repo_setup(), which in turn calls get_git_dir() anyway.\n>> Perhaps it wanted to explain why startup_info->have_repository is\n>> checked here?\n>\n> Yes, I think so. Maybe updating the comment to say \n> “call repo_get_git_dir() after setting up the_repository”\n> would be more appropriate.\n\nI do not think so.\n\ne5b17bda (git: ensure correct git directory setup with -h,\n2021-12-06) unfortunately moved lines around and made it look like\nthe comment is about what happens when the if() condition holds, but\nif we look at the way how a9ca8a85 (builtins: print setup info if\nrepo is found, 2010-11-26) initially placed this comment, we can see\nthat this comment was to only explain the reason why we look at\nstartup_info->have_repository there.  \"Only if we know we have\nrepository, do the trace_repo_setup() thing because that one calls\nget_git_dir() that would die otherwise\" is what the comment wants to\nsay, and if we revert the moving-line-around done by e5b17bda to\nrecover the original layout in a9ca8a85, I think it is clear enough.\n\nBut the theme of this patch is not about improving the comment\nanyway, so it should probably be done as a separate patch (if we\nreally cared, that is).\n\nThanks.\n"},{"id":"520275","messageId":"xmqqbjqo4gw7.fsf@gitster.g","threadId":"63632","inReplyTo":"20250615144604.1447302-1-502024330056@smail.nju.edu.cn","subject":"Re: [RFC PATCH v3 0/2] small fixes for git.c and setup.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-16T01:25:12Z","receivedAt":"2025-06-16T01:25:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lidong Yan <yldhome2d2@gmail.com> writes:\n\n> I've been reading through the git code from the beginning. This\n> patch series fixes some NEEDSWORKs and cleans up some unnecessary\n> uses of the_repository that I came across.\n\nFYI, when we have \"NEEDSWORK: do X\", the intention is often \"we\nhaven't spent enough brain cycles when we wrote this comment, so the\nfirst step is to evaluate if doing X is a sensible thing in the\nfirst place, and only if that is the case, do X\".\n\n> The first commit replace the use of the_repository to run_builtin()'s\n> argument repo. Since each caller pass the_repository to run_builtin(),\n> this replacement is safe.\n\nThat change is safe.  I'd rather see the comment left intact or\nreverted to the original shape to clarify what code the comment\napplies to (see the other message).\n\n> The second commit takes care of a NEEDSWORK in setup_git_directory_gently()\n> we now properly error out if we hit a .git that is not a file or directory\n> when looking for the .git.\n\nWe used to just ignore and keep going to check the parent directory,\nright?  Now we would error out when .git is a FIFO or device or any\nother random things.  Is a bit of behaviour change, but I am not\nsure if it is worth doing.  As finding these weird non-file things\nin your working tree and naming them \".git\" is extremely rare and\nuseless (from Git's point of view), I suspect that the user is\ndeliberately doing so for whatever reason they have, so it smells\nlike this change has very little chance to detect a real problem\nwith a larger chance to break a set-up that was deliberately done by\nthe end-user.  I dunno.\n\nThanks.\n\n\n"},{"id":"520276","messageId":"0AE631DC-F4C6-4896-BAE0-F0D35E4E642A@smail.nju.edu.cn","threadId":"63632","inReplyTo":"xmqqbjqo4gw7.fsf@gitster.g","subject":"Re: [RFC PATCH v3 0/2] small fixes for git.c and setup.c","fromName":"Lidong Yan","fromEmail":"502024330056@smail.nju.edu.cn","sentAt":"2025-06-16T01:36:34Z","receivedAt":"2025-06-16T01:37:11Z","isPatch":true,"sender":{"key":"502024330056@smail.nju.edu.cn","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes：\n> \n> Lidong Yan <yldhome2d2@gmail.com> writes:\n> \n>> I've been reading through the git code from the beginning. This\n>> patch series fixes some NEEDSWORKs and cleans up some unnecessary\n>> uses of the_repository that I came across.\n> \n> FYI, when we have \"NEEDSWORK: do X\", the intention is often \"we\n> haven't spent enough brain cycles when we wrote this comment, so the\n> first step is to evaluate if doing X is a sensible thing in the\n> first place, and only if that is the case, do X\".\n\nUnderstood. I initially thought that NEEDSWORK indicated work that was\nguaranteed to be implemented later.\n\n> \n>> The first commit replace the use of the_repository to run_builtin()'s\n>> argument repo. Since each caller pass the_repository to run_builtin(),\n>> this replacement is safe.\n> \n> That change is safe.  I'd rather see the comment left intact or\n> reverted to the original shape to clarify what code the comment\n> applies to (see the other message).\n\nI’d like to revert the comment to original shape and split it into separate commit.\nThe reason I changed this comment was simply because I found it a bit confusing\nwhen reading through the code.\n\n> \n>> The second commit takes care of a NEEDSWORK in setup_git_directory_gently()\n>> we now properly error out if we hit a .git that is not a file or directory\n>> when looking for the .git.\n> \n> We used to just ignore and keep going to check the parent directory,\n> right?  Now we would error out when .git is a FIFO or device or any\n> other random things.  Is a bit of behaviour change, but I am not\n> sure if it is worth doing.  As finding these weird non-file things\n> in your working tree and naming them \".git\" is extremely rare and\n> useless (from Git's point of view), I suspect that the user is\n> deliberately doing so for whatever reason they have, so it smells\n> like this change has very little chance to detect a real problem\n> with a larger chance to break a set-up that was deliberately done by\n> the end-user.  I dunno.\n> \n> Thanks.\n\nI would just discard the commit about NEEDSWORK.\n\nThanks for your review,\nLidong\n\n"},{"id":"520283","messageId":"191FDEFA-786C-4CD7-9D4F-06495FCBDDA6@smail.nju.edu.cn","threadId":"63632","inReplyTo":"xmqqsek04id9.fsf@gitster.g","subject":"Re: [PATCH v2] git.c: remove the_repository dependence in run_builtin()","fromName":"Lidong Yan","fromEmail":"502024330056@smail.nju.edu.cn","sentAt":"2025-06-16T05:36:41Z","receivedAt":"2025-06-16T05:37:16Z","isPatch":true,"sender":{"key":"502024330056@smail.nju.edu.cn","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes：\n> e5b17bda (git: ensure correct git directory setup with -h,\n> 2021-12-06) unfortunately moved lines around and made it look like\n> the comment is about what happens when the if() condition holds, but\n> if we look at the way how a9ca8a85 (builtins: print setup info if\n> repo is found, 2010-11-26) initially placed this comment, we can see\n> that this comment was to only explain the reason why we look at\n> startup_info->have_repository there.  \"Only if we know we have\n> repository, do the trace_repo_setup() thing because that one calls\n> get_git_dir() that would die otherwise\" is what the comment wants to\n> say, and if we revert the moving-line-around done by e5b17bda to\n> recover the original layout in a9ca8a85, I think it is clear enough.\n\nI’ve changed my mind. Layout like\n\n\tif (run_setup &&\n\t\tstartup_info->have_repository) /* get_git_dir() may set up repo, avoid that */\n\t\ttrace_repo_setup(repo);\n\nLooks unbalanced, and git-clang-format tries to format this code\nsnippet into\n\n\tif (run_setup && startup_info->have_repository) /* get_git_dir() may set\n\t\t\t\t\t\t\t   up repo, avoid that\n\t\t\t\t\t\t\t */\n\nWhich looks even worse, I will leave this comment intact.\n\nThanks,\nLidong\n\n"},{"id":"520284","messageId":"xmqqjz5c2qgf.fsf@gitster.g","threadId":"63632","inReplyTo":"191FDEFA-786C-4CD7-9D4F-06495FCBDDA6@smail.nju.edu.cn","subject":"Re: [PATCH v2] git.c: remove the_repository dependence in run_builtin()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-16T05:41:36Z","receivedAt":"2025-06-16T05:41:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lidong Yan <502024330056@smail.nju.edu.cn> writes:\n\n> Which looks even worse, I will leave this comment intact.\n\nThat is perfectly fine.  After all, this is not about comment style.\nIt is about the_repository.  Let's stay focused on that.\n"},{"id":"520285","messageId":"20250616062233.1589172-1-502024330056@smail.nju.edu.cn","threadId":"63632","inReplyTo":"20250615144604.1447302-1-502024330056@smail.nju.edu.cn","subject":"[PATCH v4] git.c: remove the_repository dependence in run_builtin()","fromName":"Lidong Yan","fromEmail":"yldhome2d2@gmail.com","sentAt":"2025-06-16T06:22:33Z","receivedAt":"2025-06-16T06:22:42Z","isPatch":true,"sender":{"key":"yldhome2d2@gmail.com","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"run_builtin() takes a repo parameter, so the use of the_repository\nis no longer necessary. Removed the usage of the_repository.\n\nSigned-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n---\n git.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex 77c4359522..8525ede550 100644\n--- a/git.c\n+++ b/git.c\n@@ -462,12 +462,12 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct\n \tprecompose_argv_prefix(argc, argv, NULL);\n \tif (use_pager == -1 && run_setup &&\n \t\t!(p->option & DELAY_PAGER_CONFIG))\n-\t\tuse_pager = check_pager_config(the_repository, p->cmd);\n+\t\tuse_pager = check_pager_config(repo, p->cmd);\n \tif (use_pager == -1 && p->option & USE_PAGER)\n \t\tuse_pager = 1;\n \tif (run_setup && startup_info->have_repository)\n \t\t/* get_git_dir() may set up repo, avoid that */\n-\t\ttrace_repo_setup(the_repository);\n+\t\ttrace_repo_setup(repo);\n \tcommit_pager_choice();\n \n \tif (!help && p->option & NEED_WORK_TREE)\n-- \n2.50.0-rc1\n\n"}]}