From e026bc3b05ca45c2d855d0a38820034ab4ef3c4c Mon Sep 17 00:00:00 2001
From: Eelco Dolstra <edolstra@gmail.com>
Date: Wed, 1 Nov 2017 18:43:11 +0100
Subject: [PATCH] fetchMercurial: Don't fetch hashes we already have

---
 src/libexpr/primops/fetchMercurial.cc | 26 +++++++++++------
 src/libutil/util.cc                   | 41 +++++++++++++++++----------
 src/libutil/util.hh                   | 17 +++++++++++
 tests/fetchMercurial.sh               | 18 ++++++++----
 4 files changed, 74 insertions(+), 28 deletions(-)

diff --git a/src/libexpr/primops/fetchMercurial.cc b/src/libexpr/primops/fetchMercurial.cc
index 391ce45a0..e68d1a2a5 100644
--- a/src/libexpr/primops/fetchMercurial.cc
+++ b/src/libexpr/primops/fetchMercurial.cc
@@ -22,6 +22,8 @@ struct HgInfo
     uint64_t revCount = 0;
 };
 
+std::regex commitHashRegex("^[0-9a-fA-F]{40}$");
+
 HgInfo exportMercurial(ref<Store> store, const std::string & uri,
     std::string rev, const std::string & name)
 {
@@ -66,20 +68,28 @@ HgInfo exportMercurial(ref<Store> store, const std::string & uri,
     Path stampFile = fmt("%s/.hg/%s.stamp", cacheDir, hashString(htSHA512, rev).to_string(Base32, false));
 
     /* If we haven't pulled this repo less than ‘tarball-ttl’ seconds,
-       do so now. FIXME: don't do this if "rev" is a hash and we
-       fetched it previously */
+       do so now. */
     time_t now = time(0);
     struct stat st;
     if (stat(stampFile.c_str(), &st) != 0 ||
         st.st_mtime < now - settings.tarballTtl)
     {
-        Activity act(*logger, lvlTalkative, actUnknown, fmt("fetching Mercurial repository '%s'", uri));
+        /* Except that if this is a commit hash that we already have,
+           we don't have to pull again. */
+        if (!(std::regex_match(rev, commitHashRegex)
+                && pathExists(cacheDir)
+                && runProgram(
+                    RunOptions("hg", { "log", "-R", cacheDir, "-r", rev, "--template", "1" })
+                    .killStderr(true)).second == "1"))
+        {
+            Activity act(*logger, lvlTalkative, actUnknown, fmt("fetching Mercurial repository '%s'", uri));
 
-        if (pathExists(cacheDir)) {
-            runProgram("hg", true, { "pull", "-R", cacheDir, "--", uri });
-        } else {
-            createDirs(dirOf(cacheDir));
-            runProgram("hg", true, { "clone", "--noupdate", "--", uri, cacheDir });
+            if (pathExists(cacheDir)) {
+                runProgram("hg", true, { "pull", "-R", cacheDir, "--", uri });
+            } else {
+                createDirs(dirOf(cacheDir));
+                runProgram("hg", true, { "clone", "--noupdate", "--", uri, cacheDir });
+            }
         }
 
         writeFile(stampFile, "");
diff --git a/src/libutil/util.cc b/src/libutil/util.cc
index 9346d5dc4..f56153cd4 100644
--- a/src/libutil/util.cc
+++ b/src/libutil/util.cc
@@ -895,32 +895,46 @@ std::vector<char *> stringsToCharPtrs(const Strings & ss)
 
 string runProgram(Path program, bool searchPath, const Strings & args,
     const std::experimental::optional<std::string> & input)
+{
+    RunOptions opts(program, args);
+    opts.searchPath = searchPath;
+    opts.input = input;
+
+    auto res = runProgram(opts);
+
+    if (!statusOk(res.first))
+        throw ExecError(res.first, fmt("program '%1%' %2%", program, statusToString(res.first)));
+
+    return res.second;
+}
+
+std::pair<int, std::string> runProgram(const RunOptions & options)
 {
     checkInterrupt();
 
     /* Create a pipe. */
     Pipe out, in;
     out.create();
-    if (input) in.create();
+    if (options.input) in.create();
 
     /* Fork. */
     Pid pid = startProcess([&]() {
         if (dup2(out.writeSide.get(), STDOUT_FILENO) == -1)
             throw SysError("dupping stdout");
-        if (input && dup2(in.readSide.get(), STDIN_FILENO) == -1)
+        if (options.input && dup2(in.readSide.get(), STDIN_FILENO) == -1)
             throw SysError("dupping stdin");
 
-        Strings args_(args);
-        args_.push_front(program);
+        Strings args_(options.args);
+        args_.push_front(options.program);
 
         restoreSignals();
 
-        if (searchPath)
-            execvp(program.c_str(), stringsToCharPtrs(args_).data());
+        if (options.searchPath)
+            execvp(options.program.c_str(), stringsToCharPtrs(args_).data());
         else
-            execv(program.c_str(), stringsToCharPtrs(args_).data());
+            execv(options.program.c_str(), stringsToCharPtrs(args_).data());
 
-        throw SysError(format("executing '%1%'") % program);
+        throw SysError("executing '%1%'", options.program);
     });
 
     out.writeSide = -1;
@@ -935,11 +949,11 @@ string runProgram(Path program, bool searchPath, const Strings & args,
     });
 
 
-    if (input) {
+    if (options.input) {
         in.readSide = -1;
         writerThread = std::thread([&]() {
             try {
-                writeFull(in.writeSide.get(), *input);
+                writeFull(in.writeSide.get(), *options.input);
                 promise.set_value();
             } catch (...) {
                 promise.set_exception(std::current_exception());
@@ -952,14 +966,11 @@ string runProgram(Path program, bool searchPath, const Strings & args,
 
     /* Wait for the child to finish. */
     int status = pid.wait();
-    if (!statusOk(status))
-        throw ExecError(status, format("program '%1%' %2%")
-            % program % statusToString(status));
 
     /* Wait for the writer thread to finish. */
-    if (input) promise.get_future().get();
+    if (options.input) promise.get_future().get();
 
-    return result;
+    return {status, result};
 }
 
 
diff --git a/src/libutil/util.hh b/src/libutil/util.hh
index 63a93f2ca..a3494e09b 100644
--- a/src/libutil/util.hh
+++ b/src/libutil/util.hh
@@ -245,6 +245,23 @@ string runProgram(Path program, bool searchPath = false,
     const Strings & args = Strings(),
     const std::experimental::optional<std::string> & input = {});
 
+struct RunOptions
+{
+    Path program;
+    bool searchPath = true;
+    Strings args;
+    std::experimental::optional<std::string> input;
+    bool _killStderr = false;
+
+    RunOptions(const Path & program, const Strings & args)
+        : program(program), args(args) { };
+
+    RunOptions & killStderr(bool v) { _killStderr = true; return *this; }
+};
+
+std::pair<int, std::string> runProgram(const RunOptions & options);
+
+
 class ExecError : public Error
 {
 public:
diff --git a/tests/fetchMercurial.sh b/tests/fetchMercurial.sh
index 5e4782658..83bf1331a 100644
--- a/tests/fetchMercurial.sh
+++ b/tests/fetchMercurial.sh
@@ -9,7 +9,7 @@ clearStore
 
 repo=$TEST_ROOT/hg
 
-rm -rfv $repo ${repo}-tmp $TEST_HOME/.cache/nix/hg
+rm -rf $repo ${repo}-tmp $TEST_HOME/.cache/nix/hg
 
 hg init $repo
 echo '[ui]' >> $repo/.hg/hgrc
@@ -24,13 +24,14 @@ echo world > $repo/hello
 hg commit --cwd $repo -m 'Bla2'
 rev2=$(hg log --cwd $repo -r tip --template '{node}')
 
-hg log --cwd $repo
-
-hg log --cwd $repo -r tip --template '{node}\n'
-
+# Fetch the default branch.
 path=$(nix eval --raw "(builtins.fetchMercurial file://$repo).outPath")
 [[ $(cat $path/hello) = world ]]
 
+# Fetch using an explicit revision hash.
+path2=$(nix eval --raw "(builtins.fetchMercurial { url = file://$repo; rev = \"$rev2\"; }).outPath")
+[[ $path = $path2 ]]
+
 # Fetch again. This should be cached.
 mv $repo ${repo}-tmp
 path2=$(nix eval --raw "(builtins.fetchMercurial file://$repo).outPath")
@@ -43,6 +44,13 @@ path2=$(nix eval --raw "(builtins.fetchMercurial file://$repo).outPath")
 # But with TTL 0, it should fail.
 (! nix eval --tarball-ttl 0 --raw "(builtins.fetchMercurial file://$repo)")
 
+# Fetching with a explicit hash should succeed.
+path2=$(nix eval --tarball-ttl 0 --raw "(builtins.fetchMercurial { url = file://$repo; rev = \"$rev2\"; }).outPath")
+[[ $path = $path2 ]]
+
+path2=$(nix eval --tarball-ttl 0 --raw "(builtins.fetchMercurial { url = file://$repo; rev = \"$rev1\"; }).outPath")
+[[ $(cat $path2/hello) = utrecht ]]
+
 mv ${repo}-tmp $repo
 
 # Using a clean working tree should produce the same result.