{"thread":{"id":"63386","subject":"[PATCH] wrapper: Fix a errno discrepancy on NetBSD.","startedAt":"2025-05-02T23:34:19Z","lastAt":"2025-05-06T22:58:11Z","messageCount":25,"participants":["Collin Funk","brian m. carlson","Junio C Hamano","Jeff King","shejialuo","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"517142","messageId":"20250502233403.289761-1-collin.funk1@gmail.com","threadId":"63386","inReplyTo":null,"subject":"[PATCH] wrapper: Fix a errno discrepancy on NetBSD.","fromName":"Collin Funk","fromEmail":"collin.funk1@gmail.com","sentAt":"2025-05-02T23:33:32Z","receivedAt":"2025-05-02T23:34:19Z","isPatch":true,"sender":{"key":"collin.funk1@gmail.com","avatar":"https://avatars.githubusercontent.com/u/65689063?v=4"},"body":"As documented on NetBSD's man page, open with the O_NOFOLLOW flag and a\nsymlink returns -1 and sets errno to EFTYPE which differs from POSIX.\nThis patch fixes the following test failure:\n\n$ sh t0602-reffiles-fsck.sh --verbose\n--- expect\t2025-05-02 23:05:23.920890147 +0000\n+++ err\t2025-05-02 23:05:23.916794959 +0000\n@@ -1 +1 @@\n-error: packed-refs: badRefFiletype: not a regular file but a symlink\n+error: unable to open '.git/packed-refs': Inappropriate file type or format\nnot ok 12 - the filetype of packed-refs should be checked\n\nThis portability issue was introduced in Commit\ncfea2f2da8 (packed-backend: check whether the \"packed-refs\" is regular file, 2025-02-28)\n\nSigned-off-by: Collin Funk <collin.funk1@gmail.com>\n---\n wrapper.c | 14 +++++++++++++-\n 1 file changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git a/wrapper.c b/wrapper.c\nindex 3c79778055..4d448d7c57 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -737,7 +737,19 @@ int is_empty_or_missing_file(const char *filename)\n int open_nofollow(const char *path, int flags)\n {\n #ifdef O_NOFOLLOW\n-\treturn open(path, flags | O_NOFOLLOW);\n+\tint ret = open(path, flags | O_NOFOLLOW);\n+#ifdef __NetBSD__\n+\t/*\n+\t * NetBSD sets errno to EFTYPE when path is a symlink. The only other\n+\t * time this errno occurs when O_REGULAR is used. Since we don't use\n+\t * it anywhere we can avoid an lstat here.\n+\t */\n+\tif (ret < 0 && errno == EFTYPE) {\n+\t\terrno = ELOOP;\n+\t\treturn -1;\n+\t}\n+#endif\n+\treturn ret;\n #else\n \tstruct stat st;\n \tif (lstat(path, &st) < 0)\n-- \n2.49.0\n\n"},{"id":"517148","messageId":"aBVp51yLwxBpRskt@tapette.crustytoothpaste.net","threadId":"63386","inReplyTo":"20250502233403.289761-1-collin.funk1@gmail.com","subject":"Re: [PATCH] wrapper: Fix a errno discrepancy on NetBSD.","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2025-05-03T00:57:11Z","receivedAt":"2025-05-03T00:57:14Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2025-05-02 at 23:33:32, Collin Funk wrote:\n> As documented on NetBSD's man page, open with the O_NOFOLLOW flag and a\n> symlink returns -1 and sets errno to EFTYPE which differs from POSIX.\n> This patch fixes the following test failure:\n> \n> $ sh t0602-reffiles-fsck.sh --verbose\n> --- expect\t2025-05-02 23:05:23.920890147 +0000\n> +++ err\t2025-05-02 23:05:23.916794959 +0000\n> @@ -1 +1 @@\n> -error: packed-refs: badRefFiletype: not a regular file but a symlink\n> +error: unable to open '.git/packed-refs': Inappropriate file type or format\n> not ok 12 - the filetype of packed-refs should be checked\n> \n> This portability issue was introduced in Commit\n> cfea2f2da8 (packed-backend: check whether the \"packed-refs\" is regular file, 2025-02-28)\n> \n> Signed-off-by: Collin Funk <collin.funk1@gmail.com>\n> ---\n>  wrapper.c | 14 +++++++++++++-\n>  1 file changed, 13 insertions(+), 1 deletion(-)\n> \n> diff --git a/wrapper.c b/wrapper.c\n> index 3c79778055..4d448d7c57 100644\n> --- a/wrapper.c\n> +++ b/wrapper.c\n> @@ -737,7 +737,19 @@ int is_empty_or_missing_file(const char *filename)\n>  int open_nofollow(const char *path, int flags)\n>  {\n>  #ifdef O_NOFOLLOW\n> -\treturn open(path, flags | O_NOFOLLOW);\n> +\tint ret = open(path, flags | O_NOFOLLOW);\n> +#ifdef __NetBSD__\n> +\t/*\n> +\t * NetBSD sets errno to EFTYPE when path is a symlink. The only other\n> +\t * time this errno occurs when O_REGULAR is used. Since we don't use\n> +\t * it anywhere we can avoid an lstat here.\n> +\t */\n> +\tif (ret < 0 && errno == EFTYPE) {\n> +\t\terrno = ELOOP;\n> +\t\treturn -1;\n> +\t}\n> +#endif\n> +\treturn ret;\n\nThis patch seems reasonable and correct.  I don't use NetBSD, but I do\noften test there, and I'm aware of this infelicity.  I'm surprised we\nhaven't hit it before.\n\nI suspect we'll also hit this on FreeBSD, which has a similar issue in\nthat it returns `EMLINK` instead of `ELOOP`.  I do wish these two OSes\nwould provide an appropriate POSIX-compatible `open` call when set with\n`_POSIX_SOURCE`, since this is one of the biggest portability problems\nwith them.\n-- \nbrian m. carlson (they/them)\nToronto, Ontario, CA\n"},{"id":"517156","messageId":"xmqqtt62sdv9.fsf@gitster.g","threadId":"63386","inReplyTo":"aBVp51yLwxBpRskt@tapette.crustytoothpaste.net","subject":"Re: [PATCH] wrapper: Fix a errno discrepancy on NetBSD.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-03T01:05:14Z","receivedAt":"2025-05-03T01:05:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n\n> On 2025-05-02 at 23:33:32, Collin Funk wrote:\n>> As documented on NetBSD's man page, open with the O_NOFOLLOW flag and a\n>> symlink returns -1 and sets errno to EFTYPE which differs from POSIX.\n>> This patch fixes the following test failure:\n>> \n>> $ sh t0602-reffiles-fsck.sh --verbose\n>> --- expect\t2025-05-02 23:05:23.920890147 +0000\n>> +++ err\t2025-05-02 23:05:23.916794959 +0000\n>> @@ -1 +1 @@\n>> -error: packed-refs: badRefFiletype: not a regular file but a symlink\n>> +error: unable to open '.git/packed-refs': Inappropriate file type or format\n>> not ok 12 - the filetype of packed-refs should be checked\n>> \n>> This portability issue was introduced in Commit\n>> cfea2f2da8 (packed-backend: check whether the \"packed-refs\" is regular file, 2025-02-28)\n>> \n>> Signed-off-by: Collin Funk <collin.funk1@gmail.com>\n>> ---\n>>  wrapper.c | 14 +++++++++++++-\n>>  1 file changed, 13 insertions(+), 1 deletion(-)\n>> \n>> diff --git a/wrapper.c b/wrapper.c\n>> index 3c79778055..4d448d7c57 100644\n>> --- a/wrapper.c\n>> +++ b/wrapper.c\n>> @@ -737,7 +737,19 @@ int is_empty_or_missing_file(const char *filename)\n>>  int open_nofollow(const char *path, int flags)\n>>  {\n>>  #ifdef O_NOFOLLOW\n>> -\treturn open(path, flags | O_NOFOLLOW);\n>> +\tint ret = open(path, flags | O_NOFOLLOW);\n>> +#ifdef __NetBSD__\n>> +\t/*\n>> +\t * NetBSD sets errno to EFTYPE when path is a symlink. The only other\n>> +\t * time this errno occurs when O_REGULAR is used. Since we don't use\n>> +\t * it anywhere we can avoid an lstat here.\n>> +\t */\n>> +\tif (ret < 0 && errno == EFTYPE) {\n>> +\t\terrno = ELOOP;\n>> +\t\treturn -1;\n>> +\t}\n>> +#endif\n>> +\treturn ret;\n>\n> This patch seems reasonable and correct.  I don't use NetBSD, but I do\n> often test there, and I'm aware of this infelicity.  I'm surprised we\n> haven't hit it before.\n\nThanks, both.  Will queue after fixing the proposed log message a\nbit (the sample must be indented, especially when it contains lines\nthat look like a patch).\n\n> I suspect we'll also hit this on FreeBSD, which has a similar issue in\n> that it returns `EMLINK` instead of `ELOOP`.\n\nI won't expect Collin or you to redo this patch to cover FreeBSD;\nanybody with FreeBSD box/vm can do a separate patch on a different\nday.\n\n> I do wish these two OSes\n> would provide an appropriate POSIX-compatible `open` call when set with\n> `_POSIX_SOURCE`, since this is one of the biggest portability problems\n> with them.\n\nThat may be true, but not something we can fix here X-<.\n"},{"id":"517162","messageId":"877c2ybbhz.fsf@gmail.com","threadId":"63386","inReplyTo":"aBVp51yLwxBpRskt@tapette.crustytoothpaste.net","subject":"Re: [PATCH] wrapper: Fix a errno discrepancy on NetBSD.","fromName":"Collin Funk","fromEmail":"collin.funk1@gmail.com","sentAt":"2025-05-03T03:48:24Z","receivedAt":"2025-05-03T03:48:26Z","isPatch":true,"sender":{"key":"collin.funk1@gmail.com","avatar":"https://avatars.githubusercontent.com/u/65689063?v=4"},"body":"Hi Brian,\n\n\"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n\n> I suspect we'll also hit this on FreeBSD, which has a similar issue in\n> that it returns `EMLINK` instead of `ELOOP`.\n\nGood memory. I can confirm that FreeBSD fails in the same place with a\nmessage for EMLINK. I'll write another patch for that.\n\n> I do wish these two OSes would provide an appropriate POSIX-compatible\n> `open` call when set with `_POSIX_SOURCE`, since this is one of the\n> biggest portability problems with them.\n\nIt is documented in their man pages, so I assume it is intentional. But\nI don't see any benefit to differing from POSIX here. I'll see if I can\nfind any discussion before I submit a bug report.\n\nCollin\n"},{"id":"517164","messageId":"20250503041718.42195-1-collin.funk1@gmail.com","threadId":"63386","inReplyTo":"20250502233403.289761-1-collin.funk1@gmail.com","subject":"[PATCH v2] wrapper: NetBSD gives EFTYPE and FreeBSD gives EMFILE where POSIX uses ELOOP","fromName":"Collin Funk","fromEmail":"collin.funk1@gmail.com","sentAt":"2025-05-03T04:16:51Z","receivedAt":"2025-05-03T04:17:30Z","isPatch":true,"sender":{"key":"collin.funk1@gmail.com","avatar":"https://avatars.githubusercontent.com/u/65689063?v=4"},"body":"As documented on NetBSD's man page, open with the O_NOFOLLOW flag and a\nsymlink returns -1 and sets errno to EFTYPE which differs from POSIX.\n\nThis patch fixes the following test failure:\n\n    $ sh t0602-reffiles-fsck.sh --verbose\n    --- expect\t2025-05-02 23:05:23.920890147 +0000\n    +++ err\t2025-05-02 23:05:23.916794959 +0000\n    @@ -1 +1 @@\n    -error: packed-refs: badRefFiletype: not a regular file but a symlink\n    +error: unable to open '.git/packed-refs': Inappropriate file type or format\n    not ok 12 - the filetype of packed-refs should be checked\n\nFreeBSD has the same issue for EMLINK instead of EFTYPE.\n\nThis portability issue was introduced in cfea2f2da8 (packed-backend:\ncheck whether the \"packed-refs\" is regular file, 2025-02-28)\n\nSigned-off-by: Collin Funk <collin.funk1@gmail.com>\n---\n wrapper.c | 21 ++++++++++++++++++++-\n 1 file changed, 20 insertions(+), 1 deletion(-)\n\ndiff --git a/wrapper.c b/wrapper.c\nindex 3c79778055..f74e3f7747 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -737,7 +737,26 @@ int is_empty_or_missing_file(const char *filename)\n int open_nofollow(const char *path, int flags)\n {\n #ifdef O_NOFOLLOW\n-\treturn open(path, flags | O_NOFOLLOW);\n+\tint ret = open(path, flags | O_NOFOLLOW);\n+\t/*\n+\t * NetBSD sets errno to EFTYPE when path is a symlink. The only other\n+\t * time this errno occurs when O_REGULAR is used. Since we don't use\n+\t * it anywhere we can avoid an lstat here. FreeBSD does the same with\n+\t * EMLINK.\n+\t */\n+#ifdef __NetBSD__\n+#define SYMLINK_ERRNO EFTYPE\n+#elif defined(__FreeBSD__)\n+#define SYMLINK_ERRNO EMLINK\n+#endif\n+#if SYMLINK_ERRNO\n+\tif (ret < 0 && errno == SYMLINK_ERRNO) {\n+\t\terrno = ELOOP;\n+\t\treturn -1;\n+\t}\n+#undef SYMLINK_ERRNO\n+#endif\n+\treturn ret;\n #else\n \tstruct stat st;\n \tif (lstat(path, &st) < 0)\n-- \n2.49.0\n\n"},{"id":"517165","messageId":"87jz6ycojo.fsf@gmail.com","threadId":"63386","inReplyTo":"xmqqtt62sdv9.fsf@gitster.g","subject":"Re: [PATCH] wrapper: Fix a errno discrepancy on NetBSD.","fromName":"Collin Funk","fromEmail":"collin.funk1@gmail.com","sentAt":"2025-05-03T04:21:15Z","receivedAt":"2025-05-03T04:21:17Z","isPatch":true,"sender":{"key":"collin.funk1@gmail.com","avatar":"https://avatars.githubusercontent.com/u/65689063?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Thanks, both.  Will queue after fixing the proposed log message a\n> bit (the sample must be indented, especially when it contains lines\n> that look like a patch).\n\nYes, that looks better. Thanks!\n\n>> I suspect we'll also hit this on FreeBSD, which has a similar issue in\n>> that it returns `EMLINK` instead of `ELOOP`.\n>\n> I won't expect Collin or you to redo this patch to cover FreeBSD;\n> anybody with FreeBSD box/vm can do a separate patch on a different\n> day.\n\nIt is no problem. I have access to a FreeBSD 14.2 machine. I can confirm\nthat it fails with EMLINK.\n\nMy V2 patch fixes the issue on both platforms. From the documentation\nOpenBSD shouldn't be an issue. But if so it should be trivial to fix.\n\nCollin\n"},{"id":"517170","messageId":"20250503133158.GA4450@coredump.intra.peff.net","threadId":"63386","inReplyTo":"aBVp51yLwxBpRskt@tapette.crustytoothpaste.net","subject":"Re: [PATCH] wrapper: Fix a errno discrepancy on NetBSD.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-05-03T13:31:58Z","receivedAt":"2025-05-03T13:32:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, May 03, 2025 at 12:57:11AM +0000, brian m. carlson wrote:\n\n> > diff --git a/wrapper.c b/wrapper.c\n> > index 3c79778055..4d448d7c57 100644\n> > --- a/wrapper.c\n> > +++ b/wrapper.c\n> > @@ -737,7 +737,19 @@ int is_empty_or_missing_file(const char *filename)\n> >  int open_nofollow(const char *path, int flags)\n> >  {\n> >  #ifdef O_NOFOLLOW\n> > -\treturn open(path, flags | O_NOFOLLOW);\n> > +\tint ret = open(path, flags | O_NOFOLLOW);\n> > +#ifdef __NetBSD__\n> > +\t/*\n> > +\t * NetBSD sets errno to EFTYPE when path is a symlink. The only other\n> > +\t * time this errno occurs when O_REGULAR is used. Since we don't use\n> > +\t * it anywhere we can avoid an lstat here.\n> > +\t */\n> > +\tif (ret < 0 && errno == EFTYPE) {\n> > +\t\terrno = ELOOP;\n> > +\t\treturn -1;\n> > +\t}\n> > +#endif\n> > +\treturn ret;\n> \n> This patch seems reasonable and correct.  I don't use NetBSD, but I do\n> often test there, and I'm aware of this infelicity.  I'm surprised we\n> haven't hit it before.\n> \n> I suspect we'll also hit this on FreeBSD, which has a similar issue in\n> that it returns `EMLINK` instead of `ELOOP`.  I do wish these two OSes\n> would provide an appropriate POSIX-compatible `open` call when set with\n> `_POSIX_SOURCE`, since this is one of the biggest portability problems\n> with them.\n\nThe inconsistency in errno has been there since open_nofollow() was\nadded years ago. But we didn't notice it because in general we try not\nto be too particular about which errno value we receive.\n\nThat changed in cfea2f2da8 (packed-backend: check whether the\n\"packed-refs\" is regular file, 2025-02-28), which uses open_nofollow()\nto check for symlinks while we open it. But it feels like it would be\nmore direct to just lstat() the file in the first place (which we end up\ndoing anyway to check for other things besides symlinks!).\n\nI.e., I'd think this would just work everywhere:\n\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 3ad1ed0787..a247220df9 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -2071,35 +2071,32 @@ static int packed_fsck(struct ref_store *ref_store,\n \tif (o->verbose)\n \t\tfprintf_ln(stderr, \"Checking packed-refs file %s\", refs->path);\n \n-\tfd = open_nofollow(refs->path, O_RDONLY);\n-\tif (fd < 0) {\n+\tif (lstat(refs->path, &st) < 0) {\n \t\t/*\n \t\t * If the packed-refs file doesn't exist, there's nothing\n \t\t * to check.\n \t\t */\n \t\tif (errno == ENOENT)\n \t\t\tgoto cleanup;\n \n-\t\tif (errno == ELOOP) {\n-\t\t\tstruct fsck_ref_report report = { 0 };\n-\t\t\treport.path = \"packed-refs\";\n-\t\t\tret = fsck_report_ref(o, &report,\n-\t\t\t\t\t      FSCK_MSG_BAD_REF_FILETYPE,\n-\t\t\t\t\t      \"not a regular file but a symlink\");\n-\t\t\tgoto cleanup;\n-\t\t}\n-\n-\t\tret = error_errno(_(\"unable to open '%s'\"), refs->path);\n-\t\tgoto cleanup;\n-\t} else if (fstat(fd, &st) < 0) {\n \t\tret = error_errno(_(\"unable to stat '%s'\"), refs->path);\n \t\tgoto cleanup;\n-\t} else if (!S_ISREG(st.st_mode)) {\n+\t}\n+\n+\tif (!S_ISREG(st.st_mode)) {\n \t\tstruct fsck_ref_report report = { 0 };\n \t\treport.path = \"packed-refs\";\n \t\tret = fsck_report_ref(o, &report,\n \t\t\t\t      FSCK_MSG_BAD_REF_FILETYPE,\n \t\t\t\t      \"not a regular file\");\n+\t\t/* XXX optionally could keep going here and actually\n+\t\t * check the contents we do find */\n+\t\tgoto cleanup;\n+\t}\n+\n+\tfd = open(refs->path, O_RDONLY);\n+\tif (fd < 0) {\n+\t\tret = error_errno(_(\"unable to open '%s'\"), refs->path);\n \t\tgoto cleanup;\n \t}\n \ndiff --git a/t/t0602-reffiles-fsck.sh b/t/t0602-reffiles-fsck.sh\nindex 9d1dc2144c..34d54a7c05 100755\n--- a/t/t0602-reffiles-fsck.sh\n+++ b/t/t0602-reffiles-fsck.sh\n@@ -632,7 +632,7 @@ test_expect_success SYMLINKS 'the filetype of packed-refs should be checked' '\n \t\tln -sf packed-refs-back .git/packed-refs &&\n \t\ttest_must_fail git refs verify 2>err &&\n \t\tcat >expect <<-EOF &&\n-\t\terror: packed-refs: badRefFiletype: not a regular file but a symlink\n+\t\terror: packed-refs: badRefFiletype: not a regular file\n \t\tEOF\n \t\trm .git/packed-refs &&\n \t\ttest_cmp expect err &&\n\nIt's not as \"atomic\" as open_nofollow() and fstat(), but I don't think\nwe care about that for fsck. This is about consistency checking, not\ntrying to beat races against active adversaries (not to mention that our\nopen_nofollow() is best-effort anyway, and may be racy).\n\nI dunno. I don't mind making errno returns more consistent to prevent a\nfuture foot-gun, but I think as a general rule we may be better off not\nlooking too hard at errno for exotic conditions.\n\n-Peff\n\nPS I notice that this same function reads the whole packed-refs file\n   into a strbuf. That may be a problem, as they can grow pretty big in\n   extreme cases (e.g., GitHub's fork networks easily got into the\n   gigabytes, as it was every ref of every fork). We usually mmap it.\n   Not related to this discussion, but just something I noticed while\n   reading the function.\n"},{"id":"517172","messageId":"aBYvMjtGjzEhKg4s@ArchLinux","threadId":"63386","inReplyTo":"20250503133158.GA4450@coredump.intra.peff.net","subject":"Re: [PATCH] wrapper: Fix a errno discrepancy on NetBSD.","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-05-03T14:58:58Z","receivedAt":"2025-05-03T14:58:39Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Sat, May 03, 2025 at 09:31:58AM -0400, Jeff King wrote:\n> On Sat, May 03, 2025 at 12:57:11AM +0000, brian m. carlson wrote:\n> \n> > > diff --git a/wrapper.c b/wrapper.c\n> > > index 3c79778055..4d448d7c57 100644\n> > > --- a/wrapper.c\n> > > +++ b/wrapper.c\n> > > @@ -737,7 +737,19 @@ int is_empty_or_missing_file(const char *filename)\n> > >  int open_nofollow(const char *path, int flags)\n> > >  {\n> > >  #ifdef O_NOFOLLOW\n> > > -\treturn open(path, flags | O_NOFOLLOW);\n> > > +\tint ret = open(path, flags | O_NOFOLLOW);\n> > > +#ifdef __NetBSD__\n> > > +\t/*\n> > > +\t * NetBSD sets errno to EFTYPE when path is a symlink. The only other\n> > > +\t * time this errno occurs when O_REGULAR is used. Since we don't use\n> > > +\t * it anywhere we can avoid an lstat here.\n> > > +\t */\n> > > +\tif (ret < 0 && errno == EFTYPE) {\n> > > +\t\terrno = ELOOP;\n> > > +\t\treturn -1;\n> > > +\t}\n> > > +#endif\n> > > +\treturn ret;\n> > \n> > This patch seems reasonable and correct.  I don't use NetBSD, but I do\n> > often test there, and I'm aware of this infelicity.  I'm surprised we\n> > haven't hit it before.\n> > \n> > I suspect we'll also hit this on FreeBSD, which has a similar issue in\n> > that it returns `EMLINK` instead of `ELOOP`.  I do wish these two OSes\n> > would provide an appropriate POSIX-compatible `open` call when set with\n> > `_POSIX_SOURCE`, since this is one of the biggest portability problems\n> > with them.\n> \n> The inconsistency in errno has been there since open_nofollow() was\n> added years ago. But we didn't notice it because in general we try not\n> to be too particular about which errno value we receive.\n> \n> That changed in cfea2f2da8 (packed-backend: check whether the\n> \"packed-refs\" is regular file, 2025-02-28), which uses open_nofollow()\n> to check for symlinks while we open it. But it feels like it would be\n> more direct to just lstat() the file in the first place (which we end up\n> doing anyway to check for other things besides symlinks!).\n> \n> I.e., I'd think this would just work everywhere:\n> \n> diff --git a/refs/packed-backend.c b/refs/packed-backend.c\n> index 3ad1ed0787..a247220df9 100644\n> --- a/refs/packed-backend.c\n> +++ b/refs/packed-backend.c\n> @@ -2071,35 +2071,32 @@ static int packed_fsck(struct ref_store *ref_store,\n>  \tif (o->verbose)\n>  \t\tfprintf_ln(stderr, \"Checking packed-refs file %s\", refs->path);\n>  \n> -\tfd = open_nofollow(refs->path, O_RDONLY);\n> -\tif (fd < 0) {\n> +\tif (lstat(refs->path, &st) < 0) {\n>  \t\t/*\n>  \t\t * If the packed-refs file doesn't exist, there's nothing\n>  \t\t * to check.\n>  \t\t */\n>  \t\tif (errno == ENOENT)\n>  \t\t\tgoto cleanup;\n>  \n> -\t\tif (errno == ELOOP) {\n> -\t\t\tstruct fsck_ref_report report = { 0 };\n> -\t\t\treport.path = \"packed-refs\";\n> -\t\t\tret = fsck_report_ref(o, &report,\n> -\t\t\t\t\t      FSCK_MSG_BAD_REF_FILETYPE,\n> -\t\t\t\t\t      \"not a regular file but a symlink\");\n> -\t\t\tgoto cleanup;\n> -\t\t}\n> -\n> -\t\tret = error_errno(_(\"unable to open '%s'\"), refs->path);\n> -\t\tgoto cleanup;\n> -\t} else if (fstat(fd, &st) < 0) {\n>  \t\tret = error_errno(_(\"unable to stat '%s'\"), refs->path);\n>  \t\tgoto cleanup;\n> -\t} else if (!S_ISREG(st.st_mode)) {\n> +\t}\n> +\n> +\tif (!S_ISREG(st.st_mode)) {\n>  \t\tstruct fsck_ref_report report = { 0 };\n>  \t\treport.path = \"packed-refs\";\n>  \t\tret = fsck_report_ref(o, &report,\n>  \t\t\t\t      FSCK_MSG_BAD_REF_FILETYPE,\n>  \t\t\t\t      \"not a regular file\");\n> +\t\t/* XXX optionally could keep going here and actually\n> +\t\t * check the contents we do find */\n> +\t\tgoto cleanup;\n> +\t}\n> +\n> +\tfd = open(refs->path, O_RDONLY);\n> +\tif (fd < 0) {\n> +\t\tret = error_errno(_(\"unable to open '%s'\"), refs->path);\n>  \t\tgoto cleanup;\n>  \t}\n>  \n> diff --git a/t/t0602-reffiles-fsck.sh b/t/t0602-reffiles-fsck.sh\n> index 9d1dc2144c..34d54a7c05 100755\n> --- a/t/t0602-reffiles-fsck.sh\n> +++ b/t/t0602-reffiles-fsck.sh\n> @@ -632,7 +632,7 @@ test_expect_success SYMLINKS 'the filetype of packed-refs should be checked' '\n>  \t\tln -sf packed-refs-back .git/packed-refs &&\n>  \t\ttest_must_fail git refs verify 2>err &&\n>  \t\tcat >expect <<-EOF &&\n> -\t\terror: packed-refs: badRefFiletype: not a regular file but a symlink\n> +\t\terror: packed-refs: badRefFiletype: not a regular file\n>  \t\tEOF\n>  \t\trm .git/packed-refs &&\n>  \t\ttest_cmp expect err &&\n> \n> It's not as \"atomic\" as open_nofollow() and fstat(), but I don't think\n> we care about that for fsck. This is about consistency checking, not\n> trying to beat races against active adversaries (not to mention that our\n> open_nofollow() is best-effort anyway, and may be racy).\n> \n\nThe motivation why I use \"open_nofollow\" is that we want to avoid race\nconditions as many as possible. However, as you have said, our\n\"open_nofollow\" function still has the race. And it would be a little\ncumbersome to make \"open_nofollow\" compatible. So, I rather prefer that\nwe simply use this way to solve the problem.\n\n> I dunno. I don't mind making errno returns more consistent to prevent a\n> future foot-gun, but I think as a general rule we may be better off not\n> looking too hard at errno for exotic conditions.\n> \n\nI don't mind either.\n\n> -Peff\n> \n> PS I notice that this same function reads the whole packed-refs file\n>    into a strbuf. That may be a problem, as they can grow pretty big in\n>    extreme cases (e.g., GitHub's fork networks easily got into the\n>    gigabytes, as it was every ref of every fork). We usually mmap it.\n>    Not related to this discussion, but just something I noticed while\n>    reading the function.\n\nPeff, thanks for notifying me. I want to know more background.\nInitially, the reason why I don't use `mmap` is that when checking the\nref consistency, we usually don't need to share the \"packed-refs\"\ncontent for multiple processes via `mmap`.\n\nI don't know how Github executes \"git fsck\" for the forked repositories.\nIs there any regular tasks for \"git fsck\"? And would \"packed-refs\" file\nbe shared for all these repositories?\n\nIf above is the case, I agree that we should reuse the logic of\n\"load_contents\" to enhance. But I don't know whether we need to do this\nin the first place.\n\nThanks,\nJialuo\n"},{"id":"517173","messageId":"aBY6BPnuSfslYlYt@tapette.crustytoothpaste.net","threadId":"63386","inReplyTo":"20250503041718.42195-1-collin.funk1@gmail.com","subject":"Re: [PATCH v2] wrapper: NetBSD gives EFTYPE and FreeBSD gives EMFILE where POSIX uses ELOOP","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2025-05-03T15:45:08Z","receivedAt":"2025-05-03T15:45:11Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2025-05-03 at 04:16:51, Collin Funk wrote:\n> As documented on NetBSD's man page, open with the O_NOFOLLOW flag and a\n> symlink returns -1 and sets errno to EFTYPE which differs from POSIX.\n> \n> This patch fixes the following test failure:\n> \n>     $ sh t0602-reffiles-fsck.sh --verbose\n>     --- expect\t2025-05-02 23:05:23.920890147 +0000\n>     +++ err\t2025-05-02 23:05:23.916794959 +0000\n>     @@ -1 +1 @@\n>     -error: packed-refs: badRefFiletype: not a regular file but a symlink\n>     +error: unable to open '.git/packed-refs': Inappropriate file type or format\n>     not ok 12 - the filetype of packed-refs should be checked\n> \n> FreeBSD has the same issue for EMLINK instead of EFTYPE.\n> \n> This portability issue was introduced in cfea2f2da8 (packed-backend:\n> check whether the \"packed-refs\" is regular file, 2025-02-28)\n\nYup, this looks good.  Thanks again for the patch.\n\nI'll just add one resource for people who might like to look into these\nkinds of things more.  https://man.freebsd.org/cgi/man.cgi is the\nFreeBSD man page viewer, which lets you view manual pages from the BSDs,\nLinux, and some proprietary Unix systems.  It can be quite helpful for\nfinding and fixing portability issues like this or just seeing what\ncommand-line options or arguments a certain Unix supports.\n-- \nbrian m. carlson (they/them)\nToronto, Ontario, CA\n"},{"id":"517174","messageId":"20250503154928.GA3412@coredump.intra.peff.net","threadId":"63386","inReplyTo":"aBYvMjtGjzEhKg4s@ArchLinux","subject":"Re: [PATCH] wrapper: Fix a errno discrepancy on NetBSD.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-05-03T15:49:28Z","receivedAt":"2025-05-03T15:49:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, May 03, 2025 at 10:58:58PM +0800, shejialuo wrote:\n\n> > PS I notice that this same function reads the whole packed-refs file\n> >    into a strbuf. That may be a problem, as they can grow pretty big in\n> >    extreme cases (e.g., GitHub's fork networks easily got into the\n> >    gigabytes, as it was every ref of every fork). We usually mmap it.\n> >    Not related to this discussion, but just something I noticed while\n> >    reading the function.\n> \n> Peff, thanks for notifying me. I want to know more background.\n> Initially, the reason why I don't use `mmap` is that when checking the\n> ref consistency, we usually don't need to share the \"packed-refs\"\n> content for multiple processes via `mmap`.\n\nYou're not sharing with other processes running fsck, but you'd be\nsharing the memory with all of the other processes using that\npacked-refs file for normal lookups.\n\nBut even if it's shared with nobody, reading it all into memory is\nstrictly worse than just mmap (since the data is getting copied into the\nnew allocation).\n\n> I don't know how Github executes \"git fsck\" for the forked repositories.\n> Is there any regular tasks for \"git fsck\"? And would \"packed-refs\" file\n> be shared for all these repositories?\n\nI don't know offhand how often GitHub runs fsck in an automated way\nthese days. Or even how big packed-refs files get, for that matter.\n\nThe specific case I'm thinking of for GitHub is that each fork network\nhas a master \"network.git\" repo that stores the objects for all of the\nforks (which point to it via their objects/info/alternates files).  That\nnetwork.git repo doesn't technically need to have all of the refs all\nthe time, but in practice it wants to know about them for reachability\nduring repacking, etc.\n\nSo it has something like \"refs/remotes/<fork_id>/heads/master\", and so\non, copying the whole refs/* namespace of each fork. If you look at,\nsay, torvalds/linux, the refs data for a single fork is probably ~30k or\nso (based on looking at what's in a clone). And there are ~55k forks. So\nthat's around 1.5G. Not a deal-breaker to allocate (keeping in mind they\nhave pretty beefy systems), but enough that mmap is probably better.\n\nI'm also sure that's not the worst case. It has a lot of forks but the\nref namespace is not that huge compared to some other projects (and it's\nthe product of the two that is the problem).\n\n> If above is the case, I agree that we should reuse the logic of\n> \"load_contents\" to enhance. But I don't know whether we need to do this\n> in the first place.\n\nI think you can skip the stat validity bits. In theory you can also skip\nthe mmap_strategy stuff, but I guess it might mean that \"fsck\" could\nblock other writers on Windows temporarily (though we wouldn't plan to\nhold it open long, the way the normal reader does).\n\nThe other gotcha is that the result won't be NUL-terminated, but it\nlooks like the helper functions already take an \"eof\" pointer to avoid\nlooking past the end of what was read.\n\n-Peff\n"},{"id":"517178","messageId":"xmqqjz6xsiqq.fsf@gitster.g","threadId":"63386","inReplyTo":"87jz6ycojo.fsf@gmail.com","subject":"Re: [PATCH] wrapper: Fix a errno discrepancy on NetBSD.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-03T17:32:13Z","receivedAt":"2025-05-03T17:32:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Collin Funk <collin.funk1@gmail.com> writes:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> Thanks, both.  Will queue after fixing the proposed log message a\n>> bit (the sample must be indented, especially when it contains lines\n>> that look like a patch).\n>\n> Yes, that looks better. Thanks!\n>\n>>> I suspect we'll also hit this on FreeBSD, which has a similar issue in\n>>> that it returns `EMLINK` instead of `ELOOP`.\n>>\n>> I won't expect Collin or you to redo this patch to cover FreeBSD;\n>> anybody with FreeBSD box/vm can do a separate patch on a different\n>> day.\n>\n> It is no problem. I have access to a FreeBSD 14.2 machine. I can confirm\n> that it fails with EMLINK.\n\nExcellent.\n"},{"id":"517179","messageId":"87tt61mt4q.fsf@gmail.com","threadId":"63386","inReplyTo":"aBY6BPnuSfslYlYt@tapette.crustytoothpaste.net","subject":"Re: [PATCH v2] wrapper: NetBSD gives EFTYPE and FreeBSD gives EMFILE where POSIX uses ELOOP","fromName":"Collin Funk","fromEmail":"collin.funk1@gmail.com","sentAt":"2025-05-03T18:44:21Z","receivedAt":"2025-05-03T18:44:23Z","isPatch":true,"sender":{"key":"collin.funk1@gmail.com","avatar":"https://avatars.githubusercontent.com/u/65689063?v=4"},"body":"\"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n\n> I'll just add one resource for people who might like to look into these\n> kinds of things more.  https://man.freebsd.org/cgi/man.cgi is the\n> FreeBSD man page viewer, which lets you view manual pages from the BSDs,\n> Linux, and some proprietary Unix systems.  It can be quite helpful for\n> finding and fixing portability issues like this or just seeing what\n> command-line options or arguments a certain Unix supports.\n\nGnulib documents most portability quirks too [1]. For example, it had the\nFreeBSD EMLINK and NetBSD EFTYPE with 'open(\"symlink\", O_NOFOLLOW ...)\ndocumented, but for some very old versions released around 2014. Now\nthat I have confirmed it still exists from this git test I have updated\nthe documentation there [2].\n\nThanks,\nCollin\n\n[1] https://www.gnu.org/software/gnulib/manual/\n[2] https://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=commit;h=c0c646e29fbda0a6eadd6012d8ed1eb33b6c3968\n"},{"id":"517180","messageId":"87frhlmsks.fsf@gmail.com","threadId":"63386","inReplyTo":"20250503133158.GA4450@coredump.intra.peff.net","subject":"Re: [PATCH] wrapper: Fix a errno discrepancy on NetBSD.","fromName":"Collin Funk","fromEmail":"collin.funk1@gmail.com","sentAt":"2025-05-03T18:56:19Z","receivedAt":"2025-05-03T18:56:22Z","isPatch":true,"sender":{"key":"collin.funk1@gmail.com","avatar":"https://avatars.githubusercontent.com/u/65689063?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I dunno. I don't mind making errno returns more consistent to prevent a\n> future foot-gun, but I think as a general rule we may be better off not\n> looking too hard at errno for exotic conditions.\n\nI generally agree. But in this case FreeBSD only sets errno to EMLINK in\nthis specific case and the only other case NetBSD sets errno to EFTYPE\nis when the O_REGULAR flag is used and the path is not a regular file.\nUsing 'grep -r O_REGULAR' confirms it is never used in git, and since it\nis a NetBSD extension I doubt it will ever be used. So not an exotic\ncase, in my opinion.\n\nThanks,\nCollin\n"},{"id":"517205","messageId":"aBhdH9jWpnpbkPHn@pks.im","threadId":"63386","inReplyTo":"20250503154928.GA3412@coredump.intra.peff.net","subject":"Re: [PATCH] wrapper: Fix a errno discrepancy on NetBSD.","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-05T06:39:27Z","receivedAt":"2025-05-05T06:39:36Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sat, May 03, 2025 at 11:49:28AM -0400, Jeff King wrote:\n> On Sat, May 03, 2025 at 10:58:58PM +0800, shejialuo wrote:\n> \n> > > PS I notice that this same function reads the whole packed-refs file\n> > >    into a strbuf. That may be a problem, as they can grow pretty big in\n> > >    extreme cases (e.g., GitHub's fork networks easily got into the\n> > >    gigabytes, as it was every ref of every fork). We usually mmap it.\n> > >    Not related to this discussion, but just something I noticed while\n> > >    reading the function.\n> > \n> > Peff, thanks for notifying me. I want to know more background.\n> > Initially, the reason why I don't use `mmap` is that when checking the\n> > ref consistency, we usually don't need to share the \"packed-refs\"\n> > content for multiple processes via `mmap`.\n> \n> You're not sharing with other processes running fsck, but you'd be\n> sharing the memory with all of the other processes using that\n> packed-refs file for normal lookups.\n> \n> But even if it's shared with nobody, reading it all into memory is\n> strictly worse than just mmap (since the data is getting copied into the\n> new allocation).\n> \n> > I don't know how Github executes \"git fsck\" for the forked repositories.\n> > Is there any regular tasks for \"git fsck\"? And would \"packed-refs\" file\n> > be shared for all these repositories?\n> \n> I don't know offhand how often GitHub runs fsck in an automated way\n> these days. Or even how big packed-refs files get, for that matter.\n\nThey typically are at most a couple of megabytes, but there certainly\nare outliers. For as at GitLab.com, the vast majority (>99%) of such\nfiles is less than 50MB and typically even less than 5MB.\n\n> The specific case I'm thinking of for GitHub is that each fork network\n> has a master \"network.git\" repo that stores the objects for all of the\n> forks (which point to it via their objects/info/alternates files).  That\n> network.git repo doesn't technically need to have all of the refs all\n> the time, but in practice it wants to know about them for reachability\n> during repacking, etc.\n> \n> So it has something like \"refs/remotes/<fork_id>/heads/master\", and so\n> on, copying the whole refs/* namespace of each fork. If you look at,\n> say, torvalds/linux, the refs data for a single fork is probably ~30k or\n> so (based on looking at what's in a clone). And there are ~55k forks. So\n> that's around 1.5G. Not a deal-breaker to allocate (keeping in mind they\n> have pretty beefy systems), but enough that mmap is probably better.\n> \n> I'm also sure that's not the worst case. It has a lot of forks but the\n> ref namespace is not that huge compared to some other projects (and it's\n> the product of the two that is the problem).\n\nYeah, the interesting case is always the outliers. One of the worst\noffenders we have at GitLab.com is our own \"gitlab-org/gitlab\"\nrepository. This particular repository has a \"packed-refs\" file that is\naround 2GB in size.\n\nSo I think refactoring this code to use `mmap()` would probably make\nsense.\n\nPatrick\n"},{"id":"517206","messageId":"aBheGySF1FTsIVzx@pks.im","threadId":"63386","inReplyTo":"20250503041718.42195-1-collin.funk1@gmail.com","subject":"Re: [PATCH v2] wrapper: NetBSD gives EFTYPE and FreeBSD gives EMFILE where POSIX uses ELOOP","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-05T06:43:39Z","receivedAt":"2025-05-05T06:43:44Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, May 02, 2025 at 09:16:51PM -0700, Collin Funk wrote:\n> As documented on NetBSD's man page, open with the O_NOFOLLOW flag and a\n> symlink returns -1 and sets errno to EFTYPE which differs from POSIX.\n> \n> This patch fixes the following test failure:\n> \n>     $ sh t0602-reffiles-fsck.sh --verbose\n>     --- expect\t2025-05-02 23:05:23.920890147 +0000\n>     +++ err\t2025-05-02 23:05:23.916794959 +0000\n>     @@ -1 +1 @@\n>     -error: packed-refs: badRefFiletype: not a regular file but a symlink\n>     +error: unable to open '.git/packed-refs': Inappropriate file type or format\n>     not ok 12 - the filetype of packed-refs should be checked\n> \n> FreeBSD has the same issue for EMLINK instead of EFTYPE.\n> \n> This portability issue was introduced in cfea2f2da8 (packed-backend:\n> check whether the \"packed-refs\" is regular file, 2025-02-28)\n\nOk, makes sense.\n\n> diff --git a/wrapper.c b/wrapper.c\n> index 3c79778055..f74e3f7747 100644\n> --- a/wrapper.c\n> +++ b/wrapper.c\n> @@ -737,7 +737,26 @@ int is_empty_or_missing_file(const char *filename)\n>  int open_nofollow(const char *path, int flags)\n>  {\n>  #ifdef O_NOFOLLOW\n> -\treturn open(path, flags | O_NOFOLLOW);\n> +\tint ret = open(path, flags | O_NOFOLLOW);\n> +\t/*\n> +\t * NetBSD sets errno to EFTYPE when path is a symlink. The only other\n> +\t * time this errno occurs when O_REGULAR is used. Since we don't use\n> +\t * it anywhere we can avoid an lstat here. FreeBSD does the same with\n> +\t * EMLINK.\n> +\t */\n> +#ifdef __NetBSD__\n> +#define SYMLINK_ERRNO EFTYPE\n> +#elif defined(__FreeBSD__)\n> +#define SYMLINK_ERRNO EMLINK\n> +#endif\n\nNit, to make this a bit easier to read: our style guide says that nested\npreprocessor directives should be indented by one spaces. So this would\nbecome:\n\n    # ifdef __NetBSD__\n    #  define SYMLINK_ERRNO EFTYPE\n    # elif defined(__FreeBSD__)\n    #  define SYMLINK_ERRNO EMLINK\n    # endif\n\nNote that the `ifdef` itself would also be indented because we already\nhave a surrounding `#ifdef O_NOFOLLOW`.\n\n> +#if SYMLINK_ERRNO\n> +\tif (ret < 0 && errno == SYMLINK_ERRNO) {\n> +\t\terrno = ELOOP;\n> +\t\treturn -1;\n> +\t}\n> +#undef SYMLINK_ERRNO\n> +#endif\n\nThese three preprocessor defines should be indented, as well.\n\nPatrick\n"},{"id":"517244","messageId":"aBisTxORH95BgLIT@ArchLinux","threadId":"63386","inReplyTo":"aBhdH9jWpnpbkPHn@pks.im","subject":"Re: [PATCH] wrapper: Fix a errno discrepancy on NetBSD.","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-05-05T12:17:19Z","receivedAt":"2025-05-05T12:16:57Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Mon, May 05, 2025 at 08:39:27AM +0200, Patrick Steinhardt wrote:\n> On Sat, May 03, 2025 at 11:49:28AM -0400, Jeff King wrote:\n> > On Sat, May 03, 2025 at 10:58:58PM +0800, shejialuo wrote:\n> > \n> > > > PS I notice that this same function reads the whole packed-refs file\n> > > >    into a strbuf. That may be a problem, as they can grow pretty big in\n> > > >    extreme cases (e.g., GitHub's fork networks easily got into the\n> > > >    gigabytes, as it was every ref of every fork). We usually mmap it.\n> > > >    Not related to this discussion, but just something I noticed while\n> > > >    reading the function.\n> > > \n> > > Peff, thanks for notifying me. I want to know more background.\n> > > Initially, the reason why I don't use `mmap` is that when checking the\n> > > ref consistency, we usually don't need to share the \"packed-refs\"\n> > > content for multiple processes via `mmap`.\n> > \n> > You're not sharing with other processes running fsck, but you'd be\n> > sharing the memory with all of the other processes using that\n> > packed-refs file for normal lookups.\n> > \n> > But even if it's shared with nobody, reading it all into memory is\n> > strictly worse than just mmap (since the data is getting copied into the\n> > new allocation).\n> > \n> > > I don't know how Github executes \"git fsck\" for the forked repositories.\n> > > Is there any regular tasks for \"git fsck\"? And would \"packed-refs\" file\n> > > be shared for all these repositories?\n> > \n> > I don't know offhand how often GitHub runs fsck in an automated way\n> > these days. Or even how big packed-refs files get, for that matter.\n> \n> They typically are at most a couple of megabytes, but there certainly\n> are outliers. For as at GitLab.com, the vast majority (>99%) of such\n> files is less than 50MB and typically even less than 5MB.\n> \n> > The specific case I'm thinking of for GitHub is that each fork network\n> > has a master \"network.git\" repo that stores the objects for all of the\n> > forks (which point to it via their objects/info/alternates files).  That\n> > network.git repo doesn't technically need to have all of the refs all\n> > the time, but in practice it wants to know about them for reachability\n> > during repacking, etc.\n> > \n> > So it has something like \"refs/remotes/<fork_id>/heads/master\", and so\n> > on, copying the whole refs/* namespace of each fork. If you look at,\n> > say, torvalds/linux, the refs data for a single fork is probably ~30k or\n> > so (based on looking at what's in a clone). And there are ~55k forks. So\n> > that's around 1.5G. Not a deal-breaker to allocate (keeping in mind they\n> > have pretty beefy systems), but enough that mmap is probably better.\n> > \n> > I'm also sure that's not the worst case. It has a lot of forks but the\n> > ref namespace is not that huge compared to some other projects (and it's\n> > the product of the two that is the problem).\n> \n> Yeah, the interesting case is always the outliers. One of the worst\n> offenders we have at GitLab.com is our own \"gitlab-org/gitlab\"\n> repository. This particular repository has a \"packed-refs\" file that is\n> around 2GB in size.\n> \n> So I think refactoring this code to use `mmap()` would probably make\n> sense.\n> \n\nThank Peff and Patrick for the information. I will send a patch later.\n\n> Patrick\n"},{"id":"517264","messageId":"xmqqtt5zoyg9.fsf@gitster.g","threadId":"63386","inReplyTo":"20250503133158.GA4450@coredump.intra.peff.net","subject":"Re: [PATCH] wrapper: Fix a errno discrepancy on NetBSD.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-05T15:43:18Z","receivedAt":"2025-05-05T15:43:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> That changed in cfea2f2da8 (packed-backend: check whether the\n> \"packed-refs\" is regular file, 2025-02-28), which uses open_nofollow()\n> to check for symlinks while we open it. But it feels like it would be\n> more direct to just lstat() the file in the first place (which we end up\n> doing anyway to check for other things besides symlinks!).\n> ...\n> It's not as \"atomic\" as open_nofollow() and fstat(), but I don't think\n> we care about that for fsck. This is about consistency checking, not\n> trying to beat races against active adversaries (not to mention that our\n> open_nofollow() is best-effort anyway, and may be racy).\n\nTrue.  Atomicity, which the use of open_nofollow() and fstat() tries\nto achieve, may not matter in fsck.  We can think of the use of\nopen_nofollow() in this particular codepath merely as a convenient\nhelper function, and I do not think we have any problem with such a\nhelper function.\n\nBut open_nofollow() and its emulation can be called from other\ncodepaths that may care about atomicity, and I am not sure what our\nattitude towards atomicity requirements vs platform capabilities\nshould be.\n\nIf an atomicity (or any other) requirement in a particular codepath\nhas a simple and obvious way to solve on common platforms, but that\nthe mechanism to implement the simple and obvious way is unavailable\non other platforms, where does it lead us?\n\nFor some kind of requirements, we can treat it merely as a quality\nof implementation issue, similar to how finalize_object_file()\nideally wants to do the create(TMP) then link(TMP->FINAL) then\nunlink(TMP) dance (because we want to detect collisions when able)\nbut has fallback implementation to create(TMP) then\nrename(TMP->FINAL) (which punts on collision detection) on platforms\nwhere the preferred way does not work.  It falls into this category,\nI would think, to think of use of open_nofollow() in this codepath\nas a mere helper function that makes the code in fsck shorter.\n\nBut for other kind of requirements, we want to fulfill them on all\nplatforms that we claim to support.  Using open_nofollow() to\nachieve hard atomicity requirement would be a bug in such a\nsituation.  Should we somehow warn our developers against its use?\n\nIdealists in us first try hard to find the right abstraction that\nwould work everywhere, and use compat/ layer to implement that\nabstraction, but we of course are often not successful, and end up\nwith a series of #ifdef for pieces of platform-specific code in\nfairly higher layer.  It feels that open_nofollow() that is not\nnecesarily atomic is the latter but that is done at a level that is\na bit too low.  I dunno.\n\n\n"},{"id":"517270","messageId":"20250505180311.GA29783@coredump.intra.peff.net","threadId":"63386","inReplyTo":"xmqqtt5zoyg9.fsf@gitster.g","subject":"Re: [PATCH] wrapper: Fix a errno discrepancy on NetBSD.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-05-05T18:03:11Z","receivedAt":"2025-05-05T18:03:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 05, 2025 at 08:43:18AM -0700, Junio C Hamano wrote:\n\n> But for other kind of requirements, we want to fulfill them on all\n> platforms that we claim to support.  Using open_nofollow() to\n> achieve hard atomicity requirement would be a bug in such a\n> situation.  Should we somehow warn our developers against its use?\n\nThe comment above the declaration says:\n\n  /*\n   * Open with O_NOFOLLOW, or equivalent. Note that the fallback equivalent\n   * may be racy. Do not use this as protection against an attacker who can\n   * simultaneously create paths.\n   */\n  int open_nofollow(const char *path, int flags);\n\nthough that may not be enough. 00611d8440 (add open_nofollow() helper,\n2021-02-16) discusses a way that it could be made less racy, at a\nslightly increased cost.\n\nIMHO that is somewhat orthogonal to the issue here, though, which is\npurely about the case where O_NOFOLLOW does exist (ironically, our\nracy fallback code consistently returns ELOOP ;) ).\n\nThe issue at hand is that particular errno responses are not always\nportable. The patch discussed here improves that. My point was more that\nI'm not sure to what degree we should care about errno consistency in\nour wrappers (which is inherently a bit whack-a-mole as we find new\ncases), versus trying not to care too hard about specific errno values\nin calling code.\n\nI can see arguments either way (and as I said, an argument for making\nerrno values consistent even if we try to rely on them less). Mostly I\nwas just a little surprised to see open_nofollow() being used in this\nway (especially since we have to end up stat()-ing anyway to check for\nother cases).\n\n-Peff\n"},{"id":"517275","messageId":"xmqqo6w6okni.fsf@gitster.g","threadId":"63386","inReplyTo":"aBheGySF1FTsIVzx@pks.im","subject":"Re: [PATCH v2] wrapper: NetBSD gives EFTYPE and FreeBSD gives EMFILE where POSIX uses ELOOP","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-05T20:41:21Z","receivedAt":"2025-05-05T20:41:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n>> +#ifdef __NetBSD__\n>> +#define SYMLINK_ERRNO EFTYPE\n>> +#elif defined(__FreeBSD__)\n>> +#define SYMLINK_ERRNO EMLINK\n>> +#endif\n>\n> Nit, to make this a bit easier to read: our style guide says that nested\n> preprocessor directives should be indented by one spaces. So this would\n> become:\n>\n>     # ifdef __NetBSD__\n>     #  define SYMLINK_ERRNO EFTYPE\n>     # elif defined(__FreeBSD__)\n>     #  define SYMLINK_ERRNO EMLINK\n>     # endif\n>\n> Note that the `ifdef` itself would also be indented because we already\n> have a surrounding `#ifdef O_NOFOLLOW`.\n\nHmph, it does look easier to read.  I think we used to have some\noutlier files that indented CPP directives by prefixing spaces in\nfront of the whole line, but these days we standardized to express\nthe indentation by inserting spaces immediately after '#' that\nalways sit at the beginning of line, so what you showed here is a\ngood example to mimic.\n\nThanks.\n\n>\n>> +#if SYMLINK_ERRNO\n>> +\tif (ret < 0 && errno == SYMLINK_ERRNO) {\n>> +\t\terrno = ELOOP;\n>> +\t\treturn -1;\n>> +\t}\n>> +#undef SYMLINK_ERRNO\n>> +#endif\n>\n> These three preprocessor defines should be indented, as well.\n>\n> Patrick\n"},{"id":"517285","messageId":"20250506010946.212068-1-collin.funk1@gmail.com","threadId":"63386","inReplyTo":"20250503041718.42195-1-collin.funk1@gmail.com","subject":"[PATCH v3] wrapper: NetBSD gives EFTYPE and FreeBSD gives EMFILE where POSIX uses ELOOP","fromName":"Collin Funk","fromEmail":"collin.funk1@gmail.com","sentAt":"2025-05-06T01:08:59Z","receivedAt":"2025-05-06T01:09:54Z","isPatch":true,"sender":{"key":"collin.funk1@gmail.com","avatar":"https://avatars.githubusercontent.com/u/65689063?v=4"},"body":"As documented on NetBSD's man page, open with the O_NOFOLLOW flag and a\nsymlink returns -1 and sets errno to EFTYPE which differs from POSIX.\n\nThis patch fixes the following test failure:\n\n    $ sh t0602-reffiles-fsck.sh --verbose\n    --- expect\t2025-05-02 23:05:23.920890147 +0000\n    +++ err\t2025-05-02 23:05:23.916794959 +0000\n    @@ -1 +1 @@\n    -error: packed-refs: badRefFiletype: not a regular file but a symlink\n    +error: unable to open '.git/packed-refs': Inappropriate file type or format\n    not ok 12 - the filetype of packed-refs should be checked\n\nFreeBSD has the same issue for EMLINK instead of EFTYPE.\n\nThis portability issue was introduced in cfea2f2da8 (packed-backend:\ncheck whether the \"packed-refs\" is regular file, 2025-02-28)\n\nSigned-off-by: Collin Funk <collin.funk1@gmail.com>\n---\n wrapper.c | 21 ++++++++++++++++++++-\n 1 file changed, 20 insertions(+), 1 deletion(-)\n\ndiff --git a/wrapper.c b/wrapper.c\nindex 8b98593149..38fce5327a 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -737,7 +737,26 @@ int is_empty_or_missing_file(const char *filename)\n int open_nofollow(const char *path, int flags)\n {\n #ifdef O_NOFOLLOW\n-\treturn open(path, flags | O_NOFOLLOW);\n+\tint ret = open(path, flags | O_NOFOLLOW);\n+\t/*\n+\t * NetBSD sets errno to EFTYPE when path is a symlink. The only other\n+\t * time this errno occurs when O_REGULAR is used. Since we don't use\n+\t * it anywhere we can avoid an lstat here. FreeBSD does the same with\n+\t * EMLINK.\n+\t */\n+# ifdef __NetBSD__\n+#  define SYMLINK_ERRNO EFTYPE\n+# elif defined(__FreeBSD__)\n+#  define SYMLINK_ERRNO EMLINK\n+# endif\n+# if SYMLINK_ERRNO\n+\tif (ret < 0 && errno == SYMLINK_ERRNO) {\n+\t\terrno = ELOOP;\n+\t\treturn -1;\n+\t}\n+#  undef SYMLINK_ERRNO\n+# endif\n+\treturn ret;\n #else\n \tstruct stat st;\n \tif (lstat(path, &st) < 0)\n-- \n2.49.0\n\n"},{"id":"517286","messageId":"87ikmemtd8.fsf@gmail.com","threadId":"63386","inReplyTo":"xmqqo6w6okni.fsf@gitster.g","subject":"Re: [PATCH v2] wrapper: NetBSD gives EFTYPE and FreeBSD gives EMFILE where POSIX uses ELOOP","fromName":"Collin Funk","fromEmail":"collin.funk1@gmail.com","sentAt":"2025-05-06T01:16:03Z","receivedAt":"2025-05-06T01:16:05Z","isPatch":true,"sender":{"key":"collin.funk1@gmail.com","avatar":"https://avatars.githubusercontent.com/u/65689063?v=4"},"body":"Hi all,\n\nJunio C Hamano <gitster@pobox.com> writes:\n\n>> Nit, to make this a bit easier to read: our style guide says that nested\n>> preprocessor directives should be indented by one spaces. So this would\n>> become:\n>>\n>>     # ifdef __NetBSD__\n>>     #  define SYMLINK_ERRNO EFTYPE\n>>     # elif defined(__FreeBSD__)\n>>     #  define SYMLINK_ERRNO EMLINK\n>>     # endif\n>>\n>> Note that the `ifdef` itself would also be indented because we already\n>> have a surrounding `#ifdef O_NOFOLLOW`.\n>\n> Hmph, it does look easier to read.  I think we used to have some\n> outlier files that indented CPP directives by prefixing spaces in\n> front of the whole line, but these days we standardized to express\n> the indentation by inserting spaces immediately after '#' that\n> always sit at the beginning of line, so what you showed here is a\n> good example to mimic.\n\nNo problem, I sent V3 with the suggested changes. That is actually my\npreferred why of indenting preprocessor directives. But I saw a mix if\nCPP indenting, so I was unsure what was correct. I guess I could have\nlooked harder for a style guide, but at least hopefully I followed\n'SubmittingPatches' mostly correct. :)\n\nCollin\n"},{"id":"517341","messageId":"aBoNbDgHncAeGW4e@pks.im","threadId":"63386","inReplyTo":"87ikmemtd8.fsf@gmail.com","subject":"Re: [PATCH v2] wrapper: NetBSD gives EFTYPE and FreeBSD gives EMFILE where POSIX uses ELOOP","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-06T13:23:56Z","receivedAt":"2025-05-06T13:24:01Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, May 05, 2025 at 06:16:03PM -0700, Collin Funk wrote:\n> Hi all,\n> \n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> >> Nit, to make this a bit easier to read: our style guide says that nested\n> >> preprocessor directives should be indented by one spaces. So this would\n> >> become:\n> >>\n> >>     # ifdef __NetBSD__\n> >>     #  define SYMLINK_ERRNO EFTYPE\n> >>     # elif defined(__FreeBSD__)\n> >>     #  define SYMLINK_ERRNO EMLINK\n> >>     # endif\n> >>\n> >> Note that the `ifdef` itself would also be indented because we already\n> >> have a surrounding `#ifdef O_NOFOLLOW`.\n> >\n> > Hmph, it does look easier to read.  I think we used to have some\n> > outlier files that indented CPP directives by prefixing spaces in\n> > front of the whole line, but these days we standardized to express\n> > the indentation by inserting spaces immediately after '#' that\n> > always sit at the beginning of line, so what you showed here is a\n> > good example to mimic.\n> \n> No problem, I sent V3 with the suggested changes. That is actually my\n> preferred why of indenting preprocessor directives. But I saw a mix if\n> CPP indenting, so I was unsure what was correct. I guess I could have\n> looked harder for a style guide, but at least hopefully I followed\n> 'SubmittingPatches' mostly correct. :)\n\nYeah, the rule was only introduced rather recently in 7df3f55b92e\n(Documentation: clarify indentation style for C preprocessor directives,\n2024-07-30), so we're still wildly inconsistent.\n\nPatrick\n"},{"id":"517342","messageId":"aBoNq8sih36ToGGb@pks.im","threadId":"63386","inReplyTo":"20250506010946.212068-1-collin.funk1@gmail.com","subject":"Re: [PATCH v3] wrapper: NetBSD gives EFTYPE and FreeBSD gives EMFILE where POSIX uses ELOOP","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-06T13:24:59Z","receivedAt":"2025-05-06T13:25:03Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, May 05, 2025 at 06:08:59PM -0700, Collin Funk wrote:\n> As documented on NetBSD's man page, open with the O_NOFOLLOW flag and a\n> symlink returns -1 and sets errno to EFTYPE which differs from POSIX.\n> \n> This patch fixes the following test failure:\n> \n>     $ sh t0602-reffiles-fsck.sh --verbose\n>     --- expect\t2025-05-02 23:05:23.920890147 +0000\n>     +++ err\t2025-05-02 23:05:23.916794959 +0000\n>     @@ -1 +1 @@\n>     -error: packed-refs: badRefFiletype: not a regular file but a symlink\n>     +error: unable to open '.git/packed-refs': Inappropriate file type or format\n>     not ok 12 - the filetype of packed-refs should be checked\n> \n> FreeBSD has the same issue for EMLINK instead of EFTYPE.\n> \n> This portability issue was introduced in cfea2f2da8 (packed-backend:\n> check whether the \"packed-refs\" is regular file, 2025-02-28)\n\nThanks, this version addresses my nit.\n\nPatrick\n"},{"id":"517343","messageId":"aBoR8hrcpK6CzbA3@ArchLinux","threadId":"63386","inReplyTo":"20250505180311.GA29783@coredump.intra.peff.net","subject":"Re: [PATCH] wrapper: Fix a errno discrepancy on NetBSD.","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-05-06T13:43:14Z","receivedAt":"2025-05-06T13:42:52Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Mon, May 05, 2025 at 02:03:11PM -0400, Jeff King wrote:\n> On Mon, May 05, 2025 at 08:43:18AM -0700, Junio C Hamano wrote:\n> \n> > But for other kind of requirements, we want to fulfill them on all\n> > platforms that we claim to support.  Using open_nofollow() to\n> > achieve hard atomicity requirement would be a bug in such a\n> > situation.  Should we somehow warn our developers against its use?\n> \n> The comment above the declaration says:\n> \n>   /*\n>    * Open with O_NOFOLLOW, or equivalent. Note that the fallback equivalent\n>    * may be racy. Do not use this as protection against an attacker who can\n>    * simultaneously create paths.\n>    */\n>   int open_nofollow(const char *path, int flags);\n> \n> though that may not be enough. 00611d8440 (add open_nofollow() helper,\n> 2021-02-16) discusses a way that it could be made less racy, at a\n> slightly increased cost.\n> \n> IMHO that is somewhat orthogonal to the issue here, though, which is\n> purely about the case where O_NOFOLLOW does exist (ironically, our\n> racy fallback code consistently returns ELOOP ;) ).\n> \n> The issue at hand is that particular errno responses are not always\n> portable. The patch discussed here improves that. My point was more that\n> I'm not sure to what degree we should care about errno consistency in\n> our wrappers (which is inherently a bit whack-a-mole as we find new\n> cases), versus trying not to care too hard about specific errno values\n> in calling code.\n> \n> I can see arguments either way (and as I said, an argument for making\n> errno values consistent even if we try to rely on them less). Mostly I\n> was just a little surprised to see open_nofollow() being used in this\n> way (especially since we have to end up stat()-ing anyway to check for\n> other cases).\n> \n\nIIRC, we wanted to try our best to make our code consistent. In the very\nearly implementation, I actually firstly checked the file type and then\nopened the file.\n\nHowever, there is a chance that the raw \"packed-refs\" file could be\nconverted to symlink between checking the filetype and opening the file\nto get the fd. Although, in fsck, we may just ignore this. But during\nthe review, I found out that using \"open_nofollow\" could avoid race in\nsome platforms. Sadly, I haven't realized that this would break\ncompatibility ;)\n\nBecause using \"open_nofollow\" could only check whether the filetype is\nthe symlink, we also need to use \"stat\" again to check whether the\nfiletype is OK. I agree that it is a little redundant.\n\nSince the patch from Collin would solve the problem. I won't change the\nlogic. I'll focus on using `mmap` to open the \"packed-refs\" file.\n\n> -Peff\n\nThanks,\nJialuo\n"},{"id":"517403","messageId":"xmqqzffpe48u.fsf@gitster.g","threadId":"63386","inReplyTo":"20250505180311.GA29783@coredump.intra.peff.net","subject":"Re: [PATCH] wrapper: Fix a errno discrepancy on NetBSD.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-06T22:58:09Z","receivedAt":"2025-05-06T22:58:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Mon, May 05, 2025 at 08:43:18AM -0700, Junio C Hamano wrote:\n>\n>> But for other kind of requirements, we want to fulfill them on all\n>> platforms that we claim to support.  Using open_nofollow() to\n>> achieve hard atomicity requirement would be a bug in such a\n>> situation.  Should we somehow warn our developers against its use?\n>\n> The comment above the declaration says:\n>\n>   /*\n>    * Open with O_NOFOLLOW, or equivalent. Note that the fallback equivalent\n>    * may be racy. Do not use this as protection against an attacker who can\n>    * simultaneously create paths.\n>    */\n>   int open_nofollow(const char *path, int flags);\n>\n> though that may not be enough. 00611d8440 (add open_nofollow() helper,\n> 2021-02-16) discusses a way that it could be made less racy, at a\n> slightly increased cost.\n>\n> IMHO that is somewhat orthogonal to the issue here, though, which is\n> purely about the case where O_NOFOLLOW does exist (ironically, our\n> racy fallback code consistently returns ELOOP ;) ).\n\nYup.  And with the above comment, I would say that my worries above\nare unfounded.\n\n> The issue at hand is that particular errno responses are not always\n> portable. The patch discussed here improves that. My point was more that\n> I'm not sure to what degree we should care about errno consistency in\n> our wrappers (which is inherently a bit whack-a-mole as we find new\n> cases), versus trying not to care too hard about specific errno values\n> in calling code.\n\nYeah, it does give me a bad aftertaste having to pretend (by adding\ncompat code to translate as needed) that all systems share the same\nset of errno, but we live in not-so-ideal world, so I am afraild\nthat it cannot be avoided.\n\n> I can see arguments either way (and as I said, an argument for making\n> errno values consistent even if we try to rely on them less). Mostly I\n> was just a little surprised to see open_nofollow() being used in this\n> way (especially since we have to end up stat()-ing anyway to check for\n> other cases).\n\nThat is true.\n\nThe callers in attr.c and dir.c do want to fstat() after they open,\nbut they are more interested in atomicity (with \"the best effort on\nless capable systems\" attitude).  The one in mailmap.c doesn't do\nany stat(), but again it is more about atomisity with the same\nattitude, I think.\n"}]}