Conversation
The "files" transaction backend may encounter errors related to managing
the temporary directory used to stage objects, but silently ignores
these errors. Instead return errors encountered in the
`odb_transaction_files_{prepare,begin,commit}()` interfaces to allow
callers to handle them as needed.
Signed-off-by: Justin Tobler <jltobler@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
When `odb_transaction_begin()` is invoked, the function returns the transaction pointer directly. There is no way for the backend to signal that it failed to set up its state, such as when creating the temporary object directory backing the transaction. In a subsequent commit, git-receive-pack(1) starts using ODB transactions and needs to be able to report such failures rather than silently ignore them. Refactor `odb_transaction_begin()` to return an int error code and write the resulting transaction into an out parameter. Also introduce `odb_transaction_begin_or_die()` as a convenience for callsites that do not need to handle errors explicitly. Note that `odb_transaction_begin()` now returns an error when the ODB already has an inflight transaction pending. ODB transaction call sites that may encounter an inflight transaction are updated to explicitly handle this case. Signed-off-by: Justin Tobler <jltobler@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
When `odb_transaction_commit()` is invoked, the return value of the backend commit callback is silently discarded. A backend has no way to signal that committing failed, such as when the "files" backend cannot migrate its temporary object directory into the permanent ODB. In a subsequent commit, git-receive-pack(1) starts using ODB transaction to stage objects and consequently cares about such failures so it can handle the error appropriately. Change the commit callback signature to return an int error code and have `odb_transaction_commit()` forward it accordingly. Signed-off-by: Justin Tobler <jltobler@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The ODB transaction backend is responsible for creating/managing its own staging area for writing objects. Other child processes spawned by Git may need access to uncommitted objects or write new objects in the staging area though. Introduce `odb_transaction_env()` which is expected to provide the set of environment variables needed by a child process to access the transaction's staging area. Signed-off-by: Justin Tobler <jltobler@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The temporary directory used by git-receive-pack(1) to write objects is
managed slightly differently than how it is done via ODB transactions:
- The temporary directory is eagerly created upfront, instead of
waiting for the first object write.
- The prefix name of the temporary directory is "incoming" instead of
"bulk-fsync".
In a subsequent commit, git-receive-pack(1) will use ODB transactions
instead of `tmp_objdir` directly. To provide a means to configure the
same transaction behavior, introduce `enum odb_transaction_flags` and
the ODB_TRANSACTION_RECEIVE flag intended as a signal for ODB
transactions using the "files" backend to be set up for
git-receive-pack(1). Transaction call sites are updated accordingly to
provide the required flag parameter.
Signed-off-by: Justin Tobler <jltobler@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
When performing the connectivity checks for a shallow ref in `update_shallow_ref()`, the child process environment variables are populated via `tmp_objdir_env()`. This is unnecessary though as `update_shallow_ref()` is only reached after `tmp_objdir_migrate()` has been performed which means there is no longer a temporary directory that needs to be shared with child processes. Drop the call to `tmp_objdir_env()` accordingly. Signed-off-by: Justin Tobler <jltobler@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Objects received by git-receive-pack(1) are quarantined in a temporary "incoming" directory and migrated into the object database prior to the reference updates. The quarantine is currently managed through `tmp_objdir` directly. In a pluggable ODB future, how exactly an object gets written to a transaction may vary for a given ODB source. Refactor git-receive-pack(1) to use the ODB transaction interfaces to manage the object staging area in a more agnostic manner accordingly. Note that the ODB transaction is now responsible for managing the primary and alternate ODBs for the repository. One small change as a result is that the temporary directory is now applied as the primary ODB in the main process instead of an alternate. This does not change anything for git-receive-pack(1) though because it only needs access to the newly written objects and doesn't care how exactly it is set up. Signed-off-by: Justin Tobler <jltobler@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
In 10a6762 (object-file: adapt `stream_object_signature()` to take a stream, 2026-02-23), we have refactored `stream_object_signature()` so that it doesn't create the stream ad-hoc anymore. Instead, callers are expected to pass in a stream, which allows them to construct the streams from different sources. While the stream was previously managed by `stream_object_signature()`, the full lifecycle is now owned by the caller. Hence, it's the caller's responsibility to close the stream, and the called function shouldn't do that anymore. And while the mentioned commit did drop one call that closed the stream, there's a second such call that was missed when reading from the stream fails. The consequence of this can be a double free of the stream. Fix the bug by dropping that leftover call to `odb_read_stream_close()`. Note that it was originally discussed whether this should be treated as a security vulnerability. But there are only two callers: once via `parse_object_with_flags()`, and once via `verify_packfile()`. Neither of these callers plays any role on the transport layer, so this issue is only relevant for objects that are already available via the local object database. Furthermore, a packfile that is corrupted in this way would be detected when receiving the packfile, so it's not easy for an adversary to plant such a packfile, either. Consequently, we decided that this is not covered as part of our threat model. Reported-by: xuqing yang <rigelyoung@icloud.com> Helped-by: Jeff King <peff@peff.net> Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Rewrite single-line && chains by breaking them into multiple lines. Co-authored-by: Vinicius Lira de Freitas <vinilira@usp.br> Signed-off-by: Vinicius Lira de Freitas <vinilira@usp.br> Signed-off-by: Marcelo Machado Lage <marcelomlage@usp.br> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Replace the basic shell commands 'test -f', with more modern test helpers 'test_path_is_file' and 'test_path_is_missing'. These modern helpers emit useful information when the corresponding tests fail, unlike 'test -f' and '! test -f'. The occurrences of '! test -f filename' were replaced by 'file_path_is_missing filename', a stronger guarantee equivalent to '! test -e filename'. Co-authored-by: Vinicius Lira de Freitas <vinilira@usp.br> Signed-off-by: Vinicius Lira de Freitas <vinilira@usp.br> Signed-off-by: Marcelo Machado Lage <marcelomlage@usp.br> Signed-off-by: Junio C Hamano <gitster@pobox.com>
We have several tests in t7900 that verify whether specific maintenance
tasks did or did not run. This is done rather ad-hoc by checking for
spawned Git commands, which is awfully fragile:
- We have to adjust tests whenever arguments to the spawned Git
commands change.
- We don't have a way to verify that negative matches are still
working as expected.
- We rely on maintenance tasks spawning a Git command in the first
place.
We can do much better though, as we already have trace2 regions for each
of the maintenance tasks. Introduce a helper function that extracts all
such regions so that we can get a direct list of all maintenance tasks
that a certain command ran.
Adapt tests that care about whether or not a specific task ran to use
this new helper. Note that many tests still use `test_subcommand`
though, as they really care about the exact command that was executed.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The "pre-auto-gc" hook is supposed to run before auto-maintenance starts. The intent of this is to give users the ability to intercept running maintenance in case there's for example an event that is not supposed to run in parallel with repository maintenance. This hook runs via `need_to_gc()`, which is invoked via two paths: - It is called directly by git-gc(1). - It is called indirectly by git-maintenance(1) via the "gc" task. While the former makes sense, the latter is somewhat off. While the hook is indeed strongly tied to gc'ing a repository, the original intent of the hook is rather to inhibit any kind of automated garbage collection. That noticeably also includes all the other maintenance tasks that our new infrastructure may run, but those aren't getting intercepted at all. The move towards our new maintenance strategy has thus somewhat neutered the effectiveness of the hook. Fix this issue by running the hook before the first auto-maintenance task that would run as determined by the tasks's auto condition. Note that this requires us to lift the call to `run_hooks()` out of `needs_to_gc()`, as the hook would otherwise potentially run multiple times. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
In subsequent patches we'll consolidate all tasks that relate to
maintenance of the object database and move it into the "files" backend.
The relevant code is somewhat scattered though, as several other tasks
are interspersed between.
Refactor the code so that all object database optimizations are grouped
together, which requires us to move worktree pruning and rerere garbage
collection around. In theory, rearranging this code can have an effect
on the object database optimizations:
- Rerere entries really shouldn't impact garbage collection at all, as
these entries are not stored in the object database.
- The index and HEAD reference of pruned worktrees may reference
objects that become unreachable.
That being said, the impact should be overall rather negligible. If the
user was asking us to prune objects with immediate expiration time then
we might now prune objects that were previously still kept alive by the
worktree. But besides being a very specific edge case, it's arguably not
even the wrong thing to also prune any potentially-unreachable objects
immediately.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Extract the object database optimization logic from `cmd_gc()` into a new `maintenance_task_odb()` helper function. This is a pure refactoring with no intended functional change. Note that the message that notifies the user about too many loose objects is moved into the new function, as well. It is inherently an implementation detail of how the "files" source works, and as a consequence we'll move it around in a later commit, as well. This reordering means that the warning may now be printed at a different point in time, but it's not expected that this will have any practical implications. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
When optimizing the object database most of the heavy-lifting is done by git-repack(1). The arguments we pass to this function are assembled in global scope, which is hard to follow. Refactor the logic by moving the vector into `maintenance_task_odb()`. While that means we have to pass more arguments to this function, it has the upside that the logic becomes self-contained without any kind of global interdependencies. This is a pure refactoring with no intended functional change. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The `struct gc_config` contains a set of values that we read via the Git repository's configuration. Several of those values that are consumed by the object database optimization logic are inherently specific to the "files" config. In a later commit we'll make the logic to optimize object databases pluggable. So by carrying these "files"-backend specific values in the generic config struct means that other backends would have to worry about these values, too. This feels somewhat dirty, as implementation- specific details should live with the backends themselves. Inline these values directly at the call sites that need them instead. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Introduce `struct odb_optimize_options` to decouple the options that are specific to optimizing the object database from `struct gc_config`. This structure will be moved into the object database layer in a subsequent commit. Note that there are a small set of backend-specific options in this structure. In an ideal world those of course wouldn't exist, but as we're introducing the object database abstractions retroactively we are somewhat forced to keep them. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
We have two major object database optimization strategies:
- The legacy strategy used by git-gc(1), which absorbs loose objects
into packfiles, and eventually merges all packfiles once we have too
many of them.
- The more recent "geometric" strategy used by git-maintenance(1),
which merges packfiles using a geometric sequence.
These two strategies are still using completely separate code paths. In
a subsequent commit we'll want to make both strategies pluggable though.
Prepare for this change by merging the "geometric" strategy into
`odb_optimize()`. This also allows us to reuse some of the logic we have
in that function.
Note that this change requires us to adapt tests because we're now using
"-q" instead of "--quiet". Naturally though, these invocations are of
course equivalent to one another.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
When invoking either git-gc(1) or git-maintenance(1) with the "--auto" flag then we only perform those maintenance tasks that are actually required. This logic is inherently an implementation detail of the object database backend that's in use. But the logic is scattered around multiple different functions, which makes it hard to make the logic pluggable. Introduce a new `odb_optimize_required()` function that allows us to check these conditions in a generic way. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
We have a couple of functions that are implementation details of how the "files" object database source performs optimizations. These functions often use global state like `the_repository` and implicitly derive the source they are supposed to optimize. Refactor these interfaces to accept a "files" source directly. This will make it easier to move around the whole logic into "odb/source-files.c" in a subsequent step. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
There are a couple of signedness issues in ODB-related functionality.
These are not a problem because we disable -Wsign-compare in this file,
but once we move these functions into "odb/source-files.c" they will
result in warnings.
Fix those issues:
- In `too_many_loose_objects()` we receive a signed limit, but compare
it with the unsigned actual number of loose objects. This is fixed
by bailing out immediately when the limit is smaller than or equal
to zero, which we also do similarly in other places. The warning is
then squelched via a cast.
- In `find_base_packs()` we compare the signed size of the pack
against the unsigned limit. As the pack size is always going to be a
positive file size it's safe to cast it to an unsigned value.
- In `odb_optimize()` we compare the unsigned `keep_pack.nr` value
against the signed `gc_auto_pack_limit`. We only reach this code
when `too_many_packs()` returns true-ish, and that can only happen
when `gc_auto_pack_limit > 0`. Consequently, we can fix the warning
by casting the limit to an unsigned value.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Move `odb_optimize()` and `odb_optimize_required()` from "builtin/gc.c" into the "files" source and wire them up via newly introduced vtable pointers for the object database sources. This makes the logic pluggable and thus allows other backends to have their own, custom implementation. Signed-off-by: Patrick Steinhardt <ps@pks.im> 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>
The object ID shortening and linking in the 'commitdiff' view of 'gitweb' has been corrected to work even when the index line carries a trailing file mode. * tl/gitweb-shorten-hashes-with-modes: gitweb: shorten index hashes with trailing file modes
The logic to write loose objects has been refactored and moved from 'object-file.c' to the loose backend source file 'odb/source-loose.c', making the loose backend more self-contained. This is achieved by first refactoring force_object_loose() to use generic ODB write interfaces instead of loose-backend internals. * ps/odb-move-loose-object-writing: object-file: move logic to write loose objects object-file: move `force_object_loose()` object-file: force objects loose via generic interface object-file: fix memory leak in `force_object_loose()` odb: support setting mtime when writing objects odb: lift object existence check out of the "loose" backend odb: compute object hash in `odb_write_object_ext()` t/u-odb-inmemory: implement wrapper for writing objects odb: compute compat object ID in `odb_write_object_ext()`
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The get_remote_group() function declaration continuation lines used mixed tabs and spaces for indentation, causing the check-whitespace CI job to fail with "indent with spaces" errors. Replace the mixed indentation with tabs only.
When the push remote is specified as a URL, the fetch refspec of a
uniquely matching configured remote is now used to find and update
the remote-tracking branch (e.g., '@{push}').
* hn/url-push-tracking:
remote: find tracking branches for URL push destinations
remote: pass repository to push tracking helper
Traversals with '--exclude-first-parent-only' have been corrected to properly stop after the first parent even when it has already been marked as 'SEEN'. * jc/exclude-first-parent-seen: revision: honor --exclude-first-parent-only with SEEN first parent
A segfault when 'git clone --revision' talks to a server that does not support protocol v2 (falling back to protocol v0) has been corrected. * af/clone-revision-v0-segfault-fix: builtin/clone: fix segfault when using --revision with protocol v0
rewrites_release() in 'remote.c' has been updated to free 'struct rewrite' instances, their '.instead_of' arrays, and their contents. * jc/remote-insteadof-leakfix: remote: plug memory leaks
The remote-matching logic for submodules has been corrected to resolve 'url.*.insteadOf' aliases before comparing the inventoried URL from '.gitmodules' with the URLs of configured remotes. * en/submodule-insteadof-remote-match: submodule: resolve insteadOf aliases when matching remote
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The `chriscool@tuxfamily.org` address is an old one that I don't use anymore, while `christian.couder@gmail.com` is the address I have been sending patches from for a long time. Let's swap the two addresses in the existing entry, so that the Gmail address becomes the primary one and the old tuxfamily.org address is mapped to it. This way both addresses still resolve to the same person, and the address I actually use is the canonical one. Signed-off-by: Christian Couder <christian.couder@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Ben uses the +github GMail trick to identify emails sent to him by folks that found his GitHub profile. At the time, that also meant he had to commit under the same email for GitHub to recognize his commits. He has since found out that GitHub can be configured with more than one email for identification, and he would prefer his canonical email to omit mention of GitHub (where it's not relevant) going forward. Signed-off-by: D. Ben Knoble <ben.knoble@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
'git diff --relative' running with '--cached' has been corrected to avoid a segfault when encountering unmerged paths outside the prefix. * jk/diff-relative-cached-unmerged: diff-lib: add idx/tree sanity check to oneway_diff diff: ignore unmerged paths outside prefix with --relative --cached
Two bugs in how 'git rebase' handles skipped 'fixup' and 'squash' commands have been fixed. One bug caused an incorrect commit count to be shown in the template message when multiple commands were skipped, and another prevented the editor from opening when the final command in a chain containing 'fixup -c' was skipped. * pw/rebase-fixup-fixes: rebase: remember fixup -c after skipping fixup/squash rebase -i: fix counting of fixups after rebase --skip
Object database housekeeping in 'git gc' and 'git maintenance' has been refactored to be pluggable. The files-backend-specific logic, including incremental and geometric repacking as well as object pruning, has been moved out of the command implementation and into the files object database source, enabling future alternative object database backends to implement their own housekeeping services. * ps/odb-pluggable-housekeeping: odb: make optimizations pluggable builtin/gc: fix signedness issues in ODB-related functionality builtin/gc: refactor ODB optimizations to operate on "files" source builtin/gc: introduce `odb_optimize_required()` builtin/gc: move geometric repacking into `odb_optimize()` builtin/gc: introduce object database optimization options builtin/gc: inline config values specific to the "files" backend builtin/gc: make repack arguments self-contained builtin/gc: extract object database optimizations into separate function builtin/gc: move worktree and rerere tasks before object optimizations odb: run "pre-auto-gc" hook for all maintenance tasks t7900: simplify how we check for maintenance tasks
'git branch -d' has been taught to report when a branch cannot be deleted because it is being used in an active bisect run. * rs/branch-delete-bisect-warning: branch: report active bisect run when rejecting delete
The image version used by the static-analysis CI job has been bumped to ubuntu-latest (Ubuntu 24.04), which brings in a newer Coccinelle version that resolves a severe performance regression. A false positive warning from the 'CHECK_ASSERTION_SIDE_EFFECTS' build with GCC 15 in the Bloom filter code has also been silenced to facilitate the image upgrade. * jk/ci-static-analysis-image-bump: ci: bump ubuntu image version for static-analysis job bloom: silence CHECK_ASSERTION_SIDE_EFFECTS false positive
The alias tests in 't/t0014-alias.sh' have been updated to dynamically query the list of deprecated commands using 'git --list-cmds=deprecated' to avoid test failures when running with 'WITH_BREAKING_CHANGES' in a build directory that contains stale executables of formerly deprecated commands. * jk/t0014-dynamic-deprecated-cmds: t0014: generate deprecated command names dynamically t0014: factor out choice of deprecated commands
Git for Windows has been updated to avoid auto-detecting the symlink type if the target path starts with a slash, preventing NTLM credential leaks when checking out repositories with crafted symbolic links pointing to network shares. * js/mingw-symlink-net-share-leak: mingw: skip symlink type auto-detection for network share targets
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Co-authored-by: hub966 <205647957+hub966@users.noreply.github.com>
…errors Co-authored-by: hub966 <205647957+hub966@users.noreply.github.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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Thanks for taking the time to contribute to Git! Please be advised that the
Git community does not use github.com for their contributions. Instead, we use
a mailing list (git@vger.kernel.org) for code submissions, code reviews, and
bug reports. Nevertheless, you can use GitGitGadget (https://gitgitgadget.github.io/)
to conveniently send your Pull Requests commits to our mailing list.
For a single-commit pull request, please leave the pull request description
empty: your commit message itself should describe your changes.
Please read the "guidelines for contributing" linked above!