{"thread":{"id":"64979","subject":"[RFC] setup: fail if .git is not a file or directory","startedAt":"2026-02-11T18:21:37Z","lastAt":"2026-02-19T05:12:00Z","messageCount":35,"participants":["Tian Yuchen","Junio C Hamano","brian m. carlson","Karthik Nayak"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"535791","messageId":"20260211182122.35352-1-a3205153416@gmail.com","threadId":"64979","inReplyTo":null,"subject":"[RFC] setup: fail if .git is not a file or directory","fromName":"Tian Yuchen","fromEmail":"a3205153416@gmail.com","sentAt":"2026-02-11T18:21:22Z","receivedAt":"2026-02-11T18:21:37Z","isPatch":false,"sender":{"key":"cat@malon.dev","avatar":"https://avatars.githubusercontent.com/u/232002048?v=4"},"body":"Currently, `setup_git_directory_gently_1()` checks if `.git` is a\nregular file (handling submodules/worktrees) or a directory. If it is\nneither (e.g., a FIFO), the code hits a NEEDSWORK comment and simply\nignores the entity, continuing the discovery process in the parent\ndirectory.\n\nThis behavior can be very dangerous. If a user is inside a subdirectory\ncontaining a melformed/broken `.git` entity, the Git will traverse up,\nattach to a parent repository and might execute destructive commands.\n\nI tried to resolve the NEEDSWORK by using `lstat()` to explicitly check\nthe entity's mode. If it is neither a regular file nor a directory, we\nkill the discovery process.\n\nBut I still have questions:\n1. Is failing hard the desired behavior here? Should skipping it and\n continuing discovery be an option for the user, which might seem\n more fault-tolerant?\n2. Should we die() immediately here, or return GIT_DIR_INVALID_GITFILE\n and let the caller decide?\n\nSigned-off-by: Tian Yuchen \n---\n setup.c | 12 +++++++++++-\n 1 file changed, 11 insertions(+), 1 deletion(-)\n\ndiff --git a/setup.c b/setup.c\nindex 3a6a048620..a1b56de67a 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1581,7 +1581,17 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\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\tstruct stat st;\n+\t\t\t\tif (!lstat(dir->buf, &st) &&\n+\t\t\t\t\t!S_ISREG(st.st_mode) &&\n+\t\t\t\t\t!S_ISDIR(st.st_mode)){\n+\n+\t\t\t\t\tif (die_on_error)\n+\t\t\t\t\t\tdie(_(\"Invalid %s: not a regular file or directory\"), dir->buf);\n+\t\t\t\t\telse\n+\t\t\t\t\t\treturn GIT_DIR_INVALID_GITFILE;\n+\t\t\t\t}\n+\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);\n-- \n2.43.0\n\n"},{"id":"535794","messageId":"xmqq3437t643.fsf@gitster.g","threadId":"64979","inReplyTo":"20260211182122.35352-1-a3205153416@gmail.com","subject":"Re: [RFC] setup: fail if .git is not a file or directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-02-11T19:47:24Z","receivedAt":"2026-02-11T19:47:27Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tian Yuchen writes:\n\n> Currently, `setup_git_directory_gently_1()` checks if `.git` is a\n> regular file (handling submodules/worktrees) or a directory. If it is\n> neither (e.g., a FIFO), the code hits a NEEDSWORK comment and simply\n> ignores the entity, continuing the discovery process in the parent\n> directory.\n\nThanks for noticing the needswork comment.\n\n> This behavior can be very dangerous. If a user is inside a subdirectory\n> containing a melformed/broken `.git` entity, the Git will traverse up,\n> attach to a parent repository and might execute destructive commands.\n\nIs it? If a malicious party can replace your .git file in your\nsubmodule with a device node or a fifo, it means they can write into\nyour working tree of your top level superproject, so they have other\nand better things to use to attack you if they wanted to. I would\nsay 'very dangerous' is an overstatement. If \"git add\" in a place\nyou thought was in your submodule ends up adding the file to the\nsuperproject because of filesystem corruption making \".git\" in your\nsubmodule to something strange, you'd have larger problem anyway---\nwould you trust your commits and trees recorded in such a corrupt\nfilesystem?\n\nHaving said all that.\n\nFailing instead of ignoring may make it easier for the end-users to\nnotice anomalies and take corrective action, if this non-file\nnon-directory filesystem entity was created by mistake or by\nfile-system corruption. It would be an unnecessary breakage to\nthose who deliberately named a fifo they use \".git\", fully knowing\nwell that it is a reserved name that \"git add\" and friends happily\nignore, but I somehow find it unlikely.\n\n> But I still have questions:\n> 1. Is failing hard the desired behavior here? Should skipping it and\n> continuing discovery be an option for the user, which might seem\n> more fault-tolerant?\n> 2. Should we die() immediately here, or return GIT_DIR_INVALID_GITFILE\n> and let the caller decide?\n\nDetermining if it makes sense to do so is 80% of the work needed to\nresolve a NEEDSWORK comment. The actual implementation is often\nmuch simpler than the work needed for it. ;-)\n\n> Signed-off-by: Tian Yuchen \n> ---\n> setup.c | 12 +++++++++++-\n> 1 file changed, 11 insertions(+), 1 deletion(-)\n>\n> diff --git a/setup.c b/setup.c\n> index 3a6a048620..a1b56de67a 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -1581,7 +1581,17 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\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\tstruct stat st;\n> +\t\t\t\tif (!lstat(dir->buf, &st) &&\n> +\t\t\t\t\t!S_ISREG(st.st_mode) &&\n> +\t\t\t\t\t!S_ISDIR(st.st_mode)){\n> +\n> +\t\t\t\t\tif (die_on_error)\n> +\t\t\t\t\t\tdie(_(\"Invalid %s: not a regular file or directory\"), dir->buf);\n> +\t\t\t\t\telse\n> +\t\t\t\t\t\treturn GIT_DIR_INVALID_GITFILE;\n> +\t\t\t\t}\n\nI would have expected that the new code would come after the\nexisting \"if the thing is a .git directory, it is a happy outcome\"\ncode. In this code path, where we found \".git\" that is not a file,\nthe most likely reason is because we found a healthy git directory,\nso it is a sane thing to avoid extra lstat() in that normal\ncodepath. Only after we see that the \".git\" we have is neither a\n\"gitdir:\" file nor a git directory, we know we are dealing with a\nrare anomaly that we can afford to waste extra cycles.\n\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);\n"},{"id":"535876","messageId":"20260212172405.48614-1-a3205153416@gmail.com","threadId":"64979","inReplyTo":"20260211182122.35352-1-a3205153416@gmail.com","subject":"[PATCH v2] setup: fail if .git is not a file or directory","fromName":"Tian Yuchen","fromEmail":"a3205153416@gmail.com","sentAt":"2026-02-12T17:24:05Z","receivedAt":"2026-02-12T17:24:13Z","isPatch":true,"sender":{"key":"cat@malon.dev","avatar":"https://avatars.githubusercontent.com/u/232002048?v=4"},"body":"Currently, `setup_git_directory_gently_1()` checks if `.git` is a\nregular file (handling submodules/worktrees) or a directory. If it is\nneither (e.g., a FIFO), the code hits a NEEDSWORK comment and simply\nignores the entity, continuing the discovery process in the parent\ndirectory.\n\nFailing instead of ignoring here makes it easier for the users to notice\nanomalies and take action, particularly if this non-file non-directory\nentity was created by mistake or by file system corruption.\n\nResolve the NEEDSWORK by using `lstat()` to check the entity's mode. To\navoid performance penalties on the happy path, this check is only\nperformed after\t`is_git_directory()` fails.\n\nSigned-off-by: Tian Yuchen \n---\nChange in v2:\n\n- Moved the `lstat` check into an `else` block after `is_git_directory()`\nto avoid any extra system call overhead on the normal happy path, as\nsuggested by Junio.\n\n setup.c | 18 +++++++++++++++---\n 1 file changed, 15 insertions(+), 3 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 3a6a048620..95658f2547 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1580,11 +1580,23 @@ 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\tif (is_git_directory(dir->buf)) {\n+\t\t\t\terror_code == READ_GITFILE_ERR_NOT_A_FILE) {\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);\n+\t\t\t\t} else {\n+\t\t\t\t\t/*\n+\t\t\t\t\t * It is neither a gitfile nor a git directory.\n+\t\t\t\t\t */\n+\t\t\t\t\tstruct stat st;\n+\t\t\t\t\tif (!lstat(dir->buf, &st) &&\n+\t\t\t\t\t\t!S_ISREG(st.st_mode) &&\n+\t\t\t\t\t\t!S_ISDIR(st.st_mode)) {\n+\t\t\t\t\t\tif (die_on_error)\n+\t\t\t\t\t\t\tdie(_(\"Invalid %s: not a regular file or directory\"), dir->buf);\n+\t\t\t\t\t\telse\n+\t\t\t\t\t\t\treturn GIT_DIR_INVALID_GITFILE;\n+\t\t\t\t\t}\n \t\t\t\t}\n \t\t\t} else if (error_code != READ_GITFILE_ERR_STAT_FAILED)\n \t\t\t\treturn GIT_DIR_INVALID_GITFILE;\n-- \n2.43.0\n\n"},{"id":"535878","messageId":"950a54d3-2b00-4e1e-937d-d22bf5cbbf27@gmail.com","threadId":"64979","inReplyTo":"xmqq3437t643.fsf@gitster.g","subject":"Re: [RFC] setup: fail if .git is not a file or directory","fromName":"Tian Yuchen","fromEmail":"a3205153416@gmail.com","sentAt":"2026-02-12T17:33:46Z","receivedAt":"2026-02-12T17:33:50Z","isPatch":false,"sender":{"key":"cat@malon.dev","avatar":"https://avatars.githubusercontent.com/u/232002048?v=4"},"body":"Hi Junio,\n\nI've rewrote the code based on your suggestions. If there\nare further improvements to be made regarding coding style\nor readability(especially the style of comments), please\nlet me know.\n\nHappy valentine's day(though it's tomorrow)!\n\nYuchen\n"},{"id":"535885","messageId":"xmqqtsvln0ev.fsf@gitster.g","threadId":"64979","inReplyTo":"20260212172405.48614-1-a3205153416@gmail.com","subject":"Re: [PATCH v2] setup: fail if .git is not a file or directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-02-12T20:59:20Z","receivedAt":"2026-02-12T20:59:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tian Yuchen writes:\n\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\terror_code == READ_GITFILE_ERR_NOT_A_FILE) {\n\nWhy this indentation change?\n\n> -\t\t\t\t/* NEEDSWORK: fail if .git is not file nor dir */\n> -\t\t\t\tif (is_git_directory(dir->buf)) {\n> +\t\t\t\tif (is_git_directory(dir->buf)){\n\nWhy this change (which is style violation that lack necessary SP\nbetween \"){\")?\n\n> \t\t\t\t\tgitdirenv = DEFAULT_GIT_DIR_ENVIRONMENT;\n> \t\t\t\t\tgitdir_path = xstrdup(dir->buf);\n> +\t\t\t\t} else {\n> +\t\t\t\t\t/*\n> +\t\t\t\t\t * It is neither a gitfile nor a git directory.\n> +\t\t\t\t\t */\n> +\t\t\t\t\tstruct stat st;\n> +\t\t\t\t\tif (!lstat(dir->buf, &st) &&\n> +\t\t\t\t\t\t!S_ISREG(st.st_mode) &&\n> +\t\t\t\t\t\t!S_ISDIR(st.st_mode)) {\n> +\t\t\t\t\t\tif (die_on_error)\n> +\t\t\t\t\t\t\tdie(_(\"Invalid %s: not a regular file or directory\"), dir->buf);\n> +\t\t\t\t\t\telse\n> +\t\t\t\t\t\t\treturn GIT_DIR_INVALID_GITFILE;\n> +\t\t\t\t\t}\n> \t\t\t\t}\n\nStepping back a bit, even though the NEEDSWORK comment is placed\nhere, I am not sure if this is the bast place to do much of the\nnecessary work.\n\nWhen gitdirenv is NULL and die_on_error is set, we would have died\non any error other than STAT_FAILED (most often, this is because\nthere wasn't .git at the path in the first place) or NOT_A_FILE\n(again, a happy case is that .git existed and it was a directory).\nWe would have already died in read_gitfile_error_die() in all other\ncases. But these two error cases are not necessarily entirely happy\nand that is what the NEEDSWORK comment is about.\n\nSo, if we wanted to tighten the error checking to help users\ndiagnose problems in their filesystem, I wonder if it is a better\napproach to refine the set of READ_GITFILE_ERR_* error codes:\n\n - STAT_ENOENT (new) is returned when stat failed and we got ENOENT,\n and it is not a fatal error.\n\n - STAT_FAILED becomes a fatal error in read_gitfile_error_die().\n\n - IS_A_DIR (new) is returned when stat succeeded and it is a\n directory. It is not a fatal error.\n\n - NOT_A_FILE becomes a fatal error in read_gitfile_error_die().\n\nExisting callers of the two functions, read_gitfile_gently() and\nread_gitfile_error_die(), must be audited and adjusted\nappropriately, but once it is done, it would become much simpler,\nwouldn't it?\n\nThanks.\n"},{"id":"535898","messageId":"aY5Wid6eg1-LwZm8@fruit.crustytoothpaste.net","threadId":"64979","inReplyTo":"20260211182122.35352-1-a3205153416@gmail.com","subject":"Re: [RFC] setup: fail if .git is not a file or directory","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2026-02-12T22:39:05Z","receivedAt":"2026-02-12T22:39:13Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2026-02-11 at 18:21:22, Tian Yuchen wrote:\n> Currently, `setup_git_directory_gently_1()` checks if `.git` is a\n> regular file (handling submodules/worktrees) or a directory. If it is\n> neither (e.g., a FIFO), the code hits a NEEDSWORK comment and simply\n> ignores the entity, continuing the discovery process in the parent\n> directory.\n> \n> This behavior can be very dangerous. If a user is inside a subdirectory\n> containing a melformed/broken `.git` entity, the Git will traverse up,\n> attach to a parent repository and might execute destructive commands.\n> \n> I tried to resolve the NEEDSWORK by using `lstat()` to explicitly check\n> the entity's mode. If it is neither a regular file nor a directory, we\n> kill the discovery process.\n\nWe used to allow symlinks as well. That was used instead of gitfiles\nfor submodules at one point, I believe, and there may still be some\npeople using that. A brief test indicates that that functionality still\nworks, so if we make a change here, we should be sure to accept symlinks\nas well.\n\nIn general, we should allow people to use symlinks wherever they can use\na file or directory unless we can definitively prove that there's a\nclear security or functionality problem that cannot be avoided. Git was\noriginally written for Unix, after all.\n-- \nbrian m. carlson (they/them)\nToronto, Ontario, CA\n"},{"id":"535899","messageId":"xmqqy0kxlgy6.fsf@gitster.g","threadId":"64979","inReplyTo":"aY5Wid6eg1-LwZm8@fruit.crustytoothpaste.net","subject":"Re: [RFC] setup: fail if .git is not a file or directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-02-12T22:45:05Z","receivedAt":"2026-02-12T22:45:08Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n> We used to allow symlinks as well. That was used instead of gitfiles\n> for submodules at one point, I believe, and there may still be some\n> people using that. A brief test indicates that that functionality still\n> works, so if we make a change here, we should be sure to accept symlinks\n> as well.\n\nIn my review, I outlined a way to avoid this extra lstat(), and\ninstead reuse the result of stat() used in read_gitfile_gently()\nalready; the check in that function being stat() is exactly because\nwe want to follow such a symbolic link.\n\n> In general, we should allow people to use symlinks wherever they can use\n> a file or directory unless we can definitively prove that there's a\n> clear security or functionality problem that cannot be avoided. Git was\n> originally written for Unix, after all.\n\nIs this also an obvlique reference to a separate potential security\nissue, I wonder. It reminds me that I need to see if I have to ping\nthe thread again.\n\nThanks.\n"},{"id":"535900","messageId":"aY5cNkxjzOOCGOow@fruit.crustytoothpaste.net","threadId":"64979","inReplyTo":"xmqqy0kxlgy6.fsf@gitster.g","subject":"Re: [RFC] setup: fail if .git is not a file or directory","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2026-02-12T23:03:18Z","receivedAt":"2026-02-12T23:03:20Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2026-02-12 at 22:45:05, Junio C Hamano wrote:\n> \"brian m. carlson\" writes:\n> \n> > In general, we should allow people to use symlinks wherever they can use\n> > a file or directory unless we can definitively prove that there's a\n> > clear security or functionality problem that cannot be avoided. Git was\n> > originally written for Unix, after all.\n> \n> Is this also an obvlique reference to a separate potential security\n> issue, I wonder. It reminds me that I need to see if I have to ping\n> the thread again.\n\nIt was intended to be a reference to the situation we had with, I\nbelieve, `.gitmodules` or `.gitignore` or another file of that sort,\nwhere symlinks were very much broken in that context. I merely\nmentioned security issues for completeness.\n\nI believe it was bb6832d552 (\"fsck: warn about symlinked dotfiles we'll\nopen with O_NOFOLLOW\", 2021-05-03) that introduced that change and was\nwhat I was thinking of.\n-- \nbrian m. carlson (they/them)\nToronto, Ontario, CA\n"},{"id":"535940","messageId":"0892ee4c-c274-4ed3-9b4e-6ea1e910407a@gmail.com","threadId":"64979","inReplyTo":"xmqqtsvln0ev.fsf@gitster.g","subject":"Re: [PATCH v2] setup: fail if .git is not a file or directory","fromName":"Tian Yuchen","fromEmail":"a3205153416@gmail.com","sentAt":"2026-02-13T16:37:38Z","receivedAt":"2026-02-13T16:37:43Z","isPatch":true,"sender":{"key":"cat@malon.dev","avatar":"https://avatars.githubusercontent.com/u/232002048?v=4"},"body":"On 2/13/26 04:59, Junio C Hamano wrote:\n\n> Why this indentation change?\n> \n>> -\t\t\t\t/* NEEDSWORK: fail if .git is not file nor dir */\n>> -\t\t\t\tif (is_git_directory(dir->buf)) {\n>> +\t\t\t\tif (is_git_directory(dir->buf)){\n> \n> Why this change (which is style violation that lack necessary SP\n> between \"){\")?\n\nSorry for the typo. I will fix it in the next patch :(\n\n> Stepping back a bit, even though the NEEDSWORK comment is placed\n> here, I am not sure if this is the bast place to do much of the\n> necessary work.\n> \n> When gitdirenv is NULL and die_on_error is set, we would have died\n> on any error other than STAT_FAILED (most often, this is because\n> there wasn't .git at the path in the first place) or NOT_A_FILE\n> (again, a happy case is that .git existed and it was a directory).\n> We would have already died in read_gitfile_error_die() in all other\n> cases. But these two error cases are not necessarily entirely happy\n> and that is what the NEEDSWORK comment is about.\n> \n> So, if we wanted to tighten the error checking to help users\n> diagnose problems in their filesystem, I wonder if it is a better\n> approach to refine the set of READ_GITFILE_ERR_* error codes:\n> \n> - STAT_ENOENT (new) is returned when stat failed and we got ENOENT,\n> and it is not a fatal error.\n> \n> - STAT_FAILED becomes a fatal error in read_gitfile_error_die().\n> \n> - IS_A_DIR (new) is returned when stat succeeded and it is a\n> directory. It is not a fatal error.\n> \n> - NOT_A_FILE becomes a fatal error in read_gitfile_error_die().\n\nThis approach makes sense to me. I will work on v3 patch that:\n\n1. Refines 'read_gitfile_error' with 'STAT_ENOENT' and 'IS_A_DIR'.\n\n2. Makes 'STAT_FAILED' and 'NOT_A_FILE' fatal by default in \n'read_gitfile_error_die()'\n\n3. Adjusts existing callers to handle these new codes.\n\nThis will be a larger refactor, so it might take a bit of time to ensure \nI don't break other call sites.\n\n> Existing callers of the two functions, read_gitfile_gently() and\n> read_gitfile_error_die(), must be audited and adjusted\n> appropriately, but once it is done, it would become much simpler,\n> wouldn't it?\nYes indeed.\n\nThanks for guiding me toward a cleaner architecture.\n\n\nBuon san valentino,\n\nYuchen\n"},{"id":"535999","messageId":"20260214045247.118013-1-a3205153416@gmail.com","threadId":"64979","inReplyTo":"20260212172405.48614-1-a3205153416@gmail.com","subject":"[PATCH v3] setup: fail if .git is not a file or directory","fromName":"Tian Yuchen","fromEmail":"a3205153416@gmail.com","sentAt":"2026-02-14T04:52:47Z","receivedAt":"2026-02-14T04:53:03Z","isPatch":true,"sender":{"key":"cat@malon.dev","avatar":"https://avatars.githubusercontent.com/u/232002048?v=4"},"body":"Currently, `setup_git_directory_gently_1()` checks if `.git` is a\nregular file (handling submodules/worktrees) or a directory. If it is\nneither (e.g., a FIFO), the code hits a NEEDSWORK comment and simply\nignores the entity, continuing the discovery process in the parent\ndirectory.\n\nFailing instead of ignoring here makes it easier for the users to notice\nanomalies and take action, particularly if this non-file non-directory\nentity was created by mistake or by file system corruption.\n\nHowever, strictly enforcing 'lstat()' and 'S_ISREG()' breaks valid\nworkflows where '.git' is a symlink pointing to a real git directory\n(e.g. created via 'ln -s'). To ensure safety and correctness:\n\n1. Differentiate between \"missing file\" and \"is a directory\". This\n removes the long standing NEEDSWORK comment in 'read_gitfile_gently()'.\n\n2. Explicitly check 'st_mode' after 'stat()'. If the path resolves to a\n directory, return 'READ_GITFILE_ERR_IS_A_DIR' so the caller can try\n to handle it as a directory.\n\n3. If the path exists but is neither a regular file nor a directory,\n return 'READ_GITFILE_ERR_NOT_A_FILE'.\n\nUpdate 'setup_git_directory_gently_1()' to aborts setup immeditaely upon\nencountering a malicous '.git' file.\n\nSigned-off-by: Tian Yuchen \n---\n\nI have verified this with a test script covering:\n1. Normal .git file\n2. .git as a symlink to a directory\n3. .git as a FIFO\n4. .git as a symlink to a FIFO\n5. .git with garbage content\n\n setup.c | 39 +++++++++++++++++++++++++++++----------\n setup.h | 3 +++\n 2 files changed, 32 insertions(+), 10 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 3a6a048620..8681a8a9d1 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -911,6 +911,10 @@ void read_gitfile_error_die(int error_code, const char *path, const char *dir)\n \t\tdie(_(\"no path in gitfile: %s\"), path);\n \tcase READ_GITFILE_ERR_NOT_A_REPO:\n \t\tdie(_(\"not a git repository: %s\"), dir);\n+\tcase READ_GITFILE_ERR_STAT_ENOENT:\n+\t\tdie(_(\"Not a git repository: %s\"), path);\n+\tcase READ_GITFILE_ERR_IS_A_DIR:\n+\t\tdie(_(\"Not a git file (is a directory): %s\"), path);\n \tdefault:\n \t\tBUG(\"unknown error code\");\n \t}\n@@ -939,8 +943,14 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)\n \tstatic struct strbuf realpath = STRBUF_INIT;\n \n \tif (stat(path, &st)) {\n-\t\t/* NEEDSWORK: discern between ENOENT vs other errors */\n-\t\terror_code = READ_GITFILE_ERR_STAT_FAILED;\n+\t\tif (errno == ENOENT)\n+\t\t\terror_code = READ_GITFILE_ERR_STAT_ENOENT;\n+\t\telse\n+\t\t\terror_code = READ_GITFILE_ERR_STAT_FAILED;\n+\t\tgoto cleanup_return;\n+\t}\n+\tif (S_ISDIR(st.st_mode)) {\n+\t\terror_code = READ_GITFILE_ERR_IS_A_DIR;\n \t\tgoto cleanup_return;\n \t}\n \tif (!S_ISREG(st.st_mode)) {\n@@ -994,7 +1004,9 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)\n cleanup_return:\n \tif (return_error_code)\n \t\t*return_error_code = error_code;\n-\telse if (error_code)\n+\telse if (error_code &&\n+\t\terror_code != READ_GITFILE_ERR_STAT_ENOENT &&\n+\t\terror_code != READ_GITFILE_ERR_IS_A_DIR)\n \t\tread_gitfile_error_die(error_code, path, dir);\n \n \tfree(buf);\n@@ -1576,18 +1588,25 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n \t\tif (offset > min_offset)\n \t\t\tstrbuf_addch(dir, '/');\n \t\tstrbuf_addstr(dir, DEFAULT_GIT_DIR_ENVIRONMENT);\n-\t\tgitdirenv = read_gitfile_gently(dir->buf, die_on_error ?\n-\t\t\t\t\t\tNULL : &error_code);\n+\t\tgitdirenv = read_gitfile_gently(dir->buf, &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\tif (error_code == READ_GITFILE_ERR_STAT_ENOENT ||\n+\t\t\t\terror_code == READ_GITFILE_ERR_IS_A_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);\n \t\t\t\t}\n-\t\t\t} else if (error_code != READ_GITFILE_ERR_STAT_FAILED)\n-\t\t\t\treturn GIT_DIR_INVALID_GITFILE;\n+\t\t\t} else if (error_code == READ_GITFILE_ERR_NOT_A_FILE) {\n+\t\t\t\tif (die_on_error)\n+\t\t\t\t\tdie(_(\"Invalid %s: not a regular file or directory\"), dir->buf);\n+\t\t\t\telse\n+\t\t\t\t\treturn GIT_DIR_INVALID_GITFILE;\n+\t\t\t} else if (error_code != READ_GITFILE_ERR_STAT_FAILED) {\n+\t\t\t\tif (die_on_error)\n+\t\t\t\t\tread_gitfile_error_die(error_code, dir->buf, NULL);\n+\t\t\t\telse\n+\t\t\t\t\treturn GIT_DIR_INVALID_GITFILE;\n+\t\t\t}\n \t\t} else\n \t\t\tgitfile = xstrdup(dir->buf);\n \t\t/*\ndiff --git a/setup.h b/setup.h\nindex d55dcc6608..0271cc8f93 100644\n--- a/setup.h\n+++ b/setup.h\n@@ -36,6 +36,9 @@ int is_nonbare_repository_dir(struct strbuf *path);\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_STAT_ENOENT 9\n+#define READ_GITFILE_ERR_IS_A_DIR 10\n+\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)\n-- \n2.43.0\n\n"},{"id":"536042","messageId":"xmqqfr72flga.fsf@gitster.g","threadId":"64979","inReplyTo":"20260214045247.118013-1-a3205153416@gmail.com","subject":"Re: [PATCH v3] setup: fail if .git is not a file or directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-02-15T08:41:09Z","receivedAt":"2026-02-15T08:41:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tian Yuchen writes:\n\n> Currently, `setup_git_directory_gently_1()` checks if `.git` is a\n> regular file (handling submodules/worktrees) or a directory. If it is\n> neither (e.g., a FIFO), the code hits a NEEDSWORK comment and simply\n> ignores the entity, continuing the discovery process in the parent\n> directory.\n>\n> Failing instead of ignoring here makes it easier for the users to notice\n> anomalies and take action, particularly if this non-file non-directory\n> entity was created by mistake or by file system corruption.\n\nOK.\n\n> However, strictly enforcing 'lstat()' and 'S_ISREG()' breaks valid\n> workflows where '.git' is a symlink pointing to a real git directory\n> (e.g. created via 'ln -s').\n\nThis is true, but it is more or less irrelevant, isn't it?\n\nThe existing check to see .git is a regular file or a directory\nalready uses stat() and has no such problem. A mistaken use of\nlstat() will only become relevant if we were to add an extra lstat()\ncall on top of what we already have, but that is not what we are\ndoing here, right?\n\n> To ensure safety and correctness:\n>\n> 1. Differentiate between \"missing file\" and \"is a directory\". This\n> removes the long standing NEEDSWORK comment in 'read_gitfile_gently()'.\n\nThis is more about telling \"not a file\" and \"is a directory\" apart,\nisn't it? Missing file (STAT_FAILED) is distinct from NOT_A_FILE\nand we check if it is a git directory only when we get the latter,\nand ignore if the thing that is not a file is not a git directory.\n\n> 2. Explicitly check 'st_mode' after 'stat()'. If the path resolves to a\n> directory, return 'READ_GITFILE_ERR_IS_A_DIR' so the caller can try\n> to handle it as a directory.\n\nThis lets us tell \"not a file\" and \"is a directory\" apart, which is\nduplicate of the above #1, isn't it?\n\n> 3. If the path exists but is neither a regular file nor a directory,\n> return 'READ_GITFILE_ERR_NOT_A_FILE'.\n\nAgain, the same thing. I _think_ what we discussed as the\nmotivation behind introducing new error classes are:\n\n * We are assuming \"not a file\" is more or less synonymous to \"is a\n directory\", and ignoring a path that is neither a file nor a git\n directory, which is what the NEEDSWORK comment in\n setup_git_directory_gently_1() is about.\n\n * We are assuing failure from stat(2) is more or less synonymous to\n \"nothing there\", and blindly go uplevel, which is what the\n NEEDSWORK comment in read_gitfile_error_die() is about.\n\nSo, we introduce IS_A_DIR (which is *not* an error) and together\nwith NOT_A_FILE (which was not an error, but now is an error), we\ncan tell \"whoa, there is .git that is not a file or a directory\"\nreliably, to solve the first NEEDSWORK. We also introduce\nSTAT_ENOENT (which is *not* an error) and together with STAT_FAILED,\nwe can tell \"ok, there is nothing there, so we go up one level and\ntry to see if there is .git there\" (i.e., happy normal case) and\n\"whoa, there is .git we cannot tell what it is\" case apart, which is\nabout the second NEEDSWORK.\n\n> I have verified this with a test script covering:\n> 1. Normal .git file\n> 2. .git as a symlink to a directory\n> 3. .git as a FIFO\n> 4. .git as a symlink to a FIFO\n> 5. .git with garbage content\n>\n> setup.c | 39 +++++++++++++++++++++++++++++----------\n> setup.h | 3 +++\n> 2 files changed, 32 insertions(+), 10 deletions(-)\n\nThere is no test addition here, though? What am I missing?\n\n> diff --git a/setup.c b/setup.c\n> index 3a6a048620..8681a8a9d1 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -911,6 +911,10 @@ void read_gitfile_error_die(int error_code, const char *path, const char *dir)\n> \t\tdie(_(\"no path in gitfile: %s\"), path);\n> \tcase READ_GITFILE_ERR_NOT_A_REPO:\n> \t\tdie(_(\"not a git repository: %s\"), dir);\n> +\tcase READ_GITFILE_ERR_STAT_ENOENT:\n> +\t\tdie(_(\"Not a git repository: %s\"), path);\n> +\tcase READ_GITFILE_ERR_IS_A_DIR:\n> +\t\tdie(_(\"Not a git file (is a directory): %s\"), path);\n\nHmph, isn't this backwards?\n\nWe used to treat STAT_FAILED as OK without dying in this function,\nbecause we conflated \"there is nothing there, so you should go one\nlevel up and try again\" happy case with all other stat(2) failure,\nand that is why we introduced STAT_ENOENT here. ENOENT is the\n*only* case among what used to be STAT_FAILED that we do *not* want\nto die in this function. The same thing with NOT_A_FILE vs\nIS_A_DIR. We used to treat the former as OK but the only case we\nwanted to treat as OK was IS_A_DIR and all other cases, like FIFO,\nwe wanted to complain, no?\n\nThat is why the caller in read_gitfile_gently() before this patch\nused to call this function regardless of what the error_code is.\nBecause the function above was updated to die in these two happy\ncases, the caller (below, hunk at around line 1000) excludes these\ntwo happy error codes, which it should not have to do.\n\n> @@ -939,8 +943,14 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)\n> \tstatic struct strbuf realpath = STRBUF_INIT;\n> \n> \tif (stat(path, &st)) {\n> -\t\t/* NEEDSWORK: discern between ENOENT vs other errors */\n> -\t\terror_code = READ_GITFILE_ERR_STAT_FAILED;\n> +\t\tif (errno == ENOENT)\n> +\t\t\terror_code = READ_GITFILE_ERR_STAT_ENOENT;\n> +\t\telse\n> +\t\t\terror_code = READ_GITFILE_ERR_STAT_FAILED;\n> +\t\tgoto cleanup_return;\n\nOK. We used to bundle any stat failure into STAT_FAILED, and\ntreated it as a single happy case, but by extracting STAT_ENOENT out\nof various reasons stat(2) failed, we can single out ENOENT as the\nonly happy case and treat all other stat(2) failure as errors.\n\n> +\t}\n> +\tif (S_ISDIR(st.st_mode)) {\n> +\t\terror_code = READ_GITFILE_ERR_IS_A_DIR;\n> \t\tgoto cleanup_return;\n> \t}\n> \tif (!S_ISREG(st.st_mode)) {\n\nAnd we used to bundle anything other than S_ISREG() as a single\nNOT_A_FILE class, and assumed that would be a directory and treated\nit as a non-error. We now explicitly return IS_A_DIR so that the\ncaller can treat that as the only happy case among other NOT_A_FILE\ncases that are suspicious.\n\n> @@ -994,7 +1004,9 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)\n> cleanup_return:\n> \tif (return_error_code)\n> \t\t*return_error_code = error_code;\n> -\telse if (error_code)\n> +\telse if (error_code &&\n> +\t\terror_code != READ_GITFILE_ERR_STAT_ENOENT &&\n> +\t\terror_code != READ_GITFILE_ERR_IS_A_DIR)\n> \t\tread_gitfile_error_die(error_code, path, dir);\n\nThis looks seriously wrong, and it is because it is trying to paper\nover the misdesign of the updates made to read_gitfile_error_die(),\nisn't it?\n\n> @@ -1576,18 +1588,25 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n> \t\tif (offset > min_offset)\n> \t\t\tstrbuf_addch(dir, '/');\n> \t\tstrbuf_addstr(dir, DEFAULT_GIT_DIR_ENVIRONMENT);\n> -\t\tgitdirenv = read_gitfile_gently(dir->buf, die_on_error ?\n> -\t\t\t\t\t\tNULL : &error_code);\n> +\t\tgitdirenv = read_gitfile_gently(dir->buf, &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\n> +\t\t\tif (error_code == READ_GITFILE_ERR_STAT_ENOENT ||\n> +\t\t\t\terror_code == READ_GITFILE_ERR_IS_A_DIR) {\n\nThis smells seriously wrong.\n\nIS_A_DIR leading to a call to is_git_directory() check makes sense,\nbut if we got STAT_ENOENT, what would we expect to find there by\nmaking an is_git_directory() call?\n\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);\n> \t\t\t\t}\n> +\t\t\t} else if (error_code == READ_GITFILE_ERR_NOT_A_FILE) {\n> +\t\t\t\tif (die_on_error)\n> +\t\t\t\t\tdie(_(\"Invalid %s: not a regular file or directory\"), dir->buf);\n> +\t\t\t\telse\n> +\t\t\t\t\treturn GIT_DIR_INVALID_GITFILE;\n> +\t\t\t} else if (error_code != READ_GITFILE_ERR_STAT_FAILED) {\n\nDo you mean \"!=\" here, not \"==\"???\n\n> +\t\t\t\tif (die_on_error)\n> +\t\t\t\t\tread_gitfile_error_die(error_code, dir->buf, NULL);\n> +\t\t\t\telse\n> +\t\t\t\t\treturn GIT_DIR_INVALID_GITFILE;\n> +\t\t\t}\n\nIf we wanted to have an explicit STAT_NOENT check, it would make\nsense to add it as the last \"else if\" arm here, with an empty body,\ni.e.\n\n\t\t\t} else if (error_code == READ_GITFILE_STAT_ENOENT) {\n\t\t\t\t; /* nothing there, go up one level */\n\t\t\t}\n\n"},{"id":"536055","messageId":"f7426def-dce4-41d4-81de-91388fb41997@gmail.com","threadId":"64979","inReplyTo":"xmqqfr72flga.fsf@gitster.g","subject":"Re: [PATCH v3] setup: fail if .git is not a file or directory","fromName":"Tian Yuchen","fromEmail":"a3205153416@gmail.com","sentAt":"2026-02-15T16:22:02Z","receivedAt":"2026-02-15T16:22:06Z","isPatch":true,"sender":{"key":"cat@malon.dev","avatar":"https://avatars.githubusercontent.com/u/232002048?v=4"},"body":"Hi Junio,\n\nI admit the code logic is indeed full of flaws - the vast majority of \nyour suggestions make more sense than the current code. I do lack \nexperience handling this kind of call chain, but I'll keep learning #^#\n\nGiven the extensive changes required, I won't address each point you \nraised individually. Instead, I'll rewrite the code directly based on \nyour proposed solution. Please reserve your review for the next patch.\n\n >> I have verified this with a test script covering:\n >> 1. Normal .git file\n >> 2. .git as a symlink to a directory\n >> 3. .git as a FIFO\n >> 4. .git as a symlink to a FIFO\n >> 5. .git with garbage content\n >>\n >> setup.c | 39 +++++++++++++++++++++++++++++----------\n >> setup.h | 3 +++\n >> 2 files changed, 32 insertions(+), 10 deletions(-)\n\n >There is no test addition here, though? What am I missing?\n\nSorry, I didn't express myself clearly. I meant I tested it myself but \nnever add a test script. Test script will be included in the next patch.\n\n\nOn the other hand, if I understand correctly, state flows should be \ncategorized as follows:\n\n 1. Nothing there (ENOENT) ---> ignore and go up one level\n 2. Directory (IS_A_DIR) ---> check is_git_directory\n 3. NOT_A_FILE ---> die\n 4. *REAL* error (READ_FAILED, INVALID_FORMAT) ---> die\n\nAnd I mixed 1 and 2 and covered 4 in an obscure way (!= STAT_FAILED). I \ndon't think this code is \"unrunable\" but indeed the logic flow is \nGARBAGE. I'll fix it.\n\nThank you for your advice,\n\nRegards,\n\nYuchen\n\n"},{"id":"536056","messageId":"eae3b31b-802a-4fbf-9c84-47d410081de9@gmail.com","threadId":"64979","inReplyTo":"xmqqfr72flga.fsf@gitster.g","subject":"Re: [PATCH v3] setup: fail if .git is not a file or directory","fromName":"Tian Yuchen","fromEmail":"a3205153416@gmail.com","sentAt":"2026-02-15T17:08:20Z","receivedAt":"2026-02-15T17:08:24Z","isPatch":true,"sender":{"key":"cat@malon.dev","avatar":"https://avatars.githubusercontent.com/u/232002048?v=4"},"body":"diff --git a/setup.c b/setup.c\nindex 3a6a048620..269aa9faaa 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -939,8 +939,14 @@ const char *read_gitfile_gently(const char *path, \nint *return_error_code)\n \tstatic struct strbuf realpath = STRBUF_INIT;\n\n \tif (stat(path, &st)) {\n-\t\t/* NEEDSWORK: discern between ENOENT vs other errors */\n-\t\terror_code = READ_GITFILE_ERR_STAT_FAILED;\n+\t\tif (errno == ENOENT)\n+\t\t\terror_code = READ_GITFILE_ERR_STAT_ENOENT;\n+\t\telse\n+\t\t\terror_code = READ_GITFILE_ERR_STAT_FAILED;\n+\t\tgoto cleanup_return;\n+\t}\n+\tif (S_ISDIR(st.st_mode)) {\n+\t\terror_code = READ_GITFILE_ERR_IS_A_DIR;\n \t\tgoto cleanup_return;\n \t}\n \tif (!S_ISREG(st.st_mode)) {\n@@ -994,7 +1000,9 @@ const char *read_gitfile_gently(const char *path, \nint *return_error_code)\n cleanup_return:\n \tif (return_error_code)\n \t\t*return_error_code = error_code;\n-\telse if (error_code)\n+\telse if (error_code &&\n+\t\terror_code != READ_GITFILE_ERR_STAT_ENOENT &&\n+\t\terror_code != READ_GITFILE_ERR_IS_A_DIR)\n \t\tread_gitfile_error_die(error_code, path, dir);\n\n \tfree(buf);\n@@ -1576,20 +1584,27 @@ static enum discovery_result \nsetup_git_directory_gently_1(struct strbuf *dir,\n \t\tif (offset > min_offset)\n \t\t\tstrbuf_addch(dir, '/');\n \t\tstrbuf_addstr(dir, DEFAULT_GIT_DIR_ENVIRONMENT);\n-\t\tgitdirenv = read_gitfile_gently(dir->buf, die_on_error ?\n-\t\t\t\t\t\tNULL : &error_code);\n+\t\tgitdirenv = read_gitfile_gently(dir->buf, &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\tif (error_code == READ_GITFILE_ERR_STAT_ENOENT) {\n+\t\t\t\t;\n+\t\t\t} else if (error_code == READ_GITFILE_ERR_IS_A_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);\n \t\t\t\t}\n-\t\t\t} else if (error_code != READ_GITFILE_ERR_STAT_FAILED)\n-\t\t\t\treturn GIT_DIR_INVALID_GITFILE;\n-\t\t} else\n-\t\t\tgitfile = xstrdup(dir->buf);\n+\t\t\t} else if (error_code == READ_GITFILE_ERR_NOT_A_FILE) {\n+\t\t\t\tif (die_on_error)\n+\t\t\t\t\tdie(_(\"Invalid %s: not a regular file or directory\"), dir->buf);\n+\t\t\t\telse\n+\t\t\t\t\treturn GIT_DIR_INVALID_GITFILE;\n+\t\t\t} else if (error_code != READ_GITFILE_ERR_STAT_FAILED) {\n+\t\t\t\tif (die_on_error)\n+\t\t\t\t\tread_gitfile_error_die(error_code, dir->buf, NULL);\n+\t\t\t\telse\n+\t\t\t\t\treturn GIT_DIR_INVALID_GITFILE;\n+\t\t\t}\n+\t\t}\n \t\t/*\n \t\t * Earlier, we tentatively added DEFAULT_GIT_DIR_ENVIRONMENT\n \t\t * to check that directory for a repository.\ndiff --git a/setup.h b/setup.h\nindex d55dcc6608..0271cc8f93 100644\n--- a/setup.h\n+++ b/setup.h\n@@ -36,6 +36,9 @@ int is_nonbare_repository_dir(struct strbuf *path);\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_STAT_ENOENT 9\n+#define READ_GITFILE_ERR_IS_A_DIR 10\n+\n\n\nStill working on it.\n\nLooks clearer, right? I just wanna confirm whether I'm on the right \ntrack. Notice that I didn't change the following part: (though you \nmentioned earlier)\n\n > cleanup_return:\n > \tif (return_error_code)\n > \t\t*return_error_code = error_code;\n >-\telse if (error_code)\n >+\telse if (error_code &&\n >+\t\terror_code != READ_GITFILE_ERR_STAT_ENOENT &&\n >+\t\terror_code != READ_GITFILE_ERR_IS_A_DIR)\n > \t\tread_gitfile_error_die(error_code, path, dir);\n\nMy understanding is that we haven't distinguished between “unexpected” \n(ENOENT, IS_A_DIR) and “errors” (NOT_A_FILE, format error) in the code \nusing a categorical approach. Instead, we differentiate them solely \nbased on whether they are listed in `read_gitfile_error_die()` (if I \nhaven't missed anything). Consequently, retaining this code here seems \nto be the easiest and most readable approach to ensure that it doesn't \nreturn prematurely. However, I believe this check shouldn't reside here. \nThat's why I'm replying to you again.\n\n(By the way, I've always been a bit curious about how your name is \npronounced :/)\n\nRegards,\n\nYuchen\n\n\n\n\n\n\n\n"},{"id":"536080","messageId":"871pil76sd.fsf@gitster.g","threadId":"64979","inReplyTo":"f7426def-dce4-41d4-81de-91388fb41997@gmail.com","subject":"Re: [PATCH v3] setup: fail if .git is not a file or directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-02-16T02:37:22Z","receivedAt":"2026-02-16T02:37:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tian Yuchen writes:\n\n> Sorry, I didn't express myself clearly. I meant I tested it myself but \n> never add a test script. Test script will be included in the next patch.\n\nI see. Thanks.\n\n> On the other hand, if I understand correctly, state flows should be \n> categorized as follows:\n>\n> 1. Nothing there (ENOENT) ---> ignore and go up one level\n> 2. Directory (IS_A_DIR) ---> check is_git_directory\n> 3. NOT_A_FILE ---> die\n> 4. *REAL* error (READ_FAILED, INVALID_FORMAT) ---> die\n>\n> And I mixed 1 and 2 and covered 4 in an obscure way (!= STAT_FAILED). I \n> don't think this code is \"unrunable\" but indeed the logic flow is \n> GARBAGE. I'll fix it.\n\nThe above 4-bullet list makes sense to me. It makes me wonder what\nthe current code does and more importantly what we want to do when\nwe find a directory and is_git_directory() says that it is *not* a\nvalid one.\n\nThanks.\n\n"},{"id":"536118","messageId":"5b29218a-8d18-41f0-8a03-eac707151945@gmail.com","threadId":"64979","inReplyTo":"871pil76sd.fsf@gitster.g","subject":"Re: [PATCH v3] setup: fail if .git is not a file or directory","fromName":"Tian Yuchen","fromEmail":"a3205153416@gmail.com","sentAt":"2026-02-16T16:02:22Z","receivedAt":"2026-02-16T16:02:25Z","isPatch":true,"sender":{"key":"cat@malon.dev","avatar":"https://avatars.githubusercontent.com/u/232002048?v=4"},"body":"On 2/16/26 10:37, Junio C Hamano wrote:\n> Tian Yuchen writes:\n\n> \n>> Sorry, I didn't express myself clearly. I meant I tested it myself but\n>> never add a test script. Test script will be included in the next patch.\n> \n> I see. Thanks.\n>\n\nI am currently working on it. However, this is my first time writing \ntest scripts, so I can' guarantee they'll be perfect right away. Please \nfeel free to leave your suggestions when the time comes.\n\n>> On the other hand, if I understand correctly, state flows should be\n>> categorized as follows:\n>>\n>> 1. Nothing there (ENOENT) ---> ignore and go up one level\n>> 2. Directory (IS_A_DIR) ---> check is_git_directory\n>> 3. NOT_A_FILE ---> die\n>> 4. *REAL* error (READ_FAILED, INVALID_FORMAT) ---> die\n>>\n>> And I mixed 1 and 2 and covered 4 in an obscure way (!= STAT_FAILED). I\n>> don't think this code is \"unrunable\" but indeed the logic flow is\n>> GARBAGE. I'll fix it.\n> \n> The above 4-bullet list makes sense to me. It makes me wonder what\n> the current code does and more importantly what we want to do when\n> we find a directory and is_git_directory() says that it is *not* a\n> valid one.\n> \n> Thanks.\n> \n\nIf we find a directory but 'is_git_directory()' reports it as invalid, I \nbelieve we should treat it the same way as 'ENOENT', isn't it? This \nensures that a random empty directory named '.git' doesn't stop the \ndiscovery process.\n\nSo, the logic for case 2 becomes:\n\n - Found a directory;\n - Check 'is_git_directory';\n - a. Valid? -----> Stop, we found it;\n - b. Invalid? -----> Ignore, continue the loop.\n\nRegards,\n\nYuchen\n\n"},{"id":"536151","messageId":"20260217084124.150366-1-a3205153416@gmail.com","threadId":"64979","inReplyTo":"5b29218a-8d18-41f0-8a03-eac707151945@gmail.com","subject":"[PATCH v4] setup: allow cwd/.git to be a symlink to a directory","fromName":"Tian Yuchen","fromEmail":"a3205153416@gmail.com","sentAt":"2026-02-17T08:41:24Z","receivedAt":"2026-02-17T08:41:34Z","isPatch":true,"sender":{"key":"cat@malon.dev","avatar":"https://avatars.githubusercontent.com/u/232002048?v=4"},"body":"Strictly enforcing 'lstat()' and 'S_ISREG()' on '.git' prevents valid\nworkflows where '.git' is a symbolic link pointing to a real git\ndirectory (e.g. created via 'ln -s').\n\nRefactor 'setup_git_directory_gently_1()' to use 'stat()' instead of\n'lstat()'. This allows the filesystem to automatically resolve symbolic\nlinks.\n\nTo ensure safety and correctness, the logic flow is updated to:\n\n1. Ignore 'ENOENT' (file missing).\n2. Check 'IS_A_DIR' cases via 'is_git_directory()'.\n3. Explicitly reject 'NOT_A_FILE' cases (FIFOs or sockets).\n\nAdd a new test script t/t0009-setup-security.sh which verifies:\n\n- Valid .git symlinks to real directories are accepted.\n- .git as a named pipe (FIFO) is rejected.\n- .git as a symlink to a named pipe is rejected.\n- .git with garbage content is rejected.\n- Empty .git directories are ignored.\n\nSigned-off-by: Tian Yuchen \n---\n setup.c | 39 ++++++++++++++-------\n setup.h | 2 ++\n t/t0009-setup-security.sh | 72 +++++++++++++++++++++++++++++++++++++++\n 3 files changed, 101 insertions(+), 12 deletions(-)\n create mode 100755 t/t0009-setup-security.sh\n\ndiff --git a/setup.c b/setup.c\nindex 3a6a048620..269aa9faaa 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -939,8 +939,14 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)\n \tstatic struct strbuf realpath = STRBUF_INIT;\n \n \tif (stat(path, &st)) {\n-\t\t/* NEEDSWORK: discern between ENOENT vs other errors */\n-\t\terror_code = READ_GITFILE_ERR_STAT_FAILED;\n+\t\tif (errno == ENOENT)\n+\t\t\terror_code = READ_GITFILE_ERR_STAT_ENOENT;\n+\t\telse\n+\t\t\terror_code = READ_GITFILE_ERR_STAT_FAILED;\n+\t\tgoto cleanup_return;\n+\t}\n+\tif (S_ISDIR(st.st_mode)) {\n+\t\terror_code = READ_GITFILE_ERR_IS_A_DIR;\n \t\tgoto cleanup_return;\n \t}\n \tif (!S_ISREG(st.st_mode)) {\n@@ -994,7 +1000,9 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)\n cleanup_return:\n \tif (return_error_code)\n \t\t*return_error_code = error_code;\n-\telse if (error_code)\n+\telse if (error_code &&\n+\t\terror_code != READ_GITFILE_ERR_STAT_ENOENT &&\n+\t\terror_code != READ_GITFILE_ERR_IS_A_DIR)\n \t\tread_gitfile_error_die(error_code, path, dir);\n \n \tfree(buf);\n@@ -1576,20 +1584,27 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n \t\tif (offset > min_offset)\n \t\t\tstrbuf_addch(dir, '/');\n \t\tstrbuf_addstr(dir, DEFAULT_GIT_DIR_ENVIRONMENT);\n-\t\tgitdirenv = read_gitfile_gently(dir->buf, die_on_error ?\n-\t\t\t\t\t\tNULL : &error_code);\n+\t\tgitdirenv = read_gitfile_gently(dir->buf, &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\tif (error_code == READ_GITFILE_ERR_STAT_ENOENT) {\n+\t\t\t\t;\n+\t\t\t} else if (error_code == READ_GITFILE_ERR_IS_A_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);\n \t\t\t\t}\n-\t\t\t} else if (error_code != READ_GITFILE_ERR_STAT_FAILED)\n-\t\t\t\treturn GIT_DIR_INVALID_GITFILE;\n-\t\t} else\n-\t\t\tgitfile = xstrdup(dir->buf);\n+\t\t\t} else if (error_code == READ_GITFILE_ERR_NOT_A_FILE) {\n+\t\t\t\tif (die_on_error)\n+\t\t\t\t\tdie(_(\"Invalid %s: not a regular file or directory\"), dir->buf);\n+\t\t\t\telse\n+\t\t\t\t\treturn GIT_DIR_INVALID_GITFILE;\n+\t\t\t} else if (error_code != READ_GITFILE_ERR_STAT_FAILED) {\n+\t\t\t\tif (die_on_error)\n+\t\t\t\t\tread_gitfile_error_die(error_code, dir->buf, NULL);\n+\t\t\t\telse\n+\t\t\t\t\treturn GIT_DIR_INVALID_GITFILE;\n+\t\t\t}\n+\t\t}\n \t\t/*\n \t\t * Earlier, we tentatively added DEFAULT_GIT_DIR_ENVIRONMENT\n \t\t * to check that directory for a repository.\ndiff --git a/setup.h b/setup.h\nindex d55dcc6608..c23629cb4f 100644\n--- a/setup.h\n+++ b/setup.h\n@@ -36,6 +36,8 @@ int is_nonbare_repository_dir(struct strbuf *path);\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_STAT_ENOENT 9\n+#define READ_GITFILE_ERR_IS_A_DIR 10\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/t/t0009-setup-security.sh b/t/t0009-setup-security.sh\nnew file mode 100755\nindex 0000000000..72c5232147\n--- /dev/null\n+++ b/t/t0009-setup-security.sh\n@@ -0,0 +1,72 @@\n+#!/bin/sh\n+\n+test_description='setup: validation of .git file/directory types\n+\n+Verify that setup_git_directory() correctly handles:\n+1. Valid .git directories (including symlinks to them).\n+2. Invalid .git files (FIFOs, sockets) by erroring out.\n+3. Invalid .git files (garbage) by erroring out.\n+'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup: create parent git repository' '\n+\tgit init parent &&\n+\ttest_commit -C parent \"root-commit\"\n+'\n+\n+test_expect_success SYMLINKS 'setup: .git as a symlink to a directory is valid' '\n+\tmkdir -p parent/link-to-dir &&\n+\t(\n+\t\tcd parent/link-to-dir &&\n+\t\tgit init real-repo &&\n+\t\tln -s real-repo/.git .git &&\n+\t\tgit rev-parse --git-dir >actual &&\n+\t\techo .git >expect &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success PIPE 'setup: .git as a FIFO (named pipe) is rejected' '\n+\tmkdir -p parent/fifo &&\n+\t(\n+\t\tcd parent/fifo &&\n+\t\tmkfifo .git &&\n+\t\ttest_must_fail git rev-parse --git-dir 2>stderr &&\n+\t\tgrep \"not a regular file\" stderr\n+\t)\n+'\n+\n+test_expect_success SYMLINKS,PIPE 'setup: .git as a symlink to a FIFO is rejected' '\n+\tmkdir -p parent/symlink-fifo &&\n+\t(\n+\t\tcd parent/symlink-fifo &&\n+\t\tmkfifo target-fifo &&\n+\t\tln -s target-fifo .git &&\n+\t\ttest_must_fail git rev-parse --git-dir 2>stderr &&\n+\t\tgrep \"not a regular file\" stderr\n+\t)\n+'\n+\n+test_expect_success 'setup: .git with garbage content is rejected' '\n+\tmkdir -p parent/garbage &&\n+\t(\n+\t\tcd parent/garbage &&\n+\t\techo \"garbage\" >.git &&\n+\t\ttest_must_fail git rev-parse --git-dir 2>stderr &&\n+\t\tgrep \"invalid gitfile format\" stderr\n+\t)\n+'\n+\n+test_expect_success 'setup: .git as an empty directory is ignored' '\n+\tmkdir -p parent/empty-dir &&\n+\t(\n+\t\tcd parent/empty-dir &&\n+\t\tmkdir .git &&\n+\t\tgit rev-parse --git-dir >actual &&\n+\t\techo \"$TRASH_DIRECTORY/parent/.git\" >expect &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_done\n-- \n2.43.0\n\n"},{"id":"536173","messageId":"CAOLa=ZTeTWhb0Yc8rPEv8vONTHtSg3bSvW6FBC-AWrZzi12oCA@mail.gmail.com","threadId":"64979","inReplyTo":"20260217084124.150366-1-a3205153416@gmail.com","subject":"Re: [PATCH v4] setup: allow cwd/.git to be a symlink to a directory","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2026-02-17T11:26:10Z","receivedAt":"2026-02-17T11:26:12Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Tian Yuchen writes:\n\n> Strictly enforcing 'lstat()' and 'S_ISREG()' on '.git' prevents valid\n> workflows where '.git' is a symbolic link pointing to a real git\n> directory (e.g. created via 'ln -s').\n>\n> Refactor 'setup_git_directory_gently_1()' to use 'stat()' instead of\n> 'lstat()'. This allows the filesystem to automatically resolve symbolic\n> links.\n>\n> To ensure safety and correctness, the logic flow is updated to:\n>\n> 1. Ignore 'ENOENT' (file missing).\n> 2. Check 'IS_A_DIR' cases via 'is_git_directory()'.\n> 3. Explicitly reject 'NOT_A_FILE' cases (FIFOs or sockets).\n>\n\nSmall nit, it would have been a bit nicer to separate these out into\nindividual commits with tests added per commit.\n\n> Add a new test script t/t0009-setup-security.sh which verifies:\n>\n\nWouldn't something like 't0009-git-dir-validation.sh' be a better name?\n\n> - Valid .git symlinks to real directories are accepted.\n> - .git as a named pipe (FIFO) is rejected.\n> - .git as a symlink to a named pipe is rejected.\n> - .git with garbage content is rejected.\n> - Empty .git directories are ignored.\n>\n> Signed-off-by: Tian Yuchen \n> ---\n> setup.c | 39 ++++++++++++++-------\n> setup.h | 2 ++\n> t/t0009-setup-security.sh | 72 +++++++++++++++++++++++++++++++++++++++\n> 3 files changed, 101 insertions(+), 12 deletions(-)\n> create mode 100755 t/t0009-setup-security.sh\n>\n> diff --git a/setup.c b/setup.c\n> index 3a6a048620..269aa9faaa 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -939,8 +939,14 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)\n> \tstatic struct strbuf realpath = STRBUF_INIT;\n>\n> \tif (stat(path, &st)) {\n> -\t\t/* NEEDSWORK: discern between ENOENT vs other errors */\n> -\t\terror_code = READ_GITFILE_ERR_STAT_FAILED;\n> +\t\tif (errno == ENOENT)\n> +\t\t\terror_code = READ_GITFILE_ERR_STAT_ENOENT;\n> +\t\telse\n> +\t\t\terror_code = READ_GITFILE_ERR_STAT_FAILED;\n> +\t\tgoto cleanup_return;\n\nSo this differentiates between 'stat()' failing and the path not\nexisting. Ok.\n\n> +\t}\n> +\tif (S_ISDIR(st.st_mode)) {\n> +\t\terror_code = READ_GITFILE_ERR_IS_A_DIR;\n> \t\tgoto cleanup_return;\n> \t}\n\nOkay so if the '.git' file is a directory, we set the appropriate error.\nSo this error code is new and we introduce it in this patch.\n\n\n> \tif (!S_ISREG(st.st_mode)) {\n> @@ -994,7 +1000,9 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)\n> cleanup_return:\n> \tif (return_error_code)\n> \t\t*return_error_code = error_code;\n> -\telse if (error_code)\n> +\telse if (error_code &&\n> +\t\terror_code != READ_GITFILE_ERR_STAT_ENOENT &&\n> +\t\terror_code != READ_GITFILE_ERR_IS_A_DIR)\n> \t\tread_gitfile_error_die(error_code, path, dir);\n>\n\nI understand the exclusion here (they are non-fatal flows), but wouldn't\nit more make sense to add these two exclusions within\n`read_gitfile_error_die()` which already has two such exclusions? By\nseparating this out, it gets really confusing.\n\n> \tfree(buf);\n> @@ -1576,20 +1584,27 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n> \t\tif (offset > min_offset)\n> \t\t\tstrbuf_addch(dir, '/');\n> \t\tstrbuf_addstr(dir, DEFAULT_GIT_DIR_ENVIRONMENT);\n> -\t\tgitdirenv = read_gitfile_gently(dir->buf, die_on_error ?\n> -\t\t\t\t\t\tNULL : &error_code);\n> +\t\tgitdirenv = read_gitfile_gently(dir->buf, &error_code);\n\nOkay so we unconditionally read the error into errorcode, quick question\nthat comes to mind: Wouldn't this break the previous flow for when\n`die_on_error = 1`? Where `read_gitfile_error_die()` would've been\ncalled?\n\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\tif (error_code == READ_GITFILE_ERR_STAT_ENOENT) {\n> +\t\t\t\t;\n> +\t\t\t} else if (error_code == READ_GITFILE_ERR_IS_A_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);\n> \t\t\t\t}\n> -\t\t\t} else if (error_code != READ_GITFILE_ERR_STAT_FAILED)\n> -\t\t\t\treturn GIT_DIR_INVALID_GITFILE;\n> -\t\t} else\n> -\t\t\tgitfile = xstrdup(dir->buf);\n> +\t\t\t} else if (error_code == READ_GITFILE_ERR_NOT_A_FILE) {\n> +\t\t\t\tif (die_on_error)\n> +\t\t\t\t\tdie(_(\"Invalid %s: not a regular file or directory\"), dir->buf);\n> +\t\t\t\telse\n> +\t\t\t\t\treturn GIT_DIR_INVALID_GITFILE;\n> +\t\t\t} else if (error_code != READ_GITFILE_ERR_STAT_FAILED) {\n> +\t\t\t\tif (die_on_error)\n> +\t\t\t\t\tread_gitfile_error_die(error_code, dir->buf, NULL);\n\nWell seems a bit convoluted as we explicitly skip this in our call to\n`read_gitfile_gently()` to then call it ourselves.\n\n> +\t\t\t\telse\n> +\t\t\t\t\treturn GIT_DIR_INVALID_GITFILE;\n> +\t\t\t}\n> +\t\t}\n> \t\t/*\n> \t\t * Earlier, we tentatively added DEFAULT_GIT_DIR_ENVIRONMENT\n> \t\t * to check that directory for a repository.\n\n[snip]\n"},{"id":"536195","messageId":"e5cee6ca-b908-466a-b496-0b170c6a2838@gmail.com","threadId":"64979","inReplyTo":"CAOLa=ZTeTWhb0Yc8rPEv8vONTHtSg3bSvW6FBC-AWrZzi12oCA@mail.gmail.com","subject":"Re: [PATCH v4] setup: allow cwd/.git to be a symlink to a directory","fromName":"Tian Yuchen","fromEmail":"a3205153416@gmail.com","sentAt":"2026-02-17T15:30:25Z","receivedAt":"2026-02-17T15:30:33Z","isPatch":true,"sender":{"key":"cat@malon.dev","avatar":"https://avatars.githubusercontent.com/u/232002048?v=4"},"body":"Hi Karthik,\n\nThanks for the review!\n\n> Small nit, it would have been a bit nicer to separate these out into\n> individual commits with tests added per commit.\n\nSince this is a security fix involving logic changes, I kept the tests \nand code together to ensure the commit is self-contained. I hope keeping \nthem together is acceptable here!\n\n> Wouldn't something like 't0009-git-dir-validation.sh' be a better name?\n\nIndeed a much better name. Will rename it in the next reroll.\n\n> I understand the exclusion here (they are non-fatal flows), but wouldn't\n> it more make sense to add these two exclusions within\n> `read_gitfile_error_die()` which already has two such exclusions? By\n> separating this out, it gets really confusing.\n\nI actually implemented exactly that in previous patches (handling these \nexclusions inside 'read_gitfile_error_die'), but Junio pointed out that:\n\n >> diff --git a/setup.c b/setup.c\n >> index 3a6a048620..8681a8a9d1 100644\n >> --- a/setup.c\n >> +++ b/setup.c\n >> @@ -911,6 +911,10 @@ void read_gitfile_error_die(int error_code, \nconst char *path, const char *dir)\n >> \t\tdie(_(\"no path in gitfile: %s\"), path);\n >> \tcase READ_GITFILE_ERR_NOT_A_REPO:\n >> \t\tdie(_(\"not a git repository: %s\"), dir);\n >> +\tcase READ_GITFILE_ERR_STAT_ENOENT:\n >> +\t\tdie(_(\"Not a git repository: %s\"), path);\n >> +\tcase READ_GITFILE_ERR_IS_A_DIR:\n >> +\t\tdie(_(\"Not a git file (is a directory): %s\"), path);\n >\n > Hmph, isn't this backwards?\n >\n > We used to treat STAT_FAILED as OK without dying in this function,\n > because we conflated \"there is nothing there, so you should go one\n > level up and try again\" happy case with all other stat(2) failure,\n > and that is why we introduced STAT_ENOENT here. ENOENT is the\n > *only* case among what used to be STAT_FAILED that we do *not* want\n > to die in this function. The same thing with NOT_A_FILE vs\n > IS_A_DIR. We used to treat the former as OK but the only case we\n > wanted to treat as OK was IS_A_DIR and all other cases, like FIFO,\n > we wanted to complain, no?\n\nIn other word, ENOENT and IS_A_DIR cases are *VALID* states during the \ndiscovery process, not *ERRORS* that need to be suppressed in a \"die\" \nfunction. Therefore, we moved the decision-making logic up to the \ncaller. This allows 'setup_git_directory_gently_1' to decide:\n\nENOENT -> Continue search\nIS_A_DIR -> Check dir\nNOT_A_FILE -> Die\nOther -> Call 'read_gitfile_error_die()' *REAL ERROR*\n\n> Okay so we unconditionally read the error into errorcode, quick question\n> that comes to mind: Wouldn't this break the previous flow for when\n> `die_on_error = 1`? Where `read_gitfile_error_die()` would've been\n> called?\n\nIt does change the flow, but intentionally, by passing &error_code \n(making it non-NULL), we prevent 'read_gitfile_gently' from \nautomatically dying.\n\nWe must do it because if it encounters a \"garbage file\", we now want to \ncapture that error code and verify it in the caller. But more \nimportantly, if it encounters ENOENT (which is now a distinct error \ncode), we definitely do not want it to die, nor do we want to treat it \nas a fatal error.\n\nIt does look a bit verbose, but it makes the state transitions explicit \nin 'setup_git_directory_gently_1'. I believe we are on the right track!\n\nThanks again for the feedback.\n\nRegards,\n\nYuchen\n\n\n\n"},{"id":"536203","messageId":"xmqqv7fvcnj5.fsf@gitster.g","threadId":"64979","inReplyTo":"CAOLa=ZTeTWhb0Yc8rPEv8vONTHtSg3bSvW6FBC-AWrZzi12oCA@mail.gmail.com","subject":"Re: [PATCH v4] setup: allow cwd/.git to be a symlink to a directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-02-17T17:01:18Z","receivedAt":"2026-02-17T17:01:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n>> @@ -994,7 +1000,9 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)\n>> cleanup_return:\n>> \tif (return_error_code)\n>> \t\t*return_error_code = error_code;\n>> -\telse if (error_code)\n>> +\telse if (error_code &&\n>> +\t\terror_code != READ_GITFILE_ERR_STAT_ENOENT &&\n>> +\t\terror_code != READ_GITFILE_ERR_IS_A_DIR)\n>> \t\tread_gitfile_error_die(error_code, path, dir);\n>>\n>\n> I understand the exclusion here (they are non-fatal flows), but wouldn't\n> it more make sense to add these two exclusions within\n> `read_gitfile_error_die()` which already has two such exclusions? By\n> separating this out, it gets really confusing.\n\nAbsolutely. The point of this change, IIUC, is that these two\nexisting exclusions were too broad. stat() can fail for many\nreasons, but because we did not differenciate ENOENT (which we *are*\nhappy to see and do not want to consider an error) from all other\nerror cases (which we may have been better off if we diagnosed them\nas error), we pretended both ENOENT and all other stat() failures\nwere happy case and \"case ERR_STAT_FAILED:\" covered both. To fix\nthis, the patch splits stat() failures into two, ERR_STAT_ENOENT is\nthe happy case we should have been returning without dying from\nread_gitfile_error_die(), and ERR_STAT_FAILED is the rest that we\nshould have been dying there but in order to return from there\nwithout dying when we got ENOENT, we were not dying there. Now we\nhave a separate ERR_STAT_ENOENT, read_gitfile_error_die() can (and\nshould) die when we see ERR_STAT_FAILED, and it can (and should)\nreturn to us when we see ERR_STAT_ENOENT as a happy case. The story\nis exactly the same between ERR_NOT_A_FILE (which had been non-error\nonly because we wanted to treat a directory as OK, but we can make\nit an error) and ERR_IS_A DIR (which is new, and is an OK case).\n\nThe above exception on the caller's side you quoted is a complete\nopposite from that line of reasoning, and that is why it is\nconfusing.\n\nIf there are other callers of read_gitfile_error_die() and different\nsemantics, such a \"now we die on every possible errors\" may also be\na valid position to take, *but* then it does not make sense unless\nthis patch makes read_gitfile_error_die() to die on ERR_STAT_FAILED\nand ERR_NOT_A_FILE.\n"},{"id":"536210","messageId":"CAOLa=ZR=2B7yH+vtyiAPcCyU17yd2GZwonaj=JRo1f+LzSCoTg@mail.gmail.com","threadId":"64979","inReplyTo":"20260217084124.150366-1-a3205153416@gmail.com","subject":"Re: [PATCH v4] setup: allow cwd/.git to be a symlink to a directory","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2026-02-17T17:59:44Z","receivedAt":"2026-02-17T17:59:46Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Tian Yuchen writes:\n\n> Strictly enforcing 'lstat()' and 'S_ISREG()' on '.git' prevents valid\n> workflows where '.git' is a symbolic link pointing to a real git\n> directory (e.g. created via 'ln -s').\n>\n> Refactor 'setup_git_directory_gently_1()' to use 'stat()' instead of\n> 'lstat()'. This allows the filesystem to automatically resolve symbolic\n> links.\n>\n> To ensure safety and correctness, the logic flow is updated to:\n>\n> 1. Ignore 'ENOENT' (file missing).\n> 2. Check 'IS_A_DIR' cases via 'is_git_directory()'.\n> 3. Explicitly reject 'NOT_A_FILE' cases (FIFOs or sockets).\n>\n> Add a new test script t/t0009-setup-security.sh which verifies:\n>\n> - Valid .git symlinks to real directories are accepted.\n> - .git as a named pipe (FIFO) is rejected.\n> - .git as a symlink to a named pipe is rejected.\n> - .git with garbage content is rejected.\n> - Empty .git directories are ignored.\n>\n> Signed-off-by: Tian Yuchen \n> ---\n> setup.c | 39 ++++++++++++++-------\n> setup.h | 2 ++\n> t/t0009-setup-security.sh | 72 +++++++++++++++++++++++++++++++++++++++\n> 3 files changed, 101 insertions(+), 12 deletions(-)\n> create mode 100755 t/t0009-setup-security.sh\n>\n\nI also missed this in my review, but the test needs to be added to\n'meson.build', without which meson would fail. This is caught by our CI\ntoo, if you run the CI on GitLab/GitHub you should see the issue.\n\nKarthik\n"},{"id":"536216","messageId":"CAOLa=ZQ8i0pUAbF8NGidR=6jVLJuiYdUWu=ZLDrJMek005D9bA@mail.gmail.com","threadId":"64979","inReplyTo":"xmqqv7fvcnj5.fsf@gitster.g","subject":"Re: [PATCH v4] setup: allow cwd/.git to be a symlink to a directory","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2026-02-17T18:50:20Z","receivedAt":"2026-02-17T18:50:22Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Junio C Hamano writes:\n\n> Karthik Nayak writes:\n>\n>>> @@ -994,7 +1000,9 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)\n>>> cleanup_return:\n>>> \tif (return_error_code)\n>>> \t\t*return_error_code = error_code;\n>>> -\telse if (error_code)\n>>> +\telse if (error_code &&\n>>> +\t\terror_code != READ_GITFILE_ERR_STAT_ENOENT &&\n>>> +\t\terror_code != READ_GITFILE_ERR_IS_A_DIR)\n>>> \t\tread_gitfile_error_die(error_code, path, dir);\n>>>\n>>\n>> I understand the exclusion here (they are non-fatal flows), but wouldn't\n>> it more make sense to add these two exclusions within\n>> `read_gitfile_error_die()` which already has two such exclusions? By\n>> separating this out, it gets really confusing.\n>\n> Absolutely. The point of this change, IIUC, is that these two\n> existing exclusions were too broad. stat() can fail for many\n> reasons, but because we did not differenciate ENOENT (which we *are*\n> happy to see and do not want to consider an error) from all other\n> error cases (which we may have been better off if we diagnosed them\n> as error), we pretended both ENOENT and all other stat() failures\n> were happy case and \"case ERR_STAT_FAILED:\" covered both.\n\nYeah, so this is the situation before the patch, and I'm in agreement.\n\n> To fix\n> this, the patch splits stat() failures into two, ERR_STAT_ENOENT is\n> the happy case we should have been returning without dying from\n> read_gitfile_error_die(), and ERR_STAT_FAILED is the rest that we\n> should have been dying there but in order to return from there\n> without dying when we got ENOENT, we were not dying there. Now we\n> have a separate ERR_STAT_ENOENT, read_gitfile_error_die() can (and\n> should) die when we see ERR_STAT_FAILED, and it can (and should)\n> return to us when we see ERR_STAT_ENOENT as a happy case.\n\nOkay, this is what I was expecting too, historically\n`read_gitfile_error_die()` treated ERR_STAT_FAILED as the non-fatal path\nwhich made sense. But now that we have ERR_STAT_ENOENT. It should treat\nthe latter as the non-fatal path and the former as an actual issue.\n\n> The story\n> is exactly the same between ERR_NOT_A_FILE (which had been non-error\n> only because we wanted to treat a directory as OK, but we can make\n> it an error) and ERR_IS_A DIR (which is new, and is an OK case).\n>\n\nYup makes sense.\n\n> The above exception on the caller's side you quoted is a complete\n> opposite from that line of reasoning, and that is why it is\n> confusing.\n>\n> If there are other callers of read_gitfile_error_die() and different\n> semantics, such a \"now we die on every possible errors\" may also be\n> a valid position to take, *but* then it does not make sense unless\n> this patch makes read_gitfile_error_die() to die on ERR_STAT_FAILED\n> and ERR_NOT_A_FILE.\n\nExactly! Thanks for clearing it out.\n\nKarthik\n"},{"id":"536218","messageId":"CAOLa=ZR-0DGm4eHB6oqi6FpdOV1YDT6mf0=ONZnpi==3o3ab+w@mail.gmail.com","threadId":"64979","inReplyTo":"e5cee6ca-b908-466a-b496-0b170c6a2838@gmail.com","subject":"Re: [PATCH v4] setup: allow cwd/.git to be a symlink to a directory","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2026-02-17T18:56:03Z","receivedAt":"2026-02-17T18:56:05Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Tian Yuchen writes:\n\n> Hi Karthik,\n>\n> Thanks for the review!\n>\n>> Small nit, it would have been a bit nicer to separate these out into\n>> individual commits with tests added per commit.\n>\n> Since this is a security fix involving logic changes, I kept the tests\n> and code together to ensure the commit is self-contained. I hope keeping\n> them together is acceptable here!\n>\n\nMy indication wasn't separate the tests out into an individual commit.\nBut rather to highlight that this one commit is doing multiple things\nand it would be nice to split it out and each commit could tackle an\nindividual problem with tests included.\n\n>> Wouldn't something like 't0009-git-dir-validation.sh' be a better name?\n>\n> Indeed a much better name. Will rename it in the next reroll.\n>\n>> I understand the exclusion here (they are non-fatal flows), but wouldn't\n>> it more make sense to add these two exclusions within\n>> `read_gitfile_error_die()` which already has two such exclusions? By\n>> separating this out, it gets really confusing.\n>\n> I actually implemented exactly that in previous patches (handling these\n> exclusions inside 'read_gitfile_error_die'), but Junio pointed out that:\n>\n> >> diff --git a/setup.c b/setup.c\n> >> index 3a6a048620..8681a8a9d1 100644\n> >> --- a/setup.c\n> >> +++ b/setup.c\n> >> @@ -911,6 +911,10 @@ void read_gitfile_error_die(int error_code,\n> const char *path, const char *dir)\n> >> \t\tdie(_(\"no path in gitfile: %s\"), path);\n> >> \tcase READ_GITFILE_ERR_NOT_A_REPO:\n> >> \t\tdie(_(\"not a git repository: %s\"), dir);\n> >> +\tcase READ_GITFILE_ERR_STAT_ENOENT:\n> >> +\t\tdie(_(\"Not a git repository: %s\"), path);\n> >> +\tcase READ_GITFILE_ERR_IS_A_DIR:\n> >> +\t\tdie(_(\"Not a git file (is a directory): %s\"), path);\n> >\n> > Hmph, isn't this backwards?\n> >\n> > We used to treat STAT_FAILED as OK without dying in this function,\n> > because we conflated \"there is nothing there, so you should go one\n> > level up and try again\" happy case with all other stat(2) failure,\n> > and that is why we introduced STAT_ENOENT here. ENOENT is the\n> > *only* case among what used to be STAT_FAILED that we do *not* want\n> > to die in this function. The same thing with NOT_A_FILE vs\n> > IS_A_DIR. We used to treat the former as OK but the only case we\n> > wanted to treat as OK was IS_A_DIR and all other cases, like FIFO,\n> > we wanted to complain, no?\n>\n> In other word, ENOENT and IS_A_DIR cases are *VALID* states during the\n> discovery process, not *ERRORS* that need to be suppressed in a \"die\"\n> function. Therefore, we moved the decision-making logic up to the\n> caller. This allows 'setup_git_directory_gently_1' to decide:\n>\n> ENOENT -> Continue search\n> IS_A_DIR -> Check dir\n> NOT_A_FILE -> Die\n> Other -> Call 'read_gitfile_error_die()' *REAL ERROR*\n>\n\nMy understanding was Junio was suggesting that what you're doing is the\ninverse of what is expected. In short (on top of your patch), something\nlike:\n\ndiff --git a/setup.c b/setup.c\nindex c3dd6a4197..7edf921564 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -898,10 +898,14 @@ int verify_repository_format(const struct\nrepository_format *format,\n 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_STAT_ENOENT:\n+\tcase READ_GITFILE_ERR_IS_A_DIR:\n \t\t/* non-fatal; follow return path */\n \t\tbreak;\n+\tcase READ_GITFILE_ERR_STAT_FAILED:\n+\t\tdie(_(\"stat failed to run correctly\"))\n+\tcase READ_GITFILE_ERR_NOT_A_FILE:\n+\t\tdie(_(\"unsupported file type\"))\n \tcase READ_GITFILE_ERR_OPEN_FAILED:\n \t\tdie_errno(_(\"error opening '%s'\"), path);\n \tcase READ_GITFILE_ERR_TOO_LARGE:\n\n>> Okay so we unconditionally read the error into errorcode, quick question\n>> that comes to mind: Wouldn't this break the previous flow for when\n>> `die_on_error = 1`? Where `read_gitfile_error_die()` would've been\n>> called?\n>\n> It does change the flow, but intentionally, by passing &error_code\n> (making it non-NULL), we prevent 'read_gitfile_gently' from\n> automatically dying.\n>\n> We must do it because if it encounters a \"garbage file\", we now want to\n> capture that error code and verify it in the caller. But more\n> importantly, if it encounters ENOENT (which is now a distinct error\n> code), we definitely do not want it to die, nor do we want to treat it\n> as a fatal error.\n>\n> It does look a bit verbose, but it makes the state transitions explicit\n> in 'setup_git_directory_gently_1'. I believe we are on the right track!\n>\n> Thanks again for the feedback.\n>\n> Regards,\n>\n> Yuchen\n"},{"id":"536227","messageId":"xmqqo6ln9iu6.fsf@gitster.g","threadId":"64979","inReplyTo":"CAOLa=ZR-0DGm4eHB6oqi6FpdOV1YDT6mf0=ONZnpi==3o3ab+w@mail.gmail.com","subject":"Re: [PATCH v4] setup: allow cwd/.git to be a symlink to a directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-02-17T21:10:57Z","receivedAt":"2026-02-17T21:10:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> My indication wasn't separate the tests out into an individual commit.\n> But rather to highlight that this one commit is doing multiple things\n> and it would be nice to split it out and each commit could tackle an\n> individual problem with tests included.\n\nLike treating \"all stat failures silently ignored, instead of\ntreating ENOENT specifically\" and \"a non-directory non-file\nfilesystem entity being silently ignored\" two separate issues, so at\nleast two patches, and possibly if we need a preliminary clean-up\nto make these two changes, yet another one? And each patch focuses\non one thing it addresses, with its own test? That makes sense.\n\nThanks.\n"},{"id":"536244","messageId":"8e9399db-e35b-4c0c-902d-b64c99cdc099@gmail.com","threadId":"64979","inReplyTo":"CAOLa=ZQ8i0pUAbF8NGidR=6jVLJuiYdUWu=ZLDrJMek005D9bA@mail.gmail.com","subject":"Re: [PATCH v4] setup: allow cwd/.git to be a symlink to a directory","fromName":"Tian Yuchen","fromEmail":"a3205153416@gmail.com","sentAt":"2026-02-18T04:08:38Z","receivedAt":"2026-02-18T04:08:42Z","isPatch":true,"sender":{"key":"cat@malon.dev","avatar":"https://avatars.githubusercontent.com/u/232002048?v=4"},"body":"On 2/18/26 02:50, Karthik Nayak wrote:\n> Junio C Hamano writes:\n> \n>> Karthik Nayak writes:\n>>\n>>>> @@ -994,7 +1000,9 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)\n>>>> cleanup_return:\n>>>> \tif (return_error_code)\n>>>> \t\t*return_error_code = error_code;\n>>>> -\telse if (error_code)\n>>>> +\telse if (error_code &&\n>>>> +\t\terror_code != READ_GITFILE_ERR_STAT_ENOENT &&\n>>>> +\t\terror_code != READ_GITFILE_ERR_IS_A_DIR)\n>>>> \t\tread_gitfile_error_die(error_code, path, dir);\n>>>>\n>>>\n>>> I understand the exclusion here (they are non-fatal flows), but wouldn't\n>>> it more make sense to add these two exclusions within\n>>> `read_gitfile_error_die()` which already has two such exclusions? By\n>>> separating this out, it gets really confusing.\n>>\n>> Absolutely. The point of this change, IIUC, is that these two\n>> existing exclusions were too broad. stat() can fail for many\n>> reasons, but because we did not differenciate ENOENT (which we *are*\n>> happy to see and do not want to consider an error) from all other\n>> error cases (which we may have been better off if we diagnosed them\n>> as error), we pretended both ENOENT and all other stat() failures\n>> were happy case and \"case ERR_STAT_FAILED:\" covered both.\n> \n> Yeah, so this is the situation before the patch, and I'm in agreement.\n> \n>> To fix\n>> this, the patch splits stat() failures into two, ERR_STAT_ENOENT is\n>> the happy case we should have been returning without dying from\n>> read_gitfile_error_die(), and ERR_STAT_FAILED is the rest that we\n>> should have been dying there but in order to return from there\n>> without dying when we got ENOENT, we were not dying there. Now we\n>> have a separate ERR_STAT_ENOENT, read_gitfile_error_die() can (and\n>> should) die when we see ERR_STAT_FAILED, and it can (and should)\n>> return to us when we see ERR_STAT_ENOENT as a happy case.\n> \n> Okay, this is what I was expecting too, historically\n> `read_gitfile_error_die()` treated ERR_STAT_FAILED as the non-fatal path\n> which made sense. But now that we have ERR_STAT_ENOENT. It should treat\n> the latter as the non-fatal path and the former as an actual issue.\n> \n>> The story\n>> is exactly the same between ERR_NOT_A_FILE (which had been non-error\n>> only because we wanted to treat a directory as OK, but we can make\n>> it an error) and ERR_IS_A DIR (which is new, and is an OK case).\n>>\n> \n> Yup makes sense.\n> \n>> The above exception on the caller's side you quoted is a complete\n>> opposite from that line of reasoning, and that is why it is\n>> confusing.\n>>\n>> If there are other callers of read_gitfile_error_die() and different\n>> semantics, such a \"now we die on every possible errors\" may also be\n>> a valid position to take, *but* then it does not make sense unless\n>> this patch makes read_gitfile_error_die() to die on ERR_STAT_FAILED\n>> and ERR_NOT_A_FILE.\n> \n> Exactly! Thanks for clearing it out.\n> \n> Karthik\n\nI don't think leaving it up yo the caller to determine \"which are error \ncases\" is as confusing as you suggest. But honestly, your approach is \nindeed much clearer and more readable.\n\nI'll split this into two commits and send them shortly.\n\nRegards,\n\nYuchen\n"},{"id":"536254","messageId":"20260218051850.164972-1-a3205153416@gmail.com","threadId":"64979","inReplyTo":"20260217084124.150366-1-a3205153416@gmail.com","subject":"[PATCH v5 0/2] setup.c: v5 reroll","fromName":"Tian Yuchen","fromEmail":"a3205153416@gmail.com","sentAt":"2026-02-18T05:18:48Z","receivedAt":"2026-02-18T05:19:07Z","isPatch":true,"sender":{"key":"cat@malon.dev","avatar":"https://avatars.githubusercontent.com/u/232002048?v=4"},"body":"Hi Karthik, Junio,\n\nHere is the v5 patch.\n\nBases on the feedback, I have split the changed into two separate\npatches to isolate the refactoring from the logic change.\n\nPatch 1/2: Refactors read_gitfile_* functions to correctly distinguish\nENOENT from other stat() failures.\n\nPatch 2/2: Switches lstat() to stat() to support symlinks, while adding\nchecks for benign and fatal cases. Also includes the new test script\nt0009-git-dir-validation.sh (name has been changed as suggested by\nKarthik).\n\nChanges since v4:\n\n1. Split into 2 commits.\n2. read_gitfile_error_die() now handles happy/fatal cases internally.\n3. Renamed the test script.\n4. Updates t/meson.build.\n\nIf there are any further issues, please feel free to bring them to my\nattention. Thanks for guiding me towards this cleaner structure!\n\nRegards,\n\nYuchen\n\n---\n\nTian Yuchen (2):\n setup: distingush ENOENT from other stat errors\n setup: allow cwd/.git to be a symlink to a directory\n\n setup.c | 35 ++++++++++-------\n setup.h | 2 +\n t/meson.build | 1 +\n t/t0009-git-dir-validation.sh | 72 +++++++++++++++++++++++++++++++++++\n 4 files changed, 96 insertions(+), 14 deletions(-)\n create mode 100755 t/t0009-git-dir-validation.sh\n\n-- \n2.43.0\n\n"},{"id":"536255","messageId":"20260218051850.164972-2-a3205153416@gmail.com","threadId":"64979","inReplyTo":"20260218051850.164972-1-a3205153416@gmail.com","subject":"[PATCH v5 1/2] setup: distingush ENOENT from other stat errors","fromName":"Tian Yuchen","fromEmail":"a3205153416@gmail.com","sentAt":"2026-02-18T05:18:49Z","receivedAt":"2026-02-18T05:19:25Z","isPatch":true,"sender":{"key":"cat@malon.dev","avatar":"https://avatars.githubusercontent.com/u/232002048?v=4"},"body":"Currently, 'read_gitfile_gently()' treats all 'stat()' failures as\ngeneric errors. This prevents distinguishing between a missing file and\nreal errors like permission denied (fatal).\n\nIntroduce 'READ_GITFILE_ERR_STAT_ENOENT' and 'READ_GITFILE_ERR_IS_A_DIR'.\n\nUpdata 'read_gitfile_error_die()' to handle these cases:\n\n1. Return for happy cases ('ENOENT', 'IS_A_DIR');\n2. Die for fatal cases ('STAT_FAILED', 'NOT_A_FILE');\n\nThis prepares for error handling in the next commit.\n\nSigned-off-by: Tian Yuchen \n---\n setup.c | 17 +++++++++++++----\n setup.h | 2 ++\n 2 files changed, 15 insertions(+), 4 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex c8336eb20e..0ca129623e 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -897,10 +897,13 @@ int verify_repository_format(const struct repository_format *format,\n 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_ENOENT:\n+\tcase READ_GITFILE_ERR_IS_A_DIR:\n+\t\treturn;\n \tcase READ_GITFILE_ERR_STAT_FAILED:\n+\t\tdie(_(\"error reading %s\"), path);\n \tcase READ_GITFILE_ERR_NOT_A_FILE:\n-\t\t/* non-fatal; follow return path */\n-\t\tbreak;\n+\t\tdie(_(\"invalid %s: not a regular file\"), 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,8 +944,14 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)\n \tstatic struct strbuf realpath = STRBUF_INIT;\n \n \tif (stat(path, &st)) {\n-\t\t/* NEEDSWORK: discern between ENOENT vs other errors */\n-\t\terror_code = READ_GITFILE_ERR_STAT_FAILED;\n+\t\tif (errno == ENOENT)\n+\t\t\terror_code = READ_GITFILE_ERR_STAT_ENOENT;\n+\t\telse\n+\t\t\terror_code = READ_GITFILE_ERR_STAT_FAILED;\n+\t\tgoto cleanup_return;\n+\t}\n+\tif (S_ISDIR(st.st_mode)) {\n+\t\terror_code = READ_GITFILE_ERR_IS_A_DIR;\n \t\tgoto cleanup_return;\n \t}\n \tif (!S_ISREG(st.st_mode)) {\ndiff --git a/setup.h b/setup.h\nindex 0738dec244..ed4b13f061 100644\n--- a/setup.h\n+++ b/setup.h\n@@ -36,6 +36,8 @@ int is_nonbare_repository_dir(struct strbuf *path);\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_STAT_ENOENT 9\n+#define READ_GITFILE_ERR_IS_A_DIR 10\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)\n-- \n2.43.0\n\n"},{"id":"536256","messageId":"20260218051850.164972-3-a3205153416@gmail.com","threadId":"64979","inReplyTo":"20260218051850.164972-1-a3205153416@gmail.com","subject":"[PATCH v5 2/2] setup: allow cwd/.git to be a symlink to a directory","fromName":"Tian Yuchen","fromEmail":"a3205153416@gmail.com","sentAt":"2026-02-18T05:18:50Z","receivedAt":"2026-02-18T05:19:28Z","isPatch":true,"sender":{"key":"cat@malon.dev","avatar":"https://avatars.githubusercontent.com/u/232002048?v=4"},"body":"Strictly enforcing 'lstat()' prevents valid '.git' symlinks.\n\nSwitch 'setup_git_directory_gently_1()' to use 'stat()' to allow\nfilesystem resolution.\n\nCalling the refactored 'read_gitfile_error_die()' to ensure safety:\n\n1. Happy cases ('ENOENT', 'IS_A_DIR') are ignored automatically;\n2. Invalid types (like FIFOs and sockets) trigger 'die()' via\n 'NOT_A_FILE'.\n\nAdd 't/t0009-git-dir-validation.sh' to verify symlink support and FIFO\nrejection, and register it in 't/meson.build'.\n\nSigned-off-by: Tian Yuchen \n---\n setup.c | 18 ++++-----\n t/meson.build | 1 +\n t/t0009-git-dir-validation.sh | 72 +++++++++++++++++++++++++++++++++++\n 3 files changed, 81 insertions(+), 10 deletions(-)\n create mode 100755 t/t0009-git-dir-validation.sh\n\ndiff --git a/setup.c b/setup.c\nindex 0ca129623e..6e6068e5eb 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1590,17 +1590,15 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n \t\tgitdirenv = read_gitfile_gently(dir->buf, die_on_error ?\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\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);\n-\t\t\t\t}\n-\t\t\t} else if (error_code != READ_GITFILE_ERR_STAT_FAILED)\n-\t\t\t\treturn GIT_DIR_INVALID_GITFILE;\n-\t\t} else\n+\t\t\tif (error_code)\n+\t\t\t\tread_gitfile_error_die(error_code, dir->buf, NULL);\n+\t\t\tif (is_git_directory(dir->buf)) {\n+\t\t\t\tgitdirenv = DEFAULT_GIT_DIR_ENVIRONMENT;\n+\t\t\t\tgitdir_path = xstrdup(dir->buf);\n+\t\t\t}\n+\t\t} else {\n \t\t\tgitfile = xstrdup(dir->buf);\n+\t\t}\n \t\t/*\n \t\t * Earlier, we tentatively added DEFAULT_GIT_DIR_ENVIRONMENT\n \t\t * to check that directory for a repository.\ndiff --git a/t/meson.build b/t/meson.build\nindex f80e366cff..c4afaacee5 100644\n--- a/t/meson.build\n+++ b/t/meson.build\n@@ -80,6 +80,7 @@ integration_tests = [\n 't0006-date.sh',\n 't0007-git-var.sh',\n 't0008-ignores.sh',\n+ 't0009-git-dir-validation.sh',\n 't0010-racy-git.sh',\n 't0012-help.sh',\n 't0013-sha1dc.sh',\ndiff --git a/t/t0009-git-dir-validation.sh b/t/t0009-git-dir-validation.sh\nnew file mode 100755\nindex 0000000000..9b3925c85f\n--- /dev/null\n+++ b/t/t0009-git-dir-validation.sh\n@@ -0,0 +1,72 @@\n+#!/bin/sh\n+\n+test_description='setup: validation of .git file/directory types\n+\n+Verify that setup_git_directory() correctly handles:\n+1. Valid .git directories (including symlinks to them).\n+2. Invalid .git files (FIFOs, sockets) by erroring out.\n+3. Invalid .git files (garbage) by erroring out.\n+'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup: create parent git repository' '\n+\tgit init parent &&\n+\ttest_commit -C parent \"root-commit\"\n+'\n+\n+test_expect_success SYMLINKS 'setup: .git as a symlink to a directory is valid' '\n+\tmkdir -p parent/link-to-dir &&\n+\t(\n+\t\tcd parent/link-to-dir &&\n+\t\tgit init real-repo &&\n+\t\tln -s real-repo/.git .git &&\n+\t\tgit rev-parse --git-dir >actual &&\n+\t\techo .git >expect &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success PIPE 'setup: .git as a FIFO (named pipe) is rejected' '\n+\tmkdir -p parent/fifo-trap &&\n+\t(\n+\t\tcd parent/fifo-trap &&\n+\t\tmkfifo .git &&\n+\t\ttest_must_fail git rev-parse --git-dir 2>stderr &&\n+\t\tgrep \"not a regular file\" stderr\n+\t)\n+'\n+\n+test_expect_success SYMLINKS,PIPE 'setup: .git as a symlink to a FIFO is rejected' '\n+\tmkdir -p parent/symlink-fifo-trap &&\n+\t(\n+\t\tcd parent/symlink-fifo-trap &&\n+\t\tmkfifo target-fifo &&\n+\t\tln -s target-fifo .git &&\n+\t\ttest_must_fail git rev-parse --git-dir 2>stderr &&\n+\t\tgrep \"not a regular file\" stderr\n+\t)\n+'\n+\n+test_expect_success 'setup: .git with garbage content is rejected' '\n+\tmkdir -p parent/garbage-trap &&\n+\t(\n+\t\tcd parent/garbage-trap &&\n+\t\techo \"garbage\" >.git &&\n+\t\ttest_must_fail git rev-parse --git-dir 2>stderr &&\n+\t\tgrep \"invalid gitfile format\" stderr\n+\t)\n+'\n+\n+test_expect_success 'setup: .git as an empty directory is ignored' '\n+\tmkdir -p parent/empty-dir &&\n+\t(\n+\t\tcd parent/empty-dir &&\n+\t\tmkdir .git &&\n+\t\tgit rev-parse --git-dir >actual &&\n+\t\techo \"$TRASH_DIRECTORY/parent/.git\" >expect &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_done\n-- \n2.43.0\n\n"},{"id":"536267","messageId":"CAOLa=ZR_tH6A6JEj7NwziwYaVtezkHMez_cZNYyU1TQi5D8=XQ@mail.gmail.com","threadId":"64979","inReplyTo":"20260218051850.164972-2-a3205153416@gmail.com","subject":"Re: [PATCH v5 1/2] setup: distingush ENOENT from other stat errors","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2026-02-18T10:12:16Z","receivedAt":"2026-02-18T10:12:19Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Tian Yuchen writes:\n\n> Currently, 'read_gitfile_gently()' treats all 'stat()' failures as\n> generic errors. This prevents distinguishing between a missing file and\n> real errors like permission denied (fatal).\n\nCorrect.\n\n> Introduce 'READ_GITFILE_ERR_STAT_ENOENT' and 'READ_GITFILE_ERR_IS_A_DIR'.\n\nOkay, to clarify we're making two distinct changes:\n1. Split the 'stat()' error ERR_STAT_FAILED into ERR_STAT_FAILED and\n ERR_STAT_ENOENT. This is what your previous paragraph described. It\n would be nice if you could also explain this.\n2. We introduce ERR_IS_A_DIR for when the .git path is a directory. We\n don't talk about this at all.\n\n> Updata 'read_gitfile_error_die()' to handle these cases:\n>\n> 1. Return for happy cases ('ENOENT', 'IS_A_DIR');\n> 2. Die for fatal cases ('STAT_FAILED', 'NOT_A_FILE');\n>\n\nNit: It would be nice to explain on the decision behind picking errors\nfor the happy vs fatal path.\n\n> This prepares for error handling in the next commit.\n>\n> Signed-off-by: Tian Yuchen \n> ---\n> setup.c | 17 +++++++++++++----\n> setup.h | 2 ++\n> 2 files changed, 15 insertions(+), 4 deletions(-)\n>\n> diff --git a/setup.c b/setup.c\n> index c8336eb20e..0ca129623e 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -897,10 +897,13 @@ int verify_repository_format(const struct repository_format *format,\n> 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_ENOENT:\n> +\tcase READ_GITFILE_ERR_IS_A_DIR:\n> +\t\treturn;\n\nNit: For this function it should be okay to return early. But I was\nexpecting a break here, since it was using 'break' before, ideally we\nshouldn't change it unless there is a reason to.\n\n> \tcase READ_GITFILE_ERR_STAT_FAILED:\n> +\t\tdie(_(\"error reading %s\"), path);\n> \tcase READ_GITFILE_ERR_NOT_A_FILE:\n> -\t\t/* non-fatal; follow return path */\n> -\t\tbreak;\n> +\t\tdie(_(\"invalid %s: not a regular file\"), path);\n\nWould it make more sense to do 'not a regular file: %s'?\n\n> \tcase READ_GITFILE_ERR_OPEN_FAILED:\n> \t\tdie_errno(_(\"error opening '%s'\"), path);\n> \tcase READ_GITFILE_ERR_TOO_LARGE:\n> @@ -941,8 +944,14 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)\n> \tstatic struct strbuf realpath = STRBUF_INIT;\n>\n> \tif (stat(path, &st)) {\n> -\t\t/* NEEDSWORK: discern between ENOENT vs other errors */\n> -\t\terror_code = READ_GITFILE_ERR_STAT_FAILED;\n> +\t\tif (errno == ENOENT)\n> +\t\t\terror_code = READ_GITFILE_ERR_STAT_ENOENT;\n> +\t\telse\n> +\t\t\terror_code = READ_GITFILE_ERR_STAT_FAILED;\n> +\t\tgoto cleanup_return;\n> +\t}\n> +\tif (S_ISDIR(st.st_mode)) {\n> +\t\terror_code = READ_GITFILE_ERR_IS_A_DIR;\n> \t\tgoto cleanup_return;\n> \t}\n> \tif (!S_ISREG(st.st_mode)) {\n> diff --git a/setup.h b/setup.h\n> index 0738dec244..ed4b13f061 100644\n> --- a/setup.h\n> +++ b/setup.h\n> @@ -36,6 +36,8 @@ int is_nonbare_repository_dir(struct strbuf *path);\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_STAT_ENOENT 9\n> +#define READ_GITFILE_ERR_IS_A_DIR 10\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)\n> --\n> 2.43.0\n\nSo why didn't we add the tests here for the changes made?\n\nNit: I would even go further to even separate this into two commits:\n1. Split 'stat()' error into ERR_STAT_FAILED and ERR_STAT_ENOENT.\n2. Introduce 'READ_GITFILE_ERR_IS_A_DIR'.\n\nBut I'll leave that to you.\n"},{"id":"536268","messageId":"CAOLa=ZSAjDbC5bM+XvNwXW_WLWDiPfzAgaB+gHR6+DwhMW3uEw@mail.gmail.com","threadId":"64979","inReplyTo":"20260218051850.164972-3-a3205153416@gmail.com","subject":"Re: [PATCH v5 2/2] setup: allow cwd/.git to be a symlink to a directory","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2026-02-18T10:27:44Z","receivedAt":"2026-02-18T10:27:46Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Tian Yuchen writes:\n\n> Strictly enforcing 'lstat()' prevents valid '.git' symlinks.\n>\n> Switch 'setup_git_directory_gently_1()' to use 'stat()' to allow\n> filesystem resolution.\n\nBut we don't really do this no? We were calling `read_gitfile_gently()`\nbefore and continue to do so, so there was no change regards to calling\n`stat()` here. Or am I missing something?\n\n>\n> Calling the refactored 'read_gitfile_error_die()' to ensure safety:\n>\n> 1. Happy cases ('ENOENT', 'IS_A_DIR') are ignored automatically;\n> 2. Invalid types (like FIFOs and sockets) trigger 'die()' via\n> 'NOT_A_FILE'.\n>\n> Add 't/t0009-git-dir-validation.sh' to verify symlink support and FIFO\n> rejection, and register it in 't/meson.build'.\n>\n> Signed-off-by: Tian Yuchen \n> ---\n> setup.c | 18 ++++-----\n> t/meson.build | 1 +\n> t/t0009-git-dir-validation.sh | 72 +++++++++++++++++++++++++++++++++++\n> 3 files changed, 81 insertions(+), 10 deletions(-)\n> create mode 100755 t/t0009-git-dir-validation.sh\n>\n> diff --git a/setup.c b/setup.c\n> index 0ca129623e..6e6068e5eb 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -1590,17 +1590,15 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n> \t\tgitdirenv = read_gitfile_gently(dir->buf, die_on_error ?\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\nEarlier if die_on_error was false and we got any other error, let's say\nREAD_GITFILE_ERR_INVALID_FORMAT. Then we'd return\nGIT_DIR_INVALID_GITFILE from here.\n\n> -\t\t\t\t/* NEEDSWORK: fail if .git is not file nor 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);\n> -\t\t\t\t}\n> -\t\t\t} else if (error_code != READ_GITFILE_ERR_STAT_FAILED)\n> -\t\t\t\treturn GIT_DIR_INVALID_GITFILE;\n> -\t\t} else\n> +\t\t\tif (error_code)\n> +\t\t\t\tread_gitfile_error_die(error_code, dir->buf, NULL);\n> +\t\t\tif (is_git_directory(dir->buf)) {\n> +\t\t\t\tgitdirenv = DEFAULT_GIT_DIR_ENVIRONMENT;\n> +\t\t\t\tgitdir_path = xstrdup(dir->buf);\n> +\t\t\t}\n\nBut now we'd die. Correct? Doesn't that change the expected flow?\n\n> +\t\t} else {\n> \t\t\tgitfile = xstrdup(dir->buf);\n> +\t\t}\n> \t\t/*\n> \t\t * Earlier, we tentatively added DEFAULT_GIT_DIR_ENVIRONMENT\n> \t\t * to check that directory for a repository.\n\n[snip]\n"},{"id":"536270","messageId":"85978d52-7bdc-4c10-8f8e-8c4c2a804cfd@gmail.com","threadId":"64979","inReplyTo":"CAOLa=ZR_tH6A6JEj7NwziwYaVtezkHMez_cZNYyU1TQi5D8=XQ@mail.gmail.com","subject":"Re: [PATCH v5 1/2] setup: distingush ENOENT from other stat errors","fromName":"Tian Yuchen","fromEmail":"a3205153416@gmail.com","sentAt":"2026-02-18T11:11:39Z","receivedAt":"2026-02-18T11:11:42Z","isPatch":true,"sender":{"key":"cat@malon.dev","avatar":"https://avatars.githubusercontent.com/u/232002048?v=4"},"body":"Hi Karthik,\n\nThanks for the review!\n\n> Nit: For this function it should be okay to return early. But I was\n> expecting a break here, since it was using 'break' before, ideally we\n> shouldn't change it unless there is a reason to.\n\n> Would it make more sense to do 'not a regular file: %s'?\n\nPoints taken.\n\n> So why didn't we add the tests here for the changes made?\n\nI think the reason is that this commit is a refactoring of the internel \nerror handling. The actual logic change that triggers these new paths \nhappens in the next commit (2/2). Without the logic change in the next \npatch, the system behaves identically to before, doesn't it?\n\n> Nit: I would even go further to even separate this into two commits:\n> 1. Split 'stat()' error into ERR_STAT_FAILED and ERR_STAT_ENOENT.\n> 2. Introduce 'READ_GITFILE_ERR_IS_A_DIR'.\n> \n> But I'll leave that to you.\nI appreciate the suggestion. But since the changes are relatively small \nand closely related, I'll stick to keeping them in this single patch to \navoid excessive fragmentation.\n\nRegards,\n\nYuchen\n\n"},{"id":"536271","messageId":"5db39190-da1b-4807-bc2e-2ce631d7815b@gmail.com","threadId":"64979","inReplyTo":"CAOLa=ZSAjDbC5bM+XvNwXW_WLWDiPfzAgaB+gHR6+DwhMW3uEw@mail.gmail.com","subject":"Re: [PATCH v5 2/2] setup: allow cwd/.git to be a symlink to a directory","fromName":"Tian Yuchen","fromEmail":"a3205153416@gmail.com","sentAt":"2026-02-18T11:20:05Z","receivedAt":"2026-02-18T11:20:08Z","isPatch":true,"sender":{"key":"cat@malon.dev","avatar":"https://avatars.githubusercontent.com/u/232002048?v=4"},"body":"Hi Karthik,\n\n> But we don't really do this no? We were calling `read_gitfile_gently()`\n> before and continue to do so, so there was no change regards to calling\n> `stat()` here. Or am I missing something?\n\nOops, It seems I mixed up the changes in previous patches. I did make a \nmistake.\n\n> But now we'd die. Correct? Doesn't that change the expected flow?\n\nYes, this is a regression I missed. If 'die_on_error' is false, \nencountering an error like 'READ_GITFILE_ERR_INVALID_FORMAT' should \nreturn 'GIT_DIR_INVALID_GITFILE' rather than dying.\n\nSo in v6 I will ensure that we only delegate to \n'read_gitfile_error_die()' when:\n\n1. It is a happy case we want to ignore\n2. It is a security case we MUST die on;\n3. 'die_on_error' is tru\n\nOtherwise, we should fall back to returning the error code as before.\n\nThank you for your time,\n\nRegards,\n\nYuchen\n\n>> +\t\t} else {\n>> \t\t\tgitfile = xstrdup(dir->buf);\n>> +\t\t}\n>> \t\t/*\n>> \t\t * Earlier, we tentatively added DEFAULT_GIT_DIR_ENVIRONMENT\n>> \t\t * to check that directory for a repository.\n> \n> [snip]\n\n"},{"id":"536314","messageId":"xmqqy0kp7wai.fsf@gitster.g","threadId":"64979","inReplyTo":"20260218051850.164972-2-a3205153416@gmail.com","subject":"Re: [PATCH v5 1/2] setup: distingush ENOENT from other stat errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-02-18T18:15:33Z","receivedAt":"2026-02-18T18:15:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tian Yuchen writes:\n\n> Currently, 'read_gitfile_gently()' treats all 'stat()' failures as\n> generic errors. This prevents distinguishing between a missing file and\n> real errors like permission denied (fatal).\n\nThe above plan makes sense---you would split stat() error into two\ndifferent classes, start returning ERR_STAT_NOENT in addition to\nERR_STAT_FAILED, have the caller act on the new ERR_STAT_NOENT and\nadjust the way it acts on ERR_STAT_FAILED, and if possible add tests\nto make sure we react to failures from stat in an appropriate way\n(but how? --- it is where my \"if possible\" comes from). So I would\nexpect that the other patch would be to split ERR_NOT_A_FILE and add\nERR_IS_A_DIR, have the caller act on the new ERR_IS_A_DIR and adjust\nthe way it acts on ERR_NOT_A_FILE.\n\nBut then the proposed log message below says that in addition to\nNOENT, it also deals with IS_A_DIR. I do not mind doing these two\nin the same patch, but I do prefer to see each patch to be complete.\nIf a callee is changed and starts returning different return values,\nthe callers must be also adjusted to react to these new return values.\n\nLooking at the preimage of [v5 2/2], we stil check if dir->buf is a\nplain vanilla \".git\" directory only when read_gitfile_gently() returns\nERR_NOT_A_FILE with [v5 1/2] applied, but in the new world order\nwith this patch applied, shouldn't the caller deal with \".git\" when\nit gets ERR_IS_A_DIR and not ERR_NOT_A_FILE?\n\nAfter applying this patch but before applying [v5 2/2], we lose the\nability to use plain vanilla \".git\"? That is the kind of thing I\nmeant by each patch to be complete.\n\nSo, I do not understand what the splitting the topic into two along\nthis axis is trying to achieve.\n\nThanks.\n"},{"id":"536315","messageId":"xmqqtsvd7vu6.fsf@gitster.g","threadId":"64979","inReplyTo":"20260218051850.164972-3-a3205153416@gmail.com","subject":"Re: [PATCH v5 2/2] setup: allow cwd/.git to be a symlink to a directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-02-18T18:25:21Z","receivedAt":"2026-02-18T18:25:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tian Yuchen writes:\n\n> Strictly enforcing 'lstat()' prevents valid '.git' symlinks.\n\nBut nobody sane would propose running one more lstat() anyway, so\nhow is that relevant?\n\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\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);\n> -\t\t\t\t}\n> -\t\t\t} else if (error_code != READ_GITFILE_ERR_STAT_FAILED)\n> -\t\t\t\treturn GIT_DIR_INVALID_GITFILE;\n\nThe _intent_ of the original code was to\n\n * do is_git_directory() thing to deal with a plain vanilla\n \".git\" directory when read_gitfile_gently thing said \"we found\n a directory\" (NOT_A_FILE is overly coarse, which is what we\n are correcting in this topic, but the _intent_ was to do the\n is_git_directory() thing when we know it is a directory).\n\n * return INVALID_GITFILE on any error, but do not return when\n the reason why read_gitfile_gently thing failed was because\n there is no \".git\" there (again, STAT_FAILED is overly coarse,\n which is what we are correcting in this topic, but the\n _intent_ was to return INVALID thing when we not the failure\n is not due to ENOENT). Note that returning INVALID_GITFILE is\n done when die_on_error is not set.\n\n> -\t\t} else\n> +\t\t\tif (error_code)\n> +\t\t\t\tread_gitfile_error_die(error_code, dir->buf, NULL);\n\nShould this be unconditional? If our caller did not ask us to die\nupon an error with die_on_error, what happens? The original I think\nreturned INVALID_GITFILE for the caller to deal with.\n\n> +\t\t\tif (is_git_directory(dir->buf)) {\n\nShould this be unconditional? If the thing is a directory, the\noriginal would have given us NOT_A_FILE but now it would give us\nIS_A_DIR. And that is the only case original wanted to call\nis_git_directory() no?\n\n> +\t\t\t\tgitdirenv = DEFAULT_GIT_DIR_ENVIRONMENT;\n> +\t\t\t\tgitdir_path = xstrdup(dir->buf);\n> +\t\t\t}\n> +\t\t} else {\n> \t\t\tgitfile = xstrdup(dir->buf);\n> +\t\t}\n> \t\t/*\n> \t\t * Earlier, we tentatively added DEFAULT_GIT_DIR_ENVIRONMENT\n> \t\t * to check that directory for a repository.\n"},{"id":"536320","messageId":"xmqqpl617uzz.fsf@gitster.g","threadId":"64979","inReplyTo":"xmqqy0kp7wai.fsf@gitster.g","subject":"Re: [PATCH v5 1/2] setup: distingush ENOENT from other stat errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-02-18T18:43:28Z","receivedAt":"2026-02-18T18:43:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Tian Yuchen writes:\n>\n>> Currently, 'read_gitfile_gently()' treats all 'stat()' failures as\n>> generic errors. This prevents distinguishing between a missing file and\n>> real errors like permission denied (fatal).\n>\n> The above plan makes sense---you would split stat() error into two\n> different classes, start returning ERR_STAT_NOENT in addition to\n> ERR_STAT_FAILED, have the caller act on the new ERR_STAT_NOENT and\n> adjust the way it acts on ERR_STAT_FAILED, and if possible add tests\n> to make sure we react to failures from stat in an appropriate way\n> (but how? --- it is where my \"if possible\" comes from). So I would\n> expect that the other patch would be to split ERR_NOT_A_FILE and add\n> ERR_IS_A_DIR, have the caller act on the new ERR_IS_A_DIR and adjust\n> the way it acts on ERR_NOT_A_FILE.\n\nI forgot to say one thing.\n\nWhen changing the external interface for these service functions\nlike read_gitfile_gently() and read_gitfile_error_die(), we need to\nmake sure the change will not break _other_ callers of them, outside\nour main focus area. The latter, for example, has a caller in\nsubmodule.c and we need to make sure that the existing code is\nreacting to the updated definition of what ERR_STAT_FAILED and\nERR_NOT_A_FILE mean (and if not, adjust it). read_gitfile_gently()\nis used more widely outside setup.c and we need to audit these\ncallers, too.\n\n"},{"id":"536365","messageId":"e370cace-1a43-444a-a3d1-5ea35dd22e60@gmail.com","threadId":"64979","inReplyTo":"xmqqtsvd7vu6.fsf@gitster.g","subject":"Re: [PATCH v5 2/2] setup: allow cwd/.git to be a symlink to a directory","fromName":"Tian Yuchen","fromEmail":"a3205153416@gmail.com","sentAt":"2026-02-19T05:11:55Z","receivedAt":"2026-02-19T05:12:00Z","isPatch":true,"sender":{"key":"cat@malon.dev","avatar":"https://avatars.githubusercontent.com/u/232002048?v=4"},"body":"On 2/19/26 02:25, Junio C Hamano wrote:\n> Tian Yuchen writes:\n> \n>> Strictly enforcing 'lstat()' prevents valid '.git' symlinks.\n> \n> But nobody sane would propose running one more lstat() anyway, so\n> how is that relevant?\n> \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\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);\n>> -\t\t\t\t}\n>> -\t\t\t} else if (error_code != READ_GITFILE_ERR_STAT_FAILED)\n>> -\t\t\t\treturn GIT_DIR_INVALID_GITFILE;\n> \n> The _intent_ of the original code was to\n> \n> * do is_git_directory() thing to deal with a plain vanilla\n> \".git\" directory when read_gitfile_gently thing said \"we found\n> a directory\" (NOT_A_FILE is overly coarse, which is what we\n> are correcting in this topic, but the _intent_ was to do the\n> is_git_directory() thing when we know it is a directory).\n> \n> * return INVALID_GITFILE on any error, but do not return when\n> the reason why read_gitfile_gently thing failed was because\n> there is no \".git\" there (again, STAT_FAILED is overly coarse,\n> which is what we are correcting in this topic, but the\n> _intent_ was to return INVALID thing when we not the failure\n> is not due to ENOENT). Note that returning INVALID_GITFILE is\n> done when die_on_error is not set.\n> \n>> -\t\t} else\n>> +\t\t\tif (error_code)\n>> +\t\t\t\tread_gitfile_error_die(error_code, dir->buf, NULL);\n> \n> Should this be unconditional? If our caller did not ask us to die\n> upon an error with die_on_error, what happens? The original I think\n> returned INVALID_GITFILE for the caller to deal with.\n> \n>> +\t\t\tif (is_git_directory(dir->buf)) {\n> \n> Should this be unconditional? If the thing is a directory, the\n> original would have given us NOT_A_FILE but now it would give us\n> IS_A_DIR. And that is the only case original wanted to call\n> is_git_directory() no?\n> \n>> +\t\t\t\tgitdirenv = DEFAULT_GIT_DIR_ENVIRONMENT;\n>> +\t\t\t\tgitdir_path = xstrdup(dir->buf);\n>> +\t\t\t}\n>> +\t\t} else {\n>> \t\t\tgitfile = xstrdup(dir->buf);\n>> +\t\t}\n>> \t\t/*\n>> \t\t * Earlier, we tentatively added DEFAULT_GIT_DIR_ENVIRONMENT\n>> \t\t * to check that directory for a repository.\n\n\nThanks for the review.\n\nI have already sent out v6 yesterday. Here is the link:\n\nhttps://lore.kernel.org/git/20260218124638.176936-1-a3205153416@gmail.com/\n\nBased on your feedback and the changes that have been made on v6, it \nseems that the tasks that need to be completed are as follows:\n\n - Conditional 'is_git_directory' check: restrict the \n'is_git_directory()' check to only run when we explicitly get \n'READ_GITFILE_ERR_IS_A_DIR'. It makes no sense to check it for other \nerror types.\n\n - Squash two patches into one single commit, as you suggested. \n(Actually, I'm a bit confused—are you saying to “make both patches \nstandalone executable” or to “merge the two patches directly”? Either \nway, I'll go ahead and send the v7 patches first.)\n\n - Rephrase the commit message to describe the lstat() limitation, \nrather than saying 'we switch to stat()'\n\nThe holiday is over, and my efficiency in sending patches and replying \nto emails may be somewhat reduced. Please bear with me.\n\nRegards,\n\nYuchen\n"}]}