Merge pull request #9653 from obsidiansystems/improve-parse-sink

Improve the `ParseSink` interface

src/libstore/daemon.cc

@@ -441,7 +441,7 @@ static void performOp(TunnelLogger * logger, ref<Store> store,
                        eagerly consume the entire stream it's given, past the
                        length of the Nar. */
                     TeeSource savedNARSource(from, saved);
-                    NullParseSink sink; /* just parse the NAR */
+                    NullFileSystemObjectSink sink; /* just parse the NAR */
                     parseDump(sink, savedNARSource);
                 } else {
                     /* Incrementally parse the NAR file, stripping the
@@ -913,7 +913,7 @@ static void performOp(TunnelLogger * logger, ref<Store> store,
                 source = std::make_unique<TunnelSource>(from, to);
             else {
                 TeeSource tee { from, saved };
-                NullParseSink ether;
+                NullFileSystemObjectSink ether;
                 parseDump(ether, tee);
                 source = std::make_unique<StringSource>(saved.s);
             }

src/libstore/export-import.cc

@@ -65,7 +65,7 @@ StorePaths Store::importPaths(Source & source, CheckSigsFlag checkSigs)
         /* Extract the NAR from the source. */
         StringSink saved;
         TeeSource tee { source, saved };
-        NullParseSink ether;
+        NullFileSystemObjectSink ether;
         parseDump(ether, tee);
 
         uint32_t magic = readInt(source);

src/libstore/local-store.cc

@@ -1048,7 +1048,7 @@ void LocalStore::addToStore(const ValidPathInfo & info, Source & source,
     bool narRead = false;
     Finally cleanup = [&]() {
         if (!narRead) {
-            NullParseSink sink;
+            NullFileSystemObjectSink sink;
             try {
                 parseDump(sink, source);
             } catch (...) {

src/libstore/nar-accessor.cc

@@ -19,6 +19,35 @@ struct NarMember
     std::map<std::string, NarMember> children;
 };
 
+struct NarMemberConstructor : CreateRegularFileSink
+{
+private:
+
+    NarMember & narMember;
+
+    uint64_t & pos;
+
+public:
+
+    NarMemberConstructor(NarMember & nm, uint64_t & pos)
+        : narMember(nm), pos(pos)
+    { }
+
+    void isExecutable() override
+    {
+        narMember.stat.isExecutable = true;
+    }
+
+    void preallocateContents(uint64_t size) override
+    {
+        narMember.stat.fileSize = size;
+        narMember.stat.narOffset = pos;
+    }
+
+    void operator () (std::string_view data) override
+    { }
+};
+
 struct NarAccessor : public SourceAccessor
 {
     std::optional<const std::string> nar;
@@ -27,7 +56,7 @@ struct NarAccessor : public SourceAccessor
 
     NarMember root;
 
-    struct NarIndexer : ParseSink, Source
+    struct NarIndexer : FileSystemObjectSink, Source
     {
         NarAccessor & acc;
         Source & source;
@@ -42,7 +71,7 @@ struct NarAccessor : public SourceAccessor
             : acc(acc), source(source)
         { }
 
-        void createMember(const Path & path, NarMember member)
+        NarMember & createMember(const Path & path, NarMember member)
         {
             size_t level = std::count(path.begin(), path.end(), '/');
             while (parents.size() > level) parents.pop();
@@ -50,11 +79,14 @@ struct NarAccessor : public SourceAccessor
             if (parents.empty()) {
                 acc.root = std::move(member);
                 parents.push(&acc.root);
+                return acc.root;
             } else {
                 if (parents.top()->stat.type != Type::tDirectory)
                     throw Error("NAR file missing parent directory of path '%s'", path);
                 auto result = parents.top()->children.emplace(baseNameOf(path), std::move(member));
-                parents.push(&result.first->second);
+                auto & ref = result.first->second;
+                parents.push(&ref);
+                return ref;
             }
         }
 
@@ -68,34 +100,18 @@ struct NarAccessor : public SourceAccessor
             } });
         }
 
-        void createRegularFile(const Path & path) override
+        void createRegularFile(const Path & path, std::function<void(CreateRegularFileSink &)> func) override
         {
-            createMember(path, NarMember{ .stat = {
+            auto & nm = createMember(path, NarMember{ .stat = {
                 .type = Type::tRegular,
                 .fileSize = 0,
                 .isExecutable = false,
                 .narOffset = 0
             } });
+            NarMemberConstructor nmc { nm, pos };
+            func(nmc);
         }
 
-        void closeRegularFile() override
-        { }
-
-        void isExecutable() override
-        {
-            parents.top()->stat.isExecutable = true;
-        }
-
-        void preallocateContents(uint64_t size) override
-        {
-            auto & st = parents.top()->stat;
-            st.fileSize = size;
-            st.narOffset = pos;
-        }
-
-        void receiveContents(std::string_view data) override
-        { }
-
         void createSymlink(const Path & path, const std::string & target) override
         {
             createMember(path,

src/libstore/store-api.cc

@@ -424,12 +424,12 @@ ValidPathInfo Store::addToStoreSlow(
        information to narSink. */
     TeeSource tapped { *fileSource, narSink };
 
-    NullParseSink blank;
+    NullFileSystemObjectSink blank;
     auto & parseSink = method.getFileIngestionMethod() == FileIngestionMethod::Flat
-        ? (ParseSink &) fileSink
+        ? (FileSystemObjectSink &) fileSink
         : method.getFileIngestionMethod() == FileIngestionMethod::Recursive
-        ? (ParseSink &) blank
+        ? (FileSystemObjectSink &) blank
-        : (abort(), (ParseSink &)*(ParseSink *)nullptr); // handled both cases
+        : (abort(), (FileSystemObjectSink &)*(FileSystemObjectSink *)nullptr); // handled both cases
 
     /* The information that flows from tapped (besides being replicated in
        narSink), is now put in parseSink. */

src/libutil/archive.cc

@@ -133,7 +133,7 @@ static SerialisationError badArchive(const std::string & s)
 }
 
 
-static void parseContents(ParseSink & sink, Source & source, const Path & path)
+static void parseContents(CreateRegularFileSink & sink, Source & source)
 {
     uint64_t size = readLongLong(source);
 
@@ -147,7 +147,7 @@ static void parseContents(ParseSink & sink, Source & source, const Path & path)
         auto n = buf.size();
         if ((uint64_t)n > left) n = left;
         source(buf.data(), n);
-        sink.receiveContents({buf.data(), n});
+        sink({buf.data(), n});
         left -= n;
     }
 
@@ -164,109 +164,121 @@ struct CaseInsensitiveCompare
 };
 
 
-static void parse(ParseSink & sink, Source & source, const Path & path)
+static void parse(FileSystemObjectSink & sink, Source & source, const Path & path)
 {
     std::string s;
 
     s = readString(source);
     if (s != "(") throw badArchive("expected open tag");
 
-    enum { tpUnknown, tpRegular, tpDirectory, tpSymlink } type = tpUnknown;
-
     std::map<Path, int, CaseInsensitiveCompare> names;
 
-    while (1) {
+    auto getString = [&]() {
         checkInterrupt();
+        return readString(source);
+    };
 
-        s = readString(source);
+    // For first iteration
+    s = getString();
+
+    while (1) {
 
         if (s == ")") {
             break;
         }
 
         else if (s == "type") {
-            if (type != tpUnknown)
+            std::string t = getString();
-                throw badArchive("multiple type fields");
-            std::string t = readString(source);
 
             if (t == "regular") {
-                type = tpRegular;
+                sink.createRegularFile(path, [&](auto & crf) {
-                sink.createRegularFile(path);
+                    while (1) {
+                        s = getString();
+
+                        if (s == "contents") {
+                            parseContents(crf, source);
+                        }
+
+                        else if (s == "executable") {
+                            auto s2 = getString();
+                            if (s2 != "") throw badArchive("executable marker has non-empty value");
+                            crf.isExecutable();
+                        }
+
+                        else break;
+                    }
+                });
             }
 
             else if (t == "directory") {
                 sink.createDirectory(path);
-                type = tpDirectory;
+
+                while (1) {
+                    s = getString();
+
+                    if (s == "entry") {
+                        std::string name, prevName;
+
+                        s = getString();
+                        if (s != "(") throw badArchive("expected open tag");
+
+                        while (1) {
+                            s = getString();
+
+                            if (s == ")") {
+                                break;
+                            } else if (s == "name") {
+                                name = getString();
+                                if (name.empty() || name == "." || name == ".." || name.find('/') != std::string::npos || name.find((char) 0) != std::string::npos)
+                                    throw Error("NAR contains invalid file name '%1%'", name);
+                                if (name <= prevName)
+                                    throw Error("NAR directory is not sorted");
+                                prevName = name;
+                                if (archiveSettings.useCaseHack) {
+                                    auto i = names.find(name);
+                                    if (i != names.end()) {
+                                        debug("case collision between '%1%' and '%2%'", i->first, name);
+                                        name += caseHackSuffix;
+                                        name += std::to_string(++i->second);
+                                    } else
+                                        names[name] = 0;
+                                }
+                            } else if (s == "node") {
+                                if (name.empty()) throw badArchive("entry name missing");
+                                parse(sink, source, path + "/" + name);
+                            } else
+                                throw badArchive("unknown field " + s);
+                        }
+                    }
+
+                    else break;
+                }
             }
 
             else if (t == "symlink") {
-                type = tpSymlink;
+                s = getString();
+
+                if (s != "target")
+                    throw badArchive("expected 'target' got " + s);
+
+                std::string target = getString();
+                sink.createSymlink(path, target);
+
+                // for the next iteration
+                s = getString();
             }
 
             else throw badArchive("unknown file type " + t);
 
         }
 
-        else if (s == "contents" && type == tpRegular) {
-            parseContents(sink, source, path);
-            sink.closeRegularFile();
-        }
-
-        else if (s == "executable" && type == tpRegular) {
-            auto s = readString(source);
-            if (s != "") throw badArchive("executable marker has non-empty value");
-            sink.isExecutable();
-        }
-
-        else if (s == "entry" && type == tpDirectory) {
-            std::string name, prevName;
-
-            s = readString(source);
-            if (s != "(") throw badArchive("expected open tag");
-
-            while (1) {
-                checkInterrupt();
-
-                s = readString(source);
-
-                if (s == ")") {
-                    break;
-                } else if (s == "name") {
-                    name = readString(source);
-                    if (name.empty() || name == "." || name == ".." || name.find('/') != std::string::npos || name.find((char) 0) != std::string::npos)
-                        throw Error("NAR contains invalid file name '%1%'", name);
-                    if (name <= prevName)
-                        throw Error("NAR directory is not sorted");
-                    prevName = name;
-                    if (archiveSettings.useCaseHack) {
-                        auto i = names.find(name);
-                        if (i != names.end()) {
-                            debug("case collision between '%1%' and '%2%'", i->first, name);
-                            name += caseHackSuffix;
-                            name += std::to_string(++i->second);
-                        } else
-                            names[name] = 0;
-                    }
-                } else if (s == "node") {
-                    if (name.empty()) throw badArchive("entry name missing");
-                    parse(sink, source, path + "/" + name);
-                } else
-                    throw badArchive("unknown field " + s);
-            }
-        }
-
-        else if (s == "target" && type == tpSymlink) {
-            std::string target = readString(source);
-            sink.createSymlink(path, target);
-        }
-
         else
             throw badArchive("unknown field " + s);
     }
 }
 
 
-void parseDump(ParseSink & sink, Source & source)
+void parseDump(FileSystemObjectSink & sink, Source & source)
 {
     std::string version;
     try {
@@ -294,7 +306,7 @@ void copyNAR(Source & source, Sink & sink)
     // FIXME: if 'source' is the output of dumpPath() followed by EOF,
     // we should just forward all data directly without parsing.
 
-    NullParseSink parseSink; /* just parse the NAR */
+    NullFileSystemObjectSink parseSink; /* just parse the NAR */
 
     TeeSource wrapper { source, sink };
 

src/libutil/archive.hh

@@ -73,7 +73,7 @@ time_t dumpPathAndGetMtime(const Path & path, Sink & sink,
  */
 void dumpString(std::string_view s, Sink & sink);
 
-void parseDump(ParseSink & sink, Source & source);
+void parseDump(FileSystemObjectSink & sink, Source & source);
 
 void restorePath(const Path & path, Source & source);
 

src/libutil/file-content-address.hh

@@ -35,7 +35,7 @@ void dumpPath(
 /**
  * Restore a serialization of the given file system object.
  *
- * @TODO use an arbitrary `ParseSink`.
+ * @TODO use an arbitrary `FileSystemObjectSink`.
  */
 void restorePath(
     const Path & path,

src/libutil/fs-sink.cc

@@ -7,7 +7,7 @@ namespace nix {
 
 void copyRecursive(
     SourceAccessor & accessor, const CanonPath & from,
-    ParseSink & sink, const Path & to)
+    FileSystemObjectSink & sink, const Path & to)
 {
     auto stat = accessor.lstat(from);
 
@@ -19,16 +19,12 @@ void copyRecursive(
 
     case SourceAccessor::tRegular:
     {
-        sink.createRegularFile(to);
+        sink.createRegularFile(to, [&](CreateRegularFileSink & crf) {
-        if (stat.isExecutable)
+            if (stat.isExecutable)
-            sink.isExecutable();
+                crf.isExecutable();
-        LambdaSink sink2 {
+            accessor.readFile(from, crf, [&](uint64_t size) {
-            [&](auto d) {
+                crf.preallocateContents(size);
-                sink.receiveContents(d);
+            });
-            }
-        };
-        accessor.readFile(from, sink2, [&](uint64_t size) {
-            sink.preallocateContents(size);
         });
         break;
     }
@@ -71,20 +67,24 @@ void RestoreSink::createDirectory(const Path & path)
         throw SysError("creating directory '%1%'", p);
 };
 
-void RestoreSink::createRegularFile(const Path & path)
+struct RestoreRegularFile : CreateRegularFileSink {
+    AutoCloseFD fd;
+
+    void operator () (std::string_view data) override;
+    void isExecutable() override;
+    void preallocateContents(uint64_t size) override;
+};
+
+void RestoreSink::createRegularFile(const Path & path, std::function<void(CreateRegularFileSink &)> func)
 {
     Path p = dstPath + path;
-    fd = open(p.c_str(), O_CREAT | O_EXCL | O_WRONLY | O_CLOEXEC, 0666);
+    RestoreRegularFile crf;
-    if (!fd) throw SysError("creating file '%1%'", p);
+    crf.fd = open(p.c_str(), O_CREAT | O_EXCL | O_WRONLY | O_CLOEXEC, 0666);
+    if (!crf.fd) throw SysError("creating file '%1%'", p);
+    func(crf);
 }
 
-void RestoreSink::closeRegularFile()
+void RestoreRegularFile::isExecutable()
-{
-    /* Call close explicitly to make sure the error is checked */
-    fd.close();
-}
-
-void RestoreSink::isExecutable()
 {
     struct stat st;
     if (fstat(fd.get(), &st) == -1)
@@ -93,7 +93,7 @@ void RestoreSink::isExecutable()
         throw SysError("fchmod");
 }
 
-void RestoreSink::preallocateContents(uint64_t len)
+void RestoreRegularFile::preallocateContents(uint64_t len)
 {
     if (!restoreSinkSettings.preallocateContents)
         return;
@@ -111,7 +111,7 @@ void RestoreSink::preallocateContents(uint64_t len)
 #endif
 }
 
-void RestoreSink::receiveContents(std::string_view data)
+void RestoreRegularFile::operator () (std::string_view data)
 {
     writeFull(fd.get(), data);
 }
@@ -122,4 +122,32 @@ void RestoreSink::createSymlink(const Path & path, const std::string & target)
     nix::createSymlink(target, p);
 }
 
+
+void RegularFileSink::createRegularFile(const Path & path, std::function<void(CreateRegularFileSink &)> func)
+{
+    struct CRF : CreateRegularFileSink {
+        RegularFileSink & back;
+        CRF(RegularFileSink & back) : back(back) {}
+        void operator () (std::string_view data) override
+        {
+            back.sink(data);
+        }
+        void isExecutable() override {}
+    } crf { *this };
+    func(crf);
+}
+
+
+void NullFileSystemObjectSink::createRegularFile(const Path & path, std::function<void(CreateRegularFileSink &)> func)
+{
+    struct : CreateRegularFileSink {
+        void operator () (std::string_view data) override {}
+        void isExecutable() override {}
+    } crf;
+    // Even though `NullFileSystemObjectSink` doesn't do anything, it's important
+    // that we call the function, to e.g. advance the parser using this
+    // sink.
+    func(crf);
+}
+
 }

src/libutil/fs-sink.hh

@@ -9,18 +9,13 @@
 namespace nix {
 
 /**
- * \todo Fix this API, it sucks.
+ * Actions on an open regular file in the process of creating it.
+ *
+ * See `FileSystemObjectSink::createRegularFile`.
  */
-struct ParseSink
+struct CreateRegularFileSink : Sink
 {
-    virtual void createDirectory(const Path & path) = 0;
-
-    virtual void createRegularFile(const Path & path) = 0;
-    virtual void receiveContents(std::string_view data) = 0;
     virtual void isExecutable() = 0;
-    virtual void closeRegularFile() = 0;
-
-    virtual void createSymlink(const Path & path, const std::string & target) = 0;
 
     /**
      * An optimization. By default, do nothing.
@@ -28,46 +23,55 @@ struct ParseSink
     virtual void preallocateContents(uint64_t size) { };
 };
 
+
+struct FileSystemObjectSink
+{
+    virtual void createDirectory(const Path & path) = 0;
+
+    /**
+     * This function in general is no re-entrant. Only one file can be
+     * written at a time.
+     */
+    virtual void createRegularFile(
+        const Path & path,
+        std::function<void(CreateRegularFileSink &)>) = 0;
+
+    virtual void createSymlink(const Path & path, const std::string & target) = 0;
+};
+
 /**
- * Recusively copy file system objects from the source into the sink.
+ * Recursively copy file system objects from the source into the sink.
  */
 void copyRecursive(
     SourceAccessor & accessor, const CanonPath & sourcePath,
-    ParseSink & sink, const Path & destPath);
+    FileSystemObjectSink & sink, const Path & destPath);
 
 /**
  * Ignore everything and do nothing
  */
-struct NullParseSink : ParseSink
+struct NullFileSystemObjectSink : FileSystemObjectSink
 {
     void createDirectory(const Path & path) override { }
-    void receiveContents(std::string_view data) override { }
     void createSymlink(const Path & path, const std::string & target) override { }
-    void createRegularFile(const Path & path) override { }
+    void createRegularFile(
-    void closeRegularFile() override { }
+        const Path & path,
-    void isExecutable() override { }
+        std::function<void(CreateRegularFileSink &)>) override;
 };
 
 /**
  * Write files at the given path
  */
-struct RestoreSink : ParseSink
+struct RestoreSink : FileSystemObjectSink
 {
     Path dstPath;
 
     void createDirectory(const Path & path) override;
 
-    void createRegularFile(const Path & path) override;
+    void createRegularFile(
-    void receiveContents(std::string_view data) override;
+        const Path & path,
-    void isExecutable() override;
+        std::function<void(CreateRegularFileSink &)>) override;
-    void closeRegularFile() override;
 
     void createSymlink(const Path & path, const std::string & target) override;
-
-    void preallocateContents(uint64_t size) override;
-
-private:
-    AutoCloseFD fd;
 };
 
 /**
@@ -75,7 +79,7 @@ private:
  * `receiveContents` to the underlying `Sink`. For anything but a single
  * file, set `regular = true` so the caller can fail accordingly.
  */
-struct RegularFileSink : ParseSink
+struct RegularFileSink : FileSystemObjectSink
 {
     bool regular = true;
     Sink & sink;
@@ -87,19 +91,14 @@ struct RegularFileSink : ParseSink
         regular = false;
     }
 
-    void receiveContents(std::string_view data) override
-    {
-        sink(data);
-    }
-
     void createSymlink(const Path & path, const std::string & target) override
     {
         regular = false;
     }
 
-    void createRegularFile(const Path & path) override { }
+    void createRegularFile(
-    void closeRegularFile() override { }
+        const Path & path,
-    void isExecutable() override { }
+        std::function<void(CreateRegularFileSink &)>) override;
 };
 
 }

src/libutil/git.cc

@@ -52,24 +52,22 @@ static std::string getString(Source & source, int n)
     return v;
 }
 
-
+void parseBlob(
-void parse(
+    FileSystemObjectSink & sink,
-    ParseSink & sink,
     const Path & sinkPath,
     Source & source,
-    std::function<SinkHook> hook,
+    bool executable,
     const ExperimentalFeatureSettings & xpSettings)
 {
     xpSettings.require(Xp::GitHashing);
 
-    auto type = getString(source, 5);
+    sink.createRegularFile(sinkPath, [&](auto & crf) {
-
+        if (executable)
-    if (type == "blob ") {
+            crf.isExecutable();
-        sink.createRegularFile(sinkPath);
 
         unsigned long long size = std::stoi(getStringUntil(source, 0));
 
-        sink.preallocateContents(size);
+        crf.preallocateContents(size);
 
         unsigned long long left = size;
         std::string buf;
@@ -79,47 +77,91 @@ void parse(
             checkInterrupt();
             buf.resize(std::min((unsigned long long)buf.capacity(), left));
             source(buf);
-            sink.receiveContents(buf);
+            crf(buf);
             left -= buf.size();
         }
+    });
+}
+
+void parseTree(
+    FileSystemObjectSink & sink,
+    const Path & sinkPath,
+    Source & source,
+    std::function<SinkHook> hook,
+    const ExperimentalFeatureSettings & xpSettings)
+{
+    unsigned long long size = std::stoi(getStringUntil(source, 0));
+    unsigned long long left = size;
+
+    sink.createDirectory(sinkPath);
+
+    while (left) {
+        std::string perms = getStringUntil(source, ' ');
+        left -= perms.size();
+        left -= 1;
+
+        RawMode rawMode = std::stoi(perms, 0, 8);
+        auto modeOpt = decodeMode(rawMode);
+        if (!modeOpt)
+            throw Error("Unknown Git permission: %o", perms);
+        auto mode = std::move(*modeOpt);
+
+        std::string name = getStringUntil(source, '\0');
+        left -= name.size();
+        left -= 1;
+
+        std::string hashs = getString(source, 20);
+        left -= 20;
+
+        Hash hash(HashAlgorithm::SHA1);
+        std::copy(hashs.begin(), hashs.end(), hash.hash);
+
+        hook(name, TreeEntry {
+            .mode = mode,
+            .hash = hash,
+        });
+    }
+}
+
+ObjectType parseObjectType(
+    Source & source,
+    const ExperimentalFeatureSettings & xpSettings)
+{
+    xpSettings.require(Xp::GitHashing);
+
+    auto type = getString(source, 5);
+
+    if (type == "blob ") {
+        return ObjectType::Blob;
     } else if (type == "tree ") {
-        unsigned long long size = std::stoi(getStringUntil(source, 0));
+        return ObjectType::Tree;
-        unsigned long long left = size;
-
-        sink.createDirectory(sinkPath);
-
-        while (left) {
-            std::string perms = getStringUntil(source, ' ');
-            left -= perms.size();
-            left -= 1;
-
-            RawMode rawMode = std::stoi(perms, 0, 8);
-            auto modeOpt = decodeMode(rawMode);
-            if (!modeOpt)
-                throw Error("Unknown Git permission: %o", perms);
-            auto mode = std::move(*modeOpt);
-
-            std::string name = getStringUntil(source, '\0');
-            left -= name.size();
-            left -= 1;
-
-            std::string hashs = getString(source, 20);
-            left -= 20;
-
-            Hash hash(HashAlgorithm::SHA1);
-            std::copy(hashs.begin(), hashs.end(), hash.hash);
-
-            hook(name, TreeEntry {
-                .mode = mode,
-                .hash = hash,
-            });
-
-            if (mode == Mode::Executable)
-                sink.isExecutable();
-        }
     } else throw Error("input doesn't look like a Git object");
 }
 
+void parse(
+    FileSystemObjectSink & sink,
+    const Path & sinkPath,
+    Source & source,
+    bool executable,
+    std::function<SinkHook> hook,
+    const ExperimentalFeatureSettings & xpSettings)
+{
+    xpSettings.require(Xp::GitHashing);
+
+    auto type = parseObjectType(source, xpSettings);
+
+    switch (type) {
+    case ObjectType::Blob:
+        parseBlob(sink, sinkPath, source, executable, xpSettings);
+        break;
+    case ObjectType::Tree:
+        parseTree(sink, sinkPath, source, hook, xpSettings);
+        break;
+    default:
+        assert(false);
+    };
+}
+
 
 std::optional<Mode> convertMode(SourceAccessor::Type type)
 {
@@ -133,9 +175,9 @@ std::optional<Mode> convertMode(SourceAccessor::Type type)
 }
 
 
-void restore(ParseSink & sink, Source & source, std::function<RestoreHook> hook)
+void restore(FileSystemObjectSink & sink, Source & source, std::function<RestoreHook> hook)
 {
-    parse(sink, "", source, [&](Path name, TreeEntry entry) {
+    parse(sink, "", source, false, [&](Path name, TreeEntry entry) {
         auto [accessor, from] = hook(entry.hash);
         auto stat = accessor->lstat(from);
         auto gotOpt = convertMode(stat.type);

src/libutil/git.hh

@@ -13,12 +13,19 @@
 
 namespace nix::git {
 
+enum struct ObjectType {
+    Blob,
+    Tree,
+    //Commit,
+    //Tag,
+};
+
 using RawMode = uint32_t;
 
 enum struct Mode : RawMode {
     Directory = 0040000,
-    Executable = 0100755,
     Regular = 0100644,
+    Executable = 0100755,
     Symlink = 0120000,
 };
 
@@ -59,9 +66,34 @@ using Tree = std::map<std::string, TreeEntry>;
  */
 using SinkHook = void(const Path & name, TreeEntry entry);
 
-void parse(
+/**
-    ParseSink & sink, const Path & sinkPath,
+ * Parse the "blob " or "tree " prefix.
+ *
+ * @throws if prefix not recognized
+ */
+ObjectType parseObjectType(
     Source & source,
+    const ExperimentalFeatureSettings & xpSettings = experimentalFeatureSettings);
+
+void parseBlob(
+    FileSystemObjectSink & sink, const Path & sinkPath,
+    Source & source,
+    bool executable,
+    const ExperimentalFeatureSettings & xpSettings = experimentalFeatureSettings);
+
+void parseTree(
+    FileSystemObjectSink & sink, const Path & sinkPath,
+    Source & source,
+    std::function<SinkHook> hook,
+    const ExperimentalFeatureSettings & xpSettings = experimentalFeatureSettings);
+
+/**
+ * Helper putting the previous three `parse*` functions together.
+ */
+void parse(
+    FileSystemObjectSink & sink, const Path & sinkPath,
+    Source & source,
+    bool executable,
     std::function<SinkHook> hook,
     const ExperimentalFeatureSettings & xpSettings = experimentalFeatureSettings);
 
@@ -81,7 +113,7 @@ using RestoreHook = std::pair<SourceAccessor *, CanonPath>(Hash);
 /**
  * Wrapper around `parse` and `RestoreSink`
  */
-void restore(ParseSink & sink, Source & source, std::function<RestoreHook> hook);
+void restore(FileSystemObjectSink & sink, Source & source, std::function<RestoreHook> hook);
 
 /**
  * Dumps a single file to a sink

src/libutil/memory-source-accessor.cc

@@ -134,36 +134,43 @@ void MemorySink::createDirectory(const Path & path)
         throw Error("file '%s' is not a directory", path);
 };
 
-void MemorySink::createRegularFile(const Path & path)
+struct CreateMemoryRegularFile : CreateRegularFileSink {
+    File::Regular & regularFile;
+
+    CreateMemoryRegularFile(File::Regular & r)
+        : regularFile(r)
+    { }
+
+    void operator () (std::string_view data) override;
+    void isExecutable() override;
+    void preallocateContents(uint64_t size) override;
+};
+
+void MemorySink::createRegularFile(const Path & path, std::function<void(CreateRegularFileSink &)> func)
 {
     auto * f = dst.open(CanonPath{path}, File { File::Regular {} });
     if (!f)
         throw Error("file '%s' cannot be made because some parent file is not a directory", path);
-    if (!(r = std::get_if<File::Regular>(&f->raw)))
+    if (auto * rp = std::get_if<File::Regular>(&f->raw)) {
+        CreateMemoryRegularFile crf { *rp };
+        func(crf);
+    } else
         throw Error("file '%s' is not a regular file", path);
 }
 
-void MemorySink::closeRegularFile()
+void CreateMemoryRegularFile::isExecutable()
 {
-    r = nullptr;
+    regularFile.executable = true;
 }
 
-void MemorySink::isExecutable()
+void CreateMemoryRegularFile::preallocateContents(uint64_t len)
 {
-    assert(r);
+    regularFile.contents.reserve(len);
-    r->executable = true;
 }
 
-void MemorySink::preallocateContents(uint64_t len)
+void CreateMemoryRegularFile::operator () (std::string_view data)
 {
-    assert(r);
+    regularFile.contents += data;
-    r->contents.reserve(len);
-}
-
-void MemorySink::receiveContents(std::string_view data)
-{
-    assert(r);
-    r->contents += data;
 }
 
 void MemorySink::createSymlink(const Path & path, const std::string & target)

src/libutil/memory-source-accessor.hh

@@ -75,7 +75,7 @@ struct MemorySourceAccessor : virtual SourceAccessor
 /**
  * Write to a `MemorySourceAccessor` at the given path
  */
-struct MemorySink : ParseSink
+struct MemorySink : FileSystemObjectSink
 {
     MemorySourceAccessor & dst;
 
@@ -83,17 +83,11 @@ struct MemorySink : ParseSink
 
     void createDirectory(const Path & path) override;
 
-    void createRegularFile(const Path & path) override;
+    void createRegularFile(
-    void receiveContents(std::string_view data) override;
+        const Path & path,
-    void isExecutable() override;
+        std::function<void(CreateRegularFileSink &)>) override;
-    void closeRegularFile() override;
 
     void createSymlink(const Path & path, const std::string & target) override;
-
-    void preallocateContents(uint64_t size) override;
-
-private:
-    MemorySourceAccessor::File::Regular * r;
 };
 
 }

tests/unit/libutil/git.cc

@@ -66,7 +66,8 @@ TEST_F(GitTest, blob_read) {
         StringSource in { encoded };
         StringSink out;
         RegularFileSink out2 { out };
-        parse(out2, "", in, [](auto &, auto) {}, mockXpSettings);
+        ASSERT_EQ(parseObjectType(in, mockXpSettings), ObjectType::Blob);
+        parseBlob(out2, "", in, false, mockXpSettings);
 
         auto expected = readFile(goldenMaster("hello-world.bin"));
 
@@ -119,9 +120,10 @@ const static Tree tree = {
 TEST_F(GitTest, tree_read) {
     readTest("tree.bin", [&](const auto & encoded) {
         StringSource in { encoded };
-        NullParseSink out;
+        NullFileSystemObjectSink out;
         Tree got;
-        parse(out, "", in, [&](auto & name, auto entry) {
+        ASSERT_EQ(parseObjectType(in, mockXpSettings), ObjectType::Tree);
+        parseTree(out, "", in, [&](auto & name, auto entry) {
             auto name2 = name;
             if (entry.mode == Mode::Directory)
                 name2 += '/';
@@ -193,15 +195,21 @@ TEST_F(GitTest, both_roundrip) {
 
     MemorySink sinkFiles2 { files2 };
 
-    std::function<void(const Path, const Hash &)> mkSinkHook;
+    std::function<void(const Path, const Hash &, bool)> mkSinkHook;
-    mkSinkHook = [&](const Path prefix, const Hash & hash) {
+    mkSinkHook = [&](auto prefix, auto & hash, auto executable) {
         StringSource in { cas[hash] };
-        parse(sinkFiles2, prefix, in, [&](const Path & name, const auto & entry) {
+        parse(
-            mkSinkHook(prefix + "/" + name, entry.hash);
+            sinkFiles2, prefix, in, executable,
-        }, mockXpSettings);
+            [&](const Path & name, const auto & entry) {
+                mkSinkHook(
+                    prefix + "/" + name,
+                    entry.hash,
+                    entry.mode == Mode::Executable);
+            },
+            mockXpSettings);
     };
 
-    mkSinkHook("", root.hash);
+    mkSinkHook("", root.hash, false);
 
     ASSERT_EQ(files, files2);
 }