Split `OptionalPathSetting` from `PathSetting`

Rather than doing `allowEmpty` as boolean, have separate types and use
`std::optional`. This makes it harder to forget the possibility of an
empty path.

The `build-hook` setting was categorized as a `PathSetting`, but
actually it was split into arguments. No good! Now, it is
`Setting<Strings>` which actually reflects what it means and how it is
used.

Because of the subtyping, we now also have support for
`Setting<std::optional<String>>` in general. I imagine this can be used
to clean up many more settings also.

6 files changed, 23 insertions, 18 deletions

diff --git a/src/libstore/build/hook-instance.cc b/src/libstore/build/hook-instance.cc
index 075ad554f..337c60bd4 100644
--- a/src/libstore/build/hook-instance.cc
+++ b/src/libstore/build/hook-instance.cc
@@ -5,14 +5,14 @@ namespace nix {
 
 HookInstance::HookInstance()
 {
-    debug("starting build hook '%s'", settings.buildHook);
+    debug("starting build hook '%s'", concatStringsSep(" ", settings.buildHook.get()));
 
-    auto buildHookArgs = tokenizeString<std::list<std::string>>(settings.buildHook.get());
+    auto buildHookArgs = settings.buildHook.get();
 
     if (buildHookArgs.empty())
         throw Error("'build-hook' setting is empty");
 
-    auto buildHook = buildHookArgs.front();
+    auto buildHook = canonPath(buildHookArgs.front());
     buildHookArgs.pop_front();
 
     Strings args;
diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc
index 7f87cdf55..cf44779b7 100644
--- a/src/libstore/build/local-derivation-goal.cc
+++ b/src/libstore/build/local-derivation-goal.cc
@@ -65,8 +65,9 @@ void handleDiffHook(
     const Path & tryA, const Path & tryB,
     const Path & drvPath, const Path & tmpDir)
 {
-    auto diffHook = settings.diffHook;
-    if (diffHook != "" && settings.runDiffHook) {
+    auto & diffHookOpt = settings.diffHook.get();
+    if (diffHookOpt && settings.runDiffHook) {
+        auto & diffHook = *diffHookOpt;
         try {
             auto diffRes = runProgram(RunOptions {
                 .program = diffHook,
@@ -1428,7 +1429,8 @@ void LocalDerivationGoal::startDaemon()
     Store::Params params;
     params["path-info-cache-size"] = "0";
     params["store"] = worker.store.storeDir;
-    params["root"] = getLocalStore().rootDir;
+    if (auto & optRoot = getLocalStore().rootDir.get())
+        params["root"] = *optRoot;
     params["state"] = "/no-such-path";
     params["log"] = "/no-such-path";
     auto store = make_ref<RestrictedStore>(params,
diff --git a/src/libstore/globals.cc b/src/libstore/globals.cc
index d53377239..5a4cb1824 100644
--- a/src/libstore/globals.cc
+++ b/src/libstore/globals.cc
@@ -100,7 +100,10 @@ Settings::Settings()
     if (!pathExists(nixExePath)) {
         nixExePath = getSelfExe().value_or("nix");
     }
-    buildHook = nixExePath + " __build-remote";
+    buildHook = {
+        nixExePath,
+        "__build-remote",
+    };
 }
 
 void loadConfFile()
diff --git a/src/libstore/globals.hh b/src/libstore/globals.hh
index 820898350..b8dcf1f76 100644
--- a/src/libstore/globals.hh
+++ b/src/libstore/globals.hh
@@ -236,7 +236,7 @@ public:
         )",
         {"build-timeout"}};
 
-    PathSetting buildHook{this, true, "", "build-hook",
+    Setting<Strings> buildHook{this, {}, "build-hook",
         R"(
           The path to the helper program that executes remote builds.
 
@@ -556,8 +556,8 @@ public:
           line.
         )"};
 
-    PathSetting diffHook{
-        this, true, "", "diff-hook",
+    OptionalPathSetting diffHook{
+        this, std::nullopt, "diff-hook",
         R"(
           Absolute path to an executable capable of diffing build
           results. The hook is executed if `run-diff-hook` is true, and the
diff --git a/src/libstore/local-fs-store.hh b/src/libstore/local-fs-store.hh
index a03bb88f5..2ee2ef0c8 100644
--- a/src/libstore/local-fs-store.hh
+++ b/src/libstore/local-fs-store.hh
@@ -15,22 +15,22 @@ struct LocalFSStoreConfig : virtual StoreConfig
     // it to omit the call to the Setting constructor. Clang works fine
     // either way.
 
-    const PathSetting rootDir{(StoreConfig*) this, true, "",
+    const OptionalPathSetting rootDir{(StoreConfig*) this, std::nullopt,
         "root",
         "Directory prefixed to all other paths."};
 
-    const PathSetting stateDir{(StoreConfig*) this, false,
-        rootDir != "" ? rootDir + "/nix/var/nix" : settings.nixStateDir,
+    const PathSetting stateDir{(StoreConfig*) this,
+        rootDir.get() ? *rootDir.get() + "/nix/var/nix" : settings.nixStateDir,
         "state",
         "Directory where Nix will store state."};
 
-    const PathSetting logDir{(StoreConfig*) this, false,
-        rootDir != "" ? rootDir + "/nix/var/log/nix" : settings.nixLogDir,
+    const PathSetting logDir{(StoreConfig*) this,
+        rootDir.get() ? *rootDir.get() + "/nix/var/log/nix" : settings.nixLogDir,
         "log",
         "directory where Nix will store log files."};
 
-    const PathSetting realStoreDir{(StoreConfig*) this, false,
-        rootDir != "" ? rootDir + "/nix/store" : storeDir, "real",
+    const PathSetting realStoreDir{(StoreConfig*) this,
+        rootDir.get() ? *rootDir.get() + "/nix/store" : storeDir, "real",
         "Physical path of the Nix store."};
 };
 
diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh
index 2ecbe2708..14a862eef 100644
--- a/src/libstore/store-api.hh
+++ b/src/libstore/store-api.hh
@@ -114,7 +114,7 @@ struct StoreConfig : public Config
         return "";
     }
 
-    const PathSetting storeDir_{this, false, settings.nixStore,
+    const PathSetting storeDir_{this, settings.nixStore,
         "store",
         R"(
           Logical location of the Nix store, usually