{"thread":{"id":"62714","subject":"What's cooking in git.git (Dec 2024, #11; Mon, 30)","startedAt":"2024-12-30T17:33:23Z","lastAt":"2025-01-09T10:00:51Z","messageCount":37,"participants":["Junio C Hamano","René Scharfe","Jeff King","Chris Torek","Patrick Steinhardt"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"509751","messageId":"xmqqpll9xehr.fsf@gitster.g","threadId":"62714","inReplyTo":null,"subject":"What's cooking in git.git (Dec 2024, #11; Mon, 30)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-30T17:33:20Z","receivedAt":"2024-12-30T17:33:23Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Here are the topics that have been cooking in my tree. Commits\nprefixed with '+' are in 'next' (being in 'next' is a sign that a\ntopic is stable enough to be used and are candidate to be in a\nfuture release). Commits prefixed with '-' are only in 'seen', and\naren't considered \"accepted\" at all and may be annotated with an URL\nto a message that raises issues but they are no means exhaustive. A\ntopic without enough support may be discarded after a long period of\nno activity (of course they can be resubmit when new interests\narise).\n\nIt has been a very slow week, and I expect this week will also be\nslow. Git 2.48-rc1 has been tagged. Hopefully we can have 2.48\nfinal around the end of next week.\n\nExtra testing the tip of 'master' before we actually tag it is as\nalways very much appreciated.\n\nCopies of the source code to Git live in many repositories, and the\nfollowing is a list of the ones I push into or their mirrors. Some\nrepositories have only a subset of branches.\n\nWith maint, master, next, seen, todo:\n\n\tgit://git.kernel.org/pub/scm/git/git.git/\n\tgit://repo.or.cz/alt-git.git/\n\thttps://kernel.googlesource.com/pub/scm/git/git/\n\thttps://github.com/git/git/\n\thttps://gitlab.com/git-scm/git/\n\nWith all the integration branches and topics broken out:\n\n\thttps://github.com/gitster/git/\n\nEven though the preformatted documentation in HTML and man format\nare not sources, they are published in these repositories for\nconvenience (replace \"htmldocs\" with \"manpages\" for the manual\npages):\n\n\tgit://git.kernel.org/pub/scm/git/git-htmldocs.git/\n\thttps://github.com/gitster/git-htmldocs.git/\n\nRelease tarballs are available at:\n\n\thttps://www.kernel.org/pub/software/scm/git/\n\n--------------------------------------------------\n[New Topics]\n\n* jk/lsan-race-with-barrier (2024-12-30) 5 commits\n (merged to 'next' on 2024-12-30 at 3fc0e14928)\n + grep: work around LSan threading race with barrier\n + index-pack: work around LSan threading race with barrier\n + thread-utils: introduce optional barrier type\n + Revert \"index-pack: spawn threads atomically\"\n + test-lib: use individual lsan dir for --stress runs\n\n CI jobs that run threaded programs under LSan has been giving false\n positives from time to time, which has been worked around.\n\n Will merge to 'master'.\n source: <20241230042325.GA112439@coredump.intra.peff.net>\n\n\n* ps/meson-weak-sha1-build (2024-12-30) 8 commits\n - meson: provide a summary of configured backends\n - meson: wire up unsafe SHA1 backend\n - meson: add missing dots for build options\n - meson: simplify conditions for HTTPS and SHA1 dependencies\n - meson: require SecurityFramework when it's used as SHA1 backend\n - meson: deduplicate access to SHA1/SHA256 backend options\n - meson: consistenlty spell 'CommonCrypto'\n - Merge branch 'ps/weak-sha1-for-tail-sum-fix' into ps/meson-weak-sha1-build\n (this branch uses ps/weak-sha1-for-tail-sum-fix.)\n\n meson-based build now supports the unsafe-sha1 build knob.\n\n Will merge to 'next'.\n source: <20241230-pks-meson-sha1-unsafe-v1-0-efb276e171f5@pks.im>\n\n\n* ps/object-collision-check (2024-12-30) 1 commit\n (merged to 'next' on 2024-12-30 at e083ea3154)\n + object-file: fix race in object collision check\n\n CI jobs gave sporadic failures, which turns out that that the\n object finalization code was giving an error when it did not have\n to.\n\n Will merge to 'master'.\n source: <20241230-b4-pks-object-file-racy-collision-check-v1-1-11571294e60a@pks.im>\n\n\n* ps/weak-sha1-for-tail-sum-fix (2024-12-30) 3 commits\n (merged to 'next' on 2024-12-30 at c24783e99d)\n + ci: exercise unsafe OpenSSL backend\n + builtin/fast-import: fix segfault with unsafe SHA1 backend\n + bulk-checkin: fix segfault with unsafe SHA1 backend\n (this branch is used by ps/meson-weak-sha1-build.)\n\n An earlier \"csum-file checksum does not have to be computed with\n sha1dc\" topic had a few code paths that had initialized an\n implementation of a hash function to be used by an unmatching hash\n by mistake, which have been corrected.\n\n Will merge to 'master'.\n source: <20241230-pks-meson-sha1-unsafe-v1-0-efb276e171f5@pks.im>\n\n--------------------------------------------------\n[Graduated to 'master']\n\n* ms/t7611-test-path-is-file (2024-12-27) 1 commit\n (merged to 'next' on 2024-12-28 at b08a0c8e23)\n + t7611: replace test -f with test_path_is* helpers\n\n Test modernization.\n \n source: <20241227105345.10184-1-meetsoni3017@gmail.com>\n\n\n* ps/meson-test-wo-gitweb (2024-12-27) 3 commits\n (merged to 'next' on 2024-12-28 at 39938f41fd)\n + meson: enable auto-discovered \"gitweb\"\n + GIT-BUILD-OPTIONS: wire up NO_GITWEB option\n + GIT-BUILD-OPTIONS: sort variables alphabetically\n\n meson-based build without GitWeb failed the self tests.\n source: <20241227-b4-pks-meson-wo-gitweb-v1-0-14ca8515bb3b@pks.im>\n\n--------------------------------------------------\n[Cooking]\n\n* sk/strlen-returns-size_t (2024-12-26) 1 commit\n - date.c: Fix type missmatch warings from msvc\n\n Code clean-up.\n\n The remainder needs to be reviewed.\n source: <20241223110407.3308-3-soekkle@freenet.de>\n\n\n* ps/more-sign-compare (2024-12-27) 10 commits\n - sign-compare: avoid comparing ptrdiff with an int/unsigned\n - commit-reach: use `size_t` to track indices when computing merge bases\n - shallow: fix -Wsign-compare warnings\n - builtin/log: fix remaining -Wsign-compare warnings\n - builtin/log: use `size_t` to track indices\n - commit-reach: use `size_t` to track indices in `get_reachable_subset()`\n - commit-reach: use `size_t` to track indices in `remove_redundant()`\n - commit-reach: fix type of `min_commit_date`\n - commit-reach: fix index used to loop through unsigned integer\n - prio-queue: fix type of `insertion_ctr`\n\n More -Wsign-compare fixes.\n\n Will merge to 'next'.\n cf. https://staticthinking.wordpress.com/2023/07/25/wsign-compare-is-garbage/\n source: <20241227-b4-pks-commit-reach-sign-compare-v1-0-07c59c2aa632@pks.im>\n\n\n* as/long-option-help-i18n (2024-12-30) 1 commit\n (merged to 'next' on 2024-12-30 at 900c79808f)\n + parse-options: localize mark-up of placeholder text in the short help\n\n Tweak the help text used for the option value placeholders by\n parse-options API so that translations can customize the \"<>\"\n placeholder signal (e.g. \"--option=\").\n\n Will cook in 'next'.\n source: <20241228114221.10351-4-ash@kambanaria.org>\n\n\n* sk/maintenance-remote-prune (2024-12-28) 1 commit\n - maintenance: add prune-remote-refs task\n\n A new periodic maintenance task to run \"git remote prune\" has been\n introduced.\n\n Expecting a reroll.\n cf. \n source: \n\n\n* rs/reftable-realloc-errors (2024-12-28) 4 commits\n (merged to 'next' on 2024-12-30 at ebc9625a4c)\n + t-reftable-merged: handle realloc errors\n + reftable: handle realloc error in parse_names()\n + reftable: fix allocation count on realloc error\n + reftable: avoid leaks on realloc error\n\n The custom allocator code in the reftable library did not handle\n failing realloc() very well, which has been addressed.\n\n Will merge to 'master'?\n cf. \n source: \n\n\n* jc/show-index-h-update (2024-12-20) 1 commit\n - show-index: the short help should say the command reads from its input\n\n Doc and short-help text for \"show-index\" has been clarified to\n stress that the command reads its data from the standard input.\n\n Comments?\n source: \n\n\n* ps/the-repository (2024-12-18) 15 commits\n - match-trees: stop using `the_repository`\n - graph: stop using `the_repository`\n - add-interactive: stop using `the_repository`\n - tmp-objdir: stop using `the_repository`\n - resolve-undo: stop using `the_repository`\n - credential: stop using `the_repository`\n - mailinfo: stop using `the_repository`\n - diagnose: stop using `the_repository`\n - server-info: stop using `the_repository`\n - send-pack: stop using `the_repository`\n - serve: stop using `the_repository`\n - trace: stop using `the_repository`\n - pager: stop using `the_repository`\n - progress: stop using `the_repository`\n - Merge branch 'ps/build-sign-compare' into ps/the-repository\n\n More code paths have a repository passed through the callchain,\n instead of assuming the primary the_repository object.\n source: <20241217-pks-use-the-repository-conversion-v1-0-0dba48bcc239@pks.im>\n\n\n* ps/build-meson-html (2024-12-27) 12 commits\n - Documentation: wire up sanity checks for Meson\n - t/Makefile: make \"check-meson\" work with Dash\n - meson: install static files for HTML documentation\n - meson: generate articles\n - Documentation: refactor \"howto-index.sh\" for out-of-tree builds\n - Documentation: refactor \"api-index.sh\" for out-of-tree builds\n - meson: generate user manual\n - Documentation: inline user-manual.conf\n - meson: generate HTML pages for all man page categories\n - meson: fix generation of merge tools\n - meson: properly wire up dependencies for our docs\n - meson: wire up support for AsciiDoctor\n\n The build procedure based on meson learned to generate HTML\n documention pages.\n\n Needs review. On hold.\n source: <20241227-b4-pks-meson-docs-v2-0-f61e63edbfa1@pks.im>\n\n\n* jc/doc-attr-tree (2024-12-14) 1 commit\n - doc: give attr.tree a bit more visibility\n\n Make sure that \"git --attr-source=X\", GIT_ATTR_SOURCE, and\n attr.tree configuration variables appear at the same places in the\n documentation.\n\n On hold.\n cf. <20241216111112.GA2201417@coredump.intra.peff.net>\n source: \n\n\n* ps/3.0-remote-deprecation (2024-12-12) 6 commits\n - remote: announce removal of \"branches/\" and \"remotes/\"\n - builtin/pack-redundant: remove subcommand with breaking changes\n - ci: repurpose \"linux-gcc\" job for deprecations\n - ci: merge linux-gcc-default into linux-gcc\n - Makefile: wire up build option for deprecated features\n - Merge branch 'ps/build' into ps/3.0-remote-deprecation\n\n Following the procedure we established to introduce breaking\n changes for Git 3.0, allow an early opt-in for removing support of\n $GIT_DIR/branches/ and $GIT_DIR/remotes/ directories to configure\n remotes.\n\n Needs review.\n source: <20241211-pks-remote-branches-deprecation-v1-0-1431e2369135@pks.im>\n\n\n* cc/lop-remote (2024-12-07) 5 commits\n . doc: add technical design doc for large object promisors\n . promisor-remote: check advertised name or URL\n . Add 'promisor-remote' capability to protocol v2\n . strbuf: refactor strbuf_trim_trailing_ch()\n . version: refactor strbuf_sanitize()\n\n Expecting a reroll.\n cf. \n source: <20241206124248.160494-1-christian.couder@gmail.com>\n\n\n* ds/backfill (2024-12-20) 6 commits\n - backfill: assume --sparse when sparse-checkout is enabled\n - backfill: add --sparse option\n - backfill: add --min-batch-size= option\n - backfill: basic functionality and tests\n - backfill: add builtin boilerplate\n - Merge branch 'ds/path-walk-1' into ds/backfill\n (this branch uses ds/path-walk-1.)\n\n Lazy-loading missing files in a blobless clone on demand is costly\n as it tends to be one-blob-at-a-time. \"git backfill\" is introduced\n to help bulk-download necessary files beforehand.\n\n Comments?\n source: \n\n\n* re/submodule-parse-opt (2024-12-11) 7 commits\n (merged to 'next' on 2024-12-21 at 9e65a56a63)\n + git-submodule.sh: rename some variables\n + git-submodule.sh: improve variables readability\n + git-submodule.sh: add some comments\n + git-submodule.sh: get rid of unused variable\n + git-submodule.sh: get rid of isnumber\n + git-submodule.sh: improve parsing of short options\n + git-submodule.sh: improve parsing of some long options\n\n \"git submodule\" learned various ways to spell the same option,\n e.g. \"--branch=B\" can be spelled \"--branch B\" or \"-bB\".\n\n Will cook in 'next'.\n source: <20241211063234.7610-1-royeldar0@gmail.com>\n\n\n* tb/unsafe-hash-test (2024-11-21) 2 commits\n - t/helper/test-tool: implement sha1-unsafe helper\n - t/helper/test-sha1: prepare for an unsafe mode\n\n Preliminary addition to the test tool to allow a plain SHA-1 hash\n algorithm without collision protection.\n\n Comments?\n cf. \n source: \n\n\n* tb/incremental-midx-part-2 (2024-11-20) 15 commits\n - midx: implement writing incremental MIDX bitmaps\n - pack-bitmap.c: use `ewah_or_iterator` for type bitmap iterators\n - pack-bitmap.c: keep track of each layer's type bitmaps\n - ewah: implement `struct ewah_or_iterator`\n - pack-bitmap.c: apply pseudo-merge commits with incremental MIDXs\n - pack-bitmap.c: compute disk-usage with incremental MIDXs\n - pack-bitmap.c: teach `rev-list --test-bitmap` about incremental MIDXs\n - pack-bitmap.c: support bitmap pack-reuse with incremental MIDXs\n - pack-bitmap.c: teach `show_objects_for_type()` about incremental MIDXs\n - pack-bitmap.c: teach `bitmap_for_commit()` about incremental MIDXs\n - pack-bitmap.c: open and store incremental bitmap layers\n - pack-revindex: prepare for incremental MIDX bitmaps\n - Documentation: describe incremental MIDX bitmaps\n - Merge branch 'tb/pseudo-merge-bitmap-fixes' into tb/incremental-midx-part-2\n - Merge branch 'tb/incremental-midx-part-1' into tb/incremental-midx-part-2\n\n Incrementally updating multi-pack index files.\n\n Needs review.\n source: \n\n\n* ps/send-pack-unhide-error-in-atomic-push (2024-11-14) 2 commits\n - transport: don't ignore git-receive-pack(1) exit code on atomic push\n - t5504: modernize test by moving heredocs into test bodies\n\n \"git push --atomic --porcelain\" used to ignore failures from the\n other side, losing the error status from the child process, which\n has been corrected.\n\n Needs to see if competing parallel topic needs to replace this one.\n source: <20241113-pks-push-atomic-respect-exit-code-v1-0-7965f01e7f4e@pks.im>\n\n\n* jc/move-is-bare-repository-cfg-variable-to-repo (2024-11-07) 3 commits\n . repository: BUG when is_bare_cfg is not initialized\n . setup: initialize is_bare_cfg\n . git: remove is_bare_repository_cfg global variable\n\n Code rewrite to turn the is_bare_repository_cfg global variable\n into a member in the the_repo singleton repository object.\n\n Waiting for response to reviews.\n cf. \n Seems to break t0021-conversion on Windows.\n cf. https://lore.kernel.org/git/xmqqzfl1hl52.fsf@gitster.g/\n source: \n\n\n* ds/name-hash-tweaks (2024-12-20) 8 commits\n - pack-objects: add third name hash version\n - pack-objects: prevent name hash version change\n - test-tool: add helper for name-hash values\n - p5313: add size comparison test\n - pack-objects: add GIT_TEST_NAME_HASH_VERSION\n - repack: add --name-hash-version option\n - pack-objects: add --name-hash-version option\n - pack-objects: create new name-hash function version\n\n \"git pack-objects\" and its wrapper \"git repack\" learned an option\n to use an alternative path-hash function to improve delta-base\n selection to produce a packfile with deeper history than window\n size.\n\n Comments?\n source: \n\n\n* ds/path-walk-1 (2024-12-20) 7 commits\n - path-walk: reorder object visits\n - path-walk: mark trees and blobs as UNINTERESTING\n - path-walk: visit tags and cached objects\n - path-walk: allow consumer to specify object types\n - t6601: add helper for testing path-walk API\n - test-lib-functions: add test_cmp_sorted\n - path-walk: introduce an object walk by path\n (this branch is used by ds/backfill.)\n\n Introduce a new API to visit objects in batches based on a common\n path, or by type.\n\n Comments?\n source: \n\n\n* km/config-remote-by-name (2024-10-21) 1 commit\n - config: support remote name in includeIf.hasconfig condition\n\n Support conditionally including configuration by remote name, instead\n of just URL.\n\n Will discard?\n source: <20241020173216.40852-2-ken@kmatsui.me>\n\n\n* y5/diff-pager (2024-10-21) 1 commit\n - diff: setup pager only before diff contents truly ready\n\n Delay setting up the pager in 'git diff' until after the diff contents\n itself is fully prepared.\n\n Will discard?\n source: \n\n\n* ej/cat-file-remote-object-info (2024-11-25) 6 commits\n - cat-file: add remote-object-info to batch-command\n - transport: add client support for object-info\n - serve: advertise object-info feature\n - fetch-pack: move fetch initialization\n - fetch-pack: refactor packet writing\n - cat-file: add declaration of variable i inside its for loop\n\n \"git cat-file --batch\" and friends can optionally ask a remote\n server about objects it does not have.\n\n Expecting a reroll.\n cf. \n source: <20241125053616.25170-1-eric.peijian@gmail.com>\n\n\n* js/libgit-rust (2024-10-16) 5 commits\n - Makefile: add option to build and test libgit-rs and libgit-rs-sys\n - libgit: add higher-level libgit crate\n - libgit-sys: also export some config_set functions\n - libgit-sys: introduce Rust wrapper for libgit.a\n - common-main: split init and exit code into new files\n\n A rust binding to libgit.a functions has been introduced.\n\n Will discard?\n source: \n"},{"id":"509764","messageId":"003619a3-f544-4b20-9685-866fd2ed1ae2@web.de","threadId":"62714","inReplyTo":"xmqqpll9xehr.fsf@gitster.g","subject":"Re: What's cooking in git.git (Dec 2024, #11; Mon, 30)","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-12-31T17:27:16Z","receivedAt":"2024-12-31T17:27:26Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 30.12.24 um 18:33 schrieb Junio C Hamano:\n> * rs/reftable-realloc-errors (2024-12-28) 4 commits\n> (merged to 'next' on 2024-12-30 at ebc9625a4c)\n> + t-reftable-merged: handle realloc errors\n> + reftable: handle realloc error in parse_names()\n> + reftable: fix allocation count on realloc error\n> + reftable: avoid leaks on realloc error\n>\n> The custom allocator code in the reftable library did not handle\n> failing realloc() very well, which has been addressed.\n>\n> Will merge to 'master'?\n\nReftable allocation error handling was introduced by bcd5a4059a\n(reftable/error: introduce out-of-memory error code, 2024-10-02) after\nv2.47.1, and this series improves it, so I'd say yes. But of course\nI'm biased.\n\n> cf. \n> source: \n\nRené\n\n"},{"id":"509775","messageId":"20250101191422.GC1391912@coredump.intra.peff.net","threadId":"62714","inReplyTo":"xmqqpll9xehr.fsf@gitster.g","subject":"a less-invasive racy-leak fix, was Re: What's cooking in git.git (Dec 2024, #11; Mon, 30)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-01T19:14:22Z","receivedAt":"2025-01-01T19:14:24Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 30, 2024 at 09:33:20AM -0800, Junio C Hamano wrote:\n\n> * jk/lsan-race-with-barrier (2024-12-30) 5 commits\n> (merged to 'next' on 2024-12-30 at 3fc0e14928)\n> + grep: work around LSan threading race with barrier\n> + index-pack: work around LSan threading race with barrier\n> + thread-utils: introduce optional barrier type\n> + Revert \"index-pack: spawn threads atomically\"\n> + test-lib: use individual lsan dir for --stress runs\n> \n> CI jobs that run threaded programs under LSan has been giving false\n> positives from time to time, which has been worked around.\n> \n> Will merge to 'master'.\n> source: <20241230042325.GA112439@coredump.intra.peff.net>\n\nThis graduated faster than I expected. :)\n\nI think the first two patches are obviously good, and we should take\nthem as-is. But while poking at the problem a bit more, I found\nsomething quite interesting that might let us fix it without any code\nchange at all.\n\nI wanted a minimal reproduction of the problem so that I could report it\nto the sanitizer folks upstream. So I thought this would work:\n\n-- >8 --\n\ncat >foo.c <<\\EOF\n#include \n#include \n\nstatic void *run(void *data)\n{\n\treturn NULL;\n}\n\nint main(void)\n{\n\tpthread_t threads[256];\n\n\tfor (int i = 0; i < sizeof(threads)/sizeof(*threads); i++)\n\t\tpthread_create(&threads[i], NULL, run, NULL);\n\texit(1);\n}\nEOF\n\ngcc -fsanitize=leak foo.c\n./a.out\n\n-- 8< --\n\nBut it doesn't! Even if I run it thousands of times. And yet if I put\nsimilar code inside of git.c and run it within our test suite, I see the\nfailure almost immediately. So there's something about our test suite\nthat triggers it. And indeed, the secret sauce turns out to be this\noption (depending on system load you might need to run it a couple times\nto trigger the failure):\n\n $ LSAN_OPTIONS=fast_unwind_on_malloc=0 ./a.out\n =================================================================\n ==1836417==ERROR: LeakSanitizer: detected memory leaks\n \n Direct leak of 32 byte(s) in 1 object(s) allocated from:\n #0 0x7ff8bb014556 in realloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:98\n #1 0x7ff8bae9d2c1 in __pthread_getattr_np nptl/pthread_getattr_np.c:180\n #2 0x7ff8bb02500d in __sanitizer::GetThreadStackTopAndBottom(bool, unsigned long*, unsigned long*) ../../../../src/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cpp:150\n #3 0x7ff8bb025187 in __sanitizer::GetThreadStackAndTls(bool, unsigned long*, unsigned long*, unsigned long*, unsigned long*) ../../../../src/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cpp:614\n #4 0x7ff8bb017d18 in __lsan::ThreadStart(unsigned int, unsigned long long, __sanitizer::ThreadType) ../../../../src/libsanitizer/lsan/lsan_posix.cpp:53\n #5 0x7ff8bb0143a9 in ThreadStartFunc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:431\n #6 0x7ff8bae9bf51 in start_thread nptl/pthread_create.c:447\n #7 0x7ff8baf1a677 in __clone3 ../sysdeps/unix/sysv/linux/x86_64/clone3.S:78\n \n SUMMARY: LeakSanitizer: 32 byte(s) leaked in 1 allocation(s).\n\n\nIt seems a little funny to me that the different stack unwinder would\nalso have different thread setup code, but I'm not at all familiar with\nlsan's internals, so who knows. I might dig into that but haven't yet\n(and I haven't yet reported it upstream). But anyway, that implies that\nwe can get rid of the race with just:\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 96f2dfb69d..2936e810b5 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -80,7 +80,6 @@ prepend_var ASAN_OPTIONS : detect_leaks=0\n export ASAN_OPTIONS\n \n prepend_var LSAN_OPTIONS : $GIT_SAN_OPTIONS\n-prepend_var LSAN_OPTIONS : fast_unwind_on_malloc=0\n export LSAN_OPTIONS\n \n prepend_var UBSAN_OPTIONS : $GIT_SAN_OPTIONS\n\nAnd indeed, that lets t5309 run successfully under --stress for me (with\nthe log-dir fix from the base of my series).\n\nThat line comes from in 71f26798f2 (test-lib: add \"fast_unwind_on_malloc=0\"\nto LSAN_OPTIONS, 2022-02-27), with the goal of making the backtraces a\nbit more readable. I'm not sure how important it is. I tried reverting a\nrandom commit from one of Patrick's recent leak-fix series:\n\n git revert 58e7568c619cce872b17987f0d14b3de3703129a\n make SANITIZE=leak\n cd t\n ./t1091-sparse-checkout-builtin.sh -i\n\nand the trace looks quite reasonable to me. That old commit 71f2678f2\ngives an example from t0006. That script was marked leak-free by\n974c919d36 (date API: add and use a date_mode_release(), 2022-02-16). If\nwe revert its change to test-date like so:\n\n git diff-tree -Rp 974c919d36 -- t/helper/test-date.c | git apply -3\n make SANITIZE=leak\n cd t\n ./t0006-date.sh -i\n\nthen we do get the same crappy trace as mentioned in 71f26798f2 (if I\nunderstand correctly, this is because libc functions which allocate are\nnot built with -fno-omit-frame-pointer, so we can't walk past them).\n\nSo that line is doing something useful. But it may not be worth the racy\npain it's causing. So some alternatives are:\n\n - we drop that line by default, and then when people are investigating\n a specific leak, they can override LSAN_OPTIONS themselves to get\n better output (though of course knowing that you can even do is\n tricky)\n\n - we keep that line by default, but override LSAN_OPTIONS for CI to\n avoid the race. That makes all local leak-checking traces\n informative by default. But CI ones may be truncated. I'm not sure\n if people use the CI ones directly, or investigate further\n themselves.\n\n - we could annotate individual scripts or even tests to disable the\n option (since it's really just threaded programs). This is more\n hassle, but would limit the blast radius.\n\nI don't love any of those, but they may be less bad than all of the\nbarrier trickery. And it may be that this is even something we could get\nfixed in LSan upstream, and it would just be a temporary workaround. I'm\nstill going to pursue that.\n\nAnd finally, one other option (that I'm not sure why I didn't consider\nbefore): can we just ignore the false positives, similar to what we did\nin 370ef7e40d (test-lib: ignore uninteresting LSan output, 2023-08-28).\nI think we'd have to stop doing abort_on_error for the leak checker and\njust rely on the logs, but that's OK (we always check the logs these\ndays). And detecting the false positive is a little involved. But this\nseems to work:\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 96f2dfb69d..ab7d2d321e 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -80,6 +80,7 @@ prepend_var ASAN_OPTIONS : detect_leaks=0\n export ASAN_OPTIONS\n \n prepend_var LSAN_OPTIONS : $GIT_SAN_OPTIONS\n+prepend_var LSAN_OPTIONS : exitcode=0\n prepend_var LSAN_OPTIONS : fast_unwind_on_malloc=0\n export LSAN_OPTIONS\n \n@@ -346,7 +347,8 @@ nr_san_dir_leaks_ () {\n \tfind \"$TEST_RESULTS_SAN_DIR\" \\\n \t\t-type f \\\n \t\t-name \"$TEST_RESULTS_SAN_FILE_PFX.*\" 2>/dev/null |\n-\txargs grep -lv \"Unable to get registers from thread\" |\n+\txargs grep ^DEDUP_TOKEN |\n+\tgrep -v sanitizer::GetThreadStackTopAndBottom |\n \twc -l\n }\n \n\nA little hacky, but it lets us have our cake and eat it, too. No changes\nto the code, and no bad stack traces.\n\nWhat do you think?\n\n-Peff\n"},{"id":"509776","messageId":"20250101201226.GA3304465@coredump.intra.peff.net","threadId":"62714","inReplyTo":"20250101191422.GC1391912@coredump.intra.peff.net","subject":"[PATCH 0/6] a less-invasive racy-leak fix","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-01T20:12:26Z","receivedAt":"2025-01-01T20:12:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 01, 2025 at 02:14:22PM -0500, Jeff King wrote:\n\n> And finally, one other option (that I'm not sure why I didn't consider\n> before): can we just ignore the false positives, similar to what we did\n> in 370ef7e40d (test-lib: ignore uninteresting LSan output, 2023-08-28).\n> I think we'd have to stop doing abort_on_error for the leak checker and\n> just rely on the logs, but that's OK (we always check the logs these\n> days). And detecting the false positive is a little involved. But this\n> seems to work:\n\nSo here's a clean series that does that. I'm kicking myself a little for\nnot going this route immediately, and spending so much time trying to\ndeal with the actual race in code that is not even ours. :-/\n\nIt would replace what's queued in jk/lsan-race-with-barrier, though the\nfirst two patches remain the same.\n\nSince the grep exit codes and inversions are a little tricky, I checked\nat each step that we continue to correctly report the leaks found by\ndoing:\n\ndiff --git a/t/helper/test-date.c b/t/helper/test-date.c\nindex f25512de9a..7bedb51eaf 100644\n--- a/t/helper/test-date.c\n+++ b/t/helper/test-date.c\n@@ -54,8 +54,6 @@ static void show_dates(const char **argv, const char *format)\n \n \t\tprintf(\"%s -> %s\\n\", *argv, show_date(t, tz, mode));\n \t}\n-\n-\tdate_mode_release(&mode);\n }\n \n static void parse_dates(const char **argv)\n\nand running \"./t0006-date.sh -i\" (which should bail on test 23 and\nreport the full stack trace).\n\n [1/6]: test-lib: use individual lsan dir for --stress runs\n [2/6]: Revert \"index-pack: spawn threads atomically\"\n [3/6]: test-lib: rely on logs to detect leaks\n [4/6]: test-lib: simplify leak-log checking\n [5/6]: test-lib: check leak logs for presence of DEDUP_TOKEN\n [6/6]: test-lib: ignore leaks in the sanitizer's thread code\n\n builtin/index-pack.c | 2 --\n t/test-lib.sh | 25 +++++++++++--------------\n 2 files changed, 11 insertions(+), 16 deletions(-)\n\n-Peff\n"},{"id":"509777","messageId":"20250101201239.GA3305462@coredump.intra.peff.net","threadId":"62714","inReplyTo":"20250101201226.GA3304465@coredump.intra.peff.net","subject":"[PATCH 1/6] test-lib: use individual lsan dir for --stress runs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-01T20:12:39Z","receivedAt":"2025-01-01T20:12:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When storing output in test-results/, we usually give each numbered run\nin a --stress set its own output file. But we don't do that for storing\nLSan logs, so something like:\n\n ./t0003-attributes.sh --stress\n\nwill have many scripts simultaneously creating, writing to, and deleting\nthe test-results/t0003-attributes.leak directory. This can cause logs\nfrom one run to be attributed to another, spurious failures when\ncreation and deletion race, and so on.\n\nThis has always been broken, but nobody noticed because it's rare to do\na --stress run with LSan (since the point is for the code to run quickly\nmany times in order to hit races). But if you're trying to find a race\nin the leak sanitizing code, it makes sense to use these together.\n\nWe can fix it by using $TEST_RESULTS_BASE, which already incorporates\nthe stress job suffix.\n\nSigned-off-by: Jeff King \n---\n t/test-lib.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 1a67adb207..96f2dfb69d 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -331,7 +331,7 @@ TEST_RESULTS_BASE=\"$TEST_RESULTS_DIR/$TEST_NAME$TEST_STRESS_JOB_SFX\"\n TEST_RESULTS_SAN_FILE_PFX=trace\n TEST_RESULTS_SAN_DIR_SFX=leak\n TEST_RESULTS_SAN_FILE=\n-TEST_RESULTS_SAN_DIR=\"$TEST_RESULTS_DIR/$TEST_NAME.$TEST_RESULTS_SAN_DIR_SFX\"\n+TEST_RESULTS_SAN_DIR=\"$TEST_RESULTS_BASE.$TEST_RESULTS_SAN_DIR_SFX\"\n TRASH_DIRECTORY=\"trash directory.$TEST_NAME$TEST_STRESS_JOB_SFX\"\n test -n \"$root\" && TRASH_DIRECTORY=\"$root/$TRASH_DIRECTORY\"\n case \"$TRASH_DIRECTORY\" in\n-- \n2.48.0.rc1.363.g2bf91ec010\n\n"},{"id":"509778","messageId":"20250101201252.GB3305462@coredump.intra.peff.net","threadId":"62714","inReplyTo":"20250101201226.GA3304465@coredump.intra.peff.net","subject":"[PATCH 2/6] Revert \"index-pack: spawn threads atomically\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-01T20:12:52Z","receivedAt":"2025-01-01T20:12:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This reverts commit 993d38a0669a8056d496797516e743e26b6b8b54.\n\nThat commit was trying to solve a race between LSan setting up the\nthreads stack and another thread calling exit(), by making sure that all\npthread_create() calls have finished before doing any work that might\ntrigger the exit().\n\nBut that isn't sufficient. The setup code actually runs in the\nindividual threads themselves, not in the spawning thread's call to\npthread_create(). So while it may have improved the race a bit, you can\nstill trigger it pretty quickly with:\n\n make SANITIZE=leak\n cd t\n ./t5309-pack-delta-cycles.sh --stress\n\nLet's back out that failed attempt so we can try again.\n\nSigned-off-by: Jeff King \n---\n builtin/index-pack.c | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex d773809c4c..0b62b2589f 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -1336,15 +1336,13 @@ static void resolve_deltas(struct pack_idx_option *opts)\n \tbase_cache_limit = opts->delta_base_cache_limit * nr_threads;\n \tif (nr_threads > 1 || getenv(\"GIT_FORCE_THREADS\")) {\n \t\tinit_thread();\n-\t\twork_lock();\n \t\tfor (i = 0; i < nr_threads; i++) {\n \t\t\tint ret = pthread_create(&thread_data[i].thread, NULL,\n \t\t\t\t\t\t threaded_second_pass, thread_data + i);\n \t\t\tif (ret)\n \t\t\t\tdie(_(\"unable to create thread: %s\"),\n \t\t\t\t strerror(ret));\n \t\t}\n-\t\twork_unlock();\n \t\tfor (i = 0; i < nr_threads; i++)\n \t\t\tpthread_join(thread_data[i].thread, NULL);\n \t\tcleanup_thread();\n-- \n2.48.0.rc1.363.g2bf91ec010\n\n"},{"id":"509779","messageId":"20250101201444.GC3305462@coredump.intra.peff.net","threadId":"62714","inReplyTo":"20250101201226.GA3304465@coredump.intra.peff.net","subject":"[PATCH 3/6] test-lib: rely on logs to detect leaks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-01T20:14:44Z","receivedAt":"2025-01-01T20:14:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we run with sanitizers, we set abort_on_error=1 so that the tests\nthemselves can detect problems directly (when the buggy program exits\nwith SIGABRT). This has one blind spot, though: we don't always check\nthe exit codes for all programs (e.g., helpers like upload-pack invoked\nbehind the scenes).\n\nFor ASan and UBSan this is mostly fine; they exit as soon as they see an\nerror, so the unexpected abort of the program causes the test to fail\nanyway.\n\nBut for LSan, the program runs to completion, since we can only check\nfor leaks at the end. And in that case we could miss leak reports. And\nthus we started checking LSan logs in faececa53f (test-lib: have the\n\"check\" mode for SANITIZE=leak consider leak logs, 2022-07-28).\nOriginally the logs were optional, but logs are generated (and checked)\nalways as of 8c1d6691bc (test-lib: GIT_TEST_SANITIZE_LEAK_LOG enabled by\ndefault, 2024-07-11). And we even check them for each test snippet, as\nof cf1464331b (test-lib: check for leak logs after every test,\n2024-09-24).\n\nSo now aborting on error is superfluous for LSan! We can get everything\nwe need by checking the logs. And checking the logs is actually\npreferable, since it gives us more control over silencing false\npositives (something we do not yet do, but will soon).\n\nSo let's tell LSan to just exit normally, even if it finds leaks. We can\ndo so with exitcode=0, which also suppresses the abort_on_error flag.\n\nSigned-off-by: Jeff King \n---\n t/test-lib.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 96f2dfb69d..dd2ba6e6cc 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -80,6 +80,7 @@ prepend_var ASAN_OPTIONS : detect_leaks=0\n export ASAN_OPTIONS\n \n prepend_var LSAN_OPTIONS : $GIT_SAN_OPTIONS\n+prepend_var LSAN_OPTIONS : exitcode=0\n prepend_var LSAN_OPTIONS : fast_unwind_on_malloc=0\n export LSAN_OPTIONS\n \n-- \n2.48.0.rc1.363.g2bf91ec010\n\n"},{"id":"509780","messageId":"20250101201721.GD3305462@coredump.intra.peff.net","threadId":"62714","inReplyTo":"20250101201226.GA3304465@coredump.intra.peff.net","subject":"[PATCH 4/6] test-lib: simplify leak-log checking","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-01T20:17:21Z","receivedAt":"2025-01-01T20:17:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We have a function to count the number of leaks found (actually, it is\nthe number of processes which produced a log file). Once upon a time we\ncared about seeing if this number increased between runs. But we\nsimplified that away in 95c679ad86 (test-lib: stop showing old leak\nlogs, 2024-09-24), and now we only care if it returns any results or\nnot.\n\nIn preparation for refactoring it further, let's drop the counting\nfunction entirely, and roll it into the \"is it empty\" check. The outcome\nshould be the same, but we'll be free to return a boolean \"did we find\nanything\" without worrying about somebody adding a new call to the\ncounting function.\n\nSigned-off-by: Jeff King \n---\nWe need the extra \"!\" to invert the exit code of the final grep, which\nmade my head spin a little at first. Maybe it would be less confusing if\nthis was reporting non-empty results, as \"check_test_results_has_leaks\"\nor something? We'd have to update the callers, but there are only 2 of\nthem. I dunno.\n\n t/test-lib.sh | 21 ++++++++-------------\n 1 file changed, 8 insertions(+), 13 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex dd2ba6e6cc..23eb26bfbe 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -340,17 +340,6 @@ case \"$TRASH_DIRECTORY\" in\n *) TRASH_DIRECTORY=\"$TEST_OUTPUT_DIRECTORY/$TRASH_DIRECTORY\" ;;\n esac\n \n-# Utility functions using $TEST_RESULTS_* variables\n-nr_san_dir_leaks_ () {\n-\t# stderr piped to /dev/null because the directory may have\n-\t# been \"rmdir\"'d already.\n-\tfind \"$TEST_RESULTS_SAN_DIR\" \\\n-\t\t-type f \\\n-\t\t-name \"$TEST_RESULTS_SAN_FILE_PFX.*\" 2>/dev/null |\n-\txargs grep -lv \"Unable to get registers from thread\" |\n-\twc -l\n-}\n-\n # If --stress was passed, run this test repeatedly in several parallel loops.\n if test \"$GIT_TEST_STRESS_STARTED\" = \"done\"\n then\n@@ -1181,8 +1170,14 @@ test_atexit_handler () {\n }\n \n check_test_results_san_file_empty_ () {\n-\ttest -z \"$TEST_RESULTS_SAN_FILE\" ||\n-\ttest \"$(nr_san_dir_leaks_)\" = 0\n+\ttest -z \"$TEST_RESULTS_SAN_FILE\" && return 0\n+\n+\t# stderr piped to /dev/null because the directory may have\n+\t# been \"rmdir\"'d already.\n+\t! find \"$TEST_RESULTS_SAN_DIR\" \\\n+\t\t-type f \\\n+\t\t-name \"$TEST_RESULTS_SAN_FILE_PFX.*\" 2>/dev/null |\n+\txargs grep -qv \"Unable to get registers from thread\"\n }\n \n check_test_results_san_file_ () {\n-- \n2.48.0.rc1.363.g2bf91ec010\n\n"},{"id":"509781","messageId":"20250101201828.GE3305462@coredump.intra.peff.net","threadId":"62714","inReplyTo":"20250101201226.GA3304465@coredump.intra.peff.net","subject":"[PATCH 5/6] test-lib: check leak logs for presence of DEDUP_TOKEN","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-01T20:18:28Z","receivedAt":"2025-01-01T20:18:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we check the leak logs, our original strategy was to check for any\nnon-empty log file produced by LSan. We later amended that to ignore\nnoisy lines in 370ef7e40d (test-lib: ignore uninteresting LSan output,\n2023-08-28).\n\nThis makes it hard to ignore noise which is more than a single line;\nwe'd have to actually parse the file to determine the meaning of each\nline.\n\nBut there's an easy line-oriented solution. Because we always pass the\ndedup_token_length option, the output will contain a DEDUP_TOKEN line\nfor each leak that has been found. So if we invert our strategy to stop\nignoring useless lines and only look for useful ones, we can just count\nthe number of DEDUP_TOKEN lines. If it's non-zero, then we found at\nleast one leak (it would even give us a count of unique leaks, but we\nreally only care if it is non-zero).\n\nThis should yield the same outcome, but will help us build more false\npositive detection on top.\n\nSigned-off-by: Jeff King \n---\n t/test-lib.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 23eb26bfbe..c9487d0805 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1177,7 +1177,7 @@ check_test_results_san_file_empty_ () {\n \t! find \"$TEST_RESULTS_SAN_DIR\" \\\n \t\t-type f \\\n \t\t-name \"$TEST_RESULTS_SAN_FILE_PFX.*\" 2>/dev/null |\n-\txargs grep -qv \"Unable to get registers from thread\"\n+\txargs grep -q ^DEDUP_TOKEN\n }\n \n check_test_results_san_file_ () {\n-- \n2.48.0.rc1.363.g2bf91ec010\n\n"},{"id":"509782","messageId":"20250101202124.GF3305462@coredump.intra.peff.net","threadId":"62714","inReplyTo":"20250101201226.GA3304465@coredump.intra.peff.net","subject":"[PATCH 6/6] test-lib: ignore leaks in the sanitizer's thread code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-01T20:21:24Z","receivedAt":"2025-01-01T20:21:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Our CI jobs sometimes see false positive leaks like this:\n\n =================================================================\n ==3904583==ERROR: LeakSanitizer: detected memory leaks\n\n Direct leak of 32 byte(s) in 1 object(s) allocated from:\n #0 0x7fa790d01986 in __interceptor_realloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:98\n #1 0x7fa790add769 in __pthread_getattr_np nptl/pthread_getattr_np.c:180\n #2 0x7fa790d117c5 in __sanitizer::GetThreadStackTopAndBottom(bool, unsigned long*, unsigned long*) ../../../../src/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cpp:150\n #3 0x7fa790d11957 in __sanitizer::GetThreadStackAndTls(bool, unsigned long*, unsigned long*, unsigned long*, unsigned long*) ../../../../src/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cpp:598\n #4 0x7fa790d03fe8 in __lsan::ThreadStart(unsigned int, unsigned long long, __sanitizer::ThreadType) ../../../../src/libsanitizer/lsan/lsan_posix.cpp:51\n #5 0x7fa790d013fd in __lsan_thread_start_func ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:440\n #6 0x7fa790adc3eb in start_thread nptl/pthread_create.c:444\n #7 0x7fa790b5ca5b in clone3 ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81\n\nThis is not a leak in our code, but appears to be a race between one\nthread calling exit() while another one is in LSan's stack setup code.\nYou can reproduce it easily by running t0003 or t5309 with --stress\n(these trigger it because of the threading in git-grep and index-pack\nrespectively).\n\nThis may be a bug in LSan, but regardless of whether it is eventually\nfixed, it is useful to work around it so that we stop seeing these false\npositives.\n\nWe can recognize it by the mention of the sanitizer functions in the\nDEDUP_TOKEN line. With this patch, the scripts mentioned above should\nrun with --stress indefinitely.\n\nSigned-off-by: Jeff King \n---\nOne small downside here is that this just suppresses the \"were there any\nleaks\" check. If there's a real leak _and_ the race was triggered, then\nyou'd see the racy false positive in the output. I don't think that's a\nbig deal, since both the race and real leaks should be rare-ish, and\nyou'd have to encounter both in the same run of a given test script.\n\n t/test-lib.sh | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex c9487d0805..d1f62adbf8 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1177,7 +1177,8 @@ check_test_results_san_file_empty_ () {\n \t! find \"$TEST_RESULTS_SAN_DIR\" \\\n \t\t-type f \\\n \t\t-name \"$TEST_RESULTS_SAN_FILE_PFX.*\" 2>/dev/null |\n-\txargs grep -q ^DEDUP_TOKEN\n+\txargs grep ^DEDUP_TOKEN |\n+\tgrep -qv sanitizer::GetThreadStackTopAndBottom\n }\n \n check_test_results_san_file_ () {\n-- \n2.48.0.rc1.363.g2bf91ec010\n"},{"id":"509784","messageId":"xmqqa5cavz8h.fsf@gitster.g","threadId":"62714","inReplyTo":"20250101191422.GC1391912@coredump.intra.peff.net","subject":"Re: a less-invasive racy-leak fix, was Re: What's cooking in git.git (Dec 2024, #11; Mon, 30)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-02T00:25:02Z","receivedAt":"2025-01-02T00:25:05Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Dec 30, 2024 at 09:33:20AM -0800, Junio C Hamano wrote:\n>\n>> * jk/lsan-race-with-barrier (2024-12-30) 5 commits\n> ...\n> This graduated faster than I expected. :)\n\nHeh, it is before -rc2 and the change is only about tests, so ...\n\n> ...\n> So that line is doing something useful. But it may not be worth the racy\n> pain it's causing. So some alternatives are:\n>\n> - we drop that line by default, and then when people are investigating\n> a specific leak, they can override LSAN_OPTIONS themselves to get\n> better output (though of course knowing that you can even do is\n> tricky)\n>\n> - we keep that line by default, but override LSAN_OPTIONS for CI to\n> avoid the race. That makes all local leak-checking traces\n> informative by default. But CI ones may be truncated. I'm not sure\n> if people use the CI ones directly, or investigate further\n> themselves.\n>\n> - we could annotate individual scripts or even tests to disable the\n> option (since it's really just threaded programs). This is more\n> hassle, but would limit the blast radius.\n>\n> I don't love any of those, but they may be less bad than all of the\n> barrier trickery. And it may be that this is even something we could get\n> fixed in LSan upstream, and it would just be a temporary workaround. I'm\n> still going to pursue that.\n>\n> And finally, one other option (that I'm not sure why I didn't consider\n> before): can we just ignore the false positives, similar to what we did\n> in 370ef7e40d (test-lib: ignore uninteresting LSan output, 2023-08-28).\n\nGood point.\n\n> I think we'd have to stop doing abort_on_error for the leak checker and\n> just rely on the logs, but that's OK (we always check the logs these\n> days).\n> ...\n> A little hacky, but it lets us have our cake and eat it, too. No changes\n> to the code, and no bad stack traces.\n>\n> What do you think?\n\nI like the small hack. \"This is ultimately LSan's racy-ness and not\nours, so let's avoid changing our code to work it around when we can\ndo the workaround somewhere else\" is an attitude that I would endorse\nfully.\n\nThanks.\n\n\n\n\n"},{"id":"509785","messageId":"20250102023224.GA3853144@coredump.intra.peff.net","threadId":"62714","inReplyTo":"xmqqa5cavz8h.fsf@gitster.g","subject":"Re: a less-invasive racy-leak fix, was Re: What's cooking in git.git (Dec 2024, #11; Mon, 30)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-02T02:32:24Z","receivedAt":"2025-01-02T02:32:27Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 01, 2025 at 04:25:02PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > On Mon, Dec 30, 2024 at 09:33:20AM -0800, Junio C Hamano wrote:\n> >\n> >> * jk/lsan-race-with-barrier (2024-12-30) 5 commits\n> > ...\n> > This graduated faster than I expected. :)\n> \n> Heh, it is before -rc2 and the change is only about tests, so ...\n\nYeah, I figured as much. I also considered it of relatively low\nimportance during -rc, but I guess CI false positives do tend to annoy\neverybody and waste their time. :)\n\nIt looks like you pushed out the version of 'master' with it merged. I\nhad figured you'd revert jk/lsan-race-with-barrier out of next, so I\nwondered how we would proceed (revert the whole merge from master to\nrebuild, or do a moral revert of the final three).\n\nLooking at jk/lsan-race-ignore-false-positive, it looks like you did the\nmoral revert via fc89d14c63 (Revert barrier-based LSan threading race\nworkaround, 2025-01-01). That commit's tree matches what I'd expect (I\nguess you probably used \"revert -n HEAD~3..HEAD\" just like I did).\n\nIt would be nice if the 3-commit revert mentioned the specific commits\nit was reverting. If you revert the whole merge, you get the merge\ncommit's id and you can find the original commits by inspecting the\nhistory (but of course here we were reverting only part of it, so we\ncouldn't do that here). If you revert the sequence without \"-n\" you get\nthree individual reverts. Which is informative, but a little clunky in\nthe history.\n\nI wonder if revert should have a \"squash\" mode that reverts all of the\ncommits (perhaps in reverse order of application in case they depend on\neach other textually), and then gives you a commit message template\nsimilar to git-fmt-merge-msg, where we list all of the commits, one per\nline (though probably with their commit ids in this case).\n\nProbably not a big deal either way, and certainly not a blocker for the\nseries. Thanks (as usual) for doing all the maintainer juggling.\n\n-Peff\n"},{"id":"509786","messageId":"CAPx1GveQPRop_qJgAQxq+6-2ZsXX6x89sTyTtYFvbMfET09z+g@mail.gmail.com","threadId":"62714","inReplyTo":"20250102023224.GA3853144@coredump.intra.peff.net","subject":"Re: a less-invasive racy-leak fix, was Re: What's cooking in git.git (Dec 2024, #11; Mon, 30)","fromName":"Chris Torek","fromEmail":"chris.torek@gmail.com","sentAt":"2025-01-02T02:41:01Z","receivedAt":"2025-01-02T02:41:15Z","isPatch":false,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"On Wed, Jan 1, 2025 at 6:32 PM Jeff King wrote:\n> I wonder if revert should have a \"squash\" mode that reverts all of the\n> commits (perhaps in reverse order of application in case they depend on\n> each other textually), and then gives you a commit message template\n> similar to git-fmt-merge-msg, where we list all of the commits, one per\n> line (though probably with their commit ids in this case).\n\nI have kind of wanted this in the past, but never enough to build it, I just\ndid a few separate reverts.\n\nChris\n"},{"id":"509787","messageId":"20250102032404.GA817606@coredump.intra.peff.net","threadId":"62714","inReplyTo":"xmqqa5cavz8h.fsf@gitster.g","subject":"Re: a less-invasive racy-leak fix, was Re: What's cooking in git.git (Dec 2024, #11; Mon, 30)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-02T03:24:04Z","receivedAt":"2025-01-02T03:24:05Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 01, 2025 at 04:25:02PM -0800, Junio C Hamano wrote:\n\n> > What do you think?\n> \n> I like the small hack. \"This is ultimately LSan's racy-ness and not\n> ours, so let's avoid changing our code to work it around when we can\n> do the workaround somewhere else\" is an attitude that I would endorse\n> fully.\n\nBTW, in case anybody wants to follow along with what happens upstream, I\nreported it here:\n\n https://github.com/google/sanitizers/issues/1836\n\n-Peff\n"},{"id":"509798","messageId":"xmqqzfk9uvjd.fsf@gitster.g","threadId":"62714","inReplyTo":"20250102023224.GA3853144@coredump.intra.peff.net","subject":"Re: a less-invasive racy-leak fix, was Re: What's cooking in git.git (Dec 2024, #11; Mon, 30)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-02T14:42:30Z","receivedAt":"2025-01-02T14:42:33Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Jan 01, 2025 at 04:25:02PM -0800, Junio C Hamano wrote:\n>\n>> Jeff King writes:\n>> \n>> > On Mon, Dec 30, 2024 at 09:33:20AM -0800, Junio C Hamano wrote:\n>> >\n>> >> * jk/lsan-race-with-barrier (2024-12-30) 5 commits\n>> > ...\n>> > This graduated faster than I expected. :)\n>> \n>> Heh, it is before -rc2 and the change is only about tests, so ...\n>\n> Yeah, I figured as much. I also considered it of relatively low\n> importance during -rc, but I guess CI false positives do tend to annoy\n> everybody and waste their time. :)\n>\n> It looks like you pushed out the version of 'master' with it merged. I\n> had figured you'd revert jk/lsan-race-with-barrier out of next, so I\n> wondered how we would proceed (revert the whole merge from master to\n> rebuild, or do a moral revert of the final three).\n\nRevert the effect of the tip-part (except for the bottom two) and\nthen queue the new ones, which would allow me to merge the whole\nthing in one go without losing the bottom two's effect (which would\nhappen if we reverted the whole thing first, and then reused the\nbottom two commits to build the new iteration on top).\n\n> Looking at jk/lsan-race-ignore-false-positive, it looks like you did the\n> moral revert via fc89d14c63 (Revert barrier-based LSan threading race\n> workaround, 2025-01-01). That commit's tree matches what I'd expect (I\n> guess you probably used \"revert -n HEAD~3..HEAD\" just like I did).\n\nI actually did \"read-tree -u -m\" followed by \"commit\" ;-) \n\n> It would be nice if the 3-commit revert mentioned the specific commits\n> it was reverting.\n\nTrue. I should probably amend while I can.\n\n> I wonder if revert should have a \"squash\" mode that reverts all of the\n> commits (perhaps in reverse order of application in case they depend on\n> each other textually), and then gives you a commit message template\n> similar to git-fmt-merge-msg, where we list all of the commits, one per\n> line (though probably with their commit ids in this case).\n\nI am not sure if I follow. Should \"revert HEAD~3..HEAD\" give such\nconcatenation of messages, something similar to what \"rebase -i\"\ngives us when seeing multiple \"squash\"es in a row?\n"},{"id":"509811","messageId":"20250102190623.GA848764@coredump.intra.peff.net","threadId":"62714","inReplyTo":"xmqqzfk9uvjd.fsf@gitster.g","subject":"Re: a less-invasive racy-leak fix, was Re: What's cooking in git.git (Dec 2024, #11; Mon, 30)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-02T19:06:23Z","receivedAt":"2025-01-02T19:06:33Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 02, 2025 at 06:42:30AM -0800, Junio C Hamano wrote:\n\n> > I wonder if revert should have a \"squash\" mode that reverts all of the\n> > commits (perhaps in reverse order of application in case they depend on\n> > each other textually), and then gives you a commit message template\n> > similar to git-fmt-merge-msg, where we list all of the commits, one per\n> > line (though probably with their commit ids in this case).\n> \n> I am not sure if I follow. Should \"revert HEAD~3..HEAD\" give such\n> concatenation of messages, something similar to what \"rebase -i\"\n> gives us when seeing multiple \"squash\"es in a row?\n\nI don't think we need to concatenate all of the individual revert\nmessages. I was thinking of producing something more like:\n\n \n\n Revert the following commits:\n\n - 7a8d9efc26 (grep: work around LSan threading race with barrier, 2024-12-29)\n - 526c0a851b (index-pack: work around LSan threading race with barrier, 2024-12-29)\n - 7d0037b59a (thread-utils: introduce optional barrier type, 2024-12-29)\n\nYou could perhaps even auto-populate the subject with:\n\n Revert jk/lsan-race-with-barrier~3..jk/lsan-race-with-barrier\n\nsimilar to how git-merge uses \"Merge branch ...\". But it's a little\nclunky to read, and unlike merge, it's a lot easier to use names that\nare not very meaningful (e.g., I checked out a new branch based on that\none and then used HEAD~3..HEAD, which is worthless to mention).\n\n-Peff\n"},{"id":"509812","messageId":"xmqq5xmxui37.fsf@gitster.g","threadId":"62714","inReplyTo":"20250102190623.GA848764@coredump.intra.peff.net","subject":"Re: a less-invasive racy-leak fix, was Re: What's cooking in git.git (Dec 2024, #11; Mon, 30)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-02T19:33:00Z","receivedAt":"2025-01-02T19:33:03Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> \n>\n> Revert the following commits:\n>\n> - 7a8d9efc26 (grep: work around LSan threading race with barrier, 2024-12-29)\n> - 526c0a851b (index-pack: work around LSan threading race with barrier, 2024-12-29)\n> - 7d0037b59a (thread-utils: introduce optional barrier type, 2024-12-29)\n\nAh, I love it.\n\n> You could perhaps even auto-populate the subject with:\n>\n> Revert jk/lsan-race-with-barrier~3..jk/lsan-race-with-barrier\n>\n> similar to how git-merge uses \"Merge branch ...\". But it's a little\n> clunky to read, and unlike merge, it's a lot easier to use names that\n> are not very meaningful (e.g., I checked out a new branch based on that\n> one and then used HEAD~3..HEAD, which is worthless to mention).\n\nYup, agreed. The commit title is much less interesting to automate\nthan the \"we revert these three\" list.\n\nThanks.\n"},{"id":"509827","messageId":"Z3eUNedx9PNPoukE@pks.im","threadId":"62714","inReplyTo":"003619a3-f544-4b20-9685-866fd2ed1ae2@web.de","subject":"Re: What's cooking in git.git (Dec 2024, #11; Mon, 30)","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-03T07:39:33Z","receivedAt":"2025-01-03T07:39:37Z","isPatch":false,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Dec 31, 2024 at 06:27:16PM +0100, René Scharfe wrote:\n> Am 30.12.24 um 18:33 schrieb Junio C Hamano:\n> > * rs/reftable-realloc-errors (2024-12-28) 4 commits\n> > (merged to 'next' on 2024-12-30 at ebc9625a4c)\n> > + t-reftable-merged: handle realloc errors\n> > + reftable: handle realloc error in parse_names()\n> > + reftable: fix allocation count on realloc error\n> > + reftable: avoid leaks on realloc error\n> >\n> > The custom allocator code in the reftable library did not handle\n> > failing realloc() very well, which has been addressed.\n> >\n> > Will merge to 'master'?\n> \n> Reftable allocation error handling was introduced by bcd5a4059a\n> (reftable/error: introduce out-of-memory error code, 2024-10-02) after\n> v2.47.1, and this series improves it, so I'd say yes. But of course\n> I'm biased.\n\nI'm aligned with you there. Thanks!\n\nPatrick\n"},{"id":"509841","messageId":"Z3fSj-NsSg2El2wI@pks.im","threadId":"62714","inReplyTo":"20250101201444.GC3305462@coredump.intra.peff.net","subject":"Re: [PATCH 3/6] test-lib: rely on logs to detect leaks","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-03T12:05:41Z","receivedAt":"2025-01-03T12:05:46Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Jan 01, 2025 at 03:14:44PM -0500, Jeff King wrote:\n> When we run with sanitizers, we set abort_on_error=1 so that the tests\n> themselves can detect problems directly (when the buggy program exits\n> with SIGABRT). This has one blind spot, though: we don't always check\n> the exit codes for all programs (e.g., helpers like upload-pack invoked\n> behind the scenes).\n> \n> For ASan and UBSan this is mostly fine; they exit as soon as they see an\n> error, so the unexpected abort of the program causes the test to fail\n> anyway.\n> \n> But for LSan, the program runs to completion, since we can only check\n> for leaks at the end. And in that case we could miss leak reports. And\n> thus we started checking LSan logs in faececa53f (test-lib: have the\n> \"check\" mode for SANITIZE=leak consider leak logs, 2022-07-28).\n> Originally the logs were optional, but logs are generated (and checked)\n> always as of 8c1d6691bc (test-lib: GIT_TEST_SANITIZE_LEAK_LOG enabled by\n> default, 2024-07-11). And we even check them for each test snippet, as\n> of cf1464331b (test-lib: check for leak logs after every test,\n> 2024-09-24).\n> \n> So now aborting on error is superfluous for LSan! We can get everything\n> we need by checking the logs. And checking the logs is actually\n> preferable, since it gives us more control over silencing false\n> positives (something we do not yet do, but will soon).\n> \n> So let's tell LSan to just exit normally, even if it finds leaks. We can\n> do so with exitcode=0, which also suppresses the abort_on_error flag.\n\nThe only downside I can think of is that we now run the whole testcase\nto completion before checking for leaks, whereas beforehand we most\nlikely aborted the testcase on hitting the first leak. It follows that\nwe may now have multiple leak reports, and it is not immediately clear\nwhich of the commands has actually been failing.\n\nI think we're now in a clean-enough state regarding memory leaks that\nthis isn't a huge issue anymore though.\n\nPatrick\n"},{"id":"509842","messageId":"Z3fSmSYoWs-a6afW@pks.im","threadId":"62714","inReplyTo":"20250101201721.GD3305462@coredump.intra.peff.net","subject":"Re: [PATCH 4/6] test-lib: simplify leak-log checking","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-03T12:05:45Z","receivedAt":"2025-01-03T12:05:56Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Jan 01, 2025 at 03:17:21PM -0500, Jeff King wrote:\n> @@ -1181,8 +1170,14 @@ test_atexit_handler () {\n> }\n> \n> check_test_results_san_file_empty_ () {\n> -\ttest -z \"$TEST_RESULTS_SAN_FILE\" ||\n> -\ttest \"$(nr_san_dir_leaks_)\" = 0\n> +\ttest -z \"$TEST_RESULTS_SAN_FILE\" && return 0\n> +\n> +\t# stderr piped to /dev/null because the directory may have\n> +\t# been \"rmdir\"'d already.\n> +\t! find \"$TEST_RESULTS_SAN_DIR\" \\\n> +\t\t-type f \\\n> +\t\t-name \"$TEST_RESULTS_SAN_FILE_PFX.*\" 2>/dev/null |\n> +\txargs grep -qv \"Unable to get registers from thread\"\n\nCan't we use `-exec grep -qv \"Unable to get registers from thread\" {}\n\\+` instead of using xargs? Or is that unportable? Might make it a bit\neasier to reason about the `!` in the presence of a pipe.\n\nPatrick\n"},{"id":"509843","messageId":"Z3fSnK21nRBrjyfM@pks.im","threadId":"62714","inReplyTo":"20250101202124.GF3305462@coredump.intra.peff.net","subject":"Re: [PATCH 6/6] test-lib: ignore leaks in the sanitizer's thread code","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-03T12:05:48Z","receivedAt":"2025-01-03T12:05:57Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Jan 01, 2025 at 03:21:24PM -0500, Jeff King wrote:\n> One small downside here is that this just suppresses the \"were there any\n> leaks\" check. If there's a real leak _and_ the race was triggered, then\n> you'd see the racy false positive in the output. I don't think that's a\n> big deal, since both the race and real leaks should be rare-ish, and\n> you'd have to encounter both in the same run of a given test script.\n\nYeah, I think that's fine. You'd have to both be unlucky and trigger the\nleak _and_ you'd have to be debugging a testcase that hits the race in\nthe first place. Which together feels unlikely enough to really matter.\n\n> t/test-lib.sh | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n> \n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index c9487d0805..d1f62adbf8 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -1177,7 +1177,8 @@ check_test_results_san_file_empty_ () {\n> \t! find \"$TEST_RESULTS_SAN_DIR\" \\\n> \t\t-type f \\\n> \t\t-name \"$TEST_RESULTS_SAN_FILE_PFX.*\" 2>/dev/null |\n> -\txargs grep -q ^DEDUP_TOKEN\n> +\txargs grep ^DEDUP_TOKEN |\n> +\tgrep -qv sanitizer::GetThreadStackTopAndBottom\n> }\n\nIt would be nice to provide some more context here in the form of a\ncomment so that one doesn't have to blame the commit.\n\nPatrick\n"},{"id":"509888","messageId":"20250103201050.GB3212696@coredump.intra.peff.net","threadId":"62714","inReplyTo":"Z3fSj-NsSg2El2wI@pks.im","subject":"Re: [PATCH 3/6] test-lib: rely on logs to detect leaks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-03T20:10:50Z","receivedAt":"2025-01-03T20:10:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 03, 2025 at 01:05:41PM +0100, Patrick Steinhardt wrote:\n\n> > So now aborting on error is superfluous for LSan! We can get everything\n> > we need by checking the logs. And checking the logs is actually\n> > preferable, since it gives us more control over silencing false\n> > positives (something we do not yet do, but will soon).\n> > \n> > So let's tell LSan to just exit normally, even if it finds leaks. We can\n> > do so with exitcode=0, which also suppresses the abort_on_error flag.\n> \n> The only downside I can think of is that we now run the whole testcase\n> to completion before checking for leaks, whereas beforehand we most\n> likely aborted the testcase on hitting the first leak. It follows that\n> we may now have multiple leak reports, and it is not immediately clear\n> which of the commands has actually been failing.\n\nTrue, I didn't think of that. We do at least check the logs after each\ncase, so it would have to be multiple leaks in the same snippet. The\nLSan output also mentions the process name, though not the arguments\n(and some snippets may invoke the same program multiple times).\n\nThe other thing I guess we'd miss is that SIGABRT can optionally produce\na core dump. I don't think I've ever found that useful for LSan (since\nit's not exiting until the end of process) but occasionally have used it\nfor ASan (though this patch does not change anything there).\n\n> I think we're now in a clean-enough state regarding memory leaks that\n> this isn't a huge issue anymore though.\n\nYeah, agreed. We can see how much of a problem it is in practice. You\ncan always switch the flag yourself for a specific run, like:\n\n LSAN_OPTIONS=exitcode=23 ./twhatever -i\n\n-Peff\n"},{"id":"509890","messageId":"20250103202410.GC3212696@coredump.intra.peff.net","threadId":"62714","inReplyTo":"Z3fSmSYoWs-a6afW@pks.im","subject":"Re: [PATCH 4/6] test-lib: simplify leak-log checking","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-03T20:24:10Z","receivedAt":"2025-01-03T20:24:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 03, 2025 at 01:05:45PM +0100, Patrick Steinhardt wrote:\n\n> On Wed, Jan 01, 2025 at 03:17:21PM -0500, Jeff King wrote:\n> > @@ -1181,8 +1170,14 @@ test_atexit_handler () {\n> > }\n> > \n> > check_test_results_san_file_empty_ () {\n> > -\ttest -z \"$TEST_RESULTS_SAN_FILE\" ||\n> > -\ttest \"$(nr_san_dir_leaks_)\" = 0\n> > +\ttest -z \"$TEST_RESULTS_SAN_FILE\" && return 0\n> > +\n> > +\t# stderr piped to /dev/null because the directory may have\n> > +\t# been \"rmdir\"'d already.\n> > +\t! find \"$TEST_RESULTS_SAN_DIR\" \\\n> > +\t\t-type f \\\n> > +\t\t-name \"$TEST_RESULTS_SAN_FILE_PFX.*\" 2>/dev/null |\n> > +\txargs grep -qv \"Unable to get registers from thread\"\n> \n> Can't we use `-exec grep -qv \"Unable to get registers from thread\" {}\n> \\+` instead of using xargs? Or is that unportable? Might make it a bit\n> easier to reason about the `!` in the presence of a pipe.\n\nI don't think that saves us from negating, though. The \"grep\" will tell\nus if it matched any \"real\" lines, but we want to report that we found\nno real lines.\n\nPlus I don't think \"find\" propagates the exit code from -exec anyway. I\nthink you can check the exit status with more find logic, so you'd then\nuse a conditional -print for each file like:\n\n find ... \\\n -exec grep -qv \"Unable to get registers from thread\" \\{} \\; \\\n -print\n\nand you have to check whether the output is empty. The easiest way to do\nthat is with another grep! Which also needs negated. ;)\n\nI think if we really want to drop the negation, we'd be best to flip the\nfunction's return, like:\n\n have_leaks() {\n\t# not leak-checking\n\ttest -z \"$TEST_RESULTS_SAN_FILE\" && return 1\n\n\tfind \"$TEST_RESULTS_SAN_DIR\" \\\n\t\t-type f \\\n\t\t-name \"$TEST_RESULTS_SAN_FILE_PFX.*\" 2>/dev/null |\n\txargs grep ^DEDUP_TOKEN |\n\tgrep -qv sanitizer::GetThreadStackTopAndBottom\n }\n\nAnd then you could switch the initial \"grep\" to -exec if you want, but\nthere's no negation to get rid of, so it is only a preference of -exec\nversus xargs.\n\n-Peff\n"},{"id":"509892","messageId":"20250103202645.GD3212696@coredump.intra.peff.net","threadId":"62714","inReplyTo":"Z3fSnK21nRBrjyfM@pks.im","subject":"Re: [PATCH 6/6] test-lib: ignore leaks in the sanitizer's thread code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-03T20:26:45Z","receivedAt":"2025-01-03T20:26:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 03, 2025 at 01:05:48PM +0100, Patrick Steinhardt wrote:\n\n> > diff --git a/t/test-lib.sh b/t/test-lib.sh\n> > index c9487d0805..d1f62adbf8 100644\n> > --- a/t/test-lib.sh\n> > +++ b/t/test-lib.sh\n> > @@ -1177,7 +1177,8 @@ check_test_results_san_file_empty_ () {\n> > \t! find \"$TEST_RESULTS_SAN_DIR\" \\\n> > \t\t-type f \\\n> > \t\t-name \"$TEST_RESULTS_SAN_FILE_PFX.*\" 2>/dev/null |\n> > -\txargs grep -q ^DEDUP_TOKEN\n> > +\txargs grep ^DEDUP_TOKEN |\n> > +\tgrep -qv sanitizer::GetThreadStackTopAndBottom\n> > }\n> \n> It would be nice to provide some more context here in the form of a\n> comment so that one doesn't have to blame the commit.\n\nWe can add that on top, but I'm not sure what it should say. Do you want\nsomething along the lines of \"add false positives to ignore here...\" or\nare an explanation of why we are ignoring this particular false\npositive?\n\n-Peff\n"},{"id":"509960","messageId":"Z3uMxScZGjBBTFUU@pks.im","threadId":"62714","inReplyTo":"20250103202645.GD3212696@coredump.intra.peff.net","subject":"Re: [PATCH 6/6] test-lib: ignore leaks in the sanitizer's thread code","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-06T07:56:53Z","receivedAt":"2025-01-06T07:56:58Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Jan 03, 2025 at 03:26:45PM -0500, Jeff King wrote:\n> On Fri, Jan 03, 2025 at 01:05:48PM +0100, Patrick Steinhardt wrote:\n> \n> > > diff --git a/t/test-lib.sh b/t/test-lib.sh\n> > > index c9487d0805..d1f62adbf8 100644\n> > > --- a/t/test-lib.sh\n> > > +++ b/t/test-lib.sh\n> > > @@ -1177,7 +1177,8 @@ check_test_results_san_file_empty_ () {\n> > > \t! find \"$TEST_RESULTS_SAN_DIR\" \\\n> > > \t\t-type f \\\n> > > \t\t-name \"$TEST_RESULTS_SAN_FILE_PFX.*\" 2>/dev/null |\n> > > -\txargs grep -q ^DEDUP_TOKEN\n> > > +\txargs grep ^DEDUP_TOKEN |\n> > > +\tgrep -qv sanitizer::GetThreadStackTopAndBottom\n> > > }\n> > \n> > It would be nice to provide some more context here in the form of a\n> > comment so that one doesn't have to blame the commit.\n> \n> We can add that on top, but I'm not sure what it should say. Do you want\n> something along the lines of \"add false positives to ignore here...\" or\n> are an explanation of why we are ignoring this particular false\n> positive?\n\nThe latter, ideally also with a reference to the upstream issue you have\ncreated. That makes it way easier to discover why this line exists and\nmay prompt people to remove the line eventually if they discover that\nthe issue has been fixed for a while.\n\nIt doesn't have to be a full paragraph, but a sentence or to go a long\nway sometimes. For more context people can still blame the commit\nmessage.\n\nPatrick\n"},{"id":"509961","messageId":"Z3uMyQ-YfQFI8qmH@pks.im","threadId":"62714","inReplyTo":"20250103202410.GC3212696@coredump.intra.peff.net","subject":"Re: [PATCH 4/6] test-lib: simplify leak-log checking","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-06T07:56:57Z","receivedAt":"2025-01-06T07:56:59Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Jan 03, 2025 at 03:24:10PM -0500, Jeff King wrote:\n> On Fri, Jan 03, 2025 at 01:05:45PM +0100, Patrick Steinhardt wrote:\n> \n> > On Wed, Jan 01, 2025 at 03:17:21PM -0500, Jeff King wrote:\n> > > @@ -1181,8 +1170,14 @@ test_atexit_handler () {\n> > > }\n> > > \n> > > check_test_results_san_file_empty_ () {\n> > > -\ttest -z \"$TEST_RESULTS_SAN_FILE\" ||\n> > > -\ttest \"$(nr_san_dir_leaks_)\" = 0\n> > > +\ttest -z \"$TEST_RESULTS_SAN_FILE\" && return 0\n> > > +\n> > > +\t# stderr piped to /dev/null because the directory may have\n> > > +\t# been \"rmdir\"'d already.\n> > > +\t! find \"$TEST_RESULTS_SAN_DIR\" \\\n> > > +\t\t-type f \\\n> > > +\t\t-name \"$TEST_RESULTS_SAN_FILE_PFX.*\" 2>/dev/null |\n> > > +\txargs grep -qv \"Unable to get registers from thread\"\n> > \n> > Can't we use `-exec grep -qv \"Unable to get registers from thread\" {}\n> > \\+` instead of using xargs? Or is that unportable? Might make it a bit\n> > easier to reason about the `!` in the presence of a pipe.\n> \n> I don't think that saves us from negating, though. The \"grep\" will tell\n> us if it matched any \"real\" lines, but we want to report that we found\n> no real lines.\n> \n> Plus I don't think \"find\" propagates the exit code from -exec anyway. I\n> think you can check the exit status with more find logic, so you'd then\n> use a conditional -print for each file like:\n\nIt should. Quoting find(1):\n\n If any invocation with the `+' form returns a non-zero value as exit\n status, then find returns a non-zero exit status.\n\n> find ... \\\n> -exec grep -qv \"Unable to get registers from thread\" \\{} \\; \\\n> -print\n> \n> and you have to check whether the output is empty. The easiest way to do\n> that is with another grep! Which also needs negated. ;)\n\nYup, I didn't mean to say that we can drop the negation, but that it\nmakes it easier to reason about what the negation applies to (the whole\npipe or just the find(1) command)).\n\n> I think if we really want to drop the negation, we'd be best to flip the\n> function's return, like:\n> \n> have_leaks() {\n> \t# not leak-checking\n> \ttest -z \"$TEST_RESULTS_SAN_FILE\" && return 1\n> \n> \tfind \"$TEST_RESULTS_SAN_DIR\" \\\n> \t\t-type f \\\n> \t\t-name \"$TEST_RESULTS_SAN_FILE_PFX.*\" 2>/dev/null |\n> \txargs grep ^DEDUP_TOKEN |\n> \tgrep -qv sanitizer::GetThreadStackTopAndBottom\n> }\n> \n> And then you could switch the initial \"grep\" to -exec if you want, but\n> there's no negation to get rid of, so it is only a preference of -exec\n> versus xargs.\n\nYup.\n\nPatrick\n"},{"id":"510045","messageId":"20250107070104.GA584401@coredump.intra.peff.net","threadId":"62714","inReplyTo":"Z3uMyQ-YfQFI8qmH@pks.im","subject":"Re: [PATCH 4/6] test-lib: simplify leak-log checking","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-07T07:01:04Z","receivedAt":"2025-01-07T07:01:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 06, 2025 at 08:56:57AM +0100, Patrick Steinhardt wrote:\n\n> > Plus I don't think \"find\" propagates the exit code from -exec anyway. I\n> > think you can check the exit status with more find logic, so you'd then\n> > use a conditional -print for each file like:\n> \n> It should. Quoting find(1):\n> \n> If any invocation with the `+' form returns a non-zero value as exit\n> status, then find returns a non-zero exit status.\n\nAh, right. I tried using ';' to look at individual files, and it does\nignore the code. But of course we don't need to know which logs had\nleaks, only that there was at least one.\n\nI think we can make it even simpler, though. I'll post patches in a\nmoment.\n\n-Peff\n"},{"id":"510046","messageId":"20250107070409.GA584456@coredump.intra.peff.net","threadId":"62714","inReplyTo":"20250101201226.GA3304465@coredump.intra.peff.net","subject":"[PATCH 0/3] lsan test-lib readability","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-07T07:04:09Z","receivedAt":"2025-01-07T07:04:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 01, 2025 at 03:12:26PM -0500, Jeff King wrote:\n\n> [1/6]: test-lib: use individual lsan dir for --stress runs\n> [2/6]: Revert \"index-pack: spawn threads atomically\"\n> [3/6]: test-lib: rely on logs to detect leaks\n> [4/6]: test-lib: simplify leak-log checking\n> [5/6]: test-lib: check leak logs for presence of DEDUP_TOKEN\n> [6/6]: test-lib: ignore leaks in the sanitizer's thread code\n\nAnd here are a few cosmetic improvements on top based on the review from\nPatrick. They should apply on top of jk/lsan-race-ignore-false-positive,\nthough that is also in 'master' now.\n\n [1/3]: test-lib: invert return value of check_test_results_san_file_empty\n [2/3]: test-lib: simplify lsan results check\n [3/3]: test-lib: add a few comments to LSan log checking\n\n t/test-lib-functions.sh | 2 +-\n t/test-lib.sh | 20 ++++++++++----------\n 2 files changed, 11 insertions(+), 11 deletions(-)\n\n-Peff\n"},{"id":"510047","messageId":"20250107070501.GA584668@coredump.intra.peff.net","threadId":"62714","inReplyTo":"20250107070409.GA584456@coredump.intra.peff.net","subject":"[PATCH 1/3] test-lib: invert return value of check_test_results_san_file_empty","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-07T07:05:01Z","receivedAt":"2025-01-07T07:05:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We have a function to check whether LSan logged any leaks. It returns\nsuccess for no leaks, and non-zero otherwise. This is the simplest thing\nfor its callers, who want to say \"if no leaks then return early\". But\nbecause it's implemented as a shell pipeline, you end up with the\nawkward:\n\n ! find ... |\n xargs grep leaks |\n grep -v false-positives\n\nwhere the \"!\" is actually negating the final grep. Switch the return\nvalue (and name) to return success when there are leaks. This should\nmake the code a little easier to read, and the negation in the callers\nstill reads pretty naturally.\n\nSigned-off-by: Jeff King \n---\n t/test-lib-functions.sh | 2 +-\n t/test-lib.sh | 8 ++++----\n 2 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 78e054ab50..c25cee0ad8 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -927,7 +927,7 @@ test_expect_success () {\n \t\ttest -n \"$test_skip_test_preamble\" ||\n \t\tsay >&3 \"expecting success of $TEST_NUMBER.$test_count '$1': $test_body\"\n \t\tif test_run_ \"$test_body\" &&\n-\t\t check_test_results_san_file_empty_\n+\t\t ! check_test_results_san_file_has_entries_\n \t\tthen\n \t\t\ttest_ok_ \"$1\"\n \t\telse\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex d1f62adbf8..be3553e40e 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1169,20 +1169,20 @@ test_atexit_handler () {\n \tteardown_malloc_check\n }\n \n-check_test_results_san_file_empty_ () {\n-\ttest -z \"$TEST_RESULTS_SAN_FILE\" && return 0\n+check_test_results_san_file_has_entries_ () {\n+\ttest -z \"$TEST_RESULTS_SAN_FILE\" && return 1\n \n \t# stderr piped to /dev/null because the directory may have\n \t# been \"rmdir\"'d already.\n-\t! find \"$TEST_RESULTS_SAN_DIR\" \\\n+\tfind \"$TEST_RESULTS_SAN_DIR\" \\\n \t\t-type f \\\n \t\t-name \"$TEST_RESULTS_SAN_FILE_PFX.*\" 2>/dev/null |\n \txargs grep ^DEDUP_TOKEN |\n \tgrep -qv sanitizer::GetThreadStackTopAndBottom\n }\n \n check_test_results_san_file_ () {\n-\tif check_test_results_san_file_empty_\n+\tif ! check_test_results_san_file_has_entries_\n \tthen\n \t\treturn\n \tfi &&\n-- \n2.48.0.rc2.377.g0507c08f68\n\n"},{"id":"510048","messageId":"20250107070752.GB584668@coredump.intra.peff.net","threadId":"62714","inReplyTo":"20250107070409.GA584456@coredump.intra.peff.net","subject":"[PATCH 2/3] test-lib: simplify lsan results check","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-07T07:07:52Z","receivedAt":"2025-01-07T07:07:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We want to know if there are any leaks logged by LSan in the results\ndirectory, so we run \"find\" on the containing directory and pipe it to\nxargs. We can accomplish the same thing by just globbing in the shell\nand passing the result to grep, which has a few advantages:\n\n - it's one fewer process to run\n\n - we can glob on the TEST_RESULTS_SAN_FILE pattern, which is what we\n checked at the beginning of the function, and is the same glob use\n to show the logs in check_test_results_san_file_\n\n - this correctly handles the case where TEST_OUTPUT_DIRECTORY has a\n space in it. For example doing:\n\n mkdir \"/tmp/foo bar\"\n TEST_OUTPUT_DIRECTORY=\"/tmp/foo bar\" make SANITIZE=leak test\n\n would yield a lot of:\n\n grep: /tmp/foo: No such file or directory\n grep: bar/test-results/t0006-date.leak/trace.test-tool.582311: No such file or directory\n\n when there are leaks. We could do the same thing with \"xargs\n --null\", but that isn't portable.\n\nWe are now subject to command-line length limits, but that is also true\nof the globbing cat used to show the logs themselves. This hasn't been a\nproblem in practice.\n\nWe do need to use \"grep -s\" for the case that the glob does not expand\n(i.e., there are not any log files at all). This option is in POSIX, and\nhas been used in t7407 for several years without anybody complaining.\nThis also also naturally handles the case where the surrounding\ndirectory has already been removed (in which case there are likewise no\nfiles!), dropping the need to comment about it.\n\nSigned-off-by: Jeff King \n---\nI was surprised by the use of \"grep -s\" in t7407, since it is totally\npointless there. But I think we can take its presence as a positive sign\nfor portability.\n\n t/test-lib.sh | 7 +------\n 1 file changed, 1 insertion(+), 6 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex be3553e40e..898c2267b8 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1172,12 +1172,7 @@ test_atexit_handler () {\n check_test_results_san_file_has_entries_ () {\n \ttest -z \"$TEST_RESULTS_SAN_FILE\" && return 1\n \n-\t# stderr piped to /dev/null because the directory may have\n-\t# been \"rmdir\"'d already.\n-\tfind \"$TEST_RESULTS_SAN_DIR\" \\\n-\t\t-type f \\\n-\t\t-name \"$TEST_RESULTS_SAN_FILE_PFX.*\" 2>/dev/null |\n-\txargs grep ^DEDUP_TOKEN |\n+\tgrep -s ^DEDUP_TOKEN \"$TEST_RESULTS_SAN_FILE\".* |\n \tgrep -qv sanitizer::GetThreadStackTopAndBottom\n }\n \n-- \n2.48.0.rc2.377.g0507c08f68\n\n"},{"id":"510049","messageId":"20250107070831.GC584668@coredump.intra.peff.net","threadId":"62714","inReplyTo":"20250107070409.GA584456@coredump.intra.peff.net","subject":"[PATCH 3/3] test-lib: add a few comments to LSan log checking","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-07T07:08:31Z","receivedAt":"2025-01-07T07:08:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit b119a687d4 (test-lib: ignore leaks in the sanitizer's thread\ncode, 2025-01-01) added code to suppress a false positive in the leak\nchecker. But if you're just reading the code, the obscure grep call is a\nbit of a head-scratcher. Let's add a brief comment explaining what's\ngoing on (and anybody digging further can find this commit or that one\nfor all the details).\n\nSigned-off-by: Jeff King \n---\n t/test-lib.sh | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 898c2267b8..9f27a49995 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1172,6 +1172,11 @@ test_atexit_handler () {\n check_test_results_san_file_has_entries_ () {\n \ttest -z \"$TEST_RESULTS_SAN_FILE\" && return 1\n \n+\t# Lines marked with DEDUP_TOKEN show unique leaks. We only care that we\n+\t# found at least one.\n+\t#\n+\t# But also suppress any false positives caused by bugs or races in the\n+\t# sanitizer itself.\n \tgrep -s ^DEDUP_TOKEN \"$TEST_RESULTS_SAN_FILE\".* |\n \tgrep -qv sanitizer::GetThreadStackTopAndBottom\n }\n-- \n2.48.0.rc2.377.g0507c08f68\n"},{"id":"510053","messageId":"Z3zZt2OCjcDGvjBS@pks.im","threadId":"62714","inReplyTo":"20250107070752.GB584668@coredump.intra.peff.net","subject":"Re: [PATCH 2/3] test-lib: simplify lsan results check","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-07T07:37:33Z","receivedAt":"2025-01-07T07:37:39Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Jan 07, 2025 at 02:07:52AM -0500, Jeff King wrote:\n> We want to know if there are any leaks logged by LSan in the results\n> directory, so we run \"find\" on the containing directory and pipe it to\n> xargs. We can accomplish the same thing by just globbing in the shell\n> and passing the result to grep, which has a few advantages:\n> \n> - it's one fewer process to run\n> \n> - we can glob on the TEST_RESULTS_SAN_FILE pattern, which is what we\n> checked at the beginning of the function, and is the same glob use\n\ns/use/used\n\nI'm always a bit thrown off by your style of bulleted lists, where they\nfeel like sentences but start with a lower-case letter, and sometimes\nthey do and sometimes they don't end with punctuation. Maybe it's just\nme not being a native speaker and it's a natural thing to do in English.\nIn any case, it's nothing that really matters in the end, but would be\nhappy to learn if this is indeed something you tend to do in English.\n\n> to show the logs in check_test_results_san_file_\n> \n> - this correctly handles the case where TEST_OUTPUT_DIRECTORY has a\n> space in it. For example doing:\n> \n> mkdir \"/tmp/foo bar\"\n> TEST_OUTPUT_DIRECTORY=\"/tmp/foo bar\" make SANITIZE=leak test\n> \n> would yield a lot of:\n> \n> grep: /tmp/foo: No such file or directory\n> grep: bar/test-results/t0006-date.leak/trace.test-tool.582311: No such file or directory\n> \n> when there are leaks. We could do the same thing with \"xargs\n> --null\", but that isn't portable.\n> \n> We are now subject to command-line length limits, but that is also true\n> of the globbing cat used to show the logs themselves. This hasn't been a\n> problem in practice.\n\nYup, this also came to my mind immediately. But I agree that it\nshouldn't be an issue in general.\n\n> We do need to use \"grep -s\" for the case that the glob does not expand\n> (i.e., there are not any log files at all). This option is in POSIX, and\n> has been used in t7407 for several years without anybody complaining.\n> This also also naturally handles the case where the surrounding\n> directory has already been removed (in which case there are likewise no\n> files!), dropping the need to comment about it.\n\nOkay. So in case there are no matching files we don't expand the\nglobbing string, and \"--no-messages\" makes us ignore that case. A bit\nfunny, but I don't see any issue with it.\n\n> Signed-off-by: Jeff King \n> ---\n> I was surprised by the use of \"grep -s\" in t7407, since it is totally\n> pointless there. But I think we can take its presence as a positive sign\n> for portability.\n\nGood to know.\n\n> t/test-lib.sh | 7 +------\n> 1 file changed, 1 insertion(+), 6 deletions(-)\n> \n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index be3553e40e..898c2267b8 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -1172,12 +1172,7 @@ test_atexit_handler () {\n> check_test_results_san_file_has_entries_ () {\n> \ttest -z \"$TEST_RESULTS_SAN_FILE\" && return 1\n> \n> -\t# stderr piped to /dev/null because the directory may have\n> -\t# been \"rmdir\"'d already.\n> -\tfind \"$TEST_RESULTS_SAN_DIR\" \\\n> -\t\t-type f \\\n> -\t\t-name \"$TEST_RESULTS_SAN_FILE_PFX.*\" 2>/dev/null |\n> -\txargs grep ^DEDUP_TOKEN |\n> +\tgrep -s ^DEDUP_TOKEN \"$TEST_RESULTS_SAN_FILE\".* |\n> \tgrep -qv sanitizer::GetThreadStackTopAndBottom\n\nAnd this nicely simplifies things indeed.\n\nPatrick\n"},{"id":"510052","messageId":"Z3zZwDFJ-FWYYIw4@pks.im","threadId":"62714","inReplyTo":"20250107070831.GC584668@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] test-lib: add a few comments to LSan log checking","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-07T07:37:36Z","receivedAt":"2025-01-07T07:37:40Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Jan 07, 2025 at 02:08:31AM -0500, Jeff King wrote:\n> Commit b119a687d4 (test-lib: ignore leaks in the sanitizer's thread\n> code, 2025-01-01) added code to suppress a false positive in the leak\n> checker. But if you're just reading the code, the obscure grep call is a\n> bit of a head-scratcher. Let's add a brief comment explaining what's\n> going on (and anybody digging further can find this commit or that one\n> for all the details).\n> \n> Signed-off-by: Jeff King \n> ---\n> t/test-lib.sh | 5 +++++\n> 1 file changed, 5 insertions(+)\n> \n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 898c2267b8..9f27a49995 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -1172,6 +1172,11 @@ test_atexit_handler () {\n> check_test_results_san_file_has_entries_ () {\n> \ttest -z \"$TEST_RESULTS_SAN_FILE\" && return 1\n> \n> +\t# Lines marked with DEDUP_TOKEN show unique leaks. We only care that we\n> +\t# found at least one.\n> +\t#\n> +\t# But also suppress any false positives caused by bugs or races in the\n> +\t# sanitizer itself.\n> \tgrep -s ^DEDUP_TOKEN \"$TEST_RESULTS_SAN_FILE\".* |\n> \tgrep -qv sanitizer::GetThreadStackTopAndBottom\n> }\n\nThanks for adding this comment!\n\nPatrick\n"},{"id":"510091","messageId":"xmqqmsg2vbi1.fsf@gitster.g","threadId":"62714","inReplyTo":"20250107070752.GB584668@coredump.intra.peff.net","subject":"Re: [PATCH 2/3] test-lib: simplify lsan results check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-07T16:23:34Z","receivedAt":"2025-01-07T16:23:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> We want to know if there are any leaks logged by LSan in the results\n> directory, so we run \"find\" on the containing directory and pipe it to\n> xargs. We can accomplish the same thing by just globbing in the shell\n> and passing the result to grep, which has a few advantages:\n>\n> - it's one fewer process to run\n> ...\n> We are now subject to command-line length limits, but that is also true\n> of the globbing cat used to show the logs themselves. This hasn't been a\n> problem in practice.\n\nNice to see it mentioned here. And the resulting code does become\nsimpler to reason about.\n\n> We do need to use \"grep -s\" for the case that the glob does not expand\n> (i.e., there are not any log files at all). This option is in POSIX, and\n> has been used in t7407 for several years without anybody complaining.\n\nAlso since c625bf0e (git-p4: git-p4 tests with p4 triggers,\n2017-07-13) t9831 has also been using it. It is not like a stray\nerror message about unmatched glob would really matter here, though.\nWe are not doing 2>&1 to let the downstream of the pipe see it, and\nunless the test is run under \"-v\" option, it wouldn't even be seen.\n\n> This also also naturally handles the case where the surrounding\n> directory has already been removed (in which case there are likewise no\n> files!), dropping the need to comment about it.\n\nNice.\n\nThanks.\n"},{"id":"510220","messageId":"20250109075750.GC2735258@coredump.intra.peff.net","threadId":"62714","inReplyTo":"Z3zZt2OCjcDGvjBS@pks.im","subject":"Re: [PATCH 2/3] test-lib: simplify lsan results check","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-09T07:57:50Z","receivedAt":"2025-01-09T07:57:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 07, 2025 at 08:37:33AM +0100, Patrick Steinhardt wrote:\n\n> On Tue, Jan 07, 2025 at 02:07:52AM -0500, Jeff King wrote:\n> > We want to know if there are any leaks logged by LSan in the results\n> > directory, so we run \"find\" on the containing directory and pipe it to\n> > xargs. We can accomplish the same thing by just globbing in the shell\n> > and passing the result to grep, which has a few advantages:\n> > \n> > - it's one fewer process to run\n> > \n> > - we can glob on the TEST_RESULTS_SAN_FILE pattern, which is what we\n> > checked at the beginning of the function, and is the same glob use\n> \n> s/use/used\n> \n> I'm always a bit thrown off by your style of bulleted lists, where they\n> feel like sentences but start with a lower-case letter, and sometimes\n> they do and sometimes they don't end with punctuation. Maybe it's just\n> me not being a native speaker and it's a natural thing to do in English.\n> In any case, it's nothing that really matters in the end, but would be\n> happy to learn if this is indeed something you tend to do in English.\n\nHeh. Yeah, I've seen you mention them before and I've been tempted to\nstart a big discussion. But I never felt like it was worth it. But\ntonight's your lucky night. ;)\n\nIn short: I think it's a style question. I perceive them as\ncontinuations of the sentence that has the \":\". Though admittedly I do\nnot always grammatically continue that sentence. So for example I could:\n\n - have one bullet item that completes the sentence.\n\n - and then another that likewise completes it.\n\n;) I think many style guides would frown on that. Especially with the\nperiods at the end (you might argue that they should be semicolons).\n\nIn the example you quoted above they don't grammatically continue the\nsentence, so arguably what I'm saying doesn't even apply. But I also\nkind of think of the list items as sentence fragments. That sometimes\nhappen to make a full sentence. Or need punctuation because that\nfragments gets so long it contains multiple sentences.\n\nI dunno. You asked if it is something you tend to do in English. It is\nsomething _I_ tend to do in English, but I think most style guides would\nsuggest against it (but then, most also suggest against bulleted lists\nin the first place). (They probably also suggest against lots of\nparentheses). So I wouldn't necessarily copy me.\n\nMy general feeling is that unless a commit message is inaccurate or hard\nto understand, we should mostly let it pass (even typos). Yes, they are\nan artifact that is enshrined in the history. But at some point they are\nalso just a written communication between developers, and we all have\nour own voices and styles. And make mistakes. Polishing them is\nsomething we _can_ do collaboratively, but there are diminishing\nreturns.\n\nIn case it is not clear, I would not say the same for documentation,\nerror messages, etc. Those are artifacts that hits a wider audience, and\nwe have a tool for polishing them together: git.\n\nAnd people should still proofread and correct their own messages before\nsending. Believe it or not, I do always take a final pass when sending\nout my commits and still manage to have errors. ;) A lot of times I end\nup improving clarity and wording on the final pass, but end up\nintroducing a typo (I'm pretty sure that the use/used above was me\nswitching last-minute between \"the same glob we use\" and \"the same glob\nused\").\n\nBringing it back to the example at hand, my assumption is that the\nbullet list capitalization and punctuation is mostly a question of\nstyle, and isn't making the result hard to understand. But if it is, I\ncan try to adjust. I actually wrote a bulleted list in a commit message\nearlier today and capitalized it just for you. :)\n\n-Peff\n"},{"id":"510221","messageId":"20250109075913.GD2735258@coredump.intra.peff.net","threadId":"62714","inReplyTo":"xmqqmsg2vbi1.fsf@gitster.g","subject":"Re: [PATCH 2/3] test-lib: simplify lsan results check","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-09T07:59:13Z","receivedAt":"2025-01-09T07:59:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 07, 2025 at 08:23:34AM -0800, Junio C Hamano wrote:\n\n> > We do need to use \"grep -s\" for the case that the glob does not expand\n> > (i.e., there are not any log files at all). This option is in POSIX, and\n> > has been used in t7407 for several years without anybody complaining.\n> \n> Also since c625bf0e (git-p4: git-p4 tests with p4 triggers,\n> 2017-07-13) t9831 has also been using it. It is not like a stray\n> error message about unmatched glob would really matter here, though.\n> We are not doing 2>&1 to let the downstream of the pipe see it, and\n> unless the test is run under \"-v\" option, it wouldn't even be seen.\n\nYeah, I saw those. But I don't think they count since hardly anybody\nruns the p4 tests. They do run in CI, but on a rather limited set of\nplatforms. Though come to think of it, this one would only kick in for\nLSan, which may also run on a pretty limited set of platforms. :)\n\n-Peff\n"},{"id":"510238","messageId":"Z3-eTDsHQ9Nu1er9@pks.im","threadId":"62714","inReplyTo":"20250109075750.GC2735258@coredump.intra.peff.net","subject":"Re: [PATCH 2/3] test-lib: simplify lsan results check","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-09T10:00:44Z","receivedAt":"2025-01-09T10:00:51Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Jan 09, 2025 at 02:57:50AM -0500, Jeff King wrote:\n> On Tue, Jan 07, 2025 at 08:37:33AM +0100, Patrick Steinhardt wrote:\n> \n> > On Tue, Jan 07, 2025 at 02:07:52AM -0500, Jeff King wrote:\n> > > We want to know if there are any leaks logged by LSan in the results\n> > > directory, so we run \"find\" on the containing directory and pipe it to\n> > > xargs. We can accomplish the same thing by just globbing in the shell\n> > > and passing the result to grep, which has a few advantages:\n> > > \n> > > - it's one fewer process to run\n> > > \n> > > - we can glob on the TEST_RESULTS_SAN_FILE pattern, which is what we\n> > > checked at the beginning of the function, and is the same glob use\n> > \n> > s/use/used\n> > \n> > I'm always a bit thrown off by your style of bulleted lists, where they\n> > feel like sentences but start with a lower-case letter, and sometimes\n> > they do and sometimes they don't end with punctuation. Maybe it's just\n> > me not being a native speaker and it's a natural thing to do in English.\n> > In any case, it's nothing that really matters in the end, but would be\n> > happy to learn if this is indeed something you tend to do in English.\n> \n> Heh. Yeah, I've seen you mention them before and I've been tempted to\n> start a big discussion. But I never felt like it was worth it. But\n> tonight's your lucky night. ;)\n> \n> In short: I think it's a style question. I perceive them as\n> continuations of the sentence that has the \":\". Though admittedly I do\n> not always grammatically continue that sentence. So for example I could:\n> \n> - have one bullet item that completes the sentence.\n> \n> - and then another that likewise completes it.\n> \n> ;) I think many style guides would frown on that. Especially with the\n> periods at the end (you might argue that they should be semicolons).\n> \n> In the example you quoted above they don't grammatically continue the\n> sentence, so arguably what I'm saying doesn't even apply. But I also\n> kind of think of the list items as sentence fragments. That sometimes\n> happen to make a full sentence. Or need punctuation because that\n> fragments gets so long it contains multiple sentences.\n> \n> I dunno. You asked if it is something you tend to do in English. It is\n> something _I_ tend to do in English, but I think most style guides would\n> suggest against it (but then, most also suggest against bulleted lists\n> in the first place). (They probably also suggest against lots of\n> parentheses). So I wouldn't necessarily copy me.\n> \n> My general feeling is that unless a commit message is inaccurate or hard\n> to understand, we should mostly let it pass (even typos). Yes, they are\n> an artifact that is enshrined in the history. But at some point they are\n> also just a written communication between developers, and we all have\n> our own voices and styles. And make mistakes. Polishing them is\n> something we _can_ do collaboratively, but there are diminishing\n> returns.\n\nYup, agreed. It's a minor detail and I'm happy to gloss over it in the\nfuture.\n\n> In case it is not clear, I would not say the same for documentation,\n> error messages, etc. Those are artifacts that hits a wider audience, and\n> we have a tool for polishing them together: git.\n> \n> And people should still proofread and correct their own messages before\n> sending. Believe it or not, I do always take a final pass when sending\n> out my commits and still manage to have errors. ;) A lot of times I end\n> up improving clarity and wording on the final pass, but end up\n> introducing a typo (I'm pretty sure that the use/used above was me\n> switching last-minute between \"the same glob we use\" and \"the same glob\n> used\").\n> \n> Bringing it back to the example at hand, my assumption is that the\n> bullet list capitalization and punctuation is mostly a question of\n> style, and isn't making the result hard to understand. But if it is, I\n> can try to adjust. I actually wrote a bulleted list in a commit message\n> earlier today and capitalized it just for you. :)\n\nThanks for explaining!\n\nPatrick\n"}]}