From ea11727577ffdbb03d386ceec7edf47dc7d4e215 Mon Sep 17 00:00:00 2001 From: Paul Cristian Sarbu Date: Fri, 5 Apr 2024 11:03:23 +0200 Subject: import_to_git_map: Fix wrong pointer in setter This bug went under the radar because the returned pointer is never explicitly used, just tested if set. As such, the correctness of just-mr was never actually afected by it. This commit fixes the issue and also cleans up small inconsistencies. --- src/other_tools/ops_maps/import_to_git_map.cpp | 42 +++++++++++++------------- src/other_tools/ops_maps/import_to_git_map.hpp | 4 ++- 2 files changed, 24 insertions(+), 22 deletions(-) (limited to 'src') diff --git a/src/other_tools/ops_maps/import_to_git_map.cpp b/src/other_tools/ops_maps/import_to_git_map.cpp index 991fc369..6aacbaf4 100644 --- a/src/other_tools/ops_maps/import_to_git_map.cpp +++ b/src/other_tools/ops_maps/import_to_git_map.cpp @@ -27,7 +27,7 @@ void KeepCommitAndSetTree( gsl::not_null const& critical_git_op_map, std::string const& commit, std::filesystem::path const& target_path, - GitCASPtr const& git_cas, + GitCASPtr const& just_git_cas, gsl::not_null const& ts, ImportToGitMap::SetterPtr const& setter, ImportToGitMap::LoggerPtr const& logger) { @@ -43,7 +43,8 @@ void KeepCommitAndSetTree( critical_git_op_map->ConsumeAfterKeysReady( ts, {std::move(op_key)}, - [commit, target_path, git_cas, setter, logger](auto const& values) { + [commit, target_path, just_git_cas, setter, logger]( + auto const& values) { GitOpValue op_result = *values[0]; // check flag if (not op_result.result) { @@ -51,8 +52,8 @@ void KeepCommitAndSetTree( /*fatal=*/true); return; } - auto git_repo = GitRepoRemote::Open(git_cas); - if (not git_repo) { + auto just_git_repo = GitRepoRemote::Open(just_git_cas); + if (not just_git_repo) { (*logger)(fmt::format("Could not open Git repository {}", target_path.string()), /*fatal=*/true); @@ -67,13 +68,13 @@ void KeepCommitAndSetTree( msg), fatal); }); - auto res = - git_repo->GetSubtreeFromCommit(commit, ".", wrapped_logger); + auto res = just_git_repo->GetSubtreeFromCommit( + commit, ".", wrapped_logger); if (not std::holds_alternative(res)) { return; } (*setter)(std::pair( - std::get(res), git_cas)); + std::get(res), just_git_cas)); }, [logger, commit, target_path](auto const& msg, bool fatal) { (*logger)(fmt::format("While running critical Git op KEEP_TAG for " @@ -98,7 +99,7 @@ auto CreateImportToGitMap( auto logger, auto /*unused*/, auto const& key) { - // Perform initial commit at location: init + add . + commit + // Perform initial commit at import location: init + add . + commit GitOpKey op_key = {.params = { key.target_path, // target_path @@ -144,7 +145,6 @@ auto CreateImportToGitMap( [critical_git_op_map, commit = *op_result.result, target_path, - git_cas = op_result.git_cas, git_bin, launcher, ts, @@ -161,27 +161,27 @@ auto CreateImportToGitMap( auto just_git_repo = GitRepoRemote::Open(op_result.git_cas); if (not just_git_repo) { - (*logger)(fmt::format("Could not open Git " - "repository {}", - target_path.string()), - /*fatal=*/true); + (*logger)( + fmt::format( + "Could not open Git cache repository {}", + StorageConfig::GitRoot().string()), + /*fatal=*/true); return; } auto wrapped_logger = std::make_shared( [logger, target_path](auto const& msg, bool fatal) { - (*logger)( - fmt::format("While fetch via tmp repo " - "for target {}:\n{}", - target_path.string(), - msg), - fatal); + (*logger)(fmt::format("While fetch via tmp " + "repo from {}:\n{}", + target_path.string(), + msg), + fatal); }); if (not just_git_repo->FetchViaTmpRepo( target_path.string(), std::nullopt, - std::vector{} /* XXX */, + std::vector{} /* inherit_env */, git_bin, launcher, wrapped_logger)) { @@ -202,7 +202,7 @@ auto CreateImportToGitMap( KeepCommitAndSetTree(critical_git_op_map, commit, target_path, - git_cas, + op_result.git_cas, /*just_git_cas*/ ts, setter, wrapped_logger); diff --git a/src/other_tools/ops_maps/import_to_git_map.hpp b/src/other_tools/ops_maps/import_to_git_map.hpp index d56c2d71..ce17d781 100644 --- a/src/other_tools/ops_maps/import_to_git_map.hpp +++ b/src/other_tools/ops_maps/import_to_git_map.hpp @@ -55,7 +55,9 @@ struct hash { } // namespace std /// \brief Maps a directory on the file system to a pair of the tree hash of the -/// content of the directory and the Git ODB it is now a part of. +/// content of the directory and the Git cache ODB (where the content will be). +/// The second entry is set for convenience for follow-up operations (to avoid +/// overheads of opening the Git cache), and is nullptr iff the map fails. using ImportToGitMap = AsyncMapConsumer>; -- cgit v1.2.3