GitRepo: Create commit from a directory explicitly...

...by writing its tree directly in the object database instead of
working with the index. This allows the creation of trees that
contain also entries with 'magic' names, such as the .git folder
or .gitignore files.

Callers must ensure the given directory only contains the needed
entries. In particular, just-mr maps and serve service are updated
to separate the import-to-Git repository path from the temporary
path containing the content to be committed, to avoid polluting the
content path with entries generated on repository initialization.

5 files changed, 109 insertions, 104 deletions

diff --git a/src/buildtool/file_system/git_repo.cpp b/src/buildtool/file_system/git_repo.cpp
index 2e142438..5ab010d4 100644
--- a/src/buildtool/file_system/git_repo.cpp
+++ b/src/buildtool/file_system/git_repo.cpp
@@ -608,60 +608,26 @@ auto GitRepo::CommitDirectory(std::filesystem::path const& dir,
         // share the odb lock
         std::shared_lock lock{GetGitCAS()->mutex_};
 
-        // 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 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*/);
+        // Due to limitations of Git in general, and libgit2 in particular, by
+        // which updating the index with entries that have Git-specific magic
+        // names is cumbersome, if at all possible, we resort to creating
+        // manually the tree to be commited from the given subdirectory by
+        // recursively creating and writing to the object database all the blobs
+        // and subtrees.
+
+        // get tree containing the subdirectory entries
+        auto raw_id = CreateTreeFromDirectory(dir, logger);
+        if (not raw_id) {
             return std::nullopt;
         }
 
-        // add all files to be staged
-        git_index* index_ptr{nullptr};
-        git_repository_index(&index_ptr, repo_->Ptr());
-        auto index = std::unique_ptr<git_index, decltype(&index_closer)>(
-            index_ptr, index_closer);
-
-        // 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, rel_path](std::filesystem::path const& name,
-                                            bool is_tree) {
-            return is_tree or
-                   git_index_add_bypath(
-                       index.get(),
-                       (rel_path / name)
-                           .lexically_relative(".")  // remove "." prefix
-                           .c_str()) == 0;
-        };
-        if (not FileSystemManager::ReadDirectoryEntriesRecursive(
-                dir,
-                use_entry,
-                /*ignored_subdirs=*/{".git"})) {
-            (*logger)(fmt::format("staging entries in git repository {} failed "
-                                  "with:\n{}",
-                                  GetGitPath().string(),
-                                  GitLastError()),
-                      true /*fatal*/);
-            return std::nullopt;
-        }
-        // build tree from staged files
+        // get tree oid
         git_oid tree_oid;
