GitRepo: Change FetchFromRemote to fetch based on branch name

This also removes the need to call the GET_BRANCH_REFNAME critical
operation.

5 files changed, 99 insertions, 149 deletions

diff --git a/src/buildtool/file_system/git_repo.cpp b/src/buildtool/file_system/git_repo.cpp
index 7a4915a3..a3d7e388 100644
--- a/src/buildtool/file_system/git_repo.cpp
+++ b/src/buildtool/file_system/git_repo.cpp
@@ -844,7 +844,7 @@ auto GitRepo::GetCommitFromRemote(std::string const& repo_url,
 }
 
 auto GitRepo::FetchFromRemote(std::string const& repo_url,
-                              std::string const& refspec,
+                              std::optional<std::string> const& branch,
                               anon_logger_ptr const& logger) noexcept -> bool {
 #ifdef BOOTSTRAP_BUILD_TOOL
     return false;
@@ -870,29 +870,36 @@ auto GitRepo::FetchFromRemote(std::string const& repo_url,
             git_remote_free(remote_ptr);
             return false;
         }
+        // wrap remote object
         auto remote = std::unique_ptr<git_remote, decltype(&remote_closer)>(
             remote_ptr, remote_closer);
 
+        // define default fetch options
+        git_fetch_options fetch_opts{};
+        git_fetch_options_init(&fetch_opts, GIT_FETCH_OPTIONS_VERSION);
+
+        // disable update of the FETCH_HEAD pointer
+        fetch_opts.update_fetchhead = 0;
+
         // setup fetch refspecs array
         git_strarray refspecs_array_obj{};
-        if (not refspec.empty()) {
-            PopulateStrarray(&refspecs_array_obj, {refspec});
+        if (branch) {
+            // make sure we check for tags as well
+            std::string tag = fmt::format("+refs/tags/{}", *branch);
+            std::string head = fmt::format("+refs/heads/{}", *branch);
+            PopulateStrarray(&refspecs_array_obj, {tag, head});
         }
         auto refspecs_array =
             std::unique_ptr<git_strarray, decltype(&strarray_deleter)>(
                 &refspecs_array_obj, strarray_deleter);
 
-        // do the fetch
-        git_fetch_options fetch_opts{};
-        git_fetch_init_options(&fetch_opts, GIT_FETCH_OPTIONS_VERSION);
-        if (git_remote_fetch(remote.get(),
-                             refspec.empty() ? nullptr : refspecs_array.get(),
-                             &fetch_opts,
-                             nullptr) != 0) {
+        if (git_remote_fetch(
+                remote.get(), refspecs_array.get(), &fetch_opts, nullptr) !=
+            0) {
             (*logger)(
-                fmt::format("fetch of refspec {} in git repository {} failed "
+                fmt::format("fetching{} in git repository {} failed "
                             "with:\n{}",
-                            refspec,
+                            branch ? fmt::format(" branch {}", *branch) : "",
                             GetGitCAS()->git_path_.string(),
                             GitLastError()),
                 true /*fatal*/);
@@ -1201,7 +1208,7 @@ auto GitRepo::UpdateCommitViaTmpRepo(std::filesystem::path const& tmp_repo_path,
 
 auto GitRepo::FetchViaTmpRepo(std::filesystem::path const& tmp_repo_path,
                               std::string const& repo_url,
-                              std::string const& refspec,
+                              std::optional<std::string> const& branch,
                               anon_logger_ptr const& logger) noexcept -> bool {
 #ifdef BOOTSTRAP_BUILD_TOOL
     return false;
@@ -1234,7 +1241,7 @@ auto GitRepo::FetchViaTmpRepo(std::filesystem::path const& tmp_repo_path,
                             "While doing branch fetch via tmp repo:\n{}", msg),
                         fatal);
                 });
-            return tmp_repo->FetchFromRemote(repo_url, refspec, wrapped_logger);
+            return tmp_repo->FetchFromRemote(repo_url, branch, wrapped_logger);
         }
         return false;
     } catch (std::exception const& ex) {
diff --git a/src/buildtool/file_system/git_repo.hpp b/src/buildtool/file_system/git_repo.hpp
index fbb07aec..3fda0516 100644
--- a/src/buildtool/file_system/git_repo.hpp
+++ b/src/buildtool/file_system/git_repo.hpp
@@ -155,14 +155,13 @@ class GitRepo {
         std::string const& branch_refname_local,
         anon_logger_ptr const& logger) noexcept -> std::optional<std::string>;
 
-    /// \brief Fetch from given remote using refspec (usually for a branch).
+    /// \brief Fetch from given remote. It can either fetch a given named
+    /// branch, or it can fetch with base refspecs.
     /// Only possible with real repository and thus non-thread-safe.
-    /// If the refspec string in empty, performs a fetch of all branches with
-    /// default refspecs.
     /// Returns a success flag.
     /// It guarantees the logger is called exactly once with fatal if failure.
     [[nodiscard]] auto FetchFromRemote(std::string const& repo_url,
-                                       std::string const& refspec,
+                                       std::optional<std::string> const& branch,
                                        anon_logger_ptr const& logger) noexcept
         -> bool;
 
@@ -225,13 +224,13 @@ class GitRepo {
     /// custom backend to redirect the fetched objects into the desired odb.
     /// Caller needs to make sure the temporary directory exists and that the
     /// given path is thread- and process-safe!
-    /// Uses either a given branch refspec, or fetches all (if refspec empty).
+    /// Uses either a given branch, or fetches using 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_repo_path,
         std::string const& repo_url,
-        std::string const& refspec,
+        std::optional<std::string> const& branch,
         anon_logger_ptr const& logger) noexcept -> bool;
 
     /// \brief Try to retrieve the root of the repository containing the
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 05be1c8b..137a719b 100644
--- a/src/other_tools/ops_maps/import_to_git_map.cpp
+++ b/src/other_tools/ops_maps/import_to_git_map.cpp
@@ -113,7 +113,7 @@ auto CreateImportToGitMap(
                         auto success =
                             just_git_repo->FetchViaTmpRepo(*tmp_repo_path,
                                                            target_path.string(),
-                                                           "",
+                                                           std::nullopt,
                                                            wrapped_logger);
                         if (not success) {
                             return;
diff --git a/src/other_tools/root_maps/commit_git_map.cpp b/src/other_tools/root_maps/commit_git_map.cpp
index 8e34be91..86ab3447 100644
--- a/src/other_tools/root_maps/commit_git_map.cpp
+++ b/src/other_tools/root_maps/commit_git_map.cpp
@@ -131,27 +131,63 @@ void EnsureCommit(GitRepoInfo const& repo_info,
     }
     if (not is_commit_present.value()) {
         // if commit not there, fetch it
-        // get refspec for branch
+        auto tmp_dir = JustMR::Utils::CreateTypedTmpDir("fetch");
+        if (not tmp_dir) {
+            (*logger)("Failed to create fetch tmp directory!",
+                      /*fatal=*/true);
+            return;
+        }
+        // setup wrapped logger
+        auto wrapped_logger = std::make_shared<AsyncMapConsumerLogger>(
+            [logger](auto const& msg, bool fatal) {
+                (*logger)(fmt::format("While fetching via tmp repo:\n{}", msg),
+                          fatal);
+            });
+        if (not git_repo->FetchViaTmpRepo(tmp_dir->GetPath(),
+                                          repo_info.repo_url,
+                                          repo_info.branch,
+                                          wrapped_logger)) {
+            return;
+        }
+        // setup wrapped logger
+        wrapped_logger = std::make_shared<AsyncMapConsumerLogger>(
+            [logger](auto const& msg, bool fatal) {
+                (*logger)(fmt::format("While checking commit exists:\n{}", msg),
+                          fatal);
+            });
+        // check if commit exists now, after fetch
+        auto is_commit_present =
+            git_repo->CheckCommitExists(repo_info.hash, wrapped_logger);
+        if (not is_commit_present) {
+            return;
+        }
+        if (not *is_commit_present) {
+            // commit could not be fetched, so fail
+            (*logger)(fmt::format("Could not fetch commit {} from branch "
+                                  "{} for remote {}",
+                                  repo_info.hash,
+                                  repo_info.branch,
+                                  repo_info.repo_url),
+                      /*fatal=*/true);
+            return;
+        }
+        // keep tag
         GitOpKey op_key = {{
-                               repo_root,         // target_path
-                               "",                // git_hash
-                               repo_info.branch,  // branch
+                               repo_root,                    // target_path
+                               repo_info.hash,               // git_hash
+                               "",                           // branch
+                               "Keep referenced tree alive"  // message
                            },
-                           GitOpType::GET_BRANCH_REFNAME};
+                           GitOpType::KEEP_TAG};
         critical_git_op_map->ConsumeAfterKeysReady(
             ts,
             {std::move(op_key)},
-            [critical_git_op_map,
-             git_cas,
-             repo_info,
-             repo_root,
-             ts,
-             ws_setter,
-             logger](auto const& values) {
+            [git_cas, repo_info, repo_root, ws_setter, logger](
+                auto const& values) {
                 GitOpValue op_result = *values[0];
                 // check flag
                 if (not op_result.result) {
-                    (*logger)("Get branch refname failed",
+                    (*logger)("Keep tag failed",
                               /*fatal=*/true);
                     return;
                 }
@@ -164,112 +200,27 @@ void EnsureCommit(GitRepoInfo const& repo_info,
                               /*fatal=*/true);
                     return;
                 }
-                // do fetch
-                auto tmp_dir = JustMR::Utils::CreateTypedTmpDir("fetch");
-                if (not tmp_dir) {
-                    (*logger)("Failed to create fetch tmp directory!",
-                              /*fatal=*/true);
-                    return;
-                }
                 // setup wrapped logger
                 auto wrapped_logger = std::make_shared<AsyncMapConsumerLogger>(
                     [logger](auto const& msg, bool fatal) {
-                        (*logger)(fmt::format(
-                                      "While fetching via tmp repo:\n{}", msg),
-                                  fatal);
-                    });
-                if (not git_repo->FetchViaTmpRepo(tmp_dir->GetPath(),
-                                                  repo_info.repo_url,
-                                                  *op_result.result,
-                                                  wrapped_logger)) {
-                    return;
-                }
-                // setup wrapped logger
-                wrapped_logger = std::make_shared<AsyncMapConsumerLogger>(
-                    [logger](auto const& msg, bool fatal) {
-                        (*logger)(fmt::format(
-                                      "While checking commit exists:\n{}", msg),
+                        (*logger)(fmt::format("While getting subtree "
+                                              "from commit:\n{}",
+                                              msg),
                                   fatal);
                     });
-                // check if commit exists now, after fetch
-                auto is_commit_present =
-                    git_repo->CheckCommitExists(repo_info.hash, wrapped_logger);
-                if (not is_commit_present) {
-                    return;
-                }
-                if (not *is_commit_present) {
-                    // commit could not be fetched, so fail
-                    (*logger)(
-                        fmt::format("Could not fetch commit {} from branch "
-                                    "{} for remote {}",
-                                    repo_info.hash,
-                                    repo_info.branch,
-                                    repo_info.repo_url),
-                        /*fatal=*/true);
+                // get tree id and return workspace root
+                auto subtree = git_repo->GetSubtreeFromCommit(
+                    repo_info.hash, repo_info.subdir, wrapped_logger);
+                if (not subtree) {
                     return;
                 }
-                // keep tag
-                GitOpKey op_key = {{
-                                       repo_root,       // target_path
-                                       repo_info.hash,  // git_hash
-                                       "",              // branch
-                                       "Keep referenced tree alive"  // message
-                                   },
-                                   GitOpType::KEEP_TAG};
-                critical_git_op_map->ConsumeAfterKeysReady(
-                    ts,
-                    {std::move(op_key)},
-                    [git_cas, repo_info, repo_root, ws_setter, logger](
-                        auto const& values) {
-                        GitOpValue op_result = *values[0];
-                        // check flag
-                        if (not op_result.result) {
-                            (*logger)("Keep tag failed",
-                                      /*fatal=*/true);
-                            return;
-                        }
-                        // ensure commit exists, and fetch if needed
-                        auto git_repo =
-                            GitRepo::Open(git_cas);  // link fake repo to odb
-                        if (not git_repo) {
-                            (*logger)(
-                                fmt::format("Could not open repository {}",
-                                            repo_root.string()),
-                                /*fatal=*/true);
-                            return;
-                        }
-                        // setup wrapped logger
-                        auto wrapped_logger =
-                            std::make_shared<AsyncMapConsumerLogger>(
-                                [logger](auto const& msg, bool fatal) {
-                                    (*logger)(
-                                        fmt::format("While getting subtree "
-                                                    "from commit:\n{}",
-                                                    msg),
-                                        fatal);
-                                });
-                        // get tree id and return workspace root
-                        auto subtree = git_repo->GetSubtreeFromCommit(
-                            repo_info.hash, repo_info.subdir, wrapped_logger);
-                        if (not subtree) {
-                            return;
-                        }
-                        // set the workspace root
-                        (*ws_setter)(nlohmann::json::array(
-                            {"git tree", *subtree, repo_root}));
-                    },
-                    [logger, target_path = repo_root](auto const& msg,
-                                                      bool fatal) {
-                        (*logger)(fmt::format("While running critical Git op "
-                                              "KEEP_TAG for target {}:\n{}",
-                                              target_path.string(),
-                                              msg),
-                                  fatal);
-                    });
+                // set the workspace root
+                (*ws_setter)(
+                    nlohmann::json::array({"git tree", *subtree, repo_root}));
             },
             [logger, target_path = repo_root](auto const& msg, bool fatal) {
                 (*logger)(fmt::format("While running critical Git op "
-                                      "GET_BRANCH_REFNAME for target {}:\n{}",
+                                      "KEEP_TAG for target {}:\n{}",
                                       target_path.string(),
                                       msg),
                           fatal);
diff --git a/test/buildtool/file_system/git_repo.test.cpp b/test/buildtool/file_system/git_repo.test.cpp
index 0d20b379..d62ec2f1 100644
--- a/test/buildtool/file_system/git_repo.test.cpp
+++ b/test/buildtool/file_system/git_repo.test.cpp
@@ -236,14 +236,15 @@ TEST_CASE("Single-threaded real repository remote operations", "[git_repo]") {
         CHECK(*remote_commit == kRootCommit);
     }
 
-    SECTION("Fetch all from remote") {
+    SECTION("Fetch with base refspecs from remote") {
         // make bare real repo to fetch into
         auto path_fetch_all_bare = TestUtils::CreateTestRepoWithCheckout();
         REQUIRE(path_fetch_all_bare);
         auto repo_fetch_all_bare = GitRepo::Open(*path_fetch_all_bare);
 
         // fetch
-        CHECK(repo_fetch_all_bare->FetchFromRemote(*repo_path, "", logger));
+        CHECK(repo_fetch_all_bare->FetchFromRemote(
+            *repo_path, std::nullopt, logger));
     }
 
     SECTION("Fetch branch from remote") {
@@ -253,15 +254,9 @@ TEST_CASE("Single-threaded real repository remote operations", "[git_repo]") {
         auto repo_fetch_branch_bare = GitRepo::Open(*path_fetch_branch_bare);
         REQUIRE(repo_fetch_branch_bare);
 
-        // get refspec of branch
-        auto branch_refname =
-            repo_fetch_branch_bare->GetBranchLocalRefname("master", logger);
-        REQUIRE(branch_refname);
-        auto branch_refspec = std::string("+") + *branch_refname;
-        REQUIRE(branch_refspec == "+refs/heads/master");
         // fetch
         CHECK(repo_fetch_branch_bare->FetchFromRemote(
-            *repo_path, branch_refspec, logger));
+            *repo_path, "master", logger));
     }
 }
 
@@ -432,9 +427,9 @@ TEST_CASE("Single-threaded fake repository operations", "[git_repo]") {
 
             // create path for tmp repo to use for fetch
             auto tmp_path_fetch_all = TestUtils::GetRepoPath();
-            // fetch all
+            // fetch with base refspecs
             REQUIRE(repo_fetch_all->FetchViaTmpRepo(
-                tmp_path_fetch_all, *repo_path, "", logger));
+                tmp_path_fetch_all, *repo_path, std::nullopt, logger));
 
             // check commit is there after fetch
             CHECK(*repo_fetch_all->CheckCommitExists(kRootCommit, logger));
@@ -454,11 +449,8 @@ TEST_CASE("Single-threaded fake repository operations", "[git_repo]") {
             // create path for tmp repo to use for fetch
             auto tmp_path_fetch_wRefspec = TestUtils::GetRepoPath();
             // fetch all
-            REQUIRE(
-                repo_fetch_wRefspec->FetchViaTmpRepo(tmp_path_fetch_wRefspec,
-                                                     *repo_path,
-                                                     "+refs/heads/master",
-                                                     logger));
+            REQUIRE(repo_fetch_wRefspec->FetchViaTmpRepo(
+                tmp_path_fetch_wRefspec, *repo_path, "master", logger));
 
             // check commit is there after fetch
             CHECK(*repo_fetch_wRefspec->CheckCommitExists(kRootCommit, logger));
@@ -597,21 +589,22 @@ TEST_CASE("Multi-threaded fake repository operations", "[git_repo]") {
                         case 1: {
                             // create path for tmp repo to use for fetch
                             auto tmp_path_fetch_all = TestUtils::GetRepoPath();
+                            // fetch with base refspecs
                             CHECK(
                                 target_repo->FetchViaTmpRepo(tmp_path_fetch_all,
                                                              *remote_repo_path,
-                                                             "",
+                                                             std::nullopt,
                                                              logger));
                         } break;
                         case 2: {
                             // create path for tmp repo to use for fetch
                             auto tmp_path_fetch_wRefspec =
                                 TestUtils::GetRepoPath();
-                            // fetch all
+                            // fetch specific branch
                             CHECK(target_repo->FetchViaTmpRepo(
                                 tmp_path_fetch_wRefspec,
                                 *remote_repo_path,
-                                "+refs/heads/master",
+                                "master",
                                 logger));
                         } break;
                         case 3: {