Merge branch 'fix-and-document-addToStoreSlow' of github.com:obsidiansystems/nix into ca-derivation-data-types

Makefile.config.in

@@ -19,6 +19,7 @@ LIBLZMA_LIBS = @LIBLZMA_LIBS@
 OPENSSL_LIBS = @OPENSSL_LIBS@
 PACKAGE_NAME = @PACKAGE_NAME@
 PACKAGE_VERSION = @PACKAGE_VERSION@
+SHELL = @bash@
 SODIUM_LIBS = @SODIUM_LIBS@
 SQLITE3_LIBS = @SQLITE3_LIBS@
 bash = @bash@

scripts/install-nix-from-closure.sh

@@ -207,7 +207,7 @@ if [ -z "$NIX_INSTALLER_NO_MODIFY_PROFILE" ]; then
         if [ -w "$fn" ]; then
             if ! grep -q "$p" "$fn"; then
                 echo "modifying $fn..." >&2
-                echo "if [ -e $p ]; then . $p; fi # added by Nix installer" >> "$fn"
+                echo -e "\nif [ -e $p ]; then . $p; fi # added by Nix installer" >> "$fn"
             fi
             added=1
             break
@@ -218,7 +218,7 @@ if [ -z "$NIX_INSTALLER_NO_MODIFY_PROFILE" ]; then
         if [ -w "$fn" ]; then
             if ! grep -q "$p" "$fn"; then
                 echo "modifying $fn..." >&2
-                echo "if [ -e $p ]; then . $p; fi # added by Nix installer" >> "$fn"
+                echo -e "\nif [ -e $p ]; then . $p; fi # added by Nix installer" >> "$fn"
             fi
             added=1
             break

shell.nix

@@ -0,0 +1,3 @@
+(import (fetchTarball https://github.com/edolstra/flake-compat/archive/master.tar.gz) {
+  src = ./.;
+}).shellNix

src/libexpr/flake/flakeref.cc

@@ -102,56 +102,61 @@ std::pair<FlakeRef, std::string> parseFlakeRefWithFragment(
             percentDecode(std::string(match[6])));
     }
 
