What's cooking in git.git (Dec 2024, #11; Mon, 30)

What's cooking in git.git (Dec 2024, #11; Mon, 30)

[Junio C Hamano]

Here are the topics that have been cooking in my tree.  Commits
prefixed with '+' are in 'next' (being in 'next' is a sign that a
topic is stable enough to be used and are candidate to be in a
future release).  Commits prefixed with '-' are only in 'seen', and
aren't considered "accepted" at all and may be annotated with an URL
to a message that raises issues but they are no means exhaustive.  A
topic without enough support may be discarded after a long period of
no activity (of course they can be resubmit when new interests
arise).

It has been a very slow week, and I expect this week will also be
slow.  Git 2.48-rc1 has been tagged.  Hopefully we can have 2.48
final around the end of next week.

Extra testing the tip of 'master' before we actually tag it is as
always very much appreciated.

Copies of the source code to Git live in many repositories, and the
following is a list of the ones I push into or their mirrors.  Some
repositories have only a subset of branches.

With maint, master, next, seen, todo:

	git://git.kernel.org/pub/scm/git/git.git/
	git://repo.or.cz/alt-git.git/
	https://kernel.googlesource.com/pub/scm/git/git/
	https://github.com/git/git/
	https://gitlab.com/git-scm/git/

With all the integration branches and topics broken out:

	https://github.com/gitster/git/

Even though the preformatted documentation in HTML and man format
are not sources, they are published in these repositories for
convenience (replace "htmldocs" with "manpages" for the manual
pages):

	git://git.kernel.org/pub/scm/git/git-htmldocs.git/
	https://github.com/gitster/git-htmldocs.git/

Release tarballs are available at:

	https://www.kernel.org/pub/software/scm/git/

--------------------------------------------------
[New Topics]

* jk/lsan-race-with-barrier (2024-12-30) 5 commits
  (merged to 'next' on 2024-12-30 at 3fc0e14928)
 + grep: work around LSan threading race with barrier
 + index-pack: work around LSan threading race with barrier
 + thread-utils: introduce optional barrier type
 + Revert "index-pack: spawn threads atomically"
 + test-lib: use individual lsan dir for --stress runs

 CI jobs that run threaded programs under LSan has been giving false
 positives from time to time, which has been worked around.

 Will merge to 'master'.
 source: <20241230042325.GA112439@coredump.intra.peff.net>


* ps/meson-weak-sha1-build (2024-12-30) 8 commits
 - meson: provide a summary of configured backends
 - meson: wire up unsafe SHA1 backend
 - meson: add missing dots for build options
 - meson: simplify conditions for HTTPS and SHA1 dependencies
 - meson: require SecurityFramework when it's used as SHA1 backend
 - meson: deduplicate access to SHA1/SHA256 backend options
 - meson: consistenlty spell 'CommonCrypto'
 - Merge branch 'ps/weak-sha1-for-tail-sum-fix' into ps/meson-weak-sha1-build
 (this branch uses ps/weak-sha1-for-tail-sum-fix.)

 meson-based build now supports the unsafe-sha1 build knob.

 Will merge to 'next'.
 source: <20241230-pks-meson-sha1-unsafe-v1-0-efb276e171f5@pks.im>


* ps/object-collision-check (2024-12-30) 1 commit
  (merged to 'next' on 2024-12-30 at e083ea3154)
 + object-file: fix race in object collision check

 CI jobs gave sporadic failures, which turns out that that the
 object finalization code was giving an error when it did not have
 to.

 Will merge to 'master'.
 source: <20241230-b4-pks-object-file-racy-collision-check-v1-1-11571294e60a@pks.im>


* ps/weak-sha1-for-tail-sum-fix (2024-12-30) 3 commits
  (merged to 'next' on 2024-12-30 at c24783e99d)
 + ci: exercise unsafe OpenSSL backend
 + builtin/fast-import: fix segfault with unsafe SHA1 backend
 + bulk-checkin: fix segfault with unsafe SHA1 backend
 (this branch is used by ps/meson-weak-sha1-build.)

 An earlier "csum-file checksum does not have to be computed with
 sha1dc" topic had a few code paths that had initialized an
 implementation of a hash function to be used by an unmatching hash
 by mistake, which have been corrected.

 Will merge to 'master'.
 source: <20241230-pks-meson-sha1-unsafe-v1-0-efb276e171f5@pks.im>

--------------------------------------------------
[Graduated to 'master']

* ms/t7611-test-path-is-file (2024-12-27) 1 commit
  (merged to 'next' on 2024-12-28 at b08a0c8e23)
 + t7611: replace test -f with test_path_is* helpers

 Test modernization.
 
 source: <20241227105345.10184-1-meetsoni3017@gmail.com>


* ps/meson-test-wo-gitweb (2024-12-27) 3 commits
  (merged to 'next' on 2024-12-28 at 39938f41fd)
 + meson: enable auto-discovered "gitweb"
 + GIT-BUILD-OPTIONS: wire up NO_GITWEB option
 + GIT-BUILD-OPTIONS: sort variables alphabetically

 meson-based build without GitWeb failed the self tests.
 source: <20241227-b4-pks-meson-wo-gitweb-v1-0-14ca8515bb3b@pks.im>

--------------------------------------------------
[Cooking]

* sk/strlen-returns-size_t (2024-12-26) 1 commit
 - date.c: Fix type missmatch warings from msvc

 Code clean-up.

 The remainder needs to be reviewed.
 source: <20241223110407.3308-3-soekkle@freenet.de>


* ps/more-sign-compare (2024-12-27) 10 commits
 - sign-compare: avoid comparing ptrdiff with an int/unsigned
 - commit-reach: use `size_t` to track indices when computing merge bases
 - shallow: fix -Wsign-compare warnings
 - builtin/log: fix remaining -Wsign-compare warnings
 - builtin/log: use `size_t` to track indices
 - commit-reach: use `size_t` to track indices in `get_reachable_subset()`
 - commit-reach: use `size_t` to track indices in `remove_redundant()`
 - commit-reach: fix type of `min_commit_date`
 - commit-reach: fix index used to loop through unsigned integer
 - prio-queue: fix type of `insertion_ctr`

 More -Wsign-compare fixes.

 Will merge to 'next'.
 cf. https://staticthinking.wordpress.com/2023/07/25/wsign-compare-is-garbage/
 source: <20241227-b4-pks-commit-reach-sign-compare-v1-0-07c59c2aa632@pks.im>


* as/long-option-help-i18n (2024-12-30) 1 commit
  (merged to 'next' on 2024-12-30 at 900c79808f)
 + parse-options: localize mark-up of placeholder text in the short help

 Tweak the help text used for the option value placeholders by
 parse-options API so that translations can customize the "<>"
 placeholder signal (e.g. "--option=<value>").

 Will cook in 'next'.
 source: <20241228114221.10351-4-ash@kambanaria.org>


* sk/maintenance-remote-prune (2024-12-28) 1 commit
 - maintenance: add prune-remote-refs task

 A new periodic maintenance task to run "git remote prune" has been
 introduced.

 Expecting a reroll.
 cf. <Z3JLdIG4C9D2-1ZT@pks.im>
 source: <pull.1838.v2.git.1735380461980.gitgitgadget@gmail.com>


* rs/reftable-realloc-errors (2024-12-28) 4 commits
  (merged to 'next' on 2024-12-30 at ebc9625a4c)
 + t-reftable-merged: handle realloc errors
 + reftable: handle realloc error in parse_names()
 + reftable: fix allocation count on realloc error
 + reftable: avoid leaks on realloc error

 The custom allocator code in the reftable library did not handle
 failing realloc() very well, which has been addressed.

 Will merge to 'master'?
 cf. <Z3JLdIG4C9D2-1ZT@pks.im>
 source: <f4677194-0a3a-4f07-b003-c0295b51c100@web.de>


* jc/show-index-h-update (2024-12-20) 1 commit
 - show-index: the short help should say the command reads from its input

 Doc and short-help text for "show-index" has been clarified to
 stress that the command reads its data from the standard input.

 Comments?
 source: <xmqqfrmidyhk.fsf@gitster.g>


* ps/the-repository (2024-12-18) 15 commits
 - match-trees: stop using `the_repository`
 - graph: stop using `the_repository`
 - add-interactive: stop using `the_repository`
 - tmp-objdir: stop using `the_repository`
 - resolve-undo: stop using `the_repository`
 - credential: stop using `the_repository`
 - mailinfo: stop using `the_repository`
 - diagnose: stop using `the_repository`
 - server-info: stop using `the_repository`
 - send-pack: stop using `the_repository`
 - serve: stop using `the_repository`
 - trace: stop using `the_repository`
 - pager: stop using `the_repository`
 - progress: stop using `the_repository`
 - Merge branch 'ps/build-sign-compare' into ps/the-repository

 More code paths have a repository passed through the callchain,
 instead of assuming the primary the_repository object.
 source: <20241217-pks-use-the-repository-conversion-v1-0-0dba48bcc239@pks.im>


* ps/build-meson-html (2024-12-27) 12 commits
 - Documentation: wire up sanity checks for Meson
 - t/Makefile: make "check-meson" work with Dash
 - meson: install static files for HTML documentation
 - meson: generate articles
 - Documentation: refactor "howto-index.sh" for out-of-tree builds
 - Documentation: refactor "api-index.sh" for out-of-tree builds
 - meson: generate user manual
 - Documentation: inline user-manual.conf
 - meson: generate HTML pages for all man page categories
 - meson: fix generation of merge tools
 - meson: properly wire up dependencies for our docs
 - meson: wire up support for AsciiDoctor

 The build procedure based on meson learned to generate HTML
 documention pages.

 Needs review. On hold.
 source: <20241227-b4-pks-meson-docs-v2-0-f61e63edbfa1@pks.im>


* jc/doc-attr-tree (2024-12-14) 1 commit
 - doc: give attr.tree a bit more visibility

 Make sure that "git --attr-source=X", GIT_ATTR_SOURCE, and
 attr.tree configuration variables appear at the same places in the
 documentation.

 On hold.
 cf. <20241216111112.GA2201417@coredump.intra.peff.net>
 source: <xmqq5xnladwi.fsf@gitster.g>


* ps/3.0-remote-deprecation (2024-12-12) 6 commits
 - remote: announce removal of "branches/" and "remotes/"
 - builtin/pack-redundant: remove subcommand with breaking changes
 - ci: repurpose "linux-gcc" job for deprecations
 - ci: merge linux-gcc-default into linux-gcc
 - Makefile: wire up build option for deprecated features
 - Merge branch 'ps/build' into ps/3.0-remote-deprecation

 Following the procedure we established to introduce breaking
 changes for Git 3.0, allow an early opt-in for removing support of
 $GIT_DIR/branches/ and $GIT_DIR/remotes/ directories to configure
 remotes.

 Needs review.
 source: <20241211-pks-remote-branches-deprecation-v1-0-1431e2369135@pks.im>