-        if (git_index_write_tree(&tree_oid, index.get()) != 0) {
-            (*logger)(fmt::format("building tree from index in git repository "
-                                  "{} failed with:\n{}",
+        if (git_oid_fromraw(&tree_oid,
+                            reinterpret_cast<unsigned char const*>(  // NOLINT
+                                raw_id->data())) != 0) {
+            (*logger)(fmt::format("subdir tree object id parsing in git "
+                                  "repository {} failed with:\n{}",
                                   GetGitPath().string(),
                                   GitLastError()),
                       true /*fatal*/);
diff --git a/src/buildtool/file_system/git_repo.hpp b/src/buildtool/file_system/git_repo.hpp
index 2aabf7ee..4d1be53f 100644
--- a/src/buildtool/file_system/git_repo.hpp
+++ b/src/buildtool/file_system/git_repo.hpp
@@ -152,8 +152,9 @@ class GitRepo {
     using anon_logger_ptr = std::shared_ptr<anon_logger_t>;
 
     /// \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.
+    /// given message. The given path need not be a subdirectory of the
+    /// repository root path, but the caller must guarantee its entries are
+    /// readable.
     /// 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.
diff --git a/src/buildtool/serve_api/serve_service/source_tree.cpp b/src/buildtool/serve_api/serve_service/source_tree.cpp
index dd42ab83..aaa0256e 100644
--- a/src/buildtool/serve_api/serve_service/source_tree.cpp
+++ b/src/buildtool/serve_api/serve_service/source_tree.cpp
@@ -448,29 +448,38 @@ auto SourceTreeService::ResolveContentTree(
 }
 
 auto SourceTreeService::CommonImportToGit(
-    std::filesystem::path const& root_path,
+    std::filesystem::path const& content_path,
     std::string const& commit_message) -> expected<std::string, std::string> {
+    // the repository path that imports the content must be separate from the
+    // content path, to avoid polluting the entries
+    auto tmp_dir = storage_config_.CreateTypedTmpDir("import-repo");
+    if (not tmp_dir) {
+        return unexpected{
+            std::string("Failed to create tmp path for import repository")};
+    }
+    auto const& repo_path = tmp_dir->GetPath();
     // do the initial commit; no need to guard, as the tmp location is unique
-    auto git_repo = GitRepo::InitAndOpen(root_path,
+    auto git_repo = GitRepo::InitAndOpen(repo_path,
                                          /*is_bare=*/false);
     if (not git_repo) {
         return unexpected{fmt::format("Could not initialize repository {}",
-                                      root_path.string())};
+                                      repo_path.string())};
     }
     // wrap logger for GitRepo call
     std::string err;
     auto wrapped_logger = std::make_shared<GitRepo::anon_logger_t>(
-        [root_path, &err](auto const& msg, bool fatal) {
+        [content_path, repo_path, &err](auto const& msg, bool fatal) {
             if (fatal) {
                 err = fmt::format(
-                    "While staging and committing all in repository {}:\n{}",
-                    root_path.string(),
+                    "While committing directory {} in repository {}:\n{}",
+                    content_path.string(),
+                    repo_path.string(),
                     msg);
             }
         });
     // stage and commit all
     auto commit_hash =
-        git_repo->CommitDirectory(root_path, commit_message, wrapped_logger);
+        git_repo->CommitDirectory(content_path, commit_message, wrapped_logger);
     if (not commit_hash) {
         return unexpected{err};
     }
@@ -498,7 +507,7 @@ auto SourceTreeService::CommonImportToGit(
     // fetch the new commit into the Git CAS via tmp directory; the call is
     // thread-safe, so it needs no guarding
     if (not just_git_repo->LocalFetchViaTmpRepo(storage_config_,
-                                                root_path.string(),
+                                                repo_path.string(),
                                                 /*branch=*/std::nullopt,
                                                 wrapped_logger)) {
         return unexpected{err};
diff --git a/src/other_tools/ops_maps/git_tree_fetch_map.cpp b/src/other_tools/ops_maps/git_tree_fetch_map.cpp
index 7a4d9e0a..95582556 100644
--- a/src/other_tools/ops_maps/git_tree_fetch_map.cpp
+++ b/src/other_tools/ops_maps/git_tree_fetch_map.cpp
@@ -413,8 +413,9 @@ auto CreateGitTreeFetchMap(
                     return;
                 }
                 // create temporary location for command execution root
-                auto tmp_dir = storage_config->CreateTypedTmpDir("git-tree");
-                if (not tmp_dir) {
+                auto content_dir =
+                    storage_config->CreateTypedTmpDir("git-tree");
+                if (not content_dir) {
                     (*logger)(
                         "Failed to create execution root tmp directory for "
                         "tree id map!",
@@ -444,28 +445,39 @@ auto CreateGitTreeFetchMap(
                     }
                 }
                 auto const exit_code = system.Execute(
-                    cmdline, env, tmp_dir->GetPath(), out_dir->GetPath());
+                    cmdline, env, content_dir->GetPath(), out_dir->GetPath());
                 if (not exit_code) {
                     (*logger)(fmt::format("Failed to execute command:\n{}",
                                           nlohmann::json(cmdline).dump()),
                               /*fatal=*/true);
                     return;
                 }
+                // create temporary location for the import repository
+                auto repo_dir =
+                    storage_config->CreateTypedTmpDir("import-repo");
+                if (not repo_dir) {
+                    (*logger)(
+                        "Failed to create tmp directory for import repository",
+                        /*fatal=*/true);
+                    return;
+                }
                 // do an import to git with tree check
-                GitOpKey op_key = {.params =
-                                       {
-                                           tmp_dir->GetPath(),  // target_path
-                                           "",                  // git_hash
-                                           fmt::format("Content of tree {}",
-                                                       key.hash),  // message
-                                           tmp_dir->GetPath()  // source_path
-                                       },
-                                   .op_type = GitOpType::INITIAL_COMMIT};
+                GitOpKey op_key = {
+                    .params =
+                        {
+                            repo_dir->GetPath(),  // target_path
+                            "",                   // git_hash
+                            fmt::format("Content of tree {}",
+                                        key.hash),  // message
+                            content_dir->GetPath()  // source_path
+                        },
+                    .op_type = GitOpType::INITIAL_COMMIT};
                 critical_git_op_map->ConsumeAfterKeysReady(
                     ts,
                     {std::move(op_key)},
-                    [tmp_dir,  // keep tmp_dir alive
-                     out_dir,  // keep stdout/stderr of command alive
+                    [repo_dir,     // keep repo_dir alive
+                     content_dir,  // keep content_dir alive
+                     out_dir,      // keep stdout/stderr of command alive
                      critical_git_op_map,
                      just_git_cas = op_result.git_cas,
                      cmdline,
@@ -492,7 +504,7 @@ auto CreateGitTreeFetchMap(
                         if (not git_repo) {
                             (*logger)(
                                 fmt::format("Could not open repository {}",
-                                            tmp_dir->GetPath().string()),
+                                            repo_dir->GetPath().string()),
                                 /*fatal=*/true);
                             return;
                         }
@@ -541,14 +553,14 @@ auto CreateGitTreeFetchMap(
                                 /*fatal=*/true);
                             return;
                         }
-                        auto target_path = tmp_dir->GetPath();
+                        auto target_path = repo_dir->GetPath();
                         // fetch all into Git cache
                         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);
+                            (*logger)(
+                                fmt::format("Could not open Git repository {}",
+                                            storage_config->GitRoot().string()),
+                                /*fatal=*/true);
                             return;
                         }
                         // define temp repo path
@@ -634,21 +646,28 @@ auto CreateGitTreeFetchMap(
                                 // success
                                 (*setter)(false /*no cache hit*/);
                             },
-                            [logger, commit = *op_result.result](
+                            [logger,
+                             commit = *op_result.result,
+                             target_path = storage_config->GitRoot()](
                                 auto const& msg, bool fatal) {
                                 (*logger)(
                                     fmt::format("While running critical Git op "
-                                                "KEEP_TAG for commit {}:\n{}",
+                                                "KEEP_TAG for commit {} in "
+                                                "repository {}:\n{}",
                                                 commit,
+                                                target_path.string(),
                                                 msg),
                                     fatal);
                             });
                     },
-                    [logger](auto const& msg, bool fatal) {
-                        (*logger)(fmt::format("While running critical Git op "
-                                              "INITIAL_COMMIT:\n{}",
-                                              msg),
-                                  fatal);
+                    [logger, target_path = repo_dir->GetPath()](auto const& msg,
+                                                                bool fatal) {
+                        (*logger)(
+                            fmt::format("While running critical Git op "
+                                        "INITIAL_COMMIT for target {}:\n{}",
+                                        target_path.string(),
+                                        msg),
+                            fatal);
                     });
             },
             [logger, target_path = storage_config->GitRoot()](auto const& msg,
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 e883e0dd..02da0475 100644
--- a/src/other_tools/ops_maps/import_to_git_map.cpp
+++ b/src/other_tools/ops_maps/import_to_git_map.cpp
@@ -25,7 +25,6 @@ namespace {
 void KeepCommitAndSetTree(
     gsl::not_null<CriticalGitOpMap*> const& critical_git_op_map,
     std::string const& commit,
-    std::filesystem::path const& target_path,
     GitCASPtr const& just_git_cas,
     StorageConfig const& storage_config,
     gsl::not_null<TaskSystem*> const& ts,
@@ -42,7 +41,7 @@ void KeepCommitAndSetTree(
     critical_git_op_map->ConsumeAfterKeysReady(
         ts,
         {std::move(op_key)},
-        [commit, target_path, just_git_cas, setter, logger](
+        [commit, just_git_cas, storage_config, setter, logger](
             auto const& values) {
             GitOpValue op_result = *values[0];
             // check flag
@@ -54,7 +53,7 @@ void KeepCommitAndSetTree(
             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()),
+                                      storage_config.GitRoot().string()),
                           /*fatal=*/true);
                 return;
             }
@@ -75,7 +74,8 @@ void KeepCommitAndSetTree(
             (*setter)(std::pair<std::string, GitCASPtr>(*std::move(res),
                                                         just_git_cas));
         },
-        [logger, commit, target_path](auto const& msg, bool fatal) {
+        [logger, commit, target_path = storage_config.GitRoot()](
+            auto const& msg, bool fatal) {
             (*logger)(fmt::format("While running critical Git op KEEP_TAG for "
                                   "commit {} in target {}:\n{}",
                                   commit,
@@ -101,11 +101,21 @@ auto CreateImportToGitMap(
                                           auto logger,
                                           auto /*unused*/,
                                           auto const& key) {
-        // Perform initial commit at import location: init + add . + commit
+        // The repository path that imports the content must be separate from
+        // the content path, to avoid polluting the entries
+        auto repo_dir = storage_config->CreateTypedTmpDir("import-repo");
+        if (not repo_dir) {
+            (*logger)(fmt::format("Failed to create import repository tmp "
+                                  "directory for target {}",
+                                  key.target_path.string()),
+                      true);
+            return;
+        }
+        // Commit content from target_path via the tmp repository
         GitOpKey op_key = {.params =
                                {
-                                   key.target_path,  // target_path
-                                   "",               // git_hash
+                                   repo_dir->GetPath(),  // target_path
+                                   "",                   // git_hash
                                    fmt::format("Content of {} {}",
                                                key.repo_type,
                                                key.content),  // message
@@ -115,8 +125,8 @@ auto CreateImportToGitMap(
         critical_git_op_map->ConsumeAfterKeysReady(
             ts,
             {std::move(op_key)},
-            [critical_git_op_map,
-             target_path = key.target_path,
+            [repo_dir,  // keep repo_dir alive
+             critical_git_op_map,
              git_bin,
              launcher,
              storage_config,
@@ -145,9 +155,9 @@ auto CreateImportToGitMap(
                 critical_git_op_map->ConsumeAfterKeysReady(
                     ts,
                     {std::move(op_key)},
-                    [critical_git_op_map,
+                    [repo_dir,  // keep repo_dir alive
+                     critical_git_op_map,
                      commit = *op_result.result,
-                     target_path,
                      git_bin,
                      launcher,
                      storage_config,
@@ -172,6 +182,7 @@ auto CreateImportToGitMap(
                                 /*fatal=*/true);
                             return;
                         }
+                        auto const& target_path = repo_dir->GetPath();
                         auto wrapped_logger =
                             std::make_shared<AsyncMapConsumerLogger>(
                                 [logger, target_path](auto const& msg,
@@ -198,16 +209,15 @@ auto CreateImportToGitMap(
                                 [logger, target_path](auto const& msg,
                                                       bool fatal) {
                                     (*logger)(
-                                        fmt::format(
-                                            "While doing keep commit "
-                                            "and setting Git tree {}:\n{}",
-                                            target_path.string(),
-                                            msg),
+                                        fmt::format("While doing keep commit "
+                                                    "and setting Git tree for "
+                                                    "target {}:\n{}",
+                                                    target_path.string(),
+                                                    msg),
                                         fatal);
                                 });
                         KeepCommitAndSetTree(critical_git_op_map,
                                              commit,
-                                             target_path,
                                              op_result.git_cas, /*just_git_cas*/
                                              *storage_config,
                                              ts,