FileRoot: Fix parsing of ignore-special Git tree roots

When populating the GitTree instance stored in a Git tree-type
FileRoot with the ignore-special flag set, the GitTree instance
would be created with an empty raw_id_ field, signaling that some
of the entries might have been skipped and thus the root tree id
is not anymore in a one-to-one correspondence with the stored list
of entries.

This however caused FileRoot instances with missing tree id
information. This commit fixes the issue by always storing the
raw_id_ field as the root id of the Git tree, as well as clarifying
the relationship between this field and the ignore_special_ flag,
including refactoring the tree id getters.

(cherry picked from 06336198a38ffaec6c35df34580391579a7a7b22)

4 files changed, 43 insertions, 26 deletions

diff --git a/src/buildtool/execution_engine/executor/executor.hpp b/src/buildtool/execution_engine/executor/executor.hpp
index f7afb077..b7ca034c 100644
--- a/src/buildtool/execution_engine/executor/executor.hpp
+++ b/src/buildtool/execution_engine/executor/executor.hpp
@@ -269,7 +269,8 @@ class ExecutorImpl {
 
         Logger::Log(LogLevel::Trace, [&tree]() {
             std::ostringstream oss{};
-            oss << "upload directory content of " << tree.Hash() << std::endl;
+            oss << "upload directory content of " << tree.FileRootHash()
+                << std::endl;
             for (auto const& [path, entry] : tree) {
                 oss << fmt::format(" - {}: {}", path, entry->Hash())
                     << std::endl;
@@ -344,7 +345,7 @@ class ExecutorImpl {
             if (not VerifyOrUploadTree(api, *tree)) {
                 Logger::Log(LogLevel::Error,
                             "failed to verifyorupload git tree {} [{}]",
-                            tree->Hash(),
+                            tree->FileRootHash(),
                             hash);
                 return false;
             }
diff --git a/src/buildtool/file_system/file_root.hpp b/src/buildtool/file_system/file_root.hpp
index 08779e8e..eff4d450 100644
--- a/src/buildtool/file_system/file_root.hpp
+++ b/src/buildtool/file_system/file_root.hpp
@@ -225,17 +225,15 @@ class FileRoot {
             if (std::holds_alternative<tree_t>(data_)) {
                 try {
                     auto const& data = std::get<tree_t>(data_);
-                    // check if tree is ignore_special
-                    if (data->RawHash().empty()) {
-                        return std::nullopt;
-                    }
-                    auto const& id = data->Hash();
-                    auto const& size = data->Size();
-                    if (size) {
-                        return ArtifactDescription{
-                            ArtifactDigest{id, *size, /*is_tree=*/true},
-                            ObjectType::Tree,
-                            repository};
+                    // only consider tree if we have it unmodified
+                    if (auto id = data->Hash()) {
+                        auto const& size = data->Size();
+                        if (size) {
+                            return ArtifactDescription{
+                                ArtifactDigest{*id, *size, /*is_tree=*/true},
+                                ObjectType::Tree,
+                                repository};
+                        }
                     }
                 } catch (...) {
                     return std::nullopt;
@@ -351,7 +349,8 @@ class FileRoot {
                 nlohmann::json j;
                 j.push_back(ignore_special_ ? kGitTreeIgnoreSpecialMarker
                                             : kGitTreeMarker);
-                j.push_back(std::get<git_root_t>(root_).tree->Hash());
+                // we need the root tree id, irrespective of ignore_special flag
+                j.push_back(std::get<git_root_t>(root_).tree->FileRootHash());
                 return j;
             }
             if (std::holds_alternative<absent_root_t>(root_)) {
diff --git a/src/buildtool/file_system/git_tree.cpp b/src/buildtool/file_system/git_tree.cpp
index a5a3262d..e7a2f41d 100644
--- a/src/buildtool/file_system/git_tree.cpp
+++ b/src/buildtool/file_system/git_tree.cpp
@@ -88,12 +88,10 @@ auto GitTree::Read(gsl::not_null<GitCASPtr> const& cas,
                                               check_symlinks,
                                               /*is_hex_id=*/false,
                                               ignore_special)) {
-                // the raw_id value is NOT recomputed when ignore_special==true,
-                // so we set it to empty to signal that it should not be used!
-                return GitTree::FromEntries(cas,
-                                            std::move(*entries),
-                                            ignore_special ? "" : *raw_id,
-                                            ignore_special);
+                // NOTE: the raw_id value is NOT recomputed when
+                // ignore_special==true.
+                return GitTree::FromEntries(
+                    cas, std::move(*entries), *raw_id, ignore_special);
             }
         }
         else {
@@ -164,7 +162,6 @@ auto GitTreeEntry::Tree(bool ignore_special) const& noexcept
                                                   check_symlinks,
                                                   /*is_hex_id=*/false,
                                                   ignore_special)) {
-                    // the raw_id value is not used when ignore_special==true
                     return GitTree::FromEntries(
                         cas_, std::move(*entries), raw_id_, ignore_special);
                 }
diff --git a/src/buildtool/file_system/git_tree.hpp b/src/buildtool/file_system/git_tree.hpp
index 6146fb0b..c6a0833e 100644
--- a/src/buildtool/file_system/git_tree.hpp
+++ b/src/buildtool/file_system/git_tree.hpp
@@ -50,8 +50,7 @@ class GitTree {
     /// \param tree_id  Tree id as as hex string.
     /// \param ignore_special   If set, treat symlinks as absent.
     /// NOTE: If ignore_special==true, the stored entries might differ from the
-    /// actual tree, so the stored ID is set to empty to signal that it should
-    /// not be used.
+    /// actual tree, as some filesystem entries get skipped.
     [[nodiscard]] static auto Read(gsl::not_null<GitCASPtr> const& cas,
                                    std::string const& tree_id,
                                    bool ignore_special = false) noexcept
@@ -67,16 +66,37 @@ class GitTree {
 
     [[nodiscard]] auto begin() const noexcept { return entries_.begin(); }
     [[nodiscard]] auto end() const noexcept { return entries_.end(); }
-    [[nodiscard]] auto Hash() const noexcept { return ToHexString(raw_id_); }
-    [[nodiscard]] auto RawHash() const noexcept { return raw_id_; }
+
+    // Getter for the root tree id with fast lookup flag check. This enforces
+    // automatically that no filesystem entry was skipped during creation.
+    [[nodiscard]] auto Hash() const noexcept {
+        return ignore_special_ ? std::nullopt
+                               : std::make_optional(ToHexString(raw_id_));
+    }
+
+    // Getter of the raw root tree id with no fast lookup flag check. As such,
+    // the caller MUST NOT assume that there is a one-to-one correspondence
+    // between this returned tree id and the values stored in entries_.
+    [[nodiscard]] auto FileRootHash() const noexcept {
+        return ToHexString(raw_id_);
+    }
+
+    // Getter of the raw root tree id with no fast lookup flag check. As such,
+    // the caller MUST NOT assume that there is a one-to-one correspondence
+    // between this returned tree id and the values stored in entries_.
+    [[nodiscard]] auto RawFileRootHash() const noexcept { return raw_id_; }
+
     [[nodiscard]] auto Size() const noexcept -> std::optional<std::size_t>;
     [[nodiscard]] auto RawData() const noexcept -> std::optional<std::string>;
 
   private:
     gsl::not_null<GitCASPtr> cas_;
+    // If not ignore_special_, contains all the entries of tree raw_id_.
     entries_t entries_;
+    // Stores the root id of the tree; if ignore_special_, this is not
+    // guaranteed to be the same as the id of the tree containing entries_.
     std::string raw_id_;
-    // If set, ignore all fast tree lookups and always traverse
+    // If set, ignore all fast tree lookups and always traverse.
     bool ignore_special_;
 
     GitTree(gsl::not_null<GitCASPtr> const& cas,