* cc/lop-remote (2024-12-07) 5 commits
 . doc: add technical design doc for large object promisors
 . promisor-remote: check advertised name or URL
 . Add 'promisor-remote' capability to protocol v2
 . strbuf: refactor strbuf_trim_trailing_ch()
 . version: refactor strbuf_sanitize()

 Expecting a reroll.
 cf. <CAP8UFD3bdEo1_bg+aX52xSGxmg9KfNrpiX+2LwUM-yDqjvfZbQ@mail.gmail.com>
 source: <20241206124248.160494-1-christian.couder@gmail.com>


* ds/backfill (2024-12-20) 6 commits
 - backfill: assume --sparse when sparse-checkout is enabled
 - backfill: add --sparse option
 - backfill: add --min-batch-size=<n> option
 - backfill: basic functionality and tests
 - backfill: add builtin boilerplate
 - Merge branch 'ds/path-walk-1' into ds/backfill
 (this branch uses ds/path-walk-1.)

 Lazy-loading missing files in a blobless clone on demand is costly
 as it tends to be one-blob-at-a-time.  "git backfill" is introduced
 to help bulk-download necessary files beforehand.

 Comments?
 source: <pull.1820.v2.git.1734712193.gitgitgadget@gmail.com>


* re/submodule-parse-opt (2024-12-11) 7 commits
  (merged to 'next' on 2024-12-21 at 9e65a56a63)
 + git-submodule.sh: rename some variables
 + git-submodule.sh: improve variables readability
 + git-submodule.sh: add some comments
 + git-submodule.sh: get rid of unused variable
 + git-submodule.sh: get rid of isnumber
 + git-submodule.sh: improve parsing of short options
 + git-submodule.sh: improve parsing of some long options

 "git submodule" learned various ways to spell the same option,
 e.g. "--branch=B" can be spelled "--branch B" or "-bB".

 Will cook in 'next'.
 source: <20241211063234.7610-1-royeldar0@gmail.com>


* tb/unsafe-hash-test (2024-11-21) 2 commits
 - t/helper/test-tool: implement sha1-unsafe helper
 - t/helper/test-sha1: prepare for an unsafe mode

 Preliminary addition to the test tool to allow a plain SHA-1 hash
 algorithm without collision protection.

 Comments?
 cf. <xmqqr073antj.fsf@gitster.g>
 source: <cover.1730833506.git.me@ttaylorr.com>


* tb/incremental-midx-part-2 (2024-11-20) 15 commits
 - midx: implement writing incremental MIDX bitmaps
 - pack-bitmap.c: use `ewah_or_iterator` for type bitmap iterators
 - pack-bitmap.c: keep track of each layer's type bitmaps
 - ewah: implement `struct ewah_or_iterator`
 - pack-bitmap.c: apply pseudo-merge commits with incremental MIDXs
 - pack-bitmap.c: compute disk-usage with incremental MIDXs
 - pack-bitmap.c: teach `rev-list --test-bitmap` about incremental MIDXs
 - pack-bitmap.c: support bitmap pack-reuse with incremental MIDXs
 - pack-bitmap.c: teach `show_objects_for_type()` about incremental MIDXs
 - pack-bitmap.c: teach `bitmap_for_commit()` about incremental MIDXs
 - pack-bitmap.c: open and store incremental bitmap layers
 - pack-revindex: prepare for incremental MIDX bitmaps
 - Documentation: describe incremental MIDX bitmaps
 - Merge branch 'tb/pseudo-merge-bitmap-fixes' into tb/incremental-midx-part-2
 - Merge branch 'tb/incremental-midx-part-1' into tb/incremental-midx-part-2

 Incrementally updating multi-pack index files.

 Needs review.
 source: <cover.1732054032.git.me@ttaylorr.com>


* ps/send-pack-unhide-error-in-atomic-push (2024-11-14) 2 commits
 - transport: don't ignore git-receive-pack(1) exit code on atomic push
 - t5504: modernize test by moving heredocs into test bodies

 "git push --atomic --porcelain" used to ignore failures from the
 other side, losing the error status from the child process, which
 has been corrected.

 Needs to see if competing parallel topic needs to replace this one.
 source: <20241113-pks-push-atomic-respect-exit-code-v1-0-7965f01e7f4e@pks.im>


* jc/move-is-bare-repository-cfg-variable-to-repo (2024-11-07) 3 commits
 . repository: BUG when is_bare_cfg is not initialized
 . setup: initialize is_bare_cfg
 . git: remove is_bare_repository_cfg global variable

 Code rewrite to turn the is_bare_repository_cfg global variable
 into a member in the the_repo singleton repository object.

 Waiting for response to reviews.
 cf. <xmqqy116xvr3.fsf@gitster.g>
 Seems to break t0021-conversion on Windows.
 cf. https://lore.kernel.org/git/xmqqzfl1hl52.fsf@gitster.g/
 source: <pull.1826.git.git.1730926082.gitgitgadget@gmail.com>


* ds/name-hash-tweaks (2024-12-20) 8 commits
 - pack-objects: add third name hash version
 - pack-objects: prevent name hash version change
 - test-tool: add helper for name-hash values
 - p5313: add size comparison test
 - pack-objects: add GIT_TEST_NAME_HASH_VERSION
 - repack: add --name-hash-version option
 - pack-objects: add --name-hash-version option
 - pack-objects: create new name-hash function version

 "git pack-objects" and its wrapper "git repack" learned an option
 to use an alternative path-hash function to improve delta-base
 selection to produce a packfile with deeper history than window
 size.

 Comments?
 source: <pull.1823.v3.git.1734715194.gitgitgadget@gmail.com>


* ds/path-walk-1 (2024-12-20) 7 commits
 - path-walk: reorder object visits
 - path-walk: mark trees and blobs as UNINTERESTING
 - path-walk: visit tags and cached objects
 - path-walk: allow consumer to specify object types
 - t6601: add helper for testing path-walk API
 - test-lib-functions: add test_cmp_sorted
 - path-walk: introduce an object walk by path
 (this branch is used by ds/backfill.)

 Introduce a new API to visit objects in batches based on a common
 path, or by type.

 Comments?
 source: <pull.1818.v4.git.1734711675.gitgitgadget@gmail.com>


* km/config-remote-by-name (2024-10-21) 1 commit
 - config: support remote name in includeIf.hasconfig condition

 Support conditionally including configuration by remote name, instead
 of just URL.

 Will discard?
 source: <20241020173216.40852-2-ken@kmatsui.me>


* y5/diff-pager (2024-10-21) 1 commit
 - diff: setup pager only before diff contents truly ready

 Delay setting up the pager in 'git diff' until after the diff contents
 itself is fully prepared.

 Will discard?
 source: <pull.1817.git.git.1729370390416.gitgitgadget@gmail.com>


* ej/cat-file-remote-object-info (2024-11-25) 6 commits
 - cat-file: add remote-object-info to batch-command
 - transport: add client support for object-info
 - serve: advertise object-info feature
 - fetch-pack: move fetch initialization
 - fetch-pack: refactor packet writing
 - cat-file: add declaration of variable i inside its for loop

 "git cat-file --batch" and friends can optionally ask a remote
 server about objects it does not have.

 Expecting a reroll.
 cf. <Z0RIrKwUnaWWm_gJ@pks.im>
 source: <20241125053616.25170-1-eric.peijian@gmail.com>


* js/libgit-rust (2024-10-16) 5 commits
 - Makefile: add option to build and test libgit-rs and libgit-rs-sys
 - libgit: add higher-level libgit crate
 - libgit-sys: also export some config_set functions
 - libgit-sys: introduce Rust wrapper for libgit.a
 - common-main: split init and exit code into new files

 A rust binding to libgit.a functions has been introduced.

 Will discard?
 source: <cover.1729032373.git.steadmon@google.com>

Re: What's cooking in git.git (Dec 2024, #11; Mon, 30)

[René Scharfe]

Am 30.12.24 um 18:33 schrieb Junio C Hamano:
> * rs/reftable-realloc-errors (2024-12-28) 4 commits
>   (merged to 'next' on 2024-12-30 at ebc9625a4c)
>  + t-reftable-merged: handle realloc errors
>  + reftable: handle realloc error in parse_names()
>  + reftable: fix allocation count on realloc error
>  + reftable: avoid leaks on realloc error
>
>  The custom allocator code in the reftable library did not handle
>  failing realloc() very well, which has been addressed.
>
>  Will merge to 'master'?

Reftable allocation error handling was introduced by bcd5a4059a
(reftable/error: introduce out-of-memory error code, 2024-10-02) after
v2.47.1, and this series improves it, so I'd say yes.  But of course
I'm biased.

>  cf. <Z3JLdIG4C9D2-1ZT@pks.im>
>  source: <f4677194-0a3a-4f07-b003-c0295b51c100@web.de>

René

a less-invasive racy-leak fix, was Re: What's cooking in git.git (Dec 2024, #11; Mon, 30)

[Jeff King]

On Mon, Dec 30, 2024 at 09:33:20AM -0800, Junio C Hamano wrote:

> * jk/lsan-race-with-barrier (2024-12-30) 5 commits
>   (merged to 'next' on 2024-12-30 at 3fc0e14928)
>  + grep: work around LSan threading race with barrier
>  + index-pack: work around LSan threading race with barrier
>  + thread-utils: introduce optional barrier type
>  + Revert "index-pack: spawn threads atomically"
>  + test-lib: use individual lsan dir for --stress runs
> 
>  CI jobs that run threaded programs under LSan has been giving false
>  positives from time to time, which has been worked around.
> 
>  Will merge to 'master'.
>  source: <20241230042325.GA112439@coredump.intra.peff.net>

This graduated faster than I expected. :)

I think the first two patches are obviously good, and we should take
them as-is. But while poking at the problem a bit more, I found
something quite interesting that might let us fix it without any code
change at all.

I wanted a minimal reproduction of the problem so that I could report it
to the sanitizer folks upstream. So I thought this would work:

-- >8 --

cat >foo.c <<\EOF
#include <stdlib.h>
#include <pthread.h>

static void *run(void *data)
{
	return NULL;
}

int main(void)
{
	pthread_t threads[256];

	for (int i = 0; i < sizeof(threads)/sizeof(*threads); i++)
		pthread_create(&threads[i], NULL, run, NULL);
	exit(1);
}
EOF

gcc -fsanitize=leak foo.c
./a.out

-- 8< --

