{"thread":{"id":"61632","subject":"Non-blob .gitmodules and .gitattributes","startedAt":"2024-06-14T12:03:19Z","lastAt":"2024-06-20T16:20:47Z","messageCount":17,"participants":["Alexey Pelykh","Junio C Hamano","Jeff King","Eric Sunshine"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"497172","messageId":"4F3AD9A8-DA3E-43E2-BF9A-9D7458EED7EA@gmail.com","threadId":"61632","inReplyTo":null,"subject":"Non-blob .gitmodules and .gitattributes","fromName":"Alexey Pelykh","fromEmail":"alexey.pelykh@gmail.com","sentAt":"2024-06-14T12:03:05Z","receivedAt":"2024-06-14T12:03:19Z","isPatch":false,"sender":{"key":"alexey.pelykh@gmail.com","avatar":"https://gravatar.com/avatar/cea5ae66285e0b62a4323ffe6b2d7a98cb74eaf765c06ce5e416a237e66e4f9f?d=mp&s=160"},"body":"Greetings to all!\n\nI've stumbled upon a check that ensures that .gitmodules and .gitattributes must be blobs, yet while I get why they should not be e.g. a symlink, what's the downside of permitting (and ignoring for the actual purpose) e.g. .gitattributes folder?\n\nTo better understand my use-case:\n\n$ ls -lA dev/shared\ntotal 8\ndrwxr-xr-x 3 alexey-pelykh staff 96 Jun 14 11:53 .editorconfig\ndrwxr-xr-x 3 alexey-pelykh staff 96 Jun 14 13:32 .gitattributes\ndrwxr-xr-x 14 alexey-pelykh staff 448 Jun 14 11:53 .gitignore\n-rwxr-xr-x 1 alexey-pelykh staff 613 Jun 14 13:33 share\n\nand\n\n$ ls -lA dev/shared/.*\ndev/shared/.editorconfig:\ntotal 8\n-rw-r--r-- 1 alexey-pelykh staff 377 Jun 14 11:53 shared.editorconfig\n\ndev/shared/.gitattributes:\ntotal 8\n-rw-r--r-- 1 alexey-pelykh staff 143 Jun 14 13:32 shared.gitattributes\n\ndev/shared/.gitignore:\ntotal 8\n-rw-r--r-- 1 alexey-pelykh staff 4450 Jun 14 11:53 shared.gitignore\n\nSurely, the immediate workaround is use a \"_gitattributes\" or \".gitattributes_\" name for the folder, yet if a directory with .gitignore can exist - why can't .gitmodule or .gitattributes?\n\nBe well,\nAlexey"},{"id":"497174","messageId":"xmqqbk43ilk3.fsf@gitster.g","threadId":"61632","inReplyTo":"4F3AD9A8-DA3E-43E2-BF9A-9D7458EED7EA@gmail.com","subject":"Re: Non-blob .gitmodules and .gitattributes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-14T15:35:56Z","receivedAt":"2024-06-14T15:35:59Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alexey Pelykh writes:\n\n> I've stumbled upon a check that ensures that .gitmodules and\n> .gitattributes must be blobs,\n\nAlexey, thanks for a report.\n\nFor those who want to take a look, unfortunately Alexey does not\nshow any error message or describe the end-user visible effect that\nthe \"check\" caused, so there is a bit too little to go on.\n\n> yet while I get why they should not\n> be e.g. a symlink, what's the downside of permitting (and ignoring\n> for the actual purpose) e.g. .gitattributes folder?\n\nMy knee-jerk reaction is that we probably can safely loosen by\nignoring non-blob .gitWHATEVER files, but security-minded folks may\nbe able to come up with some plausible attack scenarios if we did\nso.\n\nComments from those who have worked on transfer time and runtime\nchecks on these are highly appreciated.\n\nHaving said that, the checks for .gitmodules and .gitattributes in\nfsck.c first collect objects that tree entries with these names\npoint at into oidsets (this all happens in fsck.c:fsck_tree()), but\nthe actual check for these found objects are done only when they are\nblobs. Only when we encounter a blob object, these oidsets are\nlooked at in fsck.c:fsck_blob(), and if it is .gitmodules its\ncontents inspected (and may result in a warning or an error). So\nthe \"checks\" Alexey reports may not be in the runtime or transfer\ntime checks done in fsck but something else. I dunno.\n\nThanks.\n\n"},{"id":"497175","messageId":"8E37AD78-C480-4838-9F69-3E7FC209A9FC@gmail.com","threadId":"61632","inReplyTo":"xmqqbk43ilk3.fsf@gitster.g","subject":"Re: Non-blob .gitmodules and .gitattributes","fromName":"Alexey Pelykh","fromEmail":"alexey.pelykh@gmail.com","sentAt":"2024-06-14T15:43:39Z","receivedAt":"2024-06-14T15:43:52Z","isPatch":false,"sender":{"key":"alexey.pelykh@gmail.com","avatar":"https://gravatar.com/avatar/cea5ae66285e0b62a4323ffe6b2d7a98cb74eaf765c06ce5e416a237e66e4f9f?d=mp&s=160"},"body":"Dear Junio,\n\nThanks for providing more insight!\n\nThe error I'm seeing due to \".gitattributes\" folder being present in the diff via the \".gitattributes/shared.gitattributes\" file is the following:\n\nerror in tree f8db7c96f9daf3ea8486804a3f717a807fc1a1d8: gitattributesBlob: non-blob found at .gitattributes\n\nI'm seeing this on both \"git push\" and \"git fsck\". Steps to reproduce:\n$ mkdir test\n$ cd test\n$ git init\n$ mkdir -p subdir/.gitattributes\n$ touch subdir/.gitattributes/some-file\n$ git add touch subdir/.gitattributes/some-file\n$ git commit -m \"test\"\n\n$ git fsck\nChecking object directories: 100% (256/256), done.\nerror in tree 3666f1677ba5c7ec7e69544510a0d8b99a71774a: gitattributesBlob: non-blob found at .gitattributes\n\ngit-push will give the same, at least when pushing to GitHub.\n\nKind regards,\nAlexey\n\n> On 14 Jun 2024, at 17:35, Junio C Hamano wrote:\n> \n> Alexey Pelykh writes:\n> \n>> I've stumbled upon a check that ensures that .gitmodules and\n>> .gitattributes must be blobs,\n> \n> Alexey, thanks for a report.\n> \n> For those who want to take a look, unfortunately Alexey does not\n> show any error message or describe the end-user visible effect that\n> the \"check\" caused, so there is a bit too little to go on.\n> \n>> yet while I get why they should not\n>> be e.g. a symlink, what's the downside of permitting (and ignoring\n>> for the actual purpose) e.g. .gitattributes folder?\n> \n> My knee-jerk reaction is that we probably can safely loosen by\n> ignoring non-blob .gitWHATEVER files, but security-minded folks may\n> be able to come up with some plausible attack scenarios if we did\n> so.\n> \n> Comments from those who have worked on transfer time and runtime\n> checks on these are highly appreciated.\n> \n> Having said that, the checks for .gitmodules and .gitattributes in\n> fsck.c first collect objects that tree entries with these names\n> point at into oidsets (this all happens in fsck.c:fsck_tree()), but\n> the actual check for these found objects are done only when they are\n> blobs. Only when we encounter a blob object, these oidsets are\n> looked at in fsck.c:fsck_blob(), and if it is .gitmodules its\n> contents inspected (and may result in a warning or an error). So\n> the \"checks\" Alexey reports may not be in the runtime or transfer\n> time checks done in fsck but something else. I dunno.\n> \n> Thanks.\n> \n\n"},{"id":"497297","messageId":"20240618183156.GB178291@coredump.intra.peff.net","threadId":"61632","inReplyTo":"xmqqbk43ilk3.fsf@gitster.g","subject":"Re: Non-blob .gitmodules and .gitattributes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-18T18:31:56Z","receivedAt":"2024-06-18T18:31:58Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 14, 2024 at 08:35:56AM -0700, Junio C Hamano wrote:\n\n> My knee-jerk reaction is that we probably can safely loosen by\n> ignoring non-blob .gitWHATEVER files, but security-minded folks may\n> be able to come up with some plausible attack scenarios if we did\n> so.\n> \n> Comments from those who have worked on transfer time and runtime\n> checks on these are highly appreciated.\n\nI can't think of an immediate problem security-wise, since we know that\nGit won't actually read the trees. In fact, I was a little surprised\nthat normal Git commands outside of fsck did not run into problems with\na .gitattributes directory. What happens on Linux, at least, is that we\ntry to fopen() the directory, which works, and then read() returns\nEISDIR. But because we do so through strbuf_getline(), it just looks\nlike EOF and we don't complain. Arguably that code (and all the other\nloops like it) should check ferror() and complain.\n\nBut I'd also suspect that other non-POSIX platforms would see the error\nat open(), and _would_ actually produce an error message. I seem to\nrecall running into this before with Windows, maybe?\n\nSo even if we loosened fsck, I'm not sure if it's something we want to\nsupport. And of course my bigger question is: why? These are reserved\nnames that have special meaning to Git. Sticking stuff that Git doesn't\nunderstand and may produce errors for seems odd.\n\n> Having said that, the checks for .gitmodules and .gitattributes in\n> fsck.c first collect objects that tree entries with these names\n> point at into oidsets (this all happens in fsck.c:fsck_tree()), but\n> the actual check for these found objects are done only when they are\n> blobs. Only when we encounter a blob object, these oidsets are\n> looked at in fsck.c:fsck_blob(), and if it is .gitmodules its\n> contents inspected (and may result in a warning or an error). So\n> the \"checks\" Alexey reports may not be in the runtime or transfer\n> time checks done in fsck but something else. I dunno.\n\nThe fsck checks will kick in. When we fsck the containing tree, we learn\nthat some oid X is a .gitmodules file, and queue that. The hope is that\nlater we're fsck-ing X anyway, and we get to validate for free-ish. But\nif we _don't_ see it (either we checked the blob before the tree, or\nperhaps the blob is not even part of the set of objects we're checking),\nthen at the end we validate any queued items that are left.\n\nWe have to do it this way to catch the case of somebody pushing up blob\nX in one push (perhaps with the name \"foo\"), and then doing a second\npush that references it with a new name (i.e., renaming \"foo\" to\n\".gitmodules\").\n\n-Peff\n"},{"id":"497298","messageId":"14349714-F8D1-4F7B-BB13-4FA39C6819DE@gmail.com","threadId":"61632","inReplyTo":"20240618183156.GB178291@coredump.intra.peff.net","subject":"Re: Non-blob .gitmodules and .gitattributes","fromName":"Alexey Pelykh","fromEmail":"alexey.pelykh@gmail.com","sentAt":"2024-06-18T19:07:24Z","receivedAt":"2024-06-18T19:07:38Z","isPatch":false,"sender":{"key":"alexey.pelykh@gmail.com","avatar":"https://gravatar.com/avatar/cea5ae66285e0b62a4323ffe6b2d7a98cb74eaf765c06ce5e416a237e66e4f9f?d=mp&s=160"},"body":"Hi Jeff,\n\n> But I'd also suspect that other non-POSIX platforms would see the error\n> at open(), and _would_ actually produce an error message. I seem to\n> recall running into this before with Windows, maybe?\n\nIs there an easy way to verify \"what on Windows\" part? I'd be happy to\nhelp, yet I'm not sure I got it right what to look for.\n\n> So even if we loosened fsck, I'm not sure if it's something we want to\n> support. And of course my bigger question is: why? These are reserved\n> names that have special meaning to Git. Sticking stuff that Git doesn't\n> understand and may produce errors for seems odd.\n\nSurely, reserved names are reserved for some reason. If there's a legit\nreason alike cost-to-support, having an objection would be dumb. Yet\nif supporting would turn out to be of an effort alike dropping the check\nthen it would seem having that check brings no value.\n\nWith that said, if those are reserved names, why .gitignore is not reserved?\nFor consistency at least.\n\nCheers,\nAlexey\n\n> On 18 Jun 2024, at 20:31, Jeff King wrote:\n> \n> On Fri, Jun 14, 2024 at 08:35:56AM -0700, Junio C Hamano wrote:\n> \n>> My knee-jerk reaction is that we probably can safely loosen by\n>> ignoring non-blob .gitWHATEVER files, but security-minded folks may\n>> be able to come up with some plausible attack scenarios if we did\n>> so.\n>> \n>> Comments from those who have worked on transfer time and runtime\n>> checks on these are highly appreciated.\n> \n> I can't think of an immediate problem security-wise, since we know that\n> Git won't actually read the trees. In fact, I was a little surprised\n> that normal Git commands outside of fsck did not run into problems with\n> a .gitattributes directory. What happens on Linux, at least, is that we\n> try to fopen() the directory, which works, and then read() returns\n> EISDIR. But because we do so through strbuf_getline(), it just looks\n> like EOF and we don't complain. Arguably that code (and all the other\n> loops like it) should check ferror() and complain.\n> \n> But I'd also suspect that other non-POSIX platforms would see the error\n> at open(), and _would_ actually produce an error message. I seem to\n> recall running into this before with Windows, maybe?\n> \n> So even if we loosened fsck, I'm not sure if it's something we want to\n> support. And of course my bigger question is: why? These are reserved\n> names that have special meaning to Git. Sticking stuff that Git doesn't\n> understand and may produce errors for seems odd.\n> \n>> Having said that, the checks for .gitmodules and .gitattributes in\n>> fsck.c first collect objects that tree entries with these names\n>> point at into oidsets (this all happens in fsck.c:fsck_tree()), but\n>> the actual check for these found objects are done only when they are\n>> blobs. Only when we encounter a blob object, these oidsets are\n>> looked at in fsck.c:fsck_blob(), and if it is .gitmodules its\n>> contents inspected (and may result in a warning or an error). So\n>> the \"checks\" Alexey reports may not be in the runtime or transfer\n>> time checks done in fsck but something else. I dunno.\n> \n> The fsck checks will kick in. When we fsck the containing tree, we learn\n> that some oid X is a .gitmodules file, and queue that. The hope is that\n> later we're fsck-ing X anyway, and we get to validate for free-ish. But\n> if we _don't_ see it (either we checked the blob before the tree, or\n> perhaps the blob is not even part of the set of objects we're checking),\n> then at the end we validate any queued items that are left.\n> \n> We have to do it this way to catch the case of somebody pushing up blob\n> X in one push (perhaps with the name \"foo\"), and then doing a second\n> push that references it with a new name (i.e., renaming \"foo\" to\n> \".gitmodules\").\n> \n> -Peff\n\n"},{"id":"497299","messageId":"xmqqa5jiypmm.fsf@gitster.g","threadId":"61632","inReplyTo":"14349714-F8D1-4F7B-BB13-4FA39C6819DE@gmail.com","subject":"Re: Non-blob .gitmodules and .gitattributes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-18T20:14:57Z","receivedAt":"2024-06-18T20:15:11Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alexey Pelykh writes:\n\n>> But I'd also suspect that other non-POSIX platforms would see the error\n>> at open(), and _would_ actually produce an error message. I seem to\n>> recall running into this before with Windows, maybe?\n>\n> Is there an easy way to verify \"what on Windows\" part? I'd be happy to\n> help, yet I'm not sure I got it right what to look for.\n\nCreate a .gitmodules and .gitattributes directory, \"git add\" it, and\nperform various operations that want to read them, probably. Even a\nsimple \"git diff\" should try to consult the attribute system (e.g.\nbecause it wants to know if a path needs use custom function header\nregexp pattern). As Peff said, on Linux and probably on macOS, we\nwill silently ignore such .gitattributes and that is what we want.\nOn Windows we may see \"cannot open\" error reported and visible to\nthe users.\n\n> Surely, reserved names are reserved for some reason. If there's a legit\n> reason alike cost-to-support, having an objection would be dumb. Yet\n> if supporting would turn out to be of an effort alike dropping the check\n> then it would seem having that check brings no value.\n>\n> With that said, if those are reserved names, why .gitignore is not reserved?\n> For consistency at least.\n\nWe do not bother with \".gitignore\" since we see no security\nimplications in that file. But other two whose name begin with\n\".git\" do have some security implications (actually .gitattributes\nis designed not to have any, but .gitmodules certainly does), so\nwe choose to inspect their sizes, contents, and types.\n\nBut that raises a few more questions.\n\nWhat to do about our future needs that may conflict the needs of\nusers who want to use names of their choice? If we declare that\nany name that begin with \".git\" is reserved, there will be fewer\nissues, but do we want to and can we afford to?\n\nAlso what if we later find security implications in \".gitignore\"\nfiles and decide to inspect their sizes, contents, and types? The\ncurrent system not issuing a warning does not give users any\nguarantee that the future systems won't (even though we try hard to\navoid introducing such backward incompatible changes, we are human,\ntoo, and we sometimes screw up).\n\nThanks.\n"},{"id":"497303","messageId":"20240618233303.GA188880@coredump.intra.peff.net","threadId":"61632","inReplyTo":"xmqqa5jiypmm.fsf@gitster.g","subject":"Re: Non-blob .gitmodules and .gitattributes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-18T23:33:03Z","receivedAt":"2024-06-18T23:33:05Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 18, 2024 at 01:14:57PM -0700, Junio C Hamano wrote:\n\n> Alexey Pelykh writes:\n> \n> >> But I'd also suspect that other non-POSIX platforms would see the error\n> >> at open(), and _would_ actually produce an error message. I seem to\n> >> recall running into this before with Windows, maybe?\n> >\n> > Is there an easy way to verify \"what on Windows\" part? I'd be happy to\n> > help, yet I'm not sure I got it right what to look for.\n> \n> Create a .gitmodules and .gitattributes directory, \"git add\" it, and\n> perform various operations that want to read them, probably. Even a\n> simple \"git diff\" should try to consult the attribute system (e.g.\n> because it wants to know if a path needs use custom function header\n> regexp pattern). As Peff said, on Linux and probably on macOS, we\n> will silently ignore such .gitattributes and that is what we want.\n> On Windows we may see \"cannot open\" error reported and visible to\n> the users.\n\nI don't think you even need to \"git add\" it. Just making it in the\nworking tree is enough for us to look at it. You probably do need an\nactual change in a tracked file for \"git diff\" to bother looking up\nattributes (an alternative is just \"git show\" on an existing commit).\n\nInterestingly, a directory .gitmodules _does_ complain:\n\n $ git status\n warning: unable to access '/home/peff/tmp/.gitmodules': Is a directory\n On branch main\n nothing to commit, working tree clean\n\nThat's because it uses fopen(), rather than open(). And we have a\nwrapper that complains about directories there, triggered by the\nFREAD_READS_DIRECTORIES knob in the Makefile. It's set for Linux, but\nnot for Windows (which is why I suspect Windows might just complain by\nitself). We don't see that for \".gitattributes\" or \".gitignore\" because\nthose use a raw open() syscall, and Linux is happy to allow it. Arguably\nthat's a bug, as the intent of that knob is that we'd notice and avoid\ntrying to read from directories.\n\nSo even putting fsck warnings aside, I think it's mostly luck that\nhaving .git* files as directories doesn't produce noisy warnings during\nnormal use.\n\n-Peff\n"},{"id":"497306","messageId":"20240618234436.4107855-1-gitster@pobox.com","threadId":"61632","inReplyTo":"20240618233303.GA188880@coredump.intra.peff.net","subject":"[PATCH 0/4] .git{ignore,attributes} directories?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-18T23:44:31Z","receivedAt":"2024-06-18T23:44:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"We seem to blindly open .gitignore that is a directory and rely on\nsubsequent read() to be more-or-less silent to implement a \"non-file\n.gitignore is silently ignored\" behaviour.\n\nLet's be a bit more strict in detecting and reporting I/O errors,\nand also stop reading from directories.\n\nI think the first three are reasonable changes, but the last one is\nof dubious value.\n\nJunio C Hamano (4):\n .gitignore: introduce GITIGNORE_FILE CPP macro\n attr: notice and report read failure\n exclude: notice and report read failure of .gitignore files\n submodule: ignore .gitmodules that is not a regular file\n\n attr.c | 10 +++++++++-\n builtin/read-tree.c | 3 ++-\n dir.c | 16 ++++++++++++++--\n dir.h | 1 +\n environment.h | 1 +\n submodule-config.c | 2 +-\n t/t0003-attributes.sh | 9 +++++++++\n t/t0008-ignores.sh | 18 ++++++++++++++++++\n 8 files changed, 55 insertions(+), 5 deletions(-)\n\n-- \n2.45.2-711-gd2c001ca14\n\n"},{"id":"497307","messageId":"20240618234436.4107855-2-gitster@pobox.com","threadId":"61632","inReplyTo":"20240618234436.4107855-1-gitster@pobox.com","subject":"[PATCH 1/4] .gitignore: introduce GITIGNORE_FILE CPP macro","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-18T23:44:32Z","receivedAt":"2024-06-18T23:44:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"GITATTRIBUTES_FILE and GITMODULES_FILE have their own CPP macros\nthat resolve to their string literals to help compilers catch typos.\nAdd one for \".gitignore\" as well.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/read-tree.c | 3 ++-\n dir.c | 4 ++--\n environment.h | 1 +\n 3 files changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/read-tree.c b/builtin/read-tree.c\nindex a8cf8504b8..bc12f5bd16 100644\n--- a/builtin/read-tree.c\n+++ b/builtin/read-tree.c\n@@ -6,6 +6,7 @@\n \n #include \"builtin.h\"\n #include \"config.h\"\n+#include \"environment.h\"\n #include \"gettext.h\"\n #include \"hex.h\"\n #include \"lockfile.h\"\n@@ -65,7 +66,7 @@ static int exclude_per_directory_cb(const struct option *opt, const char *arg,\n \n \tif (!opts->update)\n \t\tdie(\"--exclude-per-directory is meaningless unless -u\");\n-\tif (strcmp(arg, \".gitignore\"))\n+\tif (strcmp(arg, GITIGNORE_FILE))\n \t\tdie(\"--exclude-per-directory argument must be .gitignore\");\n \treturn 0;\n }\ndiff --git a/dir.c b/dir.c\nindex 45be4ad261..ec875e3878 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -2888,7 +2888,7 @@ static void new_untracked_cache(struct index_state *istate, int flags)\n {\n \tstruct untracked_cache *uc = xcalloc(1, sizeof(*uc));\n \tstrbuf_init(&uc->ident, 100);\n-\tuc->exclude_per_dir = \".gitignore\";\n+\tuc->exclude_per_dir = GITIGNORE_FILE;\n \tuc->dir_flags = flags >= 0 ? flags : new_untracked_cache_flags(istate);\n \tset_untracked_ident(uc);\n \tistate->untracked = uc;\n@@ -3428,7 +3428,7 @@ static GIT_PATH_FUNC(git_path_info_exclude, \"info/exclude\")\n \n void setup_standard_excludes(struct dir_struct *dir)\n {\n-\tdir->exclude_per_dir = \".gitignore\";\n+\tdir->exclude_per_dir = GITIGNORE_FILE;\n \n \t/* core.excludesfile defaulting to $XDG_CONFIG_HOME/git/ignore */\n \tif (!excludes_file)\ndiff --git a/environment.h b/environment.h\nindex e9f01d4d11..39c3c24a3f 100644\n--- a/environment.h\n+++ b/environment.h\n@@ -43,6 +43,7 @@ const char *getenv_safe(struct strvec *argv, const char *name);\n #define GITMODULES_FILE \".gitmodules\"\n #define GITMODULES_INDEX \":.gitmodules\"\n #define GITMODULES_HEAD \"HEAD:.gitmodules\"\n+#define GITIGNORE_FILE \".gitignore\"\n #define GIT_NOTES_REF_ENVIRONMENT \"GIT_NOTES_REF\"\n #define GIT_NOTES_DEFAULT_REF \"refs/notes/commits\"\n #define GIT_NOTES_DISPLAY_REF_ENVIRONMENT \"GIT_NOTES_DISPLAY_REF\"\n-- \n2.45.2-711-gd2c001ca14\n\n"},{"id":"497308","messageId":"20240618234436.4107855-3-gitster@pobox.com","threadId":"61632","inReplyTo":"20240618234436.4107855-1-gitster@pobox.com","subject":"[PATCH 2/4] attr: notice and report read failure of .gitattributes files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-18T23:44:33Z","receivedAt":"2024-06-18T23:44:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The code that reads .gitattributes files was careless in dealing in\nunusual circumstances.\n\n - It let read errors silently ignored.\n\n - It tried to read \".gitattributes\" that is a directory on\n platforms that allowed open(2) to open directories.\n\nTo make the latter consistent with what we do for fopen() on\ndirectories (\"git grep\" for FREAD_READS_DIRECTORIES for details),\ncheck if we opened a directory, silently close it and return\nsuccess. Catch all read errors before we close and report as\nneeded.\n\nSigned-off-by: Junio C Hamano \n---\n attr.c | 10 +++++++++-\n t/t0003-attributes.sh | 9 +++++++++\n 2 files changed, 18 insertions(+), 1 deletion(-)\n\ndiff --git a/attr.c b/attr.c\nindex 300f994ba6..9ab9cf1551 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -747,6 +747,11 @@ static struct attr_stack *read_attr_from_file(const char *path, unsigned flags)\n \t\tfclose(fp);\n \t\treturn NULL;\n \t}\n+\tif (S_ISDIR(st.st_mode)) {\n+\t\t/* On FREAD_READS_DIRECTORIES platforms */\n+\t\tfclose(fp);\n+\t\treturn NULL;\n+\t}\n \tif (st.st_size >= ATTR_MAX_FILE_SIZE) {\n \t\twarning(_(\"ignoring overly large gitattributes file '%s'\"), path);\n \t\tfclose(fp);\n@@ -760,7 +765,10 @@ static struct attr_stack *read_attr_from_file(const char *path, unsigned flags)\n \t\thandle_attr_line(res, buf.buf, path, ++lineno, flags);\n \t}\n \n-\tfclose(fp);\n+\tif (ferror(fp))\n+\t\twarning_errno(_(\"failed while reading gitattributes file '%s'\"), path);\n+\tif (fclose(fp))\n+\t\twarning_errno(_(\"cannot fclose gitattributes file '%s'\"), path);\n \tstrbuf_release(&buf);\n \treturn res;\n }\ndiff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\nindex 66ccb5889d..783c20146d 100755\n--- a/t/t0003-attributes.sh\n+++ b/t/t0003-attributes.sh\n@@ -603,6 +603,15 @@ test_expect_success EXPENSIVE 'large attributes blob ignored' '\n \ttest_cmp expect err\n '\n \n+test_expect_success '.gitattribute directory behaves as if it does not exist' '\n+\ttest_when_finished \"rm -fr dir\" &&\n+\tmkdir -p dir/.gitattributes &&\n+\t>dir/ectory &&\n+\tgit -C dir check-attr --all ectory >out 2>err &&\n+\ttest_grep ! \"failed while reading\" err &&\n+\ttest_grep ! \"cannot fclose\" err\n+'\n+\n test_expect_success 'builtin object mode attributes work (dir and regular paths)' '\n \t>normal &&\n \tattr_check_object_mode normal 100644 &&\n-- \n2.45.2-711-gd2c001ca14\n\n"},{"id":"497309","messageId":"20240618234436.4107855-5-gitster@pobox.com","threadId":"61632","inReplyTo":"20240618234436.4107855-1-gitster@pobox.com","subject":"[PATCH 4/4] submodule: ignore .gitmodules that is not a regular file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-18T23:44:35Z","receivedAt":"2024-06-18T23:44:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"If .gitmodules exists in the working tree but is a directory, it\nwould have just tried to use it as if it were a file. On a platform\nthat needs FREAD_READS_DIRECTORIES, this would have been hidden by\nour own fopen() that pretends as if directory did not exist, so it\nis a no-op. Just to add some documentation value, make sure we\ncheck with file_exists_as_file() instead of file_exists(), the\nlatter of which will be happy as long as the given path exists no\nmatter what it is.\n\nSigned-off-by: Junio C Hamano \n---\n dir.c | 6 ++++++\n dir.h | 1 +\n submodule-config.c | 2 +-\n 3 files changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/dir.c b/dir.c\nindex 73f89f4d8c..d943da93df 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -3142,6 +3142,12 @@ int file_exists(const char *f)\n \treturn lstat(f, &sb) == 0;\n }\n \n+int file_exists_as_file(const char *path)\n+{\n+\tstruct stat st;\n+\treturn lstat(path, &st) == 0 && S_ISREG(st.st_mode);\n+}\n+\n int repo_file_exists(struct repository *repo, const char *path)\n {\n \tif (repo != the_repository)\ndiff --git a/dir.h b/dir.h\nindex 1398a53fb4..3612dbbf9e 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -475,6 +475,7 @@ void dir_clear(struct dir_struct *dir);\n \n int repo_file_exists(struct repository *repo, const char *path);\n int file_exists(const char *);\n+int file_exists_as_file(const char *);\n \n int is_inside_dir(const char *dir);\n int dir_inside_of(const char *subdir, const char *dir);\ndiff --git a/submodule-config.c b/submodule-config.c\nindex ec45ea67b9..6c18ae3764 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -801,7 +801,7 @@ static void config_from_gitmodules(config_fn_t fn, struct repository *repo, void\n \t\tchar *oidstr = NULL;\n \n \t\tfile = repo_worktree_path(repo, GITMODULES_FILE);\n-\t\tif (file_exists(file)) {\n+\t\tif (file_exists_as_file(file)) {\n \t\t\tconfig_source.file = file;\n \t\t} else if (repo_get_oid(repo, GITMODULES_INDEX, &oid) >= 0 ||\n \t\t\t repo_get_oid(repo, GITMODULES_HEAD, &oid) >= 0) {\n-- \n2.45.2-711-gd2c001ca14\n\n"},{"id":"497310","messageId":"20240618234436.4107855-4-gitster@pobox.com","threadId":"61632","inReplyTo":"20240618234436.4107855-1-gitster@pobox.com","subject":"[PATCH 3/4] exclude: notice and report read failure of .gitignore files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-18T23:44:34Z","receivedAt":"2024-06-18T23:44:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The code that reads .gitignore files was careless in dealing in\nunusual circumstances.\n\n - It let read errors silently ignored.\n\n - It tried to read \".gitignore\" that is a directory on platforms\n that allow open(2) to open directories.\n\nTo make the latter consistent with what we do for fopen() on\ndirectories (\"git grep\" for FREAD_READS_DIRECTORIES for details),\ncheck if we opened a directory, silently close it and return\nsuccess. Catch all read errors before we close and report as\nneeded.\n\nSigned-off-by: Junio C Hamano \n---\n dir.c | 6 ++++++\n t/t0008-ignores.sh | 18 ++++++++++++++++++\n 2 files changed, 24 insertions(+)\n\ndiff --git a/dir.c b/dir.c\nindex ec875e3878..73f89f4d8c 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1127,6 +1127,10 @@ static int add_patterns(const char *fname, const char *base, int baselen,\n \t\t\t\t\t\t oid_stat);\n \t\tif (r != 1)\n \t\t\treturn r;\n+\t} else if (S_ISDIR(st.st_mode)) {\n+\t\t/* On FREAD_READS_DIRECTORIES platforms */\n+\t\tclose(fd);\n+\t\treturn 0;\n \t} else {\n \t\tsize = xsize_t(st.st_size);\n \t\tif (size == 0) {\n@@ -1140,6 +1144,8 @@ static int add_patterns(const char *fname, const char *base, int baselen,\n \t\t}\n \t\tbuf = xmallocz(size);\n \t\tif (read_in_full(fd, buf, size) != size) {\n+\t\t\twarning_errno(_(\"failed while reading gitignore file '%s'\"),\n+\t\t\t\t fname);\n \t\t\tfree(buf);\n \t\t\tclose(fd);\n \t\t\treturn -1;\ndiff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh\nindex 02a18d4fdb..c367824c66 100755\n--- a/t/t0008-ignores.sh\n+++ b/t/t0008-ignores.sh\n@@ -953,4 +953,22 @@ test_expect_success EXPENSIVE 'large exclude file ignored in tree' '\n \ttest_cmp expect err\n '\n \n+test_expect_success POSIXPERM 'unreadable exclude file reported' '\n+\ttest_when_finished \"rm -f .gitignore\" &&\n+\t>.gitignore &&\n+\tchmod a= .gitignore &&\n+\t# we do not care if the pattern matches\n+\t{ git check-ignore xyzzy 2>err || :; } &&\n+\ttest_grep \"unable to access ${SQ}\\.gitignore${SQ}:\" err\n+'\n+\n+test_expect_success '.gitignore directory ignored' '\n+\ttest_when_finished \"rm -rf .gitignore\" &&\n+\trm -f .gitignore &&\n+\tmkdir .gitignore &&\n+\t# we do not care if the pattern matches\n+\t{ git check-ignore xyzzy 2>err || :; } &&\n+\ttest_grep ! \"failed while reading gitignore file ${SQ}\\.gitignore${SQ}:\" err\n+'\n+\n test_done\n-- \n2.45.2-711-gd2c001ca14\n\n"},{"id":"497311","messageId":"CAPig+cQy2e54Pjiu=7xw7Ox=OyxmFuoW6MU31JXXuXobQ_vvXA@mail.gmail.com","threadId":"61632","inReplyTo":"20240618234436.4107855-3-gitster@pobox.com","subject":"Re: [PATCH 2/4] attr: notice and report read failure of .gitattributes files","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-06-19T00:21:07Z","receivedAt":"2024-06-19T00:21:19Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jun 18, 2024 at 7:46 PM Junio C Hamano wrote:\n> The code that reads .gitattributes files was careless in dealing in\n> unusual circumstances.\n>\n> - It let read errors silently ignored.\n\nECANTPARSE: perhaps? s/errors/& be/\n\n> - It tried to read \".gitattributes\" that is a directory on\n> platforms that allowed open(2) to open directories.\n>\n> To make the latter consistent with what we do for fopen() on\n> directories (\"git grep\" for FREAD_READS_DIRECTORIES for details),\n> check if we opened a directory, silently close it and return\n> success. Catch all read errors before we close and report as\n> needed.\n>\n> Signed-off-by: Junio C Hamano \n"},{"id":"497312","messageId":"xmqqplsdybkh.fsf@gitster.g","threadId":"61632","inReplyTo":"CAPig+cQy2e54Pjiu=7xw7Ox=OyxmFuoW6MU31JXXuXobQ_vvXA@mail.gmail.com","subject":"Re: [PATCH 2/4] attr: notice and report read failure of .gitattributes files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-19T01:18:38Z","receivedAt":"2024-06-19T01:18:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> ECANTPARSE: perhaps? s/errors/& be/\n\nYou're right. Thanks.\n"},{"id":"497313","messageId":"CAPig+cSHYFbTAf_XMucaj_+=84pc+OiBVnyRXogOGTUYfZHFEQ@mail.gmail.com","threadId":"61632","inReplyTo":"xmqqplsdybkh.fsf@gitster.g","subject":"Re: [PATCH 2/4] attr: notice and report read failure of .gitattributes files","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-06-19T02:35:18Z","receivedAt":"2024-06-19T02:35:30Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jun 18, 2024 at 9:18 PM Junio C Hamano wrote:\n> Eric Sunshine writes:\n> > ECANTPARSE: perhaps? s/errors/& be/\n>\n> You're right. Thanks.\n\nThe same problem occurs in the commit message of patch [3/4].\n"},{"id":"497353","messageId":"20240619135754.GB943023@coredump.intra.peff.net","threadId":"61632","inReplyTo":"20240618234436.4107855-3-gitster@pobox.com","subject":"Re: [PATCH 2/4] attr: notice and report read failure of .gitattributes files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-19T13:57:54Z","receivedAt":"2024-06-19T13:57:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 18, 2024 at 04:44:33PM -0700, Junio C Hamano wrote:\n\n> The code that reads .gitattributes files was careless in dealing in\n> unusual circumstances.\n> \n> - It let read errors silently ignored.\n> \n> - It tried to read \".gitattributes\" that is a directory on\n> platforms that allowed open(2) to open directories.\n> \n> To make the latter consistent with what we do for fopen() on\n> directories (\"git grep\" for FREAD_READS_DIRECTORIES for details),\n> check if we opened a directory, silently close it and return\n> success. Catch all read errors before we close and report as\n> needed.\n\nMakes sense, but...\n\n> diff --git a/attr.c b/attr.c\n> index 300f994ba6..9ab9cf1551 100644\n> --- a/attr.c\n> +++ b/attr.c\n> @@ -747,6 +747,11 @@ static struct attr_stack *read_attr_from_file(const char *path, unsigned flags)\n> \t\tfclose(fp);\n> \t\treturn NULL;\n> \t}\n> +\tif (S_ISDIR(st.st_mode)) {\n> +\t\t/* On FREAD_READS_DIRECTORIES platforms */\n> +\t\tfclose(fp);\n> +\t\treturn NULL;\n> +\t}\n\nI don't know that this is really consistent with callers of fopen(),\nsince they tend to complain noisily. Usually via warn_on_fopen_errors(),\nwhich we ourselves call above.\n\nI.e., if we were using fopen() here rather than open()+fdopen(), our\ncall would likewise complain noisily. And we even did so prior to\n2ef579e261 (attr: do not respect symlinks for in-tree .gitattributes,\n2021-02-16)! We had to switch to open() then to use O_NOFOLLOW.\n\nAnd I notice that every caller of open_nofollow() does an fdopen()\nanyway. So I wonder if the better solution would be to make an\nfopen_nofollow() that handles the FREAD_READS_DIRECTORIES stuff itself,\njust like fopen() does?\n\n\nThat is also an interesting data point regarding the \"is it sane to have\ndirectory .gitattributes\" question. Older versions would definitely\ncomplain about it (once per file in the diff even):\n\n $ mkdir .gitattributes\n $ git.v2.31.0 show >/dev/null\n warning: unable to access '.gitattributes': Is a directory\n warning: unable to access '.gitattributes': Is a directory\n\n-Peff\n"},{"id":"497411","messageId":"xmqqjzijy4a0.fsf@gitster.g","threadId":"61632","inReplyTo":"20240619135754.GB943023@coredump.intra.peff.net","subject":"Re: [PATCH 2/4] attr: notice and report read failure of .gitattributes files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-20T16:20:39Z","receivedAt":"2024-06-20T16:20:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I don't know that this is really consistent with callers of fopen(),\n> since they tend to complain noisily. Usually via warn_on_fopen_errors(),\n> which we ourselves call above.\n\nArgh, you're right. I thought the guard in warn_on_fopen_errors()\nwould catch the artificial error code we give when we found that we\nopened a directory (by mistake), but the error we use is EISDIR, so\nit will be shown.\n\nOK, let's scrap the whole thing. It does not look like it is\nsolving any real-world problems.\n\nThanks.\n\n"}]}