From 2ebf355989eb92ac9967eceee0af14d39477afe0 Mon Sep 17 00:00:00 2001 From: Oliver Reiche Date: Fri, 23 Feb 2024 18:51:51 +0100 Subject: just-mr: Fix shell out execution ... by avoiding reusing temp dirs for execute. While we are at it, also refactor LocalFetchViaTmpRepo() to create its own empty temp dirs, that cannot be reused by the caller. --- src/other_tools/git_operations/TARGETS | 1 + src/other_tools/git_operations/git_repo_remote.cpp | 27 ++++++++++++++++------ src/other_tools/git_operations/git_repo_remote.hpp | 16 ++++--------- 3 files changed, 25 insertions(+), 19 deletions(-) (limited to 'src/other_tools/git_operations') diff --git a/src/other_tools/git_operations/TARGETS b/src/other_tools/git_operations/TARGETS index dba6fc18..1cd49035 100644 --- a/src/other_tools/git_operations/TARGETS +++ b/src/other_tools/git_operations/TARGETS @@ -36,6 +36,7 @@ , ["@", "json", "", "json"] , ["src/buildtool/system", "system_command"] , "git_config_settings" + , ["src/utils/cpp", "tmp_dir"] ] } , "git_config_settings": diff --git a/src/other_tools/git_operations/git_repo_remote.cpp b/src/other_tools/git_operations/git_repo_remote.cpp index 2a434928..f3964e74 100644 --- a/src/other_tools/git_operations/git_repo_remote.cpp +++ b/src/other_tools/git_operations/git_repo_remote.cpp @@ -20,6 +20,7 @@ #include "src/buildtool/logging/logger.hpp" #include "src/buildtool/system/system_command.hpp" #include "src/other_tools/git_operations/git_config_settings.hpp" +#include "src/utils/cpp/tmp_dir.hpp" extern "C" { #include @@ -392,7 +393,6 @@ auto GitRepoRemote::FetchFromRemote(std::shared_ptr cfg, } auto GitRepoRemote::UpdateCommitViaTmpRepo( - std::filesystem::path const& tmp_dir, std::string const& repo_url, std::string const& branch, std::vector const& inherit_env, @@ -401,6 +401,13 @@ auto GitRepoRemote::UpdateCommitViaTmpRepo( anon_logger_ptr const& logger) const noexcept -> std::optional { try { + auto tmp_dir = TmpDir::Create("update"); + if (not tmp_dir) { + (*logger)("Failed to create temp dir for running 'git ls-remote'", + /*fatal=*/true); + return std::nullopt; + } + auto const& tmp_path = tmp_dir->GetPath(); // check for internally supported protocols if (IsSupported(repo_url)) { // preferably with a "fake" repository! @@ -410,7 +417,7 @@ auto GitRepoRemote::UpdateCommitViaTmpRepo( } // create the temporary real repository auto tmp_repo = - GitRepoRemote::InitAndOpen(tmp_dir, /*is_bare=*/true); + GitRepoRemote::InitAndOpen(tmp_path / "git", /*is_bare=*/true); if (tmp_repo == std::nullopt) { return std::nullopt; } @@ -456,7 +463,7 @@ auto GitRepoRemote::UpdateCommitViaTmpRepo( system.Execute(cmdline, env, GetGitPath(), // which path is not actually relevant - tmp_dir); + tmp_path); if (not command_output) { (*logger)(fmt::format("exec() on command failed."), @@ -522,8 +529,7 @@ auto GitRepoRemote::UpdateCommitViaTmpRepo( } } -auto GitRepoRemote::FetchViaTmpRepo(std::filesystem::path const& tmp_dir, - std::string const& repo_url, +auto GitRepoRemote::FetchViaTmpRepo(std::string const& repo_url, std::optional const& branch, std::vector const& inherit_env, std::string const& git_bin, @@ -531,6 +537,13 @@ auto GitRepoRemote::FetchViaTmpRepo(std::filesystem::path const& tmp_dir, anon_logger_ptr const& logger) noexcept -> bool { try { + auto tmp_dir = TmpDir::Create("fetch"); + if (not tmp_dir) { + (*logger)("Failed to create temp dir for running 'git fetch'", + /*fatal=*/true); + return false; + } + auto const& tmp_path = tmp_dir->GetPath(); // check for internally supported protocols if (IsSupported(repo_url)) { // preferably with a "fake" repository! @@ -542,7 +555,7 @@ auto GitRepoRemote::FetchViaTmpRepo(std::filesystem::path const& tmp_dir, // it can be bare, as the refspecs for this fetch will be given // explicitly. auto tmp_repo = - GitRepoRemote::InitAndOpen(tmp_dir, /*is_bare=*/true); + GitRepoRemote::InitAndOpen(tmp_path / "git", /*is_bare=*/true); if (tmp_repo == std::nullopt) { return false; } @@ -606,7 +619,7 @@ auto GitRepoRemote::FetchViaTmpRepo(std::filesystem::path const& tmp_dir, // run command SystemCommand system{repo_url}; auto const command_output = - system.Execute(cmdline, env, GetGitPath(), tmp_dir); + system.Execute(cmdline, env, GetGitPath(), tmp_path); if (not command_output) { (*logger)(fmt::format("exec() on command failed."), diff --git a/src/other_tools/git_operations/git_repo_remote.hpp b/src/other_tools/git_operations/git_repo_remote.hpp index 614e7822..c59fb48e 100644 --- a/src/other_tools/git_operations/git_repo_remote.hpp +++ b/src/other_tools/git_operations/git_repo_remote.hpp @@ -82,15 +82,11 @@ class GitRepoRemote : public GitRepo { /// \brief Get commit from given branch on the remote. If URL is SSH, shells /// out to system git to perform an ls-remote call, ensuring correct /// handling of the remote connection settings (in particular proxy and - /// SSH). A temporary directory is needed to pipe the stdout and stderr to. - /// If URL is non-SSH, uses tmp dir to connect to remote and retrieve the - /// commit of a branch asynchronously using libgit2. - /// Caller needs to make sure the temporary directory exists and that the - /// given path is thread- and process-safe! + /// SSH). For non-SSH URLs, the branch commit is retrieved asynchronously + /// using libgit2. /// Returns the commit hash, as a string, or nullopt if failure. /// It guarantees the logger is called exactly once with fatal if failure. [[nodiscard]] auto UpdateCommitViaTmpRepo( - std::filesystem::path const& tmp_dir, std::string const& repo_url, std::string const& branch, std::vector const& inherit_env, @@ -101,16 +97,12 @@ class GitRepoRemote : public GitRepo { /// \brief Fetch from a remote. If URL is SSH, shells out to system git to /// retrieve packs in a safe manner, with the only side-effect being that - /// there can be some redundancy in the fetched packs. The tmp dir is used - /// to pipe the stdout and stderr to. - /// If URL is non-SSH, uses tmp dir to fetch asynchronously using libgit2. - /// Caller needs to make sure the temporary directory exists and that the - /// given path is thread- and process-safe! + /// there can be some redundancy in the fetched packs. + /// For non-SSH URLs an asynchronous fetch is performed using libgit2. /// Uses either a given branch, or fetches all (with base refspecs). /// Returns a success flag. /// It guarantees the logger is called exactly once with fatal if failure. [[nodiscard]] auto FetchViaTmpRepo( - std::filesystem::path const& tmp_dir, std::string const& repo_url, std::optional const& branch, std::vector const& inherit_env, -- cgit v1.2.3