But it doesn't! Even if I run it thousands of times. And yet if I put
similar code inside of git.c and run it within our test suite, I see the
failure almost immediately.  So there's something about our test suite
that triggers it.  And indeed, the secret sauce turns out to be this
option (depending on system load you might need to run it a couple times
to trigger the failure):

  $ LSAN_OPTIONS=fast_unwind_on_malloc=0 ./a.out
  =================================================================
  ==1836417==ERROR: LeakSanitizer: detected memory leaks
  
  Direct leak of 32 byte(s) in 1 object(s) allocated from:
      #0 0x7ff8bb014556 in realloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:98
      #1 0x7ff8bae9d2c1 in __pthread_getattr_np nptl/pthread_getattr_np.c:180
      #2 0x7ff8bb02500d in __sanitizer::GetThreadStackTopAndBottom(bool, unsigned long*, unsigned long*) ../../../../src/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cpp:150
      #3 0x7ff8bb025187 in __sanitizer::GetThreadStackAndTls(bool, unsigned long*, unsigned long*, unsigned long*, unsigned long*) ../../../../src/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cpp:614
      #4 0x7ff8bb017d18 in __lsan::ThreadStart(unsigned int, unsigned long long, __sanitizer::ThreadType) ../../../../src/libsanitizer/lsan/lsan_posix.cpp:53
      #5 0x7ff8bb0143a9 in ThreadStartFunc<false> ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:431
      #6 0x7ff8bae9bf51 in start_thread nptl/pthread_create.c:447
      #7 0x7ff8baf1a677 in __clone3 ../sysdeps/unix/sysv/linux/x86_64/clone3.S:78
  
  SUMMARY: LeakSanitizer: 32 byte(s) leaked in 1 allocation(s).


It seems a little funny to me that the different stack unwinder would
also have different thread setup code, but I'm not at all familiar with
lsan's internals, so who knows. I might dig into that but haven't yet
(and I haven't yet reported it upstream). But anyway, that implies that
we can get rid of the race with just:

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 96f2dfb69d..2936e810b5 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -80,7 +80,6 @@ prepend_var ASAN_OPTIONS : detect_leaks=0
 export ASAN_OPTIONS
 
 prepend_var LSAN_OPTIONS : $GIT_SAN_OPTIONS
-prepend_var LSAN_OPTIONS : fast_unwind_on_malloc=0
 export LSAN_OPTIONS
 
 prepend_var UBSAN_OPTIONS : $GIT_SAN_OPTIONS

And indeed, that lets t5309 run successfully under --stress for me (with
the log-dir fix from the base of my series).

That line comes from in 71f26798f2 (test-lib: add "fast_unwind_on_malloc=0"
to LSAN_OPTIONS, 2022-02-27), with the goal of making the backtraces a
bit more readable. I'm not sure how important it is. I tried reverting a
random commit from one of Patrick's recent leak-fix series:

  git revert 58e7568c619cce872b17987f0d14b3de3703129a
  make SANITIZE=leak
  cd t
  ./t1091-sparse-checkout-builtin.sh -i

and the trace looks quite reasonable to me. That old commit 71f2678f2
gives an example from t0006. That script was marked leak-free by
974c919d36 (date API: add and use a date_mode_release(), 2022-02-16). If
we revert its change to test-date like so:

  git diff-tree -Rp 974c919d36 -- t/helper/test-date.c | git apply -3
  make SANITIZE=leak
  cd t
  ./t0006-date.sh -i

then we do get the same crappy trace as mentioned in 71f26798f2 (if I
understand correctly, this is because libc functions which allocate are
not built with -fno-omit-frame-pointer, so we can't walk past them).

So that line is doing something useful. But it may not be worth the racy
pain it's causing. So some alternatives are:

  - we drop that line by default, and then when people are investigating
    a specific leak, they can override LSAN_OPTIONS themselves to get
    better output (though of course knowing that you can even do is
    tricky)

  - we keep that line by default, but override LSAN_OPTIONS for CI to
    avoid the race. That makes all local leak-checking traces
    informative by default. But CI ones may be truncated. I'm not sure
    if people use the CI ones directly, or investigate further
    themselves.

  - we could annotate individual scripts or even tests to disable the
    option (since it's really just threaded programs). This is more
    hassle, but would limit the blast radius.

I don't love any of those, but they may be less bad than all of the
barrier trickery. And it may be that this is even something we could get
fixed in LSan upstream, and it would just be a temporary workaround. I'm
still going to pursue that.

And finally, one other option (that I'm not sure why I didn't consider
before): can we just ignore the false positives, similar to what we did
in 370ef7e40d (test-lib: ignore uninteresting LSan output, 2023-08-28).
I think we'd have to stop doing abort_on_error for the leak checker and
just rely on the logs, but that's OK (we always check the logs these
days). And detecting the false positive is a little involved. But this
seems to work:

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 96f2dfb69d..ab7d2d321e 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -80,6 +80,7 @@ prepend_var ASAN_OPTIONS : detect_leaks=0
 export ASAN_OPTIONS
 
 prepend_var LSAN_OPTIONS : $GIT_SAN_OPTIONS
+prepend_var LSAN_OPTIONS : exitcode=0
 prepend_var LSAN_OPTIONS : fast_unwind_on_malloc=0
 export LSAN_OPTIONS
 
@@ -346,7 +347,8 @@ nr_san_dir_leaks_ () {
 	find "$TEST_RESULTS_SAN_DIR" \
 		-type f \
 		-name "$TEST_RESULTS_SAN_FILE_PFX.*" 2>/dev/null |
-	xargs grep -lv "Unable to get registers from thread" |
+	xargs grep ^DEDUP_TOKEN |
+	grep -v sanitizer::GetThreadStackTopAndBottom |
 	wc -l
 }
 

A little hacky, but it lets us have our cake and eat it, too. No changes
to the code, and no bad stack traces.

What do you think?

-Peff

[PATCH 0/6] a less-invasive racy-leak fix

[Jeff King]

On Wed, Jan 01, 2025 at 02:14:22PM -0500, Jeff King wrote:

> And finally, one other option (that I'm not sure why I didn't consider
> before): can we just ignore the false positives, similar to what we did
> in 370ef7e40d (test-lib: ignore uninteresting LSan output, 2023-08-28).
> I think we'd have to stop doing abort_on_error for the leak checker and
> just rely on the logs, but that's OK (we always check the logs these
> days). And detecting the false positive is a little involved. But this
> seems to work:

So here's a clean series that does that. I'm kicking myself a little for
not going this route immediately, and spending so much time trying to
deal with the actual race in code that is not even ours. :-/

It would replace what's queued in jk/lsan-race-with-barrier, though the
first two patches remain the same.

Since the grep exit codes and inversions are a little tricky, I checked
at each step that we continue to correctly report the leaks found by
doing:

diff --git a/t/helper/test-date.c b/t/helper/test-date.c
index f25512de9a..7bedb51eaf 100644
--- a/t/helper/test-date.c
+++ b/t/helper/test-date.c
@@ -54,8 +54,6 @@ static void show_dates(const char **argv, const char *format)
 
 		printf("%s -> %s\n", *argv, show_date(t, tz, mode));
 	}
-
-	date_mode_release(&mode);
 }
 
 static void parse_dates(const char **argv)

and running "./t0006-date.sh -i" (which should bail on test 23 and
report the full stack trace).

  [1/6]: test-lib: use individual lsan dir for --stress runs
  [2/6]: Revert "index-pack: spawn threads atomically"
  [3/6]: test-lib: rely on logs to detect leaks
  [4/6]: test-lib: simplify leak-log checking
  [5/6]: test-lib: check leak logs for presence of DEDUP_TOKEN
  [6/6]: test-lib: ignore leaks in the sanitizer's thread code

 builtin/index-pack.c |  2 --
 t/test-lib.sh        | 25 +++++++++++--------------
 2 files changed, 11 insertions(+), 16 deletions(-)

-Peff

[PATCH 1/6] test-lib: use individual lsan dir for --stress runs

[Jeff King]

When storing output in test-results/, we usually give each numbered run
in a --stress set its own output file. But we don't do that for storing
LSan logs, so something like:

  ./t0003-attributes.sh --stress

will have many scripts simultaneously creating, writing to, and deleting
the test-results/t0003-attributes.leak directory. This can cause logs
from one run to be attributed to another, spurious failures when
creation and deletion race, and so on.

This has always been broken, but nobody noticed because it's rare to do
a --stress run with LSan (since the point is for the code to run quickly
many times in order to hit races). But if you're trying to find a race
in the leak sanitizing code, it makes sense to use these together.

We can fix it by using $TEST_RESULTS_BASE, which already incorporates
the stress job suffix.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/test-lib.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 1a67adb207..96f2dfb69d 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -331,7 +331,7 @@ TEST_RESULTS_BASE="$TEST_RESULTS_DIR/$TEST_NAME$TEST_STRESS_JOB_SFX"
 TEST_RESULTS_SAN_FILE_PFX=trace
 TEST_RESULTS_SAN_DIR_SFX=leak
 TEST_RESULTS_SAN_FILE=
-TEST_RESULTS_SAN_DIR="$TEST_RESULTS_DIR/$TEST_NAME.$TEST_RESULTS_SAN_DIR_SFX"
+TEST_RESULTS_SAN_DIR="$TEST_RESULTS_BASE.$TEST_RESULTS_SAN_DIR_SFX"
 TRASH_DIRECTORY="trash directory.$TEST_NAME$TEST_STRESS_JOB_SFX"
 test -n "$root" && TRASH_DIRECTORY="$root/$TRASH_DIRECTORY"
 case "$TRASH_DIRECTORY" in
-- 
2.48.0.rc1.363.g2bf91ec010

[PATCH 2/6] Revert "index-pack: spawn threads atomically"

[Jeff King]

This reverts commit 993d38a0669a8056d496797516e743e26b6b8b54.

That commit was trying to solve a race between LSan setting up the
threads stack and another thread calling exit(), by making sure that all
pthread_create() calls have finished before doing any work that might
trigger the exit().

But that isn't sufficient. The setup code actually runs in the
individual threads themselves, not in the spawning thread's call to
pthread_create(). So while it may have improved the race a bit, you can
still trigger it pretty quickly with:

  make SANITIZE=leak
  cd t
  ./t5309-pack-delta-cycles.sh --stress

