[pull] master from git:master#232
Merged
Merged
Conversation
* 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`
An early part of fill_commit_message() function uses write_file_buf() to write out what was prepared in a strbuf, which is primarily meant for use by callers that have their own message prepared fully and called as the last thing to flush it to the destination file. However, the function then opens a file stream in append mode to further write into it. It may have been understandable if this was a later addition, but it seems it came from a single commit, d205234 (builtin/history: implement "reword" subcommand, 2026-01-13), which is somewhat puzzling, but anyway... Just open the file stream upfront for writing, write the message the function has in the strbuf, and then keep writing whatever it wants to write to the same open file stream. And do not forget to close the stream. We are about to pass the resulting file to an external editor, and on some systems, notably Windows, you are not supposed to keep a file open while expecting another program to access it. Diagnosed-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Before using any of the commit-graph bloom-filter code, somebody needs
to call init_bloom_filters(). This initializes the commit-slab we use
for storing filter information. But we don't want to call it twice
(without a matching deinit call in the middle), since it overwrites the
existing slab pointers, leaking the old values.
Usually this init call is done lazily by parse_commit_graph() when we
read a graph file that contains bloom data. But this can lead to some
oddities:
1. We may call parse_commit_graph() multiple times when we have a
split commit graph. I think this doesn't produce any user-visible
bug, because we parse all of the files back-to-back. So even though
we call init_bloom_filters() multiple times, we never look up any
commits in between, so the slab is always empty and initializing it
again happens to do nothing. This is a little sketchy to rely on,
though.
2. We call init_bloom_filters() directly in the "test-tool bloom"
helper so we can call get_or_compute_bloom_filter(). Normally this
is OK, as there is no bloom data in the on-disk graph file. But if
you build with SANITIZE=leak and run:
GIT_TEST_COMMIT_GRAPH=1 \
GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=1 \
./t0095-bloom.sh
there's a leak that happens like this:
a. Our direct init_bloom_filters() sets up the slab.
b. In get_or_compute_bloom_filter() we look in the slab for a
cached entry. We won't find anything yet, but since we don't
use the read-only "peek" accessor (since we'll fill in the
entry if not present), this actually populates the slab with
an allocated chunk.
c. Now we look for an entry in the graph files. So we have to
load them and end up in parse_commit_graph(), which calls
init_bloom_filters() again. That trashes our existing slab
allocation, which is now leaked.
3. There's a similar case in write_commit_graph(), which calls
init_bloom_filters() before get_or_compute_bloom_filter(). I think
this code path is lucky to avoid the leak because it reads the
graph files first, then calls its init_bloom_filters(), and then
starts filling in entries. So even though it has the same overwrite
problem, we'd never actually allocate any slab entries between
overwrites.
The easiest solution here is just to make initialization of the slab
idempotent using an extra flag.
We could actually get away without using the extra flag, for example by
checking whether bloom_filters.stride has been set. But it's probably
better to avoid being too intimate with the commit-slab details.
Likewise we don't actually need to re-initialize after a deinit call;
the slab-clearing function leaves things in a usable state. But it
seemed less surprising to pair the init/deinit calls explicitly.
I suspect this could all be cleaned up a bit more, but it's tricky. The
only function which uses the slab is get_or_compute_bloom_filter(), so
it would be much simpler if it just lazy-initialized the slab itself.
But I think there is a subtle dependency here: we usually only
initialize the slab when we find a graph file that has bloom entries. So
if we were to lose that signal, then even repos without on-disk bloom
data would start trying to populate the slab, wasting memory that will
never get entries filled in from the disk. So we'd need some other way
of signaling "it is worth considering bloom entries at all".
This patch takes a smaller and more direct route to just dealing with
the potential leak issue.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
In prepare_revision_walk(), we convert the pruning pathspecs into bloom-filter "keyvecs" via prepare_to_use_bloom_filter(). This allocates memory which is then freed eventually by release_revisions(), via release_revisions_bloom_keyvecs(). But there's one case where we leak. If a caller uses the same rev_info for multiple walks, calling prepare_revision_walk() multiple times, then subsequent calls will overwrite the earlier keyvecs, leaking them. This can happen with "git show foo bar", which does a separate no-walk traversal for "foo" and "bar". Building with SANITIZE=leak and running the test suite like: GIT_TEST_COMMIT_GRAPH=1 \ GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=1 \ ./t4013-diff-various.sh will trigger a complaint from LSan. It does not happen without those extra flags because we don't store on-disk bloom filters by default, and thus we optimize out the keyvec computation. We can fix the leak by discarding the old entries before generating new ones. There's an alternative fix, which is that prepare_to_use_bloom_filter() could notice that we already have keyvec entries and just reuse them. But this is less safe; the keyvec depends on the pruning pathspec, and we don't know if that has changed. I think it would _probably_ work in practice, since any caller using a rev_info for multiple traversals is probably doing so with the same pathspec. But it would also create a very subtle bug if that assumption is violated. So we'll do the safer thing here, and generate fresh keyvec entries for each traversal. The efficiency difference is probably not noticeable, and this is what was happening already (we just weren't bothering to free the old ones!). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
When line_log_process_ranges_arbitrary_commit() finds out from a Bloom filter that a commit didn't touch the path in question, it can quickly pass its range on to the parent commit. It does so by making a copy of the range, and passing that copy to add_line_range(). But add_line_range() already makes its own copy (either directly, or by merging with an existing range for that parent). So the copy we make is leaked. We can plug the leak by just passing our range directly, without the extra copy. The bug goes back to f32dde8 (line-log: integrate with changed-path Bloom filters, 2020-05-11). We didn't notice because the test suite never explicitly combines these features! You can observe it by building with SANITIZE=leak and running t4211 with some extra flags: GIT_TEST_COMMIT_GRAPH=1 \ GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=1 \ ./t4211-line-log.sh It would probably be useful to have some more targeted test coverage of these features together. But I don't think there's much point in just blindly copying the existing tests and adding bloom-filter support. We already do that via the linux-TEST-vars CI job. We just don't run the leak-checking build with those flags (so if there were a correctness problem, we'd have noticed, just not a leak). So I think we'd benefit from somebody clueful thinking about the interaction of these features and testing the corner cases. But for the purposes of this leak fix, I think we can just rely on the recipe above (and consider running an extra leak-test job with more TEST-vars set). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
In `repo_read_index_unmerged()` we read the index and then drop any unmerged entries to stage 0. In a subsequent commit we'll want to perform this operation on arbitrary indexes, not only the one of the given repository. Prepare for this by splitting out the functionality into a new function that can act on an arbitrary index. While at it, fix a signedness mismatch when iterating through the index cache entries. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
In "reset.c" we still have references to `the_repository`, even though the only entry point into the file already receives a repository as parameter. Update all uses of `the_repository` to instead use the passed-in repo and drop `USE_THE_REPOSITORY_VARIABLE`. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
In a subsequent commit we're about to adapt `reset_head()` so that the reference update to HEAD is optional, only. At this point the function starts to feel misnamed, as it doesn't necessarily have anything to do with the HEAD reference anymore. The gist of the function then is that we reset the working tree to a specific new commit, updating both the index and the checked-out files. Rename it to `reset_working_tree()` to better reflect that. Note that we don't adjust the flags yet. This will happen in a subsequent commit. Suggested-by: Phillip Wood <phillip.wood123@gmail.com> Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The flags passed to `reset_working_tree()` are declared as defines. This has fallen a bit out of practice nowadays, where we instead prefer to use enums. Furthermore, the prefix of those flags does not match the function name anymore after the rename in the preceding commit. Adapt the code to follow modern best practices and adapt the flag names. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
In a subsequent commit we'll add another caller to `reset_working_tree()` that wants to perform a dry-run check of whether it would be possible to update the index and working tree when moving to a new commit. Introduce a new flag that lets the caller perform this operation. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Add an optional `struct odb_source_packed *source` parameter to `packed_object_info()` and `packed_object_info_with_index_pos()`. This parameter is unused at this point in time, but it will be used in a follow-up commit so that we can record the source of a specific object. Note that callers in "odb/source-packed.c" pass the already-available source, but all other callers pass `NULL` instead. This is fine though, as we only care about populating this info when called via the packed store. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The `struct object_info` carries two pieces of information
about how an object was looked up:
- The `whence` enum identifying the backend.
- The backend-tagged union `u` exposing backend-specific details
(currently only the packed-source case, which records the owning
pack, offset and packed object type).
The union is populated unconditionally, even though most callers don't
care about provenance at all.
Split the backend-specific union out into a new public type, `struct
object_info_source`, and make the object info structure carry it via
just another opt-in request pointer. As with all the other requestable
information, callers that need source info allocate a `struct
object_info_source` on the stack and point `sourcep` at it; callers that
don't care about it simply leave the field as a `NULL` pointer. Adapt
callers accordingly.
Note that the `whence` enum is strictly-speaking also backend-specific
information, so it would be another good candidate to be moved into the
`struct object_info_source`. For now though it is left alone, as it will
be replaced by a `struct odb_source` pointer in a subsequent commit.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The previous commit introduced `struct object_info_source` as an opt-in container for backend-specific information, but for now we only moved preexisting data into this structure. Most importantly, the caller has no way yet to learn about which source an object was actually looked up from. Instead, callers have to rely on the `whence` enum to distinguish the object type, but cannot use that enum to tell the object source. Add a `struct odb_source *source` field to the structure and populate it from each backend's lookup path. The `whence` enum is still set and used by callers; it will be removed in a subsequent commit now that `sourcep->source` can identify the backend on its own. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The `whence` field has become redundant now that callers can learn about the exact source an object has been looked up from via the `struct object_info_source::source` field. Adapt callers to use the new field. Note that all callsites already set up the `info.sourcep` request pointer, so the conversion is rather straight-forward. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
In the preceding commits we have migrated all callers to derive their information of how a specific object is stored to use the new object info source instead, and hence the field is now unused. Drop it. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Some of the fields in `struct object_info` are undocumented. Add these missing comments. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
This fixes a racy build failure.
```
builtin/bugreport.c:12:10: fatal error: hook-list.h: No such file or directory
12 | #include "hook-list.h"
| ^~~~~~~~~~~~~
```
hook-list.h must be generated before builtin/bugreport.c is compiled.
Bug: https://bugs.gentoo.org/978326
Fixes: 2eb541e (hook: move is_known_hook() to hook.c for wider use, 2026-04-10)
Signed-off-by: Mike Gilbert <floppym@gentoo.org>
Acked-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
In a subsequent commit we'll introduce a new caller to `reset_working_tree()` that really only wants to update the index and working tree, without updating any references. Introduce a new flag that makes the caller opt in to updating HEAD and adapt all callers to set that flag. Note that in a previous iteration we instead introduced a flag that made callers opt out of updating any references. This was somewhat awkward though because we already have the `UPDATE_ORIG_HEAD` flag, so the result was somewhat inconsistent. Suggested-by: Phillip Wood <phillip.wood123@gmail.com> Signed-off-by: Patrick Steinhardt <ps@pks.im> [jc: fixed-up a typo pointed out by Christian] Signed-off-by: Junio C Hamano <gitster@pobox.com>
When calling `reset_working_tree()` we automatically derive the commit that the callers wants to move from by reading the HEAD commit. Some callers may already have resolved it, or they may want to move from a different commit that doesn't match HEAD. Introduce a new `oid_from` option that lets the caller specify the commit. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
In 652bd02 (rebase: use 'skip_cache_tree_update' option, 2022-11-10), we updated `reset_working_tree()` to stop updating the index tree cache. This was done as a performance optimization: the function is only called by "sequencer.c" and "rebase.c", both of which assume a clean index before they perform their operation, so we know that the end result will be a clean index, too. Consequently, we can skip recomputing the cache as we can instead use `prime_cache_tree()` directly. In a subsequent commit we're about to add a new caller though where the assumption doesn't hold anymore: the index may be dirty before calling `reset_working_tree()`, and consequently we cannot prime the cache with a given tree anymore as the index and tree will mismatch. Adapt the logic so that we only skip the cache tree update in case we're doing a hard reset. While we could introduce logic that only skips the update in case the incoming index was dirty already, that doesn't really feel worth it: after all, the mentioned commit says itself that the performance improvement was negligible anyway. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Expose `replay_result_queue_update()`, which is used to append another reference update to the replay result. This function will be used in a subsequent commit. Suggested-by: Christian Couder <christian.couder@gmail.com> Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The function `handle_reference_updates()` is used by git-history(1) to
update all references that refer to commits that have been rewritten. As
such, it performs two steps:
- It gathers the references that need to be updated in the first
place.
- It prepares and commits the reference transaction.
In a subsequent commit we'll want to handle those two steps separately.
Prepare for this by splitting up the function into two.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
A common operation when editing the commit history is to drop a specific
commit from the history entirely, but this operation is not currently
covered by git-history(1).
A couple of noteworthy bits:
- This is the first git-history(1) command that will ultimately result
in changes to both the index and the working tree. We thus have to
add logic to merge resulting changes into those.
- It is still not possible to replay merge commits, so this limitation
is inherited for the new "drop" command.
- For now we refuse to drop root commits. While we _can_ indeed drop
root commits in the general case, there are edge cases where the
resulting history would become completely empty. This is thus left
to a subsequent patch series.
Other than that, most of the logic is rather straight-forward as we can
continue to build on the preexisting logic in git-history(1) for most of
the part.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
git blame prepends commit hashes of boundary commits with "^", ignored commits with "?" and unblamable commits with "*" and reserves one column for them by extending the hash abbreviation, to avoid showing ambiguous hashes. This reserved column wastes precious screen space, which can be especially irritating when using the option -b to blank out boundary commit hashes and not ignoring any commits. Reserve it only as needed, i.e. if any of those cases are actually shown. Pointed-out-by: Laszlo Ersek <laszlo.ersek@posteo.net> Signed-off-by: René Scharfe <l.s.r@web.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
We still have a couple of uses of `the_repository` in "builtin/refs.c". All of those are trivial to convert though as the command always requires a repository to exist. Convert them to use the passed-in repository and drop `USE_THE_REPOSITORY_VARIABLE`. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Reference-related functionality in Git is currently spread across many different commands: git-update-ref(1), git-for-each-ref(1), git-show-ref(1), git-pack-refs(1) and git-symbolic-ref(1). This makes it hard for users to discover what functionality we have available to work with references. We have thus started to consolidate this functionality into git-refs(1), which is a toolbox of everything related to references. Until now, the command doesn't handle functionality of git-update-ref(1). Fix this gap by introducing a new "delete" subcommand, which is the equivalent of `git update-ref -d`. Note that we're intentionally not using a generic "write" subcommand with a "-d" flag. This is rather harder to discover, and subcommands that are implmented as flags tend to be hard to reason about in the code as we'd have to handle mutually-exclusive flags that stem from the other subcommand-like modes. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Add a new "update" subcommand which mirrors `git update-ref <refname> <oldoid> <newoid>`. This follows the same reasoning as the preceding commit. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The "update" subcommand cannot only update an existing reference, but it can also create new branches and delete existing branches by specifying the all-zeroes object ID as either old or new value. Despite that, we already have the "delete" subcommand as a handy shortcut so that a user can easily delete a branch. This relieves them of needing to understand the more arcane uses of the "update" command, and of counting the number of zeroes they need to pass. But while we have a "delete" subcommand, we don't have an equivalent that would allow the user to create a new branch, which creates a certain asymmetry. Add a new "create" subcommand to plug this gap. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Add a "rename" subcommand to git-refs(1) with the syntax: $ git refs rename <oldref> <newref> It renames <oldref> together with its reflog to <newref>; even when used on a local branch ref, the current value and the reflog of the ref are the only things that are renamed. Document it and redirect casual users to "git branch -m" if that is what they wanted to do. Co-authored-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
A write file stream resource leak has been fixed as part of a code cleanup. * jc/history-message-prep-fix: history: streamline message preparation and plug file stream leak
The 'git refs' toolbox has been extended with new 'create', 'delete', 'update', and 'rename' subcommands to create, delete, update, and rename references, respectively. * ps/refs-writing-subcommands: builtin/refs: add "rename" subcommand builtin/refs: add "create" subcommand builtin/refs: add "update" subcommand builtin/refs: add "delete" subcommand builtin/refs: drop `the_repository`
The 'whence' field in 'struct object_info' has been removed. The backend-specific object information retrieval has been refactored into an opt-in 'struct object_info_source' structure. * ps/odb-drop-whence: odb: document object info fields odb: drop `whence` field from object info treewide: convert users of `whence` to the new source field odb: add `source` field to struct object_info_source odb: make backend-specific fields optional packfile: thread odb_source_packed through packed_object_info()
The experimental 'git history' command has been taught a new 'drop' subcommand to remove a commit, with its descendants replayed onto its parent. * ps/history-drop: builtin/history: implement "drop" subcommand builtin/history: split handling of ref updates into two phases replay: expose `replay_result_queue_update()` reset: stop assuming that the caller passes in a clean index reset: allow the caller to specify the current HEAD object reset: introduce ability to skip updating HEAD reset: introduce dry-run mode reset: modernize flags passed to `reset_working_tree()` reset: rename `reset_head()` reset: drop `USE_THE_REPOSITORY_VARIABLE` read-cache: split out function to drop unmerged entries to stage 0
Various memory leaks in the Bloom-filter code paths that are exposed when running tests with the 'GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=1' environment variable have been plugged. * jk/bloom-leak-fixes: line-log: drop extra copy of range with bloom filters revision: avoid leaking bloom keyvecs with multiple traversals bloom: make bloom-filter slab initialization idempotent
A racy build failure under Meson has been corrected by ensuring that the generated header file 'hook-list.h' is built before compiling files in 'builtin_sources' that depend on it. * mg/meson-hook-list-buildfix: meson: restore hook-list.h to builtin_sources
The alignment of commit object name abbreviations in 'git blame' output has been optimized to reserve a column for marks (caret, question mark, or asterisk) only when such marks are actually shown. * rs/blame-abbrev-marks: blame: reserve mark column only if necessary
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.
5 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 : )