{"thread":{"id":"61887","subject":"[PATCH 0/3] Small fixes for issues detected during internal CI runs","startedAt":"2024-08-02T04:10:57Z","lastAt":"2024-08-06T07:04:48Z","messageCount":29,"participants":["Kyle Lippincott via GitGitGadget","Patrick Steinhardt","Kyle Lippincott","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"499925","messageId":"pull.1756.git.git.1722571853.gitgitgadget@gmail.com","threadId":"61887","inReplyTo":null,"subject":"[PATCH 0/3] Small fixes for issues detected during internal CI runs","fromName":"Kyle Lippincott via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-08-02T04:10:50Z","receivedAt":"2024-08-02T04:10:57Z","isPatch":true,"sender":{"key":"spectral@google.com","avatar":"https://avatars.githubusercontent.com/u/6371650?v=4"},"body":"I'm attempting to get the git test suite running automatically during our\nweekly import. I have this mostly working, including with Address Sanitizer\nand Memory Sanitizer, but ran into a few issues:\n\n * several tests were failing due to strbuf_getcwd not clearing errno on\n success after it internally looped due to the path being >128 bytes. This\n is resolved in depth; though either one of the commits alone would\n resolve our issues:\n * modify locations that call strtoX and check for ERANGE to set errno =\n 0; prior to calling the conversion function. This is the typical way\n that these functions are invoked, and may indicate that we want\n compatibility helpers in git-compat-util.h to ensure that this happens\n correctly (and add these functions to the banned list).\n * have strbuf_getcwd set errno = 0; prior to a successful exit. This\n isn't very common for most functions in the codebase, but some other\n examples of this were found.\n * t6421-merge-partial-clone.sh had >10% flakiness. This is due to our build\n system using paths that contain a 64-hex-char hash, which had a 12.5%\n chance of containing the substring d0.\n\nKyle Lippincott (3):\n set errno=0 before strtoX calls\n strbuf: set errno to 0 after strbuf_getcwd\n t6421: fix test to work when repo dir contains d0\n\n builtin/get-tar-commit-id.c | 1 +\n ref-filter.c | 1 +\n strbuf.c | 1 +\n t/helper/test-json-writer.c | 2 ++\n t/helper/test-trace2.c | 1 +\n t/t6421-merge-partial-clone.sh | 6 +++---\n 6 files changed, 9 insertions(+), 3 deletions(-)\n\n\nbase-commit: e559c4bf1a306cf5814418d318cc0fea070da3c7\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1756%2Fspectral54%2Fstrbuf_getcwd-clear-errno-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1756/spectral54/strbuf_getcwd-clear-errno-v1\nPull-Request: https://github.com/git/git/pull/1756\n-- \ngitgitgadget\n"},{"id":"499926","messageId":"4dbd0bec40a0f9fd715e07a56bc6f12c4b29a83c.1722571853.git.gitgitgadget@gmail.com","threadId":"61887","inReplyTo":"pull.1756.git.git.1722571853.gitgitgadget@gmail.com","subject":"[PATCH 1/3] set errno=0 before strtoX calls","fromName":"Kyle Lippincott via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-08-02T04:10:51Z","receivedAt":"2024-08-02T04:10:57Z","isPatch":true,"sender":{"key":"spectral@google.com","avatar":"https://avatars.githubusercontent.com/u/6371650?v=4"},"body":"From: Kyle Lippincott \n\nTo detect conversion failure after calls to functions like `strtod`, one\ncan check `errno == ERANGE`. These functions are not guaranteed to set\n`errno` to `0` on successful conversion, however. Manual manipulation of\n`errno` can likely be avoided by checking that the output pointer\ndiffers from the input pointer, but that's not how other locations, such\nas parse.c:139, handle this issue; they set errno to 0 prior to\nexecuting the function.\n\nFor every place I could find a strtoX function with an ERANGE check\nfollowing it, set `errno = 0;` prior to executing the conversion\nfunction.\n\nSigned-off-by: Kyle Lippincott \n---\n builtin/get-tar-commit-id.c | 1 +\n ref-filter.c | 1 +\n t/helper/test-json-writer.c | 2 ++\n t/helper/test-trace2.c | 1 +\n 4 files changed, 5 insertions(+)\n\ndiff --git a/builtin/get-tar-commit-id.c b/builtin/get-tar-commit-id.c\nindex 66a7389f9f4..7195a072edc 100644\n--- a/builtin/get-tar-commit-id.c\n+++ b/builtin/get-tar-commit-id.c\n@@ -35,6 +35,7 @@ int cmd_get_tar_commit_id(int argc, const char **argv UNUSED, const char *prefix\n \tif (header->typeflag[0] != TYPEFLAG_GLOBAL_HEADER)\n \t\treturn 1;\n \n+\terrno = 0;\n \tlen = strtol(content, &end, 10);\n \tif (errno == ERANGE || end == content || len < 0)\n \t\treturn 1;\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 8c5e673fc0a..54880a2497a 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1628,6 +1628,7 @@ static void grab_date(const char *buf, struct atom_value *v, const char *atomnam\n \ttimestamp = parse_timestamp(eoemail + 2, &zone, 10);\n \tif (timestamp == TIME_MAX)\n \t\tgoto bad;\n+\terrno = 0;\n \ttz = strtol(zone, NULL, 10);\n \tif ((tz == LONG_MIN || tz == LONG_MAX) && errno == ERANGE)\n \t\tgoto bad;\ndiff --git a/t/helper/test-json-writer.c b/t/helper/test-json-writer.c\nindex ed52eb76bfc..a288069b04c 100644\n--- a/t/helper/test-json-writer.c\n+++ b/t/helper/test-json-writer.c\n@@ -415,6 +415,7 @@ static void get_i(struct line *line, intmax_t *s_in)\n \n \tget_s(line, &s);\n \n+\terrno = 0;\n \t*s_in = strtol(s, &endptr, 10);\n \tif (*endptr || errno == ERANGE)\n \t\tdie(\"line[%d]: invalid integer value\", line->nr);\n@@ -427,6 +428,7 @@ static void get_d(struct line *line, double *s_in)\n \n \tget_s(line, &s);\n \n+\terrno = 0;\n \t*s_in = strtod(s, &endptr);\n \tif (*endptr || errno == ERANGE)\n \t\tdie(\"line[%d]: invalid float value\", line->nr);\ndiff --git a/t/helper/test-trace2.c b/t/helper/test-trace2.c\nindex cd955ec63e9..c588c273ce7 100644\n--- a/t/helper/test-trace2.c\n+++ b/t/helper/test-trace2.c\n@@ -26,6 +26,7 @@ static int get_i(int *p_value, const char *data)\n \tif (!data || !*data)\n \t\treturn MyError;\n \n+\terrno = 0;\n \t*p_value = strtol(data, &endptr, 10);\n \tif (*endptr || errno == ERANGE)\n \t\treturn MyError;\n-- \ngitgitgadget\n\n"},{"id":"499927","messageId":"0ed09e9abb85e73a80d044c1ddaed303517752ac.1722571853.git.gitgitgadget@gmail.com","threadId":"61887","inReplyTo":"pull.1756.git.git.1722571853.gitgitgadget@gmail.com","subject":"[PATCH 2/3] strbuf: set errno to 0 after strbuf_getcwd","fromName":"Kyle Lippincott via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-08-02T04:10:52Z","receivedAt":"2024-08-02T04:10:58Z","isPatch":true,"sender":{"key":"spectral@google.com","avatar":"https://avatars.githubusercontent.com/u/6371650?v=4"},"body":"From: Kyle Lippincott \n\nIf the loop executes more than once due to cwd being longer than 128\nbytes, then `errno = ERANGE` might persist outside of this function.\nThis technically shouldn't be a problem, as all locations where the\nvalue in `errno` is tested should either (a) call a function that's\nguaranteed to set `errno` to 0 on success, or (b) set `errno` to 0 prior\nto calling the function that only conditionally sets errno, such as the\n`strtod` function. In the case of functions in category (b), it's easy\nto forget to do that.\n\nSet `errno = 0;` prior to exiting from `strbuf_getcwd` successfully.\nThis matches the behavior in functions like `run_transaction_hook`\n(refs.c:2176) and `read_ref_internal` (refs/files-backend.c:564).\n\nSigned-off-by: Kyle Lippincott \n---\n strbuf.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex 3d2189a7f64..b94ef040ab0 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -601,6 +601,7 @@ int strbuf_getcwd(struct strbuf *sb)\n \t\tstrbuf_grow(sb, guessed_len);\n \t\tif (getcwd(sb->buf, sb->alloc)) {\n \t\t\tstrbuf_setlen(sb, strlen(sb->buf));\n+\t\t\terrno = 0;\n \t\t\treturn 0;\n \t\t}\n \n-- \ngitgitgadget\n\n"},{"id":"499928","messageId":"6c08b8ceb2b87671a3e57c09e4e45170eaac37fc.1722571853.git.gitgitgadget@gmail.com","threadId":"61887","inReplyTo":"pull.1756.git.git.1722571853.gitgitgadget@gmail.com","subject":"[PATCH 3/3] t6421: fix test to work when repo dir contains d0","fromName":"Kyle Lippincott via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-08-02T04:10:53Z","receivedAt":"2024-08-02T04:10:59Z","isPatch":true,"sender":{"key":"spectral@google.com","avatar":"https://avatars.githubusercontent.com/u/6371650?v=4"},"body":"From: Kyle Lippincott \n\nThe `grep` statement in this test looks for `d0.*`, attempting\nto filter to only show lines that had tabular output where the 2nd\ncolumn had `d0` and the final column had a substring of\n[`git -c `]`fetch.negotiationAlgorithm`. These lines also have\n`child_start` in the 4th column, but this isn't part of the condition.\n\nA subsequent line will have `d1` in the 2nd column, `start` in the 4th\ncolumn, and `/path/to/git/git -c fetch.negotiationAlgorihm` in the final\ncolumn. If `/path/to/git/git` contains the substring `d0`, then this\nline is included by `grep` as well as the desired line, leading to an\neffective doubling of the number of lines, and test failures.\n\nTighten the grep expression to require `d0` to be surrounded by spaces,\nand to have the `child_start` label.\n\nSigned-off-by: Kyle Lippincott \n---\n t/t6421-merge-partial-clone.sh | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t6421-merge-partial-clone.sh b/t/t6421-merge-partial-clone.sh\nindex 711b709e755..0f312ac93dc 100755\n--- a/t/t6421-merge-partial-clone.sh\n+++ b/t/t6421-merge-partial-clone.sh\n@@ -231,7 +231,7 @@ test_expect_merge_algorithm failure success 'Objects downloaded for single relev\n \t\ttest_cmp expect actual &&\n \n \t\t# Check the number of fetch commands exec-ed\n-\t\tgrep d0.*fetch.negotiationAlgorithm trace.output >fetches &&\n+\t\tgrep \" d0 .* child_start .*fetch.negotiationAlgorithm\" trace.output >fetches &&\n \t\ttest_line_count = 2 fetches &&\n \n \t\tgit rev-list --objects --all --missing=print |\n@@ -319,7 +319,7 @@ test_expect_merge_algorithm failure success 'Objects downloaded when a directory\n \t\ttest_cmp expect actual &&\n \n \t\t# Check the number of fetch commands exec-ed\n-\t\tgrep d0.*fetch.negotiationAlgorithm trace.output >fetches &&\n+\t\tgrep \" d0 .* child_start .*fetch.negotiationAlgorithm\" trace.output >fetches &&\n \t\ttest_line_count = 1 fetches &&\n \n \t\tgit rev-list --objects --all --missing=print |\n@@ -423,7 +423,7 @@ test_expect_merge_algorithm failure success 'Objects downloaded with lots of ren\n \t\ttest_cmp expect actual &&\n \n \t\t# Check the number of fetch commands exec-ed\n-\t\tgrep d0.*fetch.negotiationAlgorithm trace.output >fetches &&\n+\t\tgrep \" d0 .* child_start .*fetch.negotiationAlgorithm\" trace.output >fetches &&\n \t\ttest_line_count = 4 fetches &&\n \n \t\tgit rev-list --objects --all --missing=print |\n-- \ngitgitgadget\n"},{"id":"499930","messageId":"ZqxqtIJi4-xBL9Sj@tanuki","threadId":"61887","inReplyTo":"4dbd0bec40a0f9fd715e07a56bc6f12c4b29a83c.1722571853.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/3] set errno=0 before strtoX calls","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-02T05:12:20Z","receivedAt":"2024-08-02T05:12:26Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Aug 02, 2024 at 04:10:51AM +0000, Kyle Lippincott via GitGitGadget wrote:\n> From: Kyle Lippincott \n> \n> To detect conversion failure after calls to functions like `strtod`, one\n> can check `errno == ERANGE`. These functions are not guaranteed to set\n> `errno` to `0` on successful conversion, however. Manual manipulation of\n> `errno` can likely be avoided by checking that the output pointer\n> differs from the input pointer, but that's not how other locations, such\n> as parse.c:139, handle this issue; they set errno to 0 prior to\n> executing the function.\n> \n> For every place I could find a strtoX function with an ERANGE check\n> following it, set `errno = 0;` prior to executing the conversion\n> function.\n\nMakes sense. I've also gone through callsites and couldn't spot any\nadditional ones that are broken.\n\nGenerally speaking, the interfaces provided by the `strtod()` family of\nfunctions is just plain awful, and ideally we wouldn't be using them in\nthe Git codebase at all without a wrapper. We already do have wrappers\nfor a subset of those functions, e.g. `strtol_i()`, which use an out\npointer to store the result and indicate success via the return value\ninstead of via `errno`.\n\nIt would be great if we could extend those wrappers to cover all of the\ninteger types, convert our code base to use them, and then extend our\n\"banned.h\" banner. I'm of course not asking you to do that in this patch\nseries.\n\nOut of curiosity, why do you hit those errors in your test setup? Do you\nuse a special libc that behaves differently than the most common ones?\n\nPatrick\n"},{"id":"499934","messageId":"CAO_smViSG27KrtE7hgq1GAzUYSoKFgrQymRYg-aKJqm4UW9DUg@mail.gmail.com","threadId":"61887","inReplyTo":"ZqxqtIJi4-xBL9Sj@tanuki","subject":"Re: [PATCH 1/3] set errno=0 before strtoX calls","fromName":"Kyle Lippincott","fromEmail":"spectral@google.com","sentAt":"2024-08-02T06:15:44Z","receivedAt":"2024-08-02T06:16:02Z","isPatch":true,"sender":{"key":"spectral@google.com","avatar":"https://avatars.githubusercontent.com/u/6371650?v=4"},"body":"On Thu, Aug 1, 2024 at 10:12 PM Patrick Steinhardt wrote:\n>\n> On Fri, Aug 02, 2024 at 04:10:51AM +0000, Kyle Lippincott via GitGitGadget wrote:\n> > From: Kyle Lippincott \n> >\n> > To detect conversion failure after calls to functions like `strtod`, one\n> > can check `errno == ERANGE`. These functions are not guaranteed to set\n> > `errno` to `0` on successful conversion, however. Manual manipulation of\n> > `errno` can likely be avoided by checking that the output pointer\n> > differs from the input pointer, but that's not how other locations, such\n> > as parse.c:139, handle this issue; they set errno to 0 prior to\n> > executing the function.\n> >\n> > For every place I could find a strtoX function with an ERANGE check\n> > following it, set `errno = 0;` prior to executing the conversion\n> > function.\n>\n> Makes sense. I've also gone through callsites and couldn't spot any\n> additional ones that are broken.\n>\n> Generally speaking, the interfaces provided by the `strtod()` family of\n> functions is just plain awful, and ideally we wouldn't be using them in\n> the Git codebase at all without a wrapper. We already do have wrappers\n> for a subset of those functions, e.g. `strtol_i()`, which use an out\n> pointer to store the result and indicate success via the return value\n> instead of via `errno`.\n>\n> It would be great if we could extend those wrappers to cover all of the\n> integer types, convert our code base to use them, and then extend our\n> \"banned.h\" banner. I'm of course not asking you to do that in this patch\n> series.\n>\n> Out of curiosity, why do you hit those errors in your test setup? Do you\n> use a special libc that behaves differently than the most common ones?\n\nThe second patch in this series fixes the original reason I noticed\nthe issues in three of the files: our remote test execution service\nuses paths that are >128 bytes long, so the getcwd call in\nstrbuf_getcwd was returning ERANGE once, and then it remained set\nsince getcwd didn't clear it on success. ref-filter.c was found via\nsearching, I think that was during the search for `ERANGE`.\n\n>\n> Patrick\n"},{"id":"499949","messageId":"xmqq34nngea0.fsf@gitster.g","threadId":"61887","inReplyTo":"ZqxqtIJi4-xBL9Sj@tanuki","subject":"Re: [PATCH 1/3] set errno=0 before strtoX calls","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-02T15:01:11Z","receivedAt":"2024-08-02T15:01:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> It would be great if we could extend those wrappers to cover all of the\n> integer types, convert our code base to use them, and then extend our\n> \"banned.h\" banner. I'm of course not asking you to do that in this patch\n> series.\n\nA good #leftoverbits material.\n\n> Out of curiosity, why do you hit those errors in your test setup? Do you\n> use a special libc that behaves differently than the most common ones?\n\n;-)\n"},{"id":"499950","messageId":"xmqqv80jeza5.fsf@gitster.g","threadId":"61887","inReplyTo":"0ed09e9abb85e73a80d044c1ddaed303517752ac.1722571853.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/3] strbuf: set errno to 0 after strbuf_getcwd","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-02T15:10:26Z","receivedAt":"2024-08-02T15:10:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kyle Lippincott via GitGitGadget\" writes:\n\n> Set `errno = 0;` prior to exiting from `strbuf_getcwd` successfully.\n> This matches the behavior in functions like `run_transaction_hook`\n> (refs.c:2176) and `read_ref_internal` (refs/files-backend.c:564).\n\nThis deep in the call chain, there is nothing that assures us that\nthe caller of this function does not care about the error before\nentering this function, so I feel a bit uneasy about the approach,\nand my initial reaction was \"wouldn't it be safer to do the usual\n\n\tint saved_errno = errno;\n\n\tfor (guessed_len = 128;; guessed_len *= 2) {\n\t\t... do things ...\n\t\tif (...) {\n\t\t\t... happy ...\n\t\t\terrno = saved_errno;\n\t\t\treturn 0;\n\t\t}\n\t}\n\npattern.\n\nWho calls this function, and inspects errno when this function\nreturns 0? I do not mind adding the \"save and restore\" fix to this\nfunction, but if there is a caller that looks at errno from a call\nthat returns success, that caller may also have to be looked at and\nfixed if necessary.\n\nThanks.\n\n> Signed-off-by: Kyle Lippincott \n> ---\n> strbuf.c | 1 +\n> 1 file changed, 1 insertion(+)\n>\n> diff --git a/strbuf.c b/strbuf.c\n> index 3d2189a7f64..b94ef040ab0 100644\n> --- a/strbuf.c\n> +++ b/strbuf.c\n> @@ -601,6 +601,7 @@ int strbuf_getcwd(struct strbuf *sb)\n> \t\tstrbuf_grow(sb, guessed_len);\n> \t\tif (getcwd(sb->buf, sb->alloc)) {\n> \t\t\tstrbuf_setlen(sb, strlen(sb->buf));\n> +\t\t\terrno = 0;\n> \t\t\treturn 0;\n> \t\t}\n"},{"id":"499951","messageId":"xmqqplqrez4u.fsf@gitster.g","threadId":"61887","inReplyTo":"6c08b8ceb2b87671a3e57c09e4e45170eaac37fc.1722571853.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/3] t6421: fix test to work when repo dir contains d0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-02T15:13:37Z","receivedAt":"2024-08-02T15:13:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kyle Lippincott via GitGitGadget\" writes:\n\n> From: Kyle Lippincott \n>\n> The `grep` statement in this test looks for `d0.*`, attempting\n> to filter to only show lines that had tabular output where the 2nd\n> column had `d0` and the final column had a substring of\n> [`git -c `]`fetch.negotiationAlgorithm`. These lines also have\n> `child_start` in the 4th column, but this isn't part of the condition.\n>\n> A subsequent line will have `d1` in the 2nd column, `start` in the 4th\n> column, and `/path/to/git/git -c fetch.negotiationAlgorihm` in the final\n> column. If `/path/to/git/git` contains the substring `d0`, then this\n> line is included by `grep` as well as the desired line, leading to an\n> effective doubling of the number of lines, and test failures.\n>\n> Tighten the grep expression to require `d0` to be surrounded by spaces,\n> and to have the `child_start` label.\n\nMakes sense.\n\nUpdating the comment with expected shape of the output might make it\neven less likely that we'd break these fixes again by mistake.\n\nThanks.\n\n> Signed-off-by: Kyle Lippincott \n> ---\n> t/t6421-merge-partial-clone.sh | 6 +++---\n> 1 file changed, 3 insertions(+), 3 deletions(-)\n>\n> diff --git a/t/t6421-merge-partial-clone.sh b/t/t6421-merge-partial-clone.sh\n> index 711b709e755..0f312ac93dc 100755\n> --- a/t/t6421-merge-partial-clone.sh\n> +++ b/t/t6421-merge-partial-clone.sh\n> @@ -231,7 +231,7 @@ test_expect_merge_algorithm failure success 'Objects downloaded for single relev\n> \t\ttest_cmp expect actual &&\n> \n> \t\t# Check the number of fetch commands exec-ed\n> -\t\tgrep d0.*fetch.negotiationAlgorithm trace.output >fetches &&\n> +\t\tgrep \" d0 .* child_start .*fetch.negotiationAlgorithm\" trace.output >fetches &&\n> \t\ttest_line_count = 2 fetches &&\n> \n> \t\tgit rev-list --objects --all --missing=print |\n> @@ -319,7 +319,7 @@ test_expect_merge_algorithm failure success 'Objects downloaded when a directory\n> \t\ttest_cmp expect actual &&\n> \n> \t\t# Check the number of fetch commands exec-ed\n> -\t\tgrep d0.*fetch.negotiationAlgorithm trace.output >fetches &&\n> +\t\tgrep \" d0 .* child_start .*fetch.negotiationAlgorithm\" trace.output >fetches &&\n> \t\ttest_line_count = 1 fetches &&\n> \n> \t\tgit rev-list --objects --all --missing=print |\n> @@ -423,7 +423,7 @@ test_expect_merge_algorithm failure success 'Objects downloaded with lots of ren\n> \t\ttest_cmp expect actual &&\n> \n> \t\t# Check the number of fetch commands exec-ed\n> -\t\tgrep d0.*fetch.negotiationAlgorithm trace.output >fetches &&\n> +\t\tgrep \" d0 .* child_start .*fetch.negotiationAlgorithm\" trace.output >fetches &&\n> \t\ttest_line_count = 4 fetches &&\n> \n> \t\tgit rev-list --objects --all --missing=print |\n"},{"id":"499964","messageId":"CAO_smVh-16cfWDOq_XwNHpov7coufu-m-buexz86+MBYnFb3YA@mail.gmail.com","threadId":"61887","inReplyTo":"xmqqv80jeza5.fsf@gitster.g","subject":"Re: [PATCH 2/3] strbuf: set errno to 0 after strbuf_getcwd","fromName":"Kyle Lippincott","fromEmail":"spectral@google.com","sentAt":"2024-08-02T17:56:17Z","receivedAt":"2024-08-02T17:56:32Z","isPatch":true,"sender":{"key":"spectral@google.com","avatar":"https://avatars.githubusercontent.com/u/6371650?v=4"},"body":"On Fri, Aug 2, 2024 at 8:10 AM Junio C Hamano wrote:\n>\n> \"Kyle Lippincott via GitGitGadget\" writes:\n>\n> > Set `errno = 0;` prior to exiting from `strbuf_getcwd` successfully.\n> > This matches the behavior in functions like `run_transaction_hook`\n> > (refs.c:2176) and `read_ref_internal` (refs/files-backend.c:564).\n>\n> This deep in the call chain, there is nothing that assures us that\n> the caller of this function does not care about the error before\n> entering this function, so I feel a bit uneasy about the approach,\n> and my initial reaction was \"wouldn't it be safer to do the usual\n>\n> int saved_errno = errno;\n>\n> for (guessed_len = 128;; guessed_len *= 2) {\n> ... do things ...\n> if (...) {\n> ... happy ...\n> errno = saved_errno;\n> return 0;\n> }\n> }\n>\n> pattern.\n>\n> Who calls this function, and inspects errno when this function\n> returns 0?\n\nThat's a difficult question to answer if you want to be wholistic for\nthe whole program :) For immediate callers:\n- unix_sockaddr_init: doesn't inspect or adjust errno itself if\nstrbuf_getcwd returns 0. Continues on to call other functions that may\nset errno.\n- strbuf_realpath_1: same\n- chdir_notify: same\n- discover_git_directory_reason: same\n- setup_git_directory_gently: same\n- setup_enlistment_directory (in scalar.c): dies immediately if\nstrbuf_getcwd returns < 0, otherwise same\n- xgetcwd: also doesn't inspect/adjust errno if strbuf_getcwd returns\n0. Doesn't call any other functions afterward (besides strbuf\nfunctions).\n- main: stores the value if strbuf_getcwd returns 0, but doesn't inspect errno.\n\n> I do not mind adding the \"save and restore\" fix to this\n> function, but if there is a caller that looks at errno from a call\n> that returns success, that caller may also have to be looked at and\n> fixed if necessary.\n\nThere aren't any that I could find, this patch is mostly a\ndefense-in-depth solution to the strtoX functions that were fixed in\npatch 1. This function _may_ set errno even on success. That errno\nvalue ends up retained indefinitely as long as things continue\nsucceeding, and then we call a function like `strtod` which has a\nsuboptimal interface. If this patch doesn't land, the codebase is\nstill correct; the main reason to want to land this is that without\nthis patch, any user that has paths longer than 128 bytes becomes de\nfacto responsible for finding and reporting/fixing issues that arise\nfrom this errno value being persisted, and I was hoping I wouldn't be\nsigning the people maintaining CI at $JOB up for that :) It's not an\nobvious failure, either. For example, t0211's failure, prior to\nsetting errno to 0 just before calling strtoX is just: `fatal: expect\n`. That's not easy to trace back to \"strbuf_getcwd sets\nERANGE in errno in our environment, so this is a misuse of a strtoX or\nparse_timestamp function\".\n\n>\n> Thanks.\n>\n> > Signed-off-by: Kyle Lippincott \n> > ---\n> > strbuf.c | 1 +\n> > 1 file changed, 1 insertion(+)\n> >\n> > diff --git a/strbuf.c b/strbuf.c\n> > index 3d2189a7f64..b94ef040ab0 100644\n> > --- a/strbuf.c\n> > +++ b/strbuf.c\n> > @@ -601,6 +601,7 @@ int strbuf_getcwd(struct strbuf *sb)\n> > strbuf_grow(sb, guessed_len);\n> > if (getcwd(sb->buf, sb->alloc)) {\n> > strbuf_setlen(sb, strlen(sb->buf));\n> > + errno = 0;\n> > return 0;\n> > }\n"},{"id":"499974","messageId":"pull.1756.v2.git.git.1722632287.gitgitgadget@gmail.com","threadId":"61887","inReplyTo":"pull.1756.git.git.1722571853.gitgitgadget@gmail.com","subject":"[PATCH v2 0/3] Small fixes for issues detected during internal CI runs","fromName":"Kyle Lippincott via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-08-02T20:58:04Z","receivedAt":"2024-08-02T20:58:11Z","isPatch":true,"sender":{"key":"spectral@google.com","avatar":"https://avatars.githubusercontent.com/u/6371650?v=4"},"body":"I'm attempting to get the git test suite running automatically during our\nweekly import. I have this mostly working, including with Address Sanitizer\nand Memory Sanitizer, but ran into a few issues:\n\n * several tests were failing due to strbuf_getcwd not clearing errno on\n success after it internally looped due to the path being >128 bytes. This\n is resolved in depth; though either one of the commits alone would\n resolve our issues:\n * modify locations that call strtoX and check for ERANGE to set errno =\n 0; prior to calling the conversion function. This is the typical way\n that these functions are invoked, and may indicate that we want\n compatibility helpers in git-compat-util.h to ensure that this happens\n correctly (and add these functions to the banned list).\n * have strbuf_getcwd set errno = 0; prior to a successful exit. This\n isn't very common for most functions in the codebase, but some other\n examples of this were found.\n * t6421-merge-partial-clone.sh had >10% flakiness. This is due to our build\n system using paths that contain a 64-hex-char hash, which had a 12.5%\n chance of containing the substring d0.\n\nKyle Lippincott (3):\n set errno=0 before strtoX calls\n strbuf: set errno to 0 after strbuf_getcwd\n t6421: fix test to work when repo dir contains d0\n\n builtin/get-tar-commit-id.c | 1 +\n ref-filter.c | 1 +\n strbuf.c | 1 +\n t/helper/test-json-writer.c | 2 ++\n t/helper/test-trace2.c | 1 +\n t/t6421-merge-partial-clone.sh | 15 +++++++++------\n 6 files changed, 15 insertions(+), 6 deletions(-)\n\n\nbase-commit: e559c4bf1a306cf5814418d318cc0fea070da3c7\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1756%2Fspectral54%2Fstrbuf_getcwd-clear-errno-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1756/spectral54/strbuf_getcwd-clear-errno-v2\nPull-Request: https://github.com/git/git/pull/1756\n\nRange-diff vs v1:\n\n 1: 4dbd0bec40a = 1: 4dbd0bec40a set errno=0 before strtoX calls\n 2: 0ed09e9abb8 = 2: 0ed09e9abb8 strbuf: set errno to 0 after strbuf_getcwd\n 3: 6c08b8ceb2b ! 3: 818dc9e6b3e t6421: fix test to work when repo dir contains d0\n @@ Commit message\n \n ## t/t6421-merge-partial-clone.sh ##\n @@ t/t6421-merge-partial-clone.sh: test_expect_merge_algorithm failure success 'Objects downloaded for single relev\n + \t\tgrep fetch_count trace.output | cut -d \"|\" -f 9 | tr -d \" .\" >actual &&\n \t\ttest_cmp expect actual &&\n \n - \t\t# Check the number of fetch commands exec-ed\n +-\t\t# Check the number of fetch commands exec-ed\n -\t\tgrep d0.*fetch.negotiationAlgorithm trace.output >fetches &&\n ++\t\t# Check the number of fetch commands exec-ed by filtering trace to\n ++\t\t# child_start events by the top-level program (2nd field == d0)\n +\t\tgrep \" d0 .* child_start .*fetch.negotiationAlgorithm\" trace.output >fetches &&\n \t\ttest_line_count = 2 fetches &&\n \n \t\tgit rev-list --objects --all --missing=print |\n @@ t/t6421-merge-partial-clone.sh: test_expect_merge_algorithm failure success 'Objects downloaded when a directory\n + \t\tgrep fetch_count trace.output | cut -d \"|\" -f 9 | tr -d \" .\" >actual &&\n \t\ttest_cmp expect actual &&\n \n - \t\t# Check the number of fetch commands exec-ed\n +-\t\t# Check the number of fetch commands exec-ed\n -\t\tgrep d0.*fetch.negotiationAlgorithm trace.output >fetches &&\n ++\t\t# Check the number of fetch commands exec-ed by filtering trace to\n ++\t\t# child_start events by the top-level program (2nd field == d0)\n +\t\tgrep \" d0 .* child_start .*fetch.negotiationAlgorithm\" trace.output >fetches &&\n \t\ttest_line_count = 1 fetches &&\n \n \t\tgit rev-list --objects --all --missing=print |\n @@ t/t6421-merge-partial-clone.sh: test_expect_merge_algorithm failure success 'Objects downloaded with lots of ren\n + \t\tgrep fetch_count trace.output | cut -d \"|\" -f 9 | tr -d \" .\" >actual &&\n \t\ttest_cmp expect actual &&\n \n - \t\t# Check the number of fetch commands exec-ed\n +-\t\t# Check the number of fetch commands exec-ed\n -\t\tgrep d0.*fetch.negotiationAlgorithm trace.output >fetches &&\n ++\t\t# Check the number of fetch commands exec-ed by filtering trace to\n ++\t\t# child_start events by the top-level program (2nd field == d0)\n +\t\tgrep \" d0 .* child_start .*fetch.negotiationAlgorithm\" trace.output >fetches &&\n \t\ttest_line_count = 4 fetches &&\n \n\n-- \ngitgitgadget\n"},{"id":"499975","messageId":"4dbd0bec40a0f9fd715e07a56bc6f12c4b29a83c.1722632287.git.gitgitgadget@gmail.com","threadId":"61887","inReplyTo":"pull.1756.v2.git.git.1722632287.gitgitgadget@gmail.com","subject":"[PATCH v2 1/3] set errno=0 before strtoX calls","fromName":"Kyle Lippincott via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-08-02T20:58:05Z","receivedAt":"2024-08-02T20:58:11Z","isPatch":true,"sender":{"key":"spectral@google.com","avatar":"https://avatars.githubusercontent.com/u/6371650?v=4"},"body":"From: Kyle Lippincott \n\nTo detect conversion failure after calls to functions like `strtod`, one\ncan check `errno == ERANGE`. These functions are not guaranteed to set\n`errno` to `0` on successful conversion, however. Manual manipulation of\n`errno` can likely be avoided by checking that the output pointer\ndiffers from the input pointer, but that's not how other locations, such\nas parse.c:139, handle this issue; they set errno to 0 prior to\nexecuting the function.\n\nFor every place I could find a strtoX function with an ERANGE check\nfollowing it, set `errno = 0;` prior to executing the conversion\nfunction.\n\nSigned-off-by: Kyle Lippincott \n---\n builtin/get-tar-commit-id.c | 1 +\n ref-filter.c | 1 +\n t/helper/test-json-writer.c | 2 ++\n t/helper/test-trace2.c | 1 +\n 4 files changed, 5 insertions(+)\n\ndiff --git a/builtin/get-tar-commit-id.c b/builtin/get-tar-commit-id.c\nindex 66a7389f9f4..7195a072edc 100644\n--- a/builtin/get-tar-commit-id.c\n+++ b/builtin/get-tar-commit-id.c\n@@ -35,6 +35,7 @@ int cmd_get_tar_commit_id(int argc, const char **argv UNUSED, const char *prefix\n \tif (header->typeflag[0] != TYPEFLAG_GLOBAL_HEADER)\n \t\treturn 1;\n \n+\terrno = 0;\n \tlen = strtol(content, &end, 10);\n \tif (errno == ERANGE || end == content || len < 0)\n \t\treturn 1;\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 8c5e673fc0a..54880a2497a 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1628,6 +1628,7 @@ static void grab_date(const char *buf, struct atom_value *v, const char *atomnam\n \ttimestamp = parse_timestamp(eoemail + 2, &zone, 10);\n \tif (timestamp == TIME_MAX)\n \t\tgoto bad;\n+\terrno = 0;\n \ttz = strtol(zone, NULL, 10);\n \tif ((tz == LONG_MIN || tz == LONG_MAX) && errno == ERANGE)\n \t\tgoto bad;\ndiff --git a/t/helper/test-json-writer.c b/t/helper/test-json-writer.c\nindex ed52eb76bfc..a288069b04c 100644\n--- a/t/helper/test-json-writer.c\n+++ b/t/helper/test-json-writer.c\n@@ -415,6 +415,7 @@ static void get_i(struct line *line, intmax_t *s_in)\n \n \tget_s(line, &s);\n \n+\terrno = 0;\n \t*s_in = strtol(s, &endptr, 10);\n \tif (*endptr || errno == ERANGE)\n \t\tdie(\"line[%d]: invalid integer value\", line->nr);\n@@ -427,6 +428,7 @@ static void get_d(struct line *line, double *s_in)\n \n \tget_s(line, &s);\n \n+\terrno = 0;\n \t*s_in = strtod(s, &endptr);\n \tif (*endptr || errno == ERANGE)\n \t\tdie(\"line[%d]: invalid float value\", line->nr);\ndiff --git a/t/helper/test-trace2.c b/t/helper/test-trace2.c\nindex cd955ec63e9..c588c273ce7 100644\n--- a/t/helper/test-trace2.c\n+++ b/t/helper/test-trace2.c\n@@ -26,6 +26,7 @@ static int get_i(int *p_value, const char *data)\n \tif (!data || !*data)\n \t\treturn MyError;\n \n+\terrno = 0;\n \t*p_value = strtol(data, &endptr, 10);\n \tif (*endptr || errno == ERANGE)\n \t\treturn MyError;\n-- \ngitgitgadget\n\n"},{"id":"499976","messageId":"0ed09e9abb85e73a80d044c1ddaed303517752ac.1722632287.git.gitgitgadget@gmail.com","threadId":"61887","inReplyTo":"pull.1756.v2.git.git.1722632287.gitgitgadget@gmail.com","subject":"[PATCH v2 2/3] strbuf: set errno to 0 after strbuf_getcwd","fromName":"Kyle Lippincott via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-08-02T20:58:06Z","receivedAt":"2024-08-02T20:58:12Z","isPatch":true,"sender":{"key":"spectral@google.com","avatar":"https://avatars.githubusercontent.com/u/6371650?v=4"},"body":"From: Kyle Lippincott \n\nIf the loop executes more than once due to cwd being longer than 128\nbytes, then `errno = ERANGE` might persist outside of this function.\nThis technically shouldn't be a problem, as all locations where the\nvalue in `errno` is tested should either (a) call a function that's\nguaranteed to set `errno` to 0 on success, or (b) set `errno` to 0 prior\nto calling the function that only conditionally sets errno, such as the\n`strtod` function. In the case of functions in category (b), it's easy\nto forget to do that.\n\nSet `errno = 0;` prior to exiting from `strbuf_getcwd` successfully.\nThis matches the behavior in functions like `run_transaction_hook`\n(refs.c:2176) and `read_ref_internal` (refs/files-backend.c:564).\n\nSigned-off-by: Kyle Lippincott \n---\n strbuf.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex 3d2189a7f64..b94ef040ab0 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -601,6 +601,7 @@ int strbuf_getcwd(struct strbuf *sb)\n \t\tstrbuf_grow(sb, guessed_len);\n \t\tif (getcwd(sb->buf, sb->alloc)) {\n \t\t\tstrbuf_setlen(sb, strlen(sb->buf));\n+\t\t\terrno = 0;\n \t\t\treturn 0;\n \t\t}\n \n-- \ngitgitgadget\n\n"},{"id":"499977","messageId":"818dc9e6b3e8a4d449cb9dbce689bfadb95099ff.1722632287.git.gitgitgadget@gmail.com","threadId":"61887","inReplyTo":"pull.1756.v2.git.git.1722632287.gitgitgadget@gmail.com","subject":"[PATCH v2 3/3] t6421: fix test to work when repo dir contains d0","fromName":"Kyle Lippincott via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-08-02T20:58:07Z","receivedAt":"2024-08-02T20:58:15Z","isPatch":true,"sender":{"key":"spectral@google.com","avatar":"https://avatars.githubusercontent.com/u/6371650?v=4"},"body":"From: Kyle Lippincott \n\nThe `grep` statement in this test looks for `d0.*`, attempting\nto filter to only show lines that had tabular output where the 2nd\ncolumn had `d0` and the final column had a substring of\n[`git -c `]`fetch.negotiationAlgorithm`. These lines also have\n`child_start` in the 4th column, but this isn't part of the condition.\n\nA subsequent line will have `d1` in the 2nd column, `start` in the 4th\ncolumn, and `/path/to/git/git -c fetch.negotiationAlgorihm` in the final\ncolumn. If `/path/to/git/git` contains the substring `d0`, then this\nline is included by `grep` as well as the desired line, leading to an\neffective doubling of the number of lines, and test failures.\n\nTighten the grep expression to require `d0` to be surrounded by spaces,\nand to have the `child_start` label.\n\nSigned-off-by: Kyle Lippincott \n---\n t/t6421-merge-partial-clone.sh | 15 +++++++++------\n 1 file changed, 9 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t6421-merge-partial-clone.sh b/t/t6421-merge-partial-clone.sh\nindex 711b709e755..b99f29ef9ba 100755\n--- a/t/t6421-merge-partial-clone.sh\n+++ b/t/t6421-merge-partial-clone.sh\n@@ -230,8 +230,9 @@ test_expect_merge_algorithm failure success 'Objects downloaded for single relev\n \t\tgrep fetch_count trace.output | cut -d \"|\" -f 9 | tr -d \" .\" >actual &&\n \t\ttest_cmp expect actual &&\n \n-\t\t# Check the number of fetch commands exec-ed\n-\t\tgrep d0.*fetch.negotiationAlgorithm trace.output >fetches &&\n+\t\t# Check the number of fetch commands exec-ed by filtering trace to\n+\t\t# child_start events by the top-level program (2nd field == d0)\n+\t\tgrep \" d0 .* child_start .*fetch.negotiationAlgorithm\" trace.output >fetches &&\n \t\ttest_line_count = 2 fetches &&\n \n \t\tgit rev-list --objects --all --missing=print |\n@@ -318,8 +319,9 @@ test_expect_merge_algorithm failure success 'Objects downloaded when a directory\n \t\tgrep fetch_count trace.output | cut -d \"|\" -f 9 | tr -d \" .\" >actual &&\n \t\ttest_cmp expect actual &&\n \n-\t\t# Check the number of fetch commands exec-ed\n-\t\tgrep d0.*fetch.negotiationAlgorithm trace.output >fetches &&\n+\t\t# Check the number of fetch commands exec-ed by filtering trace to\n+\t\t# child_start events by the top-level program (2nd field == d0)\n+\t\tgrep \" d0 .* child_start .*fetch.negotiationAlgorithm\" trace.output >fetches &&\n \t\ttest_line_count = 1 fetches &&\n \n \t\tgit rev-list --objects --all --missing=print |\n@@ -422,8 +424,9 @@ test_expect_merge_algorithm failure success 'Objects downloaded with lots of ren\n \t\tgrep fetch_count trace.output | cut -d \"|\" -f 9 | tr -d \" .\" >actual &&\n \t\ttest_cmp expect actual &&\n \n-\t\t# Check the number of fetch commands exec-ed\n-\t\tgrep d0.*fetch.negotiationAlgorithm trace.output >fetches &&\n+\t\t# Check the number of fetch commands exec-ed by filtering trace to\n+\t\t# child_start events by the top-level program (2nd field == d0)\n+\t\tgrep \" d0 .* child_start .*fetch.negotiationAlgorithm\" trace.output >fetches &&\n \t\ttest_line_count = 4 fetches &&\n \n \t\tgit rev-list --objects --all --missing=print |\n-- \ngitgitgadget\n"},{"id":"499980","messageId":"xmqqbk2abp3s.fsf@gitster.g","threadId":"61887","inReplyTo":"4dbd0bec40a0f9fd715e07a56bc6f12c4b29a83c.1722632287.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/3] set errno=0 before strtoX calls","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-02T21:18:31Z","receivedAt":"2024-08-02T21:18:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kyle Lippincott via GitGitGadget\" writes:\n\n> From: Kyle Lippincott \n>\n> To detect conversion failure after calls to functions like `strtod`, one\n> can check `errno == ERANGE`. These functions are not guaranteed to set\n> `errno` to `0` on successful conversion, however. Manual manipulation of\n> `errno` can likely be avoided by checking that the output pointer\n> differs from the input pointer, but that's not how other locations, such\n> as parse.c:139, handle this issue; they set errno to 0 prior to\n> executing the function.\n>\n> For every place I could find a strtoX function with an ERANGE check\n> following it, set `errno = 0;` prior to executing the conversion\n> function.\n>\n> Signed-off-by: Kyle Lippincott \n> ---\n> builtin/get-tar-commit-id.c | 1 +\n> ref-filter.c | 1 +\n> t/helper/test-json-writer.c | 2 ++\n> t/helper/test-trace2.c | 1 +\n> 4 files changed, 5 insertions(+)\n\nClearilng before strtoX() call like these changes make perfect sense\n(within the constraint of strtoX() API, which is horrible as pointed\nout by others many times in the past ;-)\n\nThanks, will queue.\n\n> diff --git a/builtin/get-tar-commit-id.c b/builtin/get-tar-commit-id.c\n> index 66a7389f9f4..7195a072edc 100644\n> --- a/builtin/get-tar-commit-id.c\n> +++ b/builtin/get-tar-commit-id.c\n> @@ -35,6 +35,7 @@ int cmd_get_tar_commit_id(int argc, const char **argv UNUSED, const char *prefix\n> \tif (header->typeflag[0] != TYPEFLAG_GLOBAL_HEADER)\n> \t\treturn 1;\n> \n> +\terrno = 0;\n> \tlen = strtol(content, &end, 10);\n> \tif (errno == ERANGE || end == content || len < 0)\n> \t\treturn 1;\n> diff --git a/ref-filter.c b/ref-filter.c\n> index 8c5e673fc0a..54880a2497a 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -1628,6 +1628,7 @@ static void grab_date(const char *buf, struct atom_value *v, const char *atomnam\n> \ttimestamp = parse_timestamp(eoemail + 2, &zone, 10);\n> \tif (timestamp == TIME_MAX)\n> \t\tgoto bad;\n> +\terrno = 0;\n> \ttz = strtol(zone, NULL, 10);\n> \tif ((tz == LONG_MIN || tz == LONG_MAX) && errno == ERANGE)\n> \t\tgoto bad;\n> diff --git a/t/helper/test-json-writer.c b/t/helper/test-json-writer.c\n> index ed52eb76bfc..a288069b04c 100644\n> --- a/t/helper/test-json-writer.c\n> +++ b/t/helper/test-json-writer.c\n> @@ -415,6 +415,7 @@ static void get_i(struct line *line, intmax_t *s_in)\n> \n> \tget_s(line, &s);\n> \n> +\terrno = 0;\n> \t*s_in = strtol(s, &endptr, 10);\n> \tif (*endptr || errno == ERANGE)\n> \t\tdie(\"line[%d]: invalid integer value\", line->nr);\n> @@ -427,6 +428,7 @@ static void get_d(struct line *line, double *s_in)\n> \n> \tget_s(line, &s);\n> \n> +\terrno = 0;\n> \t*s_in = strtod(s, &endptr);\n> \tif (*endptr || errno == ERANGE)\n> \t\tdie(\"line[%d]: invalid float value\", line->nr);\n> diff --git a/t/helper/test-trace2.c b/t/helper/test-trace2.c\n> index cd955ec63e9..c588c273ce7 100644\n> --- a/t/helper/test-trace2.c\n> +++ b/t/helper/test-trace2.c\n> @@ -26,6 +26,7 @@ static int get_i(int *p_value, const char *data)\n> \tif (!data || !*data)\n> \t\treturn MyError;\n> \n> +\terrno = 0;\n> \t*p_value = strtol(data, &endptr, 10);\n> \tif (*endptr || errno == ERANGE)\n> \t\treturn MyError;\n"},{"id":"499982","messageId":"xmqqv80ia9wf.fsf@gitster.g","threadId":"61887","inReplyTo":"0ed09e9abb85e73a80d044c1ddaed303517752ac.1722632287.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/3] strbuf: set errno to 0 after strbuf_getcwd","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-02T21:32:16Z","receivedAt":"2024-08-02T21:32:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kyle Lippincott via GitGitGadget\" writes:\n\n> From: Kyle Lippincott \n>\n> If the loop executes more than once due to cwd being longer than 128\n> bytes, then `errno = ERANGE` might persist outside of this function.\n> This technically shouldn't be a problem, as all locations where the\n> value in `errno` is tested should either (a) call a function that's\n> guaranteed to set `errno` to 0 on success, or (b) set `errno` to 0 prior\n> to calling the function that only conditionally sets errno, such as the\n> `strtod` function. In the case of functions in category (b), it's easy\n> to forget to do that.\n>\n> Set `errno = 0;` prior to exiting from `strbuf_getcwd` successfully.\n> This matches the behavior in functions like `run_transaction_hook`\n> (refs.c:2176) and `read_ref_internal` (refs/files-backend.c:564).\n\nI am still uneasy to see this unconditional clearing, which looks\nmore like spreading the bad practice from two places you identified\nthan following good behaviour modelled after these two places.\n\nBut I'll let it pass.\n\nAs long as our programmers understand that across strbuf_getcwd(),\nerrno will *not* be preserved, even if the function returns success,\nit would be OK. As the usual convention around errno is that a\nsuccessful call would leave errno intact, not clear it to 0, it\nwould make it a bit harder to learn our API for newcomers, though.\n\nThanks.\n\n> Signed-off-by: Kyle Lippincott \n> ---\n> strbuf.c | 1 +\n> 1 file changed, 1 insertion(+)\n>\n> diff --git a/strbuf.c b/strbuf.c\n> index 3d2189a7f64..b94ef040ab0 100644\n> --- a/strbuf.c\n> +++ b/strbuf.c\n> @@ -601,6 +601,7 @@ int strbuf_getcwd(struct strbuf *sb)\n> \t\tstrbuf_grow(sb, guessed_len);\n> \t\tif (getcwd(sb->buf, sb->alloc)) {\n> \t\t\tstrbuf_setlen(sb, strlen(sb->buf));\n> +\t\t\terrno = 0;\n> \t\t\treturn 0;\n> \t\t}\n"},{"id":"499983","messageId":"xmqqr0b6a9hp.fsf@gitster.g","threadId":"61887","inReplyTo":"818dc9e6b3e8a4d449cb9dbce689bfadb95099ff.1722632287.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 3/3] t6421: fix test to work when repo dir contains d0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-02T21:41:06Z","receivedAt":"2024-08-02T21:41:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kyle Lippincott via GitGitGadget\" writes:\n\n> From: Kyle Lippincott \n>\n> The `grep` statement in this test looks for `d0.*`, attempting\n> to filter to only show lines that had tabular output where the 2nd\n> column had `d0` and the final column had a substring of\n> [`git -c `]`fetch.negotiationAlgorithm`. These lines also have\n> `child_start` in the 4th column, but this isn't part of the condition.\n>\n> A subsequent line will have `d1` in the 2nd column, `start` in the 4th\n> column, and `/path/to/git/git -c fetch.negotiationAlgorihm` in the final\n> column. If `/path/to/git/git` contains the substring `d0`, then this\n> line is included by `grep` as well as the desired line, leading to an\n> effective doubling of the number of lines, and test failures.\n>\n> Tighten the grep expression to require `d0` to be surrounded by spaces,\n> and to have the `child_start` label.\n\nOK.\n\nI think I actually misinterpreted what you meant with these changes.\nIt is not what the patterns are picking. It is some _other_ trace\nentry we do not necessarily care about, like label:do_write_index\nthat has the path to the .git/index.lock file, that can accidentally\ncontain d0, that can be picked up with a pattern that is too loose.\nSo it really didn't have to clarify what it is looking for, as it\nwould not help seeing what false positives the patterns are designed\nto avoid matching. Sorry about that.\n\nWill queue.\n\n\n"},{"id":"499986","messageId":"CAPig+cTmzk7AN2x8-WCK_T5-_G7Wd-akB2++_4HFEbT67Rnc8A@mail.gmail.com","threadId":"61887","inReplyTo":"xmqqv80ia9wf.fsf@gitster.g","subject":"Re: [PATCH v2 2/3] strbuf: set errno to 0 after strbuf_getcwd","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-08-02T21:54:08Z","receivedAt":"2024-08-02T21:54:21Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Aug 2, 2024 at 5:32 PM Junio C Hamano wrote:\n> > [...]\n> > Set `errno = 0;` prior to exiting from `strbuf_getcwd` successfully.\n> > This matches the behavior in functions like `run_transaction_hook`\n> > (refs.c:2176) and `read_ref_internal` (refs/files-backend.c:564).\n>\n> I am still uneasy to see this unconditional clearing, which looks\n> more like spreading the bad practice from two places you identified\n> than following good behaviour modelled after these two places.\n>\n> But I'll let it pass.\n>\n> As long as our programmers understand that across strbuf_getcwd(),\n> errno will *not* be preserved, even if the function returns success,\n> it would be OK. As the usual convention around errno is that a\n> successful call would leave errno intact, not clear it to 0, it\n> would make it a bit harder to learn our API for newcomers, though.\n\nFor what it's worth, I share your misgivings about this change and\nconsider the suggestion[*] to make it save/restore `errno` upon\nsuccess more sensible. It would also be a welcome change to see the\nfunction documentation in strbuf.h updated to mention that it follows\nthe usual convention of leaving `errno` untouched upon success and\nclobbered upon error.\n\n[*]: https://lore.kernel.org/git/xmqqv80jeza5.fsf@gitster.g/\n"},{"id":"499994","messageId":"CAO_smVjYYaE3UZd0M28j+=uYMLdDPRAN08X1Yb_=5+nU4GrkSA@mail.gmail.com","threadId":"61887","inReplyTo":"xmqqv80ia9wf.fsf@gitster.g","subject":"Re: [PATCH v2 2/3] strbuf: set errno to 0 after strbuf_getcwd","fromName":"Kyle Lippincott","fromEmail":"spectral@google.com","sentAt":"2024-08-02T23:51:31Z","receivedAt":"2024-08-02T23:51:48Z","isPatch":true,"sender":{"key":"spectral@google.com","avatar":"https://avatars.githubusercontent.com/u/6371650?v=4"},"body":"On Fri, Aug 2, 2024 at 2:32 PM Junio C Hamano wrote:\n>\n> \"Kyle Lippincott via GitGitGadget\" writes:\n>\n> > From: Kyle Lippincott \n> >\n> > If the loop executes more than once due to cwd being longer than 128\n> > bytes, then `errno = ERANGE` might persist outside of this function.\n> > This technically shouldn't be a problem, as all locations where the\n> > value in `errno` is tested should either (a) call a function that's\n> > guaranteed to set `errno` to 0 on success, or (b) set `errno` to 0 prior\n> > to calling the function that only conditionally sets errno, such as the\n> > `strtod` function. In the case of functions in category (b), it's easy\n> > to forget to do that.\n> >\n> > Set `errno = 0;` prior to exiting from `strbuf_getcwd` successfully.\n> > This matches the behavior in functions like `run_transaction_hook`\n> > (refs.c:2176) and `read_ref_internal` (refs/files-backend.c:564).\n>\n> I am still uneasy to see this unconditional clearing, which looks\n> more like spreading the bad practice from two places you identified\n> than following good behaviour modelled after these two places.\n>\n> But I'll let it pass.\n>\n> As long as our programmers understand that across strbuf_getcwd(),\n> errno will *not* be preserved, even if the function returns success,\n> it would be OK. As the usual convention around errno is that a\n> successful call would leave errno intact, not clear it to 0, it\n> would make it a bit harder to learn our API for newcomers, though.\n\nI'm sympathetic to that argument. If you'd prefer to not have this\npatch, I'm fine with it not landing, and instead at some future date I\nmay try to work on those #leftoverbits from the previous patch (to\nmake a safer wrapper around strtoX, and ban the use of the unwrapped\nversions), or someone else can if they beat me to it.\n\nSince this is wrapping a posix function, and posix has things to say\nabout this (see below), I agree that it shouldn't set it to 0, and\nwithdraw this patch.\n\nI'm including my references below mostly because with the information\nI just acquired, I think that any attempt to _preserve_ errno is also\nfolly. No function we write, unless we explicitly state that it _will_\npreserve errno, should feel obligated to do so. The number of cases\nwhere errno _could_ be modified according to the various\nspecifications (C99 and posix) are just too numerous.\n\n---\n\nPerhaps because I'm not all that experienced with C, but when I did C\na couple decades ago, I operated in a mode where basically every\nfunction was actively hostile. If I wanted errno preserved across a\nfunction call, then it's up to me (the caller) to do so, regardless of\nwhat the current implementation of that function says will happen,\nbecause that can change at any point. Unless the function is\ndocumented as errno-preserving, I'm going to treat it as\nerrno-hostile. In practice, this didn't really matter much, as I've\nnever found `if (some_func()) { if (!some_other_func()) { /* use errno\nfrom `some_func` */ } }` logic to happen often, but maybe it does in\n\"real\" programs, I was just a hobbyist self-teaching at the time.\n\nThe C standard has a very precise definition of how the library\nfunctions defined in the C specification will act. It guarantees:\n- the library functions defined in the specification will never set errno to 0.\n- the library functions defined in the specification may set the value\nto non-zero whether an error occurs or not, \"provided the use of errno\nis not documented in the description of the function in this\nInternational Standard\". What this means is that (a) if the function\nas defined in the C standard mentions errno, it can only set the\nvalues as specified there, and (b) if the function as defined in the C\nstandard does _not_ mention errno, such as `fopen` or `strstr`, it can\ndo _whatever it wants_ to errno, even on success, _except_ set it to\n0.\n\nPOSIX has similar language\n(https://pubs.opengroup.org/onlinepubs/009695399/functions/errno.html),\nwith some key differences:\n- The value of errno should only be examined when it is indicated to\nbe valid by a function's return value.\n- The setting of errno after a successful call to a function is\nunspecified unless the description of that function specifies that\nerrno shall not be modified.\n\nThis means that unlike the C specification, which says that if a\nfunction doesn't describe its use of errno it can do anything it wants\nto errno [except set it to 0], in POSIX, a function can do anything it\nwants to errno [except set it to 0] at any time.\n\nWhat this means in practice is that errno should never be assumed to\nbe preserved across calls to posix functions (like getcwd). Also,\nstrbuf_getcwd calls free, malloc, and realloc, none of which mention\nerrno in the C specification, so they can do whatever they want to it\n[except set it to 0]. That I was able to find one single function that\nwas causing problems is luck, and not guaranteed by any specification.\n\nKind of makes me want to try writing an actively hostile C99 and POSIX\nenvironment, and see how many things break with it. :) C99 spec\ndoesn't say anything about malloc setting errno? Ok! malloc now sets\nerrno to ENOENT on tuesdays [in GMT because I'm not a monster], but\nonly on success. On any other day, it'll set it to ERANGE, regardless\nof success or failure.\n\n>\n> Thanks.\n>\n> > Signed-off-by: Kyle Lippincott \n> > ---\n> > strbuf.c | 1 +\n> > 1 file changed, 1 insertion(+)\n> >\n> > diff --git a/strbuf.c b/strbuf.c\n> > index 3d2189a7f64..b94ef040ab0 100644\n> > --- a/strbuf.c\n> > +++ b/strbuf.c\n> > @@ -601,6 +601,7 @@ int strbuf_getcwd(struct strbuf *sb)\n> > strbuf_grow(sb, guessed_len);\n> > if (getcwd(sb->buf, sb->alloc)) {\n> > strbuf_setlen(sb, strlen(sb->buf));\n> > + errno = 0;\n> > return 0;\n> > }\n"},{"id":"499995","messageId":"CAO_smVh-gQWy7xUJgFjd6gUWCTV5jTYJ9E9E3rvaQg0EY-2BdQ@mail.gmail.com","threadId":"61887","inReplyTo":"xmqqr0b6a9hp.fsf@gitster.g","subject":"Re: [PATCH v2 3/3] t6421: fix test to work when repo dir contains d0","fromName":"Kyle Lippincott","fromEmail":"spectral@google.com","sentAt":"2024-08-03T00:03:45Z","receivedAt":"2024-08-03T00:04:03Z","isPatch":true,"sender":{"key":"spectral@google.com","avatar":"https://avatars.githubusercontent.com/u/6371650?v=4"},"body":"On Fri, Aug 2, 2024 at 2:41 PM Junio C Hamano wrote:\n>\n> \"Kyle Lippincott via GitGitGadget\" writes:\n>\n> > From: Kyle Lippincott \n> >\n> > The `grep` statement in this test looks for `d0.*`, attempting\n> > to filter to only show lines that had tabular output where the 2nd\n> > column had `d0` and the final column had a substring of\n> > [`git -c `]`fetch.negotiationAlgorithm`. These lines also have\n> > `child_start` in the 4th column, but this isn't part of the condition.\n> >\n> > A subsequent line will have `d1` in the 2nd column, `start` in the 4th\n> > column, and `/path/to/git/git -c fetch.negotiationAlgorihm` in the final\n> > column. If `/path/to/git/git` contains the substring `d0`, then this\n> > line is included by `grep` as well as the desired line, leading to an\n> > effective doubling of the number of lines, and test failures.\n> >\n> > Tighten the grep expression to require `d0` to be surrounded by spaces,\n> > and to have the `child_start` label.\n>\n> OK.\n>\n> I think I actually misinterpreted what you meant with these changes.\n> It is not what the patterns are picking. It is some _other_ trace\n> entry we do not necessarily care about, like label:do_write_index\n> that has the path to the .git/index.lock file, that can accidentally\n> contain d0, that can be picked up with a pattern that is too loose.\n> So it really didn't have to clarify what it is looking for, as it\n> would not help seeing what false positives the patterns are designed\n> to avoid matching. Sorry about that.\n\nI would have included examples, but they're quite long (>>>80 chars),\nso seemed very out of place in both commit description and in the\ncodebase. With line wrapping, it wasn't very readable either. At the\nrisk of this also getting line-wrapped into unreadability:\n\ntest_line_count: line count for fetches != 1\n23:59:48.794453 run-command.c:733 | d0 | main\n | child_start | | 0.027328 | | |\n..........[ch1] class:? argv:[git -c fetch.negotiationAlgorithm=noop\nfetch origin --no-tags --no-write-fetch-head --recurse-submodules=no\n--filter=blob:none --stdin]\n23:59:48.798901 common-main.c:58 | d1 | main\n | start | | 0.000852 | | |\n/usr/local/google/home/spectral/src/oss/d0/git/git -c\nfetch.negotiationAlgorithm=noop fetch origin --no-tags\n--no-write-fetch-head --recurse-submodules=no --filter=blob:none\n--stdin\n\nwhere each line in the `fetches` file starts with `23:59:48` here.\nIt's 9 columns, separated by `|` characters, and the line we don't\nwant is the second one; the regex `d0.*fetch.negotiationAlgorithm`\nincludes it because of the `d0` in the path.\n\nI considered using `awk -F\"|\" \"\\$2~/d0/ &&\n\\$9~/fetch\\\\.negotiationAlgorithm/{ print }\" trace.output >fetches`,\nbut it was longer, possibly less clear, and less specific (since it\ndidn't include the $4~/child_start/ condition)\n\n>\n> Will queue.\n>\n>\n"},{"id":"499996","messageId":"xmqqcymq8n7v.fsf@gitster.g","threadId":"61887","inReplyTo":"CAO_smVh-gQWy7xUJgFjd6gUWCTV5jTYJ9E9E3rvaQg0EY-2BdQ@mail.gmail.com","subject":"Re: [PATCH v2 3/3] t6421: fix test to work when repo dir contains d0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-03T00:27:32Z","receivedAt":"2024-08-03T00:27:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kyle Lippincott writes:\n\n>> So it really didn't have to clarify what it is looking for, as it\n>> would not help seeing what false positives the patterns are designed\n>> to avoid matching. Sorry about that.\n>\n> I would have included examples, but they're quite long (>>>80 chars),\n> so seemed very out of place in both commit description and in the\n> codebase.\n\nAbsolutely. It turned out not to be so useful to show the shape of\npotential matches, like this one:\n\n ... run-command.c:733 | d0 | main | child_start |...\n\nTo explain why spaces around \" d0 \" matters, the readers need to\nunderstand that other trace entries that are irrelevant for our\npurpose, like this one\n\n ... | label:do_write_index /path/to/t/trash directory.../.git/index.lock\n\nwe want reject, and for that we want the pattern to be specific\nenough by looking for \" do \" that is followed by \" child_start \".\nOtherwise the leading paths that can contain anything won't easily\nmatch, and the original of looking for just \"d0\" was way too error\nprone. But it is hard to leave a concise hint for that there.\n\nSo, again, sorry about the bad suggestion.\n\n> I considered using `awk -F\"|\" \"\\$2~/d0/ &&\n> \\$9~/fetch\\\\.negotiationAlgorithm/{ print }\" trace.output >fetches`,\n> but it was longer, possibly less clear, and less specific (since it\n> didn't include the $4~/child_start/ condition)\n\nYeah, using the syntactic clue -F\"|\" would also be a way to convey\nthe intention (i.e. \"we are dealing with tabular output and we\nexpect nth column to be X\"), but what you have is probably good\nenough---it certainly is simpler to read and understand. I briefly\nconsidered that looking for \"| d0 |\" (i.e. explicitly mentioning the\ncolumn separator in the pattern) would make it even more obvious\nwhat we are looking for, but having to worry about quoting \"|\" in\nregexp would negate the benefit of obviousness out of the approach\nto use \"grep\".\n\nThanks.\n"},{"id":"500092","messageId":"xmqqv80f3r3d.fsf@gitster.g","threadId":"61887","inReplyTo":"CAPig+cTmzk7AN2x8-WCK_T5-_G7Wd-akB2++_4HFEbT67Rnc8A@mail.gmail.com","subject":"Re: [PATCH v2 2/3] strbuf: set errno to 0 after strbuf_getcwd","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-05T15:51:50Z","receivedAt":"2024-08-05T15:51:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Fri, Aug 2, 2024 at 5:32 PM Junio C Hamano wrote:\n>> > [...]\n>> > Set `errno = 0;` prior to exiting from `strbuf_getcwd` successfully.\n>> > This matches the behavior in functions like `run_transaction_hook`\n>> > (refs.c:2176) and `read_ref_internal` (refs/files-backend.c:564).\n>>\n>> I am still uneasy to see this unconditional clearing, which looks\n>> more like spreading the bad practice from two places you identified\n>> than following good behaviour modelled after these two places.\n>>\n>> But I'll let it pass.\n>>\n>> As long as our programmers understand that across strbuf_getcwd(),\n>> errno will *not* be preserved, even if the function returns success,\n>> it would be OK. As the usual convention around errno is that a\n>> successful call would leave errno intact, not clear it to 0, it\n>> would make it a bit harder to learn our API for newcomers, though.\n>\n> For what it's worth, I share your misgivings about this change and\n> consider the suggestion[*] to make it save/restore `errno` upon\n> success more sensible. It would also be a welcome change to see the\n> function documentation in strbuf.h updated to mention that it follows\n> the usual convention of leaving `errno` untouched upon success and\n> clobbered upon error.\n>\n> [*]: https://lore.kernel.org/git/xmqqv80jeza5.fsf@gitster.g/\n\nYup, of course save/restore would be safer, and probably easier to\nreason about for many people.\n\nThanks.\n"},{"id":"500110","messageId":"pull.1756.v3.git.git.1722877808.gitgitgadget@gmail.com","threadId":"61887","inReplyTo":"pull.1756.v2.git.git.1722632287.gitgitgadget@gmail.com","subject":"[PATCH v3 0/2] Small fixes for issues detected during internal CI runs","fromName":"Kyle Lippincott via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-08-05T17:10:06Z","receivedAt":"2024-08-05T17:10:11Z","isPatch":true,"sender":{"key":"spectral@google.com","avatar":"https://avatars.githubusercontent.com/u/6371650?v=4"},"body":"I'm attempting to get the git test suite running automatically during our\nweekly import. I have this mostly working, including with Address Sanitizer\nand Memory Sanitizer, but ran into a few issues:\n\n * several tests were failing due to strbuf_getcwd not clearing errno on\n success after it internally looped due to the path being >128 bytes. This\n is resolved in depth; though either one of the commits alone would\n resolve our issues:\n * modify locations that call strtoX and check for ERANGE to set errno =\n 0; prior to calling the conversion function. This is the typical way\n that these functions are invoked, and may indicate that we want\n compatibility helpers in git-compat-util.h to ensure that this happens\n correctly (and add these functions to the banned list).\n * have strbuf_getcwd set errno = 0; prior to a successful exit. This\n isn't very common for most functions in the codebase, but some other\n examples of this were found.\n * t6421-merge-partial-clone.sh had >10% flakiness. This is due to our build\n system using paths that contain a 64-hex-char hash, which had a 12.5%\n chance of containing the substring d0.\n\nKyle Lippincott (2):\n set errno=0 before strtoX calls\n t6421: fix test to work when repo dir contains d0\n\n builtin/get-tar-commit-id.c | 1 +\n ref-filter.c | 1 +\n t/helper/test-json-writer.c | 2 ++\n t/helper/test-trace2.c | 1 +\n t/t6421-merge-partial-clone.sh | 15 +++++++++------\n 5 files changed, 14 insertions(+), 6 deletions(-)\n\n\nbase-commit: e559c4bf1a306cf5814418d318cc0fea070da3c7\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1756%2Fspectral54%2Fstrbuf_getcwd-clear-errno-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1756/spectral54/strbuf_getcwd-clear-errno-v3\nPull-Request: https://github.com/git/git/pull/1756\n\nRange-diff vs v2:\n\n 1: 4dbd0bec40a = 1: 4dbd0bec40a set errno=0 before strtoX calls\n 2: 0ed09e9abb8 < -: ----------- strbuf: set errno to 0 after strbuf_getcwd\n 3: 818dc9e6b3e = 2: 96984c4a15e t6421: fix test to work when repo dir contains d0\n\n-- \ngitgitgadget\n"},{"id":"500111","messageId":"4dbd0bec40a0f9fd715e07a56bc6f12c4b29a83c.1722877808.git.gitgitgadget@gmail.com","threadId":"61887","inReplyTo":"pull.1756.v3.git.git.1722877808.gitgitgadget@gmail.com","subject":"[PATCH v3 1/2] set errno=0 before strtoX calls","fromName":"Kyle Lippincott via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-08-05T17:10:07Z","receivedAt":"2024-08-05T17:10:12Z","isPatch":true,"sender":{"key":"spectral@google.com","avatar":"https://avatars.githubusercontent.com/u/6371650?v=4"},"body":"From: Kyle Lippincott \n\nTo detect conversion failure after calls to functions like `strtod`, one\ncan check `errno == ERANGE`. These functions are not guaranteed to set\n`errno` to `0` on successful conversion, however. Manual manipulation of\n`errno` can likely be avoided by checking that the output pointer\ndiffers from the input pointer, but that's not how other locations, such\nas parse.c:139, handle this issue; they set errno to 0 prior to\nexecuting the function.\n\nFor every place I could find a strtoX function with an ERANGE check\nfollowing it, set `errno = 0;` prior to executing the conversion\nfunction.\n\nSigned-off-by: Kyle Lippincott \n---\n builtin/get-tar-commit-id.c | 1 +\n ref-filter.c | 1 +\n t/helper/test-json-writer.c | 2 ++\n t/helper/test-trace2.c | 1 +\n 4 files changed, 5 insertions(+)\n\ndiff --git a/builtin/get-tar-commit-id.c b/builtin/get-tar-commit-id.c\nindex 66a7389f9f4..7195a072edc 100644\n--- a/builtin/get-tar-commit-id.c\n+++ b/builtin/get-tar-commit-id.c\n@@ -35,6 +35,7 @@ int cmd_get_tar_commit_id(int argc, const char **argv UNUSED, const char *prefix\n \tif (header->typeflag[0] != TYPEFLAG_GLOBAL_HEADER)\n \t\treturn 1;\n \n+\terrno = 0;\n \tlen = strtol(content, &end, 10);\n \tif (errno == ERANGE || end == content || len < 0)\n \t\treturn 1;\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 8c5e673fc0a..54880a2497a 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1628,6 +1628,7 @@ static void grab_date(const char *buf, struct atom_value *v, const char *atomnam\n \ttimestamp = parse_timestamp(eoemail + 2, &zone, 10);\n \tif (timestamp == TIME_MAX)\n \t\tgoto bad;\n+\terrno = 0;\n \ttz = strtol(zone, NULL, 10);\n \tif ((tz == LONG_MIN || tz == LONG_MAX) && errno == ERANGE)\n \t\tgoto bad;\ndiff --git a/t/helper/test-json-writer.c b/t/helper/test-json-writer.c\nindex ed52eb76bfc..a288069b04c 100644\n--- a/t/helper/test-json-writer.c\n+++ b/t/helper/test-json-writer.c\n@@ -415,6 +415,7 @@ static void get_i(struct line *line, intmax_t *s_in)\n \n \tget_s(line, &s);\n \n+\terrno = 0;\n \t*s_in = strtol(s, &endptr, 10);\n \tif (*endptr || errno == ERANGE)\n \t\tdie(\"line[%d]: invalid integer value\", line->nr);\n@@ -427,6 +428,7 @@ static void get_d(struct line *line, double *s_in)\n \n \tget_s(line, &s);\n \n+\terrno = 0;\n \t*s_in = strtod(s, &endptr);\n \tif (*endptr || errno == ERANGE)\n \t\tdie(\"line[%d]: invalid float value\", line->nr);\ndiff --git a/t/helper/test-trace2.c b/t/helper/test-trace2.c\nindex cd955ec63e9..c588c273ce7 100644\n--- a/t/helper/test-trace2.c\n+++ b/t/helper/test-trace2.c\n@@ -26,6 +26,7 @@ static int get_i(int *p_value, const char *data)\n \tif (!data || !*data)\n \t\treturn MyError;\n \n+\terrno = 0;\n \t*p_value = strtol(data, &endptr, 10);\n \tif (*endptr || errno == ERANGE)\n \t\treturn MyError;\n-- \ngitgitgadget\n\n"},{"id":"500112","messageId":"96984c4a15e3b587cf8d590f5311a1abe1f63e28.1722877808.git.gitgitgadget@gmail.com","threadId":"61887","inReplyTo":"pull.1756.v3.git.git.1722877808.gitgitgadget@gmail.com","subject":"[PATCH v3 2/2] t6421: fix test to work when repo dir contains d0","fromName":"Kyle Lippincott via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-08-05T17:10:08Z","receivedAt":"2024-08-05T17:10:12Z","isPatch":true,"sender":{"key":"spectral@google.com","avatar":"https://avatars.githubusercontent.com/u/6371650?v=4"},"body":"From: Kyle Lippincott \n\nThe `grep` statement in this test looks for `d0.*`, attempting\nto filter to only show lines that had tabular output where the 2nd\ncolumn had `d0` and the final column had a substring of\n[`git -c `]`fetch.negotiationAlgorithm`. These lines also have\n`child_start` in the 4th column, but this isn't part of the condition.\n\nA subsequent line will have `d1` in the 2nd column, `start` in the 4th\ncolumn, and `/path/to/git/git -c fetch.negotiationAlgorihm` in the final\ncolumn. If `/path/to/git/git` contains the substring `d0`, then this\nline is included by `grep` as well as the desired line, leading to an\neffective doubling of the number of lines, and test failures.\n\nTighten the grep expression to require `d0` to be surrounded by spaces,\nand to have the `child_start` label.\n\nSigned-off-by: Kyle Lippincott \n---\n t/t6421-merge-partial-clone.sh | 15 +++++++++------\n 1 file changed, 9 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t6421-merge-partial-clone.sh b/t/t6421-merge-partial-clone.sh\nindex 711b709e755..b99f29ef9ba 100755\n--- a/t/t6421-merge-partial-clone.sh\n+++ b/t/t6421-merge-partial-clone.sh\n@@ -230,8 +230,9 @@ test_expect_merge_algorithm failure success 'Objects downloaded for single relev\n \t\tgrep fetch_count trace.output | cut -d \"|\" -f 9 | tr -d \" .\" >actual &&\n \t\ttest_cmp expect actual &&\n \n-\t\t# Check the number of fetch commands exec-ed\n-\t\tgrep d0.*fetch.negotiationAlgorithm trace.output >fetches &&\n+\t\t# Check the number of fetch commands exec-ed by filtering trace to\n+\t\t# child_start events by the top-level program (2nd field == d0)\n+\t\tgrep \" d0 .* child_start .*fetch.negotiationAlgorithm\" trace.output >fetches &&\n \t\ttest_line_count = 2 fetches &&\n \n \t\tgit rev-list --objects --all --missing=print |\n@@ -318,8 +319,9 @@ test_expect_merge_algorithm failure success 'Objects downloaded when a directory\n \t\tgrep fetch_count trace.output | cut -d \"|\" -f 9 | tr -d \" .\" >actual &&\n \t\ttest_cmp expect actual &&\n \n-\t\t# Check the number of fetch commands exec-ed\n-\t\tgrep d0.*fetch.negotiationAlgorithm trace.output >fetches &&\n+\t\t# Check the number of fetch commands exec-ed by filtering trace to\n+\t\t# child_start events by the top-level program (2nd field == d0)\n+\t\tgrep \" d0 .* child_start .*fetch.negotiationAlgorithm\" trace.output >fetches &&\n \t\ttest_line_count = 1 fetches &&\n \n \t\tgit rev-list --objects --all --missing=print |\n@@ -422,8 +424,9 @@ test_expect_merge_algorithm failure success 'Objects downloaded with lots of ren\n \t\tgrep fetch_count trace.output | cut -d \"|\" -f 9 | tr -d \" .\" >actual &&\n \t\ttest_cmp expect actual &&\n \n-\t\t# Check the number of fetch commands exec-ed\n-\t\tgrep d0.*fetch.negotiationAlgorithm trace.output >fetches &&\n+\t\t# Check the number of fetch commands exec-ed by filtering trace to\n+\t\t# child_start events by the top-level program (2nd field == d0)\n+\t\tgrep \" d0 .* child_start .*fetch.negotiationAlgorithm\" trace.output >fetches &&\n \t\ttest_line_count = 4 fetches &&\n \n \t\tgit rev-list --objects --all --missing=print |\n-- \ngitgitgadget\n"},{"id":"500113","messageId":"CAO_smVhr0YVXCDiaUcdov+o40=znSVSHsZiJegLOZezFjzWGfA@mail.gmail.com","threadId":"61887","inReplyTo":"CAO_smVjYYaE3UZd0M28j+=uYMLdDPRAN08X1Yb_=5+nU4GrkSA@mail.gmail.com","subject":"Re: [PATCH v2 2/3] strbuf: set errno to 0 after strbuf_getcwd","fromName":"Kyle Lippincott","fromEmail":"spectral@google.com","sentAt":"2024-08-05T17:12:09Z","receivedAt":"2024-08-05T17:12:32Z","isPatch":true,"sender":{"key":"spectral@google.com","avatar":"https://avatars.githubusercontent.com/u/6371650?v=4"},"body":"On Fri, Aug 2, 2024 at 4:51 PM Kyle Lippincott wrote:\n>\n> On Fri, Aug 2, 2024 at 2:32 PM Junio C Hamano wrote:\n> >\n> > \"Kyle Lippincott via GitGitGadget\" writes:\n> >\n> > > From: Kyle Lippincott \n> > >\n> > > If the loop executes more than once due to cwd being longer than 128\n> > > bytes, then `errno = ERANGE` might persist outside of this function.\n> > > This technically shouldn't be a problem, as all locations where the\n> > > value in `errno` is tested should either (a) call a function that's\n> > > guaranteed to set `errno` to 0 on success, or (b) set `errno` to 0 prior\n> > > to calling the function that only conditionally sets errno, such as the\n> > > `strtod` function. In the case of functions in category (b), it's easy\n> > > to forget to do that.\n> > >\n> > > Set `errno = 0;` prior to exiting from `strbuf_getcwd` successfully.\n> > > This matches the behavior in functions like `run_transaction_hook`\n> > > (refs.c:2176) and `read_ref_internal` (refs/files-backend.c:564).\n> >\n> > I am still uneasy to see this unconditional clearing, which looks\n> > more like spreading the bad practice from two places you identified\n> > than following good behaviour modelled after these two places.\n> >\n> > But I'll let it pass.\n> >\n> > As long as our programmers understand that across strbuf_getcwd(),\n> > errno will *not* be preserved, even if the function returns success,\n> > it would be OK. As the usual convention around errno is that a\n> > successful call would leave errno intact, not clear it to 0, it\n> > would make it a bit harder to learn our API for newcomers, though.\n>\n> I'm sympathetic to that argument. If you'd prefer to not have this\n> patch, I'm fine with it not landing, and instead at some future date I\n> may try to work on those #leftoverbits from the previous patch (to\n> make a safer wrapper around strtoX, and ban the use of the unwrapped\n> versions), or someone else can if they beat me to it.\n>\n> Since this is wrapping a posix function, and posix has things to say\n> about this (see below), I agree that it shouldn't set it to 0, and\n> withdraw this patch.\n\nDropped this patch in the reroll that (I think) I just sent.\n\n>\n> I'm including my references below mostly because with the information\n> I just acquired, I think that any attempt to _preserve_ errno is also\n> folly. No function we write, unless we explicitly state that it _will_\n> preserve errno, should feel obligated to do so. The number of cases\n> where errno _could_ be modified according to the various\n> specifications (C99 and posix) are just too numerous.\n>\n> ---\n>\n> Perhaps because I'm not all that experienced with C, but when I did C\n> a couple decades ago, I operated in a mode where basically every\n> function was actively hostile. If I wanted errno preserved across a\n> function call, then it's up to me (the caller) to do so, regardless of\n> what the current implementation of that function says will happen,\n> because that can change at any point. Unless the function is\n> documented as errno-preserving, I'm going to treat it as\n> errno-hostile. In practice, this didn't really matter much, as I've\n> never found `if (some_func()) { if (!some_other_func()) { /* use errno\n> from `some_func` */ } }` logic to happen often, but maybe it does in\n> \"real\" programs, I was just a hobbyist self-teaching at the time.\n>\n> The C standard has a very precise definition of how the library\n> functions defined in the C specification will act. It guarantees:\n> - the library functions defined in the specification will never set errno to 0.\n> - the library functions defined in the specification may set the value\n> to non-zero whether an error occurs or not, \"provided the use of errno\n> is not documented in the description of the function in this\n> International Standard\". What this means is that (a) if the function\n> as defined in the C standard mentions errno, it can only set the\n> values as specified there, and (b) if the function as defined in the C\n> standard does _not_ mention errno, such as `fopen` or `strstr`, it can\n> do _whatever it wants_ to errno, even on success, _except_ set it to\n> 0.\n>\n> POSIX has similar language\n> (https://pubs.opengroup.org/onlinepubs/009695399/functions/errno.html),\n> with some key differences:\n> - The value of errno should only be examined when it is indicated to\n> be valid by a function's return value.\n> - The setting of errno after a successful call to a function is\n> unspecified unless the description of that function specifies that\n> errno shall not be modified.\n>\n> This means that unlike the C specification, which says that if a\n> function doesn't describe its use of errno it can do anything it wants\n> to errno [except set it to 0], in POSIX, a function can do anything it\n> wants to errno [except set it to 0] at any time.\n>\n> What this means in practice is that errno should never be assumed to\n> be preserved across calls to posix functions (like getcwd). Also,\n> strbuf_getcwd calls free, malloc, and realloc, none of which mention\n> errno in the C specification, so they can do whatever they want to it\n> [except set it to 0]. That I was able to find one single function that\n> was causing problems is luck, and not guaranteed by any specification.\n>\n> Kind of makes me want to try writing an actively hostile C99 and POSIX\n> environment, and see how many things break with it. :) C99 spec\n> doesn't say anything about malloc setting errno? Ok! malloc now sets\n> errno to ENOENT on tuesdays [in GMT because I'm not a monster], but\n> only on success. On any other day, it'll set it to ERANGE, regardless\n> of success or failure.\n>\n> >\n> > Thanks.\n> >\n> > > Signed-off-by: Kyle Lippincott \n> > > ---\n> > > strbuf.c | 1 +\n> > > 1 file changed, 1 insertion(+)\n> > >\n> > > diff --git a/strbuf.c b/strbuf.c\n> > > index 3d2189a7f64..b94ef040ab0 100644\n> > > --- a/strbuf.c\n> > > +++ b/strbuf.c\n> > > @@ -601,6 +601,7 @@ int strbuf_getcwd(struct strbuf *sb)\n> > > strbuf_grow(sb, guessed_len);\n> > > if (getcwd(sb->buf, sb->alloc)) {\n> > > strbuf_setlen(sb, strlen(sb->buf));\n> > > + errno = 0;\n> > > return 0;\n> > > }\n"},{"id":"500118","messageId":"xmqqzfpq24u4.fsf@gitster.g","threadId":"61887","inReplyTo":"pull.1756.v3.git.git.1722877808.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 0/2] Small fixes for issues detected during internal CI runs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-05T18:37:55Z","receivedAt":"2024-08-05T18:38:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kyle Lippincott via GitGitGadget\" writes:\n\n> I'm attempting to get the git test suite running automatically during our\n> weekly import. I have this mostly working, including with Address Sanitizer\n> and Memory Sanitizer, but ran into a few issues:\n>\n> * several tests were failing due to strbuf_getcwd not clearing errno on\n> success after it internally looped due to the path being >128 bytes. This\n> is resolved in depth; though either one of the commits alone would\n> resolve our issues:\n> * modify locations that call strtoX and check for ERANGE to set errno =\n> 0; prior to calling the conversion function. This is the typical way\n> that these functions are invoked, and may indicate that we want\n> compatibility helpers in git-compat-util.h to ensure that this happens\n> correctly (and add these functions to the banned list).\n> * have strbuf_getcwd set errno = 0; prior to a successful exit. This\n> isn't very common for most functions in the codebase, but some other\n> examples of this were found.\n> * t6421-merge-partial-clone.sh had >10% flakiness. This is due to our build\n> system using paths that contain a 64-hex-char hash, which had a 12.5%\n> chance of containing the substring d0.\n>\n> Kyle Lippincott (2):\n> set errno=0 before strtoX calls\n> t6421: fix test to work when repo dir contains d0\n\nBoth patches make perfect sense to me. Thanks.\n"},{"id":"500156","messageId":"ZrHCCBXXWZPzAcQb@tanuki","threadId":"61887","inReplyTo":"xmqqv80f3r3d.fsf@gitster.g","subject":"Re: [PATCH v2 2/3] strbuf: set errno to 0 after strbuf_getcwd","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-06T06:26:16Z","receivedAt":"2024-08-06T06:26:21Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Aug 05, 2024 at 08:51:50AM -0700, Junio C Hamano wrote:\n> Eric Sunshine writes:\n> \n> > On Fri, Aug 2, 2024 at 5:32 PM Junio C Hamano wrote:\n> >> > [...]\n> >> > Set `errno = 0;` prior to exiting from `strbuf_getcwd` successfully.\n> >> > This matches the behavior in functions like `run_transaction_hook`\n> >> > (refs.c:2176) and `read_ref_internal` (refs/files-backend.c:564).\n> >>\n> >> I am still uneasy to see this unconditional clearing, which looks\n> >> more like spreading the bad practice from two places you identified\n> >> than following good behaviour modelled after these two places.\n> >>\n> >> But I'll let it pass.\n> >>\n> >> As long as our programmers understand that across strbuf_getcwd(),\n> >> errno will *not* be preserved, even if the function returns success,\n> >> it would be OK. As the usual convention around errno is that a\n> >> successful call would leave errno intact, not clear it to 0, it\n> >> would make it a bit harder to learn our API for newcomers, though.\n> >\n> > For what it's worth, I share your misgivings about this change and\n> > consider the suggestion[*] to make it save/restore `errno` upon\n> > success more sensible. It would also be a welcome change to see the\n> > function documentation in strbuf.h updated to mention that it follows\n> > the usual convention of leaving `errno` untouched upon success and\n> > clobbered upon error.\n> >\n> > [*]: https://lore.kernel.org/git/xmqqv80jeza5.fsf@gitster.g/\n> \n> Yup, of course save/restore would be safer, and probably easier to\n> reason about for many people.\n\nIs it really all that reasonable? We're essentially partitioning our set\nof APIs into two sets, where one set knows to keep `errno` intact\nwhereas another set doesn't. In such a world, you have to be very\ncareful about which APIs you are calling in a function that wants to\nkeep `errno` intact, which to me sounds like a maintenance headache.\n\nI'd claim that most callers never care about `errno` at all. For the\ncallers that do, I feel it is way more fragile to rely on whether or not\na called function leaves `errno` intact or not. For one, it's fragile\nbecause that may easily change due to a bug. Second, it is fragile\nbecause the dependency on `errno` is not explicitly documented via code,\nbut rather an implicit dependency.\n\nSo isn't it more reasonable to rather make the few callers that do\nrequire `errno` to be left intact to save it? It makes the dependency\nexplicit, avoids splitting our functions into two sets and allows us to\njust ignore this issue for the majority of functions that couldn't care\nless about `errno`.\n\nPatrick\n"},{"id":"500160","messageId":"CAO_smVj7kN1ywAMVagTb_ALwqb-aycUy4tSaJ47ocC1ZRBHcqQ@mail.gmail.com","threadId":"61887","inReplyTo":"ZrHCCBXXWZPzAcQb@tanuki","subject":"Re: [PATCH v2 2/3] strbuf: set errno to 0 after strbuf_getcwd","fromName":"Kyle Lippincott","fromEmail":"spectral@google.com","sentAt":"2024-08-06T07:04:30Z","receivedAt":"2024-08-06T07:04:48Z","isPatch":true,"sender":{"key":"spectral@google.com","avatar":"https://avatars.githubusercontent.com/u/6371650?v=4"},"body":"On Mon, Aug 5, 2024 at 11:26 PM Patrick Steinhardt wrote:\n>\n> On Mon, Aug 05, 2024 at 08:51:50AM -0700, Junio C Hamano wrote:\n> > Eric Sunshine writes:\n> >\n> > > On Fri, Aug 2, 2024 at 5:32 PM Junio C Hamano wrote:\n> > >> > [...]\n> > >> > Set `errno = 0;` prior to exiting from `strbuf_getcwd` successfully.\n> > >> > This matches the behavior in functions like `run_transaction_hook`\n> > >> > (refs.c:2176) and `read_ref_internal` (refs/files-backend.c:564).\n> > >>\n> > >> I am still uneasy to see this unconditional clearing, which looks\n> > >> more like spreading the bad practice from two places you identified\n> > >> than following good behaviour modelled after these two places.\n> > >>\n> > >> But I'll let it pass.\n> > >>\n> > >> As long as our programmers understand that across strbuf_getcwd(),\n> > >> errno will *not* be preserved, even if the function returns success,\n> > >> it would be OK. As the usual convention around errno is that a\n> > >> successful call would leave errno intact, not clear it to 0, it\n> > >> would make it a bit harder to learn our API for newcomers, though.\n> > >\n> > > For what it's worth, I share your misgivings about this change and\n> > > consider the suggestion[*] to make it save/restore `errno` upon\n> > > success more sensible. It would also be a welcome change to see the\n> > > function documentation in strbuf.h updated to mention that it follows\n> > > the usual convention of leaving `errno` untouched upon success and\n> > > clobbered upon error.\n> > >\n> > > [*]: https://lore.kernel.org/git/xmqqv80jeza5.fsf@gitster.g/\n> >\n> > Yup, of course save/restore would be safer, and probably easier to\n> > reason about for many people.\n>\n> Is it really all that reasonable? We're essentially partitioning our set\n> of APIs into two sets, where one set knows to keep `errno` intact\n> whereas another set doesn't. In such a world, you have to be very\n> careful about which APIs you are calling in a function that wants to\n> keep `errno` intact, which to me sounds like a maintenance headache.\n>\n> I'd claim that most callers never care about `errno` at all. For the\n> callers that do, I feel it is way more fragile to rely on whether or not\n> a called function leaves `errno` intact or not. For one, it's fragile\n> because that may easily change due to a bug. Second, it is fragile\n> because the dependency on `errno` is not explicitly documented via code,\n> but rather an implicit dependency.\n>\n> So isn't it more reasonable to rather make the few callers that do\n> require `errno` to be left intact to save it? It makes the dependency\n> explicit, avoids splitting our functions into two sets and allows us to\n> just ignore this issue for the majority of functions that couldn't care\n> less about `errno`.\n\n100% agreed. The C language specification says you can't rely on errno\npersisting across function calls, and that the caller must preserve it\nif it needs that behavior for some reason. The POSIX specification\nsays you can't either except in very rare circumstances where it\nguarantees errno will not change. The Linux man page for errno says\nyou can't rely on errno not changing, even for printf:\nhttps://man7.org/linux/man-pages/man3/errno.3.html\n\n A common mistake is to do\n\n if (somecall() == -1) {\n printf(\"somecall() failed\\n\");\n if (errno == ...) { ... }\n }\n\n where errno no longer needs to have the value it had upon return\n from somecall() (i.e., it may have been changed by the\n printf(3)). If the value of errno should be preserved across a\n library call, it must be saved:\n\n if (somecall() == -1) {\n int errsv = errno;\n printf(\"somecall() failed\\n\");\n if (errsv == ...) { ... }\n }\n\nBasically: errno is _extremely_ volatile. One should assume that\n_every_ function call is going to change it, even if they return\nsuccessfully. The only thing that can't happen is that the functions\ndefined in the C and POSIX standards set errno to 0, which is why I\nwithdrew the patch (since it's a wrapper around a function defined in\nPOSIX). But in general, I don't see any reason for any of the\nfunctions we write to be errno preserving, especially since any call\nto malloc, printf, trace functionality, etc. may modify errno.\n\n>\n> Patrick\n"}]}