Let's back out that failed attempt so we can try again.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/index-pack.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index d773809c4c..0b62b2589f 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1336,15 +1336,13 @@ static void resolve_deltas(struct pack_idx_option *opts)
 	base_cache_limit = opts->delta_base_cache_limit * nr_threads;
 	if (nr_threads > 1 || getenv("GIT_FORCE_THREADS")) {
 		init_thread();
-		work_lock();
 		for (i = 0; i < nr_threads; i++) {
 			int ret = pthread_create(&thread_data[i].thread, NULL,
 						 threaded_second_pass, thread_data + i);
 			if (ret)
 				die(_("unable to create thread: %s"),
 				    strerror(ret));
 		}
-		work_unlock();
 		for (i = 0; i < nr_threads; i++)
 			pthread_join(thread_data[i].thread, NULL);
 		cleanup_thread();
-- 
2.48.0.rc1.363.g2bf91ec010

[PATCH 3/6] test-lib: rely on logs to detect leaks

[Jeff King]

When we run with sanitizers, we set abort_on_error=1 so that the tests
themselves can detect problems directly (when the buggy program exits
with SIGABRT). This has one blind spot, though: we don't always check
the exit codes for all programs (e.g., helpers like upload-pack invoked
behind the scenes).

For ASan and UBSan this is mostly fine; they exit as soon as they see an
error, so the unexpected abort of the program causes the test to fail
anyway.

But for LSan, the program runs to completion, since we can only check
for leaks at the end. And in that case we could miss leak reports. And
thus we started checking LSan logs in faececa53f (test-lib: have the
"check" mode for SANITIZE=leak consider leak logs, 2022-07-28).
Originally the logs were optional, but logs are generated (and checked)
always as of 8c1d6691bc (test-lib: GIT_TEST_SANITIZE_LEAK_LOG enabled by
default, 2024-07-11). And we even check them for each test snippet, as
of cf1464331b (test-lib: check for leak logs after every test,
2024-09-24).

So now aborting on error is superfluous for LSan! We can get everything
we need by checking the logs. And checking the logs is actually
preferable, since it gives us more control over silencing false
positives (something we do not yet do, but will soon).

So let's tell LSan to just exit normally, even if it finds leaks. We can
do so with exitcode=0, which also suppresses the abort_on_error flag.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/test-lib.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 96f2dfb69d..dd2ba6e6cc 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -80,6 +80,7 @@ prepend_var ASAN_OPTIONS : detect_leaks=0
 export ASAN_OPTIONS
 
 prepend_var LSAN_OPTIONS : $GIT_SAN_OPTIONS
+prepend_var LSAN_OPTIONS : exitcode=0
 prepend_var LSAN_OPTIONS : fast_unwind_on_malloc=0
 export LSAN_OPTIONS
 
-- 
2.48.0.rc1.363.g2bf91ec010

[PATCH 4/6] test-lib: simplify leak-log checking

[Jeff King]

We have a function to count the number of leaks found (actually, it is
the number of processes which produced a log file). Once upon a time we
cared about seeing if this number increased between runs. But we
simplified that away in 95c679ad86 (test-lib: stop showing old leak
logs, 2024-09-24), and now we only care if it returns any results or
not.

In preparation for refactoring it further, let's drop the counting
function entirely, and roll it into the "is it empty" check. The outcome
should be the same, but we'll be free to return a boolean "did we find
anything" without worrying about somebody adding a new call to the
counting function.

Signed-off-by: Jeff King <peff@peff.net>
---
We need the extra "!" to invert the exit code of the final grep, which
made my head spin a little at first. Maybe it would be less confusing if
this was reporting non-empty results, as "check_test_results_has_leaks"
or something? We'd have to update the callers, but there are only 2 of
them. I dunno.

 t/test-lib.sh | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index dd2ba6e6cc..23eb26bfbe 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -340,17 +340,6 @@ case "$TRASH_DIRECTORY" in
  *) TRASH_DIRECTORY="$TEST_OUTPUT_DIRECTORY/$TRASH_DIRECTORY" ;;
 esac
 
-# Utility functions using $TEST_RESULTS_* variables
-nr_san_dir_leaks_ () {
-	# stderr piped to /dev/null because the directory may have
-	# been "rmdir"'d already.
-	find "$TEST_RESULTS_SAN_DIR" \
-		-type f \
-		-name "$TEST_RESULTS_SAN_FILE_PFX.*" 2>/dev/null |
-	xargs grep -lv "Unable to get registers from thread" |
-	wc -l
-}
-
 # If --stress was passed, run this test repeatedly in several parallel loops.
 if test "$GIT_TEST_STRESS_STARTED" = "done"
 then
@@ -1181,8 +1170,14 @@ test_atexit_handler () {
 }
 
 check_test_results_san_file_empty_ () {
-	test -z "$TEST_RESULTS_SAN_FILE" ||
-	test "$(nr_san_dir_leaks_)" = 0
+	test -z "$TEST_RESULTS_SAN_FILE" && return 0
+
+	# stderr piped to /dev/null because the directory may have
+	# been "rmdir"'d already.
+	! find "$TEST_RESULTS_SAN_DIR" \
+		-type f \
+		-name "$TEST_RESULTS_SAN_FILE_PFX.*" 2>/dev/null |
+	xargs grep -qv "Unable to get registers from thread"
 }
 
 check_test_results_san_file_ () {
-- 
2.48.0.rc1.363.g2bf91ec010

[PATCH 5/6] test-lib: check leak logs for presence of DEDUP_TOKEN

[Jeff King]

When we check the leak logs, our original strategy was to check for any
non-empty log file produced by LSan. We later amended that to ignore
noisy lines in 370ef7e40d (test-lib: ignore uninteresting LSan output,
2023-08-28).

This makes it hard to ignore noise which is more than a single line;
we'd have to actually parse the file to determine the meaning of each
line.

But there's an easy line-oriented solution. Because we always pass the
dedup_token_length option, the output will contain a DEDUP_TOKEN line
for each leak that has been found. So if we invert our strategy to stop
ignoring useless lines and only look for useful ones, we can just count
the number of DEDUP_TOKEN lines. If it's non-zero, then we found at
least one leak (it would even give us a count of unique leaks, but we
really only care if it is non-zero).

This should yield the same outcome, but will help us build more false
positive detection on top.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/test-lib.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 23eb26bfbe..c9487d0805 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1177,7 +1177,7 @@ check_test_results_san_file_empty_ () {
 	! find "$TEST_RESULTS_SAN_DIR" \
 		-type f \
 		-name "$TEST_RESULTS_SAN_FILE_PFX.*" 2>/dev/null |
-	xargs grep -qv "Unable to get registers from thread"
+	xargs grep -q ^DEDUP_TOKEN
 }
 
 check_test_results_san_file_ () {
-- 
2.48.0.rc1.363.g2bf91ec010

[PATCH 6/6] test-lib: ignore leaks in the sanitizer's thread code

[Jeff King]

Our CI jobs sometimes see false positive leaks like this:

        =================================================================
        ==3904583==ERROR: LeakSanitizer: detected memory leaks

        Direct leak of 32 byte(s) in 1 object(s) allocated from:
            #0 0x7fa790d01986 in __interceptor_realloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:98
            #1 0x7fa790add769 in __pthread_getattr_np nptl/pthread_getattr_np.c:180
            #2 0x7fa790d117c5 in __sanitizer::GetThreadStackTopAndBottom(bool, unsigned long*, unsigned long*) ../../../../src/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cpp:150
            #3 0x7fa790d11957 in __sanitizer::GetThreadStackAndTls(bool, unsigned long*, unsigned long*, unsigned long*, unsigned long*) ../../../../src/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cpp:598
            #4 0x7fa790d03fe8 in __lsan::ThreadStart(unsigned int, unsigned long long, __sanitizer::ThreadType) ../../../../src/libsanitizer/lsan/lsan_posix.cpp:51
            #5 0x7fa790d013fd in __lsan_thread_start_func ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:440
            #6 0x7fa790adc3eb in start_thread nptl/pthread_create.c:444
            #7 0x7fa790b5ca5b in clone3 ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81

This is not a leak in our code, but appears to be a race between one
thread calling exit() while another one is in LSan's stack setup code.
You can reproduce it easily by running t0003 or t5309 with --stress
(these trigger it because of the threading in git-grep and index-pack
respectively).

This may be a bug in LSan, but regardless of whether it is eventually
fixed, it is useful to work around it so that we stop seeing these false
positives.

We can recognize it by the mention of the sanitizer functions in the
DEDUP_TOKEN line. With this patch, the scripts mentioned above should
run with --stress indefinitely.

Signed-off-by: Jeff King <peff@peff.net>
---
One small downside here is that this just suppresses the "were there any
leaks" check. If there's a real leak _and_ the race was triggered, then
you'd see the racy false positive in the output. I don't think that's a
big deal, since both the race and real leaks should be rare-ish, and
you'd have to encounter both in the same run of a given test script.

 t/test-lib.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index c9487d0805..d1f62adbf8 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1177,7 +1177,8 @@ check_test_results_san_file_empty_ () {
 	! find "$TEST_RESULTS_SAN_DIR" \
 		-type f \
 		-name "$TEST_RESULTS_SAN_FILE_PFX.*" 2>/dev/null |
-	xargs grep -q ^DEDUP_TOKEN
+	xargs grep ^DEDUP_TOKEN |
+	grep -qv sanitizer::GetThreadStackTopAndBottom
 }
 
 check_test_results_san_file_ () {
-- 
2.48.0.rc1.363.g2bf91ec010

Re: a less-invasive racy-leak fix, was Re: What's cooking in git.git (Dec 2024, #11; Mon, 30)

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Dec 30, 2024 at 09:33:20AM -0800, Junio C Hamano wrote:
>
>> * jk/lsan-race-with-barrier (2024-12-30) 5 commits
> ...
> This graduated faster than I expected. :)

Heh, it is before -rc2 and the change is only about tests, so ...

> ...
> So that line is doing something useful. But it may not be worth the racy
> pain it's causing. So some alternatives are:
>
>   - we drop that line by default, and then when people are investigating
>     a specific leak, they can override LSAN_OPTIONS themselves to get
>     better output (though of course knowing that you can even do is
>     tricky)
>
>   - we keep that line by default, but override LSAN_OPTIONS for CI to
>     avoid the race. That makes all local leak-checking traces
>     informative by default. But CI ones may be truncated. I'm not sure
>     if people use the CI ones directly, or investigate further
>     themselves.
>
>   - we could annotate individual scripts or even tests to disable the
>     option (since it's really just threaded programs). This is more
>     hassle, but would limit the blast radius.
>
> I don't love any of those, but they may be less bad than all of the
> barrier trickery. And it may be that this is even something we could get
> fixed in LSan upstream, and it would just be a temporary workaround. I'm
> still going to pursue that.
>
> And finally, one other option (that I'm not sure why I didn't consider
> before): can we just ignore the false positives, similar to what we did
> in 370ef7e40d (test-lib: ignore uninteresting LSan output, 2023-08-28).

Good point.

> I think we'd have to stop doing abort_on_error for the leak checker and
> just rely on the logs, but that's OK (we always check the logs these
> days).
> ...
> A little hacky, but it lets us have our cake and eat it, too. No changes
> to the code, and no bad stack traces.
>
> What do you think?

I like the small hack.  "This is ultimately LSan's racy-ness and not
ours, so let's avoid changing our code to work it around when we can
do the workaround somewhere else" is an attitude that I would endorse
fully.

Thanks.

Re: a less-invasive racy-leak fix, was Re: What's cooking in git.git (Dec 2024, #11; Mon, 30)

[Jeff King]

On Wed, Jan 01, 2025 at 04:25:02PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > On Mon, Dec 30, 2024 at 09:33:20AM -0800, Junio C Hamano wrote:
> >
> >> * jk/lsan-race-with-barrier (2024-12-30) 5 commits
> > ...
> > This graduated faster than I expected. :)
> 
> Heh, it is before -rc2 and the change is only about tests, so ...

Yeah, I figured as much. I also considered it of relatively low
importance during -rc, but I guess CI false positives do tend to annoy
everybody and waste their time. :)

