[pull] master from git:master#231
Merged
Merged
Conversation
Rename the .nr member to .nr_ so that callers outside prio-queue.c that directly reference .nr get a compilation error. This catches both existing misuse and future in-flight topics. Add prio_queue_size() for callers that need to know the element count and prio_queue_for_each() for callers that need to walk all elements. Convert all external .nr users: - Loop conditions: use prio_queue_size(), prio_queue_get(), or prio_queue_peek() as the loop condition - Array iterations: use prio_queue_for_each() Signed-off-by: Kristofer Karlsson <krka@spotify.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Defer the actual removal in prio_queue_get() until the next operation. If that next operation is a prio_queue_put(), the removal and insertion are fused into a single replace — writing the new element at the root and sifting it down — which avoids a full remove-rebalance-insert cycle. This matches the dominant usage pattern in git's commit traversal: get a commit, then put its parents. The first parent insertion after each get is now a replace operation automatically. This generalizes the lazy_queue pattern from builtin/describe.c (introduced in 08bb69d) into prio_queue itself. Three callers independently implemented the same get+put fusion: - builtin/describe.c had a full lazy_queue wrapper - commit.c:pop_most_recent_commit() used peek+replace - builtin/show-branch.c:join_revs() used peek+replace All three now collapse to plain _get() and _put(), with the data structure handling the fusion internally. This simplifies callers and means every prio_queue user gets the optimization for free without needing to implement it manually. Remove prio_queue_replace() since no external callers remain. Benchmarked on a 1.8M-commit monorepo (30 interleaved runs, paired t-test, Xeon @ 2.20GHz): Code paths that previously did eager get+put (new optimization): Command base patched change p merge-base --all A A~1000 3828ms 3725ms -2.69% 0.0001 rev-list --count A~1000..A 3055ms 2986ms -2.27% 0.0601 log --oneline A~1000..A 3408ms 3350ms -1.71% 0.0482 Code paths that already had manual get+put fusion (expect neutral — the optimization moves into prio_queue but the number of heap operations stays the same): Command base patched change p show-branch A A~1000 9156ms 9127ms -0.32% 0.3470 describe (4751 revs, 81K repo) 1983ms 1963ms -1.02% <0.001 No regressions in any scenario. Suggested-by: René Scharfe <l.s.r@web.de> Signed-off-by: Kristofer Karlsson <krka@spotify.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* ps/odb-source-packed: odb/source-packed: drop pointer to "files" parent source midx: refactor interfaces to work on "packed" source odb/source-packed: stub out remaining functions odb/source-packed: wire up `freshen_object()` callback odb/source-packed: wire up `find_abbrev_len()` callback odb/source-packed: wire up `count_objects()` callback odb/source-packed: wire up `for_each_object()` callback odb/source-packed: wire up `read_object_stream()` callback odb/source-packed: wire up `read_object_info()` callback packfile: use higher-level interface to implement `has_object_pack()` odb/source-packed: wire up `reprepare()` callback odb/source-packed: wire up `close()` callback odb/source-packed: start converting to a proper `struct odb_source` odb/source-packed: store pointer to "files" instead of generic source packfile: move packed source into "odb/" subsystem packfile: split out packfile list logic packfile: rename `struct packfile_store` to `odb_source_packed`
The `reprepare()` callback function can be used to flush caches of a given object source and then prepare it anew. This is for example used when a concurrent process may have written new objects. Ultimately, this can be seen as doing two separate steps: 1. We drop any caches. 2. We prepare the source. We have one callsite in git-grep(1) though that really only want to do (2). This is done by reaching into the "files" backend directly and then calling `odb_source_packed_prepare()`, which of course may not work with alternate backends. We could in theory just call `reprepare()` here, and that would likely not have any significant downside. But this would certainly feel like a code smell. Instead, generalize the `reprepare()` callback to `prepare()` with a flag that optionally instructs the backend to also flush the caches, which allows us to drop the external `odb_source_packed_prepare()` declaration. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Introduce `odb_prepare()` as a simple wrapper to prepare alternates and then prepare each individual source. Adapt git-grep(1) to use it. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
c4adea8 (Convert CR/LF to LF in tag signatures, 2008-07-11) introduced CR stripping for GPG output on Windows, but intentionally stripped all CR characters unconditionally to "keep the code simpler", even though only CRLF sequences (Windows line endings) needed to be normalized. Later 2f47eae (Split GPG interface into its own helper library, 2011-09-07) moved the code into gpg-interface.c, and 29b3157 (ssh signing: add ssh key format and signing code, 2021-09-10) extracted it into the remove_cr_after() helper when adding SSH signing support, while noticing that it unconditionally strips all CRs, leaving a NEEDSWORK comment. Fix the loop to skip CR only when immediately followed by LF, keeping lone trailing CR characters intact. Rename the function to strip_cr_before_lf to reflect its corrected behavior, and update both call sites and their comments accordingly. Signed-off-by: Antonio De Stefani <antonio.destefani08@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
When setting the upstream of the current branch to the 'main' branch
of the remote 'origin', i.e.,
$ git branch --set-upstream-to origin/main
it is easy to mistakenly write
$ git branch --set-upstream-to origin main
That is parsed as a request to set the upstream of the local branch
'main' to 'origin'. When 'main' does not exist, the command dies
with:
fatal: branch 'main' does not exist
pointing at a branch the user never meant to name. When 'main' does
exist, it instead dies with:
fatal: the requested upstream branch 'origin' does not exist
leaving the user equally puzzled.
When the operated-on branch is missing and '<remote>/<branch>' names
a real remote-tracking ref, suggest the intended form:
$ git branch --set-upstream-to=origin/main
The suggestion is gated on '<remote>/<branch>' existing so it only
appears when a slipped slash is the likely explanation.
Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
When pushing the 'main' branch to the remote 'origin', i.e.,
$ git push origin main
it is easy to mistakenly write
$ git push origin/main
That is parsed as the repository to push to, and since 'origin/main'
is neither a configured remote nor a path it dies with:
fatal: 'origin/main' does not appear to be a git repository
Often 'origin/main' does not exist as a repository, so the command
fails without doing any harm, but it gives no hint that a space was
meant instead of a slash and can leave the user puzzled.
When the argument is not an existing path or configured remote but
its part before the first slash names one, suggest the intended
'<remote> <branch>' form:
$ git push origin main
The suggestion is shown as advice so it can be silenced with
advice.pushRepoLooksLikeRef.
Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
If the reftable writer sees a bogus block size, we return with REFTABLE_API_ERROR, leaking the reftable_writer struct we previously allocated. Originally this case was a BUG(), but it became a regular return in 445f9f4 (reftable: stop using `BUG()` in trivial cases, 2025-02-18). We could obviously fix it by calling "reftable_free(wp)". But we can observe that we never use the allocated "wp" until after we've validated the input options. So let's just bump the allocation down. That fixes the leak, and I think makes the flow of the function more logical (we validate our inputs before doing any work). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
When we find LSan errors, we dump them via "say_color", which goes to stdout. This is mostly harmless, since stdout and stderr tend to go to the same place (either the user's terminal, or to the ".out" file with --verbose-log). But when running under a TAP harness like prove, they are split and stdout is interpreted as TAP output. Historically even this was fine, as the extra lines on stdout would be ignored. But since 389c830 (t: let prove fail when parsing invalid TAP output, 2026-06-04) we instruct the TAP reader to complain, and a leaking test will result in complaints like this (this is a real leak which we have yet to fix): $ GIT_TEST_COMMIT_GRAPH=1 make SANITIZE=leak test [...] Test Summary Report ------------------- t4014-format-patch.sh (Wstat: 256 (exited 1) Tests: 226 Failed: 30) Failed tests: 197-226 Non-zero exit status: 1 Parse errors: Unknown TAP token: "" Unknown TAP token: "=================================================================" Unknown TAP token: "==git==3693658==ERROR: LeakSanitizer: detected memory leaks" Unknown TAP token: "" Unknown TAP token: "Direct leak of 200 byte(s) in 1 object(s) allocated from:" Displayed the first 5 of 1531 TAP syntax errors. Re-run prove with the -p option to see them all. You still see the failing tests, so it's mostly just an annoyance. We can fix it by redirecting to stderr (actually descriptor 4, which is our verbose-respecting variant). I confirmed manually that the output still appears with --verbose-log, and even with a single-test "-i --verbose-only=197" going to the terminal. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
In prepare_bases() we do a custom revision walk, separate from the main
format-patch walk. After we finish, we fail to call release_revisions(),
possibly leaking its contents.
We failed to notice it so far because the revision machinery doesn't
always allocate. But at least one case can trigger the leak: if a commit
graph is present, then the topo-walk allocates revs.topo_walk_info and
some associated data structures. You can see it in the test suite by
running:
make SANITIZE=leak
cd t
GIT_TEST_COMMIT_GRAPH=1 ./t4014-format-patch.sh
which yields many entries like:
==git==3687620==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 200 byte(s) in 1 object(s) allocated from:
#0 0x7f4ccba185cb in malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:74
#1 0x55cd452cdd0b in do_xmalloc wrapper.c:55
#2 0x55cd452cdd9d in xmalloc wrapper.c:76
#3 0x55cd45255473 in init_topo_walk revision.c:3845
#4 0x55cd45255bef in prepare_revision_walk revision.c:4017
#5 0x55cd44ffec40 in prepare_bases builtin/log.c:1872
#6 0x55cd450010ec in cmd_format_patch builtin/log.c:2439
The un-released rev_info has been there since the code was added in
fa2ab86 (format-patch: add '--base' option to record base tree info,
2016-04-26), but back then we didn't even have a way to release rev_info
resources! The actual leak probably started around f0d9cc4
(revision.c: begin refactoring --topo-order logic, 2018-11-01), but it's
hard to bisect because there were so many other unrelated leaks back
then.
So I'm not sure exactly when the leak started beyond "long ago", but it
is easy-ish to find now (since we've plugged all those other leaks) and
the solution is clear.
I didn't add a new test since we can demonstrate it with the existing
ones, but it does require tweaking a test variable. We might consider
ways to get more automatic leak-checking coverage there, but I think it
should be done outside of this fix.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The GPG and SSH signature parsing code has been corrected to strip carriage return characters only when they immediately precede line feeds, instead of unconditionally stripping all carriage returns. * ad/gpg-strip-cr-before-lf: gpg-interface: fix strip_cr_before_lf to only remove CR before LF
A memory leak in the 'reftable_writer_new()' initialization function has been fixed by delaying the allocation of 'struct reftable_writer' until after input options are validated. * jk/reftable-leakfix: reftable: fix unlikely leak on API error
A memory leak in the '--base' handling of 'git format-patch' has been plugged, and the leak reporting of the test suite when running under a TAP harness has been improved. * jk/format-patch-leakfix: format-patch: fix leak of rev_info in prepare_bases() t: move LSan errors from stdout to stderr
When 'git push origin/main' or 'git branch origin main' is run, the command is now recognized as a potential typo, and advice has been added to offer a typo fix. * hn/branch-push-slip-advice: push: suggest <remote> <branch> for a slash slip branch: suggest <remote>/<branch> on upstream slip
The lazy priority queue optimization pattern (deferring actual removal in 'prio_queue_get()' to allow get+put fusion) has been folded directly into 'prio_queue' itself, speeding up commit traversal workflows and simplifying callers. * kk/prio-queue-get-put-fusion: prio-queue: fold lazy_queue into prio_queue for automatic get+put fusion prio-queue: rename .nr to .nr_ and add accessor helpers
The 'reprepare()' callback for object database sources has been generalized into a 'prepare()' callback with an optional flush cache flag, and a new 'odb_prepare()' wrapper has been introduced to allow pre-opening object database sources. * ps/odb-generalize-prepare: odb: introduce `odb_prepare()` odb/source: generalize `reprepare()` callback
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )