From f56805ddde51ffcfdd6123300b6c049764a86980 Mon Sep 17 00:00:00 2001 From: Maksim Denisov Date: Wed, 2 Oct 2024 12:27:42 +0200 Subject: Replace manual new allocations for git_strarray with std::vectors ...and remove unused code from git_utils --- src/buildtool/file_system/git_repo.cpp | 30 ++++++-------------- src/buildtool/file_system/git_repo.hpp | 21 +++++++++----- src/buildtool/file_system/git_utils.cpp | 32 ---------------------- src/buildtool/file_system/git_utils.hpp | 13 --------- src/other_tools/git_operations/git_repo_remote.cpp | 14 ++++------ 5 files changed, 27 insertions(+), 83 deletions(-) (limited to 'src') diff --git a/src/buildtool/file_system/git_repo.cpp b/src/buildtool/file_system/git_repo.cpp index f3769008..489d50a7 100644 --- a/src/buildtool/file_system/git_repo.cpp +++ b/src/buildtool/file_system/git_repo.cpp @@ -905,20 +905,16 @@ auto GitRepo::FetchFromPath(std::shared_ptr cfg, fetch_opts.update_fetchhead = 0; // setup fetch refspecs array - git_strarray refspecs_array_obj{}; + GitStrArray refspecs_array_obj; if (branch) { // make sure we check for tags as well - std::string tag = fmt::format("+refs/tags/{}", *branch); - std::string head = fmt::format("+refs/heads/{}", *branch); - PopulateStrarray(&refspecs_array_obj, {tag, head}); + refspecs_array_obj.AddEntry(fmt::format("+refs/tags/{}", *branch)); + refspecs_array_obj.AddEntry(fmt::format("+refs/heads/{}", *branch)); } - auto refspecs_array = - std::unique_ptr( - &refspecs_array_obj, strarray_deleter); + auto const refspecs_array = refspecs_array_obj.Get(); if (git_remote_fetch( - remote.get(), refspecs_array.get(), &fetch_opts, nullptr) != - 0) { + remote.get(), &refspecs_array, &fetch_opts, nullptr) != 0) { (*logger)(fmt::format( "Fetching {} in local repository {} failed with:\n{}", branch ? fmt::format("branch {}", *branch) : "all", @@ -2106,17 +2102,7 @@ auto GitRepo::CreateTreeFromDirectory(std::filesystem::path const& dir, #endif // BOOTSTRAP_BUILD_TOOL } -void GitRepo::PopulateStrarray( - git_strarray* array, - std::vector const& string_list) noexcept { - array->count = string_list.size(); - array->strings = gsl::owner(new char*[string_list.size()]); - for (auto const& elem : string_list) { - auto i = - static_cast(&elem - &string_list[0]); // get index - // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-pointer-arithmetic) - array->strings[i] = gsl::owner(new char[elem.size() + 1]); - // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-pointer-arithmetic) - strncpy(array->strings[i], elem.c_str(), elem.size() + 1); - } +auto GitRepo::GitStrArray::Get() & noexcept -> git_strarray { + return git_strarray{.strings = entry_pointers_.data(), + .count = entry_pointers_.size()}; } diff --git a/src/buildtool/file_system/git_repo.hpp b/src/buildtool/file_system/git_repo.hpp index 03ce92d8..942e97b7 100644 --- a/src/buildtool/file_system/git_repo.hpp +++ b/src/buildtool/file_system/git_repo.hpp @@ -33,6 +33,7 @@ extern "C" { struct git_repository; struct git_config; +struct git_strarray; } /// \brief Git repository logic. @@ -378,13 +379,19 @@ class GitRepo { std::filesystem::path const& dir, anon_logger_ptr const& logger) noexcept -> std::optional; - /// \brief Helper function to allocate and populate the char** pointer of a - /// git_strarray from a vector of standard strings. User MUST use - /// git_strarray_dispose to deallocate the inner pointer when the strarray - /// is not needed anymore! - static void PopulateStrarray( - git_strarray* array, - std::vector const& string_list) noexcept; + class GitStrArray final { + public: + void AddEntry(std::string entry) { + char* const entry_ptr = + entries_.emplace_back(std::move(entry)).data(); + entry_pointers_.push_back(entry_ptr); + } + [[nodiscard]] auto Get() & noexcept -> git_strarray; + + private: + std::vector entries_; + std::vector entry_pointers_; + }; }; #endif // INCLUDED_SRC_BUILDTOOL_FILE_SYSTEM_GIT_REPO_HPP diff --git a/src/buildtool/file_system/git_utils.cpp b/src/buildtool/file_system/git_utils.cpp index d9833521..8612d039 100644 --- a/src/buildtool/file_system/git_utils.cpp +++ b/src/buildtool/file_system/git_utils.cpp @@ -82,38 +82,6 @@ void tree_closer(gsl::owner tree) { #endif } -void treebuilder_closer(gsl::owner builder) { -#ifndef BOOTSTRAP_BUILD_TOOL - git_treebuilder_free(builder); -#endif -} - -void index_closer(gsl::owner index) { -#ifndef BOOTSTRAP_BUILD_TOOL - git_index_free(index); -#endif -} - -void strarray_closer(gsl::owner strarray) { -#ifndef BOOTSTRAP_BUILD_TOOL - git_strarray_dispose(strarray); -#endif -} - -void strarray_deleter(gsl::owner strarray) { -#ifndef BOOTSTRAP_BUILD_TOOL - if (strarray->strings != nullptr) { - for (std::size_t i = 0; i < strarray->count; ++i) { - // NOLINTNEXTLINE(cppcoreguidelines-owning-memory,cppcoreguidelines-pro-bounds-pointer-arithmetic) - delete[] strarray->strings[i]; - } - delete[] strarray->strings; - strarray->strings = nullptr; - strarray->count = 0; - } -#endif -} - void signature_closer(gsl::owner signature) { #ifndef BOOTSTRAP_BUILD_TOOL git_signature_free(signature); diff --git a/src/buildtool/file_system/git_utils.hpp b/src/buildtool/file_system/git_utils.hpp index b2c5173f..916e9fbe 100644 --- a/src/buildtool/file_system/git_utils.hpp +++ b/src/buildtool/file_system/git_utils.hpp @@ -25,9 +25,6 @@ extern "C" { struct git_oid; struct git_odb; struct git_tree; -struct git_treebuilder; -struct git_index; -struct git_strarray; struct git_signature; struct git_object; struct git_remote; @@ -52,16 +49,6 @@ void odb_closer(gsl::owner odb); void tree_closer(gsl::owner tree); -void treebuilder_closer(gsl::owner builder); - -void index_closer(gsl::owner index); - -// to be used for strarrays allocated by libgit2 -void strarray_closer(gsl::owner strarray); - -// to be used for strarrays allocated manually -void strarray_deleter(gsl::owner strarray); - void signature_closer(gsl::owner signature); void object_closer(gsl::owner object); diff --git a/src/other_tools/git_operations/git_repo_remote.cpp b/src/other_tools/git_operations/git_repo_remote.cpp index 810df860..b9b9cc93 100644 --- a/src/other_tools/git_operations/git_repo_remote.cpp +++ b/src/other_tools/git_operations/git_repo_remote.cpp @@ -361,20 +361,16 @@ auto GitRepoRemote::FetchFromRemote(std::shared_ptr cfg, fetch_opts.update_fetchhead = 0; // setup fetch refspecs array - git_strarray refspecs_array_obj{}; + GitStrArray refspecs_array_obj; if (branch) { // make sure we check for tags as well - std::string tag = fmt::format("+refs/tags/{}", *branch); - std::string head = fmt::format("+refs/heads/{}", *branch); - PopulateStrarray(&refspecs_array_obj, {tag, head}); + refspecs_array_obj.AddEntry(fmt::format("+refs/tags/{}", *branch)); + refspecs_array_obj.AddEntry(fmt::format("+refs/heads/{}", *branch)); } - auto refspecs_array = - std::unique_ptr( - &refspecs_array_obj, strarray_deleter); + auto const refspecs_array = refspecs_array_obj.Get(); if (git_remote_fetch( - remote.get(), refspecs_array.get(), &fetch_opts, nullptr) != - 0) { + remote.get(), &refspecs_array, &fetch_opts, nullptr) != 0) { (*logger)( fmt::format("Fetching{} in git repository {} failed " "with:\n{}", -- cgit v1.2.3