It looks like you pushed out the version of 'master' with it merged. I
had figured you'd revert jk/lsan-race-with-barrier out of next, so I
wondered how we would proceed (revert the whole merge from master to
rebuild, or do a moral revert of the final three).

Looking at jk/lsan-race-ignore-false-positive, it looks like you did the
moral revert via fc89d14c63 (Revert barrier-based LSan threading race
workaround, 2025-01-01). That commit's tree matches what I'd expect (I
guess you probably used "revert -n HEAD~3..HEAD" just like I did).

It would be nice if the 3-commit revert mentioned the specific commits
it was reverting. If you revert the whole merge, you get the merge
commit's id and you can find the original commits by inspecting the
history (but of course here we were reverting only part of it, so we
couldn't do that here). If you revert the sequence without "-n" you get
three individual reverts. Which is informative, but a little clunky in
the history.

I wonder if revert should have a "squash" mode that reverts all of the
commits (perhaps in reverse order of application in case they depend on
each other textually), and then gives you a commit message template
similar to git-fmt-merge-msg, where we list all of the commits, one per
line (though probably with their commit ids in this case).

Probably not a big deal either way, and certainly not a blocker for the
series. Thanks (as usual) for doing all the maintainer juggling.

-Peff

Re: a less-invasive racy-leak fix, was Re: What's cooking in git.git (Dec 2024, #11; Mon, 30)

[Chris Torek]

On Wed, Jan 1, 2025 at 6:32 PM Jeff King <peff@peff.net> wrote:
> I wonder if revert should have a "squash" mode that reverts all of the
> commits (perhaps in reverse order of application in case they depend on
> each other textually), and then gives you a commit message template
> similar to git-fmt-merge-msg, where we list all of the commits, one per
> line (though probably with their commit ids in this case).

I have kind of wanted this in the past, but never enough to build it, I just
did a few separate reverts.

Chris

Re: a less-invasive racy-leak fix, was Re: What's cooking in git.git (Dec 2024, #11; Mon, 30)

[Jeff King]

On Wed, Jan 01, 2025 at 04:25:02PM -0800, Junio C Hamano wrote:

> > What do you think?
> 
> I like the small hack.  "This is ultimately LSan's racy-ness and not
> ours, so let's avoid changing our code to work it around when we can
> do the workaround somewhere else" is an attitude that I would endorse
> fully.

BTW, in case anybody wants to follow along with what happens upstream, I
reported it here:

  https://github.com/google/sanitizers/issues/1836

-Peff

Re: a less-invasive racy-leak fix, was Re: What's cooking in git.git (Dec 2024, #11; Mon, 30)

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Jan 01, 2025 at 04:25:02PM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > On Mon, Dec 30, 2024 at 09:33:20AM -0800, Junio C Hamano wrote:
>> >
>> >> * jk/lsan-race-with-barrier (2024-12-30) 5 commits
>> > ...
>> > This graduated faster than I expected. :)
>> 
>> Heh, it is before -rc2 and the change is only about tests, so ...
>
> Yeah, I figured as much. I also considered it of relatively low
> importance during -rc, but I guess CI false positives do tend to annoy
> everybody and waste their time. :)
>
> It looks like you pushed out the version of 'master' with it merged. I
> had figured you'd revert jk/lsan-race-with-barrier out of next, so I
> wondered how we would proceed (revert the whole merge from master to
> rebuild, or do a moral revert of the final three).

Revert the effect of the tip-part (except for the bottom two) and
then queue the new ones, which would allow me to merge the whole
thing in one go without losing the bottom two's effect (which would
happen if we reverted the whole thing first, and then reused the
bottom two commits to build the new iteration on top).

> Looking at jk/lsan-race-ignore-false-positive, it looks like you did the
> moral revert via fc89d14c63 (Revert barrier-based LSan threading race
> workaround, 2025-01-01). That commit's tree matches what I'd expect (I
> guess you probably used "revert -n HEAD~3..HEAD" just like I did).

I actually did "read-tree -u -m" followed by "commit" ;-) 

> It would be nice if the 3-commit revert mentioned the specific commits
> it was reverting.

True.  I should probably amend while I can.

> I wonder if revert should have a "squash" mode that reverts all of the
> commits (perhaps in reverse order of application in case they depend on
> each other textually), and then gives you a commit message template
> similar to git-fmt-merge-msg, where we list all of the commits, one per
> line (though probably with their commit ids in this case).

I am not sure if I follow.  Should "revert HEAD~3..HEAD" give such
concatenation of messages, something similar to what "rebase -i"
gives us when seeing multiple "squash"es in a row?

Re: a less-invasive racy-leak fix, was Re: What's cooking in git.git (Dec 2024, #11; Mon, 30)

[Jeff King]

On Thu, Jan 02, 2025 at 06:42:30AM -0800, Junio C Hamano wrote:

> > I wonder if revert should have a "squash" mode that reverts all of the
> > commits (perhaps in reverse order of application in case they depend on
> > each other textually), and then gives you a commit message template
> > similar to git-fmt-merge-msg, where we list all of the commits, one per
> > line (though probably with their commit ids in this case).
> 
> I am not sure if I follow.  Should "revert HEAD~3..HEAD" give such
> concatenation of messages, something similar to what "rebase -i"
> gives us when seeing multiple "squash"es in a row?

I don't think we need to concatenate all of the individual revert
messages. I was thinking of producing something more like:

  <SUBJECT: DESCRIBE YOUR REVERT HERE>

  Revert the following commits:

     - 7a8d9efc26 (grep: work around LSan threading race with barrier, 2024-12-29)
     - 526c0a851b (index-pack: work around LSan threading race with barrier, 2024-12-29)
     - 7d0037b59a (thread-utils: introduce optional barrier type, 2024-12-29)

You could perhaps even auto-populate the subject with:

  Revert jk/lsan-race-with-barrier~3..jk/lsan-race-with-barrier

similar to how git-merge uses "Merge branch ...". But it's a little
clunky to read, and unlike merge, it's a lot easier to use names that
are not very meaningful (e.g., I checked out a new branch based on that
one and then used HEAD~3..HEAD, which is worthless to mention).

-Peff

Re: a less-invasive racy-leak fix, was Re: What's cooking in git.git (Dec 2024, #11; Mon, 30)

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>   <SUBJECT: DESCRIBE YOUR REVERT HERE>
>
>   Revert the following commits:
>
>      - 7a8d9efc26 (grep: work around LSan threading race with barrier, 2024-12-29)
>      - 526c0a851b (index-pack: work around LSan threading race with barrier, 2024-12-29)
>      - 7d0037b59a (thread-utils: introduce optional barrier type, 2024-12-29)

Ah, I love it.

> You could perhaps even auto-populate the subject with:
>
>   Revert jk/lsan-race-with-barrier~3..jk/lsan-race-with-barrier
>
> similar to how git-merge uses "Merge branch ...". But it's a little
> clunky to read, and unlike merge, it's a lot easier to use names that
> are not very meaningful (e.g., I checked out a new branch based on that
> one and then used HEAD~3..HEAD, which is worthless to mention).

Yup, agreed.  The commit title is much less interesting to automate
than the "we revert these three" list.

Thanks.

Re: What's cooking in git.git (Dec 2024, #11; Mon, 30)

[Patrick Steinhardt]

On Tue, Dec 31, 2024 at 06:27:16PM +0100, René Scharfe wrote:
> Am 30.12.24 um 18:33 schrieb Junio C Hamano:
> > * rs/reftable-realloc-errors (2024-12-28) 4 commits
> >   (merged to 'next' on 2024-12-30 at ebc9625a4c)
> >  + t-reftable-merged: handle realloc errors
> >  + reftable: handle realloc error in parse_names()
> >  + reftable: fix allocation count on realloc error
> >  + reftable: avoid leaks on realloc error
> >
> >  The custom allocator code in the reftable library did not handle
> >  failing realloc() very well, which has been addressed.
> >
> >  Will merge to 'master'?
> 
> Reftable allocation error handling was introduced by bcd5a4059a
> (reftable/error: introduce out-of-memory error code, 2024-10-02) after
> v2.47.1, and this series improves it, so I'd say yes.  But of course
> I'm biased.

I'm aligned with you there. Thanks!

Patrick

Re: [PATCH 3/6] test-lib: rely on logs to detect leaks

[Patrick Steinhardt]

On Wed, Jan 01, 2025 at 03:14:44PM -0500, Jeff King wrote:
> When we run with sanitizers, we set abort_on_error=1 so that the tests
> themselves can detect problems directly (when the buggy program exits
> with SIGABRT). This has one blind spot, though: we don't always check
> the exit codes for all programs (e.g., helpers like upload-pack invoked
> behind the scenes).
> 
> For ASan and UBSan this is mostly fine; they exit as soon as they see an
> error, so the unexpected abort of the program causes the test to fail
> anyway.
> 
> But for LSan, the program runs to completion, since we can only check
> for leaks at the end. And in that case we could miss leak reports. And
> thus we started checking LSan logs in faececa53f (test-lib: have the
> "check" mode for SANITIZE=leak consider leak logs, 2022-07-28).
> Originally the logs were optional, but logs are generated (and checked)
> always as of 8c1d6691bc (test-lib: GIT_TEST_SANITIZE_LEAK_LOG enabled by
> default, 2024-07-11). And we even check them for each test snippet, as
> of cf1464331b (test-lib: check for leak logs after every test,
> 2024-09-24).
> 
> So now aborting on error is superfluous for LSan! We can get everything
> we need by checking the logs. And checking the logs is actually
> preferable, since it gives us more control over silencing false
> positives (something we do not yet do, but will soon).
> 
> So let's tell LSan to just exit normally, even if it finds leaks. We can
> do so with exitcode=0, which also suppresses the abort_on_error flag.

The only downside I can think of is that we now run the whole testcase
to completion before checking for leaks, whereas beforehand we most
likely aborted the testcase on hitting the first leak. It follows that
we may now have multiple leak reports, and it is not immediately clear
which of the commands has actually been failing.

I think we're now in a clean-enough state regarding memory leaks that
this isn't a huge issue anymore though.

Patrick

Re: [PATCH 4/6] test-lib: simplify leak-log checking

[Patrick Steinhardt]

