Use expected to return an error from ParseRoot

3 files changed, 48 insertions, 59 deletions

diff --git a/src/buildtool/file_system/file_root.hpp b/src/buildtool/file_system/file_root.hpp
index adbc3352..4e65e7fe 100644
--- a/src/buildtool/file_system/file_root.hpp
+++ b/src/buildtool/file_system/file_root.hpp
@@ -712,31 +712,30 @@ class FileRoot {
     /// nullopt on errors.
     [[nodiscard]] static auto ParseRoot(std::string const& repo,
                                         std::string const& keyword,
-                                        nlohmann::json const& root,
-                                        gsl::not_null<std::string*> error_msg)
-        -> std::optional<
-            std::pair<FileRoot, std::optional<std::filesystem::path>>> {
+                                        nlohmann::json const& root)
+        -> expected<std::pair<FileRoot, std::optional<std::filesystem::path>>,
+                    std::string> {
+        using ResultType =
+            std::pair<FileRoot, std::optional<std::filesystem::path>>;
         if ((not root.is_array()) or root.empty()) {
-            *error_msg = fmt::format(
+            return unexpected{fmt::format(
                 "Expected {} for {} to be of the form [<scheme>, ...], but "
                 "found {}",
                 keyword,
                 repo,
-                root.dump());
-            return std::nullopt;
+                root.dump())};
         }
         if (root[0] == "file") {
             if (root.size() != 2 or (not root[1].is_string())) {
-                *error_msg = fmt::format(
+                return unexpected{fmt::format(
                     "\"file\" scheme expects precisely one string argument, "
                     "but found {} for {} of repository {}",
                     root.dump(),
                     keyword,
-                    repo);
-                return std::nullopt;
+                    repo)};
             }
             auto path = std::filesystem::path{root[1].get<std::string>()};
-            return std::pair(FileRoot{path}, std::move(path));
+            return ResultType{FileRoot{path}, std::move(path)};
         }
         if (root[0] == FileRoot::kGitTreeMarker) {
             bool const has_one_arg = root.size() == 2 and root[1].is_string();
@@ -744,43 +743,40 @@ class FileRoot {
                                       root[1].is_string() and
                                       root[2].is_string();
             if (not has_one_arg and not has_two_args) {
-                *error_msg = fmt::format(
+                return unexpected{fmt::format(
                     "\"git tree\" scheme expects one or two string "
                     "arguments, but found {} for {} of repository {}",
                     root.dump(),
                     keyword,
-                    repo);
-                return std::nullopt;
+                    repo)};
             }
             if (root.size() == 3) {
                 if (auto git_root = FileRoot::FromGit(root[2], root[1])) {
-                    return std::pair(std::move(*git_root), std::nullopt);
+                    return ResultType{std::move(*git_root), std::nullopt};
                 }
-                *error_msg = fmt::format(
+                return unexpected{fmt::format(
                     "Could not create file root for {}tree id {}",
                     root.size() == 3
                         ? fmt::format("git repository {} and ", root[2])
                         : "",
-                    root[1]);
-                return std::nullopt;
+                    root[1])};
             }
             // return absent root
-            return std::pair(FileRoot{std::string{root[1]}}, std::nullopt);
+            return ResultType{FileRoot{std::string{root[1]}}, std::nullopt};
         }
         if (root[0] == FileRoot::kFileIgnoreSpecialMarker) {
             if (root.size() != 2 or (not root[1].is_string())) {
-                *error_msg = fmt::format(
+                return unexpected{fmt::format(
                     "\"file ignore-special\" scheme expects precisely "
                     "one string "
                     "argument, but found {} for {} of repository {}",
                     root.dump(),
                     keyword,
-                    repo);
-                return std::nullopt;
+                    repo)};
             }
             auto path = std::filesystem::path{root[1].get<std::string>()};
-            return std::pair(FileRoot{path, /*ignore_special=*/true},
-                             std::move(path));
+            return ResultType{FileRoot{path, /*ignore_special=*/true},
+                              std::move(path)};
         }
         if (root[0] == FileRoot::kGitTreeIgnoreSpecialMarker) {
             bool const has_one_arg = root.size() == 2 and root[1].is_string();
@@ -788,60 +784,56 @@ class FileRoot {
                                       root[1].is_string() and
                                       root[2].is_string();
             if (not has_one_arg and not has_two_args) {
-                *error_msg = fmt::format(
+                return unexpected{fmt::format(
                     "\"git tree ignore-special\" scheme expects one or two "
                     "string arguments, but found {} for {} of repository {}",
                     root.dump(),
                     keyword,
-                    repo);
-                return std::nullopt;
+                    repo)};
             }
             if (root.size() == 3) {
                 if (auto git_root = FileRoot::FromGit(
                         root[2], root[1], /*ignore_special=*/true)) {
-                    return std::pair(std::move(*git_root), std::nullopt);
+                    return ResultType{std::move(*git_root), std::nullopt};
                 }
-                *error_msg = fmt::format(
+                return unexpected{fmt::format(
                     "Could not create ignore-special file root for {}tree id "
                     "{}",
                     root.size() == 3
                         ? fmt::format("git repository {} and ", root[2])
                         : "",
-                    root[1]);
-                return std::nullopt;
+                    root[1])};
             }
             // return absent root
-            return std::pair(
+            return ResultType{
                 FileRoot{std::string{root[1]}, /*ignore_special=*/true},
-                std::nullopt);
+                std::nullopt};
         }
         if (root[0] == FileRoot::kComputedMarker) {
             // NOLINTNEXTLINE(cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers)
             if (root.size() != 5 or (not root[1].is_string()) or
                 (not root[2].is_string()) or (not root[3].is_string()) or
                 (not root[4].is_object())) {
-                *error_msg = fmt::format(
+                return unexpected{fmt::format(
                     "{} scheme requires, in this order, the arugments root, "
                     "module, name, config. However found {} for {} of "
                     "repository {}",
                     kComputedMarker,
                     root.dump(),
                     keyword,
-                    repo);
-                return std::nullopt;
+                    repo)};
             }
-            return std::pair(FileRoot{std::string{root[1]},
-                                      std::string{root[2]},
-                                      std::string{root[3]},
-                                      root[4]},
-                             std::nullopt);
+            return ResultType{FileRoot{std::string{root[1]},
+                                       std::string{root[2]},
+                                       std::string{root[3]},
+                                       root[4]},
+                              std::nullopt};
         }
-        *error_msg = fmt::format(
+        return unexpected{fmt::format(
             "Unknown scheme in the specification {} of {} of repository {}",
             root.dump(),
             keyword,
-            repo);
-        return std::nullopt;
+            repo)};
     }
 
   private:
diff --git a/src/buildtool/main/main.cpp b/src/buildtool/main/main.cpp
index 6a37feec..629d2d78 100644
--- a/src/buildtool/main/main.cpp
+++ b/src/buildtool/main/main.cpp
@@ -556,16 +556,16 @@ auto DetermineRoots(gsl::not_null<RepositoryConfig*> const& repository_config,
         bool const is_main_repo{repo == main_repo};
         auto it_ws = desc.find("workspace_root");
         if (it_ws != desc.end()) {
-            std::string error_msg;
-            if (auto parsed_root = FileRoot::ParseRoot(
-                    repo, "workspace_root", *it_ws, &error_msg)) {
-                ws_root = std::move(parsed_root->first);
-                if (is_main_repo and parsed_root->second.has_value()) {
-                    main_ws_root = std::move(parsed_root->second);
+            if (auto parsed_root =
+                    FileRoot::ParseRoot(repo, "workspace_root", *it_ws)) {
+                auto result = *std::move(parsed_root);
+                ws_root = std::move(result.first);
+                if (is_main_repo and result.second.has_value()) {
+                    main_ws_root = std::move(result.second);
                 }
             }
             else {
-                Logger::Log(LogLevel::Error, error_msg);
+                Logger::Log(LogLevel::Error, parsed_root.error());
                 std::exit(kExitFailure);
             }
         }
@@ -595,13 +595,12 @@ auto DetermineRoots(gsl::not_null<RepositoryConfig*> const& repository_config,
                                       auto const& keyword_carg) {
             auto it = desc.find(keyword);
             if (it != desc.end()) {
-                std::string error_msg;
                 if (auto parsed_root =
-                        FileRoot::ParseRoot(repo, keyword, *it, &error_msg)) {
-                    (*keyword_root) = parsed_root->first;
+                        FileRoot::ParseRoot(repo, keyword, *it)) {
+                    (*keyword_root) = std::move(parsed_root)->first;
                 }
                 else {
-                    Logger::Log(LogLevel::Error, error_msg);
+                    Logger::Log(LogLevel::Error, parsed_root.error());
                     std::exit(kExitFailure);
                 }
             }
diff --git a/src/buildtool/serve_api/serve_service/target_utils.cpp b/src/buildtool/serve_api/serve_service/target_utils.cpp
index b87682c3..2cf940a8 100644
--- a/src/buildtool/serve_api/serve_service/target_utils.cpp
+++ b/src/buildtool/serve_api/serve_service/target_utils.cpp
@@ -112,11 +112,9 @@ auto DetermineRoots(RemoteServeConfig const& serve_config,
                 std::string const& keyword) -> expected<FileRoot, std::string> {
             auto it = desc.find(keyword);
             if (it != desc.end()) {
-                std::string error_msg;
-                auto parsed_root =
-                    FileRoot::ParseRoot(repo, keyword, *it, &error_msg);
+                auto parsed_root = FileRoot::ParseRoot(repo, keyword, *it);
                 if (not parsed_root) {
-                    return unexpected{std::move(error_msg)};
+                    return unexpected{std::move(parsed_root).error()};
                 }
                 // check that root has absent-like format
                 if (not parsed_root->first.IsAbsent()) {