-    /* Check if 'url' is a path (either absolute or relative to
-       'baseDir'). If so, search upward to the root of the repo
-       (i.e. the directory containing .git). */
-
     else if (std::regex_match(url, match, pathUrlRegex)) {
         std::string path = match[1];
-        if (!baseDir && !hasPrefix(path, "/"))
-            throw BadURL("flake reference '%s' is not an absolute path", url);
-        path = absPath(path, baseDir, true);
+        std::string fragment = percentDecode(std::string(match[3]));
 
-        if (!S_ISDIR(lstat(path).st_mode))
-            throw BadURL("path '%s' is not a flake (because it's not a directory)", path);
+        if (baseDir) {
+            /* Check if 'url' is a path (either absolute or relative
+               to 'baseDir'). If so, search upward to the root of the
+               repo (i.e. the directory containing .git). */
 
-        if (!allowMissing && !pathExists(path + "/flake.nix"))
-            throw BadURL("path '%s' is not a flake (because it doesn't contain a 'flake.nix' file)", path);
+            path = absPath(path, baseDir, true);
 
-        auto fragment = percentDecode(std::string(match[3]));
+            if (!S_ISDIR(lstat(path).st_mode))
+                throw BadURL("path '%s' is not a flake (because it's not a directory)", path);
 
-        auto flakeRoot = path;
-        std::string subdir;
+            if (!allowMissing && !pathExists(path + "/flake.nix"))
+                throw BadURL("path '%s' is not a flake (because it doesn't contain a 'flake.nix' file)", path);
 
-        while (flakeRoot != "/") {
-            if (pathExists(flakeRoot + "/.git")) {
-                auto base = std::string("git+file://") + flakeRoot;
+            auto flakeRoot = path;
+            std::string subdir;
 
-                auto parsedURL = ParsedURL{
-                    .url = base, // FIXME
-                    .base = base,
-                    .scheme = "git+file",
-                    .authority = "",
-                    .path = flakeRoot,
-                    .query = decodeQuery(match[2]),
-                };
+            while (flakeRoot != "/") {
+                if (pathExists(flakeRoot + "/.git")) {
+                    auto base = std::string("git+file://") + flakeRoot;
 
-                if (subdir != "") {
-                    if (parsedURL.query.count("dir"))
-                        throw Error("flake URL '%s' has an inconsistent 'dir' parameter", url);
-                    parsedURL.query.insert_or_assign("dir", subdir);
+                    auto parsedURL = ParsedURL{
+                        .url = base, // FIXME
+                        .base = base,
+                        .scheme = "git+file",
+                        .authority = "",
+                        .path = flakeRoot,
+                        .query = decodeQuery(match[2]),
+                    };
+
+                    if (subdir != "") {
+                        if (parsedURL.query.count("dir"))
+                            throw Error("flake URL '%s' has an inconsistent 'dir' parameter", url);
+                        parsedURL.query.insert_or_assign("dir", subdir);
+                    }
+
+                    if (pathExists(flakeRoot + "/.git/shallow"))
+                        parsedURL.query.insert_or_assign("shallow", "1");
+
+                    return std::make_pair(
+                        FlakeRef(Input::fromURL(parsedURL), get(parsedURL.query, "dir").value_or("")),
+                        fragment);
                 }
 
-                if (pathExists(flakeRoot + "/.git/shallow"))
-                    parsedURL.query.insert_or_assign("shallow", "1");
-
-                return std::make_pair(
-                    FlakeRef(Input::fromURL(parsedURL), get(parsedURL.query, "dir").value_or("")),
-                    fragment);
+                subdir = std::string(baseNameOf(flakeRoot)) + (subdir.empty() ? "" : "/" + subdir);
+                flakeRoot = dirOf(flakeRoot);
             }
 
-            subdir = std::string(baseNameOf(flakeRoot)) + (subdir.empty() ? "" : "/" + subdir);
-            flakeRoot = dirOf(flakeRoot);
+        } else {
+            if (!hasPrefix(path, "/"))
+                throw BadURL("flake reference '%s' is not an absolute path", url);
+            path = canonPath(path);
         }
 
         fetchers::Attrs attrs;

src/libstore/daemon.cc

@@ -173,31 +173,6 @@ struct TunnelSource : BufferedSource
     }
 };
 
-/* If the NAR archive contains a single file at top-level, then save
-   the contents of the file to `s'.  Otherwise barf. */
-struct RetrieveRegularNARSink : ParseSink
-{
-    bool regular;
-    string s;
-
-    RetrieveRegularNARSink() : regular(true) { }
-
-    void createDirectory(const Path & path)
-    {
-        regular = false;
-    }
-
-    void receiveContents(unsigned char * data, unsigned int len)
-    {
-        s.append((const char *) data, len);
-    }
-
-    void createSymlink(const Path & path, const string & target)
-    {
-        regular = false;
-    }
-};
-
 struct ClientSettings
 {
     bool keepFailed;
@@ -391,9 +366,9 @@ static void performOp(TunnelLogger * logger, ref<Store> store,
         }
         HashType hashAlgo = parseHashType(s);
 
-        StringSink savedNAR;
-        TeeSource savedNARSource(from, savedNAR);
-        RetrieveRegularNARSink savedRegular;
+        StringSink saved;
+        TeeSource savedNARSource(from, saved);
+        RetrieveRegularNARSink savedRegular { saved };
 
         if (method == FileIngestionMethod::Recursive) {
             /* Get the entire NAR dump from the client and save it to
@@ -407,11 +382,7 @@ static void performOp(TunnelLogger * logger, ref<Store> store,
         logger->startWork();
         if (!savedRegular.regular) throw Error("regular file expected");
 
-        auto path = store->addToStoreFromDump(
-            method == FileIngestionMethod::Recursive ? *savedNAR.s : savedRegular.s,
-            baseName,
-            method,
-            hashAlgo);
+        auto path = store->addToStoreFromDump(*saved.s, baseName, method, hashAlgo);
         logger->stopWork();
 
         to << store->printStorePath(path);
@@ -727,15 +698,15 @@ static void performOp(TunnelLogger * logger, ref<Store> store,
         if (!trusted)
             info.ultimate = false;
 
-        std::string saved;
         std::unique_ptr<Source> source;
         if (GET_PROTOCOL_MINOR(clientVersion) >= 21)
             source = std::make_unique<TunnelSource>(from, to);
         else {
-            TeeParseSink tee(from);
-            parseDump(tee, tee.source);
-            saved = std::move(*tee.saved.s);
-            source = std::make_unique<StringSource>(saved);
+            StringSink saved;
+            TeeSource tee { from, saved };
+            ParseSink ether;
+            parseDump(ether, tee);
+            source = std::make_unique<StringSource>(std::move(*saved.s));
         }
 
         logger->startWork();

src/libstore/export-import.cc

@@ -60,8 +60,10 @@ StorePaths Store::importPaths(Source & source, CheckSigsFlag checkSigs)
         if (n != 1) throw Error("input doesn't look like something created by 'nix-store --export'");
 
         /* Extract the NAR from the source. */
-        TeeParseSink tee(source);
-        parseDump(tee, tee.source);
+        StringSink saved;
+        TeeSource tee { source, saved };
+        ParseSink ether;
+        parseDump(ether, tee);
 
         uint32_t magic = readInt(source);
         if (magic != exportMagic)
@@ -77,15 +79,15 @@ StorePaths Store::importPaths(Source & source, CheckSigsFlag checkSigs)
         if (deriver != "")
             info.deriver = parseStorePath(deriver);
 
-        info.narHash = hashString(htSHA256, *tee.saved.s);
-        info.narSize = tee.saved.s->size();
+        info.narHash = hashString(htSHA256, *saved.s);
+        info.narSize = saved.s->size();
 
         // Ignore optional legacy signature.
         if (readInt(source) == 1)
             readString(source);
 
         // Can't use underlying source, which would have been exhausted
-        auto source = StringSource { *tee.saved.s };
+        auto source = StringSource { *saved.s };
         addToStore(info, source, NoRepair, checkSigs);
 
         res.push_back(info.path);

src/libstore/store-api.cc

@@ -222,20 +222,73 @@ StorePath Store::computeStorePathForText(const string & name, const string & s,
 }
 
 
+/*
+The aim of this function is to compute in one pass the correct ValidPathInfo for
+the files that we are trying to add to the store. To accomplish that in one
+pass, given the different kind of inputs that we can take (normal nar archives,
+nar archives with non SHA-256 hashes, and flat files), we set up a net of sinks
+and aliases. Also, since the dataflow is obfuscated by this, we include here a
+graphviz diagram:
+
+digraph graphname {
+    node [shape=box]
+    fileSource -> narSink
+    narSink [style=dashed]
+    narSink -> unsualHashTee [style = dashed, label = "Recursive && !SHA-256"]
+    narSink -> narHashSink [style = dashed, label = "else"]
+    unsualHashTee -> narHashSink
+    unsualHashTee -> caHashSink
+    fileSource -> parseSink
+    parseSink [style=dashed]
+    parseSink-> fileSink [style = dashed, label = "Flat"]
+    parseSink -> blank [style = dashed, label = "Recursive"]
+    fileSink -> caHashSink
+}
+*/
 ValidPathInfo Store::addToStoreSlow(std::string_view name, const Path & srcPath,
     FileIngestionMethod method, HashType hashAlgo,
     std::optional<Hash> expectedCAHash)
 {
-    /* FIXME: inefficient: we're reading/hashing 'tmpFile' three
-       times. */
+    HashSink narHashSink { htSHA256 };
+    HashSink caHashSink { hashAlgo };
 
-    auto [narHash, narSize] = hashPath(htSHA256, srcPath);
+    /* Note that fileSink and unusualHashTee must be mutually exclusive, since
+       they both write to caHashSink. Note that that requisite is currently true
+       because the former is only used in the flat case. */
+    RetrieveRegularNARSink fileSink { caHashSink };
+    TeeSink unusualHashTee { narHashSink, caHashSink };
 
-    auto hash = method == FileIngestionMethod::Recursive
-        ? hashAlgo == htSHA256
-          ? narHash
-          : hashPath(hashAlgo, srcPath).first
-        : hashFile(hashAlgo, srcPath);
+    auto & narSink = method == FileIngestionMethod::Recursive && hashAlgo != htSHA256
+        ? static_cast<Sink &>(unusualHashTee)
+        : narHashSink;
+
+    /* Functionally, this means that fileSource will yield the content of
+       srcPath. The fact that we use scratchpadSink as a temporary buffer here
+       is an implementation detail. */
+    auto fileSource = sinkToSource([&](Sink & scratchpadSink) {
+        dumpPath(srcPath, scratchpadSink);
+    });
+
+    /* tapped provides the same data as fileSource, but we also write all the
+       information to narSink. */
+    TeeSource tapped { *fileSource, narSink };
+
+    ParseSink blank;
+    auto & parseSink = method == FileIngestionMethod::Flat
+        ? fileSink
+        : blank;
+
+    /* The information that flows from tapped (besides being replicated in
+       narSink), is now put in parseSink. */
+    parseDump(parseSink, tapped);
+
+    /* We extract the result of the computation from the sink by calling
+       finish. */
+    auto [narHash, narSize] = narHashSink.finish();
+
+    auto hash = method == FileIngestionMethod::Recursive && hashAlgo == htSHA256
+        ? narHash
+        : caHashSink.finish().first;
 
     if (expectedCAHash && expectedCAHash != hash)
         throw Error("hash mismatch for '%s'", srcPath);
@@ -246,8 +299,8 @@ ValidPathInfo Store::addToStoreSlow(std::string_view name, const Path & srcPath,
     info.ca = FixedOutputHash { .method = method, .hash = hash };
 
     if (!isValidPath(info.path)) {
-        auto source = sinkToSource([&](Sink & sink) {
-            dumpPath(srcPath, sink);
+        auto source = sinkToSource([&](Sink & scratchpadSink) {
+            dumpPath(srcPath, scratchpadSink);
         });
         addToStore(info, *source);
     }

src/libutil/archive.hh

@@ -63,12 +63,29 @@ struct ParseSink
     virtual void createSymlink(const Path & path, const string & target) { };
 };
 
-struct TeeParseSink : ParseSink
+/* If the NAR archive contains a single file at top-level, then save
+   the contents of the file to `s'.  Otherwise barf. */
+struct RetrieveRegularNARSink : ParseSink
 {
-    StringSink saved;
-    TeeSource source;
+    bool regular = true;
+    Sink & sink;
 
-    TeeParseSink(Source & source) : source(source, saved) { }
+    RetrieveRegularNARSink(Sink & sink) : sink(sink) { }
+
+    void createDirectory(const Path & path)
+    {
+        regular = false;
+    }
+
+    void receiveContents(unsigned char * data, unsigned int len)
+    {
+        sink(data, len);
+    }
+
+    void createSymlink(const Path & path, const string & target)
+    {
+        regular = false;
+    }
 };
 
 void parseDump(ParseSink & sink, Source & source);

src/nix/edit.cc

@@ -45,6 +45,7 @@ struct CmdEdit : InstallableCommand
 
         auto args = editorFor(pos);
 
+        restoreSignals();
         execvp(args.front().c_str(), stringsToCharPtrs(args).data());
 
         std::string command;

src/nix/profile.cc

@@ -395,7 +395,7 @@ struct CmdProfileInfo : virtual EvalCommand, virtual StoreCommand, MixDefaultPro
     }
 };
 
-struct CmdProfileDiffClosures : virtual EvalCommand, virtual StoreCommand, MixDefaultProfile
+struct CmdProfileDiffClosures : virtual StoreCommand, MixDefaultProfile
 {
     std::string description() override
     {

tests/flakes.sh

@@ -18,7 +18,6 @@ registry=$TEST_ROOT/registry.json
 flake1Dir=$TEST_ROOT/flake1
 flake2Dir=$TEST_ROOT/flake2
 flake3Dir=$TEST_ROOT/flake3
-flake4Dir=$TEST_ROOT/flake4
 flake5Dir=$TEST_ROOT/flake5
 flake6Dir=$TEST_ROOT/flake6
 flake7Dir=$TEST_ROOT/flake7
@@ -390,14 +389,12 @@ cat > $flake3Dir/flake.nix <<EOF
   };
 }
 EOF
-git -C $flake3Dir add flake.nix
+nix flake update $flake3Dir
+git -C $flake3Dir add flake.nix flake.lock
 git -C $flake3Dir commit -m 'Remove packages.xyzzy'
 git -C $flake3Dir checkout master
 
-# Test whether fuzzy-matching works for IsAlias
-(! nix build -o $TEST_ROOT/result flake4/removeXyzzy#xyzzy)
-
-# Test whether fuzzy-matching works for IsGit
+# Test whether fuzzy-matching works for registry entries.
 (! nix build -o $TEST_ROOT/result flake4/removeXyzzy#xyzzy)
 nix build -o $TEST_ROOT/result flake4/removeXyzzy#sth