On Wed, Jan 01, 2025 at 03:17:21PM -0500, Jeff King wrote:
> @@ -1181,8 +1170,14 @@ test_atexit_handler () {
>  }
>  
>  check_test_results_san_file_empty_ () {
> -	test -z "$TEST_RESULTS_SAN_FILE" ||
> -	test "$(nr_san_dir_leaks_)" = 0
> +	test -z "$TEST_RESULTS_SAN_FILE" && return 0
> +
> +	# stderr piped to /dev/null because the directory may have
> +	# been "rmdir"'d already.
> +	! find "$TEST_RESULTS_SAN_DIR" \
> +		-type f \
> +		-name "$TEST_RESULTS_SAN_FILE_PFX.*" 2>/dev/null |
> +	xargs grep -qv "Unable to get registers from thread"

Can't we use `-exec grep -qv "Unable to get registers from thread" {}
\+` instead of using xargs? Or is that unportable? Might make it a bit
easier to reason about the `!` in the presence of a pipe.

Patrick

Re: [PATCH 6/6] test-lib: ignore leaks in the sanitizer's thread code

[Patrick Steinhardt]

On Wed, Jan 01, 2025 at 03:21:24PM -0500, Jeff King wrote:
> One small downside here is that this just suppresses the "were there any
> leaks" check. If there's a real leak _and_ the race was triggered, then
> you'd see the racy false positive in the output. I don't think that's a
> big deal, since both the race and real leaks should be rare-ish, and
> you'd have to encounter both in the same run of a given test script.

Yeah, I think that's fine. You'd have to both be unlucky and trigger the
leak _and_ you'd have to be debugging a testcase that hits the race in
the first place. Which together feels unlikely enough to really matter.

>  t/test-lib.sh | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index c9487d0805..d1f62adbf8 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -1177,7 +1177,8 @@ check_test_results_san_file_empty_ () {
>  	! find "$TEST_RESULTS_SAN_DIR" \
>  		-type f \
>  		-name "$TEST_RESULTS_SAN_FILE_PFX.*" 2>/dev/null |
> -	xargs grep -q ^DEDUP_TOKEN
> +	xargs grep ^DEDUP_TOKEN |
> +	grep -qv sanitizer::GetThreadStackTopAndBottom
>  }

It would be nice to provide some more context here in the form of a
comment so that one doesn't have to blame the commit.

Patrick

Re: [PATCH 3/6] test-lib: rely on logs to detect leaks

[Jeff King]

On Fri, Jan 03, 2025 at 01:05:41PM +0100, Patrick Steinhardt wrote:

> > So now aborting on error is superfluous for LSan! We can get everything
> > we need by checking the logs. And checking the logs is actually
> > preferable, since it gives us more control over silencing false
> > positives (something we do not yet do, but will soon).
> > 
> > So let's tell LSan to just exit normally, even if it finds leaks. We can
> > do so with exitcode=0, which also suppresses the abort_on_error flag.
> 
> The only downside I can think of is that we now run the whole testcase
> to completion before checking for leaks, whereas beforehand we most
> likely aborted the testcase on hitting the first leak. It follows that
> we may now have multiple leak reports, and it is not immediately clear
> which of the commands has actually been failing.

True, I didn't think of that. We do at least check the logs after each
case, so it would have to be multiple leaks in the same snippet. The
LSan output also mentions the process name, though not the arguments
(and some snippets may invoke the same program multiple times).

The other thing I guess we'd miss is that SIGABRT can optionally produce
a core dump. I don't think I've ever found that useful for LSan (since
it's not exiting until the end of process) but occasionally have used it
for ASan (though this patch does not change anything there).

> I think we're now in a clean-enough state regarding memory leaks that
> this isn't a huge issue anymore though.

Yeah, agreed. We can see how much of a problem it is in practice. You
can always switch the flag yourself for a specific run, like:

  LSAN_OPTIONS=exitcode=23 ./twhatever -i

-Peff

Re: [PATCH 4/6] test-lib: simplify leak-log checking

[Jeff King]

On Fri, Jan 03, 2025 at 01:05:45PM +0100, Patrick Steinhardt wrote:

> On Wed, Jan 01, 2025 at 03:17:21PM -0500, Jeff King wrote:
> > @@ -1181,8 +1170,14 @@ test_atexit_handler () {
> >  }
> >  
> >  check_test_results_san_file_empty_ () {
> > -	test -z "$TEST_RESULTS_SAN_FILE" ||
> > -	test "$(nr_san_dir_leaks_)" = 0
> > +	test -z "$TEST_RESULTS_SAN_FILE" && return 0
> > +
> > +	# stderr piped to /dev/null because the directory may have
> > +	# been "rmdir"'d already.
> > +	! find "$TEST_RESULTS_SAN_DIR" \
> > +		-type f \
> > +		-name "$TEST_RESULTS_SAN_FILE_PFX.*" 2>/dev/null |
> > +	xargs grep -qv "Unable to get registers from thread"
> 
> Can't we use `-exec grep -qv "Unable to get registers from thread" {}
> \+` instead of using xargs? Or is that unportable? Might make it a bit
> easier to reason about the `!` in the presence of a pipe.

I don't think that saves us from negating, though. The "grep" will tell
us if it matched any "real" lines, but we want to report that we found
no real lines.

Plus I don't think "find" propagates the exit code from -exec anyway. I
think you can check the exit status with more find logic, so you'd then
use a conditional -print for each file like:

  find ... \
    -exec grep -qv "Unable to get registers from thread" \{} \; \
    -print

and you have to check whether the output is empty. The easiest way to do
that is with another grep! Which also needs negated. ;)

I think if we really want to drop the negation, we'd be best to flip the
function's return, like:

  have_leaks() {
	# not leak-checking
	test -z "$TEST_RESULTS_SAN_FILE" && return 1

	find "$TEST_RESULTS_SAN_DIR" \
		-type f \
		-name "$TEST_RESULTS_SAN_FILE_PFX.*" 2>/dev/null |
	xargs grep ^DEDUP_TOKEN |
	grep -qv sanitizer::GetThreadStackTopAndBottom
  }

And then you could switch the initial "grep" to -exec if you want, but
there's no negation to get rid of, so it is only a preference of -exec
versus xargs.

-Peff

Re: [PATCH 6/6] test-lib: ignore leaks in the sanitizer's thread code

[Jeff King]

On Fri, Jan 03, 2025 at 01:05:48PM +0100, Patrick Steinhardt wrote:

> > diff --git a/t/test-lib.sh b/t/test-lib.sh
> > index c9487d0805..d1f62adbf8 100644
> > --- a/t/test-lib.sh
> > +++ b/t/test-lib.sh
> > @@ -1177,7 +1177,8 @@ check_test_results_san_file_empty_ () {
> >  	! find "$TEST_RESULTS_SAN_DIR" \
> >  		-type f \
> >  		-name "$TEST_RESULTS_SAN_FILE_PFX.*" 2>/dev/null |
> > -	xargs grep -q ^DEDUP_TOKEN
> > +	xargs grep ^DEDUP_TOKEN |
> > +	grep -qv sanitizer::GetThreadStackTopAndBottom
> >  }
> 
> It would be nice to provide some more context here in the form of a
> comment so that one doesn't have to blame the commit.

We can add that on top, but I'm not sure what it should say. Do you want
something along the lines of "add false positives to ignore here..." or
are an explanation of why we are ignoring this particular false
positive?

-Peff

Re: [PATCH 6/6] test-lib: ignore leaks in the sanitizer's thread code

[Patrick Steinhardt]

On Fri, Jan 03, 2025 at 03:26:45PM -0500, Jeff King wrote:
> On Fri, Jan 03, 2025 at 01:05:48PM +0100, Patrick Steinhardt wrote:
> 
> > > diff --git a/t/test-lib.sh b/t/test-lib.sh
> > > index c9487d0805..d1f62adbf8 100644
> > > --- a/t/test-lib.sh
> > > +++ b/t/test-lib.sh
> > > @@ -1177,7 +1177,8 @@ check_test_results_san_file_empty_ () {
> > >  	! find "$TEST_RESULTS_SAN_DIR" \
> > >  		-type f \
> > >  		-name "$TEST_RESULTS_SAN_FILE_PFX.*" 2>/dev/null |
> > > -	xargs grep -q ^DEDUP_TOKEN
> > > +	xargs grep ^DEDUP_TOKEN |
> > > +	grep -qv sanitizer::GetThreadStackTopAndBottom
> > >  }
> > 
> > It would be nice to provide some more context here in the form of a
> > comment so that one doesn't have to blame the commit.
> 
> We can add that on top, but I'm not sure what it should say. Do you want
> something along the lines of "add false positives to ignore here..." or
> are an explanation of why we are ignoring this particular false
> positive?

The latter, ideally also with a reference to the upstream issue you have
created. That makes it way easier to discover why this line exists and
may prompt people to remove the line eventually if they discover that
the issue has been fixed for a while.

It doesn't have to be a full paragraph, but a sentence or to go a long
way sometimes. For more context people can still blame the commit
message.

Patrick

Re: [PATCH 4/6] test-lib: simplify leak-log checking

[Patrick Steinhardt]

On Fri, Jan 03, 2025 at 03:24:10PM -0500, Jeff King wrote:
> On Fri, Jan 03, 2025 at 01:05:45PM +0100, Patrick Steinhardt wrote:
> 
> > On Wed, Jan 01, 2025 at 03:17:21PM -0500, Jeff King wrote:
> > > @@ -1181,8 +1170,14 @@ test_atexit_handler () {
> > >  }
> > >  
> > >  check_test_results_san_file_empty_ () {
> > > -	test -z "$TEST_RESULTS_SAN_FILE" ||
> > > -	test "$(nr_san_dir_leaks_)" = 0
> > > +	test -z "$TEST_RESULTS_SAN_FILE" && return 0
> > > +
> > > +	# stderr piped to /dev/null because the directory may have
> > > +	# been "rmdir"'d already.
> > > +	! find "$TEST_RESULTS_SAN_DIR" \
> > > +		-type f \
> > > +		-name "$TEST_RESULTS_SAN_FILE_PFX.*" 2>/dev/null |
> > > +	xargs grep -qv "Unable to get registers from thread"
> > 
> > Can't we use `-exec grep -qv "Unable to get registers from thread" {}
> > \+` instead of using xargs? Or is that unportable? Might make it a bit
> > easier to reason about the `!` in the presence of a pipe.
> 
> I don't think that saves us from negating, though. The "grep" will tell
> us if it matched any "real" lines, but we want to report that we found
> no real lines.
> 
> Plus I don't think "find" propagates the exit code from -exec anyway. I
> think you can check the exit status with more find logic, so you'd then
> use a conditional -print for each file like:

It should. Quoting find(1):

    If any invocation with the `+' form returns a non-zero value as exit
    status, then find returns a non-zero exit status.

>   find ... \
>     -exec grep -qv "Unable to get registers from thread" \{} \; \
>     -print
> 
> and you have to check whether the output is empty. The easiest way to do
> that is with another grep! Which also needs negated. ;)

Yup, I didn't mean to say that we can drop the negation, but that it
makes it easier to reason about what the negation applies to (the whole
pipe or just the find(1) command)).

