From a312e71b59340f7b6d8dc5aac9202137ae81d02b Mon Sep 17 00:00:00 2001 From: Paul Cristian Sarbu Date: Tue, 13 Aug 2024 18:01:50 +0200 Subject: GitRepo: Change logic that creates commits to explicitly give directory In preparation for subsequent changes, specify the directory path containing the tree content to be committed explicitly. This change will allow eventually to be able to specify paths that are different from the root path of the repository in which the commit is created. This commit renames and refactors StageAndCommitAllAnnonymous to allow a directory path to be passed. The just-mr and serve service logic is updated such that current behaviour is otherwise unchanged. --- src/buildtool/file_system/git_repo.cpp | 46 +++++++++++++++------- src/buildtool/file_system/git_repo.hpp | 15 ++++--- .../serve_api/serve_service/source_tree.cpp | 2 +- src/other_tools/git_operations/git_operations.cpp | 5 ++- test/buildtool/file_system/git_repo.test.cpp | 8 ++-- 5 files changed, 48 insertions(+), 28 deletions(-) diff --git a/src/buildtool/file_system/git_repo.cpp b/src/buildtool/file_system/git_repo.cpp index d378a2ba..11c5a569 100644 --- a/src/buildtool/file_system/git_repo.cpp +++ b/src/buildtool/file_system/git_repo.cpp @@ -591,8 +591,9 @@ auto GitRepo::GetGitOdb() const noexcept return git_cas_->odb_; } -auto GitRepo::StageAndCommitAllAnonymous(std::string const& message, - anon_logger_ptr const& logger) noexcept +auto GitRepo::CommitDirectory(std::filesystem::path const& dir, + std::string const& message, + anon_logger_ptr const& logger) noexcept -> std::optional { #ifdef BOOTSTRAP_BUILD_TOOL return std::nullopt; @@ -600,7 +601,7 @@ auto GitRepo::StageAndCommitAllAnonymous(std::string const& message, try { // only possible for real repository! if (IsRepoFake()) { - (*logger)("cannot stage and commit files using a fake repository!", + (*logger)("cannot commit directory using a fake repository!", true /*fatal*/); return std::nullopt; } @@ -610,11 +611,23 @@ auto GitRepo::StageAndCommitAllAnonymous(std::string const& message, // cannot perform this operation on a bare repository; this has to be // checked because git_index_add_bypath will not do it for us! if (not FileSystemManager::Exists(GetGitPath() / ".git")) { - (*logger)("cannot stage and commit files in a bare repository!", + (*logger)("cannot commit directory in a bare repository!", true /*fatal*/); return std::nullopt; } + // the index approach to adding entries requires them to be reachable + // from root path + auto rel_path = dir.lexically_relative(GetGitPath()); + if (rel_path.empty() or rel_path.string().starts_with("..")) { + (*logger)( + fmt::format("unsupported directory {}: not a subpath of {}", + dir.string(), + GetGitPath().string()), + true /*fatal*/); + return std::nullopt; + } + // add all files to be staged git_index* index_ptr{nullptr}; git_repository_index(&index_ptr, repo_->Ptr()); @@ -624,19 +637,23 @@ auto GitRepo::StageAndCommitAllAnonymous(std::string const& message, // due to mismanagement of .gitignore rules by libgit2 when doing a // forced add all, we resort to using git_index_add_bypath manually for // all entries, instead of git_index_add_all with GIT_INDEX_ADD_FORCE. - auto use_entry = [&index](std::filesystem::path const& name, - bool is_tree) { + auto use_entry = [&index, rel_path](std::filesystem::path const& name, + bool is_tree) { return is_tree or - git_index_add_bypath(index.get(), name.c_str()) == 0; + git_index_add_bypath( + index.get(), + (rel_path / name) + .lexically_relative(".") // remove "." prefix + .c_str()) == 0; }; if (not FileSystemManager::ReadDirectoryEntriesRecursive( - GetGitPath(), + dir, use_entry, /*ignored_subdirs=*/{".git"})) { - (*logger)(fmt::format( - "staging files in git repository {} failed with:\n{}", - GetGitPath().string(), - GitLastError()), + (*logger)(fmt::format("staging entries in git repository {} failed " + "with:\n{}", + GetGitPath().string(), + GitLastError()), true /*fatal*/); return std::nullopt; } @@ -711,9 +728,8 @@ auto GitRepo::StageAndCommitAllAnonymous(std::string const& message, git_buf_dispose(&buffer); return commit_hash; // success! } catch (std::exception const& ex) { - Logger::Log(LogLevel::Error, - "stage and commit all failed with:\n{}", - ex.what()); + Logger::Log( + LogLevel::Error, "commit subdir failed with:\n{}", ex.what()); return std::nullopt; } #endif // BOOTSTRAP_BUILD_TOOL diff --git a/src/buildtool/file_system/git_repo.hpp b/src/buildtool/file_system/git_repo.hpp index 1e5ec839..31d9eb82 100644 --- a/src/buildtool/file_system/git_repo.hpp +++ b/src/buildtool/file_system/git_repo.hpp @@ -151,13 +151,16 @@ class GitRepo { using anon_logger_t = std::function; using anon_logger_ptr = std::shared_ptr; - /// \brief Stage all in current path and commit with given message. + /// \brief Create tree from entries at given directory and commit it with + /// given message. Currently, the caller must guarantee that given path is + /// a subdirectory of the repository root path. /// Only possible with real repository and thus non-thread-safe. - /// Returns the commit hash, or nullopt if failure. - /// It guarantees the logger is called exactly once with fatal if failure. - [[nodiscard]] auto StageAndCommitAllAnonymous( - std::string const& message, - anon_logger_ptr const& logger) noexcept -> std::optional; + /// \returns The commit hash, or nullopt if failure. It guarantees the + /// logger is called exactly once with fatal if failure. + [[nodiscard]] auto CommitDirectory(std::filesystem::path const& dir, + std::string const& message, + anon_logger_ptr const& logger) noexcept + -> std::optional; /// \brief Create annotated tag for given commit. /// Only possible with real repository and thus non-thread-safe. diff --git a/src/buildtool/serve_api/serve_service/source_tree.cpp b/src/buildtool/serve_api/serve_service/source_tree.cpp index 5de4c389..dd42ab83 100644 --- a/src/buildtool/serve_api/serve_service/source_tree.cpp +++ b/src/buildtool/serve_api/serve_service/source_tree.cpp @@ -470,7 +470,7 @@ auto SourceTreeService::CommonImportToGit( }); // stage and commit all auto commit_hash = - git_repo->StageAndCommitAllAnonymous(commit_message, wrapped_logger); + git_repo->CommitDirectory(root_path, commit_message, wrapped_logger); if (not commit_hash) { return unexpected{err}; } diff --git a/src/other_tools/git_operations/git_operations.cpp b/src/other_tools/git_operations/git_operations.cpp index 8f5b7ad7..bfd1d18f 100644 --- a/src/other_tools/git_operations/git_operations.cpp +++ b/src/other_tools/git_operations/git_operations.cpp @@ -42,8 +42,9 @@ auto CriticalGitOps::GitInitialCommit(GitOpParams const& crit_op_params, fatal); }); // Stage and commit all at the target location - auto commit_hash = git_repo->StageAndCommitAllAnonymous( - crit_op_params.message.value(), wrapped_logger); + auto commit_hash = git_repo->CommitDirectory(crit_op_params.target_path, + crit_op_params.message.value(), + wrapped_logger); if (commit_hash == std::nullopt) { return {.git_cas = nullptr, .result = std::nullopt}; } diff --git a/test/buildtool/file_system/git_repo.test.cpp b/test/buildtool/file_system/git_repo.test.cpp index 969f2072..5a0831cc 100644 --- a/test/buildtool/file_system/git_repo.test.cpp +++ b/test/buildtool/file_system/git_repo.test.cpp @@ -182,7 +182,7 @@ TEST_CASE("Single-threaded real repository local operations", "[git_repo]") { std::string(msg)); }); - SECTION("Stage and commit all") { + SECTION("Commit directory") { // make blank repo auto repo_commit_path = TestUtils::GetRepoPath(); auto repo_commit = @@ -196,9 +196,9 @@ TEST_CASE("Single-threaded real repository local operations", "[git_repo]") { REQUIRE(FileSystemManager::WriteFile( "test no 2", repo_commit_path / "test2.txt", true)); - // stage and commit all - auto commit = - repo_commit->StageAndCommitAllAnonymous("test commit", logger); + // commit subdir + auto commit = repo_commit->CommitDirectory( + repo_commit_path, "test commit", logger); CHECK(commit); } -- cgit v1.2.3