diff --git a/.clang-format b/.clang-format index 1aadf2cadce..44f493019a9 100644 --- a/.clang-format +++ b/.clang-format @@ -33,3 +33,4 @@ PPIndentWidth: 2 BinPackArguments: false BreakBeforeTernaryOperators: true SeparateDefinitionBlocks: Always +OneLineFormatOffRegex: "// NOFORMAT$" diff --git a/packaging/dependencies.nix b/packaging/dependencies.nix index 183ec3fb6a6..2a435133a3d 100644 --- a/packaging/dependencies.nix +++ b/packaging/dependencies.nix @@ -164,33 +164,35 @@ scope: { ]; }); - libgit2 = - ( - if lib.versionAtLeast pkgs.libgit2.version "1.9.4" then - pkgs.libgit2 - else - # Grab newer libgit2. - pkgs.libgit2.overrideAttrs rec { - version = "1.9.4"; - src = pkgs.fetchFromGitHub { - owner = "libgit2"; - repo = "libgit2"; - tag = "v${version}"; - hash = "sha256-ZKUiz3pdFE2SKxh53X2oyr7hs32Njj5YVA0OXDXz7h0="; - }; - } - ).overrideAttrs - (old: { - separateDebugInfo = true; - - patches = old.patches or [ ] ++ [ - # Fix a use-after-free crash when `git_thread_create` fails during - # pack building (e.g. with EAGAIN under thread pressure), leaving - # orphaned delta-search worker threads running while the - # packbuilder is freed. - ./patches/libgit2-packbuilder-dont-fail-on-thread-create-error.patch - ]; - }); + libgit2 = pkgs.libgit2.overrideAttrs ( + finalAttrs: prevAttrs: { + version = "2.0.0-rc.1"; + src = pkgs.fetchFromGitHub { + owner = "libgit2"; + repo = "libgit2"; + rev = "ae45d0d168f7e8dbfdb8c623589cb51caac96ab3"; + hash = "sha256-3sbqHm37SOwBeFgtjI2DLN6kx1F7G2N1m6rRIkqDXNI="; + }; + patches = prevAttrs.patches or [ ] ++ [ + ./patches/0002-memory-config.patch + ./patches/0003-packbuilder-correct-config.patch + + # Fix a use-after-free crash when `git_thread_create` fails during + # pack building (e.g. with EAGAIN under thread pressure), leaving + # orphaned delta-search worker threads running while the + # packbuilder is freed. + # TODO: we can probably drop this patch since we're not finding deltas anymore. + ./patches/libgit2-packbuilder-dont-fail-on-thread-create-error.patch + ]; + separateDebugInfo = true; + # Nixpkgs derives `meta.changelog` from `src.tag`, which is null + # here since we fetch an untagged commit. This would be harmless + # except that nixpkgs variants with provenance support + # (`derivationWithMeta`) force `meta.changelog` at derivation + # instantiation time, causing an eval error. + meta = builtins.removeAttrs prevAttrs.meta [ "changelog" ]; + } + ); # TODO Hack until https://github.com/NixOS/nixpkgs/issues/45462 is fixed. boost = diff --git a/packaging/patches/0002-memory-config.patch b/packaging/patches/0002-memory-config.patch new file mode 100644 index 00000000000..ce1ff61acab --- /dev/null +++ b/packaging/patches/0002-memory-config.patch @@ -0,0 +1,22 @@ +diff --git a/include/git2/sys/config.h b/include/git2/sys/config.h +index dcce18e6f..8945a3285 100644 +--- a/include/git2/sys/config.h ++++ b/include/git2/sys/config.h +@@ -200,7 +200,7 @@ GIT_EXTERN(int) git_config_backend_memory_options_init( + * @param opts the options to initialize this backend with, or NULL + * @return 0 on success or an error code + */ +-extern int git_config_backend_from_string( ++GIT_EXTERN(int) git_config_backend_from_string( + git_config_backend **out, + const char *cfg, + size_t len, +@@ -216,7 +216,7 @@ extern int git_config_backend_from_string( + * @param opts the options to initialize this backend with, or NULL + * @return 0 on success or an error code + */ +-extern int git_config_backend_from_values( ++GIT_EXTERN(int) git_config_backend_from_values( + git_config_backend **out, + const char **values, + size_t len, diff --git a/packaging/patches/0003-packbuilder-correct-config.patch b/packaging/patches/0003-packbuilder-correct-config.patch new file mode 100644 index 00000000000..8d1ab47920f --- /dev/null +++ b/packaging/patches/0003-packbuilder-correct-config.patch @@ -0,0 +1,45 @@ +diff --git a/src/libgit2/pack-objects.c b/src/libgit2/pack-objects.c +index 0da84b657..d540795f9 100644 +--- a/src/libgit2/pack-objects.c ++++ b/src/libgit2/pack-objects.c +@@ -115,8 +115,9 @@ static int packbuilder_config(git_packbuilder *pb) + GIT_PACK_DELTA_CACHE_SIZE); + config_get("pack.deltaCacheLimit", pb->cache_max_small_delta_size, + GIT_PACK_DELTA_CACHE_LIMIT); +- config_get("pack.deltaCacheSize", pb->big_file_threshold, ++ config_get("core.bigFileThreshold", pb->big_file_threshold, + GIT_PACK_BIG_FILE_THRESHOLD); ++ config_get("pack.window", pb->window_size, GIT_PACK_WINDOW); + config_get("pack.windowMemory", pb->window_memory_limit, 0); + + #undef config_get +@@ -1341,7 +1342,7 @@ int git_packbuilder__prepare(git_packbuilder *pb) + size_t i, n = 0; + int error; + +- if (pb->nr_objects == 0 || pb->done) ++ if (pb->nr_objects == 0 || pb->done || pb->window_size == 0) + return 0; /* nothing to do */ + + /* +@@ -1369,7 +1370,7 @@ int git_packbuilder__prepare(git_packbuilder *pb) + if (n > 1) { + git__tsort((void **)delta_list, n, type_size_sort); + if ((error = ll_find_deltas(pb, delta_list, n, +- GIT_PACK_WINDOW + 1, ++ pb->window_size + 1, + GIT_PACK_DEPTH)) < 0) { + git__free(delta_list); + return error; +diff --git a/src/libgit2/pack-objects.h b/src/libgit2/pack-objects.h +index ad04fb0ab..d9e03df60 100644 +--- a/src/libgit2/pack-objects.h ++++ b/src/libgit2/pack-objects.h +@@ -94,6 +94,7 @@ struct git_packbuilder { + size_t cache_max_small_delta_size; + size_t big_file_threshold; + size_t window_memory_limit; ++ size_t window_size; + + unsigned int nr_threads; /* nr of threads to use */ + diff --git a/packaging/secure-packages/flake.lock b/packaging/secure-packages/flake.lock index ff01da98612..e420ac4ef96 100644 --- a/packaging/secure-packages/flake.lock +++ b/packaging/secure-packages/flake.lock @@ -77,12 +77,12 @@ }, "nixpkgs": { "locked": { - "lastModified": 1788280191, - "narHash": "sha256-mB3qK5ql+QFtajxSI/qwCPRPaJMuWyqIKnTZvADKwU8=", - "rev": "88369aab48f8d2b0f69126d306c5af06e997849b", - "revCount": 1013502, + "lastModified": 1789082161, + "narHash": "sha256-E9iYEGs3Jx9xmYsP4nbVAK+7TXfMjGjGAazuyYLpivU=", + "rev": "07b3c48788b0deb9ee48c40ed5ff1dff7f4643e7", + "revCount": 1013551, "type": "tarball", - "url": "https://api.flakehub.com/f/pinned/DeterminateSystems/secure-packages-26.05/0.1.1013502%2Brev-88369aab48f8d2b0f69126d306c5af06e997849b/01a05deb-f862-7c32-a065-599b037b2e53/source.tar.gz" + "url": "https://api.flakehub.com/f/pinned/DeterminateSystems/secure-packages-26.05/0.1.1013551%2Brev-07b3c48788b0deb9ee48c40ed5ff1dff7f4643e7/01a08dbc-1a6d-7e60-8dd0-bc2bf8ecb4fb/source.tar.gz" }, "original": { "type": "tarball", diff --git a/src/libfetchers/git-utils.cc b/src/libfetchers/git-utils.cc index 52986a6b450..fe74979d8d5 100644 --- a/src/libfetchers/git-utils.cc +++ b/src/libfetchers/git-utils.cc @@ -16,11 +16,13 @@ #include "nix/util/executable-path.hh" #include "nix/util/deleter.hh" +#include #include #include #include #include #include +#include #include #include #include @@ -115,6 +117,11 @@ typedef std::unique_ptr> typedef std::unique_ptr> StatusList; typedef std::unique_ptr> Remote; typedef std::unique_ptr> GitConfig; +typedef std::unique_ptrfree(backend); + })> + GitConfigBackend; typedef std::unique_ptr> ConfigIterator; typedef std::unique_ptr> ObjectDb; typedef std::unique_ptr> PackBuilder; @@ -123,10 +130,22 @@ typedef std::unique_ptr> Index; static Hash toHash(const git_oid & oid) { -#ifdef GIT_EXPERIMENTAL_SHA256 - assert(oid.type == GIT_OID_SHA1); + HashAlgorithm algo; +#if LIBGIT2_VERSION_CHECK(2, 0, 0) + switch (oid.type) { + case GIT_OID_SHA1: + algo = HashAlgorithm::SHA1; + break; + case GIT_OID_SHA256: + algo = HashAlgorithm::SHA256; + break; + default: + unreachable(); + } +#else + algo = HashAlgorithm::SHA1; #endif - Hash hash(HashAlgorithm::SHA1); + Hash hash(algo); memcpy(hash.hash, oid.id, hash.hashSize); return hash; } @@ -137,14 +156,41 @@ static void initLibGit2() std::call_once(initialized, []() { if (git_libgit2_init() < 0) throw GitError("initialising libgit2"); + + /* Nuke the "hashing on all reads" behavior, since that can lead to bad + performance https://github.com/libgit2/libgit2/issues/4951. It's a + compromise of course, but one that is mostly in line with git cli and + like how we don't recalculate narHash when reading from a store. */ + git_libgit2_opts(GIT_OPT_ENABLE_STRICT_HASH_VERIFICATION, 0); }); } static git_oid hashToOID(const Hash & hash) { git_oid oid; +#if LIBGIT2_VERSION_CHECK(2, 0, 0) + git_oid_t t; +# pragma GCC diagnostic push +# pragma GCC diagnostic ignored "-Wswitch-enum" + switch (hash.algo) { + case HashAlgorithm::SHA1: + t = GIT_OID_SHA1; + break; + case HashAlgorithm::SHA256: + t = GIT_OID_SHA256; + break; + default: + throw Error("unsupported hash algorithm for Git: %s", printHashAlgo(hash.algo)); + } +# pragma GCC diagnostic pop + if (git_oid_from_raw(&oid, hash.hash, t)) + /* This can really never happen, since libgit2 just reads out our raw bytes. + The only failure mode is us specifying an invalid `type` parameter. */ + unreachable(); +#else if (git_oid_fromstr(&oid, hash.gitRev().c_str())) throw GitError("cannot convert '%s' to a Git OID", hash.gitRev()); +#endif return oid; } @@ -297,6 +343,28 @@ struct GitRepoImpl : GitRepo, std::enable_shared_from_this if (git_repository_open(Setter(repo), path.string().c_str())) throw GitError("opening Git repository %s", PathFmt(path)); + GitConfig config; + if (git_repository_config(Setter(config), *this)) + throw GitError("getting Git repository config"); + + /* Create an in-memory configuration so that we can set config options without modifying the + config file on-disk. */ + git_config_backend_memory_options configOpts = GIT_CONFIG_BACKEND_MEMORY_OPTIONS_INIT; + configOpts.backend_type = "nix"; + + std::vector configValues; + if (options.dontFindDeltas) + configValues.push_back("pack.window=0"); + + GitConfigBackend memBackend; + if (git_config_backend_from_values(Setter(memBackend), configValues.data(), configValues.size(), &configOpts)) + throw GitError("creating an in-memory Git config"); + + if (git_config_add_backend(config.get(), memBackend.get(), GIT_CONFIG_LEVEL_APP, *this, /*force=*/false)) + throw GitError("adding the in-memory Git configuration backend"); + + memBackend.release(); + ObjectDb odb; if (options.packfilesOnly) { /* Create a fresh object database because by default the repo also @@ -310,7 +378,16 @@ struct GitRepoImpl : GitRepo, std::enable_shared_from_this if (git_odb_new(Setter(odb))) throw GitError("creating Git object database"); - if (git_odb_backend_pack(&packBackend, (path / "objects").string().c_str())) +#if LIBGIT2_VERSION_CHECK(2, 0, 0) + git_odb_backend_pack_options packOpts = GIT_ODB_OPTIONS_INIT; +#endif + if (git_odb_backend_pack( + &packBackend, + (path / "objects").string().c_str() +#if LIBGIT2_VERSION_CHECK(2, 0, 0) + , &packOpts // NOFORMAT +#endif + )) throw GitError("creating pack backend"); if (git_odb_add_backend(odb.get(), packBackend, 1)) @@ -368,7 +445,20 @@ struct GitRepoImpl : GitRepo, std::enable_shared_from_this // (synchronously on the git_packbuilder_write_buf thread) Indexer indexer; git_indexer_progress stats; - if (git_indexer_new(Setter(indexer), pack_dir_path.c_str(), 0, nullptr, nullptr)) +#if LIBGIT2_VERSION_CHECK(2, 0, 0) + git_indexer_options indexerOpts = GIT_INDEXER_OPTIONS_INIT; +#endif + if (git_indexer_new( + Setter(indexer), + pack_dir_path.c_str(), +#if LIBGIT2_VERSION_CHECK(2, 0, 0) + &indexerOpts +#else + 0, + nullptr, + nullptr +#endif + )) throw GitError("creating git packfile indexer"); // TODO: provide index callback for checkInterrupt() termination @@ -1553,7 +1643,16 @@ ref Settings::getTarballCache() const static auto repoDir = std::filesystem::path(getCacheDir()) / "tarball-cache-v2"; auto tarballCache(_tarballCache.lock()); if (!*tarballCache) - *tarballCache = GitRepo::openRepo(repoDir, {.create = true, .bare = true, .packfilesOnly = true}); + *tarballCache = GitRepo::openRepo( + repoDir, + { + .create = true, + .bare = true, + .packfilesOnly = true, + /* Tarball unpacking is not expected to benefit from deltas much, + compared to how much CPU times it takes to find. */ + .dontFindDeltas = true, + }); return ref(*tarballCache); } diff --git a/src/libfetchers/include/nix/fetchers/git-utils.hh b/src/libfetchers/include/nix/fetchers/git-utils.hh index 6ffc7372df0..0d037bedb0d 100644 --- a/src/libfetchers/include/nix/fetchers/git-utils.hh +++ b/src/libfetchers/include/nix/fetchers/git-utils.hh @@ -44,6 +44,12 @@ struct GitRepo bool create = false; bool bare = false; bool packfilesOnly = false; + /** + * Whether to avoid finding deltas when writing packfiles. It's an + * expensive operation, which should be avoided if no benefit is + * expected from possible deduplication in the same packfile. + */ + bool dontFindDeltas = false; }; static ref openRepo(const std::filesystem::path & path, Options options);