> I think if we really want to drop the negation, we'd be best to flip the
> function's return, like:
> 
>   have_leaks() {
> 	# not leak-checking
> 	test -z "$TEST_RESULTS_SAN_FILE" && return 1
> 
> 	find "$TEST_RESULTS_SAN_DIR" \
> 		-type f \
> 		-name "$TEST_RESULTS_SAN_FILE_PFX.*" 2>/dev/null |
> 	xargs grep ^DEDUP_TOKEN |
> 	grep -qv sanitizer::GetThreadStackTopAndBottom
>   }
> 
> And then you could switch the initial "grep" to -exec if you want, but
> there's no negation to get rid of, so it is only a preference of -exec
> versus xargs.

Yup.

Patrick

Re: [PATCH 4/6] test-lib: simplify leak-log checking

[Jeff King]

On Mon, Jan 06, 2025 at 08:56:57AM +0100, Patrick Steinhardt wrote:

> > Plus I don't think "find" propagates the exit code from -exec anyway. I
> > think you can check the exit status with more find logic, so you'd then
> > use a conditional -print for each file like:
> 
> It should. Quoting find(1):
> 
>     If any invocation with the `+' form returns a non-zero value as exit
>     status, then find returns a non-zero exit status.

Ah, right. I tried using ';' to look at individual files, and it does
ignore the code. But of course we don't need to know which logs had
leaks, only that there was at least one.

I think we can make it even simpler, though. I'll post patches in a
moment.

-Peff

[PATCH 0/3] lsan test-lib readability

[Jeff King]

On Wed, Jan 01, 2025 at 03:12:26PM -0500, Jeff King wrote:

>   [1/6]: test-lib: use individual lsan dir for --stress runs
>   [2/6]: Revert "index-pack: spawn threads atomically"
>   [3/6]: test-lib: rely on logs to detect leaks
>   [4/6]: test-lib: simplify leak-log checking
>   [5/6]: test-lib: check leak logs for presence of DEDUP_TOKEN
>   [6/6]: test-lib: ignore leaks in the sanitizer's thread code

And here are a few cosmetic improvements on top based on the review from
Patrick. They should apply on top of jk/lsan-race-ignore-false-positive,
though that is also in 'master' now.

  [1/3]: test-lib: invert return value of check_test_results_san_file_empty
  [2/3]: test-lib: simplify lsan results check
  [3/3]: test-lib: add a few comments to LSan log checking

 t/test-lib-functions.sh |  2 +-
 t/test-lib.sh           | 20 ++++++++++----------
 2 files changed, 11 insertions(+), 11 deletions(-)

-Peff

[PATCH 1/3] test-lib: invert return value of check_test_results_san_file_empty

[Jeff King]

We have a function to check whether LSan logged any leaks. It returns
success for no leaks, and non-zero otherwise. This is the simplest thing
for its callers, who want to say "if no leaks then return early". But
because it's implemented as a shell pipeline, you end up with the
awkward:

  ! find ... |
  xargs grep leaks |
  grep -v false-positives

where the "!" is actually negating the final grep. Switch the return
value (and name) to return success when there are leaks. This should
make the code a little easier to read, and the negation in the callers
still reads pretty naturally.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/test-lib-functions.sh | 2 +-
 t/test-lib.sh           | 8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 78e054ab50..c25cee0ad8 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -927,7 +927,7 @@ test_expect_success () {
 		test -n "$test_skip_test_preamble" ||
 		say >&3 "expecting success of $TEST_NUMBER.$test_count '$1': $test_body"
 		if test_run_ "$test_body" &&
-		   check_test_results_san_file_empty_
+		   ! check_test_results_san_file_has_entries_
 		then
 			test_ok_ "$1"
 		else
diff --git a/t/test-lib.sh b/t/test-lib.sh
index d1f62adbf8..be3553e40e 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1169,20 +1169,20 @@ test_atexit_handler () {
 	teardown_malloc_check
 }
 
-check_test_results_san_file_empty_ () {
-	test -z "$TEST_RESULTS_SAN_FILE" && return 0
+check_test_results_san_file_has_entries_ () {
+	test -z "$TEST_RESULTS_SAN_FILE" && return 1
 
 	# stderr piped to /dev/null because the directory may have
 	# been "rmdir"'d already.
-	! find "$TEST_RESULTS_SAN_DIR" \
+	find "$TEST_RESULTS_SAN_DIR" \
 		-type f \
 		-name "$TEST_RESULTS_SAN_FILE_PFX.*" 2>/dev/null |
 	xargs grep ^DEDUP_TOKEN |
 	grep -qv sanitizer::GetThreadStackTopAndBottom
 }
 
 check_test_results_san_file_ () {
-	if check_test_results_san_file_empty_
+	if ! check_test_results_san_file_has_entries_
 	then
 		return
 	fi &&
-- 
2.48.0.rc2.377.g0507c08f68

[PATCH 2/3] test-lib: simplify lsan results check

[Jeff King]

We want to know if there are any leaks logged by LSan in the results
directory, so we run "find" on the containing directory and pipe it to
xargs. We can accomplish the same thing by just globbing in the shell
and passing the result to grep, which has a few advantages:

  - it's one fewer process to run

  - we can glob on the TEST_RESULTS_SAN_FILE pattern, which is what we
    checked at the beginning of the function, and is the same glob use
    to show the logs in check_test_results_san_file_

  - this correctly handles the case where TEST_OUTPUT_DIRECTORY has a
    space in it. For example doing:

       mkdir "/tmp/foo bar"
       TEST_OUTPUT_DIRECTORY="/tmp/foo bar" make SANITIZE=leak test

    would yield a lot of:

      grep: /tmp/foo: No such file or directory
      grep: bar/test-results/t0006-date.leak/trace.test-tool.582311: No such file or directory

    when there are leaks. We could do the same thing with "xargs
    --null", but that isn't portable.

We are now subject to command-line length limits, but that is also true
of the globbing cat used to show the logs themselves. This hasn't been a
problem in practice.

We do need to use "grep -s" for the case that the glob does not expand
(i.e., there are not any log files at all). This option is in POSIX, and
has been used in t7407 for several years without anybody complaining.
This also also naturally handles the case where the surrounding
directory has already been removed (in which case there are likewise no
files!), dropping the need to comment about it.

Signed-off-by: Jeff King <peff@peff.net>
---
I was surprised by the use of "grep -s" in t7407, since it is totally
pointless there. But I think we can take its presence as a positive sign
for portability.

 t/test-lib.sh | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index be3553e40e..898c2267b8 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1172,12 +1172,7 @@ test_atexit_handler () {
 check_test_results_san_file_has_entries_ () {
 	test -z "$TEST_RESULTS_SAN_FILE" && return 1
 
-	# stderr piped to /dev/null because the directory may have
-	# been "rmdir"'d already.
-	find "$TEST_RESULTS_SAN_DIR" \
-		-type f \
-		-name "$TEST_RESULTS_SAN_FILE_PFX.*" 2>/dev/null |
-	xargs grep ^DEDUP_TOKEN |
+	grep -s ^DEDUP_TOKEN "$TEST_RESULTS_SAN_FILE".* |
 	grep -qv sanitizer::GetThreadStackTopAndBottom
 }
 
-- 
2.48.0.rc2.377.g0507c08f68

[PATCH 3/3] test-lib: add a few comments to LSan log checking

[Jeff King]

Commit b119a687d4 (test-lib: ignore leaks in the sanitizer's thread
code, 2025-01-01) added code to suppress a false positive in the leak
checker. But if you're just reading the code, the obscure grep call is a
bit of a head-scratcher. Let's add a brief comment explaining what's
going on (and anybody digging further can find this commit or that one
for all the details).

Signed-off-by: Jeff King <peff@peff.net>
---
 t/test-lib.sh | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 898c2267b8..9f27a49995 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1172,6 +1172,11 @@ test_atexit_handler () {
 check_test_results_san_file_has_entries_ () {
 	test -z "$TEST_RESULTS_SAN_FILE" && return 1
 
+	# Lines marked with DEDUP_TOKEN show unique leaks. We only care that we
+	# found at least one.
+	#
+	# But also suppress any false positives caused by bugs or races in the
+	# sanitizer itself.
 	grep -s ^DEDUP_TOKEN "$TEST_RESULTS_SAN_FILE".* |
 	grep -qv sanitizer::GetThreadStackTopAndBottom
 }
-- 
2.48.0.rc2.377.g0507c08f68

Re: [PATCH 2/3] test-lib: simplify lsan results check

[Patrick Steinhardt]

On Tue, Jan 07, 2025 at 02:07:52AM -0500, Jeff King wrote:
> We want to know if there are any leaks logged by LSan in the results
> directory, so we run "find" on the containing directory and pipe it to
> xargs. We can accomplish the same thing by just globbing in the shell
> and passing the result to grep, which has a few advantages:
> 
>   - it's one fewer process to run
> 
>   - we can glob on the TEST_RESULTS_SAN_FILE pattern, which is what we
>     checked at the beginning of the function, and is the same glob use

s/use/used

I'm always a bit thrown off by your style of bulleted lists, where they
feel like sentences but start with a lower-case letter, and sometimes
they do and sometimes they don't end with punctuation. Maybe it's just
me not being a native speaker and it's a natural thing to do in English.
In any case, it's nothing that really matters in the end, but would be
happy to learn if this is indeed something you tend to do in English.

>     to show the logs in check_test_results_san_file_
> 
>   - this correctly handles the case where TEST_OUTPUT_DIRECTORY has a
>     space in it. For example doing:
> 
>        mkdir "/tmp/foo bar"
>        TEST_OUTPUT_DIRECTORY="/tmp/foo bar" make SANITIZE=leak test
> 
>     would yield a lot of:
> 
>       grep: /tmp/foo: No such file or directory
>       grep: bar/test-results/t0006-date.leak/trace.test-tool.582311: No such file or directory
> 
>     when there are leaks. We could do the same thing with "xargs
>     --null", but that isn't portable.
> 
> We are now subject to command-line length limits, but that is also true
> of the globbing cat used to show the logs themselves. This hasn't been a
> problem in practice.

Yup, this also came to my mind immediately. But I agree that it
shouldn't be an issue in general.

> We do need to use "grep -s" for the case that the glob does not expand
> (i.e., there are not any log files at all). This option is in POSIX, and
> has been used in t7407 for several years without anybody complaining.
> This also also naturally handles the case where the surrounding
> directory has already been removed (in which case there are likewise no
> files!), dropping the need to comment about it.

Okay. So in case there are no matching files we don't expand the
globbing string, and "--no-messages" makes us ignore that case. A bit
funny, but I don't see any issue with it.

> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I was surprised by the use of "grep -s" in t7407, since it is totally
> pointless there. But I think we can take its presence as a positive sign
> for portability.

Good to know.

>  t/test-lib.sh | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index be3553e40e..898c2267b8 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -1172,12 +1172,7 @@ test_atexit_handler () {
>  check_test_results_san_file_has_entries_ () {
>  	test -z "$TEST_RESULTS_SAN_FILE" && return 1
>  
> -	# stderr piped to /dev/null because the directory may have
> -	# been "rmdir"'d already.
> -	find "$TEST_RESULTS_SAN_DIR" \
> -		-type f \
> -		-name "$TEST_RESULTS_SAN_FILE_PFX.*" 2>/dev/null |
> -	xargs grep ^DEDUP_TOKEN |
> +	grep -s ^DEDUP_TOKEN "$TEST_RESULTS_SAN_FILE".* |
>  	grep -qv sanitizer::GetThreadStackTopAndBottom

And this nicely simplifies things indeed.

Patrick

Re: [PATCH 3/3] test-lib: add a few comments to LSan log checking

[Patrick Steinhardt]

On Tue, Jan 07, 2025 at 02:08:31AM -0500, Jeff King wrote:
> Commit b119a687d4 (test-lib: ignore leaks in the sanitizer's thread
> code, 2025-01-01) added code to suppress a false positive in the leak
> checker. But if you're just reading the code, the obscure grep call is a
> bit of a head-scratcher. Let's add a brief comment explaining what's
> going on (and anybody digging further can find this commit or that one
> for all the details).
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  t/test-lib.sh | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 898c2267b8..9f27a49995 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -1172,6 +1172,11 @@ test_atexit_handler () {
>  check_test_results_san_file_has_entries_ () {
>  	test -z "$TEST_RESULTS_SAN_FILE" && return 1
>  
> +	# Lines marked with DEDUP_TOKEN show unique leaks. We only care that we
> +	# found at least one.
> +	#
> +	# But also suppress any false positives caused by bugs or races in the
> +	# sanitizer itself.
>  	grep -s ^DEDUP_TOKEN "$TEST_RESULTS_SAN_FILE".* |
>  	grep -qv sanitizer::GetThreadStackTopAndBottom
>  }

Thanks for adding this comment!

Patrick

Re: [PATCH 2/3] test-lib: simplify lsan results check

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> We want to know if there are any leaks logged by LSan in the results
> directory, so we run "find" on the containing directory and pipe it to
> xargs. We can accomplish the same thing by just globbing in the shell
> and passing the result to grep, which has a few advantages:
>
>   - it's one fewer process to run
> ...
> We are now subject to command-line length limits, but that is also true
> of the globbing cat used to show the logs themselves. This hasn't been a
> problem in practice.

Nice to see it mentioned here.  And the resulting code does become
simpler to reason about.

> We do need to use "grep -s" for the case that the glob does not expand
> (i.e., there are not any log files at all). This option is in POSIX, and
> has been used in t7407 for several years without anybody complaining.

Also since c625bf0e (git-p4: git-p4 tests with p4 triggers,
2017-07-13) t9831 has also been using it.  It is not like a stray
error message about unmatched glob would really matter here, though.
We are not doing 2>&1 to let the downstream of the pipe see it, and
unless the test is run under "-v" option, it wouldn't even be seen.

> This also also naturally handles the case where the surrounding
> directory has already been removed (in which case there are likewise no
> files!), dropping the need to comment about it.

Nice.

Thanks.

Re: [PATCH 2/3] test-lib: simplify lsan results check

[Jeff King]

On Tue, Jan 07, 2025 at 08:37:33AM +0100, Patrick Steinhardt wrote:

> On Tue, Jan 07, 2025 at 02:07:52AM -0500, Jeff King wrote:
> > We want to know if there are any leaks logged by LSan in the results
> > directory, so we run "find" on the containing directory and pipe it to
> > xargs. We can accomplish the same thing by just globbing in the shell
> > and passing the result to grep, which has a few advantages:
> > 
> >   - it's one fewer process to run
> > 
> >   - we can glob on the TEST_RESULTS_SAN_FILE pattern, which is what we
> >     checked at the beginning of the function, and is the same glob use
> 
> s/use/used
> 
> I'm always a bit thrown off by your style of bulleted lists, where they
> feel like sentences but start with a lower-case letter, and sometimes
> they do and sometimes they don't end with punctuation. Maybe it's just
> me not being a native speaker and it's a natural thing to do in English.
> In any case, it's nothing that really matters in the end, but would be
> happy to learn if this is indeed something you tend to do in English.

Heh. Yeah, I've seen you mention them before and I've been tempted to
start a big discussion. But I never felt like it was worth it. But
tonight's your lucky night. ;)

In short: I think it's a style question. I perceive them as
continuations of the sentence that has the ":". Though admittedly I do
not always grammatically continue that sentence. So for example I could:

  - have one bullet item that completes the sentence.

  - and then another that likewise completes it.

;) I think many style guides would frown on that. Especially with the
periods at the end (you might argue that they should be semicolons).

In the example you quoted above they don't grammatically continue the
sentence, so arguably what I'm saying doesn't even apply. But I also
kind of think of the list items as sentence fragments. That sometimes
happen to make a full sentence. Or need punctuation because that
fragments gets so long it contains multiple sentences.

I dunno. You asked if it is something you tend to do in English. It is
something _I_ tend to do in English, but I think most style guides would
suggest against it (but then, most also suggest against bulleted lists
in the first place). (They probably also suggest against lots of
parentheses).  So I wouldn't necessarily copy me.

My general feeling is that unless a commit message is inaccurate or hard
to understand, we should mostly let it pass (even typos). Yes, they are
an artifact that is enshrined in the history. But at some point they are
also just a written communication between developers, and we all have
our own voices and styles. And make mistakes. Polishing them is
something we _can_ do collaboratively, but there are diminishing
returns.

In case it is not clear, I would not say the same for documentation,
error messages, etc. Those are artifacts that hits a wider audience, and
we have a tool for polishing them together: git.

And people should still proofread and correct their own messages before
sending. Believe it or not, I do always take a final pass when sending
out my commits and still manage to have errors. ;) A lot of times I end
up improving clarity and wording on the final pass, but end up
introducing a typo (I'm pretty sure that the use/used above was me
switching last-minute between "the same glob we use" and "the same glob
used").

Bringing it back to the example at hand, my assumption is that the
bullet list capitalization and punctuation is mostly a question of
style, and isn't making the result hard to understand. But if it is, I
can try to adjust. I actually wrote a bulleted list in a commit message
earlier today and capitalized it just for you. :)

-Peff

Re: [PATCH 2/3] test-lib: simplify lsan results check

[Jeff King]

On Tue, Jan 07, 2025 at 08:23:34AM -0800, Junio C Hamano wrote:

> > We do need to use "grep -s" for the case that the glob does not expand
> > (i.e., there are not any log files at all). This option is in POSIX, and
> > has been used in t7407 for several years without anybody complaining.
> 
> Also since c625bf0e (git-p4: git-p4 tests with p4 triggers,
> 2017-07-13) t9831 has also been using it.  It is not like a stray
> error message about unmatched glob would really matter here, though.
> We are not doing 2>&1 to let the downstream of the pipe see it, and
> unless the test is run under "-v" option, it wouldn't even be seen.

Yeah, I saw those. But I don't think they count since hardly anybody
runs the p4 tests. They do run in CI, but on a rather limited set of
platforms. Though come to think of it, this one would only kick in for
LSan, which may also run on a pretty limited set of platforms. :)

-Peff

Re: [PATCH 2/3] test-lib: simplify lsan results check

[Patrick Steinhardt]

On Thu, Jan 09, 2025 at 02:57:50AM -0500, Jeff King wrote:
> On Tue, Jan 07, 2025 at 08:37:33AM +0100, Patrick Steinhardt wrote:
> 
> > On Tue, Jan 07, 2025 at 02:07:52AM -0500, Jeff King wrote:
> > > We want to know if there are any leaks logged by LSan in the results
> > > directory, so we run "find" on the containing directory and pipe it to
> > > xargs. We can accomplish the same thing by just globbing in the shell
> > > and passing the result to grep, which has a few advantages:
> > > 
> > >   - it's one fewer process to run
> > > 
> > >   - we can glob on the TEST_RESULTS_SAN_FILE pattern, which is what we
> > >     checked at the beginning of the function, and is the same glob use
> > 
> > s/use/used
> > 
> > I'm always a bit thrown off by your style of bulleted lists, where they
> > feel like sentences but start with a lower-case letter, and sometimes
> > they do and sometimes they don't end with punctuation. Maybe it's just
> > me not being a native speaker and it's a natural thing to do in English.
> > In any case, it's nothing that really matters in the end, but would be
> > happy to learn if this is indeed something you tend to do in English.
> 
> Heh. Yeah, I've seen you mention them before and I've been tempted to
> start a big discussion. But I never felt like it was worth it. But
> tonight's your lucky night. ;)
> 
> In short: I think it's a style question. I perceive them as
> continuations of the sentence that has the ":". Though admittedly I do
> not always grammatically continue that sentence. So for example I could:
> 
>   - have one bullet item that completes the sentence.
> 
>   - and then another that likewise completes it.
> 
> ;) I think many style guides would frown on that. Especially with the
> periods at the end (you might argue that they should be semicolons).
> 
> In the example you quoted above they don't grammatically continue the
> sentence, so arguably what I'm saying doesn't even apply. But I also
> kind of think of the list items as sentence fragments. That sometimes
> happen to make a full sentence. Or need punctuation because that
> fragments gets so long it contains multiple sentences.
> 
> I dunno. You asked if it is something you tend to do in English. It is
> something _I_ tend to do in English, but I think most style guides would
> suggest against it (but then, most also suggest against bulleted lists
> in the first place). (They probably also suggest against lots of
> parentheses).  So I wouldn't necessarily copy me.
> 
> My general feeling is that unless a commit message is inaccurate or hard
> to understand, we should mostly let it pass (even typos). Yes, they are
> an artifact that is enshrined in the history. But at some point they are
> also just a written communication between developers, and we all have
> our own voices and styles. And make mistakes. Polishing them is
> something we _can_ do collaboratively, but there are diminishing
> returns.

Yup, agreed. It's a minor detail and I'm happy to gloss over it in the
future.

> In case it is not clear, I would not say the same for documentation,
> error messages, etc. Those are artifacts that hits a wider audience, and
> we have a tool for polishing them together: git.
> 
> And people should still proofread and correct their own messages before
> sending. Believe it or not, I do always take a final pass when sending
> out my commits and still manage to have errors. ;) A lot of times I end
> up improving clarity and wording on the final pass, but end up
> introducing a typo (I'm pretty sure that the use/used above was me
> switching last-minute between "the same glob we use" and "the same glob
> used").
> 
> Bringing it back to the example at hand, my assumption is that the
> bullet list capitalization and punctuation is mostly a question of
> style, and isn't making the result hard to understand. But if it is, I
> can try to adjust. I actually wrote a bulleted list in a commit message
> earlier today and capitalized it just for you. :)

Thanks for explaining!

Patrick