From b29fb58bebaa2e5d692aee600ea8a2f5815fc8fe Mon Sep 17 00:00:00 2001 From: Johan Mabille Date: Wed, 18 Oct 2023 15:31:35 +0200 Subject: [PATCH] Additional cleanup and first pass according to review --- libmamba/src/core/curl.cpp | 32 ++------------------- libmamba/src/core/curl.hpp | 12 +------- libmamba/src/core/download.cpp | 12 ++++++-- libmamba/src/core/download_progress_bar.cpp | 8 +++--- libmamba/src/core/package_fetcher.cpp | 16 ++++------- libmamba/src/core/transaction.cpp | 2 +- 6 files changed, 23 insertions(+), 59 deletions(-) diff --git a/libmamba/src/core/curl.cpp b/libmamba/src/core/curl.cpp index 814b7e99d0..ab68a808e1 100644 --- a/libmamba/src/core/curl.cpp +++ b/libmamba/src/core/curl.cpp @@ -234,7 +234,6 @@ namespace mamba CURLHandle::CURLHandle() //(const Context& ctx) : m_handle(curl_easy_init()) - , m_result(CURLE_OK) { if (m_handle == nullptr) { @@ -253,7 +252,6 @@ namespace mamba rhs.m_handle = nullptr; rhs.p_headers = nullptr; std::swap(m_errorbuffer, rhs.m_errorbuffer); - std::swap(m_result, rhs.m_result); set_opt(CURLOPT_ERRORBUFFER, m_errorbuffer.data()); } @@ -261,7 +259,6 @@ namespace mamba { using std::swap; swap(m_handle, rhs.m_handle); - swap(m_result, rhs.m_result); swap(p_headers, rhs.p_headers); swap(m_errorbuffer, rhs.m_errorbuffer); set_opt(CURLOPT_ERRORBUFFER, m_errorbuffer.data()); @@ -459,34 +456,9 @@ namespace mamba return get_info(CURLINFO_EFFECTIVE_URL).value(); } - std::size_t CURLHandle::get_result() const + CURLcode CURLHandle::perform() { - return static_cast(m_result); - } - - bool CURLHandle::is_curl_res_ok() const - { - return is_curl_res_ok(m_result); - } - - void CURLHandle::set_result(CURLcode res) - { - m_result = res; - } - - std::string CURLHandle::get_res_error() const - { - return get_res_error(m_result); - } - - bool CURLHandle::can_proceed() - { - return can_retry(m_result); - } - - void CURLHandle::perform() - { - m_result = curl_easy_perform(m_handle); + return curl_easy_perform(m_handle); } CURLId CURLHandle::get_id() const diff --git a/libmamba/src/core/curl.hpp b/libmamba/src/core/curl.hpp index 666b02ef1b..9392d6cadf 100644 --- a/libmamba/src/core/curl.hpp +++ b/libmamba/src/core/curl.hpp @@ -140,16 +140,7 @@ namespace mamba const char* get_error_buffer() const; std::string get_curl_effective_url() const; - [[deprecated]] std::size_t get_result() const; - [[deprecated]] bool is_curl_res_ok() const; - - [[deprecated]] void set_result(CURLcode res); - - [[deprecated]] std::string get_res_error() const; - - // Side-effect programming, to remove - [[deprecated]] bool can_proceed(); - void perform(); + CURLcode perform(); CURLId get_id() const; @@ -161,7 +152,6 @@ namespace mamba private: CURL* m_handle; - CURLcode m_result; // Enum range from 0 to 99 curl_slist* p_headers = nullptr; std::array m_errorbuffer; diff --git a/libmamba/src/core/download.cpp b/libmamba/src/core/download.cpp index 1b6bb73540..2e4ae5cff7 100644 --- a/libmamba/src/core/download.cpp +++ b/libmamba/src/core/download.cpp @@ -91,7 +91,13 @@ namespace mamba } } - std::pair get_env_remote_params(const Context& context) + struct EnvRemoteParams + { + bool set_low_speed_opt = false; + bool set_ssl_no_revoke = false; + }; + + EnvRemoteParams get_env_remote_params(const Context& context) { // TODO: we should probably store set_low_speed_limit and set_ssl_no_revoke in // RemoteFetchParams if the request is slower than 30b/s for 60 seconds, cancel. @@ -249,7 +255,7 @@ namespace mamba void DownloadAttempt::configure_handle(const Context& context) { - auto [set_low_speed_opt, set_ssl_no_revoke] = get_env_remote_params(context); + const auto [set_low_speed_opt, set_ssl_no_revoke] = get_env_remote_params(context); m_handle.configure_handle( util::file_uri_unc2_to_unc4(p_request->url), @@ -874,7 +880,7 @@ namespace mamba init_remote_fetch_params(ctx.remote_fetch_params); } - auto [set_low_speed_opt, set_ssl_no_revoke] = get_env_remote_params(context); + const auto [set_low_speed_opt, set_ssl_no_revoke] = get_env_remote_params(context); return curl::check_resource_exists( util::file_uri_unc2_to_unc4(url), diff --git a/libmamba/src/core/download_progress_bar.cpp b/libmamba/src/core/download_progress_bar.cpp index 619faf8695..58aab04bba 100644 --- a/libmamba/src/core/download_progress_bar.cpp +++ b/libmamba/src/core/download_progress_bar.cpp @@ -18,7 +18,7 @@ namespace mamba const DownloadProgress& progress ) { - auto now = std::chrono::steady_clock::now(); + const auto now = std::chrono::steady_clock::now(); const auto throttle_treshold = std::chrono::milliseconds(50); if (now - throttle_time < throttle_treshold) @@ -283,7 +283,7 @@ namespace mamba init_aggregated_download(); init_aggregated_extract(); - auto& pbar_manager = dynamic_cast( + auto& pbar_manager = static_cast( Console::instance().progress_bar_manager() ); pbar_manager.start(); @@ -340,7 +340,7 @@ namespace mamba void PackageDownloadMonitor::init_aggregated_extract() { - auto& pbar_manager = dynamic_cast( + auto& pbar_manager = static_cast( Console::instance().progress_bar_manager() ); auto* extract_bar = pbar_manager.aggregated_bar("Extract"); @@ -383,7 +383,7 @@ namespace mamba void PackageDownloadMonitor::init_aggregated_download() { - auto& pbar_manager = dynamic_cast( + auto& pbar_manager = static_cast( Console::instance().progress_bar_manager() ); auto* dl_bar = pbar_manager.aggregated_bar("Download"); diff --git a/libmamba/src/core/package_fetcher.cpp b/libmamba/src/core/package_fetcher.cpp index 8fea5d6d44..3b937b3ab9 100644 --- a/libmamba/src/core/package_fetcher.cpp +++ b/libmamba/src/core/package_fetcher.cpp @@ -81,7 +81,7 @@ namespace mamba ) : m_package_info(pkg_info) { - // only do this for micromamba for now + // FIXME: only do this for micromamba for now if (channel_context.context().command_params.is_micromamba) { m_url = channel_context.make_channel(pkg_info.url).urls(true)[0]; @@ -91,10 +91,10 @@ namespace mamba m_url = pkg_info.url; } - fs::u8path extracted_cache = caches.get_extracted_dir_path(m_package_info); + const fs::u8path extracted_cache = caches.get_extracted_dir_path(m_package_info); if (extracted_cache.empty()) { - fs::u8path tarball_cache = caches.get_tarball_path(m_package_info); + const fs::u8path tarball_cache = caches.get_tarball_path(m_package_info); auto& cache = caches.first_writable_cache(true); m_cache_path = cache.path(); @@ -304,10 +304,7 @@ namespace mamba { fs::remove_all(m_tarball_path); fs::u8path dest_dir = strip_package_extension(m_tarball_path.string()); - if (fs::exists(dest_dir)) - { - fs::remove_all(dest_dir); - } + fs::remove_all(dest_dir); } /******************* @@ -349,7 +346,6 @@ namespace mamba << "\nExpected: " << expected_size() << "\nActual: " << downloaded_size << "\n"; Console::instance().print(filename() + " tarball has incorrect size"); - // TODO: terminate monitor } return res; } @@ -371,8 +367,8 @@ namespace mamba void PackageFetcher::write_repodata_record(const fs::u8path& base_path) const { - fs::u8path repodata_record_path = base_path / "info" / "repodata_record.json"; - fs::u8path index_path = base_path / "info" / "index.json"; + const fs::u8path repodata_record_path = base_path / "info" / "repodata_record.json"; + const fs::u8path index_path = base_path / "info" / "index.json"; nlohmann::json index, solvable_json; std::ifstream index_file = open_ifstream(index_path); diff --git a/libmamba/src/core/transaction.cpp b/libmamba/src/core/transaction.cpp index 74c517bd4c..b4ee8711c4 100644 --- a/libmamba/src/core/transaction.cpp +++ b/libmamba/src/core/transaction.cpp @@ -1051,7 +1051,7 @@ namespace mamba task.wait(); } - bool all_valid = check_all_valid(fetchers, extract_requests); + const bool all_valid = check_all_valid(fetchers, extract_requests); return !is_sig_interrupted() && all_valid; }