[pull] master from git:master - #176
Merged
Merged
Conversation
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>
A diff using --relative ignores entries outside the current directory. This results in a segfault when we try to process an unmerged entry that's outside of our prefix, since we end up with a NULL diff_filepair and use it without checking that it's valid. I think this bug goes back to 76399c0 (diff.c: return filepair from diff_unmerge(), 2011-04-22). Prior to that, diff_unmerge() knew to skip entries outside of our prefix, due to cd676a5 (diff --relative: output paths as relative to the current subdirectory, 2008-02-12). Back then the caller didn't care that we hadn't added anything to the queue. In 76399c0 that changed; we now returned the pair (or NULL), and the caller in do_oneway_diff() was then called fill_filespec() itself. And it does so without checking for NULL, causing a segfault. The obvious fix is to skip the fill_filespec() call (after which we just return), which this patch does. There's another call to diff_unmerge() in run_diff_files(). That case was already fixed by 8174627 (diff-lib: ignore paths that are outside $cwd if --relative asked, 2021-08-22), but of course it didn't help us for --cached. That commit also claims that checking the result of diff_unmerge() is not enough, as we'd want other code paths to skip the entry, too (even if they wouldn't segfault). But as far as I can tell, that is not true for --cached. We eventually end up in diff_queue_addremove() or in diff_queue_change(), both of which know to return early when we're outside of the prefix. Arguably we could be checking at the top of oneway_diff() whether the path is interesting at all. That would not only avoid this code path entirely, but would also possibly save a small amount of work. But since everything else appears to work OK, I went for the smallest fix here to avoid any regression. Specifically, a comment in oneway_diff() claims we're supposed to advance o->pos, which we might fail to do if we return early. Though that "advance" seems to have gone away in da165f4 (unpack-trees.c: prepare for looking ahead in the index, 2010-01-07), so it is possible the comment is simply out of date. We can explore that separately; checking for a NULL return from diff_unmerge() seems like a sensible thing to do regardless. We can piggy-back on the tests added by 8174627; we're just checking the --cached variant. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
git branch refuses to delete branches that are currently checked out with a message like this: "error: cannot delete branch 'foo' used by worktree at '/path/of/worktree'". This can be confusing if it's an internal checkout for git bisect. Report a more specific error in that case to help users that might have forgotten their bisect run. Suggested-by: stsp <stsp2@yandex.ru> Signed-off-by: René Scharfe <l.s.r@web.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Using gcc 15, compiling with CHECK_ASSERTION_SIDE_EFFECTS=1 causes a complaint about this line in bloom.c having a side effect: assert(version == 1 || version == 2); I think this is pretty clearly a false positive, as those comparisons should not have side effects. The side-effect checker uses a magic definition of assert() that relies on the compiler's optimizer to drop a reference to an otherwise unused variable. And for whatever reason, gcc chooses not to do so here under -O2 (side note: if you have -O0 in your CFLAGS, that naturally creates many more false positives!). This code has been around for a while, but nobody seems to have noticed because we use an older version of the compiler in our static-analysis ci job, and it does not complain. Presumably very few people run this check locally on their more modern compilers. Let's silence the false positive to avoid confusion for anyone running locally, and to make it possible to upgrade the image we use for our static-analysis job. We could just switch to our custom ASSERT() here, but I think we can improve the code by integrating the assertion into the if/else cascade. That avoids repeating the logic about which versions are acceptable. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
We recently ran into a case[1] where old versions of coccinelle ran very slowly, but newer ones are fine. The version we use in GitHub's CI was the old slow version, leading to timeouts of the static-analysis job. We get the old version because we ask for the ubuntu-22.04 image. That has coccinelle 1.1.1, but the "fast" improvement is in coccinelle 1.3.0, specifically their 58619b8fe (break up envs for e1 & e2, 2024-08-18). Bumping to ubuntu-25.10 would be enough to get that new version. But I don't see any need to ask for a specific version at all. We originally used a specific version because coccinelle wasn't available in ubuntu 20.04, so we pinned to 18.04 in d051ed7 (.github/workflows/main.yml: run static-analysis on bionic, 2021-02-08). Later that got bumped in ef46584 (ci: update 'static-analysis' to Ubuntu 22.04, 2022-08-23) when 18.04 support was dropped. It seems like the absence of coccinelle was a blip in 20.04, and we can just stick with "latest" going forward. I tested the result on GitHub's CI. I bumped the matching line in the GitLab definition, but didn't have a simple means of testing (but it's such a trivial change nothing could go wrong, right?). [1] https://lore.kernel.org/git/20260724091152.27794-2-tnyman@openai.com/ Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
When the sequencer processes a chain of "fixup" and "squash" commands it keeps a list of the commands that have been executed. If there are conflicts, then the list is saved when the rebase stops for the user to resolve them. When the rebase resumes, the list is loaded and is used to initialize the count of how many "fixup" and "squash" commands have been processed; if a command has been skipped with "git rebase --skip", then the last command needs to be popped off the end of the list. To count the number of commands, commit_staged_changes() uses the number of newlines in the file plus one. This is due to the slightly unusual way the list is constructed - instead of appending a newline when a command is added, a newline is inserted before the command if the current count is greater than zero. Therefore, when we pop a skipped command off the list, we should also remove the newline that precedes it. Otherwise, when a new command is added, a blank line will be left before it, which will contribute to the fixup count the next time the file is read. Unfortunately, the preceding newline is not removed, leading to an incorrect count. Fix this by removing the newline that appears before the skipped command. In addition to fixing the code that removes a skipped command from the list, the code that reads the list is fixed to skip blank lines. We have had reports of users starting a rebase with one version of git and continuing it with another. Often this happens because the version of git bundled with an IDE or TUI differs from the one used at the command line. By fixing both the reading and writing ends of the problem we ensure the count is correct when an older version of git reads the fixup file written by a newer version and vice versa. Triggering the incorrect count requires the user to skip two "fixup" or "squash" commands before the final command in the chain. An existing test is extended to prevent future regressions. The consequence of miscounting is not serious: we just print the wrong count in the header of the commit message template. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> Signed-off-by: Junio C Hamano <gitster@pobox.com>
When the final command in a chain of "fixup" and "squash" commands is skipped, we should prompt the user to edit the commit message if the chain contains a "fixup -c" command that was not skipped. Unfortunately, commit_staged_changes() only looks for completed "squash" commands and so does not prompt the user to edit the message. Fix this by recording whether a fixup command has the "-c" flag set and then checking whether we have seen either a "fixup -c" or a "squash" command. Add regression tests for skipping a command in the middle of the chain (which currently works but has no test coverage), and for skipping the final command (which is fixed by this patch). Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> Signed-off-by: Junio C Hamano <gitster@pobox.com>
When looking just at the code in oneway_diff(), it seems possible for both "idx" and "tree" to be NULL, in which case we'd potentially segfault while checking the relative prefix. But if you consider what these items actually mean, it shouldn't be possible for both to be NULL. Let's add an assertion and a comment documenting this. It might help human readers, but should also silence static analyzers like Coverity which complain about the potential segfault. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
On Windows, symbolic links come in two flavors: file symlinks and directory symlinks. Since Git was born on Linux where this distinction does not exist, Git for Windows has to auto-detect the type by looking at the target. When the target does not yet exist at symlink creation time, Git for Windows creates a "phantom" file symlink and later, once checkout is complete, calls `CreateFileW()` on the target to check whether it is actually a directory. If the symlink target is a UNC path (e.g. `\\attacker\share`), this auto-detection triggers an SMB connection to the remote host. Windows performs NTLM authentication by default for such connections, which means a crafted repository can exfiltrate the cloning user's NTLMv2 hash to an attacker-controlled server without any user interaction beyond `git clone -c core.symlinks=true <url>`. There are ways to specify UNC paths that start with only a single backslash (e.g. `\??\UNC\host\share`); All of them do start like that, though, so let's use that as a tell-tale that we should skip the auto-detection in `process_phantom_symlink()`. The symlink is then left as a file symlink (the `mklink` default), and a warning is emitted suggesting the user set the `symlink` gitattribute to `dir` if a directory symlink is needed. When the attribute is already set, auto-detection is never invoked in the first place, so that code path is unaffected. This is the same class of vulnerability as CVE-2025-66413 (GHSA-hv9c-4jm9-jh3x) and follows the same general mitigation pattern that MinTTY adopted for ANSI escape sequences referencing network share paths (GHSA-jf4m-m6rv-p6c5). Note that there are legitimate paths starting with a single backslash that are _not_ network paths: drive-less absolute paths are interpreted as relative to the current working directory's drive. In practice, these are highly uncommon (and brittle, just one working directory change away from breaking). In any case, the only consequence is now that the symlink type of those has to be specified via Git attributes, is all. Reported-by: Justin Lee <jessdhoctor@gmail.com> Addresses: CVE-2026-32631 Assisted-by: Claude Opus 4.6 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
We have a few tests related to aliasing deprecated commands which use "whatchanged" and "pack-redundant", as these are the only two deprecated commands we have. Let's pull those names into variables so that we can refactor the tests without relying on the specific names. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
We have a few tests related to aliasing of deprecated commands. They use whatchanged and pack-redundant because those are the only two deprecated commands we have. Eventually those commands will be removed, at which point these tests will be checking nothing useful (they'll just be regular aliases, which we already cover in other tests). We could remove them at that point, but the code to handle deprecated commands will still remain. We probably do want to keep the tests around for the eventual day that we deprecate more commands. So let's ask Git for its list of deprecated commands, and if we don't have any, skip those tests. This also prevents an annoying corner case when your build directory contains old build products. Right now those commands are marked as deprecated builtins and treated specially; we allow aliases and never look for them as dashed external commands. But after they are removed, they aren't special anymore. If your directory happens to contain hardlinks from the build of an older version, that confuses Git: it sees the old hardlinks in place, thinks those are actual external commands, and refuses to allow aliasing. You can see that today like this: make make WITH_BREAKING_CHANGES=1 test The first "make" creates git-whatchanged as a hardlink to Git, and the second does not clean it up (it doesn't know about the whatchanged command at all anymore). t0014 fails because Git won't create an alias to the "external" whatchanged command. Signed-off-by: Jeff King <peff@peff.net> 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>
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.
8 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 : )